Re: [PATCH v3 02/16] pull: improve default warning

[Felipe Contreras <felipe.contreras@gmail.com>]

On Mon, Dec 7, 2020 at 2:55 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > We want to:
> >
> > 1. Be clear about what "specifying" means; merge, rebase, or
> >    fast-forward.
> > 2. Mention a direct shortcut for people that just want to get on with
> >    their lives: git pull --no-rebase.
> > 3. Have a quick reference for users to understand what this
> >    "fast-forward" business means.
> >
> > This patch does all three.
> >
> > Thanks to the previous patch now "git pull --help" explains what a
> > fast-forward is, and a further patch changes --no-rebase to --merge so
> > it's actually user friendly.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  builtin/pull.c | 25 +++++++++++++------------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index 1034372f8b..d71344fe28 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -345,18 +345,19 @@ static enum rebase_type config_get_rebase(void)
> >               return parse_config_rebase("pull.rebase", value, 1);
> >
> >       if (opt_verbosity >= 0 && !opt_ff) {
> > -             advise(_("Pulling without specifying how to reconcile divergent branches is\n"
> > -                      "discouraged. You can squelch this message by running one of the following\n"
> > -                      "commands sometime before your next pull:\n"
> > -                      "\n"
> > -                      "  git config pull.rebase false  # merge (the default strategy)\n"
> > -                      "  git config pull.rebase true   # rebase\n"
> > -                      "  git config pull.ff only       # fast-forward only\n"
> > -                      "\n"
> > -                      "You can replace \"git config\" with \"git config --global\" to set a default\n"
> > -                      "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
> > -                      "or --ff-only on the command line to override the configured default per\n"
> > -                      "invocation.\n"));
>
>
> > +             advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
> > +                     "you need to specify if you want a merge, a rebase, or a fast-forward.\n"
> > +                     "You can squelch this message by running one of the following commands:\n"
> > +                     "\n"
> > +                     "  git config pull.rebase false  # merge (the default strategy)\n"
> > +                     "  git config pull.rebase true   # rebase\n"
> > +                     "  git config pull.ff only       # fast-forward only\n"
> > +                     "\n"
> > +                     "You can replace \"git config\" with \"git config --global\" to set a default\n"
> > +                     "preference for all repositories.\n"
> > +                     "If unsure, run \"git pull --no-rebase\".\n"
> > +                     "Read \"git pull --help\" for more information."
> > +                     ));
> >       }
>
> It is an improvement to say what they can do from the command line
> in order to get out of the current situation, before giving them a
> configuration advice.
>
> But the exact instruction for the command line in the original seems
> to have been lost during the rewrite, which I think makes the "what
> to do now" advice less valuable than it could be.

In my opinion we should not dump the entire contents of the manpage in
a warning. The warning should be as brief as possible while giving
these:

1. A description of what's going on
2. A suggested way to move forward
3. A quick way to opt-out
4. A way to find more information

That's it.

Of course when it comes to succinctness other developers tend to
disagree with me, but that's my opinion.

> I personally think it is backwards to update this message before
> fixing the condition under which it triggers (I think by now
> everybody involved in the thread understands that we do not want to
> give this advise that does not stop to behave as-is---it should be
> made not to trigger if we know the history would fast-forward, and
> when it triggers it should stop the operation).  It may appear OK as
> long as we get the right end-state, but because this message must be
> rewritten when the triggering condition changes (i.e. when we stop
> giving this message when the history fast-forwards, there is no
> point in offering the fast-forward-only as a viable option), it
> seems to me a needless detour.

Except it's not necessary to rewrite it further, not in step 1.

Maybe it will be clearer when I send all the patches.

-- 
Felipe Contreras