| Message ID | 20191125164720.GA18080@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t/perf: don't depend on Git.pm | expand |
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
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 = '.';
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(-)