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

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...

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