Re: [PATCH v1] perf/aggregate: tighten option parsing

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 20, 2018, 18:27 UTC
Message-ID: <87h8o5ensj.fsf@evledraar.gmail.com>
In-Reply-To: <CAPig+cSDYVGpaV-beNVG57r3YfQ=Ey5zuyHRkk_tf86NNRdY6w@mail.gmail.com>

On Fri, Apr 20 2018, Eric Sunshine wrote:

Show 35 quoted lines

> On Fri, Apr 20, 2018 at 8:10 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> When passing an option '--foo' that it does not recognize, the
>> aggregate.perl script should die with an helpful error message
>> like:
>>
>>   unknown option '--foo' at ./aggregate.perl line 80.
>>
>> rather than:
>>
>>   fatal: Needed a single revision
>>   rev-parse --verify --foo: command returned error: 128
>>
>> While at it let's also prevent something like
>> 'foo--sort-by=regression' to be handled as if
>> '--sort-by=regression' had been used.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
>> @@ -46,7 +46,7 @@ while (scalar @ARGV) {
>> -       if ($arg =~ /--sort-by(?:=(.*))?/) {
>> +       if ($arg =~ /^--sort-by(?:=(.*))?$/) {
>
> Makes sense.
>
>> @@ -76,6 +76,9 @@ while (scalar @ARGV) {
>> +       if ($arg =~ /^--.+$/) {
>> +               die "unknown option '$arg'";
>> +       }
>
> Nit: In this expression, the trailing +$ makes the match no tighter
> than the simpler /^--./, so the latter would be good enough.
>
> Not necessarily worth a re-roll.

Not that it matters in this case, but just as a bit of Perl rx pedantry, yes his is tighter & more correct. You didn't consider how "." interacts with newlines:

    $ perl -wE 'my @rx = (qr/^--./, qr/^--.+$/, qr/^--./m, qr/^--.+$/m, qr/^--./s, qr/^--.+$/s); for (@rx) { my $s = "--foo\n--bar"; say $_, "\t", ($s =~ $_ ? 1 : 0) }'
    (?^u:^--.)      1
    (?^u:^--.+$)    0
    (?^um:^--.)     1
    (?^um:^--.+$)   1
    (?^us:^--.)     1
    (?^us:^--.+$)   1

I don't think it matters here, not like someone will pass \n in options to aggregate.perl...