# [PATCH v1] perf/aggregate: tighten option parsing 5 messages from 2018-04-20 to 2018-04-25. Participants: Christian Couder, Eric Sunshine, Ævar Arnfjörð Bjarmason, Junio C Hamano. Thread: https://gitlist.dev/t/48332 ## Christian Couder, 2018-04-20 12:10 Subject: [PATCH v1] perf/aggregate: tighten option parsing Message-ID: <20180420121041.32558-1-chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20180420121041.32558-1-chriscool%40tuxfamily.org ``` 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 --- 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 ``` ## Eric Sunshine, 2018-04-20 17:46 Subject: Re: [PATCH v1] perf/aggregate: tighten option parsing Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSDYVGpaV-beNVG57r3YfQ%3DEy5zuyHRkk_tf86NNRdY6w%40mail.gmail.com In-Reply-To: <20180420121041.32558-1-chriscool@tuxfamily.org> ``` On Fri, Apr 20, 2018 at 8:10 AM, Christian Couder 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 > --- > 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. ``` ## Ævar Arnfjörð Bjarmason, 2018-04-20 18:27 Subject: Re: [PATCH v1] perf/aggregate: tighten option parsing Message-ID: <87h8o5ensj.fsf@evledraar.gmail.com> URL: https://gitlist.dev/e/87h8o5ensj.fsf%40evledraar.gmail.com In-Reply-To: ``` On Fri, Apr 20 2018, Eric Sunshine wrote: > On Fri, Apr 20, 2018 at 8:10 AM, Christian Couder > 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 >> --- >> 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... ``` ## Junio C Hamano, 2018-04-21 03:50 Subject: Re: [PATCH v1] perf/aggregate: tighten option parsing Message-ID: URL: https://gitlist.dev/e/xmqqd0ytb4le.fsf%40gitster-ct.c.googlers.com In-Reply-To: <87h8o5ensj.fsf@evledraar.gmail.com> ``` Ævar Arnfjörð Bjarmason 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... ``` ## Christian Couder, 2018-04-25 12:55 Subject: Re: [PATCH v1] perf/aggregate: tighten option parsing Message-ID: URL: https://gitlist.dev/e/CAP8UFD1nkNEbbERvyhSGfSLBDNfD8QcmrYNRy_3J4Mrb7z2ekw%40mail.gmail.com In-Reply-To: ``` On Sat, Apr 21, 2018 at 5:50 AM, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason 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. ```