{"thread":{"id":"48360","subject":"[PATCH v2] perf/aggregate: use Getopt::Long for option parsing","startedAt":"2018-04-25T16:13:09Z","lastAt":"2018-04-25T16:13:09Z","messageCount":1,"participants":["Christian Couder"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"345809","messageId":"20180425161025.3242-1-chriscool@tuxfamily.org","threadId":"48360","inReplyTo":null,"subject":"[PATCH v2] perf/aggregate: use Getopt::Long for option parsing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-04-25T16:10:25Z","receivedAt":"2018-04-25T16:13:09Z","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\nUnknown option: foo\n./aggregate.perl [options] [--] [<dir_or_rev>...] [--] \\\n[<test_script>...] >\n\n  Options:\n    --codespeed          * Format output for Codespeed\n    --reponame    <str>  * Send given reponame to codespeed\n    --sort-by     <str>  * Sort output (only \"regression\" \\\ncriteria is supported)\n\nrather than:\n\n  fatal: Needed a single revision\n  rev-parse --verify --foo: command returned error: 128\n\nTo implement that let's use Getopt::Long for option parsing\ninstead of the current manual and sloppy parsing. This should\nsave some code and make option parsing simpler, tighter and\nsafer.\n\nThis will avoid something like 'foo--sort-by=regression' to\nbe handled as if '--sort-by=regression' had been used, for\nexample.\n\nAs Getopt::Long eats '--' at the end of options, this changes\na bit the way '--' is handled as we can now have '--' both\nafter the options and before the scripts.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\nActually it looks like we should not use:\n\n  Getopt::Long::Configure qw/ pass_through /;\n\nas GetOptions() would not error out when an unknown option\nlike --foo is used.\n\n t/perf/aggregate.perl | 62 ++++++++++++++++++-------------------------\n 1 file changed, 26 insertions(+), 36 deletions(-)\n\ndiff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\nindex 48637ef64b..bc865160e7 100755\n--- a/t/perf/aggregate.perl\n+++ b/t/perf/aggregate.perl\n@@ -4,6 +4,7 @@ use lib '../../perl/build/lib';\n use strict;\n use warnings;\n use JSON;\n+use Getopt::Long;\n use Git;\n \n sub get_times {\n@@ -36,46 +37,34 @@ sub format_times {\n \treturn $out;\n }\n \n+sub usage {\n+\tprint <<EOT;\n+./aggregate.perl [options] [--] [<dir_or_rev>...] [--] [<test_script>...] >\n+\n+  Options:\n+    --codespeed          * Format output for Codespeed\n+    --reponame    <str>  * Send given reponame to codespeed\n+    --sort-by     <str>  * Sort output (only \"regression\" criteria is supported)\n+    --subsection  <str>  * Use results from given subsection\n+\n+EOT\n+\texit(1);\n+}\n+\n my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,\n     $codespeed, $sortby, $subsection, $reponame);\n+\n+Getopt::Long::Configure qw/ require_order /;\n+\n+my $rc = GetOptions(\"codespeed\"     => \\$codespeed,\n+\t\t    \"reponame=s\"    => \\$reponame,\n+\t\t    \"sort-by=s\"     => \\$sortby,\n+\t\t    \"subsection=s\"  => \\$subsection);\n+usage() unless $rc;\n+\n while (scalar @ARGV) {\n \tmy $arg = $ARGV[0];\n \tmy $dir;\n-\tif ($arg eq \"--codespeed\") {\n-\t\t$codespeed = 1;\n-\t\tshift @ARGV;\n-\t\tnext;\n-\t}\n-\tif ($arg =~ /--sort-by(?:=(.*))?/) {\n-\t\tshift @ARGV;\n-\t\tif (defined $1) {\n-\t\t\t$sortby = $1;\n-\t\t} else {\n-\t\t\t$sortby = shift @ARGV;\n-\t\t\tif (! defined $sortby) {\n-\t\t\t\tdie \"'--sort-by' requires an argument\";\n-\t\t\t}\n-\t\t}\n-\t\tnext;\n-\t}\n-\tif ($arg eq \"--subsection\") {\n-\t\tshift @ARGV;\n-\t\t$subsection = $ARGV[0];\n-\t\tshift @ARGV;\n-\t\tif (! $subsection) {\n-\t\t\tdie \"empty subsection\";\n-\t\t}\n-\t\tnext;\n-\t}\n-\tif ($arg eq \"--reponame\") {\n-\t\tshift @ARGV;\n-\t\t$reponame = $ARGV[0];\n-\t\tshift @ARGV;\n-\t\tif (! $reponame) {\n-\t\t\tdie \"empty reponame\";\n-\t\t}\n-\t\tnext;\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@@ -225,7 +214,8 @@ sub print_sorted_results {\n \tmy ($sortby) = @_;\n \n \tif ($sortby ne \"regression\") {\n-\t\tdie \"only 'regression' is supported as '--sort-by' argument\";\n+\t\tprint \"Only 'regression' is supported as '--sort-by' argument\\n\";\n+\t\tusage();\n \t}\n \n \tmy @evolutions;\n-- \n2.17.0.257.g28b659db43\n\n"}]}