Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Elijah Newren <newren@gmail.com>]

On Thu, Jul 15, 2021 at 10:33 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > diff --git a/Documentation/config/pull.txt b/Documentation/config/pull.txt
> > index 54048306095..e70ed99e408 100644
> > --- a/Documentation/config/pull.txt
> > +++ b/Documentation/config/pull.txt
> > @@ -7,12 +7,13 @@ pull.ff::
> >       line). When set to `only`, only such fast-forward merges are
> >       allowed (equivalent to giving the `--ff-only` option from the
> >       command line). This setting overrides `merge.ff` when pulling.
> > +     Incompatible with pull.rebase.
>
> This update may mean well, but I sense that the description needs to
> be tightened up a bit more.  Unless you mean to say that I am not
> allowed to say "[pull] rebase=no" when I set "[pull] ff", that is.
>
> Or do you mean pull.ff=only is incompatible with any setting of
> pull.rebase?
>
> Or do you mean pull.ff=only is incompatible with any setting of
> pull.rebase other than false?
>
> Or do you mean any setting of pull.ff is imcompatible with any
> setting of pull.rebase other than false?
>
> (You do not have to answer---the above questions just demonstrate
> that the description is unnecessarily loose).

Fair enough.

> >  pull.rebase::
> >       When true, rebase branches on top of the fetched branch, instead
> >       of merging the default branch from the default remote when "git
> >       pull" is run. See "branch.<name>.rebase" for setting this on a
> > -     per-branch basis.
> > +     per-branch basis.  Incompatible with pull.ff.
>
> Likewise.
>
> I think it technically is OK to say "only ff update is allowed" or
> "ff update is allowed when able" while saying pull.rebase=yes.
>
>  - For those who say ff=only, the actual value of pull.rebase would
>    not matter (i.e. the integration immediately finishes by updating
>    to what was obtained from the upstream because there is no new
>    development on our own on top of theirs to either replay or
>    merge).
>
>  - For those who merely allow ff, an update may fast-forward even
>    when pull.rebase is set to true (or any non-false value), while
>    we'll replay our own development on top of theirs when their
>    history is not descendent of ours.
>
> So I do not see need to declare these combinations "incompatible".
> But perhaps I am missing some subtle cases?

Let me ask two questions:

1. When is it beneficial for users to set both pull.ff and pull.rebase?
2. Is it harmful to users for us to allow both to be set when we will
just ignore one?

I believe the answer to (1) is "never", and the answer to (2) is "yes".

For the second question in particular, I can think of two example cases:

2a) Users start with pull.ff=only, perhaps suggested by someone else
and left in their config for a long time.  When users hit a case that
can't fast-forward and they either ask for help or find a post on
stack overflow that suggests setting pull.rebase=true, they do so and
then get no warning that the setting they just added is being ignored.

2b) Perhaps users start with pull.rebase=true (suggested by a
colleague and forgot about it as they are more of a tester than a
developer and thus usually only see fast-forwards).  Then at some
point they need to function as an integrator, and they read the docs
and determine that pull.ff=false should do what they want to create
merge commits.  But then they get shocked that they've rebased public
commits (and perhaps also pushed them out) when they wanted merges.
Our docs have pretty clearly stated that pull.ff=false and --no-ff
create merges.

Additionally, pull.rebase=interactive seems like a case that interacts
poorly with pull.ff.  It may be that if you disagree that pull.rebase
and pull.ff are more generally incompatible, then we may still need to
micro-document individual incompatibilities.  (And does that list grow
and get confusing?)

> > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> > index 5c3fb67c014..03e8694e146 100644
> > --- a/Documentation/git-pull.txt
> > +++ b/Documentation/git-pull.txt
> > @@ -121,6 +121,9 @@ When false, merge the current branch into the upstream branch.
> >  +
> >  When `interactive`, enable the interactive mode of rebase.
> >  +
> > +Note that these flags are incompatible with --no-ff and --ff-only; if
> > +such incompatible flags are given, the last one will take precedence.
> > ++
>
> I am not sure about these, either.  Again, --ff-only (whether it is
> combined with --rebase or --rebase=no) would mean that the operation
> would fail when we have our own development on top of their history,
> and we repoint the tip of our history to theirs when we do not.
>
> We see "--no-ff" is explained to "always create an unnecessary extra
> merge", bit I am not sure it is the right mental model to apply when
> the user prefers rebasing.  The merge workflow is to treat our
> history as the primary and merging theirs in, so when theirs is a
> descendant (i.e. we have no new development of our own), some people
> may want to mark "we were there before we updated from them" with an
> extra merge.  Without --no-ff, the resulting history would be quite
> different in the merge workflow if you have or do not have your own
> development.  But the rebase workflow is to treat their history as
> the primary and replay our own development on top of theirs, and the
> shape of the resulting history would be the same whether you have
> your own development on top of theirs.  We start from their tip and
> then replay our own development on top (which could be an empty
> set), and there is nothing "--no-ff" would need to do differently to
> keep the resulting history similar to cases where we do have
> something of our own.  IOW, "--no-ff" becoming a no-op in a "rebase"
> workflow is a natural consequence, and there is no strong reason to
> say it is incompatible.

I think you're overlooking a variety of ways this could harm the user.

But first let me check if I understand you correctly:  I think you're
suggesting that
  * if --ff-only is given along with any --rebase[=*] flags, then we
silently ignore the --rebase[=*] flags.
  * if --no-ff is given along with any --rebase flag other than
--rebase=false, then we silently ignore --no-ff.

Let me ask the same two questions as for the config:  Does it benefit
the users to allow them to specify both flags?  Does it hurt some
users, especially if undocumented that one of the flags will be
ignored?

I think the same usecases from the config settings apply, though the
fact that both flags are present simultaneously on the command line at
least makes it less problematic in the case of --ff-only (users will
more quickly discover why their --rebase=<whatever> flag was ignored).
But there's an additional reason why treating --no-ff as a no-op is
harmful when --rebase is present:

git rebase has a --no-ff flag (a synonym for --force-rebase).  Some
users may intend for git pull --rebase --no-ff to run `git fetch
origin && git rebase --no-ff FETCH_HEAD`.  The fact that --no-ff will
be ignored and sometimes results in a fast-forward when the user
explicitly requested no fast forward seems buggy.

I thought this was the biggest hole in my proposal: there is both a
git merge --no-ff and a git rebase --no-ff, and they avoid
fast-forwards in different ways with different results.  It _might_ be
beneficial to allow both to be triggered directly from the pull
command.  However, allowing both has some backward compatibility
problems because (1) --no-ff is documented as making a merge, and (2)
people could have pull.rebase=true set and forget about it and then
get surprised when they add --no-ff to the command line and get a
rebase --no-ff (or even a plain rebase without --no-ff) rather than
the documented merge --no-ff.  I think the split config/parameters of
git pull make it really hard for us to determine whether users intend
to get a merge --no-ff or rebase --no-ff.

I think silently ignoring and not even documenting these pitfalls is
problematic.  We should at least document them.