Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

On Mon, Jul 12, 2021 at 11:51 AM Elijah Newren [off-list ref] wrote:

On Mon, Jul 12, 2021 at 10:08 AM Junio C Hamano [off-list ref] wrote:

quoted

Phillip Wood [off-list ref] writes:

quoted

Thanks for revising this patch, I like this approach much better. I do
however have some concerns about the interaction of pull.ff with the
rebase config and command line options. I'd naively expect the
following behavior (where rebase can fast-forward if possible)

  pull.ff  pull.rebase  commandline  action
   only     not false                rebase
   only     not false   --no-rebase  fast-forward only
    *       not false    --ff-only   fast-forward only
   only     not false    --ff        merge --ff
   only     not false    --no-ff     merge --no-ff
   only       false                  fast-forward only
   only       false      --rebase    rebase
   only       false      --ff        merge --ff
   only       false      --no-ff     merge --no-ff

Do you mean by "not false" something other than "true"?  Are you
trying to capture what should happen when these configuration
options are unspecified as well (and your "not false" is "either set
to true or unspecified")?  I ask because the first row does not make
any sense to me.  It seems to say

    "If pull.ff is set to 'only', pull.rebase is not set to 'false',
    and the command line does not say anything, we will rebase".

I think Phillip is trying to answer what to do when pull.ff and
pull.rebase conflict.  If I read his "not false" means "is set to
something other than false", then I agree with his table, but I think
he missed covering some cases.

I think his table says that pull.rebase=false cannot conflict with
pull.ff settings, but any other value for pull.rebase can.  That makes
sense to me.

I'd similarly say that pull.ff=true cannot conflict with any
pull.rebase settings...but that both pull.ff=only AND pull.ff=false
conflict with pull.rebase={true,merges}.

My opinion would be:
  * conflicting command line flags results in the last one winning.
  * --no-rebase makes pull.ff determine the action.
  * --ff makes pull.rebase determine the action.
  * any other command line flag (-r|--rebase|--no-ff|--ff-only)
overrides both pull.ff and pull.rebase
  * If no command line option is given, and pull.ff and pull.rebase
conflict, then error out.

I believe my recommendation above is consistent with every entry in
Phillip's table except the first line (where I suggest erroring out
instead).

I'm not sure that --no-ff should imply --no-rebase because `git
rebase` actually has a --no-ff option to rewrite commits even when
fast-forwarding is possible. And it's not really necessary to make
--ff-only imply --no-rebase because we're going to make `git pull`
handle --ff-only itself without invoking `git merge`. However, the
rest of this proposal could be implemented in a straightforward manner
by making --rebase on the command line imply --ff, and I think that
would be a fine solution.

-Alex