From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 17 Jan 2018 01:32:53 GMT
Subject: Re: [PATCH v4 6/7] trace.c: print env vars in trace_run_command()
Message-ID: <xmqqwp0hs20q.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kbWvxoBtcU9iKPHSy_Agz-ko_KSE_M9budrtCOq7-Swdw@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

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

Heh.  I thought it was clear enough from the context that I was
talking about matching the remainder of the same test script; I do
not have time time to count 'test_cmp expected' in all others in t/
;-)

> But I agree that we want to go for consistency, and most likely it's
> best to go for 'expect'.

It is unclear if you are volunteering, but do not change things for
the sake of making them consistent.  Not adding new ones and fixing
existing ones when they need to change anyway for some other reason
is the way to do this without unnecessary code churn.