git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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.  :-)
Previous: Felipe ContrerasNext: Felipe Contreras
Message 13 of 79 in “pull: default warning improvements”
  1. 00/14 pull: default warning improvementsFelipe Contreras, Dec 4, 2020
  2. 03/14 pull: refactor fast-forward checkFelipe Contreras, Dec 4, 2020
  3. 04/14 pull: cleanup autostash checkFelipe Contreras, Dec 4, 2020
  4. Elijah NewrenDec 4, 2020
  5. Felipe ContrerasDec 5, 2020
  6. Elijah NewrenDec 5, 2020
  7. 01/14 doc: pull: explain what is a fast-forwardFelipe Contreras, Dec 4, 2020
  8. Elijah NewrenDec 4, 2020
  9. Jacob KellerDec 5, 2020
  10. 02/14 pull: improve default warningFelipe Contreras, Dec 4, 2020
  11. Elijah NewrenDec 4, 2020
  12. Felipe ContrerasDec 5, 2020
  13. Elijah NewrenDec 5, 2020
  14. Felipe ContrerasDec 5, 2020
  15. Chris TorekDec 5, 2020
  16. Felipe ContrerasDec 5, 2020
  17. Elijah NewrenDec 5, 2020
  18. Felipe ContrerasDec 5, 2020
  19. Elijah NewrenDec 6, 2020
  20. Felipe ContrerasDec 6, 2020
  21. Junio C HamanoDec 7, 2020
  22. Felipe ContrerasDec 7, 2020
  23. Junio C HamanoDec 7, 2020
  24. Felipe ContrerasDec 7, 2020
  25. Junio C HamanoDec 7, 2020
  26. Felipe ContrerasDec 7, 2020
  27. Jacob KellerDec 7, 2020
  28. Junio C HamanoDec 8, 2020
  29. Felipe ContrerasDec 8, 2020
  30. Junio C HamanoDec 8, 2020
  31. Felipe ContrerasDec 9, 2020
  32. Elijah NewrenDec 9, 2020
  33. Felipe ContrerasDec 10, 2020
  34. Junio C HamanoDec 10, 2020
  35. Felipe ContrerasDec 10, 2020
  36. Felipe ContrerasDec 10, 2020
  37. Junio C HamanoDec 11, 2020
  38. Felipe ContrerasDec 11, 2020
  39. Junio C HamanoDec 14, 2020
  40. Felipe ContrerasDec 14, 2020
  41. Junio C HamanoDec 14, 2020
  42. Felipe ContrerasDec 15, 2020
  43. Jeff KingDec 15, 2020
  44. Felipe ContrerasDec 15, 2020
  45. Junio C HamanoDec 15, 2020
  46. Felipe ContrerasDec 15, 2020
  47. Felipe ContrerasDec 7, 2020
  48. 06/14 pull: move default warningFelipe Contreras, Dec 4, 2020
  49. Elijah NewrenDec 4, 2020
  50. Felipe ContrerasDec 4, 2020
  51. 05/14 pull: trivial cleanupFelipe Contreras, Dec 4, 2020
  52. Elijah NewrenDec 4, 2020
  53. Felipe ContrerasDec 5, 2020
  54. 08/14 pull: trivial whitespace style fixFelipe Contreras, Dec 4, 2020
  55. 10/14 pull: add proper error with --ff-onlyFelipe Contreras, Dec 4, 2020
  56. Elijah NewrenDec 4, 2020
  57. Felipe ContrerasDec 5, 2020
  58. 12/14 pull: show warning with --ffFelipe Contreras, Dec 4, 2020
  59. Elijah NewrenDec 4, 2020
  60. Felipe ContrerasDec 5, 2020
  61. 14/14 test: pull-options: revert unnecessary changesFelipe Contreras, Dec 4, 2020
  62. Elijah NewrenDec 4, 2020
  63. Felipe ContrerasDec 5, 2020
  64. 13/14 test: merge-pull-config: trivial cleanupFelipe Contreras, Dec 4, 2020
  65. Elijah NewrenDec 4, 2020
  66. 11/14 tentative: pull: change the semantics of --ff-onlyFelipe Contreras, Dec 4, 2020
  67. Elijah NewrenDec 4, 2020
  68. Felipe ContrerasDec 5, 2020
  69. experiment: pull: change --ff-only and default modeFelipe Contreras, Dec 5, 2020
  70. Elijah NewrenDec 5, 2020
  71. Felipe ContrerasDec 5, 2020
  72. Junio C HamanoDec 7, 2020
  73. Felipe ContrerasDec 5, 2020
  74. 07/14 pull: display default warning only when non-ffFelipe Contreras, Dec 4, 2020
  75. Elijah NewrenDec 4, 2020
  76. Felipe ContrerasDec 5, 2020
  77. 09/14 pull: introduce --merge optionFelipe Contreras, Dec 4, 2020
  78. Elijah NewrenDec 4, 2020
  79. Felipe ContrerasDec 5, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.