Re: [PATCH 1/2] Update shell tests to use semantic functions

[Patrick Steinhardt <ps@pks.im>]

On Fri, Sep 04, 2026 at 04:35:52PM -0400, Mark C. Chu-Carroll via B4 Relay wrote:
> From: "Mark C. Chu-Carroll" <markchucarroll@fastmail.com>
> 
> This updates an initial bash of shell tests to replace uses
> of "test -f" and "! test -f" with "test_path_is_file" and
> "test_path_is_missing".

The commit message doesn't quite match our conventions:

  - We typically write the messages in imperative style, as if telling
    the code to change.

  - The subject should typically start with the subsystem that you're
    changing, followed by a lower-case letter. So in your case, "t:"
    would be a good prefix.

  - The message should also briefly explain what the benefit of this
    conversion is.

  - You're missing the Signed-off-by line.

>  t/t0031-lockfile-pid.sh            |  2 +-
>  t/t0200-gettext-basic.sh           |  2 +-
>  t/t1007-hash-object.sh             |  6 +++---
>  t/t2030-unresolve-info.sh          |  8 ++++----
>  t/t2201-add-update-typechange.sh   |  2 +-
>  t/t3300-funny-names.sh             |  2 +-
>  t/t3306-notes-prune.sh             |  2 +-
>  t/t3311-notes-merge-fanout.sh      |  2 +-
>  t/t4014-format-patch.sh            |  6 +++---
>  t/t4032-diff-inter-hunk-context.sh |  4 ++--
>  t/t4102-apply-rename.sh            |  2 +-
>  t/t4131-apply-fake-ancestor.sh     |  2 +-
>  t/t4132-apply-removal.sh           |  4 ++--
>  t/t5300-pack-object.sh             | 10 +++++-----
>  t/t5301-sliding-window.sh          |  4 ++--
>  t/t5302-pack-index.sh              |  8 ++++----
>  t/t5502-quickfetch.sh              |  2 +-
>  t/t5510-fetch.sh                   |  8 ++++----
>  t/t5516-fetch-push.sh              |  2 +-
>  t/t5534-push-signed.sh             |  6 +++---
>  t/t5550-http-fetch-dumb.sh         |  2 +-
>  t/t5604-clone-reference.sh         |  2 +-
>  t/t6500-gc.sh                      |  2 +-
>  t/t7012-skip-worktree-writing.sh   |  2 +-
>  t/t7102-reset.sh                   |  2 +-
>  t/t7104-reset-hard.sh              |  2 +-
>  t/t7113-post-index-change-hook.sh  | 12 ++++++------
>  t/t7201-co.sh                      |  6 +++---
>  t/t7400-submodule-basic.sh         | 10 +++++-----
>  t/t7407-submodule-foreach.sh       |  6 +++---
>  t/t7412-submodule-absorbgitdirs.sh |  8 ++++----
>  t/t7602-merge-octopus-many.sh      |  2 +-
>  t/t9001-send-email.sh              |  6 +++---
>  t/t9400-git-cvsserver-server.sh    |  6 +++---
>  34 files changed, 76 insertions(+), 76 deletions(-)

I'd recommend significantly shrinking the number of files you convert to
at most a handful in this series. The conversion to use the
`test_path_*()` helpers is something that we mostly hand out to
newcomers as the usefulness of it is really rather in the educational
part rather than it bringing a lot of value to the Git project.

> diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
> index 463b38f990..e6d1fe3e13 100755
> --- a/t/t1007-hash-object.sh
> +++ b/t/t1007-hash-object.sh
> @@ -271,7 +271,7 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT \
>  
>  test_expect_success EXPENSIVE,SIZE_T_IS_64BIT \
>  		'files over 4GB hash correctly via --stdin' '
> -	{ test -f big || test-tool genzeros $((5*1024*1024*1024)) >big; } &&
> +	{ test_path_is_file big || test-tool genzeros $((5*1024*1024*1024)) >big; } &&
>  	test_oid large5GB >expect &&
>  	git hash-object --stdin <big >actual &&
>  	test_cmp expect actual

This is wrong. The intent is that we only generate the file if we didn't
already do it beforehand, so it's a form of lazy creation. So it is
expected that the file may not exist, but with `test_path_is_file` we'd
now generate an error message if so.

Likewise for the subsequent changes in this fiel.

> diff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh
> index 7d443968e3..cc213c04a1 100755
> --- a/t/t4032-diff-inter-hunk-context.sh
> +++ b/t/t4032-diff-inter-hunk-context.sh
> @@ -28,7 +28,7 @@ t() {
>  	file=f$1
>  	expected=expected.$file.$3.$hunks
>  
> -	if ! test -f $file
> +	if test_path_is_missing $file
>  	then
>  		f A $1 B >$file
>  		git add $file
> @@ -40,7 +40,7 @@ t() {
>  		test $(git $cmd $file | grep '^@@ ' | wc -l) = $hunks
>  	"
>  
> -	if test -f $expected
> +	if test_path_is_file $expected
>  	then
>  		test_expect_success "$label: check output" "
>  			git $cmd $file | grep -v '^index ' >actual &&

Likewise, these here are expected cases where the file may be missing.
We shouldn't print an error message in such cases. There's also a couple
more such cases.

Thanks!

Patrick