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

[Felipe Contreras <felipe.contreras@gmail.com>]

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