Re: [PATCH v2 2/2] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 7, 2019, 08:31 UTC
Message-ID: <871s1ais2s.fsf@evledraar.gmail.com>
In-Reply-To: <20190507071630.GB28060@sigill.intra.peff.net>

On Tue, May 07 2019, Jeff King wrote:

Show 24 quoted lines

> On Tue, May 07, 2019 at 01:23:09AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> @@ -79,7 +95,16 @@ run_dirs_helper () {
>>  	if test $# -gt 0 -a "$1" = --; then
>>  		shift
>>  	fi
>> -	if [ ! -d "$mydir" ]; then
>> +
>> +	PERF_RESULTS_PREFIX=
>> +	if test "$mydir" = "."
>> +	then
>> +		unset GIT_TEST_INSTALLED
>> +	elif test -d "$mydir"
>> +	then
>> +		PERF_RESULTS_PREFIX=$(cd $mydir && printf "%s" "$(pwd)" | tr -c "[a-zA-Z0-9]" "_").
>> +		set_git_test_installed "$mydir"
>> +	else
>>  		rev=$(git rev-parse --verify "$mydir" 2>/dev/null) ||
>>  		die "'$mydir' is neither a directory nor a valid revision"
>>  		if [ ! -d build/$rev ]; then
>
> OK, so this is basically the same cleanup I came up with. The big
> difference is that you pushed the shared code into a function, rather
> than the funky double-conditional in the original. I'm OK with that.

Yup. It can be done either way, but this way allows for a many/many mapping where we set the GIT_TEST_INSTALLED & PERF_RESULTS_PREFIX depending on the different cases, as opposed to in perf-lib.sh where we'd need to set GIT_TEST_INSTALLED and then infer a PERF_RESULTS_PREFIX (or pass flags marking each case, which would amount to the same thing...).

Show 34 quoted lines

>> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
>> index 494907a892..c8f4a78903 100755
>> --- a/t/perf/aggregate.perl
>> +++ b/t/perf/aggregate.perl
>> @@ -6,6 +6,7 @@
>>  use JSON;
>>  use Getopt::Long;
>>  use Git;
>> +use Cwd qw(realpath);
>>
>>  sub get_times {
>>  	my $name = shift;
>> @@ -103,13 +104,14 @@ sub format_size {
>>  	if (! -d $arg) {
>>  		my $rev = Git::command_oneline(qw(rev-parse --verify), $arg);
>>  		$dir = "build/".$rev;
>> +	} elsif ($arg eq '.') {
>> +		$dir = '.';
>>  	} else {
>> -		$arg =~ s{/*$}{};
>> -		$dir = $arg;
>> -		$dirabbrevs{$dir} = $dir;
>> +		$dir = realpath($arg);
>> +		$dirnames{$dir} = $dir;
>>  	}
>>  	push @dirs, $dir;
>> -	$dirnames{$dir} = $arg;
>> +	$dirnames{$dir} ||= $arg;
>
> I'm not sure I get what's going on here. Why do we need the realpath in
> aggregate.perl? We'd want to generate the same filename that "run"
> decided to store things in, which we'd generate from the command-line
> arguments (either passed on to us by "run", or direct from the user if
> they're printing a previous run).

So this is part of the "has sucked since forever, future TODO" mentioned in 0/2.

I.e. if you pass "../.." as a path to "run" we'll try to discover a built/installed "git" in a "bindir" there, and then we need to do two things:

 1. Figure out a way to turn that into a filename sensible for the
    *.times files.
 2. Print some header showing that path in the aggregate output.

The "run" script will discover #1 for itself, that's what that "pwd && tr -c ..." command is doing, but then we just pass "../.." again to aggregate.perl and have it figure it out again on its own, so it needs to duplicate the logic.

Just having both discover the absolute path all the time for #1 made things a lot simpler, e.g. if you do ../.. on v2.21.0 you'll get things like:

    _____.p0000-perf-lib-sanity.1.times

And with $PWD/../../ you'd get:

    _home_avar_g_git_t_perf______.p0000-perf-lib-sanity.1.times

Now this is all pretty & consistent. Any path to a "git" will always be turned into the absolute path, e.g.:

    _home_avar_g_git.p0000-perf-lib-sanity.1.times

And instead of "git" or ".." being printed in the aggregate header we print the path, e.g. "/home/avar/g/git".

Show 13 quoted lines

>> @@ -312,9 +314,6 @@ sub print_codespeed_results {
>>  		$environment = $reponame;
>>  	} elsif (exists $ENV{GIT_PERF_REPO_NAME} and $ENV{GIT_PERF_REPO_NAME} ne "") {
>>  		$environment = $ENV{GIT_PERF_REPO_NAME};
>> -	} elsif (exists $ENV{GIT_TEST_INSTALLED} and $ENV{GIT_TEST_INSTALLED} ne "") {
>> -		$environment = $ENV{GIT_TEST_INSTALLED};
>> -		$environment =~ s|/bin-wrappers$||;
>>  	} else {
>>  		$environment = `uname -r`;
>>  		chomp $environment;
>
> Is this codespeed thing a totally separate bug? Should it go into its
> own patch?

I could split it up, but figured any change like this would have had to deal with and refactor with any existing uses of GIT_TEST_INSTALLED, that there was a bug in some of that code and it could be removed was just luck...