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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 11, 2020, 06:54 UTC
Message-ID: <xmqqy2i4ltnv.fsf@gitster.c.googlers.com>
In-Reply-To: <20201210100538.696787-3-felipe.contreras@gmail.com>

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

Show 5 quoted lines

> 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.

Show 14 quoted lines

> +	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.