Re: [PATCH 1/2] Update shell tests to use semantic functions
From: Patrick Steinhardt <hidden>
Date: 2026-09-07 12:10:18
On Fri, Sep 04, 2026 at 04:35:52PM -0400, Mark C. Chu-Carroll via B4 Relay wrote:
From: "Mark C. Chu-Carroll" <redacted> 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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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