Junio C Hamano wrote:

Felipe Contreras [off-list ref] writes:

quoted

quoted

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 (local)

-- 
Felipe Contreras