[PATCH] t/perf: don't depend on Git.pm

[PATCH] t/perf: don't depend on Git.pm

[Jeff King]

The perf suite's aggregate.perl depends on Git.pm, which is a mild
annoyance if you've built git with NO_PERL. It turns out that the only
thing we use it for is a single call of the command_oneline() helper.
We can just replace this with backticks or similar.

Annoyingly, perl has no backtick equivalent that avoids a shell eval,
which means our $arg would require quoting. This probably doesn't matter
for our purposes, but it's better to be safe and model good style. So
we'll just provide a short helper around open(), which takes its
arguments as a list.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/perf/aggregate.perl | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index 66554d2161..a46ef67a2b 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -4,7 +4,6 @@
 use strict;
 use warnings;
 use Getopt::Long;
-use Git;
 use Cwd qw(realpath);
 
 sub get_times {
@@ -85,6 +84,11 @@ sub format_size {
 	return $out;
 }
 
+sub sane_backticks {
+	open(my $fh, '-|', @_);
+	return <$fh>;
+}
+
 my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
     $codespeed, $sortby, $subsection, $reponame);
 
@@ -102,7 +106,8 @@ sub format_size {
 	my $prefix = '';
 	last if -f $arg or $arg eq "--";
 	if (! -d $arg) {
-		my $rev = Git::command_oneline(qw(rev-parse --verify), $arg);
+		my $rev = sane_backticks(qw(git rev-parse --verify), $arg);
+		chomp $rev;
 		$dir = "build/".$rev;
 	} elsif ($arg eq '.') {
 		$dir = '.';
-- 
2.24.0.716.g722aff65ed

Re: [PATCH] t/perf: don't depend on Git.pm

[Taylor Blau]

Hi Peff,

On Mon, Nov 25, 2019 at 11:47:20AM -0500, Jeff King wrote:
> The perf suite's aggregate.perl depends on Git.pm, which is a mild
> annoyance if you've built git with NO_PERL. It turns out that the only
> thing we use it for is a single call of the command_oneline() helper.
> We can just replace this with backticks or similar.

Hah, I think I walked right over this issue without even knowing that it
existed.

The backstory is that I was running the 't/perf' suite to test a merge
from upstream into GitHub's fork, which we build with 'NO_PERL'. This
*should* have failed, except for the fact that I had a leftover 'Git.pm'
laying around, which made everything go smoothly, even though I only got
lucky.

So, I think that this is a strict improvement for that case of "I want
to run t/perf" with "I built with 'NO_PERL' for xyz reason". Makes sense
to me (and thanks for addressing a problem that I didn't even know I had
;-).)

> Annoyingly, perl has no backtick equivalent that avoids a shell eval,
> which means our $arg would require quoting. This probably doesn't matter
> for our purposes, but it's better to be safe and model good style. So
> we'll just provide a short helper around open(), which takes its
> arguments as a list.

That seems reasonable, but I'm deferring entirely to your Perl
expertise here, since I'm not myself familiar.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/perf/aggregate.perl | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
> index 66554d2161..a46ef67a2b 100755
> --- a/t/perf/aggregate.perl
> +++ b/t/perf/aggregate.perl
> @@ -4,7 +4,6 @@
>  use strict;
>  use warnings;
>  use Getopt::Long;
> -use Git;
>  use Cwd qw(realpath);
>
>  sub get_times {
> @@ -85,6 +84,11 @@ sub format_size {
>  	return $out;
>  }
>
> +sub sane_backticks {
> +	open(my $fh, '-|', @_);
> +	return <$fh>;
> +}
> +
>  my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
>      $codespeed, $sortby, $subsection, $reponame);
>
> @@ -102,7 +106,8 @@ sub format_size {
>  	my $prefix = '';
>  	last if -f $arg or $arg eq "--";
>  	if (! -d $arg) {
> -		my $rev = Git::command_oneline(qw(rev-parse --verify), $arg);
> +		my $rev = sane_backticks(qw(git rev-parse --verify), $arg);
> +		chomp $rev;
>  		$dir = "build/".$rev;
>  	} elsif ($arg eq '.') {
>  		$dir = '.';
> --
> 2.24.0.716.g722aff65ed

The rationale makes sense to me, so please have my:

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

but do take it with a grain of salt, since your Perl skills are
certainly superior to mine.


Thanks,
Taylor