| Message ID | 20201214202647.3340193-5-gitster@pobox.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7539fdc6290750c617c0b638c2f3b2f63c389ed3 |
| Headers | show |
| Series | making pull advice not to trigger when unneeded | expand |
Junio C Hamano wrote: > Refactor the advise() call that teaches users how they can choose > between merge and rebase into a helper function. This revealed that > the caller's logic needs to be further clarified to allow future > actions (like "erroring out" instead of the current "go ahead and > merge anyway") that should happen whether the advice message is > squelched out. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > builtin/pull.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index 2976b8e5cb..1b87ea95eb 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -925,6 +925,22 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_ > return ret; > } > > +static void show_advice_pull_non_ff(void) > +{ > + 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")); > +} While this change is good, I see a lot of advice code setting up a separate constant message, like: static const char message_advice_pull_non_ff[] = N_("..."); > int cmd_pull(int argc, const char **argv, const char *prefix) > { > const char *repo, **refspecs; > @@ -1028,19 +1044,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > if (opt_rebase && merge_heads.nr > 1) > die(_("Cannot rebase onto multiple branches.")); > > - if (rebase_unspecified && 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")); > + if (rebase_unspecified && !opt_ff) { > + if (opt_verbosity >= 0) > + show_advice_pull_non_ff(); Then, we could just do: if (opt_verbosity >= 0) advise(_(message_advice_pull_non_ff)). Or even better: if (opt_verbosity >= 0) advise_if_enabled(ADVICE_PULL_NON_FF, _(message_advice_pull_non_ff)); I'm not familiar with the advise code, I don't know if there's any good reason to setup these separate messages, so feel free to disregard this suggestion. Cheers.
Felipe Contreras <felipe.contreras@gmail.com> writes: > Then, we could just do: > > if (opt_verbosity >= 0) > advise(_(message_advice_pull_non_ff)). > > Or even better: > > if (opt_verbosity >= 0) > advise_if_enabled(ADVICE_PULL_NON_FF, _(message_advice_pull_non_ff)); I do not think we've decided what's the right way to squelch this advice, so it is a bit premature to favor the latter over the former. Between a fixed message[] variable and a single-purpose helper function I have no real preference. With either, we can reword the message easily without disrupting any improvements to the codeflow. At least the first sentence in the existing message needs to be separated out of the advice and turned into a separate error or a warning.
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > Then, we could just do: > > > > if (opt_verbosity >= 0) > > advise(_(message_advice_pull_non_ff)). > > > > Or even better: > > > > if (opt_verbosity >= 0) > > advise_if_enabled(ADVICE_PULL_NON_FF, _(message_advice_pull_non_ff)); > > I do not think we've decided what's the right way to squelch this > advice, so it is a bit premature to favor the latter over the > former. I agree. Just thinking about the future. > Between a fixed message[] variable and a single-purpose > helper function I have no real preference. OK. I just mentioned for consistencty with other parts of git.
diff --git a/builtin/pull.c b/builtin/pull.c index 2976b8e5cb..1b87ea95eb 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -925,6 +925,22 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_ return ret; } +static void show_advice_pull_non_ff(void) +{ + 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")); +} + int cmd_pull(int argc, const char **argv, const char *prefix) { const char *repo, **refspecs; @@ -1028,19 +1044,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (opt_rebase && merge_heads.nr > 1) die(_("Cannot rebase onto multiple branches.")); - if (rebase_unspecified && 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")); + if (rebase_unspecified && !opt_ff) { + if (opt_verbosity >= 0) + show_advice_pull_non_ff(); } if (opt_rebase) {
Refactor the advise() call that teaches users how they can choose between merge and rebase into a helper function. This revealed that the caller's logic needs to be further clarified to allow future actions (like "erroring out" instead of the current "go ahead and merge anyway") that should happen whether the advice message is squelched out. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/pull.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)