{"thread":{"id":"60900","subject":"[PATCH] t9146: replace test -d/-f with appropriate test_path_is_* function","startedAt":"2024-02-11T14:53:20Z","lastAt":"2024-02-14T17:50:51Z","messageCount":5,"participants":["Chandra Pratap via GitGitGadget","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"488392","messageId":"pull.1661.git.1707663197543.gitgitgadget@gmail.com","threadId":"60900","inReplyTo":null,"subject":"[PATCH] t9146: replace test -d/-f with appropriate test_path_is_* function","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-11T14:53:17Z","receivedAt":"2024-02-11T14:53:20Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap <chandrapratap3519@gmail.com>\n\nThe helper functions test_path_is_* provide better debugging\ninformation than test -d/-e/-f.\n\nReplace \"! test -d\" with \"test_path_is_missing\" at places where\nwe check for non-existent directories.\n\nReplace \"test -f\" with \"test_path_is_file\" and \"test -d\" with\n\"test_path_is_dir\" at places where we expect files or directories\nto exist.\n\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n    t9146: replace test -d/-f with appropriate test_path_is_* function\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1661%2FChand-ra%2Ftestfix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1661/Chand-ra/testfix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1661\n\n t/t9146-git-svn-empty-dirs.sh | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh\nindex 09606f1b3cf..532f5baa208 100755\n--- a/t/t9146-git-svn-empty-dirs.sh\n+++ b/t/t9146-git-svn-empty-dirs.sh\n@@ -20,7 +20,7 @@ test_expect_success 'empty directories exist' '\n \t\tcd cloned &&\n \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n \t\tdo\n-\t\t\tif ! test -d \"$i\"\n+\t\t\tif test_path_is_missing \"$i\"\n \t\t\tthen\n \t\t\t\techo >&2 \"$i does not exist\" &&\n \t\t\t\texit 1\n@@ -37,7 +37,7 @@ test_expect_success 'option automkdirs set to false' '\n \t\tgit svn fetch &&\n \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n \t\tdo\n-\t\t\tif test -d \"$i\"\n+\t\t\tif test_path_is_dir \"$i\"\n \t\t\tthen\n \t\t\t\techo >&2 \"$i exists\" &&\n \t\t\t\texit 1\n@@ -52,7 +52,7 @@ test_expect_success 'more emptiness' '\n \n test_expect_success 'git svn rebase creates empty directory' '\n \t( cd cloned && git svn rebase ) &&\n-\ttest -d cloned/\"! !\"\n+\ttest_path_is_dir cloned/\"! !\"\n '\n \n test_expect_success 'git svn mkdirs recreates empty directories' '\n@@ -62,7 +62,7 @@ test_expect_success 'git svn mkdirs recreates empty directories' '\n \t\tgit svn mkdirs &&\n \t\tfor i in a b c d d/e d/e/f \"weird file name\" \"! !\"\n \t\tdo\n-\t\t\tif ! test -d \"$i\"\n+\t\t\tif test_path_is_missing \"$i\"\n \t\t\tthen\n \t\t\t\techo >&2 \"$i does not exist\" &&\n \t\t\t\texit 1\n@@ -78,21 +78,21 @@ test_expect_success 'git svn mkdirs -r works' '\n \t\tgit svn mkdirs -r7 &&\n \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n \t\tdo\n-\t\t\tif ! test -d \"$i\"\n+\t\t\tif test_path_is_missing \"$i\"\n \t\t\tthen\n \t\t\t\techo >&2 \"$i does not exist\" &&\n \t\t\t\texit 1\n \t\t\tfi\n \t\tdone &&\n \n-\t\tif test -d \"! !\"\n+\t\tif test_path_is_dir \"! !\"\n \t\tthen\n \t\t\techo >&2 \"$i should not exist\" &&\n \t\t\texit 1\n \t\tfi &&\n \n \t\tgit svn mkdirs -r8 &&\n-\t\tif ! test -d \"! !\"\n+\t\tif test_path_is_missing \"! !\"\n \t\tthen\n \t\t\techo >&2 \"$i not exist\" &&\n \t\t\texit 1\n@@ -114,7 +114,7 @@ test_expect_success 'empty directories in trunk exist' '\n \t\tcd trunk &&\n \t\tfor i in a \"weird file name\"\n \t\tdo\n-\t\t\tif ! test -d \"$i\"\n+\t\t\tif test_path_is_missing \"$i\"\n \t\t\tthen\n \t\t\t\techo >&2 \"$i does not exist\" &&\n \t\t\t\texit 1\n@@ -138,16 +138,16 @@ test_expect_success 'git svn gc-ed files work' '\n \t\tcd removed &&\n \t\tgit svn gc &&\n \t\t: Compress::Zlib may not be available &&\n-\t\tif test -f \"$unhandled\".gz\n+\t\tif test_path_is_file \"$unhandled\".gz\n \t\tthen\n \t\t\tsvn_cmd mkdir -m gz \"$svnrepo\"/gz &&\n \t\t\tgit reset --hard $(git rev-list HEAD | tail -1) &&\n \t\t\tgit svn rebase &&\n-\t\t\ttest -f \"$unhandled\".gz &&\n-\t\t\ttest -f \"$unhandled\" &&\n+\t\t\ttest_path_is_file \"$unhandled\".gz &&\n+\t\t\ttest_path_is_file \"$unhandled\" &&\n \t\t\tfor i in a b c \"weird file name\" gz \"! !\"\n \t\t\tdo\n-\t\t\t\tif ! test -d \"$i\"\n+\t\t\t\tif test_path_is_missing \"$i\"\n \t\t\t\tthen\n \t\t\t\t\techo >&2 \"$i does not exist\" &&\n \t\t\t\t\texit 1\n\nbase-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821\n-- \ngitgitgadget\n"},{"id":"488405","messageId":"CAPig+cR2jS9fhM8dbWy8pOPcUryv78qYXaB+Lbxjb3kkkqBqSQ@mail.gmail.com","threadId":"60900","inReplyTo":"pull.1661.git.1707663197543.gitgitgadget@gmail.com","subject":"Re: [PATCH] t9146: replace test -d/-f with appropriate test_path_is_* function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-11T17:58:19Z","receivedAt":"2024-02-11T17:58:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 11, 2024 at 9:53 AM Chandra Pratap via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> The helper functions test_path_is_* provide better debugging\n> information than test -d/-e/-f.\n>\n> Replace \"! test -d\" with \"test_path_is_missing\" at places where\n> we check for non-existent directories.\n>\n> Replace \"test -f\" with \"test_path_is_file\" and \"test -d\" with\n> \"test_path_is_dir\" at places where we expect files or directories\n> to exist.\n\nThe aim of this patch makes sense, but the implementation needs some\nrefinement...\n\n> diff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh\n> @@ -20,7 +20,7 @@ test_expect_success 'empty directories exist' '\n>                 for i in a b c d d/e d/e/f \"weird file name\"\n>                 do\n> -                       if ! test -d \"$i\"\n> +                       if test_path_is_missing \"$i\"\n>                         then\n>                                 echo >&2 \"$i does not exist\" &&\n>                                 exit 1\n\nThe point of functions such as test_path_is_missing() is to _assert_\nthat some condition is true, thus allowing the test to succeed; if the\ncondition is not true, then the function prints an error message and\nthe test aborts and fails.\n\n    test_path_is_missing () {\n        if test -e \"$1\"\n        then\n            echo \"Path exists:\"\n            ls -ld \"$1\"\n            false\n        fi\n    }\n\nIt is meant to replace noisy code such as:\n\n    if ! test -f bloop\n    then\n        echo >&2 \"error message\" &&\n        exit 1\n    fi &&\n    other-code\n\nwith much simpler:\n\n    test_path_exists bloop &&\n    other-code\n\nSo, the changes made by this patch are incorrect in two ways...\n\nFirst, the patch retains code which prints an error message even\nthough this code becomes redundant since the test_path_foo() functions\nalready take care of printing the error message.\n\nSecond, and more problematic, the patch incorrectly inverts the sense\nof what is being tested. For instance, the title of this test is\n\"empty directories exist\", and the body of the test asserts that the\nempty directories _do_ exist, but the patch changes the condition to\nassert that the directories do _not_ exist, which is wrong.\n\nTaking these observations into account, this test should become:\n\n    test_expect_success 'empty directories exist' '\n        (\n            cd cloned &&\n            for i in a b c d d/e d/e/f \"weird file name\"\n            do\n                test_path_exists \"$i\" || exit 1\n            done\n        )\n    '\n\nMany of the other changes made by this patch suffer similar problems\n\n> @@ -138,16 +138,16 @@ test_expect_success 'git svn gc-ed files work' '\n>                 : Compress::Zlib may not be available &&\n> -               if test -f \"$unhandled\".gz\n> +               if test_path_is_file \"$unhandled\".gz\n>                 then\n>                         svn_cmd mkdir -m gz \"$svnrepo\"/gz &&\n>                         git reset --hard $(git rev-list HEAD | tail -1) &&\n\nThis change is wrong/undesirable for a different reason. Taking into\naccount what was said above about test_path_is_file() being an\n_assertion_ that some condition is true, coupled with the comment\nabove this `if` statement which says \"Compress::Zlib may not be\navailable\", then this `test -f` is legitimately part of the\ncontrol-flow of the function. It is not a mere assertion. Thus,\nreplacing it with the assertion function test_path_is_file() breaks\nthe test for the case when Compress::Zlib is not available.\n\n> -                       test -f \"$unhandled\".gz &&\n> -                       test -f \"$unhandled\" &&\n> +                       test_path_is_file \"$unhandled\".gz &&\n> +                       test_path_is_file \"$unhandled\" &&\n\nThese replacements are correct in that they replace the _assertion_\n`test -f` with the equivalent assertion `test_path_is_file`.\n"},{"id":"488463","messageId":"pull.1661.v2.git.1707765433663.gitgitgadget@gmail.com","threadId":"60900","inReplyTo":"pull.1661.git.1707663197543.gitgitgadget@gmail.com","subject":"[PATCH v2] t9146: replace test -d/-e/-f with appropriate test_path_is_* function","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-12T19:17:13Z","receivedAt":"2024-02-12T19:17:17Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap <chandrapratap3519@gmail.com>\n\nThe helper functions test_path_is_* provide better debugging\ninformation than test -d/-e/-f.\n\nReplace \"if ! test -d then <error message>\" with \"test_path_exists\"\nand \"test -d\" with \"test_path_is_dir\" at places where we check for\nexistent directories.\n\nReplace \"test -f\" with \"test_path_is_file\" at places where we check\nfor existent files.\n\nReplace \"test ! -e\" with \"test_path_is_missing\" where we check for\nnon-existent directories.\n\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n    t9146: replace test -d/-f with appropriate test_path_is_* function\n    \n    cc: Eric Sunshine sunshine@sunshineco.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1661%2FChand-ra%2Ftestfix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1661/Chand-ra/testfix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1661\n\nRange-diff vs v1:\n\n 1:  93fe9e9eef7 ! 1:  5734b9edd61 t9146: replace test -d/-f with appropriate test_path_is_* function\n     @@ Metadata\n      Author: Chandra Pratap <chandrapratap3519@gmail.com>\n      \n       ## Commit message ##\n     -    t9146: replace test -d/-f with appropriate test_path_is_* function\n     +    t9146: replace test -d/-e/-f with appropriate test_path_is_* function\n      \n          The helper functions test_path_is_* provide better debugging\n          information than test -d/-e/-f.\n      \n     -    Replace \"! test -d\" with \"test_path_is_missing\" at places where\n     -    we check for non-existent directories.\n     +    Replace \"if ! test -d then <error message>\" with \"test_path_exists\"\n     +    and \"test -d\" with \"test_path_is_dir\" at places where we check for\n     +    existent directories.\n      \n     -    Replace \"test -f\" with \"test_path_is_file\" and \"test -d\" with\n     -    \"test_path_is_dir\" at places where we expect files or directories\n     -    to exist.\n     +    Replace \"test -f\" with \"test_path_is_file\" at places where we check\n     +    for existent files.\n      \n     +    Replace \"test ! -e\" with \"test_path_is_missing\" where we check for\n     +    non-existent directories.\n     +\n     +    Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n      \n       ## t/t9146-git-svn-empty-dirs.sh ##\n     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'empty directories exist' '\n       \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n       \t\tdo\n      -\t\t\tif ! test -d \"$i\"\n     -+\t\t\tif test_path_is_missing \"$i\"\n     - \t\t\tthen\n     - \t\t\t\techo >&2 \"$i does not exist\" &&\n     - \t\t\t\texit 1\n     +-\t\t\tthen\n     +-\t\t\t\techo >&2 \"$i does not exist\" &&\n     +-\t\t\t\texit 1\n     +-\t\t\tfi\n     ++\t\t\ttest_path_exists \"$i\" || exit 1\n     + \t\tdone\n     + \t)\n     + '\n      @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'option automkdirs set to false' '\n       \t\tgit svn fetch &&\n       \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n       \t\tdo\n      -\t\t\tif test -d \"$i\"\n     -+\t\t\tif test_path_is_dir \"$i\"\n     - \t\t\tthen\n     - \t\t\t\techo >&2 \"$i exists\" &&\n     - \t\t\t\texit 1\n     +-\t\t\tthen\n     +-\t\t\t\techo >&2 \"$i exists\" &&\n     +-\t\t\t\texit 1\n     +-\t\t\tfi\n     ++\t\t\ttest_path_is_missing \"$i\" || exit 1\n     + \t\tdone\n     + \t)\n     + '\n      @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'more emptiness' '\n       \n       test_expect_success 'git svn rebase creates empty directory' '\n     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn mkdirs recreates emp\n       \t\tfor i in a b c d d/e d/e/f \"weird file name\" \"! !\"\n       \t\tdo\n      -\t\t\tif ! test -d \"$i\"\n     -+\t\t\tif test_path_is_missing \"$i\"\n     - \t\t\tthen\n     - \t\t\t\techo >&2 \"$i does not exist\" &&\n     - \t\t\t\texit 1\n     +-\t\t\tthen\n     +-\t\t\t\techo >&2 \"$i does not exist\" &&\n     +-\t\t\t\texit 1\n     +-\t\t\tfi\n     ++\t\t\ttest_path_exists \"$i\" || exit 1\n     + \t\tdone\n     + \t)\n     + '\n      @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn mkdirs -r works' '\n       \t\tgit svn mkdirs -r7 &&\n       \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n       \t\tdo\n      -\t\t\tif ! test -d \"$i\"\n     -+\t\t\tif test_path_is_missing \"$i\"\n     - \t\t\tthen\n     - \t\t\t\techo >&2 \"$i does not exist\" &&\n     - \t\t\t\texit 1\n     - \t\t\tfi\n     +-\t\t\tthen\n     +-\t\t\t\techo >&2 \"$i does not exist\" &&\n     +-\t\t\t\texit 1\n     +-\t\t\tfi\n     ++\t\t\ttest_path_exists \"$i\" || exit 1\n       \t\tdone &&\n       \n      -\t\tif test -d \"! !\"\n     -+\t\tif test_path_is_dir \"! !\"\n     - \t\tthen\n     - \t\t\techo >&2 \"$i should not exist\" &&\n     - \t\t\texit 1\n     - \t\tfi &&\n     +-\t\tthen\n     +-\t\t\techo >&2 \"$i should not exist\" &&\n     +-\t\t\texit 1\n     +-\t\tfi &&\n     ++\t\ttest_path_is_missing \"! !\" || exit 1 &&\n       \n       \t\tgit svn mkdirs -r8 &&\n      -\t\tif ! test -d \"! !\"\n     -+\t\tif test_path_is_missing \"! !\"\n     - \t\tthen\n     - \t\t\techo >&2 \"$i not exist\" &&\n     - \t\t\texit 1\n     +-\t\tthen\n     +-\t\t\techo >&2 \"$i not exist\" &&\n     +-\t\t\texit 1\n     +-\t\tfi\n     ++\t\ttest_path_exists \"! !\" || exit 1\n     + \t)\n     + '\n     + \n      @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'empty directories in trunk exist' '\n       \t\tcd trunk &&\n       \t\tfor i in a \"weird file name\"\n       \t\tdo\n      -\t\t\tif ! test -d \"$i\"\n     -+\t\t\tif test_path_is_missing \"$i\"\n     - \t\t\tthen\n     - \t\t\t\techo >&2 \"$i does not exist\" &&\n     - \t\t\t\texit 1\n     +-\t\t\tthen\n     +-\t\t\t\techo >&2 \"$i does not exist\" &&\n     +-\t\t\t\texit 1\n     +-\t\t\tfi\n     ++\t\t\ttest_path_exists \"$i\" || exit 1\n     + \t\tdone\n     + \t)\n     + '\n     +@@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'remove a top-level directory from svn' '\n     + \n     + test_expect_success 'removed top-level directory does not exist' '\n     + \tgit svn clone \"$svnrepo\" removed &&\n     +-\ttest ! -e removed/d\n     ++\ttest_path_is_missing removed/d\n     + \n     + '\n     + unhandled=.git/svn/refs/remotes/git-svn/unhandled.log\n      @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn gc-ed files work' '\n     - \t\tcd removed &&\n     - \t\tgit svn gc &&\n     - \t\t: Compress::Zlib may not be available &&\n     --\t\tif test -f \"$unhandled\".gz\n     -+\t\tif test_path_is_file \"$unhandled\".gz\n     - \t\tthen\n       \t\t\tsvn_cmd mkdir -m gz \"$svnrepo\"/gz &&\n       \t\t\tgit reset --hard $(git rev-list HEAD | tail -1) &&\n       \t\t\tgit svn rebase &&\n     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn gc-ed files work' '\n       \t\t\tfor i in a b c \"weird file name\" gz \"! !\"\n       \t\t\tdo\n      -\t\t\t\tif ! test -d \"$i\"\n     -+\t\t\t\tif test_path_is_missing \"$i\"\n     - \t\t\t\tthen\n     - \t\t\t\t\techo >&2 \"$i does not exist\" &&\n     - \t\t\t\t\texit 1\n     +-\t\t\t\tthen\n     +-\t\t\t\t\techo >&2 \"$i does not exist\" &&\n     +-\t\t\t\t\texit 1\n     +-\t\t\t\tfi\n     ++\t\t\t\ttest_path_exists \"$i\" || exit 1\n     + \t\t\tdone\n     + \t\tfi\n     + \t)\n\n\n t/t9146-git-svn-empty-dirs.sh | 56 ++++++++---------------------------\n 1 file changed, 12 insertions(+), 44 deletions(-)\n\ndiff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh\nindex 09606f1b3cf..6bf94ad802c 100755\n--- a/t/t9146-git-svn-empty-dirs.sh\n+++ b/t/t9146-git-svn-empty-dirs.sh\n@@ -20,11 +20,7 @@ test_expect_success 'empty directories exist' '\n \t\tcd cloned &&\n \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n \t\tdo\n-\t\t\tif ! test -d \"$i\"\n-\t\t\tthen\n-\t\t\t\techo >&2 \"$i does not exist\" &&\n-\t\t\t\texit 1\n-\t\t\tfi\n+\t\t\ttest_path_exists \"$i\" || exit 1\n \t\tdone\n \t)\n '\n@@ -37,11 +33,7 @@ test_expect_success 'option automkdirs set to false' '\n \t\tgit svn fetch &&\n \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n \t\tdo\n-\t\t\tif test -d \"$i\"\n-\t\t\tthen\n-\t\t\t\techo >&2 \"$i exists\" &&\n-\t\t\t\texit 1\n-\t\t\tfi\n+\t\t\ttest_path_is_missing \"$i\" || exit 1\n \t\tdone\n \t)\n '\n@@ -52,7 +44,7 @@ test_expect_success 'more emptiness' '\n \n test_expect_success 'git svn rebase creates empty directory' '\n \t( cd cloned && git svn rebase ) &&\n-\ttest -d cloned/\"! !\"\n+\ttest_path_is_dir cloned/\"! !\"\n '\n \n test_expect_success 'git svn mkdirs recreates empty directories' '\n@@ -62,11 +54,7 @@ test_expect_success 'git svn mkdirs recreates empty directories' '\n \t\tgit svn mkdirs &&\n \t\tfor i in a b c d d/e d/e/f \"weird file name\" \"! !\"\n \t\tdo\n-\t\t\tif ! test -d \"$i\"\n-\t\t\tthen\n-\t\t\t\techo >&2 \"$i does not exist\" &&\n-\t\t\t\texit 1\n-\t\t\tfi\n+\t\t\ttest_path_exists \"$i\" || exit 1\n \t\tdone\n \t)\n '\n@@ -78,25 +66,13 @@ test_expect_success 'git svn mkdirs -r works' '\n \t\tgit svn mkdirs -r7 &&\n \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n \t\tdo\n-\t\t\tif ! test -d \"$i\"\n-\t\t\tthen\n-\t\t\t\techo >&2 \"$i does not exist\" &&\n-\t\t\t\texit 1\n-\t\t\tfi\n+\t\t\ttest_path_exists \"$i\" || exit 1\n \t\tdone &&\n \n-\t\tif test -d \"! !\"\n-\t\tthen\n-\t\t\techo >&2 \"$i should not exist\" &&\n-\t\t\texit 1\n-\t\tfi &&\n+\t\ttest_path_is_missing \"! !\" || exit 1 &&\n \n \t\tgit svn mkdirs -r8 &&\n-\t\tif ! test -d \"! !\"\n-\t\tthen\n-\t\t\techo >&2 \"$i not exist\" &&\n-\t\t\texit 1\n-\t\tfi\n+\t\ttest_path_exists \"! !\" || exit 1\n \t)\n '\n \n@@ -114,11 +90,7 @@ test_expect_success 'empty directories in trunk exist' '\n \t\tcd trunk &&\n \t\tfor i in a \"weird file name\"\n \t\tdo\n-\t\t\tif ! test -d \"$i\"\n-\t\t\tthen\n-\t\t\t\techo >&2 \"$i does not exist\" &&\n-\t\t\t\texit 1\n-\t\t\tfi\n+\t\t\ttest_path_exists \"$i\" || exit 1\n \t\tdone\n \t)\n '\n@@ -129,7 +101,7 @@ test_expect_success 'remove a top-level directory from svn' '\n \n test_expect_success 'removed top-level directory does not exist' '\n \tgit svn clone \"$svnrepo\" removed &&\n-\ttest ! -e removed/d\n+\ttest_path_is_missing removed/d\n \n '\n unhandled=.git/svn/refs/remotes/git-svn/unhandled.log\n@@ -143,15 +115,11 @@ test_expect_success 'git svn gc-ed files work' '\n \t\t\tsvn_cmd mkdir -m gz \"$svnrepo\"/gz &&\n \t\t\tgit reset --hard $(git rev-list HEAD | tail -1) &&\n \t\t\tgit svn rebase &&\n-\t\t\ttest -f \"$unhandled\".gz &&\n-\t\t\ttest -f \"$unhandled\" &&\n+\t\t\ttest_path_is_file \"$unhandled\".gz &&\n+\t\t\ttest_path_is_file \"$unhandled\" &&\n \t\t\tfor i in a b c \"weird file name\" gz \"! !\"\n \t\t\tdo\n-\t\t\t\tif ! test -d \"$i\"\n-\t\t\t\tthen\n-\t\t\t\t\techo >&2 \"$i does not exist\" &&\n-\t\t\t\t\texit 1\n-\t\t\t\tfi\n+\t\t\t\ttest_path_exists \"$i\" || exit 1\n \t\t\tdone\n \t\tfi\n \t)\n\nbase-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821\n-- \ngitgitgadget\n"},{"id":"488466","messageId":"xmqq7cj95ssb.fsf@gitster.g","threadId":"60900","inReplyTo":"pull.1661.v2.git.1707765433663.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] t9146: replace test -d/-e/-f with appropriate test_path_is_* function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-12T20:31:00Z","receivedAt":"2024-02-12T20:31:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chandra Pratap via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Chandra Pratap <chandrapratap3519@gmail.com>\n>\n> The helper functions test_path_is_* provide better debugging\n> information than test -d/-e/-f.\n\nCorrect.\n\n> Replace \"if ! test -d then <error message>\" with \"test_path_exists\"\n> and \"test -d\" with \"test_path_is_dir\" at places where we check for\n> existent directories.\n\nThe former could result in misconversion, if the intention of the\ntest was \"we cannot have directory here; a regular file is OK\", so\nwe have to be a bit more careful than mechanical conversion.\n\n> Replace \"test -f\" with \"test_path_is_file\" at places where we check\n> for existent files.\n\nOK.\n\n> Replace \"test ! -e\" with \"test_path_is_missing\" where we check for\n> non-existent directories.\n\nOK.\n\n>  \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n>  \t\tdo\n> -\t\t\tif ! test -d \"$i\"\n> -\t\t\tthen\n> -\t\t\t\techo >&2 \"$i does not exist\" &&\n> -\t\t\t\texit 1\n> -\t\t\tfi\n> +\t\t\ttest_path_exists \"$i\" || exit 1\n\nWe were saying that we are OK if \"$i\" existed as a file (not a\ndirectory), but now we complain regardless of what \"$i\" is.  Is that\ncloser to what the test originally wanted to do?  Just checking.\n\n>  \t\tdone\n>  \t)\n>  '\n> @@ -37,11 +33,7 @@ test_expect_success 'option automkdirs set to false' '\n>  \t\tgit svn fetch &&\n>  \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n>  \t\tdo\n> -\t\t\tif test -d \"$i\"\n> -\t\t\tthen\n> -\t\t\t\techo >&2 \"$i exists\" &&\n> -\t\t\t\texit 1\n> -\t\t\tfi\n> +\t\t\ttest_path_is_missing \"$i\" || exit 1\n\nDitto; are we sure the intention of the original is that nothing\nshould be at \"$i\" (instead of \"as long as it is not a directory,\nwe are OK\")?  Just checking.\n\nThe same comment applies to all conversions to test_path_exists and\ntest_path_is_missing where the original was not \"test -e\" or \"! test -e\".\nThe other ones, like the change from \"test -f\" to \"test_path_is_file\",\nlooked all correct.\n\nThanks.\n\n\n"},{"id":"488659","messageId":"pull.1661.v3.git.1707933048210.gitgitgadget@gmail.com","threadId":"60900","inReplyTo":"pull.1661.v2.git.1707765433663.gitgitgadget@gmail.com","subject":"[PATCH v3] t9146: replace test -d/-e/-f with appropriate test_path_is_* function","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-14T17:50:48Z","receivedAt":"2024-02-14T17:50:51Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap <chandrapratap3519@gmail.com>\n\nThe helper functions test_path_is_* provide better debugging\ninformation than test -d/-e/-f.\n\nReplace \"if ! test -d then <error message>\" and \"test -d\" with\n\"test_path_is_dir\" at places where we check for existent directories.\n\nReplace \"test -f\" with \"test_path_is_file\" at places where we check\nfor existent files.\n\nReplace \"test ! -e\" and \"if test -d then <error message>\" with\n\"test_path_is_missing\" where we check for non-existent directories.\n\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n    t9146: replace test -d/-f with appropriate test_path_is_* function\n    \n    I chose to retain \"test_path_is_misssing\" as a replacement for \"if test\n    -d then \" because we initialize the repository at the start of the test\n    with:\n    \n    for i in a b c d d/e d/e/f \"weird file name\" do svn_cmd mkdir -m \"mkdir\n    $i\" \"$svnrepo\"/\"$i\" || return 1 done\n    \n    and then check for the existence of these directories in the following\n    tests. I think this reproduces the behavior of the original tests close\n    enough.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1661%2FChand-ra%2Ftestfix-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1661/Chand-ra/testfix-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1661\n\nRange-diff vs v2:\n\n 1:  5734b9edd61 ! 1:  5024389e7a9 t9146: replace test -d/-e/-f with appropriate test_path_is_* function\n     @@ Commit message\n          The helper functions test_path_is_* provide better debugging\n          information than test -d/-e/-f.\n      \n     -    Replace \"if ! test -d then <error message>\" with \"test_path_exists\"\n     -    and \"test -d\" with \"test_path_is_dir\" at places where we check for\n     -    existent directories.\n     +    Replace \"if ! test -d then <error message>\" and \"test -d\" with\n     +    \"test_path_is_dir\" at places where we check for existent directories.\n      \n          Replace \"test -f\" with \"test_path_is_file\" at places where we check\n          for existent files.\n      \n     -    Replace \"test ! -e\" with \"test_path_is_missing\" where we check for\n     -    non-existent directories.\n     +    Replace \"test ! -e\" and \"if test -d then <error message>\" with\n     +    \"test_path_is_missing\" where we check for non-existent directories.\n      \n          Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'empty directories exist' '\n      -\t\t\t\techo >&2 \"$i does not exist\" &&\n      -\t\t\t\texit 1\n      -\t\t\tfi\n     -+\t\t\ttest_path_exists \"$i\" || exit 1\n     ++\t\t\ttest_path_is_dir \"$i\" || exit 1\n       \t\tdone\n       \t)\n       '\n     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn mkdirs recreates emp\n      -\t\t\t\techo >&2 \"$i does not exist\" &&\n      -\t\t\t\texit 1\n      -\t\t\tfi\n     -+\t\t\ttest_path_exists \"$i\" || exit 1\n     ++\t\t\ttest_path_is_dir \"$i\" || exit 1\n       \t\tdone\n       \t)\n       '\n     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn mkdirs -r works' '\n      -\t\t\t\techo >&2 \"$i does not exist\" &&\n      -\t\t\t\texit 1\n      -\t\t\tfi\n     -+\t\t\ttest_path_exists \"$i\" || exit 1\n     ++\t\t\ttest_path_is_dir \"$i\" || exit 1\n       \t\tdone &&\n       \n      -\t\tif test -d \"! !\"\n     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn mkdirs -r works' '\n      -\t\t\techo >&2 \"$i not exist\" &&\n      -\t\t\texit 1\n      -\t\tfi\n     -+\t\ttest_path_exists \"! !\" || exit 1\n     ++\t\ttest_path_is_dir \"! !\" || exit 1\n       \t)\n       '\n       \n     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'empty directories in trunk e\n      -\t\t\t\techo >&2 \"$i does not exist\" &&\n      -\t\t\t\texit 1\n      -\t\t\tfi\n     -+\t\t\ttest_path_exists \"$i\" || exit 1\n     ++\t\t\ttest_path_is_dir \"$i\" || exit 1\n       \t\tdone\n       \t)\n       '\n     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn gc-ed files work' '\n      -\t\t\t\t\techo >&2 \"$i does not exist\" &&\n      -\t\t\t\t\texit 1\n      -\t\t\t\tfi\n     -+\t\t\t\ttest_path_exists \"$i\" || exit 1\n     ++\t\t\t\ttest_path_is_dir \"$i\" || exit 1\n       \t\t\tdone\n       \t\tfi\n       \t)\n\n\n t/t9146-git-svn-empty-dirs.sh | 56 ++++++++---------------------------\n 1 file changed, 12 insertions(+), 44 deletions(-)\n\ndiff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh\nindex 09606f1b3cf..926ac814394 100755\n--- a/t/t9146-git-svn-empty-dirs.sh\n+++ b/t/t9146-git-svn-empty-dirs.sh\n@@ -20,11 +20,7 @@ test_expect_success 'empty directories exist' '\n \t\tcd cloned &&\n \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n \t\tdo\n-\t\t\tif ! test -d \"$i\"\n-\t\t\tthen\n-\t\t\t\techo >&2 \"$i does not exist\" &&\n-\t\t\t\texit 1\n-\t\t\tfi\n+\t\t\ttest_path_is_dir \"$i\" || exit 1\n \t\tdone\n \t)\n '\n@@ -37,11 +33,7 @@ test_expect_success 'option automkdirs set to false' '\n \t\tgit svn fetch &&\n \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n \t\tdo\n-\t\t\tif test -d \"$i\"\n-\t\t\tthen\n-\t\t\t\techo >&2 \"$i exists\" &&\n-\t\t\t\texit 1\n-\t\t\tfi\n+\t\t\ttest_path_is_missing \"$i\" || exit 1\n \t\tdone\n \t)\n '\n@@ -52,7 +44,7 @@ test_expect_success 'more emptiness' '\n \n test_expect_success 'git svn rebase creates empty directory' '\n \t( cd cloned && git svn rebase ) &&\n-\ttest -d cloned/\"! !\"\n+\ttest_path_is_dir cloned/\"! !\"\n '\n \n test_expect_success 'git svn mkdirs recreates empty directories' '\n@@ -62,11 +54,7 @@ test_expect_success 'git svn mkdirs recreates empty directories' '\n \t\tgit svn mkdirs &&\n \t\tfor i in a b c d d/e d/e/f \"weird file name\" \"! !\"\n \t\tdo\n-\t\t\tif ! test -d \"$i\"\n-\t\t\tthen\n-\t\t\t\techo >&2 \"$i does not exist\" &&\n-\t\t\t\texit 1\n-\t\t\tfi\n+\t\t\ttest_path_is_dir \"$i\" || exit 1\n \t\tdone\n \t)\n '\n@@ -78,25 +66,13 @@ test_expect_success 'git svn mkdirs -r works' '\n \t\tgit svn mkdirs -r7 &&\n \t\tfor i in a b c d d/e d/e/f \"weird file name\"\n \t\tdo\n-\t\t\tif ! test -d \"$i\"\n-\t\t\tthen\n-\t\t\t\techo >&2 \"$i does not exist\" &&\n-\t\t\t\texit 1\n-\t\t\tfi\n+\t\t\ttest_path_is_dir \"$i\" || exit 1\n \t\tdone &&\n \n-\t\tif test -d \"! !\"\n-\t\tthen\n-\t\t\techo >&2 \"$i should not exist\" &&\n-\t\t\texit 1\n-\t\tfi &&\n+\t\ttest_path_is_missing \"! !\" || exit 1 &&\n \n \t\tgit svn mkdirs -r8 &&\n-\t\tif ! test -d \"! !\"\n-\t\tthen\n-\t\t\techo >&2 \"$i not exist\" &&\n-\t\t\texit 1\n-\t\tfi\n+\t\ttest_path_is_dir \"! !\" || exit 1\n \t)\n '\n \n@@ -114,11 +90,7 @@ test_expect_success 'empty directories in trunk exist' '\n \t\tcd trunk &&\n \t\tfor i in a \"weird file name\"\n \t\tdo\n-\t\t\tif ! test -d \"$i\"\n-\t\t\tthen\n-\t\t\t\techo >&2 \"$i does not exist\" &&\n-\t\t\t\texit 1\n-\t\t\tfi\n+\t\t\ttest_path_is_dir \"$i\" || exit 1\n \t\tdone\n \t)\n '\n@@ -129,7 +101,7 @@ test_expect_success 'remove a top-level directory from svn' '\n \n test_expect_success 'removed top-level directory does not exist' '\n \tgit svn clone \"$svnrepo\" removed &&\n-\ttest ! -e removed/d\n+\ttest_path_is_missing removed/d\n \n '\n unhandled=.git/svn/refs/remotes/git-svn/unhandled.log\n@@ -143,15 +115,11 @@ test_expect_success 'git svn gc-ed files work' '\n \t\t\tsvn_cmd mkdir -m gz \"$svnrepo\"/gz &&\n \t\t\tgit reset --hard $(git rev-list HEAD | tail -1) &&\n \t\t\tgit svn rebase &&\n-\t\t\ttest -f \"$unhandled\".gz &&\n-\t\t\ttest -f \"$unhandled\" &&\n+\t\t\ttest_path_is_file \"$unhandled\".gz &&\n+\t\t\ttest_path_is_file \"$unhandled\" &&\n \t\t\tfor i in a b c \"weird file name\" gz \"! !\"\n \t\t\tdo\n-\t\t\t\tif ! test -d \"$i\"\n-\t\t\t\tthen\n-\t\t\t\t\techo >&2 \"$i does not exist\" &&\n-\t\t\t\t\texit 1\n-\t\t\t\tfi\n+\t\t\t\ttest_path_is_dir \"$i\" || exit 1\n \t\t\tdone\n \t\tfi\n \t)\n\nbase-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821\n-- \ngitgitgadget\n"}]}