Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 15, 2020, 10:58 UTC
Message-ID: <xmqqmtyf8hfm.fsf@gitster.c.googlers.com>
In-Reply-To: <5fd85811c3a6_d7c48208aa@natae.notmuch>

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 8 quoted lines

>> To avoid the ugly-looking "strcmp()" in the above, we may need to
>> adjust "--ff" (fast-forward without creating an extra merge commit)
>> and "--no-ff" (create an extra merge commit when the history could
>> be fast-forwarded) to imply "merge", though.
>> ...
>
> Or do you mean --ff doesn't override --rebase? Therefore it's more of an
> internal conceptual change.

I meant "--ff/--no-ff" without saying anything between merge and rebase would make merge implied. It was merely for the purpose of getting rid of !opt_ff in the condition there and such an implied merge would be one way to ensure that rebase_unspecified becomes false.

"--no-ff --rebase" (in any order) would be a nonsense combination, as it asks "please create an extra merge commit even when the history fast-forwards, but by the way I do not want merge I want rebase" [*1*]. It should error out when the history fast-forwards, I think, and it probably should also error out when the history does not fast-forward, instead of rebasing.

"--ff --rebase" (in any order), on the other hand, would be "if the history fast-forwards, just do so without creating an extra merge commit, by the way, I prefer rebase and not merge". If the history fast-forwards, it is OK to just fast-forward (using the merge backend if needed), as rebasing against the history that is ahead of us would mean all our unique development since we diverged from them, which is nothing, will be replayed on top of their history. If the history does not fast-forward, then the precondition of "--ff" part does not hold, so just rebasing as if "--ff" does not exist would probably be OK, too.

In any case, I haven't thought the opt_ff part of the expression through. During the review of v5 3/3 (and doing v7 5/5), what I was more interested in was to ensure the verbosity option, which would control how verbose a message we should be giving, can stay independent of other conditionals, which would control if we even need to error out and/or give hints.

Thanks.

[Footnote]

*1* Even though the current code may simply ignore --no-ff after the control is given to the rebase codepath,