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

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

Yesterday in IRC Julian described a problem he was having while doing
a catch up merge of the tree-conlficts branch with trunk.  The
attached file julianf.bad.catchip.merge.txt documents exactly what he
encountered -- in a nutshell he was getting a conflict on
'www/svn_1.5_releasenotes.html' which was entirely unexpected since
the only changes made to that file on the branch were via catch-up
merges from trunk.

The full mergeinfo the tree-conflicts branch @32461 is in the attached
file tree-conflicts.explicit.mergeinfo.at.r32461.txt.  To save you
some pain I'll sum it up: Other than the merge target itself, there
are 10 subtrees with explicit mergeinfo:

'.':
'subversion\include\private\svn_auth_private.h':
'notes\tree-conflicts\design-overview.txt':
'subversion\libsvn_auth_kwallet\kwallet.cpp':
'tools\server-side\test_svn_server_log_parse.py':
'tools\server-side\svn_server_log_parse.py':
'www\issue-tracker.html':
'www\development.html':
'notes\tree-conflicts\requirements.txt':
'www\tasks.html':
'subversion\tests\cmdline\tree_conflict_tests.txt':

Note: The root of the tree-conflicts branch has mergeinfo describing
some of it's natural history, specifically '/trunk:1-31822' (the tree
conflicts branch wasn't created until r28217, so 1-28216 describes the
branch's natural history).  This was probably due to issue #3157 which
has been fixed.  While having natural history in explicit mergeinfo is
not necessary, it should be harmless, so we can safely ignore it here.

Julian was trying to merge -r0:32172 into a tree-conflicts WC @32478.
At the start of the merge these are the ranges that require merging
for each subtree (this is simply the remaining_ranges field for each
child in the children_with_mergeinfo array):

.
31823-32172
notes/tree-conflicts/design-overview.txt
31498-31715,31823-32172
notes/tree-conflicts/requirements.txt
31498-31715,31823-32172
subversion/include/private/svn_auth_private.h
31498-31715,31823-32172
subversion/libsvn_auth_kwallet/kwallet.cpp
31498-31715,31823-32172
tools/server-side/svn_server_log_parse.py
31228-31497,31498-31715,31823-32172
tools/server-side/test_svn_server_log_parse.py
31228-31497,31498-31715,31823-32172
www/development.html
31823-32172
www/issue-tracker.html
31823-32172
www/tasks.html
31823-32172

Note that 'subversion\tests\cmdline\tree_conflict_tests.txt' is not in
this list as it was copied individually from trunk to the
tree-conflicts branch in r31225 then deleted from trunk in r31226 so
there is no merge source for this path.

Note also that the 31228-31497 and 31498-31715 ranges are contiguous,
I only broke them out to highlight which revisions are common to which
subtrees.  Since there are two contiguous ranges there are two editor
drives (i.e. calls to merge.c:drive_merge_report_editor()):

1) r31228-31715

2) r31823-32172

The first drive is where we get into trouble.  For that first editor drive we:

A) Call svn_client__get_diff_editor() - get a svn_delta_editor_t *diff
editor and edit baton with start revision 31227 and path '.' (i.e. the
merge target, the root of the tree-conflicts WC).

B) Call svn_ra_do_diff3() - using the diff editor and baton get a
svn_ra_reporter3_t *reporter and report baton

C) Make a reporter->set_path() call describing '.' at a revision such
that we don't repeat the merge of r31228-31715 into it.

The problem with the existing code is that in C) we use the start
range of '.'s remaining revisions, r31823, a revision outside of the
start revision we passed to svn_client__get_diff_editor() and the
revision we passed to svn_ra_do_diff3().

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.  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 :-\

Paul

P.S. The patch passes all tests.

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

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