Re: [PATCH]: Fix merge bug when subtrees need ranges the target doesn't

[Julian Foad <ju...@btopenworld.com>]

Paul Burba wrote:
[...]
> Looking at the documentation for svn_ra_reporter3_t and
> svn_ra_do_diff3 I confess it is not crystal clear exactly what we
> would expect this to do, but it would seem to make more sense to pass
> svn_ra_do_diff3() r31715.  Since this editor drive is merging
> r31228-31715 and '.' already has that range merged to it, it's only
> the subtrees we need to merge, so describing '.' as at r31715 seems to
> ok -- dont merge anything to the target in this drive.  The attached
> patch does just that and seems to solve the problems Julian was
> seeing.

Paul,

I've tried it out a few ways now. It seems to give the right result.

The output puzzles me, though. (Attached.) It never prints 'G' for
'merGed'; it always prints 'U' when it merges. Did you expect that?


>   Does my analysis seem correct?  The thing that bugs me is
> that it feels like it should be easy to write a new merge test to
> expose this problem, but I can't seem to do that, which makes we
> wonder if I got it all straight :-\

Uh...

- Julian

Re: [PATCH]: Fix merge bug when subtrees need ranges the target doesn't

[Julian Foad <ju...@btopenworld.com>]

On Mon, 2008-08-18 at 12:31 -0400, Paul Burba wrote:
> On Fri, Aug 15, 2008 at 6:21 AM, Julian Foad <ju...@btopenworld.com> wrote:
> > Paul Burba wrote:
> > [...]
> >> Looking at the documentation for svn_ra_reporter3_t and
> >> svn_ra_do_diff3 I confess it is not crystal clear exactly what we
> >> would expect this to do, but it would seem to make more sense to pass
> >> svn_ra_do_diff3() r31715.  Since this editor drive is merging
> >> r31228-31715 and '.' already has that range merged to it, it's only
> >> the subtrees we need to merge, so describing '.' as at r31715 seems to
> >> ok -- dont merge anything to the target in this drive.  The attached
> >> patch does just that and seems to solve the problems Julian was
> >> seeing.
> >
> > Paul,
> >
> > I've tried it out a few ways now. It seems to give the right result.
> >
> > The output puzzles me, though. (Attached.)
> 
> Looking at your ouput it doesn't seem you got any conflicts?  Or is it
> simply that when you are configured to use an external diff tool that
> there is no command line evidence of interactively resolved conflicts?

Yes - the latter. I use --diff3-cmd=kdiff3. (I have recently noticed
that there is a separate --merge-cmd option but am not using it.)

>  I get three conflicts when doing the same merge:
> 
>   Conflict discovered in 'notes/tree-conflicts/design-overview.txt'.
>   Conflict discovered in 'subversion/tests/cmdline/merge_tests.py'.
>   Conflict discovered in 'subversion/libsvn_auth_kwallet/kwallet.cpp'.

The second one, yes.
The first one... maybe. Kwallet, I don't think should conflict. I could
check, if it's important.

> > It never prints 'G' for
> > 'merGed'; it always prints 'U' when it merges. Did you expect that?
> 
> I'd expect 'G' only when merging (cleanly) into a path with local mods
> and 'U' otherwise, no?

I guess I don't know. It seems very odd for a whole merge to have no
"merGed" notifications.

How could Subversion know whether my diff3 program did a "clean" merge?
I think the answer is: it can't know, and therefore it always prints
'U'. But this seems to be a user-interface question independent of our
current topic.


> >>   Does my analysis seem correct?  The thing that bugs me is
> >> that it feels like it should be easy to write a new merge test to
> >> expose this problem, but I can't seem to do that, which makes we
> >> wonder if I got it all straight :-\
> >
> > Uh...
> 
> Heh, well it was worth a try to tempt you to look deeply into it :-)
> 
> While on a run this weekend I realized a simple way to replicate the
> problem you saw on the tree-conflicts branch using just a simple greek
> tree.  I added merge_tests.py 104 in r32522 (it was embarassingly
> simple in hindsight).  I feel confident now that this fix is correct
> and committed it in r32523.

Brilliant. Thanks.

- Julian



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH]: Fix merge bug when subtrees need ranges the target doesn't

[Paul Burba <pt...@gmail.com>]

On Fri, Aug 15, 2008 at 6:21 AM, Julian Foad <ju...@btopenworld.com> wrote:
> Paul Burba wrote:
> [...]
>> Looking at the documentation for svn_ra_reporter3_t and
>> svn_ra_do_diff3 I confess it is not crystal clear exactly what we
>> would expect this to do, but it would seem to make more sense to pass
>> svn_ra_do_diff3() r31715.  Since this editor drive is merging
>> r31228-31715 and '.' already has that range merged to it, it's only
>> the subtrees we need to merge, so describing '.' as at r31715 seems to
>> ok -- dont merge anything to the target in this drive.  The attached
>> patch does just that and seems to solve the problems Julian was
>> seeing.
>
> Paul,
>
> I've tried it out a few ways now. It seems to give the right result.
>
> The output puzzles me, though. (Attached.)

Looking at your ouput it doesn't seem you got any conflicts?  Or is it
simply that when you are configured to use an external diff tool that
there is no command line evidence of interactively resolved conflicts?
 I get three conflicts when doing the same merge:

  Conflict discovered in 'notes/tree-conflicts/design-overview.txt'.
  Conflict discovered in 'subversion/tests/cmdline/merge_tests.py'.
  Conflict discovered in 'subversion/libsvn_auth_kwallet/kwallet.cpp'.

> It never prints 'G' for
> 'merGed'; it always prints 'U' when it merges. Did you expect that?

I'd expect 'G' only when merging (cleanly) into a path with local mods
and 'U' otherwise, no?

>>   Does my analysis seem correct?  The thing that bugs me is
>> that it feels like it should be easy to write a new merge test to
>> expose this problem, but I can't seem to do that, which makes we
>> wonder if I got it all straight :-\
>
> Uh...

Heh, well it was worth a try to tempt you to look deeply into it :-)

While on a run this weekend I realized a simple way to replicate the
problem you saw on the tree-conflicts branch using just a simple greek
tree.  I added merge_tests.py 104 in r32522 (it was embarassingly
simple in hindsight).  I feel confident now that this fix is correct
and committed it in r32523.

Paul

Paul