On 23/01/18 19:12, Junio C Hamano wrote:
Phillip Wood <phillip.wood@xxxxxxxxxxxx> 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.
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
