Re: [PATCH v3 02/16] pull: improve default warning
- From
Felipe Contreras <felipe.contreras@gmail.com>
- Date
- Dec 7, 2020, 22:29 UTC
- Message-ID
- <CAMP44s1aYqzCVvELH8zULaTkOdgLSSAQ0LE8WfgQKLPfU2MHfg@mail.gmail.com>
- In-Reply-To
- <xmqq4kkx9vzx.fsf@gitster.c.googlers.com>
On Mon, Dec 7, 2020 at 2:55 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 67 quoted lines
>
> 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.
Show 11 quoted lines
> 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