[PATCH v2] perf/aggregate: use Getopt::Long for option parsing

Christian Couder· Apr 25, 2018, 16:10 UTC · lore

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 ./aggregate.perl [options] [--] [<dir_or_rev>...] [--] \ [<test_script>...] >

  Options:
    --codespeed          * Format output for Codespeed
    --reponame    <str>  * Send given reponame to codespeed
    --sort-by     <str>  * Sort output (only "regression" \
criteria is supported)

rather than:

  fatal: Needed a single revision
  rev-parse --verify --foo: command returned error: 128

To implement that let's use Getopt::Long for option parsing instead of the current manual and sloppy parsing. This should save some code and make option parsing simpler, tighter and safer.

This will avoid something like 'foo--sort-by=regression' to be handled as if '--sort-by=regression' had been used, for example.

As Getopt::Long eats '--' at the end of options, this changes a bit the way '--' is handled as we can now have '--' both after the options and before the scripts.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Actually it looks like we should not use:

  Getopt::Long::Configure qw/ pass_through /;

as GetOptions() would not error out when an unknown option like --foo is used.

 t/perf/aggregate.perl | 62 ++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 36 deletions(-)

Show changes to t/perf/aggregate.perl +26 −36

diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index 48637ef64b..bc865160e7 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -4,6 +4,7 @@ use lib '../../perl/build/lib';
 use strict;
 use warnings;
 use JSON;
+use Getopt::Long;
 use Git;
 
 sub get_times {
@@ -36,46 +37,34 @@ sub format_times {
 	return $out;
 }
 
+sub usage {
+	print <<EOT;
+./aggregate.perl [options] [--] [<dir_or_rev>...] [--] [<test_script>...] >
+
+  Options:
+    --codespeed          * Format output for Codespeed
+    --reponame    <str>  * Send given reponame to codespeed
+    --sort-by     <str>  * Sort output (only "regression" criteria is supported)
+    --subsection  <str>  * Use results from given subsection
+
+EOT
+	exit(1);
+}
+
 my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
     $codespeed, $sortby, $subsection, $reponame);
+
+Getopt::Long::Configure qw/ require_order /;
+
+my $rc = GetOptions("codespeed"     => \$codespeed,
+		    "reponame=s"    => \$reponame,
+		    "sort-by=s"     => \$sortby,
+		    "subsection=s"  => \$subsection);
+usage() unless $rc;
+
 while (scalar @ARGV) {
 	my $arg = $ARGV[0];
 	my $dir;
-	if ($arg eq "--codespeed") {
-		$codespeed = 1;
-		shift @ARGV;
-		next;
-	}
-	if ($arg =~ /--sort-by(?:=(.*))?/) {
-		shift @ARGV;
-		if (defined $1) {
-			$sortby = $1;
-		} else {
-			$sortby = shift @ARGV;
-			if (! defined $sortby) {
-				die "'--sort-by' requires an argument";
-			}
-		}
-		next;
-	}
-	if ($arg eq "--subsection") {
-		shift @ARGV;
-		$subsection = $ARGV[0];
-		shift @ARGV;
-		if (! $subsection) {
-			die "empty subsection";
-		}
-		next;
-	}
-	if ($arg eq "--reponame") {
-		shift @ARGV;
-		$reponame = $ARGV[0];
-		shift @ARGV;
-		if (! $reponame) {
-			die "empty reponame";
-		}
-		next;
-	}
 	last if -f $arg or $arg eq "--";
 	if (! -d $arg) {
 		my $rev = Git::command_oneline(qw(rev-parse --verify), $arg);
@@ -225,7 +214,8 @@ sub print_sorted_results {
 	my ($sortby) = @_;
 
 	if ($sortby ne "regression") {
-		die "only 'regression' is supported as '--sort-by' argument";
+		print "Only 'regression' is supported as '--sort-by' argument\n";
+		usage();
 	}
 
 	my @evolutions;

-- 
2.17.0.257.g28b659db43

v2perf/aggregate: use Getopt::Long for option parsing