{"thread":{"id":"48332","subject":"[PATCH v1] perf/aggregate: tighten option parsing","startedAt":"2018-04-20T12:11:14Z","lastAt":"2018-04-25T12:55:29Z","messageCount":5,"participants":["Christian Couder","Eric Sunshine","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"345212","messageId":"20180420121041.32558-1-chriscool@tuxfamily.org","threadId":"48332","inReplyTo":null,"subject":"[PATCH v1] perf/aggregate: tighten option parsing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-04-20T12:10:41Z","receivedAt":"2018-04-20T12:11:14Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When passing an option '--foo' that it does not recognize, the\naggregate.perl script should die with an helpful error message\nlike:\n\n unknown option '--foo' at ./aggregate.perl line 80.\n\nrather than:\n\n fatal: Needed a single revision\n rev-parse --verify --foo: command returned error: 128\n\nWhile at it let's also prevent something like\n'foo--sort-by=regression' to be handled as if\n'--sort-by=regression' had been used.\n\nSigned-off-by: Christian Couder \n---\n t/perf/aggregate.perl | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\nindex 48637ef64b..2cc9eb0ac5 100755\n--- a/t/perf/aggregate.perl\n+++ b/t/perf/aggregate.perl\n@@ -46,7 +46,7 @@ while (scalar @ARGV) {\n \t\tshift @ARGV;\n \t\tnext;\n \t}\n-\tif ($arg =~ /--sort-by(?:=(.*))?/) {\n+\tif ($arg =~ /^--sort-by(?:=(.*))?$/) {\n \t\tshift @ARGV;\n \t\tif (defined $1) {\n \t\t\t$sortby = $1;\n@@ -76,6 +76,9 @@ while (scalar @ARGV) {\n \t\t}\n \t\tnext;\n \t}\n+\tif ($arg =~ /^--.+$/) {\n+\t\tdie \"unknown option '$arg'\";\n+\t}\n \tlast if -f $arg or $arg eq \"--\";\n \tif (! -d $arg) {\n \t\tmy $rev = Git::command_oneline(qw(rev-parse --verify), $arg);\n-- \n2.17.0.257.g28b659db43\n\n"},{"id":"345241","messageId":"CAPig+cSDYVGpaV-beNVG57r3YfQ=Ey5zuyHRkk_tf86NNRdY6w@mail.gmail.com","threadId":"48332","inReplyTo":"20180420121041.32558-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v1] perf/aggregate: tighten option parsing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-20T17:46:59Z","receivedAt":"2018-04-20T17:47:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 20, 2018 at 8:10 AM, Christian Couder\n wrote:\n> When passing an option '--foo' that it does not recognize, the\n> aggregate.perl script should die with an helpful error message\n> like:\n>\n> unknown option '--foo' at ./aggregate.perl line 80.\n>\n> rather than:\n>\n> fatal: Needed a single revision\n> rev-parse --verify --foo: command returned error: 128\n>\n> While at it let's also prevent something like\n> 'foo--sort-by=regression' to be handled as if\n> '--sort-by=regression' had been used.\n>\n> Signed-off-by: Christian Couder \n> ---\n> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\n> @@ -46,7 +46,7 @@ while (scalar @ARGV) {\n> - if ($arg =~ /--sort-by(?:=(.*))?/) {\n> + if ($arg =~ /^--sort-by(?:=(.*))?$/) {\n\nMakes sense.\n\n> @@ -76,6 +76,9 @@ while (scalar @ARGV) {\n> + if ($arg =~ /^--.+$/) {\n> + die \"unknown option '$arg'\";\n> + }\n\nNit: In this expression, the trailing +$ makes the match no tighter\nthan the simpler /^--./, so the latter would be good enough.\n\nNot necessarily worth a re-roll.\n"},{"id":"345246","messageId":"87h8o5ensj.fsf@evledraar.gmail.com","threadId":"48332","inReplyTo":"CAPig+cSDYVGpaV-beNVG57r3YfQ=Ey5zuyHRkk_tf86NNRdY6w@mail.gmail.com","subject":"Re: [PATCH v1] perf/aggregate: tighten option parsing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-20T18:27:24Z","receivedAt":"2018-04-20T18:28:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 20 2018, Eric Sunshine wrote:\n\n> On Fri, Apr 20, 2018 at 8:10 AM, Christian Couder\n> wrote:\n>> When passing an option '--foo' that it does not recognize, the\n>> aggregate.perl script should die with an helpful error message\n>> like:\n>>\n>> unknown option '--foo' at ./aggregate.perl line 80.\n>>\n>> rather than:\n>>\n>> fatal: Needed a single revision\n>> rev-parse --verify --foo: command returned error: 128\n>>\n>> While at it let's also prevent something like\n>> 'foo--sort-by=regression' to be handled as if\n>> '--sort-by=regression' had been used.\n>>\n>> Signed-off-by: Christian Couder \n>> ---\n>> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\n>> @@ -46,7 +46,7 @@ while (scalar @ARGV) {\n>> - if ($arg =~ /--sort-by(?:=(.*))?/) {\n>> + if ($arg =~ /^--sort-by(?:=(.*))?$/) {\n>\n> Makes sense.\n>\n>> @@ -76,6 +76,9 @@ while (scalar @ARGV) {\n>> + if ($arg =~ /^--.+$/) {\n>> + die \"unknown option '$arg'\";\n>> + }\n>\n> Nit: In this expression, the trailing +$ makes the match no tighter\n> than the simpler /^--./, so the latter would be good enough.\n>\n> Not necessarily worth a re-roll.\n\nNot that it matters in this case, but just as a bit of Perl rx pedantry,\nyes his is tighter & more correct. You didn't consider how \".\" interacts\nwith newlines:\n\n $ 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) }'\n (?^u:^--.) 1\n (?^u:^--.+$) 0\n (?^um:^--.) 1\n (?^um:^--.+$) 1\n (?^us:^--.) 1\n (?^us:^--.+$) 1\n\nI don't think it matters here, not like someone will pass \\n in options\nto aggregate.perl...\n"},{"id":"345304","messageId":"xmqqd0ytb4le.fsf@gitster-ct.c.googlers.com","threadId":"48332","inReplyTo":"87h8o5ensj.fsf@evledraar.gmail.com","subject":"Re: [PATCH v1] perf/aggregate: tighten option parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-21T03:50:21Z","receivedAt":"2018-04-21T03:50:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> Not necessarily worth a re-roll.\n>\n> Not that it matters in this case, but just as a bit of Perl rx pedantry,\n> yes his is tighter & more correct. You didn't consider how \".\" interacts\n> with newlines:\n>\n> $ 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) }'\n> (?^u:^--.) 1\n> (?^u:^--.+$) 0\n> (?^um:^--.) 1\n> (?^um:^--.+$) 1\n> (?^us:^--.) 1\n> (?^us:^--.+$) 1\n>\n> I don't think it matters here, not like someone will pass \\n in options\n> to aggregate.perl...\n\nHmph, do we want the command not to barf when \"--foo\\n--bar\" is\ngiven from the command line and we cannot find such an option?\n\nI thought that the location the match under discussion is used does\nwant to see a hit with any option looking string that begins with\ndouble dashes. I would have expected \"tigher and hence incorrect\",\nin other words.\n\nSomewhat puzzled...\n"},{"id":"345782","messageId":"CAP8UFD1nkNEbbERvyhSGfSLBDNfD8QcmrYNRy_3J4Mrb7z2ekw@mail.gmail.com","threadId":"48332","inReplyTo":"xmqqd0ytb4le.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] perf/aggregate: tighten option parsing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-04-25T12:55:15Z","receivedAt":"2018-04-25T12:55:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Apr 21, 2018 at 5:50 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>> Not necessarily worth a re-roll.\n>>\n>> Not that it matters in this case, but just as a bit of Perl rx pedantry,\n>> yes his is tighter & more correct. You didn't consider how \".\" interacts\n>> with newlines:\n>>\n>> $ 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) }'\n>> (?^u:^--.) 1\n>> (?^u:^--.+$) 0\n>> (?^um:^--.) 1\n>> (?^um:^--.+$) 1\n>> (?^us:^--.) 1\n>> (?^us:^--.+$) 1\n>>\n>> I don't think it matters here, not like someone will pass \\n in options\n>> to aggregate.perl...\n>\n> Hmph, do we want the command not to barf when \"--foo\\n--bar\" is\n> given from the command line and we cannot find such an option?\n>\n> I thought that the location the match under discussion is used does\n> want to see a hit with any option looking string that begins with\n> double dashes. I would have expected \"tigher and hence incorrect\",\n> in other words.\n>\n> Somewhat puzzled...\n\nI guess it might be better at this point to just \"use Getopt::Long;\"\n(along with \"Getopt::Long::Configure qw/ pass_through /;\") as git\nsend-email does. It might avoid mistakes and subtle discussions like\nthe above.\n\nThanks for the reviews,\nChristian.\n"}]}