| Message ID | 20201204061623.1170745-3-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pull: default warning improvements | expand |
On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > We want to: > > 1. Be clear about what "specifying" means; merge or rebase. > 2. Mention a direct shortcut for people that just want to get on with > their lives: git pull --no-rebase. This is a shortcut for what? > 3. Have a quick reference for users to understand what this > "fast-forward" business means. > > This patch does all three. > > Thanks to the previous patch now "git pull --help" explains what a > fast-forward is, and a further patch changes --no-rebase to --merge so > it's actually user friendly. > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > builtin/pull.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index 1034372f8b..22a9ffcade 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -346,17 +346,18 @@ static enum rebase_type config_get_rebase(void) > > if (opt_verbosity >= 0 && !opt_ff) { > advise(_("Pulling without specifying how to reconcile divergent branches is\n" > - "discouraged. You can squelch this message by running one of the following\n" > - "commands sometime before your next pull:\n" > - "\n" > - " git config pull.rebase false # merge (the default strategy)\n" > - " git config pull.rebase true # rebase\n" > - " git config pull.ff only # fast-forward only\n" > - "\n" > - "You can replace \"git config\" with \"git config --global\" to set a default\n" > - "preference for all repositories. You can also pass --rebase, --no-rebase,\n" > - "or --ff-only on the command line to override the configured default per\n" > - "invocation.\n")); > + "discouraged; you need to specify if you want a merge, or a rebase.\n" ...want a merge, a rebase, or neither. > + "You can squelch this message by running one of the following commands:\n" > + "\n" > + " git config pull.rebase false # merge (the default strategy)\n" Should this be labelled as the default given the desire to make --ff-only the default? Perhaps I'm jumping ahead and you plan to change that later in this series. > + " git config pull.rebase true # rebase\n" > + " git config pull.ff only # fast-forward only\n" > + "\n" > + "You can replace \"git config\" with \"git config --global\" to set a default\n" > + "preference for all repositories.\n" Good up to here. > + "If unsure, run \"git pull --no-rebase\".\n" Why is that safe to suggest? The original text may not have been the easiest to parse, but this seems more problematic to me. > + "Read \"git pull --help\" for more information." Nice addition. > + )); > } > > return REBASE_FALSE; > -- > 2.29.2 >
On Fri, Dec 4, 2020 at 5:00 PM Elijah Newren <newren@gmail.com> wrote: > > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > We want to: > > > > 1. Be clear about what "specifying" means; merge or rebase. > > 2. Mention a direct shortcut for people that just want to get on with > > their lives: git pull --no-rebase. > > This is a shortcut for what? git config --global pull.rebase false git pull It's a shorter way of saying: "do a 'git pull' like you've always done but don't warn me". > > 3. Have a quick reference for users to understand what this > > "fast-forward" business means. > > > > This patch does all three. > > > > Thanks to the previous patch now "git pull --help" explains what a > > fast-forward is, and a further patch changes --no-rebase to --merge so > > it's actually user friendly. > > > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > > --- > > builtin/pull.c | 23 ++++++++++++----------- > > 1 file changed, 12 insertions(+), 11 deletions(-) > > > > diff --git a/builtin/pull.c b/builtin/pull.c > > index 1034372f8b..22a9ffcade 100644 > > --- a/builtin/pull.c > > +++ b/builtin/pull.c > > @@ -346,17 +346,18 @@ static enum rebase_type config_get_rebase(void) > > > > if (opt_verbosity >= 0 && !opt_ff) { > > advise(_("Pulling without specifying how to reconcile divergent branches is\n" > > - "discouraged. You can squelch this message by running one of the following\n" > > - "commands sometime before your next pull:\n" > > - "\n" > > - " git config pull.rebase false # merge (the default strategy)\n" > > - " git config pull.rebase true # rebase\n" > > - " git config pull.ff only # fast-forward only\n" > > - "\n" > > - "You can replace \"git config\" with \"git config --global\" to set a default\n" > > - "preference for all repositories. You can also pass --rebase, --no-rebase,\n" > > - "or --ff-only on the command line to override the configured default per\n" > > - "invocation.\n")); > > + "discouraged; you need to specify if you want a merge, or a rebase.\n" > > ...want a merge, a rebase, or neither. There is no "git pull --no-merge". Years ago some people argued for a "pull.mode=none" (essentially making "git pull" the same as "git fetch"). But right now there's no option to do that. There's an option to do --ff-only, but that's still a merge. Perhaps: a merge, a rebase, or a fast-forward? > > + "You can squelch this message by running one of the following commands:\n" > > + "\n" > > + " git config pull.rebase false # merge (the default strategy)\n" > > Should this be labelled as the default given the desire to make > --ff-only the default? Perhaps I'm jumping ahead and you plan to > change that later in this series. That's right. In the previous series which does indeed make "pull.mode=ff-only" the default [1], I do change the warning to specify the future default [2], but in that series the warnings is changed to: The pull was not fast-forward, in the future you will have to choose a merge, or a rebase. To squelch this message and maintain the current behavior, use: git config --global pull.mode merge To squelch this message and adopt the new behavior now, use: git config --global push.mode ff-only Falling back to old style for now (merge). Read "git pull --help" for more information. Since that series didn't get any traction, I decided to only implement step 1: fix the current situation. And later on another series would do step 2: introduce "pull.mode=ff-only" and do the preparations to make it the default. > > + " git config pull.rebase true # rebase\n" > > + " git config pull.ff only # fast-forward only\n" > > + "\n" > > + "You can replace \"git config\" with \"git config --global\" to set a default\n" > > + "preference for all repositories.\n" > > Good up to here. > > > + "If unsure, run \"git pull --no-rebase\".\n" > > Why is that safe to suggest? The original text may not have been the > easiest to parse, but this seems more problematic to me. Because "git pull" has been doing the same as "git pull --no-rebase" for more than a decade. It's safe for people to continue with this behavior for a few more months. Some people need to get things done today, and they are not interested in future changes, nor changing their default configuration, or what the warning has to say. They just want "git pull" to do the same as yesterday, and the year before, without being bothered with an annoying warning. Those people can start training their fingers to do "git pull --merge", and learn the problems with "git pull" later. We want to respect the user's time, and not force them to read the warning today. > > + "Read \"git pull --help\" for more information." > > Nice addition. Especially since now it explains what a fast-forward is.
Hi Felipe, On Fri, Dec 4, 2020 at 4:12 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > On Fri, Dec 4, 2020 at 5:00 PM Elijah Newren <newren@gmail.com> wrote: > > > > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > > > > > > We want to: > > > > > > 1. Be clear about what "specifying" means; merge or rebase. > > > 2. Mention a direct shortcut for people that just want to get on with > > > their lives: git pull --no-rebase. > > > > This is a shortcut for what? > > git config --global pull.rebase false > git pull > > It's a shorter way of saying: "do a 'git pull' like you've always done > but don't warn me". > > > > 3. Have a quick reference for users to understand what this > > > "fast-forward" business means. > > > > > > This patch does all three. > > > > > > Thanks to the previous patch now "git pull --help" explains what a > > > fast-forward is, and a further patch changes --no-rebase to --merge so > > > it's actually user friendly. > > > > > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > > > --- > > > builtin/pull.c | 23 ++++++++++++----------- > > > 1 file changed, 12 insertions(+), 11 deletions(-) > > > > > > diff --git a/builtin/pull.c b/builtin/pull.c > > > index 1034372f8b..22a9ffcade 100644 > > > --- a/builtin/pull.c > > > +++ b/builtin/pull.c > > > @@ -346,17 +346,18 @@ static enum rebase_type config_get_rebase(void) > > > > > > if (opt_verbosity >= 0 && !opt_ff) { > > > advise(_("Pulling without specifying how to reconcile divergent branches is\n" > > > - "discouraged. You can squelch this message by running one of the following\n" > > > - "commands sometime before your next pull:\n" > > > - "\n" > > > - " git config pull.rebase false # merge (the default strategy)\n" > > > - " git config pull.rebase true # rebase\n" > > > - " git config pull.ff only # fast-forward only\n" > > > - "\n" > > > - "You can replace \"git config\" with \"git config --global\" to set a default\n" > > > - "preference for all repositories. You can also pass --rebase, --no-rebase,\n" > > > - "or --ff-only on the command line to override the configured default per\n" > > > - "invocation.\n")); > > > + "discouraged; you need to specify if you want a merge, or a rebase.\n" > > > > ...want a merge, a rebase, or neither. > > There is no "git pull --no-merge". Years ago some people argued for a > "pull.mode=none" (essentially making "git pull" the same as "git > fetch"). But right now there's no option to do that. > > There's an option to do --ff-only, but that's still a merge. I disagree. I'm well aware that checkout_fast_forward() (which is what is ultimately called to do the fast-forwarding) is in a file called merge.c, but that doesn't make it a merge. I don't believe it was anything more than a convenient place to dump some extra code at the time. > Perhaps: a merge, a rebase, or a fast-forward? Sure, that works; in fact, that's much better than my suggestion. I like it. > > > + "You can squelch this message by running one of the following commands:\n" > > > + "\n" > > > + " git config pull.rebase false # merge (the default strategy)\n" > > > > Should this be labelled as the default given the desire to make > > --ff-only the default? Perhaps I'm jumping ahead and you plan to > > change that later in this series. > > That's right. > > In the previous series which does indeed make "pull.mode=ff-only" the > default [1], I do change the warning to specify the future default > [2], but in that series the warnings is changed to: > > The pull was not fast-forward, in the future you will have to choose > a merge, or a rebase. > To squelch this message and maintain the current behavior, use: > > git config --global pull.mode merge > > To squelch this message and adopt the new behavior now, use: > > git config --global push.mode ff-only > > Falling back to old style for now (merge). > Read "git pull --help" for more information. > > Since that series didn't get any traction, I decided to only implement > step 1: fix the current situation. And later on another series would > do step 2: introduce "pull.mode=ff-only" and do the preparations to > make it the default. I like this longer plan. However, on the shorter scale plan...I think the suggestion to use "git pull --no-rebase" makes the current situation worse rather than fixing it. > > > + " git config pull.rebase true # rebase\n" > > > + " git config pull.ff only # fast-forward only\n" > > > + "\n" > > > + "You can replace \"git config\" with \"git config --global\" to set a default\n" > > > + "preference for all repositories.\n" > > > > Good up to here. > > > > > + "If unsure, run \"git pull --no-rebase\".\n" > > > > Why is that safe to suggest? The original text may not have been the > > easiest to parse, but this seems more problematic to me. > > Because "git pull" has been doing the same as "git pull --no-rebase" > for more than a decade. It's safe for people to continue with this > behavior for a few more months. > > Some people need to get things done today, and they are not interested > in future changes, nor changing their default configuration, or what > the warning has to say. > > They just want "git pull" to do the same as yesterday, and the year > before, without being bothered with an annoying warning. > > Those people can start training their fingers to do "git pull > --merge", and learn the problems with "git pull" later. > > We want to respect the user's time, and not force them to read the > warning today. The warning was added because sending users down paths that break things badly is a waste of user's time, and often a much bigger waste of user's time than making them read up on the meaning behind the two different choices of what kind of changes they can make. I agree the warning went too far, but I fully agree with the original folks who put this warning here that we need to have it for at least some cases and that there is a decision to be made. Though I am just one voice, and perhaps others will agree with you on your point here, I'll continue to disagree with blindly suggesting "git pull --no-rebase". > > > + "Read \"git pull --help\" for more information." > > > > Nice addition. > > Especially since now it explains what a fast-forward is. Indeed. :-)
On Fri, Dec 4, 2020 at 6:56 PM Elijah Newren <newren@gmail.com> wrote: > > Hi Felipe, > > On Fri, Dec 4, 2020 at 4:12 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > On Fri, Dec 4, 2020 at 5:00 PM Elijah Newren <newren@gmail.com> wrote: > > > > > > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras > > > <felipe.contreras@gmail.com> wrote: > > > > > > > > We want to: > > > > > > > > 1. Be clear about what "specifying" means; merge or rebase. > > > > 2. Mention a direct shortcut for people that just want to get on with > > > > their lives: git pull --no-rebase. > > > > > > This is a shortcut for what? > > > > git config --global pull.rebase false > > git pull > > > > It's a shorter way of saying: "do a 'git pull' like you've always done > > but don't warn me". > > > > > > 3. Have a quick reference for users to understand what this > > > > "fast-forward" business means. > > > > > > > > This patch does all three. > > > > > > > > Thanks to the previous patch now "git pull --help" explains what a > > > > fast-forward is, and a further patch changes --no-rebase to --merge so > > > > it's actually user friendly. > > > > > > > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > > > > --- > > > > builtin/pull.c | 23 ++++++++++++----------- > > > > 1 file changed, 12 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/builtin/pull.c b/builtin/pull.c > > > > index 1034372f8b..22a9ffcade 100644 > > > > --- a/builtin/pull.c > > > > +++ b/builtin/pull.c > > > > @@ -346,17 +346,18 @@ static enum rebase_type config_get_rebase(void) > > > > > > > > if (opt_verbosity >= 0 && !opt_ff) { > > > > advise(_("Pulling without specifying how to reconcile divergent branches is\n" > > > > - "discouraged. You can squelch this message by running one of the following\n" > > > > - "commands sometime before your next pull:\n" > > > > - "\n" > > > > - " git config pull.rebase false # merge (the default strategy)\n" > > > > - " git config pull.rebase true # rebase\n" > > > > - " git config pull.ff only # fast-forward only\n" > > > > - "\n" > > > > - "You can replace \"git config\" with \"git config --global\" to set a default\n" > > > > - "preference for all repositories. You can also pass --rebase, --no-rebase,\n" > > > > - "or --ff-only on the command line to override the configured default per\n" > > > > - "invocation.\n")); > > > > + "discouraged; you need to specify if you want a merge, or a rebase.\n" > > > > > > ...want a merge, a rebase, or neither. > > > > There is no "git pull --no-merge". Years ago some people argued for a > > "pull.mode=none" (essentially making "git pull" the same as "git > > fetch"). But right now there's no option to do that. > > > > There's an option to do --ff-only, but that's still a merge. > > I disagree. I'm well aware that checkout_fast_forward() (which is > what is ultimately called to do the fast-forwarding) is in a file > called merge.c, but that doesn't make it a merge. I don't believe it > was anything more than a convenient place to dump some extra code at > the time. Right. Maybe my mind is tainted by doing so many fast-forward "git merge"s to update the current branch. But in my mind "fast-forward" is not a noun, it's an adjective, so I still expect a noun: fast-forward $what. And I don't have any other noun to put there but "merge". > > Perhaps: a merge, a rebase, or a fast-forward? > > Sure, that works; in fact, that's much better than my suggestion. I like it. Great. > > > > + "You can squelch this message by running one of the following commands:\n" > > > > + "\n" > > > > + " git config pull.rebase false # merge (the default strategy)\n" > > > > > > Should this be labelled as the default given the desire to make > > > --ff-only the default? Perhaps I'm jumping ahead and you plan to > > > change that later in this series. > > > > That's right. > > > > In the previous series which does indeed make "pull.mode=ff-only" the > > default [1], I do change the warning to specify the future default > > [2], but in that series the warnings is changed to: > > > > The pull was not fast-forward, in the future you will have to choose > > a merge, or a rebase. > > To squelch this message and maintain the current behavior, use: > > > > git config --global pull.mode merge > > > > To squelch this message and adopt the new behavior now, use: > > > > git config --global push.mode ff-only > > > > Falling back to old style for now (merge). > > Read "git pull --help" for more information. > > > > Since that series didn't get any traction, I decided to only implement > > step 1: fix the current situation. And later on another series would > > do step 2: introduce "pull.mode=ff-only" and do the preparations to > > make it the default. > > I like this longer plan. Yeah. It's a better plan, just more work for me. > However, on the shorter scale plan...I think > the suggestion to use "git pull --no-rebase" makes the current > situation worse rather than fixing it. Well, I already said I partly agree with you: in the --ff-only case the suggestion should not be brought forward. But in the "git pull" default case, *today* it's doing a merge. If uttering --merge and thus making the current behavior explicit instead of implicit seems dangerous it's because it is. But not documenting it doesn't make it any less dangerous. > > > > + " git config pull.rebase true # rebase\n" > > > > + " git config pull.ff only # fast-forward only\n" > > > > + "\n" > > > > + "You can replace \"git config\" with \"git config --global\" to set a default\n" > > > > + "preference for all repositories.\n" > > > > > > Good up to here. > > > > > > > + "If unsure, run \"git pull --no-rebase\".\n" > > > > > > Why is that safe to suggest? The original text may not have been the > > > easiest to parse, but this seems more problematic to me. > > > > Because "git pull" has been doing the same as "git pull --no-rebase" > > for more than a decade. It's safe for people to continue with this > > behavior for a few more months. > > > > Some people need to get things done today, and they are not interested > > in future changes, nor changing their default configuration, or what > > the warning has to say. > > > > They just want "git pull" to do the same as yesterday, and the year > > before, without being bothered with an annoying warning. > > > > Those people can start training their fingers to do "git pull > > --merge", and learn the problems with "git pull" later. > > > > We want to respect the user's time, and not force them to read the > > warning today. > > The warning was added because sending users down paths that break > things badly is a waste of user's time, and often a much bigger waste > of user's time than making them read up on the meaning behind the two > different choices of what kind of changes they can make. I agree the > warning went too far, but I fully agree with the original folks who > put this warning here that we need to have it for at least some cases > and that there is a decision to be made. Though I am just one voice, > and perhaps others will agree with you on your point here, I'll > continue to disagree with blindly suggesting "git pull --no-rebase". I think the warning is good. I implemented something like that years ago, and I believe Junio did so too. It should have been implemented differently though. And yes, we want to annoy the users, and nudge them in the right direction, but still leave them the *option* to easily ignore us. I'm against forcing anyone to do anything. If the user wants to analyze the warning, they can do so, if not, they can just do $cmd and go about their day as usual. But they were warned. Cheers.
On Fri, Dec 4, 2020 at 5:59 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > But in my mind "fast-forward" is not a noun, it's an adjective, so I > still expect a noun: fast-forward $what. And I don't have any other > noun to put there but "merge". It's a "fast-forward operation". :-) (The operands are branch name and hash ID.) It's just been nouned, like "a merge" instead of "a merge commit". > > > Perhaps: a merge, a rebase, or a fast-forward? > > > > Sure, that works; in fact, that's much better than my suggestion. I like it. I like this one too. In a perfect world, I'd have had `git pull` be the user facing command that does one of: fetch only, fetch-and-fast-forward, fetch-and-rebase, or fetch-and-merge. (Obviously one can achieve the fetch-only by running `git fetch`, but `git fetch` is a plumbing command.) In that same perfect world, the default probably would have been fetch only, but fetch-and-fast-forward (or fail if not fast-forward-able) seems like an OK default as well. But we have to deal with the imperfect world we have. This seems like an OK way to do that.
On Sat, Dec 5, 2020 at 3:23 AM Chris Torek <chris.torek@gmail.com> wrote: > > On Fri, Dec 4, 2020 at 5:59 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > But in my mind "fast-forward" is not a noun, it's an adjective, so I > > still expect a noun: fast-forward $what. And I don't have any other > > noun to put there but "merge". > > It's a "fast-forward operation". :-) A fast-forward operation that does what? > It's just been nouned, like "a merge" instead of "a merge commit". Except there's a difference between telling git: "do a fast-forward merge", and "do a fast-forward merge commit". The latter doesn't really make sense. > > > > Perhaps: a merge, a rebase, or a fast-forward? > > > > > > Sure, that works; in fact, that's much better than my suggestion. I like it. > > I like this one too. > > In a perfect world, I'd have had `git pull` be the user facing command > that does one of: fetch only, fetch-and-fast-forward, fetch-and-rebase, > or fetch-and-merge. (Obviously one can achieve the fetch-only by > running `git fetch`, but `git fetch` is a plumbing command.) > > In that same perfect world, the default probably would have been > fetch only, but fetch-and-fast-forward (or fail if not fast-forward-able) > seems like an OK default as well. But we have to deal with the > imperfect world we have. This seems like an OK way to do that. In the slightly more perfect word I'm aiming for those two modes could be achieved with the pull.mode configuration: * "pull.mode=none" would be the same as "fetch only" * "pull.mode=ff-only" would be the same as "fetch-and-fast-forward" But first this configuration has to actually be introduced. It didn't happen in 2014, maybe it will happen this time. Who knows. Cheers.
On Fri, Dec 4, 2020 at 5:56 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > On Fri, Dec 4, 2020 at 6:56 PM Elijah Newren <newren@gmail.com> wrote: > > > > Hi Felipe, > > > > On Fri, Dec 4, 2020 at 4:12 PM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > > > > > > On Fri, Dec 4, 2020 at 5:00 PM Elijah Newren <newren@gmail.com> wrote: > > > > > > > > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras > > > > <felipe.contreras@gmail.com> wrote: [...] > > > > > + "You can squelch this message by running one of the following commands:\n" > > > > > + "\n" > > > > > + " git config pull.rebase false # merge (the default strategy)\n" > > > > > > > > Should this be labelled as the default given the desire to make > > > > --ff-only the default? Perhaps I'm jumping ahead and you plan to > > > > change that later in this series. > > > > > > That's right. > > > > > > In the previous series which does indeed make "pull.mode=ff-only" the > > > default [1], I do change the warning to specify the future default > > > [2], but in that series the warnings is changed to: > > > > > > The pull was not fast-forward, in the future you will have to choose > > > a merge, or a rebase. > > > To squelch this message and maintain the current behavior, use: > > > > > > git config --global pull.mode merge > > > > > > To squelch this message and adopt the new behavior now, use: > > > > > > git config --global push.mode ff-only > > > > > > Falling back to old style for now (merge). > > > Read "git pull --help" for more information. > > > > > > Since that series didn't get any traction, I decided to only implement > > > step 1: fix the current situation. And later on another series would > > > do step 2: introduce "pull.mode=ff-only" and do the preparations to > > > make it the default. > > > > I like this longer plan. > > Yeah. It's a better plan, just more work for me. > > > However, on the shorter scale plan...I think > > the suggestion to use "git pull --no-rebase" makes the current > > situation worse rather than fixing it. > > Well, I already said I partly agree with you: in the --ff-only case > the suggestion should not be brought forward. > > But in the "git pull" default case, *today* it's doing a merge. If > uttering --merge and thus making the current behavior explicit instead > of implicit seems dangerous it's because it is. But not documenting it > doesn't make it any less dangerous. Sounds like we agree that the future should be ff-only-as-default. I also agree with you that the primary problem is the current default behavior, and I'll agree with you that documenting the current default is okay. However, I disagree that your wording here: + "If unsure, run \"git pull --no-rebase\".\n" does anything of the sort. It does not mention that this is the default behavior the users would get if they provided no flags. More importantly, it makes a recommendation...and one that undercuts the point of the message. It makes it feel like the message shouldn't exist at all in any circumstances. I even suspect that adding that sentence may undercut any efforts towards changing the default to ff-only-as-default. While I'm a big fan of most of what you've done in this series, I will object to its merging for as long as this stays. (I definitely don't have veto power or anything close to it, just stating what my opinion is.)
On Sat, Dec 5, 2020 at 10:28 AM Elijah Newren <newren@gmail.com> wrote: > On Fri, Dec 4, 2020 at 5:56 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > Well, I already said I partly agree with you: in the --ff-only case > > the suggestion should not be brought forward. > > > > But in the "git pull" default case, *today* it's doing a merge. If > > uttering --merge and thus making the current behavior explicit instead > > of implicit seems dangerous it's because it is. But not documenting it > > doesn't make it any less dangerous. > > Sounds like we agree that the future should be ff-only-as-default. I > also agree with you that the primary problem is the current default > behavior, and I'll agree with you that documenting the current default > is okay. However, I disagree that your wording here: > > + "If unsure, run \"git pull --no-rebase\".\n" > > does anything of the sort. It does not mention that this is the > default behavior the users would get if they provided no flags. But that is not the warning, this is the warning: Pulling without specifying how to reconcile divergent branches is discouraged; you need to specify if you want a merge, a rebase, or a fast-forward. You can squelch this message by running one of the following commands: git config pull.rebase false # merge (the default strategy) git config pull.rebase true # rebase git config pull.ff only # fast-forward only You can replace "git config" with "git config --global" to set a default preference for all repositories. If unsure, run "git pull --merge". Read "git pull --help" for more information. This warning says: 1. There's 3 options: merge, rebase, fast-forward 2. merge is the default strategy 3. If unsure, specify --merge (the default strategy) So taken altogether it does say what is the default strategy. > More > importantly, it makes a recommendation...and one that undercuts the > point of the message. So? When boarding a plane the flight attendants do a safety demonstration that passengers should pay attention to. If one passenger is not paying attention (listening to music on headphones, and reading a book) what should the crew do? 1. Remove the passenger's headphones and force him to pay attention to the safety demonstration 2. Let the passenger ignore the safety demonstration Human beings are independent agents responsible for their own actions. You as a separate human being--a crew member--can argue that it's not in the best interest of the passenger to ignore the safety demonstration, and you may be right, but the passenger decisions are still the passenger's decisions, even if they are bad. Do you think the crew should disregard the passenger's volition and force him to pay attention to the safety demonstration? > It makes it feel like the message shouldn't > exist at all in any circumstances. I even suspect that adding that > sentence may undercut any efforts towards changing the default to > ff-only-as-default. While I'm a big fan of most of what you've done > in this series, I will object to its merging for as long as this > stays. (I definitely don't have veto power or anything close to it, > just stating what my opinion is.) The current warning should not exist at all. The complaint from VÃt Ondruch [1] that reignited this series is a valid one. A *permanent* warning is not good. We should have a *temporary* warning with the express purpose of notifying users of an upcoming change. If we have not yet decided on what should be the default (Junio seems to have casted some doubt on the consensus [2]), and we don't have a clear path forward to implement such change (we can't even tell users to use "pull.ff=only", since eventually it may be "pull.mode=ff-only"), then we must remove the warning. It was a mistake to put a *permanent* warning before deciding to change the default. So, there's two options: 1. We decide on a path forward and fix the warning so it *temporarily* explains what will happen in the future 2. We remove the *permanent* warning Since we are already here, we might as well take advantage of that warning and repurpose it. But in the meantime--while the git project decides what to do, and what configurations to suggest the users to change--we should at the very least waste as little as the user's time as possible, and give him/her a quick opt-out. Yes, a quick opt-out defeats the purpose of a warning, but we must respect the users' volition. The user may be on a deadline trying to push some changes to production before the weekend, and after a system update be annoyed with this warning on every pull. The user may not have time to look at the warning, decide he wants to read the warning in the future, maybe next Monday, and thus not configure anything to silence it. What's wrong with a user saying "I don't have time for this now, please tell me what to do for now, I'll look at the warning later"? If anything for those users the configuration is the wrong thing to do, because being in a hurry they just choose the first configuration and forget about the warning without actually looking at it (because they didn't have time), and it will not appear any more. By typing "git pull --merge" the user can get rid of the warning *for now*, but the next time he does "git pull" the warning will reappear, and at that time perhaps the user does have the time to read it, and look at the manpage. Nobody likes their workflow to be interrupted and be forced to do anything. I don't think my patches plus that suggestion for a quick opt-out are in any way worse than the current situation. If you think they are, then we'll just have to agree to disagree. I quote the voice of VÃt Ondruch, which I think represents the typical user: "please select any strategy considered more appropriate and stop warning me". Cheers. [1] https://lore.kernel.org/git/742df4c2-2bc5-8a4b-8de1-cd5e48718398@redhat.com/ [2] https://lore.kernel.org/git/xmqqh7p1fjml.fsf@gitster.c.googlers.com/
On Sat, Dec 5, 2020 at 1:28 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > On Sat, Dec 5, 2020 at 10:28 AM Elijah Newren <newren@gmail.com> wrote: > > On Fri, Dec 4, 2020 at 5:56 PM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > > > > Well, I already said I partly agree with you: in the --ff-only case > > > the suggestion should not be brought forward. > > > > > > But in the "git pull" default case, *today* it's doing a merge. If > > > uttering --merge and thus making the current behavior explicit instead > > > of implicit seems dangerous it's because it is. But not documenting it > > > doesn't make it any less dangerous. > > > > Sounds like we agree that the future should be ff-only-as-default. I > > also agree with you that the primary problem is the current default > > behavior, and I'll agree with you that documenting the current default > > is okay. However, I disagree that your wording here: > > > > + "If unsure, run \"git pull --no-rebase\".\n" > > > > does anything of the sort. It does not mention that this is the > > default behavior the users would get if they provided no flags. > > But that is not the warning, this is the warning: > > Pulling without specifying how to reconcile divergent branches is discouraged; > you need to specify if you want a merge, a rebase, or a fast-forward. > You can squelch this message by running one of the following commands: > > git config pull.rebase false # merge (the default strategy) > git config pull.rebase true # rebase > git config pull.ff only # fast-forward only > > You can replace "git config" with "git config --global" to set a default > preference for all repositories. > If unsure, run "git pull --merge". > Read "git pull --help" for more information. > > This warning says: > > 1. There's 3 options: merge, rebase, fast-forward > 2. merge is the default strategy > 3. If unsure, specify --merge (the default strategy) > > So taken altogether it does say what is the default strategy. We don't need to take them together. #2 by itself states the default strategy. I don't see why defending #3 as being for the purpose of documenting the default strategy is helpful, since it doesn't do that. > > More > > importantly, it makes a recommendation...and one that undercuts the > > point of the message. > > So? > > When boarding a plane the flight attendants do a safety demonstration > that passengers should pay attention to. If one passenger is not > paying attention (listening to music on headphones, and reading a > book) what should the crew do? > > 1. Remove the passenger's headphones and force him to pay attention to > the safety demonstration > 2. Let the passenger ignore the safety demonstration > > Human beings are independent agents responsible for their own actions. > You as a separate human being--a crew member--can argue that it's not > in the best interest of the passenger to ignore the safety > demonstration, and you may be right, but the passenger decisions are > still the passenger's decisions, even if they are bad. > > Do you think the crew should disregard the passenger's volition and > force him to pay attention to the safety demonstration? > > > It makes it feel like the message shouldn't > > exist at all in any circumstances. I even suspect that adding that > > sentence may undercut any efforts towards changing the default to > > ff-only-as-default. While I'm a big fan of most of what you've done > > in this series, I will object to its merging for as long as this > > stays. (I definitely don't have veto power or anything close to it, > > just stating what my opinion is.) > > The current warning should not exist at all. > > The complaint from VÃt Ondruch [1] that reignited this series is a > valid one. A *permanent* warning is not good. We should have a > *temporary* warning with the express purpose of notifying users of an > upcoming change. > > If we have not yet decided on what should be the default (Junio seems > to have casted some doubt on the consensus [2]), and we don't have a Useful link there. Based on his comments, we may want to make --ff-only, --merge, and --rebase all be mutually exclusive and result in an error message if more than one is specified at the command line. (But still have the command line countermand any option set in the config, of course). > clear path forward to implement such change (we can't even tell users > to use "pull.ff=only", since eventually it may be > "pull.mode=ff-only"), then we must remove the warning. > > It was a mistake to put a *permanent* warning before deciding to > change the default. > > So, there's two options: > > 1. We decide on a path forward and fix the warning so it *temporarily* > explains what will happen in the future > 2. We remove the *permanent* warning > > Since we are already here, we might as well take advantage of that > warning and repurpose it. But in the meantime--while the git project > decides what to do, and what configurations to suggest the users to > change--we should at the very least waste as little as the user's time > as possible, and give him/her a quick opt-out. > > Yes, a quick opt-out defeats the purpose of a warning, but we must > respect the users' volition. The user may be on a deadline trying to > push some changes to production before the weekend, and after a system > update be annoyed with this warning on every pull. The user may not > have time to look at the warning, decide he wants to read the warning > in the future, maybe next Monday, and thus not configure anything to > silence it. > > What's wrong with a user saying "I don't have time for this now, > please tell me what to do for now, I'll look at the warning later"? If > anything for those users the configuration is the wrong thing to do, > because being in a hurry they just choose the first configuration and > forget about the warning without actually looking at it (because they > didn't have time), and it will not appear any more. By typing "git > pull --merge" the user can get rid of the warning *for now*, but the > next time he does "git pull" the warning will reappear, and at that > time perhaps the user does have the time to read it, and look at the > manpage. > > Nobody likes their workflow to be interrupted and be forced to do anything. > > I don't think my patches plus that suggestion for a quick opt-out are > in any way worse than the current situation. If you think they are, > then we'll just have to agree to disagree. Yes, we can agree to disagree on this particular point.
On Sat, Dec 5, 2020 at 7:01 PM Elijah Newren <newren@gmail.com> wrote: > > On Sat, Dec 5, 2020 at 1:28 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > On Sat, Dec 5, 2020 at 10:28 AM Elijah Newren <newren@gmail.com> wrote: > > > On Fri, Dec 4, 2020 at 5:56 PM Felipe Contreras > > > <felipe.contreras@gmail.com> wrote: > > > > > > Well, I already said I partly agree with you: in the --ff-only case > > > > the suggestion should not be brought forward. > > > > > > > > But in the "git pull" default case, *today* it's doing a merge. If > > > > uttering --merge and thus making the current behavior explicit instead > > > > of implicit seems dangerous it's because it is. But not documenting it > > > > doesn't make it any less dangerous. > > > > > > Sounds like we agree that the future should be ff-only-as-default. I > > > also agree with you that the primary problem is the current default > > > behavior, and I'll agree with you that documenting the current default > > > is okay. However, I disagree that your wording here: > > > > > > + "If unsure, run \"git pull --no-rebase\".\n" > > > > > > does anything of the sort. It does not mention that this is the > > > default behavior the users would get if they provided no flags. > > > > But that is not the warning, this is the warning: > > > > Pulling without specifying how to reconcile divergent branches is discouraged; > > you need to specify if you want a merge, a rebase, or a fast-forward. > > You can squelch this message by running one of the following commands: > > > > git config pull.rebase false # merge (the default strategy) > > git config pull.rebase true # rebase > > git config pull.ff only # fast-forward only > > > > You can replace "git config" with "git config --global" to set a default > > preference for all repositories. > > If unsure, run "git pull --merge". > > Read "git pull --help" for more information. > > > > This warning says: > > > > 1. There's 3 options: merge, rebase, fast-forward > > 2. merge is the default strategy > > 3. If unsure, specify --merge (the default strategy) > > > > So taken altogether it does say what is the default strategy. > > We don't need to take them together. No, but that's what I'm proposing. > #2 by itself states the default strategy. Yes, with configuration, not with commands. > I don't see why defending #3 as being for the purpose of > documenting the default strategy is helpful, since it doesn't do that. I disagree. It does do that, but it serves a more urgent purpose: it tells the user how to ignore us quickly, for now. > > The current warning should not exist at all. > > > > The complaint from VÃt Ondruch [1] that reignited this series is a > > valid one. A *permanent* warning is not good. We should have a > > *temporary* warning with the express purpose of notifying users of an > > upcoming change. > > > > If we have not yet decided on what should be the default (Junio seems > > to have casted some doubt on the consensus [2]), and we don't have a > > Useful link there. Based on his comments, we may want to make > --ff-only, --merge, and --rebase all be mutually exclusive and result > in an error message if more than one is specified at the command line. > (But still have the command line countermand any option set in the > config, of course). Yes, but that's a change in behavior. Moreover, what do you suggest this should do? git -c "pull.rebase=true" -c "pull.ff=only" pull I have already tried to make pull.ff=only work in several ways. It's just not a clean solution. Cheers.
Elijah Newren <newren@gmail.com> writes: >> Pulling without specifying how to reconcile divergent branches is discouraged; >> you need to specify if you want a merge, a rebase, or a fast-forward. >> You can squelch this message by running one of the following commands: >> >> git config pull.rebase false # merge (the default strategy) >> git config pull.rebase true # rebase >> git config pull.ff only # fast-forward only >> >> You can replace "git config" with "git config --global" to set a default >> preference for all repositories. >> If unsure, run "git pull --merge". >> Read "git pull --help" for more information. >> >> This warning says: >> >> 1. There's 3 options: merge, rebase, fast-forward >> 2. merge is the default strategy >> 3. If unsure, specify --merge (the default strategy) >> >> So taken altogether it does say what is the default strategy. > > We don't need to take them together. #2 by itself states the default > strategy. I don't see why defending #3 as being for the purpose of > documenting the default strategy is helpful, since it doesn't do that. Would it work if the line were If unsure, run "git -c pull.rebase=false pull". then? The reason why I bring this up is because the first part only talks about the choices in the terms of configuration variables, and for those who are afraid of committing, there is no easy way, unless they know the "git -c" single-shot mechanism, to "go ahead with one choice in an experimental basis to see if that is what they want", and "specify --merge" does not click well with "pull.rebase=false" that is given in the first part. "git pull --no-rebase" may also work better than "git pull --merge" from that point of view, though. But see below. >> Do you think the crew should disregard the passenger's volition and >> force him to pay attention to the safety demonstration? An example that is irrelevant. Passengers who sit on the aisle side and clutter the floor with their luggages can pose public hazard, and it is quite sensible for crews to remove them from the plane. A team member who runs "pull --[no-]rebase" pushes the result, which may be a new history in a wrong shape, back to the shared repository probably falls into a different category. ... Or perhaps in the same public hazard category? > Useful link there. Based on his comments, we may want to make > --ff-only, --merge, and --rebase all be mutually exclusive and result > in an error message if more than one is specified at the command line. I hope "his comment" does not refer to what I said. Redefining the semantics of --ff-only (but not --ff=no and friends) to make it mutually incompatible with merge/rebase was what Felipe wanted to do, not me. FWIW, I see the general direction as - When the history you are pulling is not a descendant of what you have on your current branch, you must show your preference between merging their history into yours or rebasing your history onto theirs. - In such a case, we error out, with an error message telling them that they must tell us which one they want in this invocation (e.g. with "--rebase" or "--no-rebase"). We further tell them that pull.rebase can be used to record their preference, if they almost always use the same. Side note: I earlier said "see below" to refer to this. I think the message should offer what they can do right now with command line option first, and then give an option to configure. IOW, the message quoted in the beginning of this response gives information in a wrong order. Also, "discouraged" is misleading, isn't it? When we give this message, we refuse to proceed until the user specifies. Another thing to consider is that pull.ff=only (and --ff-only) may be wrong choice to offer in the error message quoted above, if one of the objectives is to reduce the "annoying" message that tells the user that they must choose. By definition, we do not give the message when our side do not have our own development, so "I could run 'pull --ff-only'" is a unusable knowledge to those who see the message and want to get out of the situation. - But when the history being pulled is a descendant of the current branch, and the user does not give us preference between merge and rebase (either from the command line or with configuration), there is no need to give such an error message---if you do not have your own development, we can just fast-forward the current branch to what we fetched from the other side. This does not happen with the current system, which is what we want to fix. - Also when the history from the other side is a descedant and the user has preference, either to rebase or to merge, either from the command line or with configuration, we can fast-forward the current branch to their tip, but there are wrinkles: . when --ff=no is given and the choice is to merge (not rebase), we must create an extra merge commit. . when the choice is to rebase, by definition, rebasing what you have on top of theirs in this case is the same as fast-forwarding the current branch to theirs, because "what you have" is an empty set. . I am not sure how "pull --rebase --ff=no" should interact if we don't have our own development. Rebasing our work on top of theirs is philosophically incompatible with marking where the side branch begins and ends with an extra commit, so it either should be rejected when the command line and configuration is parsed (i.e. regardless of the shape of the history we have at hand), or --ff=no gets silently ignored when --rebase is given. I haven't thought this one through.
On Mon, Dec 7, 2020 at 1:26 AM Junio C Hamano <gitster@pobox.com> wrote: > Elijah Newren <newren@gmail.com> writes: > >> Do you think the crew should disregard the passenger's volition and > >> force him to pay attention to the safety demonstration? > > An example that is irrelevant. Passengers who sit on the aisle side > and clutter the floor with their luggages can pose public hazard, > and it is quite sensible for crews to remove them from the plane. That's a completely different example that mixes policy with regulations. Apples and oranges. > A team member who runs "pull --[no-]rebase" pushes the result, which > may be a new history in a wrong shape, back to the shared repository > probably falls into a different category. ... Or perhaps in the > same public hazard category? Yes, but if it's a public hazard or not is up to the maintainers of specific projects. Some projects are perfectly fine with those merges. It's the maintainer of the project that should decide what is the best policy for the project, and instruct his/her contributors. It's not up for individuals to decide what is a public hazard, and it's not up to Git either. Similarly; passengers can't decide what raises to the level of public hazzard, but neither can a flight attendant. > > Useful link there. Based on his comments, we may want to make > > --ff-only, --merge, and --rebase all be mutually exclusive and result > > in an error message if more than one is specified at the command line. > > I hope "his comment" does not refer to what I said. Redefining the > semantics of --ff-only (but not --ff=no and friends) to make it > mutually incompatible with merge/rebase was what Felipe wanted to > do, not me. That's not true. What I want is to introduce a "pull.mode=ff-only", not change the semantics of --ff-only. What I have been saying is that you can't make --ff-only the default without changing its semantics. But since the patch series that introduced such pull.mode [1] was completely ignored, I decided to take a shot at making --ff-only work. You said [2]: > > What we want to see can be done without such backward incompatible > > changes, e.g. declaring the combination of "--ff-only" and > > "--[no-]rebase" incompatible and/or the last one wins, I would say, and > > I suspect Alex's RFC was an attempt to make the first step in that > > direction. But in all my attempts the desired result cannot be done without backwards incompatible changes. I'm just the messenger. To be crystal clear: I don't want to change the semantics of --ff-only; I want to introduce a new "pull.mode=ff-only". > FWIW, I see the general direction as > > - When the history you are pulling is not a descendant of what you > have on your current branch, you must show your preference > between merging their history into yours or rebasing your history > onto theirs. Agreed. > - In such a case, we error out, with an error message telling them > that they must tell us which one they want in this invocation > (e.g. with "--rebase" or "--no-rebase"). We further tell them > that pull.rebase can be used to record their preference, if they > almost always use the same. Not quite. See below. > Side note: I earlier said "see below" to refer to this. I > think the message should offer what they can do right now > with command line option first, and then give an option to > configure. IOW, the message quoted in the beginning of this > response gives information in a wrong order. Also, > "discouraged" is misleading, isn't it? When we give this > message, we refuse to proceed until the user specifies. That's why I keep pointing out the end goal. In the end it's not one message, it's *two*: You can see my suggestion for both messages in my patch series for pull.mode [1]. When the user has specified "pull.mode=ff-only", the command dies with: The pull was not fast-forward, please either merge or rebase. When the user has not specified any mode (by default), the command would output a warning: The pull was not fast-forward, in the future you will have to choose a merge, or a rebase. To squelch this message and maintain the current behavior, use: git config --global pull.mode merge To squelch this message and adopt the new behavior now, use: git config --global push.mode ff-only Falling back to old style for now (merge). Read 'git pull --help' for more information. But in order to reach that point we first need to decide what will be the default mode, and how it will be configured (--ff-only or "pull.mode=ff-only"). In the meantime this particular series is not committing for any particular decision, and thus it's changing the warning message for the *current* situation in which the command does not fail, but only shows a warning. So today it's merely discouraged. This particular patch is pretty far from the end goal. It's just patch #2 of part 1. > Another thing to consider is that pull.ff=only (and > --ff-only) may be wrong choice to offer in the error message > quoted above, if one of the objectives is to reduce the > "annoying" message that tells the user that they must > choose. By definition, we do not give the message when our > side do not have our own development, so "I could run 'pull > --ff-only'" is a unusable knowledge to those who see the > message and want to get out of the situation. Those commands will squelch the warning, but won't change the underlying nature of the warning, which is that the user must choose to either merge or rebase. The temporary warning will disappear, but the permanent error message will not: The pull was not fast-forward, please either merge or rebase. > - But when the history being pulled is a descendant of the current > branch, and the user does not give us preference between merge > and rebase (either from the command line or with configuration), > there is no need to give such an error message---if you do not > have your own development, we can just fast-forward the current > branch to what we fetched from the other side. This does not > happen with the current system, which is what we want to fix. It's not an error message, it's a warning. But yes. > - Also when the history from the other side is a descedant and the > user has preference, either to rebase or to merge, either from > the command line or with configuration, we can fast-forward the > current branch to their tip, but there are wrinkles: > > . when --ff=no is given and the choice is to merge (not rebase), > we must create an extra merge commit. --ff receives no arguments, it's --no-ff. Yes. > . when the choice is to rebase, by definition, rebasing what you > have on top of theirs in this case is the same as fast-forwarding > the current branch to theirs, because "what you have" is an > empty set. Agreed. > . I am not sure how "pull --rebase --ff=no" should interact if we > don't have our own development. Rebasing our work on top of > theirs is philosophically incompatible with marking where the > side branch begins and ends with an extra commit, so it either > should be rejected when the command line and configuration is > parsed (i.e. regardless of the shape of the history we have at > hand), or --ff=no gets silently ignored when --rebase is given. > I haven't thought this one through. I thought of this instance as well. In my opinion such combination should fail. But it doesn't actually affect anything. Just to put this series in context: it's only part 1; it does not introduce pull.mode, and it doesn't make --ff-only the default. There's one patch prefixed with "tentative" that changes the semantics of --ff-only to see how it could look like when making it the default, but in no way am I proposing that patch to be merged. The series is entirely about improving the current situation, nothing contentious. In part 2 I will introduce pull.mode, which is the solution I propose, and that would be basically rebasing [1] on top of this series (hopefully). I would have hoped that by this point it would have been clear why --ff-only is not the way forward. But oh well, we'll see what happens. Cheers. [1] https://lore.kernel.org/git/20201125032938.786393-1-felipe.contreras@gmail.com/ [2] https://lore.kernel.org/git/xmqqpn3qfhps.fsf@gitster.c.googlers.com
On Fri, Dec 4, 2020 at 6:56 PM Elijah Newren <newren@gmail.com> wrote: > > > > diff --git a/builtin/pull.c b/builtin/pull.c > > > > index 1034372f8b..22a9ffcade 100644 > > > > --- a/builtin/pull.c > > > > +++ b/builtin/pull.c > > > > @@ -346,17 +346,18 @@ static enum rebase_type config_get_rebase(void) > > > > > > > > if (opt_verbosity >= 0 && !opt_ff) { > > > > advise(_("Pulling without specifying how to reconcile divergent branches is\n" > > > > - "discouraged. You can squelch this message by running one of the following\n" > > > > - "commands sometime before your next pull:\n" > > > > - "\n" > > > > - " git config pull.rebase false # merge (the default strategy)\n" > > > > - " git config pull.rebase true # rebase\n" > > > > - " git config pull.ff only # fast-forward only\n" > > > > - "\n" > > > > - "You can replace \"git config\" with \"git config --global\" to set a default\n" > > > > - "preference for all repositories. You can also pass --rebase, --no-rebase,\n" > > > > - "or --ff-only on the command line to override the configured default per\n" > > > > - "invocation.\n")); > > > > + "discouraged; you need to specify if you want a merge, or a rebase.\n" > > > > > > ...want a merge, a rebase, or neither. > > > > There is no "git pull --no-merge". Years ago some people argued for a > > "pull.mode=none" (essentially making "git pull" the same as "git > > fetch"). But right now there's no option to do that. > > > > There's an option to do --ff-only, but that's still a merge. > > I disagree. I'm well aware that checkout_fast_forward() (which is > what is ultimately called to do the fast-forwarding) is in a file > called merge.c, but that doesn't make it a merge. I don't believe it > was anything more than a convenient place to dump some extra code at > the time. > > > Perhaps: a merge, a rebase, or a fast-forward? > > Sure, that works; in fact, that's much better than my suggestion. I like it. Actually no. If we are talking about divergent branches you cannot fast-forward. The original text was fine.
Felipe Contreras <felipe.contreras@gmail.com> writes: > When the user has specified "pull.mode=ff-only", the command dies with: > > The pull was not fast-forward, please either merge or rebase. > > When the user has not specified any mode (by default), the command > would output a warning: > > The pull was not fast-forward, in the future you will have to choose a > merge, or a rebase. I quite don't get it. They say the same thing. At that point the user needs to choose between merge and rebase to move forward and ff-only would not be a sensible choice to offer the user. > Just to put this series in context: it's only part 1; it does not > introduce pull.mode, and it doesn't make --ff-only the default. I'd view the "in a non-fast-forward situation, the warning kicks in to those who haven't chosen between merge and rebase (i.e. no pull.rebase set to either true or false, and pull.ff not set to only), which is a bit more gentle than the current situtation" a good stopping point. That state is already making ff-only the default for unconfigured users, or you can view it as shipping "git pull" in a shape that has the more dangerous half of its feature disabled to avoid hurting users. So I am not sure why you keep saying you do not have --ff-only as the default.
On Mon, Dec 7, 2020 at 12:16 PM Junio C Hamano <gitster@pobox.com> wrote: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > When the user has specified "pull.mode=ff-only", the command dies with: > > > > The pull was not fast-forward, please either merge or rebase. > > > > When the user has not specified any mode (by default), the command > > would output a warning: > > > > The pull was not fast-forward, in the future you will have to choose a > > merge, or a rebase. > > I quite don't get it. They say the same thing. At that point the > user needs to choose between merge and rebase to move forward and > ff-only would not be a sensible choice to offer the user. The error happens *only* when the user has explicitly set "pull.mode=ff-only", and the pull dies. At that point; yes, the user must choose between one or the other. But the warning happens by default, when the user has not chosen any mode, and the pull succeeds. The user doesn't need to choose; the pull continues as if the user chose the "merge" mode, which is the historic behavior. They start by saying the same thing. But one errors out and says the user must choose, and the other warns that in the future the user must choose. > > Just to put this series in context: it's only part 1; it does not > > introduce pull.mode, and it doesn't make --ff-only the default. > > I'd view the "in a non-fast-forward situation, the warning kicks in > to those who haven't chosen between merge and rebase (i.e. no > pull.rebase set to either true or false, and pull.ff not set to > only), which is a bit more gentle than the current situtation" a > good stopping point. That state is already making ff-only the > default for unconfigured users, or you can view it as shipping "git > pull" in a shape that has the more dangerous half of its feature > disabled to avoid hurting users. So I am not sure why you keep > saying you do not have --ff-only as the default. The warning doesn't make the pull fail, ff-only does. On the current master this works: test_expect_success 'non-fast-forward (default)' ' setup_non_ff && git pull ' And after my patch series (part 1) it still does. The default behavior has not changed. I'm not sure what it is that I'm not explaining correctly.
Felipe Contreras <felipe.contreras@gmail.com> writes: > They start by saying the same thing. But one errors out and says the > user must choose, and the other warns that in the future the user must > choose. Then I do not see the point in giving the warning---even in the future they do not have to choose as long as they are merely following along. >> > Just to put this series in context: it's only part 1; it does not >> > introduce pull.mode, and it doesn't make --ff-only the default. >> >> I'd view the "in a non-fast-forward situation, the warning kicks in >> to those who haven't chosen between merge and rebase (i.e. no >> pull.rebase set to either true or false, and pull.ff not set to >> only), which is a bit more gentle than the current situtation" a >> good stopping point. That state is already making ff-only the >> default for unconfigured users, or you can view it as shipping "git >> pull" in a shape that has the more dangerous half of its feature >> disabled to avoid hurting users. So I am not sure why you keep >> saying you do not have --ff-only as the default. > > The warning doesn't make the pull fail, ff-only does. Then probably you are giving an error and a warning at a wrong place. - When history fast-forwards, and the user hasn't chosen between rebase or merge, there is no need to give any warning. Just succeed by fast-forwarding. - When history does not fast-forward and the user hasn't chosen between rebase or merge, whether pull.ff is set to "only" or not, we should fail and the error message can instruct the user to choose between rebase and merge; there is no "ff-only" option that is useful in the situation. And that essentially makes the "ff-only" mode the safe default that castrates one half of the feature (the more dangerous half) of "git pull". Why do we make it more complicated than that by warning that the user must choose in the future? They will see an error tell them that when they start pulling while on their own work, and I do not see a need to bother them before that point.
On Mon, Dec 7, 2020 at 1:53 PM Junio C Hamano <gitster@pobox.com> wrote: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > They start by saying the same thing. But one errors out and says the > > user must choose, and the other warns that in the future the user must > > choose. > > Then I do not see the point in giving the warning---even in the > future they do not have to choose as long as they are merely > following along. They don't. With my patch series they see the warning only when the pull is non-fast-forward. > >> > Just to put this series in context: it's only part 1; it does not > >> > introduce pull.mode, and it doesn't make --ff-only the default. > >> > >> I'd view the "in a non-fast-forward situation, the warning kicks in > >> to those who haven't chosen between merge and rebase (i.e. no > >> pull.rebase set to either true or false, and pull.ff not set to > >> only), which is a bit more gentle than the current situtation" a > >> good stopping point. That state is already making ff-only the > >> default for unconfigured users, or you can view it as shipping "git > >> pull" in a shape that has the more dangerous half of its feature > >> disabled to avoid hurting users. So I am not sure why you keep > >> saying you do not have --ff-only as the default. > > > > The warning doesn't make the pull fail, ff-only does. > > Then probably you are giving an error and a warning at a wrong > place. > > - When history fast-forwards, and the user hasn't chosen between > rebase or merge, there is no need to give any warning. Just > succeed by fast-forwarding. Yes. That's what my patch [1] in this series does. > - When history does not fast-forward and the user hasn't chosen > between rebase or merge, whether pull.ff is set to "only" or not, > we should fail and the error message can instruct the user to > choose between rebase and merge; there is no "ff-only" option > that is useful in the situation. Yes. *Eventually*, that's part 3. > And that essentially makes the "ff-only" mode the safe default that > castrates one half of the feature (the more dangerous half) of "git > pull". Why do we make it more complicated than that by warning that > the user must choose in the future? They will see an error tell > them that when they start pulling while on their own work, and I do > not see a need to bother them before that point. Because the amount of time users have seen the correct error message telling them about this upcoming change is *zero*. No one is aware of this backwards-incompatible change that is upcoming, since the current warning message doesn't specify any such future change. Moreover, no one has configured their pull.mode, because no one has seen the message that tells them to do so. And no one has had time to try out the upcoming "pull.mode=ff-only". They haven't had time to find bugs on it, or weird interactions, or suggestions how to improve it. Because the code has not been deployed, and no warning has told them to tentatively enable that mode. When the default for push.default was changed, it was a good thing that we gave users (and ourselves) a grace period to try it out before permanently flipping the switch. The transition went smoothly. I think the progression of the warning is hard to see from all the patch series. I'm sending all my patches and I will explain in a timeline how it progresses until eventually we reach the desired end goal. Cheers. [1] https://lore.kernel.org/git/20201204061623.1170745-8-felipe.contreras@gmail.com/
On Mon, Dec 7, 2020 at 11:53 AM Junio C Hamano <gitster@pobox.com> wrote: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > They start by saying the same thing. But one errors out and says the > > user must choose, and the other warns that in the future the user must > > choose. > > Then I do not see the point in giving the warning---even in the > future they do not have to choose as long as they are merely > following along. > I think the key point is that this "in the future" is referring to "a future version of git will make this an error" This might be better if it said something like "The pull was not fast-forward. In a future version of git you will need to specify whether to merge or rebase, using pull.mode" or something similar. In theory, this warning will go away once that future version of git changes so that pull.mode defaults to ff-only. The difference being that a warning will allow the command to continue doing the default of today (merging), where as an error will stop the command essentially just after the fetch portion finishes, without changing the branch. Thanks, Jake > >> > Just to put this series in context: it's only part 1; it does not > >> > introduce pull.mode, and it doesn't make --ff-only the default. > >> > >> I'd view the "in a non-fast-forward situation, the warning kicks in > >> to those who haven't chosen between merge and rebase (i.e. no > >> pull.rebase set to either true or false, and pull.ff not set to > >> only), which is a bit more gentle than the current situtation" a > >> good stopping point. That state is already making ff-only the > >> default for unconfigured users, or you can view it as shipping "git > >> pull" in a shape that has the more dangerous half of its feature > >> disabled to avoid hurting users. So I am not sure why you keep > >> saying you do not have --ff-only as the default. > > > > The warning doesn't make the pull fail, ff-only does. > > Then probably you are giving an error and a warning at a wrong > place. > > - When history fast-forwards, and the user hasn't chosen between > rebase or merge, there is no need to give any warning. Just > succeed by fast-forwarding. > Correct. > - When history does not fast-forward and the user hasn't chosen > between rebase or merge, whether pull.ff is set to "only" or not, > we should fail and the error message can instruct the user to > choose between rebase and merge; there is no "ff-only" option > that is useful in the situation. > Yes, I understand that this is the destination Felipe wants us to end up at. > And that essentially makes the "ff-only" mode the safe default that > castrates one half of the feature (the more dangerous half) of "git > pull". Why do we make it more complicated than that by warning that > the user must choose in the future? They will see an error tell > them that when they start pulling while on their own work, and I do > not see a need to bother them before that point. The warning would be in an earlier version of git before that is the default. The warning is for a transition period between today when we merge by default (with a warning) and when we fail with an error without completing the pull. Once the default is the ff-only mode, then the warning is no longer necessary, as we will get an error indicating that you must choose. Thanks, Jake
Jacob Keller <jacob.keller@gmail.com> writes: > I think the key point is that this "in the future" is referring to "a > future version of git will make this an error" > > This might be better if it said something like "The pull was not > fast-forward. In a future version of git you will need to specify > whether to merge or rebase, using pull.mode" Oh, I've actually been assuming that the current "warn but go ahead anyway asssuming the preference is to merge" can just be declared as a bug (iow, there is no need to say 'in the future'---we'd fix the bug right away). > or something similar. In theory, this warning will go away once that > future version of git changes so that pull.mode defaults to ff-only. > > The difference being that a warning will allow the command to continue > doing the default of today (merging), where as an error will stop the > command essentially just after the fetch portion finishes, without > changing the branch. Yup. If we want to take things slow, that is fine by me as well, but I am not sure if that is even necessary, given how annoying the existing "loudly warn but still go ahead" behaviour is, and how easy for existing users to have squelched the annoyance by choosing between rebase and merge already. I've always assumed that any existing users who started using Git in the past several years have already set pull.rebase to one or the other value and they won't be affected by fixing "git pull" to just error out.
On Mon, Dec 7, 2020 at 8:23 PM Junio C Hamano <gitster@pobox.com> wrote: > > Jacob Keller <jacob.keller@gmail.com> writes: > > > I think the key point is that this "in the future" is referring to "a > > future version of git will make this an error" > > > > This might be better if it said something like "The pull was not > > fast-forward. In a future version of git you will need to specify > > whether to merge or rebase, using pull.mode" > > Oh, I've actually been assuming that the current "warn but go ahead > anyway asssuming the preference is to merge" can just be declared as > a bug (iow, there is no need to say 'in the future'---we'd fix the > bug right away). It is a bug, in my opinion. If we were not planning on changing the default, I would say drop the warning altogether. *But*, if we are going to change the default, the warning can be repurposed to say "in the future this will fail". That requires other changes though. > > or something similar. In theory, this warning will go away once that > > future version of git changes so that pull.mode defaults to ff-only. > > > > The difference being that a warning will allow the command to continue > > doing the default of today (merging), where as an error will stop the > > command essentially just after the fetch portion finishes, without > > changing the branch. Exactly. And we can choose between those behaviors with a configuration, like it happened with push.default. > Yup. If we want to take things slow, that is fine by me as well, > but I am not sure if that is even necessary, given how annoying the > existing "loudly warn but still go ahead" behaviour is, and how easy > for existing users to have squelched the annoyance by choosing > between rebase and merge already. I've always assumed that any > existing users who started using Git in the past several years have > already set pull.rebase to one or the other value and they won't be > affected by fixing "git pull" to just error out. Existing users that are using up-to-date distributions, maybe. Debian stable is using v2.20.1. In general it's not a good idea to assume anything about our users. Recently a user of Oh My Zsh reported an issue with the backwards compatibility code of git-completion; he was using v2.17 I think. That is exemplified by the fact that this whole thread started from a user that refused to configure pull.rebase and expected the Git project to just do the right thing (which is basically choosing a useful default). Cheers.
Felipe Contreras <felipe.contreras@gmail.com> writes: > That is exemplified by the fact that this whole thread started from a > user that refused to configure pull.rebase and expected the Git > project to just do the right thing (which is basically choosing a > useful default). Which is basically asking for impossible, and I do not think it is a good idea to focus our effort to satisfy such a request in general. There is no useful default that suits everybody in this particular case. The "force ff-only" approach indeed gives a very sensible default behaviour of dying for those who haven't expressed the choice between rebase and merge in a situation where the difference between the two results in histories of different shapes. But for anybody who uses git for real (read: produces his or her own history), it would be pretty much a useless default that forces the user to say rebase or merge every time 'git pull' is run. That is why I am not enthused by the pull.mode=(rebase/merge/ff-only) configuration. The third choice does help completeness. When a user asks "the documentation tells pull.mode can be set to non-default behaviour---what value can one set it to to get the default behaviour of not allowing any original work?", it can be answered if we had pull.mode=ff-only. But other than that, I do not see any real use for the choice, which would mean in practice, pull.mode would have only two useful values, rebase or merge. That does not feel a good enough reason to supersede what already exists, which is pull.rebase=yes/no. Perhaps there is a good reason why certain classes of users would want to configure pull.mode=ff-only (i.e. "I might type 'git pull' by mistake, please stop me if I did so on a branch I have real work already."). If that is the case, I would very much agree that it would be awkward to express that choice in the current framework to choose between pull.rebase=yes/no and pull.mode=(rebase/merge/ff-only) would become a lot easier to explain. I dunno.
On Tue, Dec 8, 2020 at 2:16 PM Junio C Hamano <gitster@pobox.com> wrote: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > That is exemplified by the fact that this whole thread started from a > > user that refused to configure pull.rebase and expected the Git > > project to just do the right thing (which is basically choosing a > > useful default). > > Which is basically asking for impossible, and I do not think it is a > good idea to focus our effort to satisfy such a request in general. > There is no useful default that suits everybody in this particular > case. I think I already made this point, but this is the nirvana fallacy (the perfect is the enemy of the good) [1]. Just because we can't have the perfect solution doesn't mean we can't pursue something better than the current state. What was asked was not a perfect solution, just a better default. If right now the default is good enough for 20% of users, and with some changes we can make it better for 40%... that's progress. We don't have to wait until we have a solution for 100% of them. > But for anybody who uses git for real (read: produces his or her own > history), it would be pretty much a useless default that forces the > user to say rebase or merge every time 'git pull' is run. This is not true. I will give you a real example. I create a branch named "fc/margin" based on "master", I make my changes, I push the branch to my personal repository, and create a pull request. This is the typical triangular workflow. Then I do "git pull [--ff-only]". What will happen? 1) As long as my branch is not merged upstream, I will get an error, and my branch will stay where it is. But then, 2) when my branch is finally merged to "master" it will be fast-forwarded, so now both "fc/margin" and "origin/master" point to the same commit. A. Did I use git "for real"? (produce my own history) B. Was "git pull [--ff-only]" useful in this case? I think that one of the problems is that Git has so many different workflows that finding another person that has your same workflow is like finding a person with your same birthday. It's not impossible, just takes more than a few tries. Also, and this is not a deriding question, I'm genuinely curious: how often do you send pull requests? BTW, this example above is real [2]. In my particular case very often I'm creating history, I'm just not the one pulling it. > But other than that, I do not > see any real use for the choice, which would mean in practice, > pull.mode would have only two useful values, rebase or merge. That > does not feel a good enough reason to supersede what already exists, > which is pull.rebase=yes/no. The fact that you don't see the use doesn't mean the use is not there. Why do you think this issue keeps coming back again and again, and again? And every time it comes back people say the same thing: "fast-forward-only merges should be the default". Unfortunately it's not that simple. It's a rabbit hole that leads to a cacophony of issues in git pull. However, we can fix some of them *today*. > Perhaps there is a good reason why certain classes of users would > want to configure pull.mode=ff-only (i.e. "I might type 'git pull' > by mistake, please stop me if I did so on a branch I have real work > already."). If that is the case, I would very much agree that it > would be awkward to express that choice in the current framework to > choose between pull.rebase=yes/no and pull.mode=(rebase/merge/ff-only) > would become a lot easier to explain. There's three options: 1. pull.ff=only (that doesn't work IMO) 2. pull.rebase=ff-only (that works, but it's kind of wonky) 3. pull.mode=ff-only (maybe it should be pull.mode=fast-forward) But the current option (pull.mode=merge) just doesn't fly. And BTW, I did create a poll in reddit's r/git [3], and 67% (of 789) said they didn't specify anything, just "git pull". So, most people simply do "git pull" and hope for the best. Moreover, in 2014 I said if we don't fix it now (which is likely), we will be discussing it again [4], and here we are now. And I'm saying it again: leave the mode as "merge", we will be discussing this again. I could do some mail archeology if you want, but this issue starts to be mentioned at least since 2010, and virtually everyone (except one person) agreed the default must change, even Linus Torvalds. Reading back what Linus said [5], it's something very, *very* close to what I'm proposing (I would argue my proposal is better). So you let me know. Do you want me to dig a decade of discussions and coalesce those conclusions into a summary so we can decide how to proceed? Or should I drop the plan? Only that if we drop it, I *guarantee* we will discuss it yet again years later. Moreover, this is the reason why I split the series in 3. Even if you decide you don't want to change the default, part I of the series can still be merged *today*, and everyone would benefit. Cheers. [1] https://en.wikipedia.org/wiki/Nirvana_fallacy [2] https://github.com/git/git.github.io/commit/5d90fd64d80847e3d873da43515750bc04b639e2 [3] https://www.reddit.com/r/git/comments/k6uj7c/do_you_use_git_pull/ [4] https://lore.kernel.org/git/536429e97cf5a_200c12912f094@nysa.notmuch/ [5] https://lore.kernel.org/git/CA+55aFz2Uvq4vmyjJPao5tS-uuVvKm6mbP7Uz8sdq1VMxMGJCw@mail.gmail.com/
Hi, On Wed, Dec 9, 2020 at 1:53 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > On Tue, Dec 8, 2020 at 2:16 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > > > That is exemplified by the fact that this whole thread started from a > > > user that refused to configure pull.rebase and expected the Git > > > project to just do the right thing (which is basically choosing a > > > useful default). > > > > Which is basically asking for impossible, and I do not think it is a > > good idea to focus our effort to satisfy such a request in general. > > There is no useful default that suits everybody in this particular > > case. > > I think I already made this point, but this is the nirvana fallacy > (the perfect is the enemy of the good) [1]. Just because we can't have > the perfect solution doesn't mean we can't pursue something better > than the current state. > > What was asked was not a perfect solution, just a better default. If > right now the default is good enough for 20% of users, and with some > changes we can make it better for 40%... that's progress. We don't > have to wait until we have a solution for 100% of them. > > > But for anybody who uses git for real (read: produces his or her own > > history), it would be pretty much a useless default that forces the > > user to say rebase or merge every time 'git pull' is run. > > This is not true. > > I will give you a real example. > > I create a branch named "fc/margin" based on "master", I make my > changes, I push the branch to my personal repository, and create a > pull request. This is the typical triangular workflow. > > Then I do "git pull [--ff-only]". What will happen? 1) As long as my > branch is not merged upstream, I will get an error, and my branch will > stay where it is. But then, 2) when my branch is finally merged to > "master" it will be fast-forwarded, so now both "fc/margin" and > "origin/master" point to the same commit. > > A. Did I use git "for real"? (produce my own history) > B. Was "git pull [--ff-only]" useful in this case? > > I think that one of the problems is that Git has so many different > workflows that finding another person that has your same workflow is > like finding a person with your same birthday. It's not impossible, > just takes more than a few tries. > > Also, and this is not a deriding question, I'm genuinely curious: how > often do you send pull requests? > > BTW, this example above is real [2]. In my particular case very often > I'm creating history, I'm just not the one pulling it. > > > But other than that, I do not > > see any real use for the choice, which would mean in practice, > > pull.mode would have only two useful values, rebase or merge. That > > does not feel a good enough reason to supersede what already exists, > > which is pull.rebase=yes/no. > > The fact that you don't see the use doesn't mean the use is not there. > > Why do you think this issue keeps coming back again and again, and > again? And every time it comes back people say the same thing: > "fast-forward-only merges should be the default". > > Unfortunately it's not that simple. It's a rabbit hole that leads to a > cacophony of issues in git pull. However, we can fix some of them > *today*. > > > Perhaps there is a good reason why certain classes of users would > > want to configure pull.mode=ff-only (i.e. "I might type 'git pull' > > by mistake, please stop me if I did so on a branch I have real work > > already."). If that is the case, I would very much agree that it > > would be awkward to express that choice in the current framework to > > choose between pull.rebase=yes/no and pull.mode=(rebase/merge/ff-only) > > would become a lot easier to explain. > > There's three options: > > 1. pull.ff=only (that doesn't work IMO) > 2. pull.rebase=ff-only (that works, but it's kind of wonky) > 3. pull.mode=ff-only (maybe it should be pull.mode=fast-forward) > > But the current option (pull.mode=merge) just doesn't fly. And BTW, I > did create a poll in reddit's r/git [3], and 67% (of 789) said they > didn't specify anything, just "git pull". > > So, most people simply do "git pull" and hope for the best. > > Moreover, in 2014 I said if we don't fix it now (which is likely), we > will be discussing it again [4], and here we are now. And I'm saying > it again: leave the mode as "merge", we will be discussing this again. > > I could do some mail archeology if you want, but this issue starts to > be mentioned at least since 2010, and virtually everyone (except one > person) agreed the default must change, even Linus Torvalds. Reading > back what Linus said [5], it's something very, *very* close to what > I'm proposing (I would argue my proposal is better). > > So you let me know. Do you want me to dig a decade of discussions and > coalesce those conclusions into a summary so we can decide how to > proceed? Or should I drop the plan? Only that if we drop it, I > *guarantee* we will discuss it yet again years later. > > Moreover, this is the reason why I split the series in 3. Even if you > decide you don't want to change the default, part I of the series can > still be merged *today*, and everyone would benefit. 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/
Hello, On Wed, Dec 9, 2020 at 1:05 PM Elijah Newren <newren@gmail.com> wrote: > On Wed, Dec 9, 2020 at 1:53 AM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > I could do some mail archeology if you want, but this issue starts to > > be mentioned at least since 2010, and virtually everyone (except one > > person) agreed the default must change, even Linus Torvalds. Reading > > back what Linus said [5], it's something very, *very* close to what > > I'm proposing (I would argue my proposal is better). > > > > So you let me know. Do you want me to dig a decade of discussions and > > coalesce those conclusions into a summary so we can decide how to > > proceed? Or should I drop the plan? Only that if we drop it, I > > *guarantee* we will discuss it yet again years later. > > > > Moreover, this is the reason why I split the series in 3. Even if you > > decide you don't want to change the default, part I of the series can > > still be merged *today*, and everyone would benefit. > > 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? People change their minds. Perhaps I misinterpreted something, but when Junio said "I dunno" I take it to mean: he is unsure my proposal "pull.mode=ff-only" makes sense. He also said "for anybody who uses git for real, [force -ff-only] would be pretty much a useless default", which I take it to mean that perhaps we shouldn't change the default to that. Back in 2013 Junio said "I now see how such a default makes sense." [1], but we are in 2020 and such a default that made sense is not the default. Mind reading is a really bad habbit [2]; I don't know what other people think, and I would not presume to know. All I know is that the path forward is unclear. And because I prognosticated that, I split the series in 3 parts, and I've yet to see any objection to part I, which would improve the situation for users *today*. So, while we make up our collective minds, there's no reason to bother our users with an annoying warning on *every* *single* *pull*. Cheers. [1] https://lore.kernel.org/git/7vli74baym.fsf@alter.siamese.dyndns.org/ [2] https://cogbtherapy.com/cbt-blog/common-cognitive-distortions-mind-reading
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.
On Thu, Dec 10, 2020 at 12:45 AM Junio C Hamano <gitster@pobox.com> wrote: > > 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. I don't agree with the above. The error I propose is just: The pull was not fast-forward, please either merge or rebase. That's it. Nothing more. I explained that was the final end goal in my list of steps [1]. I do not think any suggestion for commands or configurations belongs in a *permanent* error message. Do we even have an error message that long? > 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. No. I don't want to present the user *any* configuration option in the *permanent error* message, just: "please either merge or rebase". That's it. > 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. Whether we present the user with that option or not doesn't matter. "pull.rebase=yes/no", would still work. The only thing that changes is what happens if the user does *not* set that option. The reason "pull.mode=ff-only" needs to be introduced is that --ff-only doesn't work. Otherwise there's no way the user cannot select the "safe default" mode. It has absolutely nothing to do with what we present the user with. > 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. That's right, which is a *crucial* thing they must have. Part II of the proposal has these: pull: add pull.mode pull: add pull.mode=ff-only pull: advice of future changes This is where we want to be for a good considerable amount of time. A point where the user is a) warned about future changes, b) can configure both the old and the new behavior, and c) we have time to test and tune this new behavior (in case there are mistakes), *BEFORE* forcing all our users to switch to this new mode (unless they haven't configured otherwise). This is what happened with "push.default=simple". We didn't just say: the simple mode is the default mode, so just leave "push.default" blank if you want this mode. No. Because in theory we might have decided before v2.0 that another mode was to be the default, or perhaps in v3.0 another one will. We want the user to be able to say: this is the mode I want. Not whatever mode the Git project decides it's better this day of the week: *this* mode. There is nothing good about the user being unable to specify the mode he/she wants instead of the default, even if at this moment in time they happen to be the same. Moreover, I can attach a patch on top of part I that skips right ahead towards part 3, and flips the switch without any intermediary steps. The patch is simple, but it's likely to be wrong. And I doubt anyone has tested this patch series at this point. No amount of looking at the code is going to spot all the problems. People actually need to try this code. Otherwise it's just some patches flying around that will never be merged, and the problem will remain forever there. > Other than that, the existing > pull.rebase=yes/no is perfectly adequate. You cannot specify "pull.rebase=ff-only", but I did send a patch for that [2]. It was you the one that said it looked "quite funny" [3]: "it looks quite funny for that single variable that controls the way how pull works, whether rebase or merge is used, is pull.REBASE". If you think it's fine to have this warning: The pull was not fast-forward, in the future you will have to choose a merge, or a rebase. To quell this message you have two main options: 1. Adopt the new behavior: git config --global pull.rebase ff-only 2. Maintain the current behavior: git config --global pull.rebase false For now we will fall back to the traditional behavior (merge). Read "git pull --help" for more information. For months, possibly a year, possibly until v3.0, instead of this one (which is my proposal): The pull was not fast-forward, in the future you will have to choose a merge, or a rebase. To quell this message you have two main options: 1. Adopt the new behavior: git config --global pull.mode fast-forward 2. Maintain the current behavior: git config --global pull.mode merge For now we will fall back to the traditional behavior (merge). Read "git pull --help" for more information. By all means, let's go for that, make the decision. But it was you who wanted to see how a pull.mode solution would look like [4][5]. Well, this is how it looks like: 3 patches. > 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). No. It doesn't matter if it's "pull.mode=ff-only", or "pull.rebase=ff-only", or "pull.ff=only", but one of those must turn on the behavior that we want. And right now no option does. > Thinking about it again, I guess pull.rebase=yes/no plus a new > advise entry can easily give what Felipe seems to want. It's not about what I want, it's about what the project wants. And I think it's pretty clear what the project wants: 1. git pull to eventually fail in non-fast-forward cases 2. A grace period before that switch is flipped That's it. My proposal is the only one (so far) that gives us that. Cheers. [1] https://lore.kernel.org/git/CAMP44s2hUCd9qc83LReGyjy8N+u++eK6VjwGhDhrX0f0SbKmig@mail.gmail.com/ [2] https://lore.kernel.org/git/20201123224621.2573159-2-felipe.contreras@gmail.com/ [3] https://lore.kernel.org/git/xmqqim9vlkdn.fsf@gitster.c.googlers.com/ [4] https://lore.kernel.org/git/xmqqy2irjy4f.fsf@gitster.c.googlers.com/ [5] https://lore.kernel.org/git/xmqq7dqagtgx.fsf@gitster.c.googlers.com/
On Thu, Dec 10, 2020 at 3:08 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > It's not about what I want, it's about what the project wants. And I > think it's pretty clear what the project wants: > > 1. git pull to eventually fail in non-fast-forward cases > 2. A grace period before that switch is flipped > > That's it. > > My proposal is the only one (so far) that gives us that. Not to mention that part I is not even contentious. We can improve the situation of users *today*.
Felipe Contreras <felipe.contreras@gmail.com> writes: >> 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. > > I don't agree with the above. > > The error I propose is just: > > The pull was not fast-forward, please either merge or rebase. > > That's it. Nothing more. It says "why we are stopping." quite well. It would be a good message to use as the first part of the three-part message I mentioned above. > I explained that was the final end goal in my list of steps [1]. I do > not think any suggestion for commands or configurations belongs in a > *permanent* error message. In the design I have in mind in the message you are responding to, the users who haven't told their choice to Git would be the only folks who get all three. You want to let the user express: "I do not want to choose either rebase or merge. I want 'pull' to fail when it needs to deal with non-ff history. But I do not need to be told about command line option and configuration every time." I said I don't (I view that disabling half the "git pull" just a safe fallback behaviour until the user chooses between merge and rebase), but if we wanted to offer it as a valid choice to users, we can do so. We just make it possible to squelch the latter two parts of the three-part message---you leave pull.rebase unconfigured and squelch the latter two parts of the message, and you got the "stop me, I do not merge or rebase, but don't even tell me how to further configure" already. I agree the latter two should not be part of *permanent* error message. And my suggestion did not intend to make them so---it should have been quite obvious to who read the message you are responding to through to the end and understood what it said. Now, how would we make it possible to squelch the latter two parts? It is not a good idea to introduce pull.mode=ff-only for that purpose (pull.mode=rebase and pull.rebase=yes would unnecessarily become two redundant ways to do the same thing if we added a new pull.mode variable). But there is an established way used in this project when we allow squelching overly-helpful help messages, and we can apply it here as well. That way: - unconfigured folks would get all the three parts of the messages, just like the current system. - if you tell rebase or merge, you do not see any. - if you do not choose between rebase or merge, you can still squelch the latter two by setting advice.pullNonFF to false. The last one is "keep the more dangerous half of 'git pull' disabled, without getting told how to enable it over and over", which is what you want to be able to specify. > The reason "pull.mode=ff-only" needs to be introduced is that > --ff-only doesn't work. Otherwise there's no way the user cannot > select the "safe default" mode. It has absolutely nothing to do with > what we present the user with. I too initially thought that pull.mode may be needed, but probably I was wrong. I do think this can be done without pull.mode at all, at least in two ways, without adding different ways to do the same thing. - When pull.rebase is set to 'no' and pull.ff is set to 'only', "git pull" that sees a non-ff history should error out safely. The user is telling that their preference is to merge, but the difference between merge and rebase does not really matter because pull.ff=only would mean we forbid merges of non-ff history anyway. The message you'd get would be "fatal: Not possible to fast-forward, aborting." though. - Or with the advice that hides the latter two points, a user can unset pull.rebase and set the advice.pullNonFF to false to get the same behaviour (i.e. disable the more dangerous half of "pull") with just the "we stopped" error message. I think either of these are close enough to what you want, and I think the latter gives us more flexibility in how we tone down the message with advice.pullNonFF.
On Fri, Dec 11, 2020 at 4:02 AM Junio C Hamano <gitster@pobox.com> wrote: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > >> 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. > > > > I don't agree with the above. > > > > The error I propose is just: > > > > The pull was not fast-forward, please either merge or rebase. > > > > That's it. Nothing more. > > It says "why we are stopping." quite well. It would be a good > message to use as the first part of the three-part message I > mentioned above. The two key parts of the message are: 1. It is an *error* 2. It is *permanent* > > I explained that was the final end goal in my list of steps [1]. I do > > not think any suggestion for commands or configurations belongs in a > > *permanent* error message. > > In the design I have in mind in the message you are responding to, > the users who haven't told their choice to Git would be the only > folks who get all three. What would that error message look like? And do you have any other example of the current user interface where such a condescending long error message is displayed? > You want to let the user express: "I do not want to choose either > rebase or merge. I want 'pull' to fail when it needs to deal with > non-ff history. But I do not need to be told about command line > option and configuration every time." That's right. > I said I don't (I view that disabling half the "git pull" just a > safe fallback behaviour until the user chooses between merge and > rebase), but if we wanted to offer it as a valid choice to users, we > can do so. We just make it possible to squelch the latter two parts > of the three-part message---you leave pull.rebase unconfigured and > squelch the latter two parts of the message, and you got the "stop > me, I do not merge or rebase, but don't even tell me how to further > configure" already. > > I agree the latter two should not be part of *permanent* error > message. And my suggestion did not intend to make them so---it > should have been quite obvious to who read the message you are > responding to through to the end and understood what it said. It doesn't matter (much) if it's temporary or permanent, it's still an *error* message. Currently it's a warning, and people are complaining, even though the pull still works. And you want to make it an error, and *always* fail? Even though the user has not been warned that such a change was coming and how to evade it? I'm against that. I would rather do nothing than intentionally break user experience without warning. Johannes said he didn't mind the warning. I want the warning to remain, just make it a different warning. You want to break his experience without a grace period and suddenly make it an error. > Now, how would we make it possible to squelch the latter two parts? ... This is irrelevant. As long as it's an error I don't care if it's short or long. I'm against turning on an error from one version to the next. > > The reason "pull.mode=ff-only" needs to be introduced is that > > --ff-only doesn't work. Otherwise there's no way the user cannot > > select the "safe default" mode. It has absolutely nothing to do with > > what we present the user with. > > I too initially thought that pull.mode may be needed, but probably I > was wrong. I do think this can be done without pull.mode at all, at > least in two ways, without adding different ways to do the same > thing. > > - When pull.rebase is set to 'no' and pull.ff is set to 'only', > "git pull" that sees a non-ff history should error out safely. > The user is telling that their preference is to merge, but the > difference between merge and rebase does not really matter > because pull.ff=only would mean we forbid merges of non-ff > history anyway. The message you'd get would be "fatal: Not > possible to fast-forward, aborting." though. > > - Or with the advice that hides the latter two points, a user can > unset pull.rebase and set the advice.pullNonFF to false to get > the same behaviour (i.e. disable the more dangerous half of > "pull") with just the "we stopped" error message. So, after your hypothetical patch, there would be no difference between: git -c pull.rebase=no -c pull.ff=only pull and: git -c advice.pullnonff=false pull ? > I think either of these are close enough to what you want, and I > think the latter gives us more flexibility in how we tone down the > message with advice.pullNonFF. You are missing at least two things. I'll wait for your response. Cheers.
Felipe Contreras <felipe.contreras@gmail.com> writes: > This is irrelevant. Oh, now you are saying that you do not need a way to squelch these message to help unconfigured users choose between rebase or merge? I am confused. > As long as it's an error I don't care if it's short or long. I'm > against turning on an error from one version to the next. Now you are changing your mind? Current version and past ones do not make it an error to pull non-ff history without choosing rebase or merge---we go ahead and merge anyway. I thought that in the far future agreed between two of us, it would be turnend into an error at some point. There needs one version that turns it into an error for that to happen. Puzzled. >> I too initially thought that pull.mode may be needed, but probably I >> was wrong. I do think this can be done without pull.mode at all, at >> least in two ways, without adding different ways to do the same >> thing. >> >> - When pull.rebase is set to 'no' and pull.ff is set to 'only', >> "git pull" that sees a non-ff history should error out safely. >> The user is telling that their preference is to merge, but the >> difference between merge and rebase does not really matter >> because pull.ff=only would mean we forbid merges of non-ff >> history anyway. The message you'd get would be "fatal: Not >> possible to fast-forward, aborting." though. >> >> - Or with the advice that hides the latter two points, a user can >> unset pull.rebase and set the advice.pullNonFF to false to get >> the same behaviour (i.e. disable the more dangerous half of >> "pull") with just the "we stopped" error message. > > So, after your hypothetical patch, there would be no difference between: > > git -c pull.rebase=no -c pull.ff=only pull > > and: > > git -c advice.pullnonff=false pull > > ? We do not have to or implement both, but either should give us the "when pull sees a non-ff history, it should stop without merging or rebasing, and the user won't be given the advice on how to choose between merge and rebase" behaviour, I would think. >> I think either of these are close enough to what you want, and I >> think the latter gives us more flexibility in how we tone down the >> message with advice.pullNonFF. > > You are missing at least two things. I am guessing that the '?' above I just answered is one you wanted to ask me, but what's the other one?
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > This is irrelevant. > > Oh, now you are saying that you do not need a way to squelch these > message to help unconfigured users choose between rebase or merge? > I am confused. We need a way to squelch the *advise* message, yes. But this is a secondary priority. The first priority is not to break current behavior users rely on. > > As long as it's an error I don't care if it's short or long. I'm > > against turning on an error from one version to the next. > > Now you are changing your mind? Current version and past ones do > not make it an error to pull non-ff history without choosing rebase > or merge---we go ahead and merge anyway. I thought that in the far > future agreed between two of us, it would be turnend into an error > at some point. There needs one version that turns it into an error > for that to happen. Puzzled. Yes, in the future, *after* a deprecation period. Once the users have had a chance to configure git to do what they want, have tried the new mode, haven't had issues with the new mode, or if they disagree with the proposed *future* behavior; complain in the forums at their disposal. Not before. > >> I too initially thought that pull.mode may be needed, but probably I > >> was wrong. I do think this can be done without pull.mode at all, at > >> least in two ways, without adding different ways to do the same > >> thing. > >> > >> - When pull.rebase is set to 'no' and pull.ff is set to 'only', > >> "git pull" that sees a non-ff history should error out safely. > >> The user is telling that their preference is to merge, but the > >> difference between merge and rebase does not really matter > >> because pull.ff=only would mean we forbid merges of non-ff > >> history anyway. The message you'd get would be "fatal: Not > >> possible to fast-forward, aborting." though. > >> > >> - Or with the advice that hides the latter two points, a user can > >> unset pull.rebase and set the advice.pullNonFF to false to get > >> the same behaviour (i.e. disable the more dangerous half of > >> "pull") with just the "we stopped" error message. > > > > So, after your hypothetical patch, there would be no difference between: > > > > git -c pull.rebase=no -c pull.ff=only pull > > > > and: > > > > git -c advice.pullnonff=false pull > > > > ? > > We do not have to or implement both, but either should give us the > "when pull sees a non-ff history, it should stop without merging or > rebasing, and the user won't be given the advice on how to choose > between merge and rebase" behaviour, I would think. Right, so both should error out. And what should these do? git -c pull.rebase=no -c pull.ff=only pull --merge git -c advice.pullnonff=false pull --merge I'm going to answer because I think it's obvious what you would expect: if you pass --merge, both should succeed. Except they won't, because "git pull --ff-only --merge" fails. Correct? > >> I think either of these are close enough to what you want, and I > >> think the latter gives us more flexibility in how we tone down the > >> message with advice.pullNonFF. > > > > You are missing at least two things. > > I am guessing that the '?' above I just answered is one you wanted > to ask me, but what's the other one? Yes. The other is what I explained above; we need a grace deprecation period where we can explain to the user in a simple way what to expect in the future. And it's much easier to explain to the user that: git pull --merge -> pull.mode = merge git pull --rebase -> pull.mode = rebase git pull -> pull.mode = fast-forward Than: git pull --merge -> pull.rebase = false git pull --rebase -> pull.rebase = true git pull -> pull.rebase = false + pull.ff = only But those are not the only two. For example there's this additional problem of how to interact with the other values of pull.rebase (other than true and false): pull.mode = merge pull.rebase = merges I would expect in this particular configuration that: git pull -> git pull --merge git pull --rebase -> git pull --rebase=merges There's no way to represent that with just pull.rebase. And there's more: some people suggested other modes in 2013, like "pull.mode=none" (essentially "git fetch"), or "pull.mode=merge-reverse-parents". But the first one should be enough ("git pull --ff-only --merge" doesn't work). Cheers.
Felipe Contreras <felipe.contreras@gmail.com> writes: >> We do not have to or implement both, but either should give us the >> "when pull sees a non-ff history, it should stop without merging or >> rebasing, and the user won't be given the advice on how to choose >> between merge and rebase" behaviour, I would think. > > Right, so both should error out. One of them, not both; the one we choose to implement would make it fail, I would think. > And what should these do? > > git -c pull.rebase=no -c pull.ff=only pull --merge There is no --merge, so let's reread with s/--merge/--no-rebase/; the command line would be saying "I only want to fast-forward, or I want the command to fail". The advice code should not trigger (i.e. we gave an explicit preference to merge and not to rebase, so rebase_unspecified would be false), but the configured preference that says "we take only fast-forward merges" will still be in effect. If the history is fast-forward, the merge backend will happily advance our history. If not, the merge backend will happily fail the pull. If you want to countermand the configured preference from the command line and allow a merge to be done when non-ff history is given, what you'd need to override is not --merge/--no-rebase. The configuration pull.rebase=no already says you do not want rebase and you want merge). You want to override --ff-only, so I'd expect "pull --ff" (or "pull --no-ff") to be how you override your configured preference and merge in a non-ff history, either by fast forwarding or creating an extra merge commit. > git -c advice.pullnonff=false pull --merge Again with s/--merge/--no-rebase/; but that is showing the preference not to rebase and to merge from the command line, so shouldn't it just go ahead and merge without any advice? > I'm going to answer because I think it's obvious what you would expect: > if you pass --merge, both should succeed. > > Except they won't, because "git pull --ff-only --merge" fails. > > Correct? See above.
On Thu, Dec 10, 2020 at 11:17:01PM -0800, Junio C Hamano wrote: > You want to let the user express: "I do not want to choose either > rebase or merge. I want 'pull' to fail when it needs to deal with > non-ff history. But I do not need to be told about command line > option and configuration every time." > > I said I don't (I view that disabling half the "git pull" just a > safe fallback behaviour until the user chooses between merge and > rebase), but if we wanted to offer it as a valid choice to users, we > can do so. We just make it possible to squelch the latter two parts > of the three-part message---you leave pull.rebase unconfigured and > squelch the latter two parts of the message, and you got the "stop > me, I do not merge or rebase, but don't even tell me how to further > configure" already. FWIW, I would be such a user who would want to squelch the error but keep the ff-only behavior (and have been doing so for years via pull.ff=only). I primarily use "git pull" only with branches where I am tracking upstream work, but don't have any of my own (e.g., if I do all of my work on topic branches, I may still keep a "master" branch that mirrors "origin/master", and use "git checkout master && git pull" to fast-forward it). So I think this is a valuable thing to have. And it does work just fine already, without pull.mode. The reasons to care about pull.mode (IMHO) are mostly about explaining it: - it isn't respected with rebase. So if you set pull.rebase=true, now pull.ff=only does nothing. This is arguably confusing, though I doubt it comes up much in practice. - having a single tri-state of merge/rebase/error for "what do I do when pulling a non-fast-forward merge" is conceptually simple and easy to explain. So I like pull.mode in that sense. But it is also weighed against the fact that we'd still have to support pull.rebase, and we'd still have to support pull.ff, and explain how those interact with pull.mode (and I think any new error in this area must be squelched by those existing variables, or it would be a regression for people who already picked their default long ago). > But there is an established way used in this project when we allow > squelching overly-helpful help messages, and we can apply it here as > well. That way: > > - unconfigured folks would get all the three parts of the messages, > just like the current system. > > - if you tell rebase or merge, you do not see any. > > - if you do not choose between rebase or merge, you can still > squelch the latter two by setting advice.pullNonFF to false. > > The last one is "keep the more dangerous half of 'git pull' disabled, > without getting told how to enable it over and over", which is what > you want to be able to specify. Using advice.* to squelch the advice would be fine with me, provided it was _also_ squelched by the existing config options. Which I think is where your thinking is ending up. -Peff
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > >> We do not have to or implement both, but either should give us the > >> "when pull sees a non-ff history, it should stop without merging or > >> rebasing, and the user won't be given the advice on how to choose > >> between merge and rebase" behaviour, I would think. > > > > Right, so both should error out. > > One of them, not both; the one we choose to implement would make it > fail, I would think. If only one of them fail, then it's impossible to tell the user: If you want to try the new behavior, type this command: git config $something We have to go straight for the error, with no deprecation period. > > And what should these do? > > > > git -c pull.rebase=no -c pull.ff=only pull --merge > > There is no --merge, so let's reread with s/--merge/--no-rebase/; Back in 2013 everyone found it easier to speak in terms of --merge, even you, BTW, who was the first one to send a patch with --merge [1], after Linus suggested it [2]. > the command line would be saying "I only want to fast-forward, or I > want the command to fail". The advice code should not trigger > (i.e. we gave an explicit preference to merge and not to rebase, so > rebase_unspecified would be false), but the configured preference > that says "we take only fast-forward merges" will still be in effect. > If the history is fast-forward, the merge backend will happily > advance our history. If not, the merge backend will happily fail > the pull. > > If you want to countermand the configured preference from the > command line and allow a merge to be done when non-ff history is > given, what you'd need to override is not --merge/--no-rebase. The > configuration pull.rebase=no already says you do not want rebase and > you want merge). You want to override --ff-only, so I'd expect > "pull --ff" (or "pull --no-ff") to be how you override your > configured preference and merge in a non-ff history, either by fast > forwarding or creating an extra merge commit. Precisely, so you can't tell the user: If you want to try the new behavior, type this command: git config pull.ff only Because, after the user gets the error: Not a fast-forward, either merge or rebase. She cannot do: git pull --merge Which is the best idiom to specify she wants a merge. The command will still fail, even after the user has specified she wants the new mode, and that she wants to merge in this perticular case. > > git -c advice.pullnonff=false pull --merge > > Again with s/--merge/--no-rebase/; but that is showing the > preference not to rebase and to merge from the command line, so > shouldn't it just go ahead and merge without any advice? Yes, but *before* we make this the default, what configuration get us there today? > > I'm going to answer because I think it's obvious what you would expect: > > if you pass --merge, both should succeed. > > > > Except they won't, because "git pull --ff-only --merge" fails. > > > > Correct? > > See above. That's a "correct". There's no configuration today that gives us both: 1. fail: git -c $config pull 2. pass: git -c $config pull --merge Like this does: 1. fail: git -c pull.mode=fast-forward pull 2. pass: git -c pull.mode=fast-forward pull --merge Cheers. [1] https://lore.kernel.org/git/7v4ncjs5az.fsf_-_@alter.siamese.dyndns.org/ [2] https://lore.kernel.org/git/CA+55aFz2Uvq4vmyjJPao5tS-uuVvKm6mbP7Uz8sdq1VMxMGJCw@mail.gmail.com/
Jeff King wrote: > On Thu, Dec 10, 2020 at 11:17:01PM -0800, Junio C Hamano wrote: > > > You want to let the user express: "I do not want to choose either > > rebase or merge. I want 'pull' to fail when it needs to deal with > > non-ff history. But I do not need to be told about command line > > option and configuration every time." > > > > I said I don't (I view that disabling half the "git pull" just a > > safe fallback behaviour until the user chooses between merge and > > rebase), but if we wanted to offer it as a valid choice to users, we > > can do so. We just make it possible to squelch the latter two parts > > of the three-part message---you leave pull.rebase unconfigured and > > squelch the latter two parts of the message, and you got the "stop > > me, I do not merge or rebase, but don't even tell me how to further > > configure" already. > > FWIW, I would be such a user who would want to squelch the error but > keep the ff-only behavior (and have been doing so for years via > pull.ff=only). If you have configured pull.ff=only, you should be getting this error: Not possible to fast-forward, aborting. We would like this error instead: Not a fast-forward, either merge or rebase. And after that it should be obvious what this command should do with your current configuration: git pull --merge But it would fail, even after specifying you want to do a merge, because "git pull --ff-only --merge" fails. > So I think this is a valuable thing to have. And it does work just fine > already, without pull.mode. The reasons to care about pull.mode (IMHO) > are mostly about explaining it: > > - it isn't respected with rebase. So if you set pull.rebase=true, now > pull.ff=only does nothing. This is arguably confusing, though I > doubt it comes up much in practice. > > - having a single tri-state of merge/rebase/error for "what do I do > when pulling a non-fast-forward merge" is conceptually simple and > easy to explain. Indeed. It's *mostly* about explaining it, but there's also what I stated above: git -c pull.mode=fast-forward pull git -c pull.mode=fast-forward pull --merge We want the first one to fail, and the second one to succeed, which can't be done with the current "pull.ff=only". > So I like pull.mode in that sense. But it is also weighed against the > fact that we'd still have to support pull.rebase, and we'd still have to > support pull.ff, and explain how those interact with pull.mode (and I > think any new error in this area must be squelched by those existing > variables, or it would be a regression for people who already picked > their default long ago). The interactions should be easy: * pull.rebase=false -> pull.mode=merge * pull.rebase=the-rest -> pull.mode=rebase * pull.ff=only -> pull.mode=fast-forward (note: I'm not entirely sure of the last one) Then, any --merge/--rebase option overrides the mode, but not so the --ff options. We would still like this to fail: git -c pull.mode=merge pull --ff-only Right? In other words: pull.mode is strongly linked with pull.rebase, but not so much wih --ff*. > > But there is an established way used in this project when we allow > > squelching overly-helpful help messages, and we can apply it here as > > well. That way: > > > > - unconfigured folks would get all the three parts of the messages, > > just like the current system. > > > > - if you tell rebase or merge, you do not see any. > > > > - if you do not choose between rebase or merge, you can still > > squelch the latter two by setting advice.pullNonFF to false. > > > > The last one is "keep the more dangerous half of 'git pull' disabled, > > without getting told how to enable it over and over", which is what > > you want to be able to specify. > > Using advice.* to squelch the advice would be fine with me, provided it > was _also_ squelched by the existing config options. > > Which I think is where your thinking is ending up. Yes. At least that's my thinking. Cheers.
Jeff King <peff@peff.net> writes: > So I like pull.mode in that sense. But it is also weighed against the > fact that we'd still have to support pull.rebase, and we'd still have to > support pull.ff, and explain how those interact with pull.mode (and I > think any new error in this area must be squelched by those existing > variables, or it would be a regression for people who already picked > their default long ago). I agree that if we were starting from scratch, things would have been much simpler; choose among three ways to reconcile histories (merge, rebase, or ff-only), without adding --[no-]rebase, and allow --[no-]ff only when merging (i.e. things like --ff-only --ff, --no-ff --rebase, would be nonsense combinations). The additional complexity of introducing pull.mode is the primary thing I am hesitant to support it, as we have to design and explain how existing knobs interact with newer one. > Using advice.* to squelch the advice would be fine with me, provided it > was _also_ squelched by the existing config options. Meaning "once you choose between rebase or merge, facing a non-ff history would not trigger the advice message"? I think that is already the case with the released versions of Git, and I think that is a good thing to keep. The advice is only for unconfigured folks who did not tell --[no-]rebase from the command line. One bad thing about it in the released versions is that it would trigger even when the history fast-forwards. The latest round of patches in 'seen' squelches the advice when pulling a fast-forward history even for unconfigured folks as we'd just fast-forward without erroring out. > Which I think is where your thinking is ending up. Pretty much.
Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > So I like pull.mode in that sense. But it is also weighed against the > > fact that we'd still have to support pull.rebase, and we'd still have to > > support pull.ff, and explain how those interact with pull.mode (and I > > think any new error in this area must be squelched by those existing > > variables, or it would be a regression for people who already picked > > their default long ago). > > I agree that if we were starting from scratch, things would have > been much simpler; choose among three ways to reconcile histories > (merge, rebase, or ff-only), without adding --[no-]rebase, and allow > --[no-]ff only when merging (i.e. things like --ff-only --ff, > --no-ff --rebase, would be nonsense combinations). The additional > complexity of introducing pull.mode is the primary thing I am > hesitant to support it, as we have to design and explain how > existing knobs interact with newer one. I have already tried all the options. If we go through a deprecation period (which I insist we must), the options are these: 1. Change the semantics of --ff-only (breaks backwards-compatibility) 2. Add a "pull.rebase=ff-only" option (very weird) 3. Add a "pull.mode" configuration (tons of advantages) Yes, we have to map the modes to the existing knobs, but it's not complex: * pull.rebase = false <=> pull.mode = merge * pull.rebase = true <=> pull.mode = rebase * pull.rebase = * -> pull.mode = rebase That's it. We don't even have to map --ff-only (which has different semantics anyway). Cheers.
diff --git a/builtin/pull.c b/builtin/pull.c index 1034372f8b..22a9ffcade 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -346,17 +346,18 @@ static enum rebase_type config_get_rebase(void) if (opt_verbosity >= 0 && !opt_ff) { advise(_("Pulling without specifying how to reconcile divergent branches is\n" - "discouraged. You can squelch this message by running one of the following\n" - "commands sometime before your next pull:\n" - "\n" - " git config pull.rebase false # merge (the default strategy)\n" - " git config pull.rebase true # rebase\n" - " git config pull.ff only # fast-forward only\n" - "\n" - "You can replace \"git config\" with \"git config --global\" to set a default\n" - "preference for all repositories. You can also pass --rebase, --no-rebase,\n" - "or --ff-only on the command line to override the configured default per\n" - "invocation.\n")); + "discouraged; you need to specify if you want a merge, or a rebase.\n" + "You can squelch this message by running one of the following commands:\n" + "\n" + " git config pull.rebase false # merge (the default strategy)\n" + " git config pull.rebase true # rebase\n" + " git config pull.ff only # fast-forward only\n" + "\n" + "You can replace \"git config\" with \"git config --global\" to set a default\n" + "preference for all repositories.\n" + "If unsure, run \"git pull --no-rebase\".\n" + "Read \"git pull --help\" for more information." + )); } return REBASE_FALSE;
We want to: 1. Be clear about what "specifying" means; merge or rebase. 2. Mention a direct shortcut for people that just want to get on with their lives: git pull --no-rebase. 3. Have a quick reference for users to understand what this "fast-forward" business means. This patch does all three. Thanks to the previous patch now "git pull --help" explains what a fast-forward is, and a further patch changes --no-rebase to --merge so it's actually user friendly. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- builtin/pull.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)