Hi Junio
On 05/07/2021 10:58, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
As I understand it the motivation for this change is to have 'git -c
pull.rebase=true pull --ff-only' actually fast forward. Why cant we
just change pull not to rebase in that case?
...
Is there a use for this outside of 'git pull --ff-only'? I'm far from
convinced we want this new option but if we do end up adding it I
think it should error out in combination with '-i' or '-x' as '-i'
implies the user wants to change the existing commits and '-x' can end
up changing them as well.
I think this patch addresses a valid problem but it seems to me that
the approach taken pushes complexity into rebase to handle a case when
pull does not need to invoke rebase in the first place.
I share the sentiment, but my conclusion would be different.
Even though we explain that "pull" is _like_ "fetch" followed by
"merge" (or "rebase"), at the conceptual level, "pull --ff-only"
should not have to invoke merge or rebase backend. If the branch
fast-fowards to the fetched tip, "pull" should be able to just
update the branch and check it out, and if it doesn't, "pull" should
just be able to fail.
Similarly, "pull --ff" (i.e. fast-forwading allowed) should be able
to just update the branch and check it out when the current tip of
the branch can be fast-forwarded to the fetched tip.
But as you said, pull.rebase=interactive will break such a clean
separation of concerns. You can leave "pull" oblivious of what
"rebase -i" wants to do when seeing a fast-forwardable history by
teaching "rebase" (and "merge") backend to take "--ff-only", "--ff",
and "--no-ff" options and having "pull" pass them through.
My main concern with the new rebase option was about a user invoking
'git rebase -i --ff-only' directly.
If a user has pull.rebase=interactive and runs 'git pull --ff-only' then
I'm not clear what they expect to happen. Assuming we can fast-forward
would they expect pull to run 'rebase -i' which would open their editor
with the todo list or would they expect that '--ff-only' means "I just
want to fast-forward, I don't want to run 'rebase -i'". If it is the
latter then we can just invoke 'git merge --ff-only' (so long as we
don't mind running the post-merge hook in this case) and not worry about
adding more complexity to 'git rebase'
The relevant section of the pull man page only talks about merging in
relation to --ff-only
--ff, --no-ff, --ff-only
Specifies how a merge is handled when the merged-in history
is already a descendant of the current history. --ff is the
default unless merging an annotated (and possibly signed)
tag that is not stored in its natural place in the
refs/tags/ hierarchy, in which case --no-ff is assumed.
With --ff, when possible resolve the merge as a fast-forward
(only update the branch pointer to match the merged branch;
do not create a merge commit). When not possible (when the
merged-in history is not a descendant of the current
history), create a merge commit.
With --no-ff, create a merge commit in all cases, even when
the merge could instead be resolved as a fast-forward.
With --ff-only, resolve the merge as a fast-forward when
possible. When not possible, refuse to merge and exit with a
non-zero status.
We can teach "pull" that certain backends (namely "rebase -i" in
this case) want to handle "ff logic" [*] themselves, and other
backends (i.e. "rebase" and "merge") do not mind if "pull" handles
"ff logic" for them, but that will break the abstraction (e.g. do we
really want to teach "pull" that "rebase -i" wants to rewrite all
the commits even when the history fast-forwards?) and will become a
source of duplicated logic.
Another thing to worry about are hooks. post-rebase
There is no post-rebase hook. There is the post-rewrite hook, I haven't
checked if we invoke it with no input of skip it entirely when we
fast-forward.
or post-merge
operations would want to be carried out even when the history would
fast-forward, and making "pull" to perform the fast-forwarding and
know which hooks should be called with what parameter so that we
could pretend as if the "merge" or "rebase" backend was indeed ran,
breaks the abstraction.
So, even though I wish that the world was simpler and we could
handle "ff logic" inside "pull", I am not sure if it is a realistic
wish.
I think if we decide that 'pull --ff-only' always implies merging then
the world stays fairly simple. On the other hand if we want to somehow
combine rebasing with --ff-only it will be more complicated. If we go
for the latter then unless someone comes up with a good use for 'rebase
--ff-only' in another context I would prefer the new rebase option to be
marked with PARSE_OPT_HIDDEN and that we also avoid making incidental
changes the existing rebase options.
Best Wishes
Phillip
[Footnote]
* By "ff logic", I am referring to what should happen in the 3x2=6
matrix when one of ("--ff", "--ff-only", or "--no-ff") is given
and the history (does or does not) fast-forward.
