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 :-(
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
Received on 2008-04-05 19:56:51 CEST