From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Fri, 30 Apr 2021 09:55:04 GMT
Subject: Re: [PATCH v3 06/30] subtree: t7900: use 'test' for string equality
Message-ID: <87r1isxffw.fsf@evledraar.gmail.com>
In-Reply-To: <20210427211748.2607474-7-lukeshu@lukeshu.com>


On Tue, Apr 27 2021, Luke Shumaker wrote:

> From: Luke Shumaker <lukeshu@datawire.io>
>
> 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 <lukeshu@datawire.io>
> ---
>  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.

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