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

[Junio C Hamano <gitster@pobox.com>]

Elijah Newren <newren@gmail.com> writes:

> Have I missed some subtlety here?  This whole email appears to me to
> be arguing against a strawman.  Reading Junio's other emails in this
> thread[1][2], it's pretty clear he thinks the current behavior is
> buggy and suggests how it should be changed.  From what I can tell,
> you appear to be arguing against doing nothing and against only
> accepting perfection, neither of which were positions I saw anyone
> take.  In fact, the positions you argue for at length appear to
> exactly match the ones he took[1][2].  What am I missing?
>
> [1] https://lore.kernel.org/git/xmqq360h8286.fsf@gitster.c.googlers.com/
> [2] https://lore.kernel.org/git/xmqqlfe99yvy.fsf@gitster.c.googlers.com/

I tend to agree that the endgame state I want is pretty similar to
what Felipe wants.  The seeming confusion is probably due to what
exactly I mean by "default" is different from what he means.

I view the proposed "for unconfigured users, pull dies, and tells
them to choose between rebase or merge before it can continue, when
faced with a non-ff history" as a safe fallback behaviour until the
users make their choice.

It is a safe fallback to disable the more dangerous half of the
command until the user gives enough information to the command to do
its job without damaging the resulting history; it is not something
the users would actively want to choose.  

And that is what I meant by the default behaviour.

And when we stop in such a manner, it is sensible to give an error
message telling them 

 - why we are stopping,

 - what they can do to move the immediate situation forward
   (i.e. command line option that lets them choose), and

 - what they can do to make their choice permanent so that they
   would never see the command stop when facing a non-ff history
   (i.e. the configuration variables).

Up to this point, I think both of us agree with the above.

We start to differ after this point.  I would want to see only
"rebase" and "merge in the "choice" in the above list.  Felipe, if I
understand correctly, wants to add the third one, "ff-only", which
means the more dangerous half of "pull" is disabled by default.

I do not want to include that choice, as it would mean the more
familiar pull.rebase=yes/no would no longer work, and we'd need to
introduce a new variable, like pull.mode=ff-only.  It would mean for
those who use "pull" to consolidate histories on their side and
other people's side, whether they use rebase or merge, would now
have two ways to do the same thing (i.e. pull.rebase=no or
pull.mode=merge).  That is just making the end user experience more
complex than necessary.

Without introducing pull.mode, the only thing the users cannot do,
as far as I can tell, is to explicitly ask for the behaviour of an
unconfigured user (i.e. error out when faced with non-ff history)
without being told about the way to use configuration variable to
permanently record their choice.  Other than that, the existing
pull.rebase=yes/no is perfectly adequate.  Felipe seems to think
otherwise and wants not just the "safe fallback behaviour", but can
explicitly be configured using pull.mode=ff-only (and if that is not
what Felipe thinks, perhaps we are already in agreement without
realizing it).

Thinking about it again, I guess pull.rebase=yes/no plus a new
advise entry can easily give what Felipe seems to want.  We teach
"git pull", when it is not told to rebase or merge (either via the
command line --[no-]rebase or via pull.rebase configuration) and
when it sees a non-ff history, to

 - error out with a message telling the user that we are stopping,
   because the two histories needs consolidating to conclude this
   pull, but the two available ways would give vastly different
   result.

 - check the advice.pullNonFF configuration (as usual, it defaults
   to true) and if true, further tell the user that

    (1) they can either run "git pull --rebase" or "git pull
        --no-rebase",

    (2) they can configure their choice permanently with
        pull.rebase=yes/no, and

    (3) they can leave "pull" in the half-disabled state but stop
        seeing this message by setting advice.pullNonFF to false.

The last part (i.e. advice.pullnonFF) is the only new thing I said
in the various discussions on this topic (simply because I just came
up with the idea).  Everything else I've already said it elsewhere.

I dunno.