Daniel Becroft wrote on Tue, Jan 18, 2011 at 07:13:12 +1000:
> Hi guys,
>
> I was looking through the "Getting Involved" section of the Subversion
> website, and decided to start by writing a regression test for an
> issue, starting with 3686 (executable flag not correctly set on
> merge). This issue was already assigned to someone, so apologies if
> it's already being worked on.
>
Welcome aboard :-).
As a note for next time, though, when an issue is assigned to someone,
it would be better to ask them (or dev@) whether they're working on it
before starting to work on it yourself. I'm CCing Blair now for this
reason.
> I've attached the patch, and included it below as well. A few questions I
> have:
>
> 1) I couldn't find a precedent for examining the OS's executable flag,
> so my approach might be less than ideal. Should I add a test to
> prop_tests.py to check that setting svn:executable also sets the
> executable bit?
No, the test you're just adding already does just that. Just copy the
two asserts above the "# commit r2" line to after the comment and you're
done :-)
> 2) The problem also exists under the '--reintegrate' scenario, even
> after a subsequent commit. Would it be better to include that case
> here, or move the entire test to the merge_reintegrate.py suite?
>
I'm not sure we need a separate test for the --reintegrate case;
it seems reasonable to assume it does (and will) share the 'set +x
permission' logic with normal merges.
As to location, if I wrote this I'd probably put it in prop_tests or
special_tests along with the other svn:executable tests. (That's
somewhat of a bikeshed, though, and I don't feel strongly about this.)
> Regards,
> Daniel Becroft
>
>
> [[[
> Add regression test for issue #3686.
>
> This issue involves the executable flag being lost during a merge of a
> binary file with the 'svn:executable' property set.
>
> * subversion/tests/cmdline/merge_tests.py
> (merge_change_to_file_with_executable(sbox)) New test case.
Drop the signature, and add a colon:
(merge_change_to_file_with_executable): New test case.
> ]]]
(you've attached the patch both inline and as an attachment, I'll review
only the latter)
> Index: subversion/tests/cmdline/merge_tests.py
> ===================================================================
> --- subversion/tests/cmdline/merge_tests.py (revision 1059656)
> +++ subversion/tests/cmdline/merge_tests.py (working copy)
> @@ -16316,6 +16316,78 @@
> "Subtree merge under working merge produced the wrong mergeinfo",
> '/A/C/nu:9', [], 'pg', SVN_PROP_MERGEINFO, nu_COPY_path)
>
> +
> +#----------------------------------------------------------------------
> +# Test for issue #3686 'executable flag not correctly set on merge'
> +# See http://subversion.tigris.org/issues/show_bug.cgi?id=3686
> +def merge_change_to_file_with_executable(sbox):
> + "executable flag is maintained during binary merge"
> +
> + # Scenario: When merging a change to a binary file with the
> 'svn:executable'
> + # property set, the file is not marked as 'executable'. After commit, the
> + # executable bit is set correctly.
> + sbox.build()
> + wc_dir = sbox.wc_dir
> + trunk_url = sbox.repo_url + '/A/B/E'
> +
> + alpha_path = os.path.join(wc_dir, "A", "B", "E", "alpha")
> + beta_path = os.path.join(wc_dir, "A", "B", "E", "beta")
> +
> + # Force one of the files to be a binary type
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'propset', 'svn:mime-type',
> 'application/octet-stream',
> + alpha_path)
> +
> + # Set the 'svn:executable' property on both files
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'propset', 'svn:executable', 'ON',
> + beta_path)
> +
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'propset', 'svn:executable', 'ON',
> + alpha_path)
> +
> + # Verify the executable bit has been set before committing
> + assert os.access(alpha_path, os.X_OK)
> + assert os.access(beta_path, os.X_OK)
> +
No. Please use 'assert' for bugs in the test itself, and something else
('raise svntest.Failure' is the most generic) for bugs in the code being
tested. Example:
struct test_case {
const char *input;
int *exit_code;
} test_cases[] = { ... };
for (i = 0; i < NUMBER_OF_TESTS; i++)
{
struct test_case t = test_cases[i];
SVN_ERR_ASSERT(t.exit_code);
SVN_TEST_ASSERT(system(t.input) == *t.exit_code);
}
> + # Commit change (r2)
> + sbox.simple_commit()
> +
> + # Create the branch
> + svntest.actions.run_and_verify_svn(None, None, [], 'cp',
> + trunk_url,
> + sbox.repo_url + '/branch',
> + '-m', "Creating the Branch")
> +
> + # Modify the files + commit (r3)
> + svntest.main.file_append(alpha_path, 'appended alpha text')
> + svntest.main.file_append(beta_path, 'appended beta text')
> + sbox.simple_commit()
> +
> + svntest.actions.run_and_verify_svn(None, None, [], 'switch',
> + sbox.repo_url + '/branch',
> + wc_dir)
A comment before this line would be useful, in my opinion.
> +
> + # Recalculate the paths
> + alpha_path = os.path.join(wc_dir, "alpha")
> + beta_path = os.path.join(wc_dir, "beta")
> +
> + # Merge the changes across
> + svntest.actions.run_and_verify_svn(None, None, [], 'merge',
> + trunk_url, wc_dir)
> +
> + # Verify the executable bit has been set
> + assert os.access(alpha_path, os.X_OK)
> + assert os.access(beta_path, os.X_OK)
> +
As expected, the test passes if you remove these two lines.
> + # Commit (r4)
> + sbox.simple_commit()
> +
> + # Verify the executable bit has been set
> + assert os.access(alpha_path, os.X_OK)
> + assert os.access(beta_path, os.X_OK)
> +
> ########################################################################
> # Run the tests
>
> @@ -16507,6 +16579,7 @@
> merge_with_os_deleted_subtrees,
> no_self_referential_or_nonexistent_inherited_mergeinfo,
> XFail(subtree_merges_inherit_invalid_working_mergeinfo),
> + XFail(SkipUnless(merge_change_to_file_with_executable,
> svntest.main.is_posix_os)),
> ]
>
> if __name__ == '__main__':
In short: +1 to commit, assuming you replace the asserts with something
else per my earlier comment.
(you aren't a committer yet, though, so please re-send the patch)
