From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] t/perf: don't depend on Git.pm
Date: Mon, 25 Nov 2019 23:11:08 -0800 [thread overview]
Message-ID: <20191126071108.GA75059@syl.local> (raw)
In-Reply-To: <20191125164720.GA18080@sigill.intra.peff.net>
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
prev parent reply other threads:[~2019-11-26 7:12 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-25 16:47 [PATCH] t/perf: don't depend on Git.pm Jeff King
2019-11-26 7:11 ` Taylor Blau [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191126071108.GA75059@syl.local \
--to=me@ttaylorr.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).