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

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

Junio C Hamano <gitster@pobox.com> writes:
>
> 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?

Actually, the change to t/t5521 does not even sweep the problem
under the rug X-<

>> 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 affects the repository at the top of the trash directory, but
the test immediately after this one looks like this:

    test_expect_success 'git pull -q' '
            mkdir clonedq &&
            (cd clonedq && git init &&
            git pull -q "../parent" >out 2>err &&
            test_must_be_empty err &&
            test_must_be_empty out)
    '

The patch added pull.rebase to "t/trash directory.5521/.git/config",
but it would not affect this "git pull -q", that is run in the
repository that uses "t/trash directory.5521/clonedq/.git/config"
as its configuration.

For now, I added the following patch on top of the topic before
queuing it to 'pu'.

Thanks.

-- >8 --
Subject: [PATCH] SQUASH???

Let's declare that for the purpose of "pull --quiet", the new "you
might have created a merge when you may wanted to rebase" warning is
an unwanted noise.

Revert the hack made to t5521; the second test that creates a new
repository and runs a pull inside it will not get affected by the
local configuration setting it makes to the original trash
repository and the breakage won't be worked around with it anyways.

This is not a full remedy for the previous step, yet.  The topic
needs a test to ensure the warning is emitted when it should be.
Existing t5521 is a test that ensures the warning is not given in
one of the conditions it should not be (i.e. when --quiet is in
effect).  Negative tests for other cases (e.g. when --ff-only is
given from the command line, without pull.rebase configured) should
also be added.
---
 builtin/pull.c          | 3 ++-
 t/t5521-pull-options.sh | 3 +--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 351b933c4d..0ef192fd64 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -327,7 +327,8 @@ 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 (0 <= opt_verbosity &&
+	    (!opt_ff || strcmp(opt_ff, "--ff-only"))) {
 		warning(_("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"
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 6e890ec936..ccde8ba491 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -8,8 +8,7 @@ test_expect_success 'setup' '
 	mkdir parent &&
 	(cd parent && git init &&
 	 echo one >file && git add file &&
-	 git commit -m one) &&
-	git config pull.rebase false
+	 git commit -m one)
 '
 
 test_expect_success 'git pull -q' '
-- 
2.25.1-595-g3aa4872962