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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, May 07 2019, Jeff King wrote:

> 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...).

>> 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".

>> @@ -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...