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 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").
--dave
> Paul
>
> >
> > > $ svn co -r30328 http://svn.collab.net/repos/svn/branches/1.5.x wc
> > > $ cd wc
> > > $ svn merge \
> > > http://svn.collab.net/repos/svn/branches/1.5.x-r30215 . --reintegrate
> > > [see abort happen]
> > > $
> > >
> > > It's in libsvn_client/merge.c:calculate_left_hand_side(), here:
> > >
> > > /* We only got here because we had mergeinfo for the source; if
> > > there were no segments, then our logic was wrong. */
> > > abort();
> > >
> > > In IRC just now, Paul made these comments
> > >
> > > <pburba> kfogel: It looks like this will occur anytime you copy A to
> > > B, then reintegrate B back to A without ever merging anything from A
> > > to B this will happen.
> > >
> > > <pburba> kfogel: B has no explicit mergeinfo from A, so once we
> > > 'filter mergeinfo_catalog so that all of the ranges come fromthe
> > > target's history' then all we are left with is empty mergeinfo. This
> > > is still sufficient to make have_mergeinfo_for_source TRUE though, so
> > > we get into that 'else if (!have_mergeinfo_for_descendants)' block
> > > but end up with a NULL rangelist and that gets us to the abort().
> > >
> > > It seems to me Paul is closer to fixing it than I am to finishing this
> > > mail, so I'm just going to post now for the record. Paul can refer to
> > > this thread in his commit, or just paste the repro recipe above into his
> > > log message.
> > >
> > > Below are some things I found poking around quickly in GDB, before he
> > > made his comments.
> > >
> > > (gdb) p ((svn_ra_neon__session_t *) (ra_session->priv))->url->data
> > > $4 = 0x818ea88 "https://svn.collab.net/repos/svn"
> > >
> > > So the session is rooted at the repository root, as it should be. Good.
> > >
> > > (gdb) p have_mergeinfo_for_source
> > > $5 = 1
> > > (gdb) p have_mergeinfo_for_descendants
> > > $6 = 0
> > > (gdb) p source_repos_rel_path
> > > $7 = 0x8288220 "branches/1.5.x-r30215"
> > > (gdb) p apr_hash_get(mergeinfo_catalog, source_repos_rel_path, -1)
> > > $8 = (void *) 0x8291c70
> > > (gdb) p apr_hash_count(mergeinfo_catalog)
> > > $9 = 1
> > >
> > > Notice how source_repos_rel_path doesn't start with slash, even though
> > > there's a TODO(reint) comment that says it should. Hmmm.
> > >
> > > And here we see that source_mergeinfo has count == 0:
> > >
> > > (gdb) p *source_mergeinfo
> > > $14 = {pool = 0x81d9738, array = 0x8291c98,
> > > iterator = {ht = 0x0, this = 0x8291d40, next = 0x82908d0,
> > > index = 136911976}, count = 0, max = 15,
> > > hash_func = 0xb7daefa0 <apr_hashfunc_default>, free = 0x0}
> > >
> > > Meanwhile, there are two segments:
> > >
> > > (gdb) p *segments
> > > $15 = {pool = 0x81d9738, elt_size = 4, nelts = 2, nalloc = 8,
> > > elts = 0x81d9788 <this part always looks like garbage>}
> > >
> > > So yeah, the 'return' inside the for-loop just before the abort() will
> > > never return, because
> > >
> > > rangelist = apr_hash_get(source_mergeinfo,
> > > apr_pstrcat(iterpool, "/", segment->path, NULL),
> > > APR_HASH_KEY_STRING);
> > >
> > > will never retrieve a non-NULL rangelist.
> > >
> > > -Karl
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
> > > For additional commands, e-mail: dev-help_at_subversion.tigris.org
> > >
> > >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: dev-help_at_subversion.tigris.org
>
>
--
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
Received on 2008-04-05 20:33:05 CEST