On Wed, Jan 19, 2011 at 3:08 AM, Daniel Shahaf <d.s_at_daniel.shahaf.name>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.
Received on 2011-01-18 22:28:13 CET