Re: [PATCH 3/3] git-svn: Fix discarding of extra parents from svn:mergeinfo

From: Tuomas Suutari <tuomas.suutari@gmail.com>
Date: Feb 22, 2010, 18:04 UTC
Message-ID: <201002222004.55898.tuomas.suutari@gmail.com>
In-Reply-To: <201002221057.35088.trast@student.ethz.ch>

On Mon 2010-02-22 11:57:34 Thomas Rast wrote:

Show 5 quoted lines

> On Monday 22 February 2010 08:57:22 Tuomas Suutari wrote:
> > Use merge-base rather than rev-list for detecting if a parent is an
> > ancestor of another, because rev-list gives incorrect results
> > sometimes.
>

[...]

> 
> I think you swapped the test (or I got confused, which is entirely
> possible).

Ah, seems so. I was already writing a long reply explaining why rev-list can't be used but only merge-base just to realize that, indeed, rev-list can be used after all. The code just used to discard the wrong parent.

Show 15 quoted lines

> Let I = $new_parents[$i] and J = $new_parents[$j].  The
> old one was
> 
>   test -z "$(git rev-list -1 I..J)"
> 
> which reads "unless there are any commits on J which are not on I",
> i.e., it fails unless J is an ancestor of I.
>
> But the new one is
> 
>   "$(git merge-base I J)" == I.
> 
> so suddenly I must be an ancestor of J.
>
> Is that what you were fixing?

Yes.

> Because I don't think the 'rev-list -1'
> test is any worse than the merge-base test.

You're right. I failed to see that I can get the same results by swapping $i with $j in the undef($new_parents[$i]) statement.

My pitfall was that I considered only changing the "if ( !$revs )" part to "if ( $revs )" with the rev-list approach. That wouldn't have worked, because then the other test case, included in PATCH 2, would have failed. (When there are two distinct branches merged to one.)

> If it's not, please tell
> us what you are fixing.  Either way, please change the commit message
> appropriately.

Yes, the commit message was horrible. I was hoping that, by writing a test case as a "description" about the problem, I would avoid writing so much English... ;)

I will send another patch soon. Hopefully with better commit message.

-- 
Tuomas