Re: [PATCH] t/perf: don't depend on Git.pm
From: Taylor Blau <hidden>
Date: 2019-11-26 07:12:17
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.
quoted hunk ↗ jump to hunk
Signed-off-by: Jeff King <redacted> --- 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 [off-list ref] but do take it with a grain of salt, since your Perl skills are certainly superior to mine. Thanks, Taylor