Daniel Becroft wrote on Wed, Jan 19, 2011 at 07:27:15 +1000: > 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: > > > 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. >
Ah, if there's a different in the results between merge and reintegrate merge, then two tests are justifiable. (I missed that part of your original comment.) > > > 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. > As I said, I don't feel strongly about this. > > > + # 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. > OK. > > In short: +1 to commit, assuming you replace the asserts with something > > else per my earlier comment. > > 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? > Reply to this one. > Cheers, > Daniel B.
