Re: [PATCH v3] bugreport: reject positional arguments

[Phillip Wood <phillip.wood123@gmail.com>]

On 26/10/2023 18:18, Eric Sunshine wrote:
> On Thu, Oct 26, 2023 at 11:55 AM <emilyshaffer@google.com> wrote:
>> git-bugreport already rejected unrecognized flag arguments, like
>> `--diaggnose`, but this doesn't help if the user's mistake was to forget
>> the `--` in front of the argument. This can result in a user's intended
>> argument not being parsed with no indication to the user that something
>> went wrong. Since git-bugreport presently doesn't take any positionals
>> at all, let's reject all positionals and give the user a usage hint.
>>
>> Signed-off-by: Emily Shaffer <nasamuffin@google.com>
>> ---
>> Per Eric's and Dragan's comments, dropped the null checking for argv[0].
>> No point in being too paranoid, I suppose :)
>>
>> Note that after this morning it's not likely that I'll be able to find
>> time to update this again so quickly, so if there are other nits,
>> reviewers can feel free to send their own rerolls rather than waiting
>> for me to see it and turn the patch around.
> 
> Thanks. This version looks good enough to me. Just one minor comment below...
> 
>> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
>> @@ -69,6 +69,13 @@ test_expect_success 'incorrect arguments abort with usage' '
>> +test_expect_success 'incorrect positional arguments abort with usage and hint' '
>> +       test_must_fail git bugreport false 2>output &&
>> +       test_i18ngrep usage output &&
>> +       test_i18ngrep false output &&
>> +       test_path_is_missing git-bugreport-*
>> +'
> 
> I didn't really pay attention to the test in earlier rounds so didn't
> notice this, but these days we just use 'grep' rather than
> 'test_i18ngrep'. (Indeed, the existing tests in this script use
> 'grep'.)

It is rather unfortunate that test_i18ngrep was deprecated without 
providing an alternative that offers the same debugging experience. When 
test_i18ngrep fails it prints a message with the pattern and text that 
failed to match so it is easy to see where the test failed. If grep 
fails there is no output and so unless the test is run with "-x" it can 
be hard to see which command caused the test to fail.

Best Wishes

Phillip