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

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 <redacted>

[...]

[...]

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

[...]

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