Daniel Becroft wrote on Wed, Jan 19, 2011 at 07:27:15 +1000:
> 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:
> > > 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.
Received on 2011-01-18 22:40:21 CET