Re: [PATCH v5 2/3] pull: move default warning

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Dec 14, 2020, 11:02 UTC
Message-ID: <5fd7466250a4f_d17f5208fc@natae.notmuch>
In-Reply-To: <xmqqpn3cep51.fsf@gitster.c.googlers.com>

Junio C Hamano wrote:

Show 39 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > On Fri, Dec 11, 2020 at 6:00 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> Felipe Contreras <felipe.contreras@gmail.com> writes:
> >
> >> > Should actually be something like:
> >> >
> >> >         if (rebase_unspecified && !can_ff)
> >> >                 die("Not a fast-forward; must either merge or rebase");
> >>
> >> The illustration I gave in the message you are responding to was
> >> made in the context of patch 2/3; with patch 3/3 where can_ff
> >> exists, it would not become like what you gave above.  It should
> >> instead become
> >>
> >>         if (rebase_unspecified && !opt_ff && !can_ff) {
> >>                 if (opt_verbosity >= 0 && advice_pull_non_ff)
> >>                         show_advice_pull_non_ff();
> >>                 die("not a fast-forward; must merge or rebase");
> >>         }
> >>
> >> i.e. when we can fast-forward, we do not trigger the "you must
> >> specify rebase/merge" message, and we do not trigger the "not a
> >> fast-forward" error.
> >
> > It's not the !can_ff part I'm trying to highlight, it's the lack of
> > advice *after* we have decided to flip the switch.
> >
> > As I said in another thread: I don't think we have any long
> > condescending error in any other command.
> 
> Only the "not a fast-forward and you must choose between merge and
> rebase" part (i.e. what I listed as the first part of three-part
> message) is the error.
> 
> The rest, the message that teaches how to choose between merge and
> rebase from command line and configuration (or how to choose
> permanently not to make the choice between the two), is not an
> error---it's called advice.

Right. After re-reading the 2013 discussion, and the GitHub trainers' input that Jeff King shared with us [1], it might make sense to eventually keep this advice (permanently), since many users might not know what a rebase is, and would not want to know at this point of their learning curve.

That being said, the advice still needs to be improved, and I have already yet another version taking this into consideration ready to be sent. Just waiting for feedback on the previous can_ff series.

> Even if we were to introduce the third choice (i.e. permanently not
> to choose between rebase or merge and instead just error out without
> getting advice), the advice must stay for those who didn't make any
> choice among the three (i.e. merge, rebase or ff-only).

With the introduction of such mode, and the permanent advice, we would have:

  if (!can_ff) {
	  if (!mode && opt_verbosity >= 0 && advice_pull_non_ff)
		  show_advice_pull_non_ff();

	  if (mode == PULL_MODE_FF_ONLY)
		  die(_("Not a fast-forward, either merge or rebase.\n"));
  }

If in the future we decide to make it the default, we would have:

  if (!can_ff) {
	  if (!mode && opt_verbosity >= 0 && advice_pull_non_ff)
		  show_advice_pull_non_ff();

	  if (!mode || mode == PULL_MODE_FF_ONLY)
		  die(_("Not a fast-forward, either merge or rebase.\n"));
  }

The transition is very straightforward.

> We have many of them.  An easy example to spot is a similar sized
> onefor "git checkout HEAD^0".  Neither that one or the one under
> discussion is particularly condescending.

I am not familiar with these "advices", since I basically ignore them, and they are not presented to me in any different color, or special presentation. I generally ignore textwalls in my command line.

But fine, if these walls of text already exist, perhaps it makes sense to have yet another permanent one (just with better text, and better options).

With an improved advice--which includes an option that is good enough we *can* make the default (pull.mode=ff-only)--perhaps that would be enough, and in a couple of years we can make the assessment if it's enough or not.

But clearly *today* we don't have such mode, which people have found missing for more than a decadte.

Cheers.

[1] https://lore.kernel.org/git/20130909201751.GA14437@sigill.intra.peff.net/

-- 
Felipe Contreras