Pick the right default and stop warn on `git pull`

Pick the right default and stop warn on `git pull`

[Vít Ondruch]

[-- Attachment #1.1.1: Type: text/plain, Size: 769 bytes --]

Hi,

While I understand that the merge after `git pull` is probably not the 
best strategy (as explained in [1]), asking people to change their 
configuration to choose their default strategy with every `git pull` is 
not any better. Please select any strategy considered more appropriate 
and stop warning me. I know I could change my configuration, but I'll 
change it just if I want to differ from defaults. I don't need to be 
told that I am using wrong default. I'd like to keep my trust in Git 
upstream, but this is was not trustworthy decision IMO.

BTW, I was motivated to open this discussion by this [2] Fedora ticket.


Vít



[1] https://github.com/git/git/commit/d18c950a69

[2] https://bugzilla.redhat.com/show_bug.cgi?id=1900441


[-- Attachment #1.1.2: OpenPGP_0x0CE09EE79917B87C.asc --]
[-- Type: application/pgp-keys, Size: 61897 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: Pick the right default and stop warn on `git pull`

[Alex Henrie]

On Mon, Nov 23, 2020 at 8:14 AM Vít Ondruch <vondruch@redhat.com> wrote:
>
> Hi,
>
> While I understand that the merge after `git pull` is probably not the
> best strategy (as explained in [1]), asking people to change their
> configuration to choose their default strategy with every `git pull` is
> not any better. Please select any strategy considered more appropriate
> and stop warning me. I know I could change my configuration, but I'll
> change it just if I want to differ from defaults. I don't need to be
> told that I am using wrong default. I'd like to keep my trust in Git
> upstream, but this is was not trustworthy decision IMO.
>
> BTW, I was motivated to open this discussion by this [2] Fedora ticket.
>
>
> Vít
>
>
>
> [1] https://github.com/git/git/commit/d18c950a69
>
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1900441

Hi Vit, as the message explains, you can use `git config --global
pull.rebase false` to permanently silence the warning for all of your
repositories. The idea is that in a future version of Git, users will
not be allowed to pull at all without specifying a reconciliation
strategy. This will prevent users (novices in particular) from
accidentally creating messes by merging branches that should have been
rebased instead.

Personally, I would rather have pull.rebase be true by default (and
I'm sure many others would like that too), but there are so many users
who really do want to merge that it's not reasonable to make rebasing
the default. The most reasonable solution is to ask the user to be
explicit about what they want and not make assumptions.

-Alex

Re: Pick the right default and stop warn on `git pull`

[Theodore Y. Ts'o]

On Mon, Nov 23, 2020 at 10:59:13AM -0700, Alex Henrie wrote:
> 
> Hi Vit, as the message explains, you can use `git config --global
> pull.rebase false` to permanently silence the warning for all of your
> repositories. The idea is that in a future version of Git, users will
> not be allowed to pull at all without specifying a reconciliation
> strategy. This will prevent users (novices in particular) from
> accidentally creating messes by merging branches that should have been
> rebased instead.
> 
> Personally, I would rather have pull.rebase be true by default (and
> I'm sure many others would like that too), but there are so many users
> who really do want to merge that it's not reasonable to make rebasing
> the default. The most reasonable solution is to ask the user to be
> explicit about what they want and not make assumptions.

It's really going to depend on your workflow.  If your repository is
used as a building point by other repositories, and you pull from the
upstream repo, rebasing may cause a massive headache for your
downstream "sub-maintainers" (assuming that you are a subsystem
maintainer).

If your repository is effectively a leaf repo, then rebasing may be
harmless, although there are still who don't like rebasing because it
invalidates your previous testing.  My personal preference is to do a
git fetch, followed by a git merge --ff-only, and if that errors out,
then I know I need to take a bit more care before deciding what to do
next.

But everyone's workflow varies, which is why creating a default is
going to controversial no matter what the default might be.

Cheers,

					- Ted

Re: Pick the right default and stop warn on `git pull`

[Felipe Contreras]

On Mon, Nov 23, 2020 at 1:17 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:

> If your repository is effectively a leaf repo, then rebasing may be
> harmless, although there are still who don't like rebasing because it
> invalidates your previous testing.  My personal preference is to do a
> git fetch, followed by a git merge --ff-only, and if that errors out,
> then I know I need to take a bit more care before deciding what to do
> next.

Which is why I suggested to make fast-forward-only the default:

https://lore.kernel.org/git/1398988808-29678-1-git-send-email-felipe.contreras@gmail.com/

In what case would that default not be what most people want?

-- 
Felipe Contreras

Re: Pick the right default and stop warn on `git pull`

[Theodore Y. Ts'o]

On Mon, Nov 23, 2020 at 01:40:38PM -0600, Felipe Contreras wrote:
> On Mon, Nov 23, 2020 at 1:17 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
> 
> > If your repository is effectively a leaf repo, then rebasing may be
> > harmless, although there are still who don't like rebasing because it
> > invalidates your previous testing.  My personal preference is to do a
> > git fetch, followed by a git merge --ff-only, and if that errors out,
> > then I know I need to take a bit more care before deciding what to do
> > next.
> 
> Which is why I suggested to make fast-forward-only the default:
> 
> https://lore.kernel.org/git/1398988808-29678-1-git-send-email-felipe.contreras@gmail.com/
> 
> In what case would that default not be what most people want?

Well, it *was* the default, previously, IIRC.  The problem is that for
"simple" use cases, using rebases for git-pull is "simpler".  Well,
it's simpler until it does something super-surprising when the project
becomes more complex, but if the goal is to have a more gentle
learning curve for newbies, especially for small projects --- which
are the vast majority of projectds, on, say github and sourceforge ---
the case can be made.

So intead of having a huge discussion which is going to be very hard
to come to converge (much like the "main" vs "master" question),
requiring people to set their own global default or per-repo default
is a pretty good compromise.

						- Ted

Re: Pick the right default and stop warn on `git pull`

[Felipe Contreras]

On Mon, Nov 23, 2020 at 2:20 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Mon, Nov 23, 2020 at 01:40:38PM -0600, Felipe Contreras wrote:
> > On Mon, Nov 23, 2020 at 1:17 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
> >
> > > If your repository is effectively a leaf repo, then rebasing may be
> > > harmless, although there are still who don't like rebasing because it
> > > invalidates your previous testing.  My personal preference is to do a
> > > git fetch, followed by a git merge --ff-only, and if that errors out,
> > > then I know I need to take a bit more care before deciding what to do
> > > next.
> >
> > Which is why I suggested to make fast-forward-only the default:
> >
> > https://lore.kernel.org/git/1398988808-29678-1-git-send-email-felipe.contreras@gmail.com/
> >
> > In what case would that default not be what most people want?
>
> Well, it *was* the default, previously, IIRC.

There has never been a "pull.mode=ff-only" option; that's what I tried
to introduce.

> The problem is that for
> "simple" use cases, using rebases for git-pull is "simpler".  Well,
> it's simpler until it does something super-surprising when the project
> becomes more complex, but if the goal is to have a more gentle
> learning curve for newbies, especially for small projects --- which
> are the vast majority of projectds, on, say github and sourceforge ---
> the case can be made.

The people that want rebases can configure git pull to do rebases.

This issue is about the *unconfigured* default.

> So intead of having a huge discussion which is going to be very hard
> to come to converge (much like the "main" vs "master" question),
> requiring people to set their own global default or per-repo default
> is a pretty good compromise.

This discussion already happened in 2014, and the conclusion was that
doing fast-forward merge if possible, and fail otherwise was a good
default.

The problem is that the patches were never merged.

Cheers.

-- 
Felipe Contreras

Re: Pick the right default and stop warn on `git pull`

[Jeff King]

On Mon, Nov 23, 2020 at 02:34:18PM -0600, Felipe Contreras wrote:

> > > Which is why I suggested to make fast-forward-only the default:
> > >
> > > https://lore.kernel.org/git/1398988808-29678-1-git-send-email-felipe.contreras@gmail.com/
> > >
> > > In what case would that default not be what most people want?
> >
> > Well, it *was* the default, previously, IIRC.
> 
> There has never been a "pull.mode=ff-only" option; that's what I tried
> to introduce.

There is pull.ff=only these days, which also squelches the message. But
you're right that it was never the default.

The default has continued to be merging. If we were going to switch that
to bailing out and doing nothing with a warning, I agree that doing so
only in the non-fast-forward case would be much better (because in that
case, all three modes have the same result).

I don't recall whether there is any active plan to move away from the
merge default, though.

-Peff

Re: Pick the right default and stop warn on `git pull`

[Alex Henrie]

On Mon, Nov 23, 2020 at 2:48 PM Jeff King <peff@peff.net> wrote:
>
> On Mon, Nov 23, 2020 at 02:34:18PM -0600, Felipe Contreras wrote:
>
> > > > Which is why I suggested to make fast-forward-only the default:
> > > >
> > > > https://lore.kernel.org/git/1398988808-29678-1-git-send-email-felipe.contreras@gmail.com/
> > > >
> > > > In what case would that default not be what most people want?
> > >
> > > Well, it *was* the default, previously, IIRC.
> >
> > There has never been a "pull.mode=ff-only" option; that's what I tried
> > to introduce.
>
> There is pull.ff=only these days, which also squelches the message. But
> you're right that it was never the default.
>
> The default has continued to be merging. If we were going to switch that
> to bailing out and doing nothing with a warning, I agree that doing so
> only in the non-fast-forward case would be much better (because in that
> case, all three modes have the same result).

I'd be fine with making fast-forward-only the default. But then again,
some users like to set pull.ff=no, which creates a merge commit even
when fast-forwarding is possible. The existence of that workflow might
be a good reason to require the user to always be explicit.

> I don't recall whether there is any active plan to move away from the
> merge default, though.

There's no formal plan yet. I didn't want to propose anything until
people had gotten used to explicitly setting the reconciliation
strategy. However, it has been on my long-term radar ever since the
warning was added.

-Alex

Re: Pick the right default and stop warn on `git pull`

[Jeff King]

On Mon, Nov 23, 2020 at 03:03:07PM -0700, Alex Henrie wrote:

> > The default has continued to be merging. If we were going to switch that
> > to bailing out and doing nothing with a warning, I agree that doing so
> > only in the non-fast-forward case would be much better (because in that
> > case, all three modes have the same result).
> 
> I'd be fine with making fast-forward-only the default. But then again,
> some users like to set pull.ff=no, which creates a merge commit even
> when fast-forwarding is possible. The existence of that workflow might
> be a good reason to require the user to always be explicit.

Good point. I was thinking there was no option the users could set where
they'd be disappointed by a fast-forward, and for all other cases we'd
stop and ask the user to tell us more explicitly what they want. But
you're right that pull.ff=no is an exception there.

It might still be OK to allow it by default, though as we have always
allowed a fast-forward merge by default. So this would be a strict
tightening from the long-standing behavior of "if fast-forward, do that;
otherwise, merge" to "if fast-forward, do that; otherwise punt". Anybody
who wants pull.ff=no already had to configure it.

-Peff

Re: Pick the right default and stop warn on `git pull`

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't recall whether there is any active plan to move away from the
> merge default, though.

I don't, either, but if we were to do so, keeping the warning/advice
being discussed is crucial to avoid breakage.  

Hopefully by now most existing users who would have depended on
unconfigured default behaviour of merging have explicitly shown
their preference by setting the configuration to squelch the
message, so it might not be such a huge backward compatibility
nightmare.  I dunno.

Re: Pick the right default and stop warn on `git pull`

[Felipe Contreras]

On Mon, Nov 23, 2020 at 3:48 PM Jeff King <peff@peff.net> wrote:
> On Mon, Nov 23, 2020 at 02:34:18PM -0600, Felipe Contreras wrote:

> > There has never been a "pull.mode=ff-only" option; that's what I tried
> > to introduce.
>
> There is pull.ff=only these days, which also squelches the message. But
> you're right that it was never the default.

But that's not the same thing. The whole point of "pull.mode=ff-only"
was to print a message like:

  The pull was not fast-forward, please either merge or rebase.

Cheers.

-- 
Felipe Contreras

Re: Pick the right default and stop warn on `git pull`

[Jeff King]

On Mon, Nov 23, 2020 at 04:55:31PM -0600, Felipe Contreras wrote:

> On Mon, Nov 23, 2020 at 3:48 PM Jeff King <peff@peff.net> wrote:
> > On Mon, Nov 23, 2020 at 02:34:18PM -0600, Felipe Contreras wrote:
> 
> > > There has never been a "pull.mode=ff-only" option; that's what I tried
> > > to introduce.
> >
> > There is pull.ff=only these days, which also squelches the message. But
> > you're right that it was never the default.
> 
> But that's not the same thing. The whole point of "pull.mode=ff-only"
> was to print a message like:
> 
>   The pull was not fast-forward, please either merge or rebase.

Perhaps I don't understand the distinction you are making. I have
pull.ff=ff set, and I get:

  $ git pull
  remote: Enumerating objects: 1, done.
  remote: Counting objects: 100% (1/1), done.
  remote: Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
  Receiving objects: 100% (1/1), done.
  From /home/peff/tmp/.
     a4f00e2..22a4151  master     -> origin/master
  fatal: Not possible to fast-forward, aborting.

Which sounds like the same thing, modulo the error message not being
nearly as helpful (which I would be happy to see rectified).

Is there some other difference you're thinking of?

-Peff

Re: Pick the right default and stop warn on `git pull`

[Felipe Contreras]

On Mon, Nov 23, 2020 at 6:39 PM Jeff King <peff@peff.net> wrote:
> On Mon, Nov 23, 2020 at 04:55:31PM -0600, Felipe Contreras wrote:
> > On Mon, Nov 23, 2020 at 3:48 PM Jeff King <peff@peff.net> wrote:

> > > There is pull.ff=only these days, which also squelches the message. But
> > > you're right that it was never the default.
> >
> > But that's not the same thing. The whole point of "pull.mode=ff-only"
> > was to print a message like:
> >
> >   The pull was not fast-forward, please either merge or rebase.
>
> Perhaps I don't understand the distinction you are making. I have
> pull.ff=ff set, and I get:

You mean "pull.ff=only" ?

>   $ git pull

Try the same thing with "pull.rebase=true".

> Which sounds like the same thing, modulo the error message not being
> nearly as helpful (which I would be happy to see rectified).
>
> Is there some other difference you're thinking of?

It's mainly the error message, but also the fact that pull.ff is
ignored when rebasing.

Additionally, using "pull.mode=default", I introduced the following warning:

  The pull was not fast-forward, in the future you would have to
  choose a merge or a rebase, falling back to old style for now (git
  pull --merge).
  Read 'git pull --help' for more information."

Which was triggered *only* when pull.mode wasn't set, *and* the merge
wasn't fast-forward.

This warning would have been much better than what we have now. I think.

Cheers.

-- 
Felipe Contreras

Re: Pick the right default and stop warn on `git pull`

[Jeff King]

On Mon, Nov 23, 2020 at 06:57:17PM -0600, Felipe Contreras wrote:

> > > But that's not the same thing. The whole point of "pull.mode=ff-only"
> > > was to print a message like:
> > >
> > >   The pull was not fast-forward, please either merge or rebase.
> >
> > Perhaps I don't understand the distinction you are making. I have
> > pull.ff=ff set, and I get:
> 
> You mean "pull.ff=only" ?

Er, yeah, sorry.

> >   $ git pull
> 
> Try the same thing with "pull.rebase=true".

Ah, that's what I was missing. I agree it would be nice for it to behave
consistently in both cases (though why one would set both pull.ff=only
and pull.rebase=true, I don't know).

-Peff

Re: Pick the right default and stop warn on `git pull`

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Ah, that's what I was missing. I agree it would be nice for it to behave
> consistently in both cases (though why one would set both pull.ff=only
> and pull.rebase=true, I don't know).

So an obvious thing we could do, if pull.mode is too much of a
change, is to make "pull --rebase" codepath honor pull.ff as well,
perhaps?  I.e. those who set pull.ff=only are saying that "please
stop me when I have any local change---I want to be notified if my
pull on this branch results in anything but a fast-forward from the
upstream".

And then making an unconfigured pull.ff to default to pull.ff=only
may give a proper failure whether you merge or rebase.  I dunno.

Re: Pick the right default and stop warn on `git pull`

[Jeff King]

On Mon, Nov 23, 2020 at 06:18:40PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Ah, that's what I was missing. I agree it would be nice for it to behave
> > consistently in both cases (though why one would set both pull.ff=only
> > and pull.rebase=true, I don't know).
> 
> So an obvious thing we could do, if pull.mode is too much of a
> change, is to make "pull --rebase" codepath honor pull.ff as well,
> perhaps?  I.e. those who set pull.ff=only are saying that "please
> stop me when I have any local change---I want to be notified if my
> pull on this branch results in anything but a fast-forward from the
> upstream".
> 
> And then making an unconfigured pull.ff to default to pull.ff=only
> may give a proper failure whether you merge or rebase.  I dunno.

Yeah, I would be perfectly happy with that (and it's in fact what I
_thought_ was happening before today's discussion).

I do wonder if anybody has set:

  pull.rebase=true
  pull.ff=only

which would then refuse to rebase at all, and whether they would be
annoyed. I am scratching my head over why one would do that, though. It
is meaningful only if you usually rebase, but when you say "--no-rebase"
you want to make sure you do not create a merge commit. Which seems
weird.

-Peff

Re: Pick the right default and stop warn on `git pull`

[Felipe Contreras]

On Mon, Nov 23, 2020 at 8:32 PM Jeff King <peff@peff.net> wrote:
> On Mon, Nov 23, 2020 at 06:18:40PM -0800, Junio C Hamano wrote:

> > So an obvious thing we could do, if pull.mode is too much of a
> > change, is to make "pull --rebase" codepath honor pull.ff as well,
> > perhaps?  I.e. those who set pull.ff=only are saying that "please
> > stop me when I have any local change---I want to be notified if my
> > pull on this branch results in anything but a fast-forward from the
> > upstream".
> >
> > And then making an unconfigured pull.ff to default to pull.ff=only
> > may give a proper failure whether you merge or rebase.  I dunno.
>
> Yeah, I would be perfectly happy with that (and it's in fact what I
> _thought_ was happening before today's discussion).
>
> I do wonder if anybody has set:
>
>   pull.rebase=true
>   pull.ff=only
>
> which would then refuse to rebase at all, and whether they would be
> annoyed. I am scratching my head over why one would do that, though. It
> is meaningful only if you usually rebase, but when you say "--no-rebase"
> you want to make sure you do not create a merge commit. Which seems
> weird.

I think you are losing track of the goal.

The goal is that *eventually*:

1. No warning is issued
2. No configuration is needed
3. The default behavior is sane.

The whole point of "pull.rebase=ff-only" (aka. "pull.mode=ff-only")
was to make it the *default*.

If you make "pull.ff=only" the default, *and* you make "git pull
--rebase" respect that, then "git pull --rebase" will fail by default
(unless it's a fast-forward).

What we really need is something like:

1. git pull # fail by default unless it's a fast-forward
2. git pull --merge # force a merge (unless it's a fast-forward,
depending on pull.ff)
3. git pull --rebase # force a rebase (unless it's a fast-forward,
depending on pull.ff)

Therefore, what we really want is "git pull --rebase" *ignore*
"pull.ff=only" (a possible default) or ignore "pull.rebase=ff-only"
(also another possible default).

It would be possible to do something like:

  if (!opt_rebase && (!opt_ff || !strcmp(opt_ff, "--ff-only")))
    turn_default_behavior = 1;

But then how would we distinguish between "git pull", and "git pull
--no-rebase" (aka. "git pull --merge" / "pull.rebase=false")?


This is just too much unnecessary complication There's no need to
entertain a dozen possible heuristics to avoid "pull.mode", none of
which avoid breaking existing behavior.

Let's just accept we need push.mode, and then we can have everything:
default, ff-only, merge, rebase.

Cheers.

-- 
Felipe Contreras

Re: Pick the right default and stop warn on `git pull`

[Jeff King]

On Mon, Nov 23, 2020 at 09:41:05PM -0600, Felipe Contreras wrote:

> What we really need is something like:
> 
> 1. git pull # fail by default unless it's a fast-forward
> 2. git pull --merge # force a merge (unless it's a fast-forward,
> depending on pull.ff)
> 3. git pull --rebase # force a rebase (unless it's a fast-forward,
> depending on pull.ff)
> 
> Therefore, what we really want is "git pull --rebase" *ignore*
> "pull.ff=only" (a possible default) or ignore "pull.rebase=ff-only"
> (also another possible default).

Yep. After reading the first half of your mail, I started to respond
with the exact same thing. The key thing is letting the command-line
options override all of the related config. But I guess after reading to
the end that you don't actually like this. ;)

I do agree it would be more clear in the long run with a single option
(config and command-line) that makes it clear the values are mutually
exclusive. I'm just not sure if it's painful to get there without
breaking compatibility or introducing confusion in the meantime.

> It would be possible to do something like:
> 
>   if (!opt_rebase && (!opt_ff || !strcmp(opt_ff, "--ff-only")))
>     turn_default_behavior = 1;
> 
> But then how would we distinguish between "git pull", and "git pull
> --no-rebase" (aka. "git pull --merge" / "pull.rebase=false")?

I'm not sure what you mean. We can tell the difference between those
based on what we saw on the command-line option. I.e., your
"!opt_rebase" is really a tri-state, which allows something like:

  if (opt_rebase == REBASE_UNSET) {
          if (opt_ff == FF_UNSET)
	          opt_ff = ff_default; /* from config or baked-in */
	  }
	  opt_rebase = rebase_default;
  }

but I didn't look at the logic in git-pull.

That said...

> This is just too much unnecessary complication There's no need to
> entertain a dozen possible heuristics to avoid "pull.mode", none of
> which avoid breaking existing behavior.
> 
> Let's just accept we need push.mode, and then we can have everything:
> default, ff-only, merge, rebase.

I think it could be possible for the documentation to make clear the
interactions, especially if the feature is designed with eventual
deprecation of other options (e.g., if it says "pull.mode=ff-only" means
that pull.ff won't be examined, and there's no need to ever use it
anymore).

-Peff

Re: Pick the right default and stop warn on `git pull`

[Felipe Contreras]

On Tue, Nov 24, 2020 at 1:19 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Nov 23, 2020 at 09:41:05PM -0600, Felipe Contreras wrote:
>
> > What we really need is something like:
> >
> > 1. git pull # fail by default unless it's a fast-forward
> > 2. git pull --merge # force a merge (unless it's a fast-forward,
> > depending on pull.ff)
> > 3. git pull --rebase # force a rebase (unless it's a fast-forward,
> > depending on pull.ff)
> >
> > Therefore, what we really want is "git pull --rebase" *ignore*
> > "pull.ff=only" (a possible default) or ignore "pull.rebase=ff-only"
> > (also another possible default).
>
> Yep. After reading the first half of your mail, I started to respond
> with the exact same thing. The key thing is letting the command-line
> options override all of the related config. But I guess after reading to
> the end that you don't actually like this. ;)

Yes, the command-line options should override the configuration, and
the configuration should override the default.

I'm not sure what makes you think I wouldn't like that.

> I do agree it would be more clear in the long run with a single option
> (config and command-line) that makes it clear the values are mutually
> exclusive. I'm just not sure if it's painful to get there without
> breaking compatibility or introducing confusion in the meantime.

I think it is possible. I did the patches several years ago. And I'm
working on the patches right now. We'll see.

> > It would be possible to do something like:
> >
> >   if (!opt_rebase && (!opt_ff || !strcmp(opt_ff, "--ff-only")))
> >     turn_default_behavior = 1;
> >
> > But then how would we distinguish between "git pull", and "git pull
> > --no-rebase" (aka. "git pull --merge" / "pull.rebase=false")?
>
> I'm not sure what you mean. We can tell the difference between those
> based on what we saw on the command-line option. I.e., your
> "!opt_rebase" is really a tri-state, which allows something like:
>
>   if (opt_rebase == REBASE_UNSET) {
>           if (opt_ff == FF_UNSET)
>                   opt_ff = ff_default; /* from config or baked-in */
>           }
>           opt_rebase = rebase_default;
>   }
>
> but I didn't look at the logic in git-pull.

Well, in git-pull there's a callback called: parse_opt_rebase(), and
if no argument is passed, then it returns REBASE_FALSE (0).

The rest of the code assumes 0 is no-rebase (i.e. merge).

There's no REBASE_UNSET.

Granted, it may be possible to change all the code, introduce a
REBASE_UNSET, and make it so 0 is not REBASE_FALSE, and so on.
Additionally, the code that deals with the configuration part needs to
be changed too.

I'd rather not.

> > This is just too much unnecessary complication There's no need to
> > entertain a dozen possible heuristics to avoid "pull.mode", none of
> > which avoid breaking existing behavior.
> >
> > Let's just accept we need push.mode, and then we can have everything:
> > default, ff-only, merge, rebase.
>
> I think it could be possible for the documentation to make clear the
> interactions, especially if the feature is designed with eventual
> deprecation of other options (e.g., if it says "pull.mode=ff-only" means
> that pull.ff won't be examined, and there's no need to ever use it
> anymore).

I'm not sure there will be no need for "pull.ff". Even with
"pull.mode=ff-only", you should be able to do "git pull --merge"
(should override the configured mode), in which case pull.ff will be
considered (and maybe in "git pull --rebase" too).

Regarding the documentation; I think it should be possible to describe
the interactions clearly, but as a sequence of overrides, and when no
override is specified (default) do something sensible (which right now
it's quite complex to determine).

Cheers.

-- 
Felipe Contreras

Re: Pick the right default and stop warn on `git pull`

[Jeff King]

On Tue, Nov 24, 2020 at 01:48:56AM -0600, Felipe Contreras wrote:

> > Yep. After reading the first half of your mail, I started to respond
> > with the exact same thing. The key thing is letting the command-line
> > options override all of the related config. But I guess after reading to
> > the end that you don't actually like this. ;)
> 
> Yes, the command-line options should override the configuration, and
> the configuration should override the default.
> 
> I'm not sure what makes you think I wouldn't like that.

I think it's obvious that "--rebase" should override "pull.rebase". But
what I thought you were suggesting (and which I think is reasonable) is
for "--rebase" to affect pull.ff, as well. That's less clear (and
doesn't happen now), but I think could produce sensible semantics.

But it sounded like you thought that was too complicated.

> > I do agree it would be more clear in the long run with a single option
> > (config and command-line) that makes it clear the values are mutually
> > exclusive. I'm just not sure if it's painful to get there without
> > breaking compatibility or introducing confusion in the meantime.
> 
> I think it is possible. I did the patches several years ago. And I'm
> working on the patches right now. We'll see.

OK. Then I'm happy to wait and see.

> Well, in git-pull there's a callback called: parse_opt_rebase(), and
> if no argument is passed, then it returns REBASE_FALSE (0).
> 
> The rest of the code assumes 0 is no-rebase (i.e. merge).
> 
> There's no REBASE_UNSET.

Right, I meant that you would add one. But if you are pursuing the other
option, we can see how that goes.

-Peff

Re: Pick the right default and stop warn on `git pull`

[Vít Ondruch]

[-- Attachment #1.1.1: Type: text/plain, Size: 2052 bytes --]


Dne 24. 11. 20 v 4:41 Felipe Contreras napsal(a):
> On Mon, Nov 23, 2020 at 8:32 PM Jeff King <peff@peff.net> wrote:
>> On Mon, Nov 23, 2020 at 06:18:40PM -0800, Junio C Hamano wrote:
>>> So an obvious thing we could do, if pull.mode is too much of a
>>> change, is to make "pull --rebase" codepath honor pull.ff as well,
>>> perhaps?  I.e. those who set pull.ff=only are saying that "please
>>> stop me when I have any local change---I want to be notified if my
>>> pull on this branch results in anything but a fast-forward from the
>>> upstream".
>>>
>>> And then making an unconfigured pull.ff to default to pull.ff=only
>>> may give a proper failure whether you merge or rebase.  I dunno.
>> Yeah, I would be perfectly happy with that (and it's in fact what I
>> _thought_ was happening before today's discussion).
>>
>> I do wonder if anybody has set:
>>
>>    pull.rebase=true
>>    pull.ff=only
>>
>> which would then refuse to rebase at all, and whether they would be
>> annoyed. I am scratching my head over why one would do that, though. It
>> is meaningful only if you usually rebase, but when you say "--no-rebase"
>> you want to make sure you do not create a merge commit. Which seems
>> weird.
> I think you are losing track of the goal.
>
> The goal is that *eventually*:
>
> 1. No warning is issued
> 2. No configuration is needed
> 3. The default behavior is sane.
>
> The whole point of "pull.rebase=ff-only" (aka. "pull.mode=ff-only")
> was to make it the *default*.
>
> If you make "pull.ff=only" the default, *and* you make "git pull
> --rebase" respect that, then "git pull --rebase" will fail by default
> (unless it's a fast-forward).
>
> What we really need is something like:
>
> 1. git pull # fail by default unless it's a fast-forward
> 2. git pull --merge # force a merge (unless it's a fast-forward,
> depending on pull.ff)
> 3. git pull --rebase # force a rebase (unless it's a fast-forward,
> depending on pull.ff)


This is the default I would expect.


Vít



[-- Attachment #1.1.2: OpenPGP_0x0CE09EE79917B87C.asc --]
[-- Type: application/pgp-keys, Size: 61897 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: Pick the right default and stop warn on `git pull`

[Alex Henrie]

On Mon, Nov 23, 2020 at 8:41 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> 1. git pull # fail by default unless it's a fast-forward
> 2. git pull --merge # force a merge (unless it's a fast-forward,
> depending on pull.ff)
> 3. git pull --rebase # force a rebase (unless it's a fast-forward,
> depending on pull.ff)

I'm not sure that we need a new --merge flag for this. It sounds like
we just want pull.ff to default to "only" if pull.rebase is not set,
and if pull.rebase is set (or --rebase or --no-rebase is passed on the
command line) then pull.ff should default to "true". If I understand
correctly, that would get us everything we want without adding any new
variables or making any major changes to how Git works.

-Alex

Re: Pick the right default and stop warn on `git pull`

[Felipe Contreras]

On Tue, Nov 24, 2020 at 2:22 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> On Mon, Nov 23, 2020 at 8:41 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > 1. git pull # fail by default unless it's a fast-forward
> > 2. git pull --merge # force a merge (unless it's a fast-forward,
> > depending on pull.ff)
> > 3. git pull --rebase # force a rebase (unless it's a fast-forward,
> > depending on pull.ff)
>
> I'm not sure that we need a new --merge flag for this. It sounds like
> we just want pull.ff to default to "only" if pull.rebase is not set,
> and if pull.rebase is set (or --rebase or --no-rebase is passed on the
> command line) then pull.ff should default to "true". If I understand
> correctly, that would get us everything we want without adding any new
> variables or making any major changes to how Git works.

Before making "pull.ff=only" the default, we need a transitional
period in which if it's unset, a warning is printed when pulling a
non-fast-forward branch. This is what my patch did several years ago.

And yes, it should be possible to do what you suggest, but have you
tried? I suspect both the code and the documentation would be quite
convoluted.

Cheers.

-- 
Felipe Contreras

Re: Pick the right default and stop warn on `git pull`

[Alex Henrie]

On Tue, Nov 24, 2020 at 3:11 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Before making "pull.ff=only" the default, we need a transitional
> period in which if it's unset, a warning is printed when pulling a
> non-fast-forward branch. This is what my patch did several years ago.

We'd only need a warning if both pull.ff and pull.rebase are unset,
since that's the only situation where the behavior would change. And
providentially, we already have a warning in that exact case, although
we should probably tweak it to explain what the new behavior is going
to be :-)

> And yes, it should be possible to do what you suggest, but have you
> tried? I suspect both the code and the documentation would be quite
> convoluted.

Yes, I have written a very simple patch that implements the behavior
that I am suggesting. I will send it to the mailing list later
tonight.

-Alex

Re: Pick the right default and stop warn on `git pull`

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> We'd only need a warning if both pull.ff and pull.rebase are unset,
> since that's the only situation where the behavior would change. And
> providentially, we already have a warning in that exact case, although
> we should probably tweak it to explain what the new behavior is going
> to be :-)

If we were starting "git pull" from scratch without any existing
configuration and command line options, pull.mode might present a
simpler end user experience, but I have to say that pull.ff and
pull.rebase that are fairly well ingrained in docs and users' minds,
the above plan sounds like a better option to reduce the cognitive
overhead for users.

>> And yes, it should be possible to do what you suggest, but have you
>> tried? I suspect both the code and the documentation would be quite
>> convoluted.
>
> Yes, I have written a very simple patch that implements the behavior
> that I am suggesting. I will send it to the mailing list later
> tonight.

Let's see how well the comparison between two approaches play out.
Thanks.

Re: Pick the right default and stop warn on `git pull`

[Felipe Contreras]

On Tue, Nov 24, 2020 at 6:39 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > We'd only need a warning if both pull.ff and pull.rebase are unset,
> > since that's the only situation where the behavior would change. And
> > providentially, we already have a warning in that exact case, although
> > we should probably tweak it to explain what the new behavior is going
> > to be :-)
>
> If we were starting "git pull" from scratch without any existing
> configuration and command line options, pull.mode might present a
> simpler end user experience, but I have to say that pull.ff and
> pull.rebase that are fairly well ingrained in docs and users' minds,
> the above plan sounds like a better option to reduce the cognitive
> overhead for users.

Is it? From forums, blogs, and answers in Stack Overflow what I mostly
see is "do not do git pull".

> >> And yes, it should be possible to do what you suggest, but have you
> >> tried? I suspect both the code and the documentation would be quite
> >> convoluted.
> >
> > Yes, I have written a very simple patch that implements the behavior
> > that I am suggesting. I will send it to the mailing list later
> > tonight.
>
> Let's see how well the comparison between two approaches play out.
> Thanks.

I tried to remove pull.mode from my patch series, and it mostly works,
except if you:

1. Have "pull.ff=only"
2. Do a "git pull --no-rebase"

When you have "pull.mode=ff-only", doing "git pull --no-rebase" ("git
pull --merge") that mode is overridden, and the merge succeeds.

If you have "pull.ff=only", you would have to do "git pull --no-rebase
--no-ff" for the merge to succeed, which I don't think should be the
default.

We could change the semantics of "pull.ff=only", but that would break
existing behavior, and it starts to get convoluted.

Cheers.

-- 
Felipe Contreras

Re: Pick the right default and stop warn on `git pull`

[Junio C Hamano]

Vít Ondruch <vondruch@redhat.com> writes:

> While I understand that the merge after `git pull` is probably not the
> best strategy (as explained in [1]), asking people to change their 
> configuration to choose their default strategy with every `git pull`
> is not any better.

There is no clear "default that would suit everybody".  There are
many people for whom 'rebase' is totally a wrong thing in their
'pull', and there are many people for whom 'merge' is totally a
wrong thing in their 'pull'.  Worse, because a user does not
necessarily play the same role in multiple projects, even the same
human user may want to use 'rebase' while working on some projects
while 'merge' is preferred on other projects.

And the userbase of Git is large enough that it is no longer a
viable approach to say "which group has more people?  pick the one
that is better to them" unless we are dealing with a _new_ feature.
Even 1% minority of the userbase is still a large number in absolute
terms.

That is why pull.rebase=(yes|no) exists to serve people with either
workflow to record preference just once for the repository with "git
config" and "git pull" would stop asking in the repository.

When they always prefer one way, they can even run "git config
--global" just once and the setting applies to all of their
repositories, stopping "git pull" from asking in any repository.

Re: Pick the right default and stop warn on `git pull`

[Felipe Contreras]

On Mon, Nov 23, 2020 at 1:15 PM Junio C Hamano <gitster@pobox.com> wrote:
> Vít Ondruch <vondruch@redhat.com> writes:

> > While I understand that the merge after `git pull` is probably not the
> > best strategy (as explained in [1]), asking people to change their
> > configuration to choose their default strategy with every `git pull`
> > is not any better.
>
> There is no clear "default that would suit everybody".

This is a false dichotomy.

Yes, there's no default that would suit everybody, but there is a
better default than the current one:

fast-forward-only:

https://lore.kernel.org/git/1398988808-29678-1-git-send-email-felipe.contreras@gmail.com/

-- 
Felipe Contreras

Re: Pick the right default and stop warn on `git pull`

[Felipe Contreras]

On Mon, Nov 23, 2020 at 9:17 AM Vít Ondruch <vondruch@redhat.com> wrote:

> While I understand that the merge after `git pull` is probably not the
> best strategy (as explained in [1]), asking people to change their
> configuration to choose their default strategy with every `git pull` is
> not any better. Please select any strategy considered more appropriate
> and stop warning me. I know I could change my configuration, but I'll
> change it just if I want to differ from defaults. I don't need to be
> told that I am using wrong default. I'd like to keep my trust in Git
> upstream, but this is was not trustworthy decision IMO.

Indeed.

This is why I sent patches to change the "git pull" default to merge
the remote branch, but only if it's a fast-forward. If it's not, then
the user would have to either merge, or rebase.

https://lore.kernel.org/git/1398988808-29678-1-git-send-email-felipe.contreras@gmail.com/

This is the best sensible default.

Cheers.

-- 
Felipe Contreras