From: Stefan Beller <sbeller@google.com>
Date: Tue, 16 Jan 2018 22:20:53 GMT
Subject: Re: [PATCH v4 6/7] trace.c: print env vars in trace_run_command()
Message-ID: <CAGZ79kbWvxoBtcU9iKPHSy_Agz-ko_KSE_M9budrtCOq7-Swdw@mail.gmail.com>
In-Reply-To: <xmqqmv1dtptj.fsf@gitster.mtv.corp.google.com>

On Tue, Jan 16, 2018 at 2:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> +test_trace() {
>> +     local expected="$1"
>
> Style: "test_trace () {" is how we start a shell function.
> Portability: we do not use "local".
>
>> +     shift
>> +     GIT_TRACE=1 test-run-command "$@" run-command true 2>&1 >/dev/null | \
>> +             sed 's/.* run_command: //' >actual &&
>> +     echo "$expected true" >expected &&
>> +     test_cmp expected actual
>
> Consistency: everybody else in the test script contrasts "actual" vs
> "expect" (they happen to be of the same length ;-); don't say
> expectED just to be different.

3491 entries in the t/ directory disagree with your imagination of
consistency. ;)
But I agree that we want to go for consistency, and most likely it's
best to go for 'expect'.