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

From: Thomas Rast <trast@student.ethz.ch>
Date: Feb 23, 2010, 16:58 UTC
Message-ID: <201002231758.05297.trast@student.ethz.ch>
In-Reply-To: <1266862373-28365-1-git-send-email-tuomas.suutari@gmail.com>

On Monday 22 February 2010 19:12:53 Tuomas Suutari wrote:

Show 11 quoted lines

> If parent J is an ancestor of parent I, then parent J should be
> discarded, not I.
> 
> Note that J is an ancestor of I if and only if rev-list I..J is emtpy,
> which is what we are testing here.
> 
> Signed-off-by: Tuomas Suutari <tuomas.suutari@gmail.com>
> ---
> Thanks to Thomas Rast for pointing out that this can be made with a
> smaller change and there is no need swap rev-list to merge-base after
> all.

[...]

> -					undef($new_parents[$i]);
> +					undef($new_parents[$j]);

Just so this doesn't get lost...

I'm hesitating to give my "Ack", since I haven't looked into what the surrounding code does. I can't even see at a glance how the parent reduction relates to the commit that introduced it, which was

  commit 7a955a5365d9ebd5e12c12ed926b2b51b61c02ee
  Author: Sam Vilain <sam@vilain.net>
  Date:   Sun Dec 20 05:26:26 2009 +1300

      git-svn: detect cherry-picks correctly.

      The old function was incorrect; in some instances it marks a cherry picked
      range as a merged branch (because of an incorrect assumption that
      'rev-list COMMIT --not RANGE' would work).  This is replaced with a
      function which should detect them correctly, memoized to limit the expense
      of dealing with branches with many cherry picks to one 'merge-base' call
      per merge, per branch which used cherry picking.

      Signed-off-by: Sam Vilain <sam@vilain.net>
      Acked-by: Eric Wong <normalperson@yhbt.net>

That being said, you have clearly addressed the points I raised in my earlier mail. The loop, taken by itself, now throws out elements of the $new_parents list that are ancestors of another element which is a sane thing to do if you're building a merge.

So with the catch that I only looked at the immediate neighbourhood in the code:

  Acked-by: Thomas Rast <trast@student.ethz.ch>

-- 
Thomas Rast
trast@{inf,student}.ethz.ch