On Mon, Aug 16, 2010 at 03:39, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx> >> --- >> >> On Sat, Aug 14, 2010 at 21:05, Jonathan Nieder <jrnieder@xxxxxxxxx> 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. >> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html