Re: [PATCH v4] pull: warn if the user didn't say whether to rebase or to merge

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 4, 2020, 17:18 UTC
Message-ID: <xmqq4kv45995.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20200304022931.2469455-1-alexhenrie24@gmail.com>

Alex Henrie <alexhenrie24@gmail.com> writes:

> - Revise warning message based on Junio's feedback
> - Consistently wrap warning lines to 75 characters for easy viewing in
> PO files

Nice to see attention to such a detail ;-)

> - Fix test failures

Ah, OK, hmmm.

For --quiet test, that wants to ensure that "pull --quiet" does not say anything, it certainly stops the test from failing if we set the configuration before executing such a test, but I wonder if that is in line with the spirit of the feature the test tries to protect in the first place. I would imagine those who write "pull --quiet" in automation would not want to see any non-error message, and because this is not an error, they do not want to see any output. Shouldn't such a use of "pull --quiet" bypass this warning, too?

Show 14 quoted lines

> ---
>  builtin/pull.c          | 15 +++++++++++++++
>  t/t5521-pull-options.sh |  3 ++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 3e624d1e00..351b933c4d 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -327,6 +327,21 @@ static enum rebase_type config_get_rebase(void)
>  	if (!git_config_get_value("pull.rebase", &value))
>  		return parse_config_rebase("pull.rebase", value, 1);
>  
> +	if (!opt_ff || strcmp(opt_ff, "--ff-only")) {

If we want to squelch the warning under "--quiet", I think we can do this.

	if (0 < opt_verbosity &&
	    (!opt_ff || strcmp(opt_ff, "--ff-only"))) {

and ...

Show 13 quoted lines

> +		warning(_("Pulling without specifying how to reconcile divergent branches is\n"
> ...
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index ccde8ba491..6e890ec936 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -8,7 +8,8 @@ test_expect_success 'setup' '
>  	mkdir parent &&
>  	(cd parent && git init &&
>  	 echo one >file && git add file &&
> -	 git commit -m one)
> +	 git commit -m one) &&
> +	git config pull.rebase false

... this change can safely go.

If we agree that "git pull --quiet" should stay quiet in an environment where the user has been happy with the default choice, that is. I am raising this issue to invite others to think about it, and I am on the fence, but I am leaning towards saying "yes".

>  '

Regardless of what the resolution for "pull --quiet" would be, shouldn't we have a test for this change to ensure that we do warn under the condition we should, and that we do not do so when we should not?

Thanks.