On Fri, 30 Apr 2021 03:55:04 -0600,
Ævar Arnfjörð Bjarmason wrote:

On Tue, Apr 27 2021, Luke Shumaker wrote:

quoted

From: Luke Shumaker <redacted>

t7900-subtree.sh defines its own `check_equal A B` function, instead of
just using `test A = B` like all of the other tests.  Don't be special,
get rid of `check_equal` in favor of `test`.

Signed-off-by: Luke Shumaker <redacted>
---
 contrib/subtree/t/t7900-subtree.sh | 60 ++++++++++++------------------
 1 file changed, 24 insertions(+), 36 deletions(-)

diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 12b8cb03c7..76183153c9 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh

@@ -28,18 +28,6 @@ create () {
 	git add "$1"
 }
 
-check_equal () {
-	test_debug 'echo'
-	test_debug "echo \"check a:\" \"{$1}\""
-	test_debug "echo \"      b:\" \"{$2}\""
-	if test "$1" = "$2"
-	then
-		return 0
-	else
-		return 1
-	fi
-}

It looks like the reason this was used because when this fails just
having the "test" makes for bad debugging. I.e. if the values don't
match the $1 and $2 are not aligned, so it's hard to eyeball what went
wrong.

It's easy to make that assumption, but looking at the history it seems
the "actual" reason it exists is that it's vestigial from before the
subtree tests used test-lib.sh, and echoing it like that was the only
way you'd get feedback.

These days this is more idiomatic:

    echo "Add [...]" >expected
    last_commit_message >actual &&
    test_cmp expected actual

So I think in this case a better narrower improvement would be to keep
the check_equal function. I wonder if we shouldn't just in general in
t/test-lib.sh have a test_cmp_str for this use-case. I.e. a trivial
wrapper that echos the two strings to a file for you, before running
diff(1).

But it's been my experience that the tests are already impossible to
debug without passing `-x`, so everything from `check_equal` ends up
being just noise.

And also I figured if `test` is good enough for t9350-fast-export.sh,
then it's good enough for t7900-subtree.sh... after working with
subtree, I'd accidentally written a couple of the checks in one of my
fast-export patchsets using `check_equal`.  Being special makes things
harder to hack on.

-- 
Happy hacking,
~ Luke Shumaker