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

On Sat, Apr 5, 2008 at 1:23 PM, Paul Burba <ptburba_at_gmail.com> wrote:
>
> 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.
> ]]]

Seems fine, but maybe your new comment could mention why the exception
is being made (a link to this thread even)?

--dave

-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org