On Wed, Dec 13, 2017 at 9:33 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Christian Couder <christian.couder@xxxxxxxxx> writes:
>
>> 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};
>> }
>
> ...
>
>> + 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?
Yeah, I agree that it would be useful. Unfortunately it looks like the
number of fields in Codespeed is limited and I am not sure what will
be more important for people in general. Another option would be to
have "MacOS" in the "environment" field. Or maybe there is a need for
a generic way to customize this. For now I just tried to come up with
something sensible for what AEvar and me want to do.
> 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?
It's a silly mistake I made when I reorganized the patches just before
sending them. The chomp should be after "$executable = `uname -o -p`;"
(so in the other branch of the "if ... else"). I will fix this in the
next version.
