You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2008/04/05 15:56:22 UTC
Reachable abort() in 'svn merge'.
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:
$ 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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reachable abort() in 'svn merge'.
Posted by Paul Burba <pt...@gmail.com>.
On Sat, Apr 5, 2008 at 4:43 PM, David Glasser <gl...@davidglasser.net> wrote:
>
> On Sat, Apr 5, 2008 at 1:23 PM, Paul Burba <pt...@gmail.com> wrote:
> >
> > On Sat, Apr 5, 2008 at 2:32 PM, David Glasser <gl...@davidglasser.net> wrote:
> > >
> > > On Sat, Apr 5, 2008 at 10:56 AM, Paul Burba <pt...@gmail.com> wrote:
> > > >
> > > > On Sat, Apr 5, 2008 at 1:48 PM, Paul Burba <pt...@gmail.com> wrote:
> > > > > On Sat, Apr 5, 2008 at 11:56 AM, Karl Fogel <kf...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reachable abort() in 'svn merge'.
Posted by David Glasser <gl...@davidglasser.net>.
On Sat, Apr 5, 2008 at 1:23 PM, Paul Burba <pt...@gmail.com> wrote:
>
> On Sat, Apr 5, 2008 at 2:32 PM, David Glasser <gl...@davidglasser.net> wrote:
> >
> > On Sat, Apr 5, 2008 at 10:56 AM, Paul Burba <pt...@gmail.com> wrote:
> > >
> > > On Sat, Apr 5, 2008 at 1:48 PM, Paul Burba <pt...@gmail.com> wrote:
> > > > On Sat, Apr 5, 2008 at 11:56 AM, Karl Fogel <kf...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reachable abort() in 'svn merge'.
Posted by Paul Burba <pt...@gmail.com>.
On Sat, Apr 5, 2008 at 2:32 PM, David Glasser <gl...@davidglasser.net> wrote:
>
> On Sat, Apr 5, 2008 at 10:56 AM, Paul Burba <pt...@gmail.com> wrote:
> >
> > On Sat, Apr 5, 2008 at 1:48 PM, Paul Burba <pt...@gmail.com> wrote:
> > > On Sat, Apr 5, 2008 at 11:56 AM, Karl Fogel <kf...@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.
]]]
Re: Reachable abort() in 'svn merge'.
Posted by David Glasser <gl...@davidglasser.net>.
On Sat, Apr 5, 2008 at 10:56 AM, Paul Burba <pt...@gmail.com> wrote:
>
> On Sat, Apr 5, 2008 at 1:48 PM, Paul Burba <pt...@gmail.com> wrote:
> > On Sat, Apr 5, 2008 at 11:56 AM, Karl Fogel <kf...@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@subversion.tigris.org
> > > For additional commands, e-mail: dev-help@subversion.tigris.org
> > >
> > >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reachable abort() in 'svn merge'.
Posted by Paul Burba <pt...@gmail.com>.
On Sat, Apr 5, 2008 at 1:48 PM, Paul Burba <pt...@gmail.com> wrote:
> On Sat, Apr 5, 2008 at 11:56 AM, Karl Fogel <kf...@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@subversion.tigris.org
> > For additional commands, e-mail: dev-help@subversion.tigris.org
> >
> >
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reachable abort() in 'svn merge'.
Posted by Paul Burba <pt...@gmail.com>.
On Sat, Apr 5, 2008 at 11:56 AM, Karl Fogel <kf...@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@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>