On Sat, Apr 5, 2008 at 4:43 PM, David Glasser <glasser_at_davidglasser.net> wrote:
>
> 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,
I expanded the comment and referenced this thread. Committed r30368
and nominated for backport.
Thanks again for taking a look at this.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-04-06 01:30:22 CEST