Re: [PATCH v2] reset: Better warning message on git reset --mixed <paths>

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 16, 2010, 04:23 UTC
Message-ID: <AANLkTimHQ7c=F8y50Keq1eo5aC+D9fuaF7Eye5GJj1t-@mail.gmail.com>
In-Reply-To: <7vmxsndy5b.fsf@alter.siamese.dyndns.org>

On Mon, Aug 16, 2010 at 03:39, Junio C Hamano <gitster@pobox.com> wrote:

Show 41 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> When you call "git reset --mixed <paths>" git will complain that using
>> mixed with paths is deprecated:
>>
>>     warning: --mixed option is deprecated with paths.
>>
>> That doesn't tell the user why it's deprecated, or what he should use
>> instead. Expand on the warning and tell the user to just omit --mixed:
>>
>>     warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead
>>
>> The exact wording of the warning was suggested by Jonathan Nieder.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> On Sat, Aug 14, 2010 at 21:05, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> Maybe:
>>>
>>>  warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead
>>
>> That's better, thanks. Here's an amended version, and with tests this
>> time.
>
> While the new message is an improvement, I do not think the added test
> that checks the exact wording is good.
>
> Think for 5 seconds what the expected code change would be to break that
> test.  I can only think of two realistic cases.  (1) Command line parsing
> is broken and "reset --mixed <pathspec>" does not go through the codepath
> that produces this warning anymore; (2) we deem that we had the
> deprecation long enough and error out on this usage; or (3) Somebody comes
> up with an even better wording but the string in this test still expects
> suboptimal warning.
>
> When we change this to an error, that is a behaviour change (it will
> change the exit status as well), and it would be Ok to force the person
> who does such a change to update the test.  But (3) shows that this test
> is making it harder for people to improve the wording than necessary;
> isn't it sufficient to check if any warning is issued at all?

I thought we might test for deprecation warnings, since they're a step above general warnings as they imply a deprecation cycle.

I'm just used to a codebase where every single warning message of that sort is explicitly tested for, so I mostly did that out of habit.

> I personally do not think this deserves to consume a new test number,
> which is rather a scarce resource.

Just drop the test file from the patch, I don't think it's really needed, and maybe drop the patch altogether. I don't know what we want to do about this reset behavior in general.

Show 12 quoted lines

>> diff --git a/t/t7112-reset-messages.sh b/t/t7112-reset-messages.sh
>> new file mode 100755
>> index 0000000..6f2669b
>> --- /dev/null
>> +++ b/t/t7112-reset-messages.sh
>> @@ -0,0 +1,33 @@
>> ...
>> +test_expect_success 'git reset --mixed <paths> warning' '
>> +     # Not test_commit() due to "ambiguous argument [..] both revision
>> +     # and filename"
>
> Huh?

I think I forgot that test_commit could take two args there.