{"thread":{"id":"50594","subject":"[PATCH 0/1] [GSoC][PATCH] tests: replace test -(d|f) with test_path_is_(dir|file)","startedAt":"2019-02-26T13:42:09Z","lastAt":"2019-03-05T04:58:55Z","messageCount":42,"participants":["Rohit Ashiwal via GitGitGadget","Duy Nguyen","Johannes Schindelin","Ævar Arnfjörð Bjarmason","Martin Ågren","SZEDER Gábor","Jeff King","Matthieu Moy","Rohit Ashiwal","Junio C Hamano","Rafael Ascensão","Thomas Gummerer"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"370236","messageId":"pull.152.git.gitgitgadget@gmail.com","threadId":"50594","inReplyTo":null,"subject":"[PATCH 0/1] [GSoC][PATCH] tests: replace test -(d|f) with test_path_is_(dir|file)","fromName":"Rohit Ashiwal via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-26T13:42:05Z","receivedAt":"2019-02-26T13:42:09Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Previously we were using test -(d|f) to verify the presencee of a\ndirectory/file, but we already have helper functions, viz, test_path_is_dir\nand test_path_is_file with same functionality. This patch will replace test\n-(d|f) calls in t3600-rm.sh.\n\nRohit Ashiwal (1):\n tests: replace `test -(d|f)` with test_path_is_(dir|file)\n\n t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------\n 1 file changed, 48 insertions(+), 48 deletions(-)\n\n\nbase-commit: 8104ec994ea3849a968b4667d072fedd1e688642\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-152%2Fr1walz%2Frefactor-tests-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-152/r1walz/refactor-tests-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/152\n-- \ngitgitgadget\n"},{"id":"370237","messageId":"bf5eb045795579dd5d996e787e246996688cf4bf.1551188524.git.gitgitgadget@gmail.com","threadId":"50594","inReplyTo":"pull.152.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)","fromName":"Rohit Ashiwal via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-26T13:42:06Z","receivedAt":"2019-02-26T13:42:10Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"From: Rohit Ashiwal \n\nt3600-rm.sh: Previously we were using `test -(d|f)`\nto verify the presencee of a directory/file, but we\nalready have helper functions, viz, test_path_is_dir\nand test_path_is_file with same functionality. This\npatch will replace `test -(d|f)` calls in t3600-rm.sh.\n\nSigned-off-by: Rohit Ashiwal \n---\n t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------\n 1 file changed, 48 insertions(+), 48 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 04e5d42bd3..dcaa2ab4d6 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -137,8 +137,8 @@ test_expect_success 'Re-add foo and baz' '\n test_expect_success 'Modify foo -- rm should refuse' '\n \techo >>foo &&\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n@@ -159,8 +159,8 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '\n \n test_expect_success 'foo is different in index from HEAD -- rm should refuse' '\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n@@ -194,21 +194,21 @@ test_expect_success 'Recursive test setup' '\n \n test_expect_success 'Recursive without -r fails' '\n \ttest_must_fail git rm frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r but dirty' '\n \techo qfwfq >>frotz/nitfol &&\n \ttest_must_fail git rm -r frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r -f' '\n \tgit rm -f -r frotz &&\n-\t! test -f frotz/nitfol &&\n-\t! test -d frotz\n+\t! test_path_is_file frotz/nitfol &&\n+\t! test_path_is_dir frotz\n '\n \n test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n@@ -254,7 +254,7 @@ test_expect_success 'rm removes subdirectories recursively' '\n \techo content >dir/subdir/subsubdir/file &&\n \tgit add dir/subdir/subsubdir/file &&\n \tgit rm -f dir/subdir/subsubdir/file &&\n-\t! test -d dir\n+\t! test_path_is_dir dir\n '\n \n cat >expect <actual &&\n \ttest_cmp expect.modified actual &&\n \tgit rm -f submod &&\n@@ -359,8 +359,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm --cached submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.cached actual &&\n \tgit config -f .gitmodules submodule.sub.url &&\n@@ -371,7 +371,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm -n submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-index --exit-code HEAD\n '\n \n@@ -381,8 +381,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '\n \tgit rm .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\t! test_path_is_dir submod &&\n+\t! test_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.both_deleted actual\n '\n@@ -393,14 +393,14 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta\n \tgit config -f .gitmodules foo.bar true &&\n \ttest_must_fail git rm submod >actual 2>actual.err &&\n \ttest -s actual.err &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-files --quiet -- submod &&\n \tgit add .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\t! test_path_is_dir submod &&\n+\t! test_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -413,8 +413,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule\n \techo \"warning: Could not find section in .gitmodules where path=submod\" >expect.err &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_i18ncmp expect.err actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\t! test_path_is_dir submod &&\n+\t! test_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -424,8 +424,8 @@ test_expect_success 'rm of a populated submodule with modifications fails unless\n \tgit submodule update &&\n \techo X >submod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n@@ -439,8 +439,8 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \tgit submodule update &&\n \techo X >submod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n@@ -493,8 +493,8 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n@@ -512,8 +512,8 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f\n \techo X >submod/empty &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n@@ -531,8 +531,8 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files\n \techo X >submod/untracked &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n@@ -552,13 +552,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director\n \t) &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \ttest_must_fail git rm -f submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit merge --abort &&\n@@ -586,8 +586,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates\n \t\trm -r ../.git/modules/sub\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/.git &&\n+\t! test_path_is_dir submod &&\n+\t! test_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest -s actual &&\n \ttest_i18ngrep Migrating output.err\n@@ -624,8 +624,8 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \tgit submodule update --recursive &&\n \tgit -C submod/subsubmod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n@@ -639,8 +639,8 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n@@ -654,8 +654,8 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n@@ -673,8 +673,8 @@ test_expect_success \"rm absorbs submodule's nested .git directory\" '\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/subsubmod/.git &&\n+\t! test_path_is_dir submod &&\n+\t! test_path_is_dir submod/subsubmod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest -s actual &&\n \ttest_i18ngrep Migrating output.err\n-- \ngitgitgadget\n"},{"id":"370238","messageId":"CACsJy8DG6+mmA5NT67V46=n1-5H_eh3779eE28YN4kcjb0Cq0A@mail.gmail.com","threadId":"50594","inReplyTo":"bf5eb045795579dd5d996e787e246996688cf4bf.1551188524.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-26T14:04:46Z","receivedAt":"2019-02-26T14:05:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 26, 2019 at 8:42 PM Rohit Ashiwal via GitGitGadget\n wrote:\n>\n> From: Rohit Ashiwal \n>\n> t3600-rm.sh: Previously we were using `test -(d|f)`\n> to verify the presencee of a directory/file, but we\n> already have helper functions, viz, test_path_is_dir\n> and test_path_is_file with same functionality. This\n\nIt's not just the same (no point replacing then). It's better. When\ntest_path_is_xxx fails, you get an error message. If \"test -xxx\"\nfails, you get a failed test with no clue what caused it.\n\n> patch will replace `test -(d|f)` calls in t3600-rm.sh.\n>\n> Signed-off-by: Rohit Ashiwal \n> ---\n> t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------\n> 1 file changed, 48 insertions(+), 48 deletions(-)\n>\n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> index 04e5d42bd3..dcaa2ab4d6 100755\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n> @@ -137,8 +137,8 @@ test_expect_success 'Re-add foo and baz' '\n> test_expect_success 'Modify foo -- rm should refuse' '\n> echo >>foo &&\n> test_must_fail git rm foo baz &&\n> - test -f foo &&\n> - test -f baz &&\n> + test_path_is_file foo &&\n> + test_path_is_file baz &&\n> git ls-files --error-unmatch foo baz\n> '\n>\n> @@ -159,8 +159,8 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '\n>\n> test_expect_success 'foo is different in index from HEAD -- rm should refuse' '\n> test_must_fail git rm foo baz &&\n> - test -f foo &&\n> - test -f baz &&\n> + test_path_is_file foo &&\n> + test_path_is_file baz &&\n> git ls-files --error-unmatch foo baz\n> '\n>\n> @@ -194,21 +194,21 @@ test_expect_success 'Recursive test setup' '\n>\n> test_expect_success 'Recursive without -r fails' '\n> test_must_fail git rm frotz &&\n> - test -d frotz &&\n> - test -f frotz/nitfol\n> + test_path_is_dir frotz &&\n> + test_path_is_file frotz/nitfol\n> '\n>\n> test_expect_success 'Recursive with -r but dirty' '\n> echo qfwfq >>frotz/nitfol &&\n> test_must_fail git rm -r frotz &&\n> - test -d frotz &&\n> - test -f frotz/nitfol\n> + test_path_is_dir frotz &&\n> + test_path_is_file frotz/nitfol\n> '\n>\n> test_expect_success 'Recursive with -r -f' '\n> git rm -f -r frotz &&\n> - ! test -f frotz/nitfol &&\n> - ! test -d frotz\n> + ! test_path_is_file frotz/nitfol &&\n> + ! test_path_is_dir frotz\n> '\n>\n> test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n> @@ -254,7 +254,7 @@ test_expect_success 'rm removes subdirectories recursively' '\n> echo content >dir/subdir/subsubdir/file &&\n> git add dir/subdir/subsubdir/file &&\n> git rm -f dir/subdir/subsubdir/file &&\n> - ! test -d dir\n> + ! test_path_is_dir dir\n> '\n>\n> cat >expect < @@ -343,8 +343,8 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles\n> git submodule update &&\n> git -C submod checkout HEAD^ &&\n> test_must_fail git rm submod &&\n> - test -d submod &&\n> - test -f submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_file submod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test_cmp expect.modified actual &&\n> git rm -f submod &&\n> @@ -359,8 +359,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g\n> git reset --hard &&\n> git submodule update &&\n> git rm --cached submod &&\n> - test -d submod &&\n> - test -f submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_file submod/.git &&\n> git status -s -uno >actual &&\n> test_cmp expect.cached actual &&\n> git config -f .gitmodules submodule.sub.url &&\n> @@ -371,7 +371,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '\n> git reset --hard &&\n> git submodule update &&\n> git rm -n submod &&\n> - test -f submod/.git &&\n> + test_path_is_file submod/.git &&\n> git diff-index --exit-code HEAD\n> '\n>\n> @@ -381,8 +381,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '\n> git rm .gitmodules &&\n> git rm submod >actual 2>actual.err &&\n> test_must_be_empty actual.err &&\n> - ! test -d submod &&\n> - ! test -f submod/.git &&\n> + ! test_path_is_dir submod &&\n> + ! test_path_is_file submod/.git &&\n> git status -s -uno >actual &&\n> test_cmp expect.both_deleted actual\n> '\n> @@ -393,14 +393,14 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta\n> git config -f .gitmodules foo.bar true &&\n> test_must_fail git rm submod >actual 2>actual.err &&\n> test -s actual.err &&\n> - test -d submod &&\n> - test -f submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_file submod/.git &&\n> git diff-files --quiet -- submod &&\n> git add .gitmodules &&\n> git rm submod >actual 2>actual.err &&\n> test_must_be_empty actual.err &&\n> - ! test -d submod &&\n> - ! test -f submod/.git &&\n> + ! test_path_is_dir submod &&\n> + ! test_path_is_file submod/.git &&\n> git status -s -uno >actual &&\n> test_cmp expect actual\n> '\n> @@ -413,8 +413,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule\n> echo \"warning: Could not find section in .gitmodules where path=submod\" >expect.err &&\n> git rm submod >actual 2>actual.err &&\n> test_i18ncmp expect.err actual.err &&\n> - ! test -d submod &&\n> - ! test -f submod/.git &&\n> + ! test_path_is_dir submod &&\n> + ! test_path_is_file submod/.git &&\n> git status -s -uno >actual &&\n> test_cmp expect actual\n> '\n> @@ -424,8 +424,8 @@ test_expect_success 'rm of a populated submodule with modifications fails unless\n> git submodule update &&\n> echo X >submod/empty &&\n> test_must_fail git rm submod &&\n> - test -d submod &&\n> - test -f submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_file submod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test_cmp expect.modified_inside actual &&\n> git rm -f submod &&\n> @@ -439,8 +439,8 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n> git submodule update &&\n> echo X >submod/untracked &&\n> test_must_fail git rm submod &&\n> - test -d submod &&\n> - test -f submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_file submod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test_cmp expect.modified_untracked actual &&\n> git rm -f submod &&\n> @@ -493,8 +493,8 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD\n> git -C submod checkout HEAD^ &&\n> test_must_fail git merge conflict2 &&\n> test_must_fail git rm submod &&\n> - test -d submod &&\n> - test -f submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_file submod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test_cmp expect.conflict actual &&\n> git rm -f submod &&\n> @@ -512,8 +512,8 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f\n> echo X >submod/empty &&\n> test_must_fail git merge conflict2 &&\n> test_must_fail git rm submod &&\n> - test -d submod &&\n> - test -f submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_file submod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test_cmp expect.conflict actual &&\n> git rm -f submod &&\n> @@ -531,8 +531,8 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files\n> echo X >submod/untracked &&\n> test_must_fail git merge conflict2 &&\n> test_must_fail git rm submod &&\n> - test -d submod &&\n> - test -f submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_file submod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test_cmp expect.conflict actual &&\n> git rm -f submod &&\n> @@ -552,13 +552,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director\n> ) &&\n> test_must_fail git merge conflict2 &&\n> test_must_fail git rm submod &&\n> - test -d submod &&\n> - test -d submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_dir submod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test_cmp expect.conflict actual &&\n> test_must_fail git rm -f submod &&\n> - test -d submod &&\n> - test -d submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_dir submod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test_cmp expect.conflict actual &&\n> git merge --abort &&\n> @@ -586,8 +586,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates\n> rm -r ../.git/modules/sub\n> ) &&\n> git rm submod 2>output.err &&\n> - ! test -d submod &&\n> - ! test -d submod/.git &&\n> + ! test_path_is_dir submod &&\n> + ! test_path_is_dir submod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test -s actual &&\n> test_i18ngrep Migrating output.err\n> @@ -624,8 +624,8 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n> git submodule update --recursive &&\n> git -C submod/subsubmod checkout HEAD^ &&\n> test_must_fail git rm submod &&\n> - test -d submod &&\n> - test -f submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_file submod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test_cmp expect.modified_inside actual &&\n> git rm -f submod &&\n> @@ -639,8 +639,8 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n> git submodule update --recursive &&\n> echo X >submod/subsubmod/empty &&\n> test_must_fail git rm submod &&\n> - test -d submod &&\n> - test -f submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_file submod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test_cmp expect.modified_inside actual &&\n> git rm -f submod &&\n> @@ -654,8 +654,8 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n> git submodule update --recursive &&\n> echo X >submod/subsubmod/untracked &&\n> test_must_fail git rm submod &&\n> - test -d submod &&\n> - test -f submod/.git &&\n> + test_path_is_dir submod &&\n> + test_path_is_file submod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test_cmp expect.modified_untracked actual &&\n> git rm -f submod &&\n> @@ -673,8 +673,8 @@ test_expect_success \"rm absorbs submodule's nested .git directory\" '\n> GIT_WORK_TREE=. git config --unset core.worktree\n> ) &&\n> git rm submod 2>output.err &&\n> - ! test -d submod &&\n> - ! test -d submod/subsubmod/.git &&\n> + ! test_path_is_dir submod &&\n> + ! test_path_is_dir submod/subsubmod/.git &&\n> git status -s -uno --ignore-submodules=none >actual &&\n> test -s actual &&\n> test_i18ngrep Migrating output.err\n> --\n> gitgitgadget\n\n\n\n-- \nDuy\n"},{"id":"370239","messageId":"pull.152.v2.git.gitgitgadget@gmail.com","threadId":"50594","inReplyTo":"pull.152.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] [GSoC][PATCH] t3600: use test_path_is_dir and test_path_is_file","fromName":"Rohit Ashiwal via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-26T14:26:09Z","receivedAt":"2019-02-26T14:26:14Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Previously we were using test -(d|f) to verify the presencee of a\ndirectory/file, but we already have helper functions, viz, test_path_is_dir\nand test_path_is_file with same functionality. This patch will replace test\n-(d|f) calls in t3600-rm.sh.\n\nRohit Ashiwal (1):\n t3600: use test_path_is_dir and test_path_is_file\n\n t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------\n 1 file changed, 48 insertions(+), 48 deletions(-)\n\n\nbase-commit: 8104ec994ea3849a968b4667d072fedd1e688642\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-152%2Fr1walz%2Frefactor-tests-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-152/r1walz/refactor-tests-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/152\n\nRange-diff vs v1:\n\n 1: bf5eb04579 ! 1: fcafc87b38 tests: replace `test -(d|f)` with test_path_is_(dir|file)\n @@ -1,12 +1,16 @@\n Author: Rohit Ashiwal \n \n - tests: replace `test -(d|f)` with test_path_is_(dir|file)\n + t3600: use test_path_is_dir and test_path_is_file\n \n - t3600-rm.sh: Previously we were using `test -(d|f)`\n - to verify the presencee of a directory/file, but we\n - already have helper functions, viz, test_path_is_dir\n - and test_path_is_file with same functionality. This\n - patch will replace `test -(d|f)` calls in t3600-rm.sh.\n + Previously we were using `test -(d|f)` to verify\n + the presence of a directory/file, but we already\n + have helper functions, viz, `test_path_is_dir`\n + and `test_path_is_file` with better functionality.\n + This patch will replace `test -(d|f)` calls in t3660.sh\n +\n + These helper functions make code more readable\n + and informative to someone new to code, also\n + these functions have better error messages\n \n Signed-off-by: Rohit Ashiwal \n \n\n-- \ngitgitgadget\n"},{"id":"370240","messageId":"fcafc87b382dfef00d8e33e875bcb8b03d5667e4.1551191168.git.gitgitgadget@gmail.com","threadId":"50594","inReplyTo":"pull.152.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file","fromName":"Rohit Ashiwal via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-26T14:26:09Z","receivedAt":"2019-02-26T14:26:15Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"From: Rohit Ashiwal \n\nPreviously we were using `test -(d|f)` to verify\nthe presence of a directory/file, but we already\nhave helper functions, viz, `test_path_is_dir`\nand `test_path_is_file` with better functionality.\nThis patch will replace `test -(d|f)` calls in t3660.sh\n\nThese helper functions make code more readable\nand informative to someone new to code, also\nthese functions have better error messages\n\nSigned-off-by: Rohit Ashiwal \n---\n t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------\n 1 file changed, 48 insertions(+), 48 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 04e5d42bd3..dcaa2ab4d6 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -137,8 +137,8 @@ test_expect_success 'Re-add foo and baz' '\n test_expect_success 'Modify foo -- rm should refuse' '\n \techo >>foo &&\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n@@ -159,8 +159,8 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '\n \n test_expect_success 'foo is different in index from HEAD -- rm should refuse' '\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n@@ -194,21 +194,21 @@ test_expect_success 'Recursive test setup' '\n \n test_expect_success 'Recursive without -r fails' '\n \ttest_must_fail git rm frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r but dirty' '\n \techo qfwfq >>frotz/nitfol &&\n \ttest_must_fail git rm -r frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r -f' '\n \tgit rm -f -r frotz &&\n-\t! test -f frotz/nitfol &&\n-\t! test -d frotz\n+\t! test_path_is_file frotz/nitfol &&\n+\t! test_path_is_dir frotz\n '\n \n test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n@@ -254,7 +254,7 @@ test_expect_success 'rm removes subdirectories recursively' '\n \techo content >dir/subdir/subsubdir/file &&\n \tgit add dir/subdir/subsubdir/file &&\n \tgit rm -f dir/subdir/subsubdir/file &&\n-\t! test -d dir\n+\t! test_path_is_dir dir\n '\n \n cat >expect <actual &&\n \ttest_cmp expect.modified actual &&\n \tgit rm -f submod &&\n@@ -359,8 +359,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm --cached submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.cached actual &&\n \tgit config -f .gitmodules submodule.sub.url &&\n@@ -371,7 +371,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm -n submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-index --exit-code HEAD\n '\n \n@@ -381,8 +381,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '\n \tgit rm .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\t! test_path_is_dir submod &&\n+\t! test_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.both_deleted actual\n '\n@@ -393,14 +393,14 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta\n \tgit config -f .gitmodules foo.bar true &&\n \ttest_must_fail git rm submod >actual 2>actual.err &&\n \ttest -s actual.err &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-files --quiet -- submod &&\n \tgit add .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\t! test_path_is_dir submod &&\n+\t! test_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -413,8 +413,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule\n \techo \"warning: Could not find section in .gitmodules where path=submod\" >expect.err &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_i18ncmp expect.err actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\t! test_path_is_dir submod &&\n+\t! test_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -424,8 +424,8 @@ test_expect_success 'rm of a populated submodule with modifications fails unless\n \tgit submodule update &&\n \techo X >submod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n@@ -439,8 +439,8 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \tgit submodule update &&\n \techo X >submod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n@@ -493,8 +493,8 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n@@ -512,8 +512,8 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f\n \techo X >submod/empty &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n@@ -531,8 +531,8 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files\n \techo X >submod/untracked &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n@@ -552,13 +552,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director\n \t) &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \ttest_must_fail git rm -f submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit merge --abort &&\n@@ -586,8 +586,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates\n \t\trm -r ../.git/modules/sub\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/.git &&\n+\t! test_path_is_dir submod &&\n+\t! test_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest -s actual &&\n \ttest_i18ngrep Migrating output.err\n@@ -624,8 +624,8 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \tgit submodule update --recursive &&\n \tgit -C submod/subsubmod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n@@ -639,8 +639,8 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n@@ -654,8 +654,8 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n@@ -673,8 +673,8 @@ test_expect_success \"rm absorbs submodule's nested .git directory\" '\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/subsubmod/.git &&\n+\t! test_path_is_dir submod &&\n+\t! test_path_is_dir submod/subsubmod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest -s actual &&\n \ttest_i18ngrep Migrating output.err\n-- \ngitgitgadget\n"},{"id":"370242","messageId":"nycvar.QRO.7.76.6.1902261657000.41@tvgsbejvaqbjf.bet","threadId":"50594","inReplyTo":"bf5eb045795579dd5d996e787e246996688cf4bf.1551188524.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-26T16:01:27Z","receivedAt":"2019-02-26T16:01:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Rohit,\n\nthe oneline suggests that this fixes all the tests, but it only fixes\nt3600. So maybe use \"t3600:\" instead of \"tests:\"?\n\nOn Tue, 26 Feb 2019, Rohit Ashiwal via GitGitGadget wrote:\n\n> From: Rohit Ashiwal \n> \n> t3600-rm.sh: Previously we were using `test -(d|f)`\n> to verify the presencee of a directory/file, but we\n> already have helper functions, viz, test_path_is_dir\n> and test_path_is_file with same functionality. This\n> patch will replace `test -(d|f)` calls in t3600-rm.sh.\n\nThis answers a bit of the \"what?\", but little in the way of \"how?\".\n\nAnother thing to mention in the commit message is the \"why?\"... So far, a\ncasual reader will not exactly understand what the benefit might be, and\nmight even disagree that it is an improvement because\n\n 1. the new code will be slower (as it adds one level of indirection: a\nshell function)\n\n 2. the new code is more verbose (`test -d` is shorter than\n`test_path_is_dir`)\n\n\nObviously, the active Git developers do agree, though, that it is a good\nchange (otherwise they would not have suggested it as a GSoC\nmicroproject), and I think it is because:\n\n 1. the new code is a lot more obvious to developers who are not fluent\nin Unix shell scripting, and\n\n 2. the new code is a lot more informative in case of a breakage.\n\nThe patch looks fine,\nJohannes\n\n> \n> Signed-off-by: Rohit Ashiwal \n> ---\n> t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------\n> 1 file changed, 48 insertions(+), 48 deletions(-)\n> \n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> index 04e5d42bd3..dcaa2ab4d6 100755\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n> @@ -137,8 +137,8 @@ test_expect_success 'Re-add foo and baz' '\n> test_expect_success 'Modify foo -- rm should refuse' '\n> \techo >>foo &&\n> \ttest_must_fail git rm foo baz &&\n> -\ttest -f foo &&\n> -\ttest -f baz &&\n> +\ttest_path_is_file foo &&\n> +\ttest_path_is_file baz &&\n> \tgit ls-files --error-unmatch foo baz\n> '\n> \n> @@ -159,8 +159,8 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '\n> \n> test_expect_success 'foo is different in index from HEAD -- rm should refuse' '\n> \ttest_must_fail git rm foo baz &&\n> -\ttest -f foo &&\n> -\ttest -f baz &&\n> +\ttest_path_is_file foo &&\n> +\ttest_path_is_file baz &&\n> \tgit ls-files --error-unmatch foo baz\n> '\n> \n> @@ -194,21 +194,21 @@ test_expect_success 'Recursive test setup' '\n> \n> test_expect_success 'Recursive without -r fails' '\n> \ttest_must_fail git rm frotz &&\n> -\ttest -d frotz &&\n> -\ttest -f frotz/nitfol\n> +\ttest_path_is_dir frotz &&\n> +\ttest_path_is_file frotz/nitfol\n> '\n> \n> test_expect_success 'Recursive with -r but dirty' '\n> \techo qfwfq >>frotz/nitfol &&\n> \ttest_must_fail git rm -r frotz &&\n> -\ttest -d frotz &&\n> -\ttest -f frotz/nitfol\n> +\ttest_path_is_dir frotz &&\n> +\ttest_path_is_file frotz/nitfol\n> '\n> \n> test_expect_success 'Recursive with -r -f' '\n> \tgit rm -f -r frotz &&\n> -\t! test -f frotz/nitfol &&\n> -\t! test -d frotz\n> +\t! test_path_is_file frotz/nitfol &&\n> +\t! test_path_is_dir frotz\n> '\n> \n> test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n> @@ -254,7 +254,7 @@ test_expect_success 'rm removes subdirectories recursively' '\n> \techo content >dir/subdir/subsubdir/file &&\n> \tgit add dir/subdir/subsubdir/file &&\n> \tgit rm -f dir/subdir/subsubdir/file &&\n> -\t! test -d dir\n> +\t! test_path_is_dir dir\n> '\n> \n> cat >expect < @@ -343,8 +343,8 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles\n> \tgit submodule update &&\n> \tgit -C submod checkout HEAD^ &&\n> \ttest_must_fail git rm submod &&\n> -\ttest -d submod &&\n> -\ttest -f submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_file submod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest_cmp expect.modified actual &&\n> \tgit rm -f submod &&\n> @@ -359,8 +359,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g\n> \tgit reset --hard &&\n> \tgit submodule update &&\n> \tgit rm --cached submod &&\n> -\ttest -d submod &&\n> -\ttest -f submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_file submod/.git &&\n> \tgit status -s -uno >actual &&\n> \ttest_cmp expect.cached actual &&\n> \tgit config -f .gitmodules submodule.sub.url &&\n> @@ -371,7 +371,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '\n> \tgit reset --hard &&\n> \tgit submodule update &&\n> \tgit rm -n submod &&\n> -\ttest -f submod/.git &&\n> +\ttest_path_is_file submod/.git &&\n> \tgit diff-index --exit-code HEAD\n> '\n> \n> @@ -381,8 +381,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '\n> \tgit rm .gitmodules &&\n> \tgit rm submod >actual 2>actual.err &&\n> \ttest_must_be_empty actual.err &&\n> -\t! test -d submod &&\n> -\t! test -f submod/.git &&\n> +\t! test_path_is_dir submod &&\n> +\t! test_path_is_file submod/.git &&\n> \tgit status -s -uno >actual &&\n> \ttest_cmp expect.both_deleted actual\n> '\n> @@ -393,14 +393,14 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta\n> \tgit config -f .gitmodules foo.bar true &&\n> \ttest_must_fail git rm submod >actual 2>actual.err &&\n> \ttest -s actual.err &&\n> -\ttest -d submod &&\n> -\ttest -f submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_file submod/.git &&\n> \tgit diff-files --quiet -- submod &&\n> \tgit add .gitmodules &&\n> \tgit rm submod >actual 2>actual.err &&\n> \ttest_must_be_empty actual.err &&\n> -\t! test -d submod &&\n> -\t! test -f submod/.git &&\n> +\t! test_path_is_dir submod &&\n> +\t! test_path_is_file submod/.git &&\n> \tgit status -s -uno >actual &&\n> \ttest_cmp expect actual\n> '\n> @@ -413,8 +413,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule\n> \techo \"warning: Could not find section in .gitmodules where path=submod\" >expect.err &&\n> \tgit rm submod >actual 2>actual.err &&\n> \ttest_i18ncmp expect.err actual.err &&\n> -\t! test -d submod &&\n> -\t! test -f submod/.git &&\n> +\t! test_path_is_dir submod &&\n> +\t! test_path_is_file submod/.git &&\n> \tgit status -s -uno >actual &&\n> \ttest_cmp expect actual\n> '\n> @@ -424,8 +424,8 @@ test_expect_success 'rm of a populated submodule with modifications fails unless\n> \tgit submodule update &&\n> \techo X >submod/empty &&\n> \ttest_must_fail git rm submod &&\n> -\ttest -d submod &&\n> -\ttest -f submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_file submod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest_cmp expect.modified_inside actual &&\n> \tgit rm -f submod &&\n> @@ -439,8 +439,8 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n> \tgit submodule update &&\n> \techo X >submod/untracked &&\n> \ttest_must_fail git rm submod &&\n> -\ttest -d submod &&\n> -\ttest -f submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_file submod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest_cmp expect.modified_untracked actual &&\n> \tgit rm -f submod &&\n> @@ -493,8 +493,8 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD\n> \tgit -C submod checkout HEAD^ &&\n> \ttest_must_fail git merge conflict2 &&\n> \ttest_must_fail git rm submod &&\n> -\ttest -d submod &&\n> -\ttest -f submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_file submod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest_cmp expect.conflict actual &&\n> \tgit rm -f submod &&\n> @@ -512,8 +512,8 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f\n> \techo X >submod/empty &&\n> \ttest_must_fail git merge conflict2 &&\n> \ttest_must_fail git rm submod &&\n> -\ttest -d submod &&\n> -\ttest -f submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_file submod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest_cmp expect.conflict actual &&\n> \tgit rm -f submod &&\n> @@ -531,8 +531,8 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files\n> \techo X >submod/untracked &&\n> \ttest_must_fail git merge conflict2 &&\n> \ttest_must_fail git rm submod &&\n> -\ttest -d submod &&\n> -\ttest -f submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_file submod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest_cmp expect.conflict actual &&\n> \tgit rm -f submod &&\n> @@ -552,13 +552,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director\n> \t) &&\n> \ttest_must_fail git merge conflict2 &&\n> \ttest_must_fail git rm submod &&\n> -\ttest -d submod &&\n> -\ttest -d submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_dir submod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest_cmp expect.conflict actual &&\n> \ttest_must_fail git rm -f submod &&\n> -\ttest -d submod &&\n> -\ttest -d submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_dir submod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest_cmp expect.conflict actual &&\n> \tgit merge --abort &&\n> @@ -586,8 +586,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates\n> \t\trm -r ../.git/modules/sub\n> \t) &&\n> \tgit rm submod 2>output.err &&\n> -\t! test -d submod &&\n> -\t! test -d submod/.git &&\n> +\t! test_path_is_dir submod &&\n> +\t! test_path_is_dir submod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest -s actual &&\n> \ttest_i18ngrep Migrating output.err\n> @@ -624,8 +624,8 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n> \tgit submodule update --recursive &&\n> \tgit -C submod/subsubmod checkout HEAD^ &&\n> \ttest_must_fail git rm submod &&\n> -\ttest -d submod &&\n> -\ttest -f submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_file submod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest_cmp expect.modified_inside actual &&\n> \tgit rm -f submod &&\n> @@ -639,8 +639,8 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n> \tgit submodule update --recursive &&\n> \techo X >submod/subsubmod/empty &&\n> \ttest_must_fail git rm submod &&\n> -\ttest -d submod &&\n> -\ttest -f submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_file submod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest_cmp expect.modified_inside actual &&\n> \tgit rm -f submod &&\n> @@ -654,8 +654,8 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n> \tgit submodule update --recursive &&\n> \techo X >submod/subsubmod/untracked &&\n> \ttest_must_fail git rm submod &&\n> -\ttest -d submod &&\n> -\ttest -f submod/.git &&\n> +\ttest_path_is_dir submod &&\n> +\ttest_path_is_file submod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest_cmp expect.modified_untracked actual &&\n> \tgit rm -f submod &&\n> @@ -673,8 +673,8 @@ test_expect_success \"rm absorbs submodule's nested .git directory\" '\n> \t\tGIT_WORK_TREE=. git config --unset core.worktree\n> \t) &&\n> \tgit rm submod 2>output.err &&\n> -\t! test -d submod &&\n> -\t! test -d submod/subsubmod/.git &&\n> +\t! test_path_is_dir submod &&\n> +\t! test_path_is_dir submod/subsubmod/.git &&\n> \tgit status -s -uno --ignore-submodules=none >actual &&\n> \ttest -s actual &&\n> \ttest_i18ngrep Migrating output.err\n> -- \n> gitgitgadget\n> \n"},{"id":"370243","messageId":"87sgwav8cp.fsf@evledraar.gmail.com","threadId":"50594","inReplyTo":"CACsJy8DG6+mmA5NT67V46=n1-5H_eh3779eE28YN4kcjb0Cq0A@mail.gmail.com","subject":"Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-26T16:10:30Z","receivedAt":"2019-02-26T16:10:37Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 26 2019, Duy Nguyen wrote:\n\n> On Tue, Feb 26, 2019 at 8:42 PM Rohit Ashiwal via GitGitGadget\n> wrote:\n>>\n>> From: Rohit Ashiwal \n>>\n>> t3600-rm.sh: Previously we were using `test -(d|f)`\n>> to verify the presencee of a directory/file, but we\n>> already have helper functions, viz, test_path_is_dir\n>> and test_path_is_file with same functionality. This\n>\n> It's not just the same (no point replacing then). It's better. When\n> test_path_is_xxx fails, you get an error message. If \"test -xxx\"\n> fails, you get a failed test with no clue what caused it.\n\nI swear I'm not just on a mission to ruin everyone's GSOC projects. This\npatch definitely looks good, and given that we have this / document it\nmakes sense.\n\nHowever. I wonder in general if we've re-visited the utility of these\nwrappers and maybe other similar wrappers after -x was added.\n\nBack when this was added in 2caf20c52b (\"test-lib: user-friendly\nalternatives to test [-d|-f|-e]\", 2010-08-10) we didn't have -x. So we'd\nat best fail like this:\n\n $ ./t0001-init.sh -v -i\n Initialized empty Git repository in /home/avar/g/git/t/trash directory.t0001-init/.git/\n expecting success:\n test -d .git &&\n test -f doesnotexist &&\n test -f .git/config\n\n not ok 1 - check files\n #\n # test -d .git &&\n # test -f doesnotexist &&\n # test -f .git/config\n #\n\nAt that point this was a definite improvement:\n\n expecting success:\n test_path_is_dir .git &&\n test_path_is_file doesnotexist &&\n test_path_is_file .git/config\n\n File doesnotexist doesn't exist.\n not ok 1 - check files\n\nBut 4 years after this was added in a136f6d8ff (\"test-lib.sh: support -x\noption for shell-tracing\", 2014-10-10) we got -x, and then with \"-i -v -x\":\n\n expecting success:\n test_path_is_dir .git &&\n test_path_is_file doesnotexist &&\n test_path_is_file .git/config\n\n + test_path_is_dir .git\n + test -d .git\n + test_path_is_file doesnotexist\n + test -f doesnotexist\n + echo File doesnotexist doesn't exist.\n File doesnotexist doesn't exist.\n + false\n error: last command exited with $?=1\n not ok 1 - check files\n\nBut by just using \"test -d/-e\": the much shorter:\n\n + test -d .git\n + test -f doesnotexist\n error: last command exited with $?=1\n not ok 1 - check files\n\nSo I wonder if these days we shouldn't do this the other way around and\nget rid of these. Every test_* wrapper we add adds a bit of cognitive\noverload when you have to remember Git's specific shellscript dialect.\n\nAnd at least to me whenever I have a test failure the first thing I do\nis try with -x (if I wasn't already using it). Under that the wrapper\noutput is more verbose and no more helpful. It's immediately clear\nwhat's going on with:\n\n + test -f doesnotexist\n error: last command exited with $?=1\n\nWhereas:\n\n + test -f doesnotexist\n + echo File doesnotexist doesn't exist.\n File doesnotexist doesn't exist.\n + false\n error: last command exited with $?=1\n\nGives me the same thing, but I have to read 5 lines instead of 2 that\nultimately don't tell me any more (and a bit of \"huh, 'false' returned\n1? Of course! Oh! It's faking things up and it's the 'echo' that\nmatters...\").\n\nLooking over test-lib-functions.sh this patch would do it. I couldn't\nspot any other functions redundant to -x:\n\n diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n index 80402a428f..b3a95b4968 100644\n --- a/t/test-lib-functions.sh\n +++ b/t/test-lib-functions.sh\n @@ -555,33 +555,6 @@ test_external_without_stderr () {\n \tfi\n }\n\n -# debugging-friendly alternatives to \"test [-f|-d|-e]\"\n -# The commands test the existence or non-existence of $1. $2 can be\n -# given to provide a more precise diagnosis.\n -test_path_is_file () {\n -\tif ! test -f \"$1\"\n -\tthen\n -\t\techo \"File $1 doesn't exist. $2\"\n -\t\tfalse\n -\tfi\n -}\n -\n -test_path_is_dir () {\n -\tif ! test -d \"$1\"\n -\tthen\n -\t\techo \"Directory $1 doesn't exist. $2\"\n -\t\tfalse\n -\tfi\n -}\n -\n -test_path_exists () {\n -\tif ! test -e \"$1\"\n -\tthen\n -\t\techo \"Path $1 doesn't exist. $2\"\n -\t\tfalse\n -\tfi\n -}\n -\n # Check if the directory exists and is empty as expected, barf otherwise.\n test_dir_is_empty () {\n \ttest_path_is_dir \"$1\" &&\n @@ -593,19 +566,6 @@ test_dir_is_empty () {\n \tfi\n }\n\n -test_path_is_missing () {\n -\tif test -e \"$1\"\n -\tthen\n -\t\techo \"Path exists:\"\n -\t\tls -ld \"$1\"\n -\t\tif test $# -ge 1\n -\t\tthen\n -\t\t\techo \"$*\"\n -\t\tfi\n -\t\tfalse\n -\tfi\n -}\n -\n # test_line_count checks that a file has the number of lines it\n # ought to. For example:\n #\n @@ -849,6 +809,9 @@ verbose () {\n # otherwise.\n\n test_must_be_empty () {\n +\t# We don't want to remove this as noted in ec10b018e7 (\"tests:\n +\t# use 'test_must_be_empty' instead of '! test -s'\",\n +\t# 2018-08-19)\n \ttest_path_is_file \"$1\" &&\n \tif test -s \"$1\"\n \tthen\n"},{"id":"370244","messageId":"CAN0heSqSp-a0zUKT5EaGLBYnRtESTnu9GKWtGARz2kaOAhc1HQ@mail.gmail.com","threadId":"50594","inReplyTo":"bf5eb045795579dd5d996e787e246996688cf4bf.1551188524.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-02-26T16:30:51Z","receivedAt":"2019-02-26T16:31:07Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 26 Feb 2019 at 14:43, Rohit Ashiwal via GitGitGadget\n wrote:\n>\n> t3600-rm.sh: Previously we were using `test -(d|f)`\n> to verify the presencee of a directory/file, but we\n> already have helper functions, viz, test_path_is_dir\n> and test_path_is_file with same functionality. This\n> patch will replace `test -(d|f)` calls in t3600-rm.sh.\n\nI think this makes a lot of sense. If a test breaks, we'll get some\nhelpful error message. Thank you for working on this.\n\n> - ! test -d submod &&\n> + ! test_path_is_dir submod &&\n\nNow, here I wonder. This (and other changes like this) means that every\ntime the test passes, we see \"Directory submod doesn't exist.\", which is\nperhaps not too irritating. But more importantly, when the test fails,\nwe don't get any hint. So a failure is just as silent and \"non-helpful\"\nas before. I can think of a few approaches:\n\n 1 Teach `test_path_is_dir` and friends to handle \"!\" in a clever way, and\n write these as `test_path_is_dir ! foo`. (We already have helpers\n that do this, see, e.g., `test_i18ngrep`.)\n\n 2 Don't be clever, and just introduce `test_path_is_not_dir`.\n\n 3 Don't bother, because this small change here doesn't make the error\n case any worse.\n\n 4 Don't do this small change here, and leave cases like this for a\n later change (something like 1 or 2 above).\n\nWhat do you think?\n\nThere are a few of these \"!\". The other changes look good to me.\n\nCheers\nMartin\n"},{"id":"370245","messageId":"20190226163737.GB19739@szeder.dev","threadId":"50594","inReplyTo":"fcafc87b382dfef00d8e33e875bcb8b03d5667e4.1551191168.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-26T16:37:37Z","receivedAt":"2019-02-26T16:37:44Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi and welcome!\n\nOn Tue, Feb 26, 2019 at 06:26:09AM -0800, Rohit Ashiwal via GitGitGadget wrote:\n> Previously we were using `test -(d|f)` to verify\n> the presence of a directory/file, but we already\n> have helper functions, viz, `test_path_is_dir`\n> and `test_path_is_file` with better functionality.\n> This patch will replace `test -(d|f)` calls in t3660.sh\n\nWe prefer to use imperative mode when talking about what a patch does,\nas if the author were to give orders to the code base. So e.g.\ninstead of\n\n This patch will ...\n\nwe would usually write something like this:\n\n Replace 'test -(d|f)' calls in t3600 with the corresponding helper\n functions.\n\n> These helper functions make code more readable\n> and informative to someone new to code, also\n> these functions have better error messages\n\n> Signed-off-by: Rohit Ashiwal \n> ---\n> t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------\n> 1 file changed, 48 insertions(+), 48 deletions(-)\n\nThe patch itself seems to be a straightforward application of\n\n s/test -f/test_path_is_file/\n s/test -d/test_path_is_dir/\n\nso it looks good for the most part, but it has a few issues:\n\n> test_expect_success 'Recursive with -r -f' '\n> \tgit rm -f -r frotz &&\n> -\t! test -f frotz/nitfol &&\n> -\t! test -d frotz\n> +\t! test_path_is_file frotz/nitfol &&\n> +\t! test_path_is_dir frotz\n> '\n\nThese should rather use the test_path_is_missing helper function.\n\nHowever, if the directory 'frotz' is missing, then surely\n'frotz/nitfol' could not possibly exist either, could it? I'm not\nsure why this test (and a couple of others) checks both, and wonder\nwhether the redundant check for the file inside the supposedly\nnon-existing directory could be removed.\n\n\nFurthermore, there are a couple of place where the '!' is not in front\nof the whole 'test' command but is given as an argument, e.g.:\n\n test ! -f file\n\nPlease convert those cases as well.\n\n"},{"id":"370246","messageId":"20190226170400.GC19739@szeder.dev","threadId":"50594","inReplyTo":"87sgwav8cp.fsf@evledraar.gmail.com","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-26T17:04:00Z","receivedAt":"2019-02-26T17:04:07Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Feb 26, 2019 at 05:10:30PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> However. I wonder in general if we've re-visited the utility of these\n> wrappers and maybe other similar wrappers after -x was added.\n\n> But 4 years after this was added in a136f6d8ff (\"test-lib.sh: support -x\n> option for shell-tracing\", 2014-10-10) we got -x, and then with \"-i -v -x\":\n\n'-x' tracing doesn't work in all test scripts, unless it is run with a\nBash version already supporting BASH_XTRACEFD, i.e. v4.1 or later.\nNotably the default Bash shipped in macOS is somewhere around v3.2.\n\n> And at least to me whenever I have a test failure the first thing I do\n> is try with -x (if I wasn't already using it). Under that the wrapper\n> output is more verbose and no more helpful. It's immediately clear\n> what's going on with:\n> \n> + test -f doesnotexist\n> error: last command exited with $?=1\n> \n> Whereas:\n> \n> + test -f doesnotexist\n> + echo File doesnotexist doesn't exist.\n> File doesnotexist doesn't exist.\n> + false\n> error: last command exited with $?=1\n> \n> Gives me the same thing, but I have to read 5 lines instead of 2 that\n> ultimately don't tell me any more (and a bit of \"huh, 'false' returned\n> 1? Of course! Oh! It's faking things up and it's the 'echo' that\n> matters...\").\n\nI didn't find this to be an issue, but because of functions like\n'test_seq' and 'test_must_fail' I've thought about suppressing '-x'\noutput for test helpers (haven't actually done anything about it,\nthough).\n\n> Looking over test-lib-functions.sh this patch would do it. I couldn't\n> spot any other functions redundant to -x:\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 80402a428f..b3a95b4968 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -555,33 +555,6 @@ test_external_without_stderr () {\n> \tfi\n> }\n> \n> -# debugging-friendly alternatives to \"test [-f|-d|-e]\"\n> -# The commands test the existence or non-existence of $1. $2 can be\n> -# given to provide a more precise diagnosis.\n\nNote the second parameter; though, of course, you could argue that we\nuse it so rarely that it wouldn't really be missed.\n\n> -test_path_is_file () {\n> -\tif ! test -f \"$1\"\n> -\tthen\n> -\t\techo \"File $1 doesn't exist. $2\"\n> -\t\tfalse\n> -\tfi\n> -}\n> -\n> -test_path_is_dir () {\n> -\tif ! test -d \"$1\"\n> -\tthen\n> -\t\techo \"Directory $1 doesn't exist. $2\"\n> -\t\tfalse\n> -\tfi\n> -}\n> -\n> -test_path_exists () {\n> -\tif ! test -e \"$1\"\n> -\tthen\n> -\t\techo \"Path $1 doesn't exist. $2\"\n> -\t\tfalse\n> -\tfi\n> -}\n> -\n> # Check if the directory exists and is empty as expected, barf otherwise.\n> test_dir_is_empty () {\n> \ttest_path_is_dir \"$1\" &&\n> @@ -593,19 +566,6 @@ test_dir_is_empty () {\n> \tfi\n> }\n> \n> -test_path_is_missing () {\n> -\tif test -e \"$1\"\n> -\tthen\n> -\t\techo \"Path exists:\"\n> -\t\tls -ld \"$1\"\n\nThis 'ls' command gives a bit of additional info.\n\n> -\t\tif test $# -ge 1\n> -\t\tthen\n> -\t\t\techo \"$*\"\n> -\t\tfi\n> -\t\tfalse\n> -\tfi\n> -}\n> -\n> # test_line_count checks that a file has the number of lines it\n> # ought to. For example:\n> #\n> @@ -849,6 +809,9 @@ verbose () {\n> # otherwise.\n> \n> test_must_be_empty () {\n> +\t# We don't want to remove this as noted in ec10b018e7 (\"tests:\n> +\t# use 'test_must_be_empty' instead of '! test -s'\",\n> +\t# 2018-08-19)\n\nIndeed.\n\n> \ttest_path_is_file \"$1\" &&\n\nThis still uses 'test_path_is_file'.\n\n> \tif test -s \"$1\"\n> \tthen\n"},{"id":"370254","messageId":"20190226173542.GC19606@sigill.intra.peff.net","threadId":"50594","inReplyTo":"87sgwav8cp.fsf@evledraar.gmail.com","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-26T17:35:43Z","receivedAt":"2019-02-26T17:35:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 26, 2019 at 05:10:30PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> But 4 years after this was added in a136f6d8ff (\"test-lib.sh: support -x\n> option for shell-tracing\", 2014-10-10) we got -x, and then with \"-i -v -x\":\n> \n> expecting success:\n> test_path_is_dir .git &&\n> test_path_is_file doesnotexist &&\n> test_path_is_file .git/config\n> \n> + test_path_is_dir .git\n> + test -d .git\n> + test_path_is_file doesnotexist\n> + test -f doesnotexist\n> + echo File doesnotexist doesn't exist.\n> File doesnotexist doesn't exist.\n> + false\n> error: last command exited with $?=1\n> not ok 1 - check files\n> \n> But by just using \"test -d/-e\": the much shorter:\n> \n> + test -d .git\n> + test -f doesnotexist\n> error: last command exited with $?=1\n> not ok 1 - check files\n> \n> So I wonder if these days we shouldn't do this the other way around and\n> get rid of these. Every test_* wrapper we add adds a bit of cognitive\n> overload when you have to remember Git's specific shellscript dialect.\n\nI don't have a strong opinion, but I do agree that with \"-x\" it's nicer\nwithout the wrappers. I typically re-run with just \"-v\" on a failure,\nand only turn to \"-x\" if the verbose output isn't helpful. However, with\nthe rise of multi-platform CI jobs which try to collect as much\ninformation as possible in the initial run, I do find myself looking at\n\"-x\" more often.\n\nAs Gábor notes, you can't run every script with \"-x\". But I find it's\npretty consistent these days (and totally so if you have a recent bash).\nI dunno. Maybe people on other platforms (who might not have bash) would\ncare more.\n\nI had a vague notion that there was some reason (portability?) that we\npreferred to have the wrappers. But as your patch shows, they really are\njust calling \"test\" and nothing else.\n\n> test_must_be_empty () {\n> +\t# We don't want to remove this as noted in ec10b018e7 (\"tests:\n> +\t# use 'test_must_be_empty' instead of '! test -s'\",\n> +\t# 2018-08-19)\n> \ttest_path_is_file \"$1\" &&\n> \tif test -s \"$1\"\n> \tthen\n\nYou'd still want it to become \"test -f\" though, right?\n\n-Peff\n"},{"id":"370255","messageId":"20190226174316.GD19606@sigill.intra.peff.net","threadId":"50594","inReplyTo":"20190226170400.GC19739@szeder.dev","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-26T17:43:17Z","receivedAt":"2019-02-26T17:43:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 26, 2019 at 06:04:00PM +0100, SZEDER Gábor wrote:\n\n> > Whereas:\n> > \n> > + test -f doesnotexist\n> > + echo File doesnotexist doesn't exist.\n> > File doesnotexist doesn't exist.\n> > + false\n> > error: last command exited with $?=1\n> > \n> > Gives me the same thing, but I have to read 5 lines instead of 2 that\n> > ultimately don't tell me any more (and a bit of \"huh, 'false' returned\n> > 1? Of course! Oh! It's faking things up and it's the 'echo' that\n> > matters...\").\n> \n> I didn't find this to be an issue, but because of functions like\n> 'test_seq' and 'test_must_fail' I've thought about suppressing '-x'\n> output for test helpers (haven't actually done anything about it,\n> though).\n\nI'd be curious how you'd do that. We can wrap the function and redirect\nits stderr, but you'd still get a crufty line invoking the inner\nfunction (plus the outer function). That's better than seeing the inner\ndetails, but not as nice as just seeing the outer function invocation.\n\nI don't think we can play games like the one we do in test_eval_(),\nbecause \"set -x\" will already be on.\n\n-Peff\n"},{"id":"370257","messageId":"86va1630g4.fsf@matthieu-moy.fr","threadId":"50594","inReplyTo":"20190226170400.GC19739@szeder.dev","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@matthieu-moy.fr","sentAt":"2019-02-26T17:48:43Z","receivedAt":"2019-02-26T17:48:48Z","isPatch":false,"sender":{"key":"matthieu.moy@matthieu-moy.fr","avatar":"https://gravatar.com/avatar/0d82caa87c23154bfb465283a9678b89180ce8b6e07e9da84f9ee111183c7fbc?d=mp&s=160"},"body":"SZEDER Gábor writes:\n\n> On Tue, Feb 26, 2019 at 05:10:30PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> However. I wonder in general if we've re-visited the utility of these\n>> wrappers and maybe other similar wrappers after -x was added.\n>\n>> But 4 years after this was added in a136f6d8ff (\"test-lib.sh: support -x\n>> option for shell-tracing\", 2014-10-10) we got -x, and then with \"-i -v -x\":\n>\n> '-x' tracing doesn't work in all test scripts, unless it is run with a\n> Bash version already supporting BASH_XTRACEFD, i.e. v4.1 or later.\n> Notably the default Bash shipped in macOS is somewhere around v3.2.\n\nAccording to http://www.tldp.org/LDP/abs/html/bashver4.html#AEN21183,\nbash 4.1 was released on May, 2010. Are you sure macOS is _that_ late?\n\nI also tried with dash, and -x seems to work fine too (I use \"works with\ndash\" as a heuristic for \"should word on any shell\", but it doesn't\nalways work).\n\nIf -x doesn't work in some setups, it may be a good reason to wait a bit\nbefore trashing test_path_is_*, but if it's clear enough that the vast\nmajority of platforms get -x, then why not trash these wrappers indeed.\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"370260","messageId":"20190226182407.GF19606@sigill.intra.peff.net","threadId":"50594","inReplyTo":"86va1630g4.fsf@matthieu-moy.fr","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-26T18:24:08Z","receivedAt":"2019-02-26T18:24:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 26, 2019 at 06:48:43PM +0100, Matthieu Moy wrote:\n\n> > '-x' tracing doesn't work in all test scripts, unless it is run with a\n> > Bash version already supporting BASH_XTRACEFD, i.e. v4.1 or later.\n> > Notably the default Bash shipped in macOS is somewhere around v3.2.\n> \n> According to http://www.tldp.org/LDP/abs/html/bashver4.html#AEN21183,\n> bash 4.1 was released on May, 2010. Are you sure macOS is _that_ late?\n\nIt's not \"late\", it's \"never\". Bash 4 switched to GPLv3.\n\n> I also tried with dash, and -x seems to work fine too (I use \"works with\n> dash\" as a heuristic for \"should word on any shell\", but it doesn't\n> always work).\n\nYes, \"-x\" works everywhere. The problem is scripts which capture the\nstderr of subshells or functions, which then get polluted by \"-x\"\noutput. You can fix that in two ways:\n\n 1. Use bash 4.1+, which works around that with BASH_XTRACEFD.\n\n 2. Don't do that. Gábor fixed most such instances already, except the\n ones in t1510. That one automatically disables \"-x\" tracing.\n\nSo I don't know what you tried exactly, but you should be able to\nsuccessfully run with \"-x\" on any script. Including t1510, but you\njust won't get tracing output then.\n\n> If -x doesn't work in some setups, it may be a good reason to wait a bit\n> before trashing test_path_is_*, but if it's clear enough that the vast\n> majority of platforms get -x, then why not trash these wrappers indeed.\n\nI do think it basically works everywhere these days.\n\n-Peff\n"},{"id":"370261","messageId":"CAL7ArXoau1ZfBsV9JaUDprwjSijyo6K5d9JyC1mdfc=KEvgJxw@mail.gmail.com","threadId":"50594","inReplyTo":"CAN0heSqSp-a0zUKT5EaGLBYnRtESTnu9GKWtGARz2kaOAhc1HQ@mail.gmail.com","subject":"Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-02-26T18:29:51Z","receivedAt":"2019-02-26T18:30:54Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Hi Martin\n\nOn Tue, Feb 26, 2019 at 10:01 PM Martin Ågren wrote:\n>\n> > - ! test -d submod &&\n> > + ! test_path_is_dir submod &&\n>\n> Now, here I wonder. This (and other changes like this) means that every\n> time the test passes, we see \"Directory submod doesn't exist.\", which is\n> perhaps not too irritating. But more importantly, when the test fails,\n> we don't get any hint. So a failure is just as silent and \"non-helpful\"\n> as before. I can think of a few approaches:\n\n>\n> 1 Teach `test_path_is_dir` and friends to handle \"!\" in a clever way, and\n> write these as `test_path_is_dir ! foo`. (We already have helpers\n> that do this, see, e.g., `test_i18ngrep`.)\n>\n\nYes, I also think that it should be corrected and I think this(1)\napproach is good as it resonates well with the existing code. I'll\nstart working on it and submit the patch as soon as possible.\n\nThanks\nRohit\n"},{"id":"370262","messageId":"CAL7ArXocrtCBpEoCM6_aSWcKgaVDwBADMZ9WEBzxpwOfHKuHGQ@mail.gmail.com","threadId":"50594","inReplyTo":"20190226163737.GB19739@szeder.dev","subject":"Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-02-26T18:40:14Z","receivedAt":"2019-02-26T18:41:16Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Hi SZEDER\n\nOn Tue, Feb 26, 2019 at 10:07 PM SZEDER Gábor wrote:\n>\n> We prefer to use imperative mode when talking about what a patch does,\n> as if the author were to give orders to the code base. So e.g.\n> instead of\n>\n> This patch will ...\n>\n> we would usually write something like this:\n>\n> Replace 'test -(d|f)' calls in t3600 with the corresponding helper\n> functions.\n>\n\n>\n> > test_expect_success 'Recursive with -r -f' '\n> > git rm -f -r frotz &&\n> > - ! test -f frotz/nitfol &&\n> > - ! test -d frotz\n> > + ! test_path_is_file frotz/nitfol &&\n> > + ! test_path_is_dir frotz\n> > '\n>\n> These should rather use the test_path_is_missing helper function.\n>\n> However, if the directory 'frotz' is missing, then surely\n> 'frotz/nitfol' could not possibly exist either, could it? I'm not\n> sure why this test (and a couple of others) checks both, and wonder\n> whether the redundant check for the file inside the supposedly\n> non-existing directory could be removed.\n>\n\nOkay! I'll scan through the file to check for redundancy like this and fix them.\n\n> Furthermore, there are a couple of place where the '!' is not in front\n> of the whole 'test' command but is given as an argument, e.g.:\n>\n> test ! -f file\n>\n> Please convert those cases as well.\n>\n\nI think since I'm modifying `test_path_is_{dir|file}` functions to\nhandle calls like `! test_path_is_dir` well as mentioned in this\nthread[1]. I think we should replace `! test` calls with `test !`, so\nthat the changes are in agreement with each other. What do you say?\n\nThanks for advice\nRohit\n\n[1]: https://public-inbox.org/git/CAN0heSqSp-a0zUKT5EaGLBYnRtESTnu9GKWtGARz2kaOAhc1HQ@mail.gmail.com/\n"},{"id":"370263","messageId":"20190226193912.GD19739@szeder.dev","threadId":"50594","inReplyTo":"20190226174316.GD19606@sigill.intra.peff.net","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-26T19:39:12Z","receivedAt":"2019-02-26T19:39:19Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Feb 26, 2019 at 12:43:17PM -0500, Jeff King wrote:\n> On Tue, Feb 26, 2019 at 06:04:00PM +0100, SZEDER Gábor wrote:\n> \n> > > Whereas:\n> > > \n> > > + test -f doesnotexist\n> > > + echo File doesnotexist doesn't exist.\n> > > File doesnotexist doesn't exist.\n> > > + false\n> > > error: last command exited with $?=1\n> > > \n> > > Gives me the same thing, but I have to read 5 lines instead of 2 that\n> > > ultimately don't tell me any more (and a bit of \"huh, 'false' returned\n> > > 1? Of course! Oh! It's faking things up and it's the 'echo' that\n> > > matters...\").\n> > \n> > I didn't find this to be an issue, but because of functions like\n> > 'test_seq' and 'test_must_fail' I've thought about suppressing '-x'\n> > output for test helpers (haven't actually done anything about it,\n> > though).\n> \n> I'd be curious how you'd do that.\n\nWell, I started replying with \"Dunno\" and explaining why I don't think\nthat it can be done with 'test_must_fail'... but then got a bit of a\nlightbulb moment. Now look at this:\n\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 80402a428f..16adcd54c9 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -664,7 +664,15 @@ list_contains () {\n # Currently recognized signal names are: sigpipe, success.\n # (Don't use 'success', use 'test_might_fail' instead.)\n \n+restore_tracing () {\n+\tif test -n \"$trace\"\n+\tthen\n+\t\tset -x\n+\tfi\n+} 2>/dev/null 4>/dev/null\n+\n test_must_fail () {\n+\t{ set +x ; } 2>/dev/null 4>/dev/null\n \tcase \"$1\" in\n \tok=*)\n \t\t_test_ok=${1#ok=}\n@@ -679,24 +687,29 @@ test_must_fail () {\n \tif test $exit_code -eq 0 && ! list_contains \"$_test_ok\" success\n \tthen\n \t\techo >&4 \"test_must_fail: command succeeded: $*\"\n+\t\trestore_tracing\n \t\treturn 1\n \telif test_match_signal 13 $exit_code && list_contains \"$_test_ok\" sigpipe\n \tthen\n+\t\trestore_tracing\n \t\treturn 0\n \telif test $exit_code -gt 129 && test $exit_code -le 192\n \tthen\n \t\techo >&4 \"test_must_fail: died by signal $(($exit_code - 128)): $*\"\n+\t\trestore_tracing\n \t\treturn 1\n \telif test $exit_code -eq 127\n \tthen\n \t\techo >&4 \"test_must_fail: command not found: $*\"\n+\t\trestore_tracing\n \t\treturn 1\n \telif test $exit_code -eq 126\n \tthen\n \t\techo >&4 \"test_must_fail: valgrind error: $*\"\n+\t\trestore_tracing\n \t\treturn 1\n \tfi\n-\treturn 0\n+\trestore_tracing\n } 7>&2 2>&4\n \n # Similar to test_must_fail, but tolerates success, too. This is\n\n\nYeah, it's a hassle, especially in a function with as many return\npaths as 'test_must_fail', but look at its output:\n\n + test_must_fail git rev-parse nope --\n fatal: bad revision 'nope'\n + test_must_fail git rev-parse HEAD --\n 48ab21c1a5972e0fa9d87da7c5da9982872b8db2\n test_must_fail: command succeeded: git rev-parse HEAD --\n + return 1\n error: last command exited with $?=1\n\nNot even the 'set +x' shows up in the trace output! Unfortunately,\nthat line is not particularly pleasing on the eyes, but I don't see\nany way around that...\n\nPerhaps we could even go one step further with this 'restore_tracing'\nhelper and add a parameter specifying its return code, so we could\nmake it the last command invoked in the test helper function, and then\neven that 'return 1' would disappear from the trace output.\nFurthermore, this would be helpful in those functions where the last\ncommand's return code is relevant, e.g: \n\n test_cmp() {\n { set +x ; } 2>/dev/null 4>/dev/null\n $GIT_TEST_CMP \"$@\"\n restore_tracing $?\n }\n\n\nThere are a couple of tricky cases:\n\n - Some test helper functions call other test helper functions, and\n in those cases tracing would be enabled upon returning from the\n inner helper function. This is not an issue with e.g.\n 'test_might_fail' or 'test_cmp_config', because the inner helper\n function is the last command anyway. However, there is\n 'test_must_be_empty', 'test_dir_is_empty', 'test_config',\n 'test_commit', etc. which call the other test helper functions\n right at the start or in the middle.\n\n - && chains in test helper functions; we must make sure that the\n tracing is restored even in case of a failure.\n\n\n\n"},{"id":"370264","messageId":"nycvar.QRO.7.76.6.1902262051080.41@tvgsbejvaqbjf.bet","threadId":"50594","inReplyTo":"CAL7ArXoau1ZfBsV9JaUDprwjSijyo6K5d9JyC1mdfc=KEvgJxw@mail.gmail.com","subject":"Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-26T19:52:01Z","receivedAt":"2019-02-26T19:52:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 26 Feb 2019, Rohit Ashiwal wrote:\n\n> Hi Martin\n> \n> On Tue, Feb 26, 2019 at 10:01 PM Martin Ågren wrote:\n> >\n> > > - ! test -d submod &&\n> > > + ! test_path_is_dir submod &&\n> >\n> > Now, here I wonder. This (and other changes like this) means that every\n> > time the test passes, we see \"Directory submod doesn't exist.\", which is\n> > perhaps not too irritating. But more importantly, when the test fails,\n> > we don't get any hint. So a failure is just as silent and \"non-helpful\"\n> > as before. I can think of a few approaches:\n> \n> >\n> > 1 Teach `test_path_is_dir` and friends to handle \"!\" in a clever way, and\n> > write these as `test_path_is_dir ! foo`. (We already have helpers\n> > that do this, see, e.g., `test_i18ngrep`.)\n> >\n> \n> Yes, I also think that it should be corrected and I think this(1)\n> approach is good as it resonates well with the existing code. I'll\n> start working on it and submit the patch as soon as possible.\n\nWe already have `test_path_is_missing`. Why not use that instead of `!\ntest -d` or `! test -f`?\n\nCiao,\nJohannes"},{"id":"370265","messageId":"nycvar.QRO.7.76.6.1902262055260.41@tvgsbejvaqbjf.bet","threadId":"50594","inReplyTo":"20190226173542.GC19606@sigill.intra.peff.net","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-26T19:58:43Z","receivedAt":"2019-02-26T19:59:12Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 26 Feb 2019, Jeff King wrote:\n\n> I had a vague notion that there was some reason (portability?) that we\n> preferred to have the wrappers. But as your patch shows, they really are\n> just calling \"test\" and nothing else.\n\nLet's also not forget about the fact that `test -f` is actually not all\nthat intuitive an interface. Whereas even somebody without training in\nsoftware development (let alone Unix shell scripting) understands the\nmeaning of\n\n\ttest_path_is_file this-file.txt\n\nAnd even for a trained eye, the trace of `test -f` is sometimes hard to\nread, as you do *not* see the exit code in the trace, so you have to guess\nfrom circumstantial evidence whether it failed or succeeded.\n\nCiao,\nDscho\n"},{"id":"370266","messageId":"CAL7ArXq37k2qmjLSB6DROq3K_wd0YaD9a-thNXgVwcxX7BEUMg@mail.gmail.com","threadId":"50594","inReplyTo":"nycvar.QRO.7.76.6.1902262051080.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-02-26T20:01:34Z","receivedAt":"2019-02-26T20:02:36Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Hey!\n\nOn Wed, Feb 27, 2019 at 1:22 AM Johannes Schindelin\n wrote:\n>\n> We already have `test_path_is_missing`. Why not use that instead of `!\n> test -d` or `! test -f`?\n>\n\nYes, I think this is better. It will satisfy all the requirements I guess.\n\nCiao\nRohit\n"},{"id":"370267","messageId":"nycvar.QRO.7.76.6.1902262101180.41@tvgsbejvaqbjf.bet","threadId":"50594","inReplyTo":"CAL7ArXocrtCBpEoCM6_aSWcKgaVDwBADMZ9WEBzxpwOfHKuHGQ@mail.gmail.com","subject":"Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-26T20:02:46Z","receivedAt":"2019-02-26T20:03:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Rohit,\n\nOn Wed, 27 Feb 2019, Rohit Ashiwal wrote:\n\n> On Tue, Feb 26, 2019 at 10:07 PM SZEDER Gábor wrote:\n>\n> > Furthermore, there are a couple of place where the '!' is not in front\n> > of the whole 'test' command but is given as an argument, e.g.:\n> >\n> > test ! -f file\n> >\n> > Please convert those cases as well.\n> \n> I think since I'm modifying `test_path_is_{dir|file}` functions to\n> handle calls like `! test_path_is_dir` well as mentioned in this\n> thread[1]. I think we should replace `! test` calls with `test !`, so\n> that the changes are in agreement with each other. What do you say?\n\nI think what Gábor meant was that both `test ! -f file` and `! test -f\nfile` should be converted to `test_path_is_missing file`.\n\nCiao,\nJohannes"},{"id":"370268","messageId":"CAL7ArXp6YhpzXdbuXD3HmdUPC38AqBdAnVn5MRRNgjHgbHBvqA@mail.gmail.com","threadId":"50594","inReplyTo":"nycvar.QRO.7.76.6.1902262101180.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-02-26T20:05:15Z","receivedAt":"2019-02-26T20:06:17Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Hi Johannes\n\nOn Wed, Feb 27, 2019 at 1:33 AM Johannes Schindelin\n wrote:\n>\n> I think what Gábor meant was that both `test ! -f file` and `! test -f\n> file` should be converted to `test_path_is_missing file`.\n>\n\nI'll work over this and submit the patch.\n\nThanks for clarifying\nRohit\n"},{"id":"370275","messageId":"20190226210101.GA27914@sigill.intra.peff.net","threadId":"50594","inReplyTo":"20190226193912.GD19739@szeder.dev","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-26T21:01:01Z","receivedAt":"2019-02-26T21:01:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 26, 2019 at 08:39:12PM +0100, SZEDER Gábor wrote:\n\n> > > I didn't find this to be an issue, but because of functions like\n> > > 'test_seq' and 'test_must_fail' I've thought about suppressing '-x'\n> > > output for test helpers (haven't actually done anything about it,\n> > > though).\n> > \n> > I'd be curious how you'd do that.\n> \n> Well, I started replying with \"Dunno\" and explaining why I don't think\n> that it can be done with 'test_must_fail'... but then got a bit of a\n> lightbulb moment. Now look at this:\n> [...]\n> +\t{ set +x ; } 2>/dev/null 4>/dev/null\n\nAh, this is the magic. Doing:\n\n set +x 2>/dev/null\n\nwill still show it, but doing the redirection in a wrapping block means\nthat it is applied before the command inside the block is run. Clever.\n\nI think this braces trick could be used in general to fix all of the\nremaining \"you can't run this under -x\" cases, though it might be ugly.\nIt might also be possible to make test_eval_ a bit less subtle with it,\nthough I think it is relying on the braces already (which makes me\nwonder if I just totally forgot about its existence today, or if I\nearlier somehow stumbled onto a working recipe because I wanted to run\nmultiple redirected commands).\n\n> There are a couple of tricky cases:\n> \n> - Some test helper functions call other test helper functions, and\n> in those cases tracing would be enabled upon returning from the\n> inner helper function. This is not an issue with e.g.\n> 'test_might_fail' or 'test_cmp_config', because the inner helper\n> function is the last command anyway. However, there is\n> 'test_must_be_empty', 'test_dir_is_empty', 'test_config',\n> 'test_commit', etc. which call the other test helper functions\n> right at the start or in the middle.\n\nYeah, this is inherently a global flag that we're playing games with. It\ndoes seem like it would be easy to get it wrong. I guess the right model\nis considering it like a stack, like:\n\n-- >8 --\n#!/bin/sh\n\nx_counter=0\npop_x() {\n\tret=$?\n\tcase \"$x_counter\" in\n\t0)\n\t\techo >&2 \"BUG: too many pops\"\n\t\texit 1\n\t\t;;\n\t1)\n\t\tx_counter=0\n\t\tset -x\n\t\t;;\n\t*)\n\t\tx_counter=$((x_counter - 1))\n\t\t;;\n\tesac\n\t{ return $ret; } 2>/dev/null\n}\n\n# you _must_ call this as \"{ push_x; } 2>/dev/null\" to avoid polluting\n# trace output with the push call\npush_x() {\n\tset +x 2>/dev/null\n\tx_counter=$((x_counter + 1))\n}\n\nbar() {\n\t{ push_x; } 2>/dev/null\n\techo in bar\n\tpop_x\n}\n\nfoo() {\n\t{ push_x; } 2>/dev/null\n\techo in foo, before bar\n\tbar\n\techo in foo, after bar\n\tfalse\n\tpop_x\n}\n\nset -x\nfoo\necho \\$? is $?\n-- 8< --\n\nI wish there was a way to avoid having to do the block-and-redirect in\nthe push_x calls in each function, though.\n\nI dunno. I do like the output, but this is rapidly getting complex.\n\n> - && chains in test helper functions; we must make sure that the\n> tracing is restored even in case of a failure.\n\nYeah, there is no \"goto out\" to help give a common exit point from the\nfunction. You could probably do it with a wrapper, like:\n\n foo() {\n\t{ push_x; } 2>/dev/null\n\treal_foo \"$@\"\n\tpop_x\n }\n\nand then real_foo() is free to return however it likes. I wonder if you\ncould even wrap that up in a helper:\n\n disable_function_tracing () {\n\t# rename foo() to orig_foo(); this works in bash, but I'm not\n\t# sure if there's a portable way to do it (and ideally one that\n\t# wouldn't involve an extra process).\n\teval \"real_$1 () $(declare -f $1 | tail -n +2)\"\n\n\t# and then install a wrapper which pushes/pops tracing\n\teval \"$1 () { { push_x; } 2>/dev/null; real_$1 \\\"\\$@\\\"; pop_x; }\"\n }\n\n foo () { .... }\n disable_function_tracing foo\n\nIt would be easier if you could just declare the function body as an\nargument (and then it would be \"declare_untraceable_function\", where you\ndo it all in one step). But then the function body has to be in single\nquotes, which is a pain. I think this is definitely pushing the limits\nof portable shell (and quite possibly the limits of good taste).\n\n-Peff\n"},{"id":"370276","messageId":"20190226210229.GB27914@sigill.intra.peff.net","threadId":"50594","inReplyTo":"nycvar.QRO.7.76.6.1902262055260.41@tvgsbejvaqbjf.bet","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-26T21:02:30Z","receivedAt":"2019-02-26T21:02:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 26, 2019 at 08:58:43PM +0100, Johannes Schindelin wrote:\n\n> On Tue, 26 Feb 2019, Jeff King wrote:\n> \n> > I had a vague notion that there was some reason (portability?) that we\n> > preferred to have the wrappers. But as your patch shows, they really are\n> > just calling \"test\" and nothing else.\n> \n> Let's also not forget about the fact that `test -f` is actually not all\n> that intuitive an interface. Whereas even somebody without training in\n> software development (let alone Unix shell scripting) understands the\n> meaning of\n> \n> \ttest_path_is_file this-file.txt\n> \n> And even for a trained eye, the trace of `test -f` is sometimes hard to\n> read, as you do *not* see the exit code in the trace, so you have to guess\n> from circumstantial evidence whether it failed or succeeded.\n\nTrue. For old-timers, I think \"test -f\" is idiomatic, but that is not\ntrue for everyone. Sometimes wrappers can cut both ways, making things\nharder for people who are used to the idioms. But \"test_path_is_file\"\nshould be pretty readable for everyone, old and new alike, I would\nthink.\n\n-Peff\n"},{"id":"370282","messageId":"pull.152.v3.git.gitgitgadget@gmail.com","threadId":"50594","inReplyTo":"pull.152.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v3 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions","fromName":"Rohit Ashiwal via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-26T22:48:55Z","receivedAt":"2019-02-26T22:49:00Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Replace test -(d|f|e) calls in t3600-rm.sh. Previously we were using test\n-(d|f|e) to verify the presence of a directory/file, but we already have\nhelper functions, viz, test_path_is_dir, test_path_is_file and\ntest_path_is_missing with better functionality.\n\nRohit Ashiwal (1):\n t3600: use test_path_is_* functions\n\n t/t3600-rm.sh | 138 +++++++++++++++++++++++++-------------------------\n 1 file changed, 69 insertions(+), 69 deletions(-)\n\n\nbase-commit: 8104ec994ea3849a968b4667d072fedd1e688642\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-152%2Fr1walz%2Frefactor-tests-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-152/r1walz/refactor-tests-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/152\n\nRange-diff vs v2:\n\n 1: fcafc87b38 ! 1: bfeba25c88 t3600: use test_path_is_dir and test_path_is_file\n @@ -1,12 +1,14 @@\n Author: Rohit Ashiwal \n \n - t3600: use test_path_is_dir and test_path_is_file\n + t3600: use test_path_is_* functions\n \n - Previously we were using `test -(d|f)` to verify\n + Replace `test -(d|f|e)` calls in t3600-rm.sh\n +\n + Previously we were using `test -(d|f|e)` to verify\n the presence of a directory/file, but we already\n - have helper functions, viz, `test_path_is_dir`\n - and `test_path_is_file` with better functionality.\n - This patch will replace `test -(d|f)` calls in t3660.sh\n + have helper functions, viz, `test_path_is_dir`,\n + `test_path_is_file` and `test_path_is_missing`\n + with better functionality.\n \n These helper functions make code more readable\n and informative to someone new to code, also\n @@ -18,6 +20,15 @@\n --- a/t/t3600-rm.sh\n +++ b/t/t3600-rm.sh\n @@\n + \n + test_expect_success \\\n + 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' \\\n +- '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'\n ++ 'test_path_is_missing bar && test_must_fail git ls-files --error-unmatch bar'\n + \n + test_expect_success \\\n + 'Test that \"git rm -- -q\" succeeds (remove a file that looks like an option)' \\\n +@@\n test_expect_success 'Modify foo -- rm should refuse' '\n \techo >>foo &&\n \ttest_must_fail git rm foo baz &&\n @@ -28,6 +39,15 @@\n \tgit ls-files --error-unmatch foo baz\n '\n \n + test_expect_success 'Modified foo -- rm -f should work' '\n + \tgit rm -f foo baz &&\n +-\ttest ! -f foo &&\n +-\ttest ! -f baz &&\n ++\ttest_path_is_missing foo &&\n ++\ttest_path_is_missing baz &&\n + \ttest_must_fail git ls-files --error-unmatch foo &&\n + \ttest_must_fail git ls-files --error-unmatch bar\n + '\n @@\n \n test_expect_success 'foo is different in index from HEAD -- rm should refuse' '\n @@ -39,6 +59,15 @@\n \tgit ls-files --error-unmatch foo baz\n '\n \n + test_expect_success 'but with -f it should work.' '\n + \tgit rm -f foo baz &&\n +-\ttest ! -f foo &&\n +-\ttest ! -f baz &&\n ++\ttest_path_is_missing foo &&\n ++\ttest_path_is_missing baz &&\n + \ttest_must_fail git ls-files --error-unmatch foo &&\n + \ttest_must_fail git ls-files --error-unmatch baz\n + '\n @@\n \n test_expect_success 'Recursive without -r fails' '\n @@ -62,20 +91,56 @@\n \tgit rm -f -r frotz &&\n -\t! test -f frotz/nitfol &&\n -\t! test -d frotz\n -+\t! test_path_is_file frotz/nitfol &&\n -+\t! test_path_is_dir frotz\n ++\ttest_path_is_missing frotz/nitfol &&\n ++\ttest_path_is_missing frotz\n '\n \n test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n +@@\n + \tgit reset --hard &&\n + \ttest-tool chmtime -86400 frotz/nitfol &&\n + \tgit rm frotz/nitfol &&\n +-\ttest ! -f frotz/nitfol\n ++\ttest_path_is_missing frotz/nitfol\n + \n + '\n + \n @@\n \techo content >dir/subdir/subsubdir/file &&\n \tgit add dir/subdir/subsubdir/file &&\n \tgit rm -f dir/subdir/subsubdir/file &&\n -\t! test -d dir\n -+\t! test_path_is_dir dir\n ++\ttest_path_is_missing dir\n '\n \n cat >expect <actual &&\n + \ttest_cmp expect actual &&\n + \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n +@@\n + \tgit reset --hard &&\n + \tgit submodule update &&\n + \tgit rm submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual &&\n + \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n +@@\n + \tgit reset --hard &&\n + \tgit submodule update &&\n + \tgit rm submod/ &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual\n + '\n @@\n \tgit submodule update &&\n \tgit -C submod checkout HEAD^ &&\n @@ -87,6 +152,11 @@\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified actual &&\n \tgit rm -f submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual &&\n + \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n @@\n \tgit reset --hard &&\n \tgit submodule update &&\n @@ -113,8 +183,8 @@\n \ttest_must_be_empty actual.err &&\n -\t! test -d submod &&\n -\t! test -f submod/.git &&\n -+\t! test_path_is_dir submod &&\n -+\t! test_path_is_file submod/.git &&\n ++\ttest_path_is_missing submod &&\n ++\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.both_deleted actual\n '\n @@ -132,8 +202,8 @@\n \ttest_must_be_empty actual.err &&\n -\t! test -d submod &&\n -\t! test -f submod/.git &&\n -+\t! test_path_is_dir submod &&\n -+\t! test_path_is_file submod/.git &&\n ++\ttest_path_is_missing submod &&\n ++\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n @@ -143,8 +213,8 @@\n \ttest_i18ncmp expect.err actual.err &&\n -\t! test -d submod &&\n -\t! test -f submod/.git &&\n -+\t! test_path_is_dir submod &&\n -+\t! test_path_is_file submod/.git &&\n ++\ttest_path_is_missing submod &&\n ++\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n @@ -159,6 +229,11 @@\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual\n + '\n @@\n \tgit submodule update &&\n \techo X >submod/untracked &&\n @@ -170,6 +245,20 @@\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual\n + '\n +@@\n + \tgit submodule update &&\n + \ttest_must_fail git merge conflict2 &&\n + \tgit rm submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual\n + '\n @@\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git merge conflict2 &&\n @@ -181,6 +270,11 @@\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual &&\n + \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n @@\n \techo X >submod/empty &&\n \ttest_must_fail git merge conflict2 &&\n @@ -192,6 +286,11 @@\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual &&\n + \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n @@\n \techo X >submod/untracked &&\n \ttest_must_fail git merge conflict2 &&\n @@ -203,6 +302,11 @@\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual\n + '\n @@\n \t) &&\n \ttest_must_fail git merge conflict2 &&\n @@ -221,18 +325,36 @@\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit merge --abort &&\n +@@\n + \tgit reset --hard &&\n + \ttest_must_fail git merge conflict2 &&\n + \tgit rm submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual\n + '\n @@\n \t\trm -r ../.git/modules/sub\n \t) &&\n \tgit rm submod 2>output.err &&\n -\t! test -d submod &&\n -\t! test -d submod/.git &&\n -+\t! test_path_is_dir submod &&\n -+\t! test_path_is_dir submod/.git &&\n ++\ttest_path_is_missing submod &&\n ++\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest -s actual &&\n \ttest_i18ngrep Migrating output.err\n @@\n + \n + test_expect_success 'rm recursively removes work tree of unmodified submodules' '\n + \tgit rm submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual\n + '\n +@@\n \tgit submodule update --recursive &&\n \tgit -C submod/subsubmod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n @@ -243,6 +365,11 @@\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual\n + '\n @@\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/empty &&\n @@ -254,6 +381,11 @@\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual\n + '\n @@\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/untracked &&\n @@ -265,14 +397,19 @@\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n +-\ttest ! -d submod &&\n ++\ttest_path_is_missing submod &&\n + \tgit status -s -uno --ignore-submodules=none >actual &&\n + \ttest_cmp expect actual\n + '\n @@\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \tgit rm submod 2>output.err &&\n -\t! test -d submod &&\n -\t! test -d submod/subsubmod/.git &&\n -+\t! test_path_is_dir submod &&\n -+\t! test_path_is_dir submod/subsubmod/.git &&\n ++\ttest_path_is_missing submod &&\n ++\ttest_path_is_missing submod/subsubmod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest -s actual &&\n \ttest_i18ngrep Migrating output.err\n\n-- \ngitgitgadget\n"},{"id":"370283","messageId":"bfeba25c885484093bf8307294935f8baa6b155b.1551221334.git.gitgitgadget@gmail.com","threadId":"50594","inReplyTo":"pull.152.v3.git.gitgitgadget@gmail.com","subject":"[PATCH v3 1/1] t3600: use test_path_is_* functions","fromName":"Rohit Ashiwal via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-26T22:48:55Z","receivedAt":"2019-02-26T22:49:01Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"From: Rohit Ashiwal \n\nReplace `test -(d|f|e)` calls in t3600-rm.sh\n\nPreviously we were using `test -(d|f|e)` to verify\nthe presence of a directory/file, but we already\nhave helper functions, viz, `test_path_is_dir`,\n`test_path_is_file` and `test_path_is_missing`\nwith better functionality.\n\nThese helper functions make code more readable\nand informative to someone new to code, also\nthese functions have better error messages\n\nSigned-off-by: Rohit Ashiwal \n---\n t/t3600-rm.sh | 138 +++++++++++++++++++++++++-------------------------\n 1 file changed, 69 insertions(+), 69 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 04e5d42bd3..ad638490ac 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -83,7 +83,7 @@ test_expect_success \\\n \n test_expect_success \\\n 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' \\\n- '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'\n+ 'test_path_is_missing bar && test_must_fail git ls-files --error-unmatch bar'\n \n test_expect_success \\\n 'Test that \"git rm -- -q\" succeeds (remove a file that looks like an option)' \\\n@@ -137,15 +137,15 @@ test_expect_success 'Re-add foo and baz' '\n test_expect_success 'Modify foo -- rm should refuse' '\n \techo >>foo &&\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n test_expect_success 'Modified foo -- rm -f should work' '\n \tgit rm -f foo baz &&\n-\ttest ! -f foo &&\n-\ttest ! -f baz &&\n+\ttest_path_is_missing foo &&\n+\ttest_path_is_missing baz &&\n \ttest_must_fail git ls-files --error-unmatch foo &&\n \ttest_must_fail git ls-files --error-unmatch bar\n '\n@@ -159,15 +159,15 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '\n \n test_expect_success 'foo is different in index from HEAD -- rm should refuse' '\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n test_expect_success 'but with -f it should work.' '\n \tgit rm -f foo baz &&\n-\ttest ! -f foo &&\n-\ttest ! -f baz &&\n+\ttest_path_is_missing foo &&\n+\ttest_path_is_missing baz &&\n \ttest_must_fail git ls-files --error-unmatch foo &&\n \ttest_must_fail git ls-files --error-unmatch baz\n '\n@@ -194,21 +194,21 @@ test_expect_success 'Recursive test setup' '\n \n test_expect_success 'Recursive without -r fails' '\n \ttest_must_fail git rm frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r but dirty' '\n \techo qfwfq >>frotz/nitfol &&\n \ttest_must_fail git rm -r frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r -f' '\n \tgit rm -f -r frotz &&\n-\t! test -f frotz/nitfol &&\n-\t! test -d frotz\n+\ttest_path_is_missing frotz/nitfol &&\n+\ttest_path_is_missing frotz\n '\n \n test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n@@ -232,7 +232,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '\n \tgit reset --hard &&\n \ttest-tool chmtime -86400 frotz/nitfol &&\n \tgit rm frotz/nitfol &&\n-\ttest ! -f frotz/nitfol\n+\ttest_path_is_missing frotz/nitfol\n \n '\n \n@@ -254,7 +254,7 @@ test_expect_success 'rm removes subdirectories recursively' '\n \techo content >dir/subdir/subsubdir/file &&\n \tgit add dir/subdir/subsubdir/file &&\n \tgit rm -f dir/subdir/subsubdir/file &&\n-\t! test -d dir\n+\ttest_path_is_missing dir\n '\n \n cat >expect <actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -314,7 +314,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -325,7 +325,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm submod/ &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -343,12 +343,12 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles\n \tgit submodule update &&\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -359,8 +359,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm --cached submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.cached actual &&\n \tgit config -f .gitmodules submodule.sub.url &&\n@@ -371,7 +371,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm -n submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-index --exit-code HEAD\n '\n \n@@ -381,8 +381,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '\n \tgit rm .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.both_deleted actual\n '\n@@ -393,14 +393,14 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta\n \tgit config -f .gitmodules foo.bar true &&\n \ttest_must_fail git rm submod >actual 2>actual.err &&\n \ttest -s actual.err &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-files --quiet -- submod &&\n \tgit add .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -413,8 +413,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule\n \techo \"warning: Could not find section in .gitmodules where path=submod\" >expect.err &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_i18ncmp expect.err actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -424,12 +424,12 @@ test_expect_success 'rm of a populated submodule with modifications fails unless\n \tgit submodule update &&\n \techo X >submod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -439,12 +439,12 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \tgit submodule update &&\n \techo X >submod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -481,7 +481,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '\n \tgit submodule update &&\n \ttest_must_fail git merge conflict2 &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -493,12 +493,12 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -512,12 +512,12 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f\n \techo X >submod/empty &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -531,12 +531,12 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files\n \techo X >submod/untracked &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -552,13 +552,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director\n \t) &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \ttest_must_fail git rm -f submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit merge --abort &&\n@@ -570,7 +570,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '\n \tgit reset --hard &&\n \ttest_must_fail git merge conflict2 &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -586,8 +586,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates\n \t\trm -r ../.git/modules/sub\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest -s actual &&\n \ttest_i18ngrep Migrating output.err\n@@ -614,7 +614,7 @@ test_expect_success 'setup subsubmodule' '\n \n test_expect_success 'rm recursively removes work tree of unmodified submodules' '\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -624,12 +624,12 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \tgit submodule update --recursive &&\n \tgit -C submod/subsubmod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -639,12 +639,12 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -654,12 +654,12 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -673,8 +673,8 @@ test_expect_success \"rm absorbs submodule's nested .git directory\" '\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/subsubmod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/subsubmod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest -s actual &&\n \ttest_i18ngrep Migrating output.err\n-- \ngitgitgadget\n"},{"id":"370288","messageId":"CAN0heSpebSEMo_OFrVFRXssVqRwnq8m=_b9E6b1oj1GNiPrqDw@mail.gmail.com","threadId":"50594","inReplyTo":"CAL7ArXq37k2qmjLSB6DROq3K_wd0YaD9a-thNXgVwcxX7BEUMg@mail.gmail.com","subject":"Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-02-27T05:49:41Z","receivedAt":"2019-02-27T05:49:57Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 26 Feb 2019 at 21:02, Rohit Ashiwal wrote:\n>\n> Hey!\n>\n> On Wed, Feb 27, 2019 at 1:22 AM Johannes Schindelin\n> wrote:\n> >\n> > We already have `test_path_is_missing`. Why not use that instead of `!\n> > test -d` or `! test -f`?\n> >\n>\n> Yes, I think this is better. It will satisfy all the requirements I guess.\n\nGood suggestion, Johannes. That is probably what most (all) of these\nwanted to express.\n\nMartin\n"},{"id":"370292","messageId":"CACsJy8DjYUn+45E04gPjXhN0xqqjeyf8XoQsR8PyLefFrO4RGQ@mail.gmail.com","threadId":"50594","inReplyTo":"87sgwav8cp.fsf@evledraar.gmail.com","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-27T10:01:58Z","receivedAt":"2019-02-27T10:02:28Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 26, 2019 at 11:10 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Tue, Feb 26 2019, Duy Nguyen wrote:\n>\n> > On Tue, Feb 26, 2019 at 8:42 PM Rohit Ashiwal via GitGitGadget\n> > wrote:\n> >>\n> >> From: Rohit Ashiwal \n> >>\n> >> t3600-rm.sh: Previously we were using `test -(d|f)`\n> >> to verify the presencee of a directory/file, but we\n> >> already have helper functions, viz, test_path_is_dir\n> >> and test_path_is_file with same functionality. This\n> >\n> > It's not just the same (no point replacing then). It's better. When\n> > test_path_is_xxx fails, you get an error message. If \"test -xxx\"\n> > fails, you get a failed test with no clue what caused it.\n>\n> I swear I'm not just on a mission to ruin everyone's GSOC projects. This\n> patch definitely looks good, and given that we have this / document it\n> makes sense.\n>\n> However. I wonder in general if we've re-visited the utility of these\n> wrappers and maybe other similar wrappers after -x was added.\n\nIt's personal, but every time I have to use -x I curse a little. It's\njust often too much to read.\n\nBesides what people have already said, there's another good potential\nfor test_path_is_file and friends. You can make it support multiple\narguments, so that you can check if many paths are file with just one\nline. Used properly, this could reduce repetition and shorten some\ntest cases a bit without sacrificing readability.\n-- \nDuy\n"},{"id":"370293","messageId":"CACsJy8BYeLvB7BSM_Jt4vwfGsEBuhaCZfzGPOHe=B=7cvnRwrg@mail.gmail.com","threadId":"50594","inReplyTo":"bfeba25c885484093bf8307294935f8baa6b155b.1551221334.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/1] t3600: use test_path_is_* functions","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-27T10:12:24Z","receivedAt":"2019-02-27T10:12:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 27, 2019 at 5:49 AM Rohit Ashiwal via GitGitGadget\n wrote:\n>\n> From: Rohit Ashiwal \n>\n> Replace `test -(d|f|e)` calls in t3600-rm.sh\n>\n> Previously we were using `test -(d|f|e)` to verify\n> the presence of a directory/file, but we already\n> have helper functions, viz, `test_path_is_dir`,\n> `test_path_is_file` and `test_path_is_missing`\n> with better functionality.\n>\n> These helper functions make code more readable\n> and informative to someone new to code, also\n> these functions have better error messages\n>\n> Signed-off-by: Rohit Ashiwal \n> ---\n> t/t3600-rm.sh | 138 +++++++++++++++++++++++++-------------------------\n> 1 file changed, 69 insertions(+), 69 deletions(-)\n>\n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> index 04e5d42bd3..ad638490ac 100755\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n> @@ -83,7 +83,7 @@ test_expect_success \\\n>\n> test_expect_success \\\n> 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' \\\n> - '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'\n> + 'test_path_is_missing bar && test_must_fail git ls-files --error-unmatch bar'\n\nThis line should be broken down in two. It was reasonably short\nbefore, but now getting long and two checks in one line seem easy to\nmiss.\n\nI was a bit worried that the \"test ! something\" could be incorrectly\nconverted because for example, \"test ! -d foo\" is not always the same\nas \"test_path_is_missing\". If \"foo\" is intended to be a file, then the\nconversion is wrong.\n\nBut I don't think you made any wrong conversion here. All these\nnegative \"test\" are preceded by \"git rm\" so the expectation is always\n\"test ! -e\".\n-- \nDuy\n"},{"id":"370359","messageId":"pull.152.v4.git.gitgitgadget@gmail.com","threadId":"50594","inReplyTo":"pull.152.v3.git.gitgitgadget@gmail.com","subject":"[PATCH v4 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions","fromName":"Rohit Ashiwal via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-28T10:26:48Z","receivedAt":"2019-02-28T10:26:52Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Replace test -(d|f|e) calls in t3600-rm.sh. Previously we were using test\n-(d|f|e) to verify the presence of a directory/file, but we already have\nhelper functions, viz, test_path_is_dir, test_path_is_file and\ntest_path_is_missing with better functionality.\n\nRohit Ashiwal (1):\n t3600: use test_path_is_* functions\n\n t/t3600-rm.sh | 160 ++++++++++++++++++++++++++------------------------\n 1 file changed, 84 insertions(+), 76 deletions(-)\n\n\nbase-commit: 8104ec994ea3849a968b4667d072fedd1e688642\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-152%2Fr1walz%2Frefactor-tests-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-152/r1walz/refactor-tests-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/152\n\nRange-diff vs v3:\n\n 1: bfeba25c88 ! 1: f881f01e4f t3600: use test_path_is_* functions\n @@ -20,11 +20,48 @@\n --- a/t/t3600-rm.sh\n +++ b/t/t3600-rm.sh\n @@\n + \"\n \n test_expect_success \\\n - 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' \\\n +- 'Pre-check that foo exists and is in index before git rm foo' \\\n +- '[ -f foo ] && git ls-files --error-unmatch foo'\n ++ 'Pre-check that foo exists and is in index before git rm foo' '\n ++\t test_path_is_file foo &&\n ++\t git ls-files --error-unmatch foo\n ++'\n + \n + test_expect_success \\\n + 'Test that git rm foo succeeds' \\\n +@@\n + git rm --cached -f foo'\n + \n + test_expect_success \\\n +- 'Post-check that foo exists but is not in index after git rm foo' \\\n +- '[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'\n ++ 'Post-check that foo exists but is not in index after git rm foo' '\n ++\t test_path_is_file foo &&\n ++\t test_must_fail git ls-files --error-unmatch foo\n ++'\n + \n + test_expect_success \\\n +- 'Pre-check that bar exists and is in index before \"git rm bar\"' \\\n +- '[ -f bar ] && git ls-files --error-unmatch bar'\n ++ 'Pre-check that bar exists and is in index before \"git rm bar\"' '\n ++\t test_path_is_file bar &&\n ++\t git ls-files --error-unmatch bar\n ++'\n + \n + test_expect_success \\\n + 'Test that \"git rm bar\" succeeds' \\\n + 'git rm bar'\n + \n + test_expect_success \\\n +- 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' \\\n - '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'\n -+ 'test_path_is_missing bar && test_must_fail git ls-files --error-unmatch bar'\n ++ 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' '\n ++\t test_path_is_missing bar &&\n ++\t test_must_fail git ls-files --error-unmatch bar\n ++'\n \n test_expect_success \\\n 'Test that \"git rm -- -q\" succeeds (remove a file that looks like an option)' \\\n\n-- \ngitgitgadget\n"},{"id":"370360","messageId":"f881f01e4f05c1c9ad7e35fea5fd7db2947427a1.1551349607.git.gitgitgadget@gmail.com","threadId":"50594","inReplyTo":"pull.152.v4.git.gitgitgadget@gmail.com","subject":"[PATCH v4 1/1] t3600: use test_path_is_* functions","fromName":"Rohit Ashiwal via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-28T10:26:49Z","receivedAt":"2019-02-28T10:26:54Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"From: Rohit Ashiwal \n\nReplace `test -(d|f|e)` calls in t3600-rm.sh\n\nPreviously we were using `test -(d|f|e)` to verify\nthe presence of a directory/file, but we already\nhave helper functions, viz, `test_path_is_dir`,\n`test_path_is_file` and `test_path_is_missing`\nwith better functionality.\n\nThese helper functions make code more readable\nand informative to someone new to code, also\nthese functions have better error messages\n\nSigned-off-by: Rohit Ashiwal \n---\n t/t3600-rm.sh | 160 ++++++++++++++++++++++++++------------------------\n 1 file changed, 84 insertions(+), 76 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 04e5d42bd3..3a5bd97df7 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -27,8 +27,10 @@ embedded' &&\n \"\n \n test_expect_success \\\n- 'Pre-check that foo exists and is in index before git rm foo' \\\n- '[ -f foo ] && git ls-files --error-unmatch foo'\n+ 'Pre-check that foo exists and is in index before git rm foo' '\n+\t test_path_is_file foo &&\n+\t git ls-files --error-unmatch foo\n+'\n \n test_expect_success \\\n 'Test that git rm foo succeeds' \\\n@@ -70,20 +72,26 @@ test_expect_success \\\n git rm --cached -f foo'\n \n test_expect_success \\\n- 'Post-check that foo exists but is not in index after git rm foo' \\\n- '[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'\n+ 'Post-check that foo exists but is not in index after git rm foo' '\n+\t test_path_is_file foo &&\n+\t test_must_fail git ls-files --error-unmatch foo\n+'\n \n test_expect_success \\\n- 'Pre-check that bar exists and is in index before \"git rm bar\"' \\\n- '[ -f bar ] && git ls-files --error-unmatch bar'\n+ 'Pre-check that bar exists and is in index before \"git rm bar\"' '\n+\t test_path_is_file bar &&\n+\t git ls-files --error-unmatch bar\n+'\n \n test_expect_success \\\n 'Test that \"git rm bar\" succeeds' \\\n 'git rm bar'\n \n test_expect_success \\\n- 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' \\\n- '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'\n+ 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' '\n+\t test_path_is_missing bar &&\n+\t test_must_fail git ls-files --error-unmatch bar\n+'\n \n test_expect_success \\\n 'Test that \"git rm -- -q\" succeeds (remove a file that looks like an option)' \\\n@@ -137,15 +145,15 @@ test_expect_success 'Re-add foo and baz' '\n test_expect_success 'Modify foo -- rm should refuse' '\n \techo >>foo &&\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n test_expect_success 'Modified foo -- rm -f should work' '\n \tgit rm -f foo baz &&\n-\ttest ! -f foo &&\n-\ttest ! -f baz &&\n+\ttest_path_is_missing foo &&\n+\ttest_path_is_missing baz &&\n \ttest_must_fail git ls-files --error-unmatch foo &&\n \ttest_must_fail git ls-files --error-unmatch bar\n '\n@@ -159,15 +167,15 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '\n \n test_expect_success 'foo is different in index from HEAD -- rm should refuse' '\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n test_expect_success 'but with -f it should work.' '\n \tgit rm -f foo baz &&\n-\ttest ! -f foo &&\n-\ttest ! -f baz &&\n+\ttest_path_is_missing foo &&\n+\ttest_path_is_missing baz &&\n \ttest_must_fail git ls-files --error-unmatch foo &&\n \ttest_must_fail git ls-files --error-unmatch baz\n '\n@@ -194,21 +202,21 @@ test_expect_success 'Recursive test setup' '\n \n test_expect_success 'Recursive without -r fails' '\n \ttest_must_fail git rm frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r but dirty' '\n \techo qfwfq >>frotz/nitfol &&\n \ttest_must_fail git rm -r frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r -f' '\n \tgit rm -f -r frotz &&\n-\t! test -f frotz/nitfol &&\n-\t! test -d frotz\n+\ttest_path_is_missing frotz/nitfol &&\n+\ttest_path_is_missing frotz\n '\n \n test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n@@ -232,7 +240,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '\n \tgit reset --hard &&\n \ttest-tool chmtime -86400 frotz/nitfol &&\n \tgit rm frotz/nitfol &&\n-\ttest ! -f frotz/nitfol\n+\ttest_path_is_missing frotz/nitfol\n \n '\n \n@@ -254,7 +262,7 @@ test_expect_success 'rm removes subdirectories recursively' '\n \techo content >dir/subdir/subsubdir/file &&\n \tgit add dir/subdir/subsubdir/file &&\n \tgit rm -f dir/subdir/subsubdir/file &&\n-\t! test -d dir\n+\ttest_path_is_missing dir\n '\n \n cat >expect <actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -314,7 +322,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -325,7 +333,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm submod/ &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -343,12 +351,12 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles\n \tgit submodule update &&\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -359,8 +367,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm --cached submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.cached actual &&\n \tgit config -f .gitmodules submodule.sub.url &&\n@@ -371,7 +379,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm -n submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-index --exit-code HEAD\n '\n \n@@ -381,8 +389,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '\n \tgit rm .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.both_deleted actual\n '\n@@ -393,14 +401,14 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta\n \tgit config -f .gitmodules foo.bar true &&\n \ttest_must_fail git rm submod >actual 2>actual.err &&\n \ttest -s actual.err &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-files --quiet -- submod &&\n \tgit add .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -413,8 +421,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule\n \techo \"warning: Could not find section in .gitmodules where path=submod\" >expect.err &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_i18ncmp expect.err actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -424,12 +432,12 @@ test_expect_success 'rm of a populated submodule with modifications fails unless\n \tgit submodule update &&\n \techo X >submod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -439,12 +447,12 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \tgit submodule update &&\n \techo X >submod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -481,7 +489,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '\n \tgit submodule update &&\n \ttest_must_fail git merge conflict2 &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -493,12 +501,12 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -512,12 +520,12 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f\n \techo X >submod/empty &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -531,12 +539,12 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files\n \techo X >submod/untracked &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -552,13 +560,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director\n \t) &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \ttest_must_fail git rm -f submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit merge --abort &&\n@@ -570,7 +578,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '\n \tgit reset --hard &&\n \ttest_must_fail git merge conflict2 &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -586,8 +594,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates\n \t\trm -r ../.git/modules/sub\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest -s actual &&\n \ttest_i18ngrep Migrating output.err\n@@ -614,7 +622,7 @@ test_expect_success 'setup subsubmodule' '\n \n test_expect_success 'rm recursively removes work tree of unmodified submodules' '\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -624,12 +632,12 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \tgit submodule update --recursive &&\n \tgit -C submod/subsubmod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -639,12 +647,12 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -654,12 +662,12 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -673,8 +681,8 @@ test_expect_success \"rm absorbs submodule's nested .git directory\" '\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/subsubmod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/subsubmod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest -s actual &&\n \ttest_i18ngrep Migrating output.err\n-- \ngitgitgadget\n"},{"id":"370369","messageId":"20190228190242.20680-1-rohit.ashiwal265@gmail.com","threadId":"50594","inReplyTo":"f881f01e4f05c1c9ad7e35fea5fd7db2947427a1.1551349607.git.gitgitgadget@gmail.com","subject":"[GSoC] acknowledging mistakes","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-02-28T19:02:42Z","receivedAt":"2019-02-28T19:03:16Z","isPatch":false,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Hey people\n\nI had a discussion with Rafael over the #git irc channel and Thanks to\nhim I was able to find these minute mistakes:\n\n1. Commit message was less than 50 chars which should be around 72 chars\n according to coding guide lines. Should I change this to match 72?\n\n2. My changes had some uneven use of tabs and spaces, which I made\n considering that pre-existing code had them too. Is there a\n possibility to change the whole code according to CodingGuidelines?\n If yes should I only change my code according to guidelines or the\n whole file?\n\n3. There is no helper function for `test -s` but Rafael suggested we can\n make use of other helper functions to provide similar functionality,\n if we can.\n\nOpen to suggestions and debate. These will be fixed in next revision\naccordingly.\n\nThanks\nRohit\n\n"},{"id":"370401","messageId":"xmqqef7r9uil.fsf@gitster-ct.c.googlers.com","threadId":"50594","inReplyTo":"20190228190242.20680-1-rohit.ashiwal265@gmail.com","subject":"Re: [GSoC] acknowledging mistakes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-01T02:51:46Z","receivedAt":"2019-03-01T02:51:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rohit Ashiwal writes:\n\n> 1. Commit message was less than 50 chars which should be around 72 chars\n> according to coding guide lines. Should I change this to match 72?\n\nSimple things do not need that many letters to tell ;-) The\nsuggestion of 72 is about the maximum. \n\nIf you are doing something in a single patch that needs a longer\ntitle, it generally is a sign that you are trying to do too much in\na single patch and should be splitting the patch into more\ndigestable smaller steps. And the purpose of having a maximum is to\nnudge patch authors to realize that.\n\n> 2. My changes had some uneven use of tabs and spaces, which I made\n> considering that pre-existing code had them too. Is there a\n> possibility to change the whole code according to CodingGuidelines?\n> If yes should I only change my code according to guidelines or the\n> whole file?\n\nI think you are talking about t3600, which uses an ancient style.\nIf this were a real project, then the preferred order would be\n\n - A preliminary patch (or a series of patches) that modernizes\n existing tests in t3600. Just style updates and adding or\n removing nothing else.\n\n - Update test that use \"test -f\" and friends to use the helpers in\n t3600.\n\n> 3. There is no helper function for `test -s` but Rafael suggested we can\n> make use of other helper functions to provide similar functionality,\n> if we can.\n\nIf we often see if a path is an non-empty file in our tests (not\nlimited to t3600), then it may make sense to add a new helper\ntest_path_is_non_empty_file in t/test-lib-functions.sh next to where\ntest_path_is_file and friends are defined.\n\nThanks.\n\n[jch: I am still mostly offline til the next week, but I had a\nchance to sit in front of my mailbox long enough, so...]\n"},{"id":"370403","messageId":"xmqqzhqf8fw5.fsf@gitster-ct.c.googlers.com","threadId":"50594","inReplyTo":"87sgwav8cp.fsf@evledraar.gmail.com","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-01T02:52:58Z","receivedAt":"2019-03-01T02:53:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I swear I'm not just on a mission to ruin everyone's GSOC projects. This\n> patch definitely looks good, and given that we have this / document it\n> makes sense.\n>\n> However. I wonder in general if we've re-visited the utility of these\n> wrappers and maybe other similar wrappers after -x was added.\n>\n> Back when this was added in 2caf20c52b (\"test-lib: user-friendly\n> alternatives to test [-d|-f|-e]\", 2010-08-10) we didn't have -x.\n> ...\n> But 4 years after this was added in a136f6d8ff (\"test-lib.sh: support -x\n> option for shell-tracing\", 2014-10-10) we got -x, and then with \"-i -v -x\":\n\nI think two things need to be considered separately.\n\n - Do the path-is-file and friends make the test source easier to\n read and undrstand? Special bonus if it helps us by making it\n harder to write a wrong test.\n\n - Do these helpers make the output from the test execution easier\n to diagnose or harder?\n\nIf your primary compalint is the latter (which I think it is, and I\nshare the same feeling to a certain degree), I think it is to throw\nthe baby with bathwater to get rid of path-is-* family.\n\nAnd as to the former question, I think we even are getting special\nbonus. Often when people write tests to ensure a fix that left an\nunwanted file behind would say \"! test -f unwanted\", but if we say\n\"path-is-missing unwanted\" that would catch not just a regular file\nbut also catch other kinds of filesystem entities.\n\nAs to readablity, I do not think \"test -f/-d\" etc are unnecessary\nhard to read, but using path-is-* does not make it harder to read,\nso I'd say it would not give us much to revert to the bare \"test -f\"\nand friends.\n\nUnless you are after squeezing the last cycle spent executing a\nshell builtin in the test scripts by using bare-bones \"test -f\",\nthat is. But that is not among the two I said we need to consider\nseparately, so I won't go there.\n\nThanks.\n\n[jch: I am still mostly offline til the next week, but I had a\nchance to sit in front of my mailbox long enough, so...]\n"},{"id":"370413","messageId":"20190301131326.7898-1-rohit.ashiwal265@gmail.com","threadId":"50594","inReplyTo":"xmqqef7r9uil.fsf@gitster-ct.c.googlers.com","subject":"Feeling confused a little bit","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-01T13:13:26Z","receivedAt":"2019-03-01T13:14:33Z","isPatch":false,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Hey!\n\nI'm a little confused as you never provide a clear indication to\nwhere shall I proceed? :-\n\nOn Fri, 01 Mar 2019 11:51:46 +0900 Junio C Hamano wrote:\n\n>\n> Simple things do not need that many letters to tell ;-) The\n> suggestion of 72 is about the maximum. \n>\n\nTotally agree on this!\n\n>\n> I think you are talking about t3600, which uses an ancient style.\n> If this were a real project, then the preferred order would be\n>\n> - A preliminary patch (or a series of patches) that modernizes\n> existing tests in t3600. Just style updates and adding or\n> removing nothing else.\n>\n> - Update test that use \"test -f\" and friends to use the helpers in\n> t3600.\n>\n\nYes, this is a microproject after all. But I think I can work on this as\nif it were a real project, should I proceed according to this plan? (I have\na lot of free time over this weekend)\n\n>\n> If we often see if a path is an non-empty file in our tests (not\n> limited to t3600), then it may make sense to add a new helper\n> test_path_is_non_empty_file in t/test-lib-functions.sh next to where\n> test_path_is_file and friends are defined.\n>\n\nSince my project does not deal with `test-lib-functions.sh`, I think I\nshould not edit it anyway, but I'd be more than happy to add a new\nmember to `test_path_is_*` family.\n\nThanks\nRohit\n"},{"id":"370463","messageId":"20190302042414.GA24599@rigel","threadId":"50594","inReplyTo":"20190301131326.7898-1-rohit.ashiwal265@gmail.com","subject":"Re: Feeling confused a little bit","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2019-03-02T04:24:14Z","receivedAt":"2019-03-02T04:24:50Z","isPatch":false,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"On Fri, Mar 01, 2019 at 06:43:26PM +0530, Rohit Ashiwal wrote:\n> >\n> > Simple things do not need that many letters to tell ;-) The\n> > suggestion of 72 is about the maximum. \n> >\n> \n> Totally agree on this!\n>\n\nI was bikeshedding the patch and mentioned that the commit message body\nis usually wrapped at 72 because I noticed you were wrapping the body at\n50.\n\nSo to make things clear, when you're writing the subject, i.e. the first\nline, you should aim towards 50 and do not exceed 72.\n\nThe body, i.e. 3rd line until EOF is usually wrapped at 72.\n\nThere are exceptions, these are guidelines. Sometimes commits will break\nthe first rule. (Merges are the most common example I can think of, but\nyou won't be doing any as a contributor).\nPre-formatted content, like the output of a program, will break the\nsecond. Look at 3b41fb0cb217f4b4491f2e67ce4183e5d2a5d873 for an example.\n\nBut my nitpick wasn't necessarily because I didn't agree about the way\nyou line wrapped the patch. It was about figuring out if you had a\nmisconfigured editor (that could also be the cause of tabs and spaces\nmix), which you later mentioned was probably the case.\n\nCheers,\nRafael Ascensão\n"},{"id":"370467","messageId":"20190302144647.GT6085@hank.intra.tgummerer.com","threadId":"50594","inReplyTo":"20190301131326.7898-1-rohit.ashiwal265@gmail.com","subject":"Re: Feeling confused a little bit","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-02T14:46:47Z","receivedAt":"2019-03-02T14:46:57Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/01, Rohit Ashiwal wrote:\n> Hey!\n> \n> I'm a little confused as you never provide a clear indication to\n> where shall I proceed? :-\n> \n> On Fri, 01 Mar 2019 11:51:46 +0900 Junio C Hamano wrote:\n> > I think you are talking about t3600, which uses an ancient style.\n> > If this were a real project, then the preferred order would be\n> >\n> > - A preliminary patch (or a series of patches) that modernizes\n> > existing tests in t3600. Just style updates and adding or\n> > removing nothing else.\n> >\n> > - Update test that use \"test -f\" and friends to use the helpers in\n> > t3600.\n> >\n> \n> Yes, this is a microproject after all. But I think I can work on this as\n> if it were a real project, should I proceed according to this plan? (I have\n> a lot of free time over this weekend)\n\nYes, I think it would be good to make those changes, to try and get\nthis merged. Having the microproject merged is not a requirement (its\nmain purpose is to see how students communicate on the mailing list,\nand to get them familiar with the workflow ahead of GSoC), but it can\nbe a nice achievement in itself.\n\nThat said, I would also encourage you to start thinking about a\nproject proposal, as that is another important part that should be\ndone for the application.\n\n> >\n> > If we often see if a path is an non-empty file in our tests (not\n> > limited to t3600), then it may make sense to add a new helper\n> > test_path_is_non_empty_file in t/test-lib-functions.sh next to where\n> > test_path_is_file and friends are defined.\n> >\n> \n> Since my project does not deal with `test-lib-functions.sh`, I think I\n> should not edit it anyway, but I'd be more than happy to add a new\n> member to `test_path_is_*` family.\n\nIt is up to you how far you would like to go with this. If you want\nto add the helper, to make further cleanups in t3600, I think that\nwould be a good thing to do (after double checking that it would be\nuseful in other test files as well), and should be done in a separate\npatch. Then you can use it in the same patch as using the helpers for\n\"test -f\" etc.\n\n> Thanks\n> Rohit\n"},{"id":"370469","messageId":"20190302162109.11172-1-rohit.ashiwal265@gmail.com","threadId":"50594","inReplyTo":"20190302144647.GT6085@hank.intra.tgummerer.com","subject":"[GSoC] Thanking","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-02T16:21:09Z","receivedAt":"2019-03-02T16:21:47Z","isPatch":false,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Hey! Thomas\n\nThank you for replying over my woes.\n\n>\n> It is up to you how far you would like to go with this. If you want\n> to add the helper, to make further cleanups in t3600, I think that\n> would be a good thing to do (after double checking that it would be\n> useful in other test files as well), and should be done in a separate\n> patch. Then you can use it in the same patch as using the helpers for\n> \"test -f\" etc.\n>\n\nI guess I should work on this one first. I checked and around 18 test\nfiles use `test -s`, it will be useful nonetheless.\n\n>\n> Yes, I think it would be good to make those changes, to try and get\n> this merged. Having the microproject merged is not a requirement (its\n> main purpose is to see how students communicate on the mailing list,\n> and to get them familiar with the workflow ahead of GSoC), but it can\n> be a nice achievement in itself.\n>\n\nYes, it is a nice experience to interact with people who \"run\" git over\nwhich most of the open source community depends for code sharing and\ncollaboration.\n\n>\n> That said, I would also encourage you to start thinking about a\n> project proposal, as that is another important part that should be\n> done for the application.\n>\n\nThat is really encouraging, I'll try to finish my work as soon as\npossible and work on the proposal side by side!\n\nThanks\nRohit\n\n"},{"id":"370530","messageId":"20190303160459.GB28939@szeder.dev","threadId":"50594","inReplyTo":"20190226210101.GA27914@sigill.intra.peff.net","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-03T16:04:59Z","receivedAt":"2019-03-03T16:05:06Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Feb 26, 2019 at 04:01:01PM -0500, Jeff King wrote:\n> On Tue, Feb 26, 2019 at 08:39:12PM +0100, SZEDER Gábor wrote:\n> \n> > > > I didn't find this to be an issue, but because of functions like\n> > > > 'test_seq' and 'test_must_fail' I've thought about suppressing '-x'\n> > > > output for test helpers (haven't actually done anything about it,\n> > > > though).\n\n> > There are a couple of tricky cases:\n> > \n> > - Some test helper functions call other test helper functions, and\n> > in those cases tracing would be enabled upon returning from the\n> > inner helper function. This is not an issue with e.g.\n> > 'test_might_fail' or 'test_cmp_config', because the inner helper\n> > function is the last command anyway. However, there is\n> > 'test_must_be_empty', 'test_dir_is_empty', 'test_config',\n> > 'test_commit', etc. which call the other test helper functions\n> > right at the start or in the middle.\n> \n> Yeah, this is inherently a global flag that we're playing games with. It\n> does seem like it would be easy to get it wrong. I guess the right model\n> is considering it like a stack, like:\n> \n> -- >8 --\n> #!/bin/sh\n> \n> x_counter=0\n> pop_x() {\n> \tret=$?\n> \tcase \"$x_counter\" in\n> \t0)\n> \t\techo >&2 \"BUG: too many pops\"\n> \t\texit 1\n> \t\t;;\n> \t1)\n> \t\tx_counter=0\n> \t\tset -x\n> \t\t;;\n> \t*)\n> \t\tx_counter=$((x_counter - 1))\n> \t\t;;\n> \tesac\n> \t{ return $ret; } 2>/dev/null\n> }\n> \n> # you _must_ call this as \"{ push_x; } 2>/dev/null\" to avoid polluting\n> # trace output with the push call\n> push_x() {\n> \tset +x 2>/dev/null\n> \tx_counter=$((x_counter + 1))\n> }\n> \n> bar() {\n> \t{ push_x; } 2>/dev/null\n> \techo in bar\n> \tpop_x\n> }\n> \n> foo() {\n> \t{ push_x; } 2>/dev/null\n> \techo in foo, before bar\n> \tbar\n> \techo in foo, after bar\n> \tfalse\n> \tpop_x\n> }\n> \n> set -x\n> foo\n> echo \\$? is $?\n> -- 8< --\n> \n> I wish there was a way to avoid having to do the block-and-redirect in\n> the push_x calls in each function, though.\n> \n> I dunno. I do like the output, but this is rapidly getting complex.\n> \n> > - && chains in test helper functions; we must make sure that the\n> > tracing is restored even in case of a failure.\n\nActually, the && chain is not really an issue, because we can simply\nbreak the && chain at the very end:\n\n test_func () {\n { disable_tracing ; } 2>/dev/null 4>&2\n do this &&\n do that\n restore_tracing\n }\n\nand make restore_tracing exit with $? (like you did above in pop_x()).\n\n> Yeah, there is no \"goto out\" to help give a common exit point from the\n> function. You could probably do it with a wrapper, like:\n\nYeah, the wrapper works.\nThere are only a few test helper functions with multiple 'return'\nstatements, and refactoring them to have a single 'return $ret' at the\nend worked, too.\n\n> foo() {\n> \t{ push_x; } 2>/dev/null\n> \treal_foo \"$@\"\n> \tpop_x\n> }\n> \n> and then real_foo() is free to return however it likes. I wonder if you\n> could even wrap that up in a helper:\n> \n> disable_function_tracing () {\n> \t# rename foo() to orig_foo(); this works in bash, but I'm not\n> \t# sure if there's a portable way to do it (and ideally one that\n> \t# wouldn't involve an extra process).\n> \teval \"real_$1 () $(declare -f $1 | tail -n +2)\"\n> \n> \t# and then install a wrapper which pushes/pops tracing\n> \teval \"$1 () { { push_x; } 2>/dev/null; real_$1 \\\"\\$@\\\"; pop_x; }\"\n> }\n> \n> foo () { .... }\n> disable_function_tracing foo\n\nWe can wrap all functions at once:\n\n eval \"$(declare -f \\\n test_cmp \\\n test_cmp_bin \\\n <....> \\\n write_script |\n sed -e 's%^\\([a-zA-Z0-9_]*\\) ()% \\\n \\1 () { \\\n { disable_tracing; } 2>/dev/null 4>/dev/null \\\n real_\\1 \\\"\\$@\\\" \\\n restore_tracing \\\n } \\\n real_\\1 ()%')\"\n\nYeah, not particularly pretty, but with the s/// command broken up\ninto several lines it's not all that terrible either... And at least\nit doesn't need extra processes for each wrapped function.\n\nWe should also be careful and don't switch on tracing when returning\nfrom test helper functions invoked outside of tests, e.g.\n'test_create_repo' while initializing the trash directory or\n'test_set_port' while sourcing a daemon-specific lib.\n\nAlas, 'declare' is Bash-only, and I don't see any way around that.\nBummer.\n\n\nOn a mostly unrelated note, but I just noticed it while playing around\nwith this: 't0000'-basic.sh' runs its internal tests with $SHELL_PATH\ninstead of $TEST_SHELL_PATH. I'm not sure whether that's right or\nwrong.\n\n"},{"id":"370605","messageId":"20190304143633.GC28939@szeder.dev","threadId":"50594","inReplyTo":"20190226210101.GA27914@sigill.intra.peff.net","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-04T14:36:33Z","receivedAt":"2019-03-04T14:36:39Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Feb 26, 2019 at 04:01:01PM -0500, Jeff King wrote:\n> > +\t{ set +x ; } 2>/dev/null 4>/dev/null\n> \n> Ah, this is the magic. Doing:\n> \n> set +x 2>/dev/null\n> \n> will still show it, but doing the redirection in a wrapping block means\n> that it is applied before the command inside the block is run. Clever.\n\nYeah, clever, but unfortunately (and to me suprisingly) unportable:\n\n $ ksh\n $ set -x\n $ echo foo\n + echo foo\n foo\n $ set +x\n $ \n\nIt doesn't show 'set +x', how convenient! :)\nHowever:\n\n $ set -x\n $ echo foo 2>/dev/null\n + echo foo\n + 2> /dev/null\n foo\n $ { set +x; } 2>/dev/null\n + 2> /dev/null\n $ \n\nApparently ksh, ksh93 and mksh show not only the executed commands\nbut any redirections as well. It's already visible when running our\ntests with ksh and '-x':\n\n $ ksh ./t9999-test.sh -x\n Initialized empty Git repository in /home/szeder/src/git/t/trash directory.t9999-test/.git/\n expecting success: \n true\n \n + true\n + 2> /dev/null ok 1 - first\n \n # passed all 1 test(s)\n 1..1\n\nNetBSD's sh:\n\n # set -x\n # echo foo\n + echo foo\n foo\n # echo foo 2>/dev/null\n + echo foo 2>/dev/null\n foo\n # { set +x; } 2>/dev/null\n + using redirections: 2>/dev/null do\n\n\n"},{"id":"370665","messageId":"20190305045535.GI19800@sigill.intra.peff.net","threadId":"50594","inReplyTo":"20190303160459.GB28939@szeder.dev","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-05T04:55:35Z","receivedAt":"2019-03-05T04:55:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 03, 2019 at 05:04:59PM +0100, SZEDER Gábor wrote:\n\n> > > - && chains in test helper functions; we must make sure that the\n> > > tracing is restored even in case of a failure.\n> \n> Actually, the && chain is not really an issue, because we can simply\n> break the && chain at the very end:\n> \n> test_func () {\n> { disable_tracing ; } 2>/dev/null 4>&2\n> do this &&\n> do that\n> restore_tracing\n> }\n> \n> and make restore_tracing exit with $? (like you did above in pop_x()).\n\nYeah, good point.\n\n> > Yeah, there is no \"goto out\" to help give a common exit point from the\n> > function. You could probably do it with a wrapper, like:\n> \n> Yeah, the wrapper works.\n> There are only a few test helper functions with multiple 'return'\n> statements, and refactoring them to have a single 'return $ret' at the\n> end worked, too.\n\nYeah, that might be less sneaky than this wrapper business. Or we could\njust do a few basic wrappers. The non-portable bit in my wrapper\nsuggestion was the renaming of the old function. But if we accept just:\n\n real_foo() {\n\t... do stuff with multiple returns ...\n }\n disable_function_tracing real_foo foo\n\nthen that is pretty trivial to do with an eval. It does disallow your\n\"wrap all functions at once\", but I think that is OK. We might want to\nonly do a subset anyway.\n\n> We should also be careful and don't switch on tracing when returning\n> from test helper functions invoked outside of tests, e.g.\n> 'test_create_repo' while initializing the trash directory or\n> 'test_set_port' while sourcing a daemon-specific lib.\n\nYeah, it would probably make sense in the \"push\" half to check that we\nare actually tracing at that moment.\n\n> On a mostly unrelated note, but I just noticed it while playing around\n> with this: 't0000'-basic.sh' runs its internal tests with $SHELL_PATH\n> instead of $TEST_SHELL_PATH. I'm not sure whether that's right or\n> wrong.\n\nI'd say probably wrong, though it likely doesn't matter that much in\npractice.\n\n-Peff\n"},{"id":"370666","messageId":"20190305045851.GJ19800@sigill.intra.peff.net","threadId":"50594","inReplyTo":"20190304143633.GC28939@szeder.dev","subject":"Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-05T04:58:51Z","receivedAt":"2019-03-05T04:58:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2019 at 03:36:33PM +0100, SZEDER Gábor wrote:\n\n> On Tue, Feb 26, 2019 at 04:01:01PM -0500, Jeff King wrote:\n> > > +\t{ set +x ; } 2>/dev/null 4>/dev/null\n> > \n> > Ah, this is the magic. Doing:\n> > \n> > set +x 2>/dev/null\n> > \n> > will still show it, but doing the redirection in a wrapping block means\n> > that it is applied before the command inside the block is run. Clever.\n> \n> Yeah, clever, but unfortunately (and to me suprisingly) unportable:\n> \n> $ ksh\n> $ set -x\n> $ echo foo\n> + echo foo\n> foo\n> $ set +x\n> $ \n> \n> It doesn't show 'set +x', how convenient! :)\n> However:\n> \n> $ set -x\n> $ echo foo 2>/dev/null\n> + echo foo\n> + 2> /dev/null\n> foo\n> $ { set +x; } 2>/dev/null\n> + 2> /dev/null\n> $ \n\nHmph. Good find. As you note, this is already a problem with \"-x\". I'm\nnot sure if there's an easy way to fix this. We can't wrap it in a\nconditional function easily. I guess we could do something like:\n\n if test \"$SOMEHOW_WE_DETECT_KSH\"\n then\n\teval \"set -x; run_the_test; set +x\"\n else\n\teval \"set -x; run_the_test; { set +x; } 2>/dev/null\"\n fi\n\nbut I wonder if just ignoring it is a viable option here. We're talking\nabout debugging output from the test suite, after all. As long as the\ntest suite still _works_ on those shells, and as long as there are no\ndevelopers on ksh-primary systems who can't bear to use another\n$TEST_SHELL_PATH, it's really not hurting anybody. The worst case is\nsomebody reporting a test failure on NetBSD might have a slightly more\nverbose \"-x\" output.\n\n-Peff\n"}]}