Re: [PATCH] rebase: Allow merge strategies to be used when rebasing

From: Eric Wong <normalperson@yhbt.net>
Date: Jun 19, 2006, 21:39 UTC
Message-ID: <20060619213951.GA6987@hand.yhbt.net>
In-Reply-To: <7vd5d63k7f.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:

Show 15 quoted lines

> Eric Wong <normalperson@yhbt.net> writes:
> 
> > This solves the problem of rebasing local commits against an
> > upstream that has renamed files.
> 
> I think leveraging the merge strategy to perform rebase is
> sound, but the selection of merge base for this purpose is quite
> different from the regular merge, and I think unfortunately this
> patch is probably wrong in letting git-merge choose the merge
> base.
> 
> But let's mention other things as well.
> 
>  - You kept the original "format-patch piped to am" workflow
>    optionally working.

I left it as the default, too. I figured that it's best not to change the default (and most likely faster) behavior of something people rely on.

Show 7 quoted lines

>  - You check if merge or patch was used for failed rebase and
>    follow the appropriate codepath while resuming, which is
>    good.
> 
>  - The list of commits you generate with tac seem to include
>    merge commit -- you may want to give --no-merges to
>    rev-list.

Good point, I'll change it.

>  - I do not think we use "tac" elsewhere -- is it portable
>    enough?

Nope. perl -e 'print reverse <>' is equivalent and we already use plenty of perl.

>  - Exiting with success unconditionally after "git am" feels
>    wrong.  I would do "exit $?" instead of "exit 0" there.

Oops, I'll change that, too.

Show 32 quoted lines

> Suppose you have this commit ancestry graph:
> 
> ----------------------------------------------------------------
> Example:       git-rebase --onto master A topic
> 
>         A---B---C topic                       B'--C' topic
>        /                   -->               /
>   D---E---F---G master          D---E---F---G master
> ----------------------------------------------------------------
> 
> This is slightly different from the one at the beginning of the
> script.  The idea is A turned out to be not so cool, and we
> would want to drop it.
> 
> > +call_merge () {
> > +	cmt="$(cat $dotest/`printf %0${prec}d $1`)"
> > +	echo "$cmt" > "$dotest/current"
> > +	git-merge $strategy_args "rebase-merge: $cmt" HEAD "$cmt" \
> > +			|| die "$MRESOLVEMSG"
> > +}
> 
> call_merge is first called with B in cmt, and HEAD is pointing
> at G.  But the merge in this function makes a merge between B
> and G, taking the effect of E->A.
> 
> I think the three-way merge you would want here is not between B
> and G using E as the pivot, but between B and G using A as the
> pivot.  That's how cherry-pick and revert works.  I would
> leverage the interface that is one level lower for this -- the
> strategy modules themselves.
> 
> 	git-merge-$strategy $cmt^ -- HEAD $cmt

Changing the 'git-merge $strategy_args "rebase-merge: $cmt" HEAD "$cmt"' line in call_merge() to this seems to have broken more tests.

I'm not an expert at merging strategies by any measure, I've just trusted merge-recursive to Do The Right Thing(TM) more often than not, and use rerere to avoid repeating work.

I'm not entirely sure I want (or fully understand how) to support cherry-picking/revert with this, as I've already dropped --skip when --merge was in use.

I'll think about this some more when I'm less distracted.

Show 9 quoted lines

> The strategy modules take merge base(s), double-dash as the
> separator, our head and the other head.  They do not make commit
> themselves (instead they leave working tree and index in
> committable state) and signal the results with their exit
> status:
> 
> 	0 -- success
>         1 -- conflicts
>         2 -- did not handle the merge at all

Cool, that's good.

-- 
Eric Wong