| Message ID | 20250318131033.48691-1-contact@aynp.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [GSoC,v2] Use `test_path_*` helper functions instead of `test -[efd]`. | expand |
Thanks for your GSoC microproject submission. See comments below... On Tue, Mar 18, 2025 at 9:11 AM Aryan Pathania <contact@aynp.dev> wrote: > Use `test_path_*` helper functions instead of `test -[efd]` According to Documentation/SubmittingPatches, you'd probably instead want to compose the commit message summary line something like this: t9400: use test_path_*` functions to improve diagnostic output > Change testcase `gitcvs.enabled = false` to check for missing path > instead of a missing file. The change is justified as new assertion is > stronger. The description seems somehow outdated since this particular change is being made to more than just that one test, isn't it? Rather than describing the new assertion as "stronger", it might make more sense to state that it is more semantically in line with what is actually being tested (i.e. that the directory/path should not exist when, as expected, the command fails). > All other testcases remain equivalent. Okay, but... > @@ -296,8 +296,8 @@ test_expect_success 'gitcvs.ext.dbname' ' > test_cmp cvswork cvswork2 && > - test -f "$SERVERDIR/gitcvs1.ext.main.sqlite" && > - test ! -f "$SERVERDIR/gitcvs2.ext.main.sqlite" && > + test_path_is_file "$SERVERDIR/gitcvs1.ext.main.sqlite" && > + test_path_is_missing "$SERVERDIR/gitcvs2.ext.main.sqlite" && ... although `test_path_is_file` is a faithful replacement for `test -f`, it could be argued that `test_path_exists` would be a semantically better choice, especially given the use of `test_path_is_missing` for the sibling case. The same comment applies to one or two other changes in this patch.
diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh index e499c7f955..4ddde382e8 100755 --- a/t/t9400-git-cvsserver-server.sh +++ b/t/t9400-git-cvsserver-server.sh @@ -254,7 +254,7 @@ test_expect_success 'gitcvs.enabled = false' \ true fi && grep "GITCVS emulation disabled" cvs.log && - test ! -d cvswork2' + test_path_is_missing cvswork2' rm -fr cvswork2 test_expect_success 'gitcvs.ext.enabled = true' ' @@ -276,7 +276,7 @@ test_expect_success 'gitcvs.ext.enabled = false' ' true fi && grep "GITCVS emulation disabled" cvs.log && - test ! -d cvswork2 + test_path_is_missing cvswork2 ' rm -fr cvswork2 @@ -285,7 +285,7 @@ test_expect_success 'gitcvs.dbname' ' GIT_DIR="$SERVERDIR" git config gitcvs.dbname %Ggitcvs.%a.%m.sqlite && GIT_CONFIG="$git_config" cvs -Q co -d cvswork2 main >cvs.log 2>&1 && test_cmp cvswork cvswork2 && - test -f "$SERVERDIR/gitcvs.ext.main.sqlite" && + test_path_is_file "$SERVERDIR/gitcvs.ext.main.sqlite" && cmp "$SERVERDIR/gitcvs.main.sqlite" "$SERVERDIR/gitcvs.ext.main.sqlite" ' @@ -296,8 +296,8 @@ test_expect_success 'gitcvs.ext.dbname' ' GIT_DIR="$SERVERDIR" git config gitcvs.dbname %Ggitcvs2.%a.%m.sqlite && GIT_CONFIG="$git_config" cvs -Q co -d cvswork2 main >cvs.log 2>&1 && test_cmp cvswork cvswork2 && - test -f "$SERVERDIR/gitcvs1.ext.main.sqlite" && - test ! -f "$SERVERDIR/gitcvs2.ext.main.sqlite" && + test_path_is_file "$SERVERDIR/gitcvs1.ext.main.sqlite" && + test_path_is_missing "$SERVERDIR/gitcvs2.ext.main.sqlite" && cmp "$SERVERDIR/gitcvs.main.sqlite" "$SERVERDIR/gitcvs1.ext.main.sqlite" ' @@ -346,7 +346,7 @@ test_expect_failure "cvs update w/o -d doesn't create subdir (TODO)" ' git push gitcvs.git >/dev/null && cd cvswork && GIT_CONFIG="$git_config" cvs -Q update && - test ! -d test + test_path_is_missing test ' cd "$WORKDIR" @@ -379,7 +379,7 @@ test_expect_success 'cvs update (delete file)' ' cd cvswork && GIT_CONFIG="$git_config" cvs -Q update && test -z "$(grep testfile1 CVS/Entries)" && - test ! -f testfile1 + test_path_is_missing testfile1 ' cd "$WORKDIR"