| Message ID | 20201205195313.1557473-3-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,01/16] doc: pull: explain what is a fast-forward | expand |
Felipe Contreras <felipe.contreras@gmail.com> writes: > We want to: > > 1. Be clear about what "specifying" means; merge, rebase, or > fast-forward. > 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 | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index 1034372f8b..d71344fe28 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -345,18 +345,19 @@ static enum rebase_type config_get_rebase(void) > return parse_config_rebase("pull.rebase", value, 1); > > 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")); > + advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n" > + "you need to specify if you want a merge, a rebase, or a fast-forward.\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." > + )); > } It is an improvement to say what they can do from the command line in order to get out of the current situation, before giving them a configuration advice. But the exact instruction for the command line in the original seems to have been lost during the rewrite, which I think makes the "what to do now" advice less valuable than it could be. I personally think it is backwards to update this message before fixing the condition under which it triggers (I think by now everybody involved in the thread understands that we do not want to give this advise that does not stop to behave as-is---it should be made not to trigger if we know the history would fast-forward, and when it triggers it should stop the operation). It may appear OK as long as we get the right end-state, but because this message must be rewritten when the triggering condition changes (i.e. when we stop giving this message when the history fast-forwards, there is no point in offering the fast-forward-only as a viable option), it seems to me a needless detour.
On Mon, Dec 7, 2020 at 2:55 PM Junio C Hamano <gitster@pobox.com> wrote: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > We want to: > > > > 1. Be clear about what "specifying" means; merge, rebase, or > > fast-forward. > > 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 | 25 +++++++++++++------------ > > 1 file changed, 13 insertions(+), 12 deletions(-) > > > > diff --git a/builtin/pull.c b/builtin/pull.c > > index 1034372f8b..d71344fe28 100644 > > --- a/builtin/pull.c > > +++ b/builtin/pull.c > > @@ -345,18 +345,19 @@ static enum rebase_type config_get_rebase(void) > > return parse_config_rebase("pull.rebase", value, 1); > > > > 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")); > > > > + advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n" > > + "you need to specify if you want a merge, a rebase, or a fast-forward.\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." > > + )); > > } > > It is an improvement to say what they can do from the command line > in order to get out of the current situation, before giving them a > configuration advice. > > But the exact instruction for the command line in the original seems > to have been lost during the rewrite, which I think makes the "what > to do now" advice less valuable than it could be. In my opinion we should not dump the entire contents of the manpage in a warning. The warning should be as brief as possible while giving these: 1. A description of what's going on 2. A suggested way to move forward 3. A quick way to opt-out 4. A way to find more information That's it. Of course when it comes to succinctness other developers tend to disagree with me, but that's my opinion. > I personally think it is backwards to update this message before > fixing the condition under which it triggers (I think by now > everybody involved in the thread understands that we do not want to > give this advise that does not stop to behave as-is---it should be > made not to trigger if we know the history would fast-forward, and > when it triggers it should stop the operation). It may appear OK as > long as we get the right end-state, but because this message must be > rewritten when the triggering condition changes (i.e. when we stop > giving this message when the history fast-forwards, there is no > point in offering the fast-forward-only as a viable option), it > seems to me a needless detour. Except it's not necessary to rewrite it further, not in step 1. Maybe it will be clearer when I send all the patches.
diff --git a/builtin/pull.c b/builtin/pull.c index 1034372f8b..d71344fe28 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -345,18 +345,19 @@ static enum rebase_type config_get_rebase(void) return parse_config_rebase("pull.rebase", value, 1); 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")); + advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n" + "you need to specify if you want a merge, a rebase, or a fast-forward.\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, rebase, or fast-forward. 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 | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)