Re: [PATCH 4/5] test: improve rebase -q test

[Felipe Contreras <felipe.contreras@gmail.com>]

On Wed, May 29, 2013 at 11:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>> > Let's show the output so it's clear why it failed.
>>> >
>>> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>> > ---
>>> >  t/t3400-rebase.sh | 1 +
>>> >  1 file changed, 1 insertion(+)
>>> >
>>> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>>> > index b58fa1a..fb39531 100755
>>> > --- a/t/t3400-rebase.sh
>>> > +++ b/t/t3400-rebase.sh
>>> > @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream arg is missing' '
>>> >  test_expect_success 'rebase -q is quiet' '
>>> >    git checkout -b quiet topic &&
>>> >    git rebase -q master >output.out 2>&1 &&
>>> > +  cat output.out &&
>>> >    test ! -s output.out
>>> >  '
>>>
>>> It is one thing to avoid squelching output that naturally comes out
>>> of command being tested unnecessarily, so that "./txxxx-*.sh -v"
>>> output can be used for debugging.  I however am not sure if adding
>>> "cat" to random places like this is a productive direction for us to
>>> go in.
>>>
>>> A more preferrable alternative may be adding something like this to
>>> test-lib.sh and call it from here and elsewhere (there are about 50
>>> places that do "test ! -s <filename>"), perhaps?
>>>
>>>         test_must_be_an_empty_file () {
>>>                 if test -s "$1"
>>>                 then
>>>                         cat "$1"
>>>                         false
>>>                 fi
>>>         }
>>
>> Perhaps, but I'm not interested. I'm tired of obvious fixes getting rejected
>> for hypothetical "ideal" situations that we'll never reach.
>
> That's too bad.  Addition of "cat" where there does not need one is
> clearly not an obvious fix anyway.

If you are an actual real user of this code; a developer that is
running the test; and the test finally achieves it's designed goal of
detecting a failure, you would be left scratching your head wondering
what's the problem if running './test -v' doesn't show anything, even
after you have added debugging code to narrow down the issue.

I had to add that cat line not once, but more than two times in
different lines of development.

So yeah, a cat is needed, and the fact you don't see that amazes me,
specially after you have reprimanded me for using 'grep -q' instead of
'grep' for this very reason.

-- 
Felipe Contreras