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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Dec 15, 2020, 12:22 UTC
Message-ID: <5fd8aa6a52e81_190cd7208c8@natae.notmuch>
In-Reply-To: <xmqqmtyf8hfm.fsf@gitster.c.googlers.com>

Junio C Hamano wrote:

Show 16 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> 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.

Show 6 quoted lines

> "--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