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

Re: [PATCH v2 02/14] pull: improve default warning

From
Felipe Contreras <felipe.contreras@gmail.com>
Date
Dec 7, 2020, 09:16 UTC
Message-ID
<CAMP44s2XFQoda_PMULWha-rj9HhNfEddO5fikmswk9=AWN4RCw@mail.gmail.com>
In-Reply-To
<xmqqeek2cc14.fsf@gitster.c.googlers.com>
On Mon, Dec 7, 2020 at 1:26 AM Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
Show 6 quoted lines
> >> Do you think the crew should disregard the passenger's volition and
> >> force him to pay attention to the safety demonstration?
>
> An example that is irrelevant.  Passengers who sit on the aisle side
> and clutter the floor with their luggages can pose public hazard,
> and it is quite sensible for crews to remove them from the plane.

That's a completely different example that mixes policy with regulations. Apples and oranges.

> A team member who runs "pull --[no-]rebase" pushes the result, which
> may be a new history in a wrong shape, back to the shared repository
> probably falls into a different category.  ... Or perhaps in the
> same public hazard category?

Yes, but if it's a public hazard or not is up to the maintainers of specific projects. Some projects are perfectly fine with those merges.

It's the maintainer of the project that should decide what is the best policy for the project, and instruct his/her contributors.

It's not up for individuals to decide what is a public hazard, and it's not up to Git either. Similarly; passengers can't decide what raises to the level of public hazzard, but neither can a flight attendant.

Show 8 quoted lines
> > Useful link there.  Based on his comments, we may want to make
> > --ff-only, --merge, and --rebase all be mutually exclusive and result
> > in an error message if more than one is specified at the command line.
>
> I hope "his comment" does not refer to what I said.  Redefining the
> semantics of --ff-only (but not --ff=no and friends) to make it
> mutually incompatible with merge/rebase was what Felipe wanted to
> do, not me.
That's not true.

What I want is to introduce a "pull.mode=ff-only", not change the semantics of --ff-only.

What I have been saying is that you can't make --ff-only the default without changing its semantics.

But since the patch series that introduced such pull.mode [1] was completely ignored, I decided to take a shot at making --ff-only work.

You said [2]:
Show 5 quoted lines
> > What we want to see can be done without such backward incompatible
> > changes, e.g. declaring the combination of "--ff-only" and
> > "--[no-]rebase" incompatible and/or the last one wins, I would say, and
> > I suspect Alex's RFC was an attempt to make the first step in that
> > direction.

But in all my attempts the desired result cannot be done without backwards incompatible changes. I'm just the messenger.

To be crystal clear: I don't want to change the semantics of --ff-only; I want to introduce a new "pull.mode=ff-only".

Show 6 quoted lines
> FWIW, I see the general direction as
>
>  - When the history you are pulling is not a descendant of what you
>    have on your current branch, you must show your preference
>    between merging their history into yours or rebasing your history
>    onto theirs.
Agreed.
Show 5 quoted lines
>  - In such a case, we error out, with an error message telling them
>    that they must tell us which one they want in this invocation
>    (e.g. with "--rebase" or "--no-rebase").  We further tell them
>    that pull.rebase can be used to record their preference, if they
>    almost always use the same.
Not quite. See below.
Show 7 quoted lines
>         Side note: I earlier said "see below" to refer to this.  I
>         think the message should offer what they can do right now
>         with command line option first, and then give an option to
>         configure.  IOW, the message quoted in the beginning of this
>         response gives information in a wrong order.  Also,
>         "discouraged" is misleading, isn't it?  When we give this
>         message, we refuse to proceed until the user specifies.

That's why I keep pointing out the end goal. In the end it's not one message, it's *two*:

You can see my suggestion for both messages in my patch series for pull.mode [1].

When the user has specified "pull.mode=ff-only", the command dies with:
  The pull was not fast-forward, please either merge or rebase.

When the user has not specified any mode (by default), the command would output a warning:

  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.

But in order to reach that point we first need to decide what will be the default mode, and how it will be configured (--ff-only or "pull.mode=ff-only").

In the meantime this particular series is not committing for any particular decision, and thus it's changing the warning message for the *current* situation in which the command does not fail, but only shows a warning. So today it's merely discouraged.

This particular patch is pretty far from the end goal. It's just patch #2 of part 1.

Show 8 quoted lines
>         Another thing to consider is that pull.ff=only (and
>         --ff-only) may be wrong choice to offer in the error message
>         quoted above, if one of the objectives is to reduce the
>         "annoying" message that tells the user that they must
>         choose.  By definition, we do not give the message when our
>         side do not have our own development, so "I could run 'pull
>         --ff-only'" is a unusable knowledge to those who see the
>         message and want to get out of the situation.

Those commands will squelch the warning, but won't change the underlying nature of the warning, which is that the user must choose to either merge or rebase.

The temporary warning will disappear, but the permanent error message will not:
  The pull was not fast-forward, please either merge or rebase.
Show 7 quoted lines
>  - But when the history being pulled is a descendant of the current
>    branch, and the user does not give us preference between merge
>    and rebase (either from the command line or with configuration),
>    there is no need to give such an error message---if you do not
>    have your own development, we can just fast-forward the current
>    branch to what we fetched from the other side.  This does not
>    happen with the current system, which is what we want to fix.
It's not an error message, it's a warning. But yes.
Show 7 quoted lines
>  - Also when the history from the other side is a descedant and the
>    user has preference, either to rebase or to merge, either from
>    the command line or with configuration, we can fast-forward the
>    current branch to their tip, but there are wrinkles:
>
>    . when --ff=no is given and the choice is to merge (not rebase),
>      we must create an extra merge commit.
--ff receives no arguments, it's --no-ff.
Yes.
>    . when the choice is to rebase, by definition, rebasing what you
>      have on top of theirs in this case is the same as fast-forwarding
>      the current branch to theirs, because "what you have" is an
>      empty set.
Agreed.
Show 8 quoted lines
>    . I am not sure how "pull --rebase --ff=no" should interact if we
>      don't have our own development.  Rebasing our work on top of
>      theirs is philosophically incompatible with marking where the
>      side branch begins and ends with an extra commit, so it either
>      should be rejected when the command line and configuration is
>      parsed (i.e. regardless of the shape of the history we have at
>      hand), or --ff=no gets silently ignored when --rebase is given.
>      I haven't thought this one through.

I thought of this instance as well. In my opinion such combination should fail. But it doesn't actually affect anything.

Just to put this series in context: it's only part 1; it does not introduce pull.mode, and it doesn't make --ff-only the default. There's one patch prefixed with "tentative" that changes the semantics of --ff-only to see how it could look like when making it the default, but in no way am I proposing that patch to be merged.

The series is entirely about improving the current situation, nothing contentious.

In part 2 I will introduce pull.mode, which is the solution I propose, and that would be basically rebasing [1] on top of this series (hopefully).

I would have hoped that by this point it would have been clear why --ff-only is not the way forward. But oh well, we'll see what happens.

Cheers.

[1] https://lore.kernel.org/git/20201125032938.786393-1-felipe.contreras@gmail.com/ [2] https://lore.kernel.org/git/xmqqpn3qfhps.fsf@gitster.c.googlers.com

-- 
Felipe Contreras
Previous: Junio C HamanoNext: Junio C Hamano
Message 22 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.