git/list[1] front-page[2] threads[3] people[4] search[5] about
 

RE: [PATCH v7 4/5] pull: correct condition to trigger non-ff advice

From
Felipe Contreras <felipe.contreras@gmail.com>
Date
Dec 14, 2020, 21:17 UTC
Message-ID
<5fd7d67c898e4_d44af20879@natae.notmuch>
In-Reply-To
<20201214202647.3340193-5-gitster@pobox.com>
Junio C Hamano wrote:
Show 35 quoted lines
> 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_("...");
Show 23 quoted lines
>  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
Previous: Junio C HamanoNext: Junio C Hamano
Message 8 of 21 in “making pull advice not to trigger when unneeded”
  1. 0/5 making pull advice not to trigger when unneededJunio C Hamano, Dec 14, 2020
  2. 2/5 pull: give the advice for choosing rebase/merge much laterJunio C Hamano, Dec 14, 2020
  3. 3/5 pull: get rid of unnecessary global variableJunio C Hamano, Dec 14, 2020
  4. Felipe ContrerasDec 14, 2020
  5. Junio C HamanoDec 14, 2020
  6. Felipe ContrerasDec 15, 2020
  7. 4/5 pull: correct condition to trigger non-ff adviceJunio C Hamano, Dec 14, 2020
  8. Felipe ContrerasDec 14, 2020
  9. Junio C HamanoDec 14, 2020
  10. Felipe ContrerasDec 15, 2020
  11. 1/5 pull: refactor fast-forward checkJunio C Hamano, Dec 14, 2020
  12. 5/5 pull: display default warning only when non-ffJunio C Hamano, Dec 14, 2020
  13. Felipe ContrerasDec 14, 2020
  14. Junio C HamanoDec 14, 2020
  15. Felipe ContrerasDec 15, 2020
  16. Felipe ContrerasDec 15, 2020
  17. Junio C HamanoDec 15, 2020
  18. Felipe ContrerasDec 15, 2020
  19. Felipe ContrerasDec 18, 2020
  20. Junio C HamanoDec 23, 2020
  21. Felipe ContrerasDec 23, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.