Re: [PATCH] tests: make 'test_oid' print trailing newline

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sun, Dec 18 2022, SZEDER Gábor wrote:

> Unlike other test helper functions, 'test_oid' doesn't terminate its
> output with a LF, but, alas, the reason for this, if any, is not
> mentioned in 2c02b110da (t: add test functions to translate
> hash-related values, 2018-09-13)).
>
> Now, in the vast majority of cases 'test_oid' is invoked in a command
> substitution that is part of a heredoc or supplies an argument to a
> command or the value to a variable, and the command substitution would
> chop off any trailing LFs, so in these cases the lack or presence of a
> trailing LF in its output doesn't matter.  However:
>
>   - There appear to be only three cases where 'test_oid' is not
>     invoked in a command substitution:
>
>       $ git grep '\stest_oid ' -- ':/t/*.sh'
>       t0000-basic.sh:  test_oid zero >actual &&
>       t0000-basic.sh:  test_oid zero >actual &&
>       t0000-basic.sh:  test_oid zero >actual &&
>
>     These are all in test cases checking that 'test_oid' actually
>     works, and that the size of its output matches the size of the
>     corresponding hash function with conditions like
>
>       test $(wc -c <actual) -eq 40
>
>     In these cases the lack of trailing LF does actually matter,
>     though they could be trivially updated to account for the presence
>     of a trailing LF.
>
>   - There are also a few cases where the lack of trailing LF in
>     'test_oid's output actually hurts, because tests need to compare
>     its output with LF terminated file contents, forcing developers to
>     invoke it as 'echo $(test_oid ...)' to append the missing LF:
>
>       $ git grep 'echo "\?$(test_oid ' -- ':/t/*.sh'
>       t1302-repo-version.sh:  echo $(test_oid version) >expect &&
>       t1500-rev-parse.sh:     echo "$(test_oid algo)" >expect &&
>       t4044-diff-index-unique-abbrev.sh:      echo "$(test_oid val1)" > foo &&
>       t4044-diff-index-unique-abbrev.sh:      echo "$(test_oid val2)" > foo &&
>       t5313-pack-bounds-checks.sh:    echo $(test_oid oidfff) >file &&
>
>     And there is yet another similar case in an in-flight topic at:
>
>       https://public-inbox.org/git/813e81a058227bd373cec802e443fcd677042fb4.1670862677.git.gitgitgadget@gmail.com/
>
> Arguably we would be better off if 'test_oid' terminated its output
> with a LF.  So let's update 'test_oid' accordingly, update its tests
> in t0000 to account for the extra character in those size tests, and
> remove the now unnecessary 'echo $(...)' command substitutions around
> 'test_oid' invocations as well.

I'm inclined to like this, as it certainly makes some examples better,
but e.g. here:

> @@ -826,7 +827,7 @@ test_expect_success 'test_oid can look up data for SHA-1' '
>  	grep "^00*\$" actual &&
>  	rawsz="$(test_oid rawsz)" &&
>  	hexsz="$(test_oid hexsz)" &&
> -	test $(wc -c <actual) -eq 40 &&
> +	test $(wc -c <actual) -eq 41 &&
>  	test "$rawsz" -eq 20 &&
>  	test "$hexsz" -eq 40
> [...]
>  '
> @@ -838,7 +839,7 @@ test_expect_success 'test_oid can look up data for SHA-256' '
>  	grep "^00*\$" actual &&
>  	rawsz="$(test_oid rawsz)" &&
>  	hexsz="$(test_oid hexsz)" &&
> -	test $(wc -c <actual) -eq 64 &&
> +	test $(wc -c <actual) -eq 65 &&
>  	test "$rawsz" -eq 32 &&
>  	test "$hexsz" -eq 64


If we have sibling tests we really should try to make these
consistent. These are still understandable, but it's rather annoying
that we aren't consistent here. I.e. we have 64 changed to 65, but not
32 to 33 etc.

I also vaguely recall (although probably nobody worries about such a
platform anymore) that POSIX utilities left themselves room to not work
on things that weren't \n-terminated.

>  test_expect_success 'gitdir selection on normal repos' '
> -	echo $(test_oid version) >expect &&
> +	test_oid version >expect &&
>  	git config core.repositoryformatversion >actual &&
>  	git -C test config core.repositoryformatversion >actual2 &&
>  	test_cmp expect actual &&
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 81de584ea2..37ee5091b5 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -195,7 +195,7 @@ test_expect_success 'rev-parse --is-shallow-repository in non-shallow repo' '
>  '
>  
>  test_expect_success 'rev-parse --show-object-format in repo' '
> -	echo "$(test_oid algo)" >expect &&
> +	test_oid algo >expect &&
>  	git rev-parse --show-object-format >actual &&
>  	test_cmp expect actual &&
>  	git rev-parse --show-object-format=storage >actual &&

This sort of thing is much nicer though, so maybe it's all worth it...

I wonder though if we shouldn't just have a test_cmp_oid, which would
abstract this away, and not care if it's \n-terminated or not...