Re: Small rerere in rebase regression

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 25, 2016, 05:38 UTC
Message-ID: <alpine.DEB.2.20.1605250710340.4449@virtualbox>
In-Reply-To: <5744B00D.4020006@kdbg.org>

Hi Hannes,

On Tue, 24 May 2016, Johannes Sixt wrote:

Show 10 quoted lines

> Am 24.05.2016 um 15:18 schrieb Johannes Schindelin:
> > Hannes, could you quickly test whether
> > https://github.com/dscho/git/tree/interactive-rebase calls rerere twice,
> > too? (Please call interactive rebase with the GIT_USE_REBASE_HELPER=true
> > to avoid running the interactive rebase twice.)
> >
> > I have a hunch that it does not call rerere twice, which would be a nice
> > bonus in that patch thicket
> 
> It prints the message only once:

Thanks for confirming.

> Could not apply fa62fea... mastergittest@master|REBASE-i 1/1:1008> 5~
> 
> (Take note of the missing LF at the end of the message.)

Oops. Good catch, thank you! I just fixed this:

	https://github.com/dscho/git/commit/0393d7bb2d

> Can this result be interpreted as another indication that the "git rerere"
> call in die_with_patch can be removed, or are the two git-rebase
> implementations too different to be comparable?

The code used by the rebase--helper is not *quite* the same as the entire cherry-pick code path. For one, we run the sequencer directly, without any of cherry-pick's option parsing.

Having said that, yes, we inherit sequencer's rerere() call that was part of cherry-pick originally ever since aa1a011 (Make cherry-pick use rerere for conflict resolution., 2008-08-10) (amd which was moved to builtin/revert.c in 81b50f3 (Move 'builtin-*' into a 'builtin/' subdirectory, 2010-02-22) and refactored out into the sequencer in 043a449 (sequencer: factor code out of revert builtin, 2012-01-11)).

I did notice that rerere() call when implementing rebase -i's functionality in the sequencer, and dropped the extra call in error_with_patch() (the libified version of rebase -i's die_with_patch function).

In short: yes, the explicit `git rerere` call can be dropped, that is essentially what I did in the rebase--helper branch.

Ciao, Dscho