From: Thomas Rast <trast@student.ethz.ch>
Date: Tue, 23 Feb 2010 16:58:04 GMT
Subject: Re: [PATCH 3/3 v2] git-svn: Fix discarding of extra parents from svn:mergeinfo
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:
> 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