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

From: Jeff King <peff@peff.net>
Date: May 7, 2019, 07:16 UTC
Message-ID: <20190507071630.GB28060@sigill.intra.peff.net>
In-Reply-To: <20190506232309.28538-3-avarab@gmail.com>

On Tue, May 07, 2019 at 01:23:09AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 18 quoted lines

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

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

Show 10 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?

-Peff