Re: [PATCH v5 2/3] pull: move default warning

[Junio C Hamano <gitster@pobox.com>]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Eventually we want to be able to display the warning only when
> fast-forward merges are not possible.
>
> In order to do so we need to move the default warning up to the point
> where we can check if we can fast-forward or not.

Makes sense.

> Additionally, config_get_rebase() was probably never its true home.

I agree with this point.  I've always found it suboptimal.

> This requires a temporary variable to check if we are in the
> "default mode" (no --rebase or --no-rebase specified).

Two points:

 - "mode" is so overused a word; a more focused word is preferrable.

 - by introducing a local variable in cmd_pull() and passing a
   pointer to it to config_get_rebase(), we can easily avoid having
   to rely on an extra global variable.

I'd suggest addressing the above along the following lines.

   -static enum rebase_type config_get_rebase(void)
   +static enum rebase_type config_get_rebase(int *rebase_unspecified)
    {
   +        *rebase_unspecified = 0;

            ... various "return" of configured values ...

   +        *rebase_unspecified = 1;
            return REBASE_FALSE;
    }

Then the caller would declare

	int rebase_unspecified = 0;

and call 

	if (opt_rebase < 0)
		opt_rebase = config_get_rebase(&rebase_unspecified);

to possibly cause it to set to true, and use that instead of the
global variable to decide if we want to give the help text.  When
the helper is not called due to opt_rebase already being set, it is
not using configured value but using the choice from the command
line, so rebase_unspecified is still false after this point.

> @@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  	if (opt_rebase && merge_heads.nr > 1)
>  		die(_("Cannot rebase onto multiple branches."));

And this is the point where we finish various error checks and
starts to run either rebase or merge.  It is as late as we could
delay the "non-ff and you are not configured" message.  In other
words, the place chosen in cmd_pull() to move this code to is
optimal.

> +	if (default_mode && 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"));
> +	}

Either as a part of this step, as a part of the next step, or a
separate follow-up patch, we should

 - create a single-purpose helper function that only calls advise()
   with the message and returns; name it show_advice_pull_non_ff().

 - correct the if() statement above, so that regardless of verbosity
   level, we can do _something_ common when the history does not
   fast-forward.  I.e.

	if (rebase_unspecified && !opt_ff) {
		if (opt_verbosity >= 0)
			show_advice_pull_non_ff();
	}

These would allow us to further turn the logic to

	if (rebase_unspecified && !opt_ff) {
		if (opt_verbosity >= 0 && advice_pull_non_ff)
			show_advice_pull_non_ff();
		die("not a fast-forward; must merge or rebase");
	}

later in the far future, and we do not want that die() to be
affected by verbosity settings.

I'll queue such a fix-up patch on top of the series before pushing
the integration results out on 'seen'.

Thanks.