Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

From: Junio C Hamano <gitster@pobox.com>
Date: May 16, 2015, 18:57 UTC
Message-ID: <xmqq617sfj05.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CACRoPnSP9xfyW47ZqU7QO5o4tyzROh4hGRPqG9g9OB5cquS+uw@mail.gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 10 quoted lines

> Hi Junio,
>
> On Sat, May 16, 2015 at 2:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Just to avoid misunderstanding, please do not remove 'verbose '
>> blindly without thinking while doing so, as you already did 1/3 of
>> the necessary job to make things better.
>
> Eh? I thought we established that using "verbose" does not provide
> anything more than what "set -x" already provides. So at the very
> least, its use should be removed completely.

I did not mean "do not remove and keep them". I meant "do not remove without thinking; instead, take mental notes on patterns these silent ones may have while removing them".

Show 18 quoted lines

>> You might have noticed, while adding them, there were something
>> common that we currently do with a bare 'test' only because we
>> haven't identified common needs.  As I already said,...
>> ...
>> That would be an improvement (and that is the remaining 2/3 ;-).
>
> Yeah, this kind of comparison with file contents is something that is
> done often in t5520, so I agree with adding it.
>
> However, what about these kind of tests:
>
>      test new = "$(git show HEAD:file2)"
>
> or these:
>
>      test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
>
> So, perhaps we could introduce a generic function like:

It all depends on how common they are.

> So the first example would be:
>
>     test_output "git show HEAD:file2" new

Simple things like that look fine, but when a variable is involved, use of eval combined with the fact that the test body is inside sq, makes the callers unnecessarily ugly.

	test_expect_success 'some title' '
		var=$(...) &&
		test_output "git show \$var:file2 | sed -e \"s/$old/$new/\"" new
	'

Which is the concern this shares with the other one I sent about counting the number of lines in the output from a command that made me hesitate to suggest it.

So I dunno.