Re: Reachable abort() in 'svn merge'.

On Sat, Apr 5, 2008 at 2:32 PM, David Glasser <glasser_at_davidglasser.net> wrote:
>
> On Sat, Apr 5, 2008 at 10:56 AM, Paul Burba <ptburba_at_gmail.com> wrote:
> >
> > On Sat, Apr 5, 2008 at 1:48 PM, Paul Burba <ptburba_at_gmail.com> wrote:
> > > On Sat, Apr 5, 2008 at 11:56 AM, Karl Fogel <kfogel_at_red-bean.com> wrote:
> > > > Paul Burba found a way to reach an abort() from 'svn merge'. He posted
> > > > about it at http://rifers.org/paste/show/7044. Here is a self-contained
> > > > reproduction recipe based on that paste:
> > >
> > > I think I got it.
> > >
> > > If we:
> > >
> > > 1) Copy A to B
> > >
> > > 2) Make some changes on B that add explicit mergeinfo on B, e.g. a merge from C
> > >
> > > 3) Reintegrate B back to A
> > >
> > > Then:
> > >
> > > B has no explicit mergeinfo from A, only it's implicit
> > > mergeinfo/natural history as a result of being a copy of A, but B does
> > > have *some* explict mergeinfo from C.
> > >
> > > In merge.c:calculate_left_hand_side():
> > >
> > > a) Call svn_client__repos_location_segments() to get history for A.
> > >
> > > b) Call svn_ra_get_mergeinfo to get a mergeinfo catalog for B.
> > >
> > > c) Call remove_irrelevant_ranges() to filter the catalog from b) such
> > > that all the ranges come from the A's history. Inside
> > > remove_irrelevant_ranges() we ultimately use svn_mergeinfo_intersect()
> > > to find the intersection of a) and b), but there is no intersection.
> > > Problem is that svn_mergeinfo_intersect returns an *empty* hash which
> > > we then put into our filtered catalog. But an empty hash signifies
> > > empty mergeinfo not no mergeinfo.
> > >
> > > So it looks like the fix is as simple as the attached patch (running
> > > the tests right now).
> > >
> > > [[[
> > > Don't abort when reintegrating a branch that has no explicit mergeinfo from
> > > the merge target.
> > >
> > > See http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=136908
> > >
> > > * subversion/libsvn_client/merge.c
> > > (remove_irrelevant_ranges): Don't add empty mergeinfo to the filtered
> > > catalog when we really mean *no* mergeinfo.
> > > ]]]
> > >
> > > Running the tests right now...
> >
> > And
> >
> > XPASS: merge_tests.py 79: merge --reintegrate with renamed file on branch
> > XPASS: merge_tests.py 80: merge --reintegrate on a never-updated branch
> >
> > Somehow I doubt I fixed those with this patch :-(
>
> Yeah, um. I'd believe that the patch you did was correct in making
> #80 pass (it's a similar situation)

But not that similar, test 80 still has renames on the branch, so it
*should* still fail no?

> but it's more problematic for the
> descendants.
>
> The problematic case would be if the source itself has relevant ranges
> but a descendant has no relevant ranges; in that case, NULLing out the
> descendant's mergeinfo loses the key information that it might not
> have been up to date. However, if *everything* has no relevant ranges
> it's OK. So perhaps the new conditional you added should only trigger
> for the source itself (or maybe back in calculate_left_hand_side there
> should be an explicit "if the source exists but is empty, delete it").

Thanks for the suggestions, I went the "trigger for the source itself"
only route.

How's this look? (it passes all merge_tests)

[[[
Don't abort when reintegrating a branch that has no explicit mergeinfo from
the merge target on the root of the branch.

See http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=136908

* subversion/libsvn_client/merge.c
 (remove_irrelevant_ranges): Don't add empty mergeinfo to the filtered
 catalog when we really mean *no* mergeinfo.
]]]

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org