Re: [PATCH v2 02/14] pull: improve default warning
- From
Elijah Newren <newren@gmail.com>
- Date
- Dec 5, 2020, 00:56 UTC
- Message-ID
- <CABPp-BFwWBLdFPsKi3o9hznFtAeWWfhNAyuymPS4BhMAHpnSfw@mail.gmail.com>
- In-Reply-To
- <CAMP44s1eTc4+tbULbyz5ENgbcN4tOVBA3Z-4GS4yMpciUD_1Hw@mail.gmail.com>
Hi Felipe,
On Fri, Dec 4, 2020 at 4:12 PM Felipe Contreras <felipe.contreras@gmail.com> wrote:
Show 62 quoted lines
>
> On Fri, Dec 4, 2020 at 5:00 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> > >
> > > We want to:
> > >
> > > 1. Be clear about what "specifying" means; merge or rebase.
> > > 2. Mention a direct shortcut for people that just want to get on with
> > > their lives: git pull --no-rebase.
> >
> > This is a shortcut for what?
>
> git config --global pull.rebase false
> git pull
>
> It's a shorter way of saying: "do a 'git pull' like you've always done
> but don't warn me".
>
> > > 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 | 23 ++++++++++++-----------
> > > 1 file changed, 12 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/builtin/pull.c b/builtin/pull.c
> > > index 1034372f8b..22a9ffcade 100644
> > > --- a/builtin/pull.c
> > > +++ b/builtin/pull.c
> > > @@ -346,17 +346,18 @@ static enum rebase_type config_get_rebase(void)
> > >
> > > 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"));
> > > + "discouraged; you need to specify if you want a merge, or a rebase.\n"
> >
> > ...want a merge, a rebase, or neither.
>
> There is no "git pull --no-merge". Years ago some people argued for a
> "pull.mode=none" (essentially making "git pull" the same as "git
> fetch"). But right now there's no option to do that.
>
> There's an option to do --ff-only, but that's still a merge.I disagree. I'm well aware that checkout_fast_forward() (which is what is ultimately called to do the fast-forwarding) is in a file called merge.c, but that doesn't make it a merge. I don't believe it was anything more than a convenient place to dump some extra code at the time.
> Perhaps: a merge, a rebase, or a fast-forward?
Sure, that works; in fact, that's much better than my suggestion. I like it.
Show 31 quoted lines
> > > + "You can squelch this message by running one of the following commands:\n" > > > + "\n" > > > + " git config pull.rebase false # merge (the default strategy)\n" > > > > Should this be labelled as the default given the desire to make > > --ff-only the default? Perhaps I'm jumping ahead and you plan to > > change that later in this series. > > That's right. > > In the previous series which does indeed make "pull.mode=ff-only" the > default [1], I do change the warning to specify the future default > [2], but in that series the warnings is changed to: > > The pull was not fast-forward, in the future you will have to choose > a merge, or a rebase. > To squelch this message and maintain the current behavior, use: > > git config --global pull.mode merge > > To squelch this message and adopt the new behavior now, use: > > git config --global push.mode ff-only > > Falling back to old style for now (merge). > Read "git pull --help" for more information. > > Since that series didn't get any traction, I decided to only implement > step 1: fix the current situation. And later on another series would > do step 2: introduce "pull.mode=ff-only" and do the preparations to > make it the default.
I like this longer plan. However, on the shorter scale plan...I think the suggestion to use "git pull --no-rebase" makes the current situation worse rather than fixing it.
Show 29 quoted lines
> > > + " 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" > > > > Good up to here. > > > > > + "If unsure, run \"git pull --no-rebase\".\n" > > > > Why is that safe to suggest? The original text may not have been the > > easiest to parse, but this seems more problematic to me. > > Because "git pull" has been doing the same as "git pull --no-rebase" > for more than a decade. It's safe for people to continue with this > behavior for a few more months. > > Some people need to get things done today, and they are not interested > in future changes, nor changing their default configuration, or what > the warning has to say. > > They just want "git pull" to do the same as yesterday, and the year > before, without being bothered with an annoying warning. > > Those people can start training their fingers to do "git pull > --merge", and learn the problems with "git pull" later. > > We want to respect the user's time, and not force them to read the > warning today.
The warning was added because sending users down paths that break things badly is a waste of user's time, and often a much bigger waste of user's time than making them read up on the meaning behind the two different choices of what kind of changes they can make. I agree the warning went too far, but I fully agree with the original folks who put this warning here that we need to have it for at least some cases and that there is a decision to be made. Though I am just one voice, and perhaps others will agree with you on your point here, I'll continue to disagree with blindly suggesting "git pull --no-rebase".
Show 5 quoted lines
> > > + "Read \"git pull --help\" for more information." > > > > Nice addition. > > Especially since now it explains what a fast-forward is.
Indeed. :-)