Re: [PATCH 3/6] t1006 (cat-file): use test_cmp

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 8, 2011, 16:55 UTC
Message-ID: <20111208165512.GC2394@elie.hsd1.il.comcast.net>
In-Reply-To: <1323349817-15737-6-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

> Use test_cmp in preference to repeatedly comparing command outputs by
> hand.

That could mean one of several things.

It could mean:

	1. Use test_cmp instead of open-coding it.
	2. Use test_cmp instead of using our knowledge of the underlying
	   filesystem to retrieve the files from the block device, instead
	   of relying on the perfectly good operating system facilities
	   that could take care of it for us
	3. Use test_cmp instead of calling a human over to compare command
	   outputs by eye, which idiomatically might be described as "by
	   hand".

What I mean is, I actually don't have much of a clue what you mean by "by hand". Usually it means "not automated sufficiently", but I think that is not the entire problem here (since

	test "$expect" = "$actual"

looks no less automatic than

	printf '%s\n' "$expect" >expect &&
	printf '%s\n' "$actual" >actual &&
	test_cmp expect actual

to me).

> 
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

[...]

> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -36,66 +36,41 @@ $content"

[...]

Show 14 quoted lines

> -	expect="$(maybe_remove_timestamp "$content" $no_ts)"
> -	actual="$(maybe_remove_timestamp "$(git cat-file $type $sha1)" $no_ts)"
> -
> -        if test "z$expect" = "z$actual"
> -	then
> -		: happy
> -	else
> -		echo "Oops: expected $expect"
> -		echo "but got $actual"
> -		false
> -        fi
> +	maybe_remove_timestamp "$content" $no_ts >expect &&
> +	maybe_remove_timestamp "$(git cat-file $type $sha1)" $no_ts >actual &&
> +	test_cmp expect actual

Most of the early part of patch proper looks sane from a quick glance. Wow, the whitespace is a little strange in the original.

[...]

Show 14 quoted lines

> @@ -175,30 +153,41 @@ do
>  done
>  
>  test_expect_success "--batch-check for a non-existent named object" '
> -    test "foobar42 missing
> -foobar84 missing" = \
> -    "$( ( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)"
> +    cat >expect <<\-EOF &&
> +foobar42 missing
> +foobar84 missing
> +EOF
> +    $(echo foobar42; echo_without_newline foobar84) \
> +    | git cat-file --batch-check >actual &&
> +    test_cmp expect actual

Style: the | character goes at the end of the first line (think of it
as a way to save backslashes until they're needed).

How could this $(...) command substitution possibly work?

Later tests have the same problem, so I'm stopping here.

Ciao, Jonathan