Re: [PATCH 3/8] perf/aggregate: implement codespeed JSON output

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 13, 2017, 20:33 UTC
Message-ID: <xmqqfu8ewepg.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171213151344.2138-4-chriscool@tuxfamily.org>

Christian Couder <christian.couder@gmail.com> writes:

Show 6 quoted lines

>  my $resultsdir = "test-results";
> +my $results_section = "";
>  if (exists $ENV{GIT_PERF_SUBSECTION} and $ENV{GIT_PERF_SUBSECTION} ne "") {
>  	$resultsdir .= "/" . $ENV{GIT_PERF_SUBSECTION};
> +	$results_section = $ENV{GIT_PERF_SUBSECTION};
>  }

...

Show 7 quoted lines

> +	my $executable;
> +	if ($results_section eq "") {
> +		$executable = `uname -o -p`;
> +	} else {
> +		$executable = $results_section;
> +		chomp $executable;
> +	}

Aside from portability of 'uname -o' Eric raised, I wonder if the platform information is still useful even when perf-subsection is specified. With the above code, we can identify that a single result is for (say) MacOS only when we are not limiting to a single subsection, but wouldn't it be equally a valid desire to be able to track performance figures for a single subsection over time and being able to say "On MacOS, subsection A's performance dropped between release X and X+1 quite a bit, but on Linux x86-64, there was no such change" or somesuch?

IOW, shouldn't the "executable" label always contain the platform information, plus an optional subsection info when (and only when) the result is limited to a subsection?

By the way, $results_section that is not an empty string at this point must have come from $ENV{GIT_PERF_SUBSECTION}, and from the way the environment variable is used in t/perf/run, e.g.

		(
			GIT_PERF_SUBSECTION="$subsec"
			export GIT_PERF_SUBSECTION
			echo "======== Run for subsection '$GIT_PERF_SUBSECTION' ========"
			run_subsection "$@"
		)

I do not think we expect it to have a trailing LF. What's that chomp doing there?