On Wed, Jan 19, 2011 at 3:08 AM, Daniel Shahaf <[email protected]>wrote:
> 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. > Ah, will do that in the future. Oops. > > 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. > I thought about that, but this situation fixes itself after the 'svn merge/svn commit' combination. However, after using --reintegrate. and committing, the executable bit is still not set, so I think there might be something more going on there. > 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.) > My logic for the placement was: it's exercising a problem with the svn merge process, so it belongs in merge_tests.py. Maybe that was wrong. > > 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. > Actually, the test passes if you only take out the assertion for the 'alpha_path' - the problem only arises when merging binary files, it works correctly for non-binary files. That's why I've got both in the test case. > > + # 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) > Thanks for the review, Daniel. I'll make these changes, and will send later tonight. Is it preferable to be in a new email, or in a reply to this one? Cheers, Daniel B.
