Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@xxxxxxxxx> writes:
>
> >> 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.
I see.
> "--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.
But we shoud not imply what the user didn't say.
Yes, "--no-ff --rebase" is obviously nonsense, but that's a
simplification of setups the user may have, for example:
git config pull.ff false
git pull --rebase
Here, I think the user is saying: "please do a rebase, and ignore the
pull.ff configuration".
But the other way would be:
git config pull.rebase true
git pull --no-ff
Following the same logic, the user is saying: "please do a
non-fast-forward [merge], and ignore the pull.rebase configuration".
Either we imply the merge, or we don't.
I don't think it makes sense for the code to imply the user did say
--merge, and therefore don't show the advice (or in the future error
out), but then continue as if the user did say --rebase.
> In any case, I haven't thought the opt_ff part of the expression
> through.
It is tricky.
The reason I think I see it more clearly is that I have explored many
options, and at this point I have 27 branches with different approaches
of this topic. I see the dead-ends because I literally have a branch
with the test-case failure on it.
But as I said in the other thread [1], it all boils down to this:
1. git -c pull.ff=only pull
2. git -c pull.ff=only pull --merge
Even if you remove the opt_ff part of the expression, and the logic of
the advice/error is flawless, opt_ff is still going to come bite us in
the end, because it's meant to be passed to cmd_merge(), and it's still
going to fail, just at a different point, with a different error message.
The best way to get rid of opt_ff from the expression, is to actually
completely ignore it, and leave the current behavior as it is.
By adding a different configuration, the logic becomes really simple:
if (!can_ff) {
if (!mode && opt_verbosity >= 0)
show_advice_pull_non_ff();
}
And then opt_ff doesn't interact with --rebase at all, just like it is
the case right now.
Simple.
Cheers.
[1] https://lore.kernel.org/git/5fd8213598c33_d7c4820837@natae.notmuch
--
Felipe Contreras
