Re: [PATCH 3/8] sequencer: fast-forward merge commits, if possible

From: Phillip Wood <phillip.wood@talktalk.net>
Date: Jan 24, 2018, 10:32 UTC
Message-ID: <f1c5e283-d9c9-700d-0484-ec9c6bf76068@talktalk.net>
In-Reply-To: <xmqqr2qggzie.fsf@gitster.mtv.corp.google.com>

On 23/01/18 19:12, Junio C Hamano wrote:

Show 19 quoted lines

> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
>> On 18/01/18 15:35, Johannes Schindelin wrote:
>>>
>>> Just like with regular `pick` commands, if we are trying to recreate a
>>> merge commit, we now test whether the parents of said commit match HEAD
>>> and the commits to be merged, and fast-forward if possible.
>>>
>>> This is not only faster, but also avoids unnecessary proliferation of
>>> new objects.
>>
>> I might have missed something but shouldn't this be checking opts->allow_ff?
> 
> Because the whole point of this mechanism is to recreate the
> topology faithfully to the original, even if the original was a
> redundant merge (which has a side parent that is an ancestor or a
> descendant of the first parent), we should just point at the
> original merge when the condition allows it, regardless of
> opts->allow_ff.

I agree that the merge should be recreated, but I was thinking of something slightly different. Currently the sequencer uses opts->allow_ff to control whether a new commit with the same contents should be created even if the existing one could be reused. So I was querying whether we should recreate the commit when the user run 'git rebase --recreate-merges --no-ff' rather than just reusing it. As merges also have another meaning for fast-forward the terminology gets confusing.

Show 5 quoted lines

> I think it is a different matter if an insn to create a new merge
> (i.e. "merge - <parent> <message>", not "merge <commit> <parent>")
> should honor opts->allow_ff; because it is not about recreating an
> existing history but is a way to create what did not exist before,
> I think it is sensible if allow_ff option is honored.

This is the merge sense of 'fast-forward' not the existing sequencer sense, without thinking about it more I'm not sure if one command line option for rebase is sufficient to cover both uses.

Best Wishes

Phillip