From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 30 Oct 2023 14:11:09 GMT
Subject: Re: [PATCH v3] bugreport: reject positional arguments
Message-ID: <881c7fea-47bb-45a9-b6e3-314f9ed9e0cd@gmail.com>
In-Reply-To: <xmqqcywwg9am.fsf@gitster.g>

On 30/10/2023 01:59, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>>
>>>> It is rather unfortunate that test_i18ngrep was deprecated without
>>>> providing an alternative that offers the same debugging
>>>> experience.
>>> ...
>>> We could rename test_i18ngrep to test_grep (and make test_i18ngrep
>>> into a thin wrapper with warnings).
>>>
>>> 	test_grep -e must-exist file &&
>>> 	test_grep ! -e must-not-exist file
>>
>> ... as the only remaining part in test_18ngrep has no hack to work
>> around the tainted localization tests, so "was deprecated without"
>> is a bit too strong.  There is nothing we have lost yet.
> 
> Having said all that, when re-reading the test_i18ngrep with a fresh
> pair of eyes, I somehow doubt there was much upside in "debugging
> experience" with test_i18ngrep in the first place, and I doubt if
> retaining it with a new name test_grep has much value.
> 
> Given that test_i18ngrep (hence test_grep) requires you to have the
> haystack in a file, between
> 
>      test_i18ngrep must-exist file &&
>      test_i18ngrep ! must-not-exist file
> 
> and
> 
>      grep must-exist file &&
>      ! grep must-not-exist file
> 
> I do not see any difference in "debugging experience" when you run
> the test with "-i [-v] -d".   The two cases you care about are
> 
>   (1) the test expects the string "must-exist" in the file "file" but
>       the string is not there.
> 
>   (2) the test expects the string "must-not-exist" missing from the
>       file "file", but the string is there.
> 
> The latter can clearly be seen in output from "-i -v -d" (the "grep"
> outputs a line with "must-not-exist" on it).  The former will show
> silence but since you are debugging with "-d", and your haystack is
> in a file, after such a step fails, the test stops, and without
> removing the "file" even if the test piece had test_when_finished
> to remove it (i.e. running tests in debugging mode "-d" and
> immediately stopping upon failure "-i" behaves this way exactly to
> help you debugging), so you can go there to the TRASH_DIRECTORY
> yourself and inspect "file" to see what is going on anyway.
>
> So, I dunno.  Surely with a long &&-chain of steps, where a grep
> that expects lack of something is in the middle, it is hard to see
> if the lack of hit is because an earlier step failed (and the
> control did not reach "grep must-exist file") or because the
> haystack lacked the "must-exist" needle, so from that point of view,
> it may be nicer that "did not find an expected match" is explicitly
> stated.

It is this latter point that I had in mind. I find it much easier to 
debug a test that says "This command failed" rather than looking at the 
output to try and figure out which was the last successful command. I 
take your point above that one can go and inspect the file when the test 
is run with "-i -d" but it is determining that grep failed in the first 
place that I find annoying. I've also found the output from 
test_i18ngrep is helpful when debugging CI test failures.

Best Wishes

Phillip