Re: [PATCH v2] reset: Better warning message on git reset --mixed <paths>
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 16, 2010, 03:39 UTC
- Message-ID
- <7vmxsndy5b.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1281861800-424-1-git-send-email-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 22 quoted lines
> 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 personally do not think this deserves to consume a new test number, which is rather a scarce resource.
Show 10 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?