| Message ID | 20200229212258.373891-1-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] pull: warn if the user didn't say whether to rebase or to merge | expand |
Hi Alex, On Sat, Feb 29, 2020 at 1:23 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > > Often novice Git users forget to say "pull --rebase" and end up with an > unnecessary merge from upstream. What they usually want is either "pull > --rebase" in the simpler cases, or "pull --ff-only" to update the copy > of main integration branches, and rebase their work separately. The > pull.rebase configuration variable exists to help them in the simpler > cases, but there is no mechanism to make these users aware of it. > > Issue a warning message when no --[no-]rebase option from the command > line and no pull.rebase configuration variable is given. This will > inconvenience those who never want to "pull --rebase", who haven't had > to do anything special, but the cost of the inconvenience is paid only > once per user, which should be a reasonable cost to help a number of new > users. Thanks for working on this. > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > v3: > - Fix segfault if opt_ff is NULL > - Don't say that the behavior will change in a future version of Git > (that hasn't been decided yet) > - Mention that pull.ff=only is also an option > - Mention git config's --global option > - Make the warning message longer and more clear in general > --- > builtin/pull.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/builtin/pull.c b/builtin/pull.c > index 3e624d1e00..a93ee489f6 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -327,6 +327,20 @@ static enum rebase_type config_get_rebase(void) > if (!git_config_get_value("pull.rebase", &value)) > return parse_config_rebase("pull.rebase", value, 1); > > + if (!opt_ff || strcmp(opt_ff, "--ff-only")) { > + warning(_("Pulling without specifying how to reconcile divergent branches\n" > + "is discouraged. You can squelch this message by running one of the\n" > + "following 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. Alternatively, you can pass --rebase,\n" > + "--no-rebase, or --ff-only on the command line every time you pull.\n")); Ick; we don't need the warning to regurgitate large sections of the manual, we only need to provide pointers and then they can look up the details in their favorite way (google, stackoverflow, looking at the actual manpage, etc.) How about something more like > + warning(_("Pulling without specifying how to reconcile divergent branches\n" > + "is discouraged. You can squelch this message by setting pull.rebase,\n" > + "or by passing any of --rebase, --no-rebase, or --ff-only\n"));
On Sat, Feb 29, 2020 at 4:31 PM Elijah Newren <newren@gmail.com> wrote: > > Ick; we don't need the warning to regurgitate large sections of the > manual, we only need to provide pointers and then they can look up the > details in their favorite way (google, stackoverflow, looking at the > actual manpage, etc.) How about something more like > > > + warning(_("Pulling without specifying how to reconcile divergent branches\n" > > + "is discouraged. You can squelch this message by setting pull.rebase,\n" > > + "or by passing any of --rebase, --no-rebase, or --ff-only\n")); I intentionally made the message similar to the message you get when you try to commit without setting user.name or user.email. Providing the user with clear information on the spot is much more effective at getting them to do the right thing than expecting them to use Google or reread the man pages. We also want the user to understand that they only have to set pull.rebase or pull.ff once, whereas a command-line flag applies only to the current pull. Nevertheless, any warning message is better than no warning at all, so I would be happy to resubmit with your proposed text if there's consensus that a shorter message is better. -Alex
Alex Henrie <alexhenrie24@gmail.com> writes: > On Sat, Feb 29, 2020 at 4:31 PM Elijah Newren <newren@gmail.com> wrote: >> > ... We also want the user to understand that they > only have to set pull.rebase or pull.ff once, whereas a command-line > flag applies only to the current pull. I do not think the phrasing you used has the intended effect. You can replace "git config" with "git config --global" to set a default preference for all repositories. You can pass --rebase, --no-rebase, or --ff-only on the command line to override the configured default per invocation. may be more appropriate. "You can use it, but you need to do so every time you pull" drives readers away by placing stress on how cumbersome it *can* become, but in practice, nobody would use the same command line option every time anyway. In contrast, explaining the command line option as a useful mechanism for one-shot override, and the configuration variable as a useful mechanism for set-and-forget convenience feature, would teach readers in what situation use of the configuration variables and the command line options shine the best. Thanks.
Alex Henrie <alexhenrie24@gmail.com> writes: > v3: > - Fix segfault if opt_ff is NULL > - Don't say that the behavior will change in a future version of Git > (that hasn't been decided yet) > - Mention that pull.ff=only is also an option > - Mention git config's --global option > - Make the warning message longer and more clear in general > --- > builtin/pull.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) Another thing tests noticed was that this broke #5521.2.
diff --git a/builtin/pull.c b/builtin/pull.c index 3e624d1e00..a93ee489f6 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -327,6 +327,20 @@ static enum rebase_type config_get_rebase(void) if (!git_config_get_value("pull.rebase", &value)) return parse_config_rebase("pull.rebase", value, 1); + if (!opt_ff || strcmp(opt_ff, "--ff-only")) { + warning(_("Pulling without specifying how to reconcile divergent branches\n" + "is discouraged. You can squelch this message by running one of the\n" + "following 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. Alternatively, you can pass --rebase,\n" + "--no-rebase, or --ff-only on the command line every time you pull.\n")); + } + return REBASE_FALSE; }
Often novice Git users forget to say "pull --rebase" and end up with an unnecessary merge from upstream. What they usually want is either "pull --rebase" in the simpler cases, or "pull --ff-only" to update the copy of main integration branches, and rebase their work separately. The pull.rebase configuration variable exists to help them in the simpler cases, but there is no mechanism to make these users aware of it. Issue a warning message when no --[no-]rebase option from the command line and no pull.rebase configuration variable is given. This will inconvenience those who never want to "pull --rebase", who haven't had to do anything special, but the cost of the inconvenience is paid only once per user, which should be a reasonable cost to help a number of new users. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- v3: - Fix segfault if opt_ff is NULL - Don't say that the behavior will change in a future version of Git (that hasn't been decided yet) - Mention that pull.ff=only is also an option - Mention git config's --global option - Make the warning message longer and more clear in general --- builtin/pull.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)