[PATCH v1] perf/aggregate: tighten option parsing

[PATCH v1] perf/aggregate: tighten option parsing

[Christian Couder]

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>
---
 t/perf/aggregate.perl | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index 48637ef64b..2cc9eb0ac5 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -46,7 +46,7 @@ while (scalar @ARGV) {
 		shift @ARGV;
 		next;
 	}
-	if ($arg =~ /--sort-by(?:=(.*))?/) {
+	if ($arg =~ /^--sort-by(?:=(.*))?$/) {
 		shift @ARGV;
 		if (defined $1) {
 			$sortby = $1;
@@ -76,6 +76,9 @@ while (scalar @ARGV) {
 		}
 		next;
 	}
+	if ($arg =~ /^--.+$/) {
+		die "unknown option '$arg'";
+	}
 	last if -f $arg or $arg eq "--";
 	if (! -d $arg) {
 		my $rev = Git::command_oneline(qw(rev-parse --verify), $arg);
-- 
2.17.0.257.g28b659db43

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

[Eric Sunshine]

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.

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

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 20 2018, Eric Sunshine wrote:

> 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...

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> 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...

Hmph, do we want the command not to barf when "--foo\n--bar" is
given from the command line and we cannot find such an option?

I thought that the location the match under discussion is used does
want to see a hit with any option looking string that begins with
double dashes.  I would have expected "tigher and hence incorrect",
in other words.

Somewhat puzzled...

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

[Christian Couder]

On Sat, Apr 21, 2018 at 5:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> 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...
>
> Hmph, do we want the command not to barf when "--foo\n--bar" is
> given from the command line and we cannot find such an option?
>
> I thought that the location the match under discussion is used does
> want to see a hit with any option looking string that begins with
> double dashes.  I would have expected "tigher and hence incorrect",
> in other words.
>
> Somewhat puzzled...

I guess it might be better at this point to just "use Getopt::Long;"
(along with "Getopt::Long::Configure qw/ pass_through /;") as git
send-email does. It might avoid mistakes and subtle discussions like
the above.

Thanks for the reviews,
Christian.