{"thread":{"id":"52333","subject":"[PATCH] t/perf: don't depend on Git.pm","startedAt":"2019-11-25T16:47:22Z","lastAt":"2019-11-26T07:12:17Z","messageCount":2,"participants":["Jeff King","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"387010","messageId":"20191125164720.GA18080@sigill.intra.peff.net","threadId":"52333","inReplyTo":null,"subject":"[PATCH] t/perf: don't depend on Git.pm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-25T16:47:20Z","receivedAt":"2019-11-25T16:47:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The perf suite's aggregate.perl depends on Git.pm, which is a mild\nannoyance if you've built git with NO_PERL. It turns out that the only\nthing we use it for is a single call of the command_oneline() helper.\nWe can just replace this with backticks or similar.\n\nAnnoyingly, perl has no backtick equivalent that avoids a shell eval,\nwhich means our $arg would require quoting. This probably doesn't matter\nfor our purposes, but it's better to be safe and model good style. So\nwe'll just provide a short helper around open(), which takes its\narguments as a list.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/perf/aggregate.perl | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\nindex 66554d2161..a46ef67a2b 100755\n--- a/t/perf/aggregate.perl\n+++ b/t/perf/aggregate.perl\n@@ -4,7 +4,6 @@\n use strict;\n use warnings;\n use Getopt::Long;\n-use Git;\n use Cwd qw(realpath);\n \n sub get_times {\n@@ -85,6 +84,11 @@ sub format_size {\n \treturn $out;\n }\n \n+sub sane_backticks {\n+\topen(my $fh, '-|', @_);\n+\treturn <$fh>;\n+}\n+\n my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,\n     $codespeed, $sortby, $subsection, $reponame);\n \n@@ -102,7 +106,8 @@ sub format_size {\n \tmy $prefix = '';\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+\t\tmy $rev = sane_backticks(qw(git rev-parse --verify), $arg);\n+\t\tchomp $rev;\n \t\t$dir = \"build/\".$rev;\n \t} elsif ($arg eq '.') {\n \t\t$dir = '.';\n-- \n2.24.0.716.g722aff65ed\n"},{"id":"387076","messageId":"20191126071108.GA75059@syl.local","threadId":"52333","inReplyTo":"20191125164720.GA18080@sigill.intra.peff.net","subject":"Re: [PATCH] t/perf: don't depend on Git.pm","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-11-26T07:11:08Z","receivedAt":"2019-11-26T07:12:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Peff,\n\nOn Mon, Nov 25, 2019 at 11:47:20AM -0500, Jeff King wrote:\n> The perf suite's aggregate.perl depends on Git.pm, which is a mild\n> annoyance if you've built git with NO_PERL. It turns out that the only\n> thing we use it for is a single call of the command_oneline() helper.\n> We can just replace this with backticks or similar.\n\nHah, I think I walked right over this issue without even knowing that it\nexisted.\n\nThe backstory is that I was running the 't/perf' suite to test a merge\nfrom upstream into GitHub's fork, which we build with 'NO_PERL'. This\n*should* have failed, except for the fact that I had a leftover 'Git.pm'\nlaying around, which made everything go smoothly, even though I only got\nlucky.\n\nSo, I think that this is a strict improvement for that case of \"I want\nto run t/perf\" with \"I built with 'NO_PERL' for xyz reason\". Makes sense\nto me (and thanks for addressing a problem that I didn't even know I had\n;-).)\n\n> Annoyingly, perl has no backtick equivalent that avoids a shell eval,\n> which means our $arg would require quoting. This probably doesn't matter\n> for our purposes, but it's better to be safe and model good style. So\n> we'll just provide a short helper around open(), which takes its\n> arguments as a list.\n\nThat seems reasonable, but I'm deferring entirely to your Perl\nexpertise here, since I'm not myself familiar.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/perf/aggregate.perl | 9 +++++++--\n>  1 file changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\n> index 66554d2161..a46ef67a2b 100755\n> --- a/t/perf/aggregate.perl\n> +++ b/t/perf/aggregate.perl\n> @@ -4,7 +4,6 @@\n>  use strict;\n>  use warnings;\n>  use Getopt::Long;\n> -use Git;\n>  use Cwd qw(realpath);\n>\n>  sub get_times {\n> @@ -85,6 +84,11 @@ sub format_size {\n>  \treturn $out;\n>  }\n>\n> +sub sane_backticks {\n> +\topen(my $fh, '-|', @_);\n> +\treturn <$fh>;\n> +}\n> +\n>  my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,\n>      $codespeed, $sortby, $subsection, $reponame);\n>\n> @@ -102,7 +106,8 @@ sub format_size {\n>  \tmy $prefix = '';\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> +\t\tmy $rev = sane_backticks(qw(git rev-parse --verify), $arg);\n> +\t\tchomp $rev;\n>  \t\t$dir = \"build/\".$rev;\n>  \t} elsif ($arg eq '.') {\n>  \t\t$dir = '.';\n> --\n> 2.24.0.716.g722aff65ed\n\nThe rationale makes sense to me, so please have my:\n\n  Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nbut do take it with a grain of salt, since your Perl skills are\ncertainly superior to mine.\n\n\nThanks,\nTaylor\n"}]}