Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Felipe Contreras <felipe.contreras@gmail.com>]

On Wed, Dec 2, 2020 at 8:21 PM Junio C Hamano <gitster@pobox.com> wrote:

> So, the idea is that we currently return NULL when pull.ff is set,
> but now we also check "pull.rebase" etc. and give "--ff-only" when
> we did not choose --[no-]rebase and lack the configuration.  So the
> default (i.e. when pull.ff and pull.rebase are not set) would be as
> if the user said
>
>         git pull --ff-only --no-rebase
>
> I am not seeing what problem this tries to solve, exactly, though.
>
> I would have expected that for those who did not choose between
> merge and rebase (either with the pull.rebase configuration or from
> the command line --[no-]rebase option) the command would behave as
> if --ff-only is given, regardless of how pull.ff configuration is
> set.

That's right, I would have expected the same as well, but right now
the pull warning is disabled when you have a pull.ff configuration (or
--[no-]ff[-only]), as it was done in commit: 54200cef86 (pull: don't
warn if pull.ff has been set).

In my mind doing "git pull" is the equivalent of doing an implicit
"git pull --ff", so I don't see why setting "pull.ff=true" should
change the warning. The warning should not be about lack of
configuration (we want git to eventually work correctly without
configuration), it should be about something unexpected about to
happen (i.e. a non-fast-forward merge).

So quite likely Alex extended the logic of the current warning to the
default mode, which I don't think is right.

We want the logic of the warning to change: to be displayed a) only
when there's a non-fast-forward, b) when the user has not specified or
configured either a merge or a rebase.

Only after a while of this warning on the wild should the default mode
be changed.

But we need to fix the logic of the warning first.

> > @@ -344,23 +348,7 @@ static enum rebase_type config_get_rebase(void)
> >       if (!git_config_get_value("pull.rebase", &value))
> >               return parse_config_rebase("pull.rebase", value, 1);

...

> > -     return REBASE_FALSE;
> > +     return REBASE_INVALID;
> >  }
>
> Hmph, and who reacts to this REBASE_INVALID value?  Shouldn't there
> be something that catches this INVALID value and stop/complain?

Later on there's a "if (opt_rebase < 0) opt_rebase = REBASE_FALSE", so
this is only temporarily set as REBASE_INVALID for config_get_ff().

I don't find the semantics of this approach appealing, which is why I
introduced REBASE_DEFAULT in my patch series, so that the meaning of
REBASE_INVALID is not muddled.

> Another thing.  Does "git pull --rebase --ff-only" behave sensibly?
> "rebase our history on top of theirs only if we are truly ahead" is
> a bit strange way to say "we only ride along without having our own
> development, and once we discover we have our own stuff, stop us".
> IIRC, "git pull --rebase" ignored the "--ff-only" request and
> rebased our own stuff on top of their history anyway, which we would
> want to fix.

We would want this new mode to consistently fail with a good
user-friendly message, better than "fatal: Not possible to
fast-forward, aborting.".

But this mode is the opposite of "git pull --rebase", which is why it
doesn't make sense to do "git pull --rebase --ff-only" (which is
ignored right now).

In other words: --rebase should disable the --ff-only mode.

Also, we would want --no-rebase to disable the default --ff-only mode.
That would require changing the semantics of --ff-only, so that "git
pull --no-rebase --ff-only" doesn't make sense (as --ff-only is
overridden by --no-rebase).

If we do this, then the only time where --ff-only would make sense is
in "git pull --ff-only" (no --rebase or --no-rebase), and if we change
the semantics this way, then there's no need for my suggested
pull.mode (although it still might be desired).

Moreover, I see no tests that check for this new behavior.

Cheers.

-- 
Felipe Contreras