Re: [PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[SZEDER Gábor]

On Fri, Mar 9, 2018 at 12:44 AM, Junio C Hamano  wrote:
> SZEDER Gábor  writes:

>>> This makes "rm -f failures &&" unnecessary, no?
>>
>> Indeed, it does.
>
> OK, no need to resend, as I'll amend it locally before queuing
> (unless there is something else that needs updating, that is).

Thanks.

That 'rm' was unnecessary to begin with, because none of the previous
tests wrote and left behind a file 'failures', ever.  Though one could
argue that the original author was careful and added that 'rm failures'
as a protection against future changes to previous tests...  Dunno, we
are talking about a test that checked the stderr of 'test_cmp', so
anything is possible :)

Re: [PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[Junio C Hamano]

SZEDER Gábor  writes:

> On Fri, Mar 9, 2018 at 12:33 AM, Junio C Hamano  wrote:
>> SZEDER Gábor  writes:
>>
>>> diff --git a/t/t9400-git-cvsserver-server.sh 
>>> b/t/t9400-git-cvsserver-server.sh
>>> index c30660d606..5ff3789199 100755
>>> --- a/t/t9400-git-cvsserver-server.sh
>>> +++ b/t/t9400-git-cvsserver-server.sh
>>> @@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '
>>>  GIT_CONFIG="$git_config" cvs update &&
>>>  rm -f failures &&
>>>  for i in merge no-lf empty really-empty; do
>>> -GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
>>> - test_cmp $i.out ../$i >>failures 2>&1
>>> -done &&
>>> -test -z "$(cat failures)"
>>> + GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out &&
>>> + test_cmp $i.out ../$i || return 1
>>> +done
>>>  '
>>
>> This makes "rm -f failures &&" unnecessary, no?
>
> Indeed, it does.

OK, no need to resend, as I'll amend it locally before queuing
(unless there is something else that needs updating, that is).

Thanks.

Re: [PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[SZEDER Gábor]

On Fri, Mar 9, 2018 at 12:33 AM, Junio C Hamano  wrote:
> SZEDER Gábor  writes:
>
>> diff --git a/t/t9400-git-cvsserver-server.sh 
>> b/t/t9400-git-cvsserver-server.sh
>> index c30660d606..5ff3789199 100755
>> --- a/t/t9400-git-cvsserver-server.sh
>> +++ b/t/t9400-git-cvsserver-server.sh
>> @@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '
>>  GIT_CONFIG="$git_config" cvs update &&
>>  rm -f failures &&
>>  for i in merge no-lf empty really-empty; do
>> -GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
>> - test_cmp $i.out ../$i >>failures 2>&1
>> -done &&
>> -test -z "$(cat failures)"
>> + GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out &&
>> + test_cmp $i.out ../$i || return 1
>> +done
>>  '
>
> This makes "rm -f failures &&" unnecessary, no?

Indeed, it does.

Re: [PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[Junio C Hamano]

SZEDER Gábor  writes:

> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
> index c30660d606..5ff3789199 100755
> --- a/t/t9400-git-cvsserver-server.sh
> +++ b/t/t9400-git-cvsserver-server.sh
> @@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '
>  GIT_CONFIG="$git_config" cvs update &&
>  rm -f failures &&
>  for i in merge no-lf empty really-empty; do
> -GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
> - test_cmp $i.out ../$i >>failures 2>&1
> -done &&
> -test -z "$(cat failures)"
> + GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out &&
> + test_cmp $i.out ../$i || return 1
> +done
>  '

This makes "rm -f failures &&" unnecessary, no?

[PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[SZEDER Gábor]

The test 'cvs update (-p)' redirects and checks 'test_cmp's stdout and
even its stderr.  The commit introducing this test in 6e8937a084
(cvsserver: Add test for update -p, 2008-03-27) doesn't discuss why,
in fact its log message only consists of that subject line.  Anyway,
weird as it is, it kind of made sense due to the way that test was
structured:

After a bit of preparation, this test updates four files via CVS and
checks their contents using 'test_cmp', but it does so in a for loop
iterating over the names of those four files.  Now, the exit status of
a for loop is the exit status of the last command executed in the
loop, meaning that the test can't simply rely on the exit code of
'test_cmp' in the loop's body.  Instead, the test works it around by
relying on the stdout of 'test_cmp' being silent on success and
showing the diff on failure, as it appends the stdout of all four
'test_cmp' invocations to a single file and checks that file's
emptiness after the loop (with 'test -z "$(cat ...)"', no less; there
was no 'test_must_be_empty' back then).  Furthermore, the test
redirects the stderr of those 'test_cmp' invocations to this file,
too: while 'test_cmp' itself doesn't output anything to stderr, the
invoked 'diff' or 'cmp' commands do send their error messages there,
e.g. if they can't open a file because its name was misspelled.

This also makes this test fail when the test script is run with '-x'
tracing (and using a shell other than a Bash version supporting
BASH_XTRACEFD), because 'test_cmp's stderr contains the trace of the
'diff' command executed inside the helper function, throwing off the
subsequent emptiness check.

Stop relying on 'test_cmp's output and instead run 'test_cmp a b ||
return 1' in the for loop in order to make 'test_cmp's error code fail
the test.  Furthermore, add the missing && after the cvs command to
create a && chain in the loop's body.

After this change t9400 passes with '-x', even when running with
/bin/sh.

Signed-off-by: SZEDER Gábor 
---

Changes:
Use Eric's suggestion and run 'test_cmp a b || return 1' in the for
loop to fail the test.

 t/t9400-git-cvsserver-server.sh | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index c30660d606..5ff3789199 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '
 GIT_CONFIG="$git_config" cvs update &&
 rm -f failures &&
 for i in merge no-lf empty really-empty; do
-GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
-   test_cmp $i.out ../$i >>failures 2>&1
-done &&
-test -z "$(cat failures)"
+   GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out &&
+   test_cmp $i.out ../$i || return 1
+done
 '
 
 cd "$WORKDIR"
-- 
2.16.2.603.g180c1428f0