tests: replace test -(d|f) with test_path_is_(dir|file)

PATCH In review Rohit Ashiwal via GitGitGadget 12 participants 42 messages Feb 26, 2019 – Mar 5, 2019

Share & tools

Rohit Ashiwal via GitGitGadget

author

2019-02-26 13:42 UTC #0 lore raw

cover tests: replace test -(d|f) with test_path_is_(dir|file)

Previously we were using test -(d|f) to verify the presencee of a directory/file, but we already have helper functions, viz, test_path_is_dir and test_path_is_file with same functionality. This patch will replace test -(d|f) calls in t3600-rm.sh.

Rohit Ashiwal (1):
tests: replace test -(d|f) with test_path_is_(dir|file)

 t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-152%2Fr1walz%2Frefactor-tests-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-152/r1walz/refactor-tests-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/152

Signature · 1 line

gitgitgadget

Rohit Ashiwal via GitGitGadget

author

2019-02-26 13:42 UTC #1 lore raw

PATCH 1/1 tests: replace `test -(d|f)` with test_path_is_(dir|file)

From: Rohit Ashiwal <[email protected]>

t3600-rm.sh: Previously we were using test -(d|f) to verify the presencee of a directory/file, but we already have helper functions, viz, test_path_is_dir and test_path_is_file with same functionality. This patch will replace test -(d|f) calls in t3600-rm.sh.

Signed-off-by Rohit Ashiwal <[email protected]>

---
 t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

t/t3600-rm.sh +48 ~~-48~~

index 04e5d42bd3..dcaa2ab4d6 100755
@@ -137,8 +137,8 @@ test_expect_success 'Re-add foo and baz' '
 test_expect_success 'Modify foo -- rm should refuse' '
 	echo >>foo &&
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '

@@ -159,8 +159,8 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '
 
 test_expect_success 'foo is different in index from HEAD -- rm should refuse' '
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '

@@ -194,21 +194,21 @@ test_expect_success 'Recursive test setup' '
 
 test_expect_success 'Recursive without -r fails' '
 	test_must_fail git rm frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r but dirty' '
 	echo qfwfq >>frotz/nitfol &&
 	test_must_fail git rm -r frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r -f' '
 	git rm -f -r frotz &&
-	! test -f frotz/nitfol &&
-	! test -d frotz
+	! test_path_is_file frotz/nitfol &&
+	! test_path_is_dir frotz
 '
 
 test_expect_success 'Remove nonexistent file returns nonzero exit status' '

@@ -254,7 +254,7 @@ test_expect_success 'rm removes subdirectories recursively' '
 	echo content >dir/subdir/subsubdir/file &&
 	git add dir/subdir/subsubdir/file &&
 	git rm -f dir/subdir/subsubdir/file &&
-	! test -d dir
+	! test_path_is_dir dir
 '
 
 cat >expect <<EOF

@@ -343,8 +343,8 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git submodule update &&
 	git -C submod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified actual &&
 	git rm -f submod &&

@@ -359,8 +359,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g
 	git reset --hard &&
 	git submodule update &&
 	git rm --cached submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.cached actual &&
 	git config -f .gitmodules submodule.sub.url &&

@@ -371,7 +371,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm -n submod &&
-	test -f submod/.git &&
+	test_path_is_file submod/.git &&
 	git diff-index --exit-code HEAD
 '

@@ -381,8 +381,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '
 	git rm .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.both_deleted actual
 '

@@ -393,14 +393,14 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta
 	git config -f .gitmodules foo.bar true &&
 	test_must_fail git rm submod >actual 2>actual.err &&
 	test -s actual.err &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git diff-files --quiet -- submod &&
 	git add .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '

@@ -413,8 +413,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule
 	echo "warning: Could not find section in .gitmodules where path=submod" >expect.err &&
 	git rm submod >actual 2>actual.err &&
 	test_i18ncmp expect.err actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '

@@ -424,8 +424,8 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	git submodule update &&
 	echo X >submod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&

@@ -439,8 +439,8 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	git submodule update &&
 	echo X >submod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&

@@ -493,8 +493,8 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git -C submod checkout HEAD^ &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&

@@ -512,8 +512,8 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
 	echo X >submod/empty &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&

@@ -531,8 +531,8 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files
 	echo X >submod/untracked &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&

@@ -552,13 +552,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director
 	) &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git merge --abort &&

@@ -586,8 +586,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates
 		rm -r ../.git/modules/sub
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test -s actual &&
 	test_i18ngrep Migrating output.err

@@ -624,8 +624,8 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	git submodule update --recursive &&
 	git -C submod/subsubmod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&

@@ -639,8 +639,8 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&

@@ -654,8 +654,8 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&

@@ -673,8 +673,8 @@ test_expect_success "rm absorbs submodule's nested .git directory" '
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/subsubmod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_dir submod/subsubmod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test -s actual &&
 	test_i18ngrep Migrating output.err

Signature · 1 line

gitgitgadget

Duy Nguyen

2019-02-26 14:05 UTC #2 lore raw

Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)

Quoted 7 lines · Rohit Ashiwal via GitGitGadget

From: Rohit Ashiwal <[email protected]>
t3600-rm.sh: Previously we were using test -(d|f) to verify the presencee of a directory/file, but we already have helper functions, viz, test_path_is_dir and test_path_is_file with same functionality. This

It's not just the same (no point replacing then). It's better. When test_path_is_xxx fails, you get an error message. If "test -xxx" fails, you get a failed test with no clue what caused it.

Quoted 272 lines

patch will replace test -(d|f) calls in t3600-rm.sh.

Signed-off-by Rohit Ashiwal <[email protected]>

---
 t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

t/t3600-rm.sh +48 ~~-49~~

index 04e5d42bd3..dcaa2ab4d6 100755
@@ -137,8 +137,8 @@ test_expect_success 'Re-add foo and baz' '
 test_expect_success 'Modify foo -- rm should refuse' '
        echo >>foo &&
        test_must_fail git rm foo baz &&
-       test -f foo &&
-       test -f baz &&
+       test_path_is_file foo &&
+       test_path_is_file baz &&
        git ls-files --error-unmatch foo baz
 '

@@ -159,8 +159,8 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '
 
 test_expect_success 'foo is different in index from HEAD -- rm should refuse' '
        test_must_fail git rm foo baz &&
-       test -f foo &&
-       test -f baz &&
+       test_path_is_file foo &&
+       test_path_is_file baz &&
        git ls-files --error-unmatch foo baz
 '

@@ -194,21 +194,21 @@ test_expect_success 'Recursive test setup' '
 
 test_expect_success 'Recursive without -r fails' '
        test_must_fail git rm frotz &&
-       test -d frotz &&
-       test -f frotz/nitfol
+       test_path_is_dir frotz &&
+       test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r but dirty' '
        echo qfwfq >>frotz/nitfol &&
        test_must_fail git rm -r frotz &&
-       test -d frotz &&
-       test -f frotz/nitfol
+       test_path_is_dir frotz &&
+       test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r -f' '
        git rm -f -r frotz &&
-       ! test -f frotz/nitfol &&
-       ! test -d frotz
+       ! test_path_is_file frotz/nitfol &&
+       ! test_path_is_dir frotz
 '
 
 test_expect_success 'Remove nonexistent file returns nonzero exit status' '

@@ -254,7 +254,7 @@ test_expect_success 'rm removes subdirectories recursively' '
        echo content >dir/subdir/subsubdir/file &&
        git add dir/subdir/subsubdir/file &&
        git rm -f dir/subdir/subsubdir/file &&
-       ! test -d dir
+       ! test_path_is_dir dir
 '
 
 cat >expect <<EOF

@@ -343,8 +343,8 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
        git submodule update &&
        git -C submod checkout HEAD^ &&
        test_must_fail git rm submod &&
-       test -d submod &&
-       test -f submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_file submod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test_cmp expect.modified actual &&
        git rm -f submod &&

@@ -359,8 +359,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g
        git reset --hard &&
        git submodule update &&
        git rm --cached submod &&
-       test -d submod &&
-       test -f submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_file submod/.git &&
        git status -s -uno >actual &&
        test_cmp expect.cached actual &&
        git config -f .gitmodules submodule.sub.url &&

@@ -371,7 +371,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
        git reset --hard &&
        git submodule update &&
        git rm -n submod &&
-       test -f submod/.git &&
+       test_path_is_file submod/.git &&
        git diff-index --exit-code HEAD
 '

@@ -381,8 +381,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '
        git rm .gitmodules &&
        git rm submod >actual 2>actual.err &&
        test_must_be_empty actual.err &&
-       ! test -d submod &&
-       ! test -f submod/.git &&
+       ! test_path_is_dir submod &&
+       ! test_path_is_file submod/.git &&
        git status -s -uno >actual &&
        test_cmp expect.both_deleted actual
 '

@@ -393,14 +393,14 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta
        git config -f .gitmodules foo.bar true &&
        test_must_fail git rm submod >actual 2>actual.err &&
        test -s actual.err &&
-       test -d submod &&
-       test -f submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_file submod/.git &&
        git diff-files --quiet -- submod &&
        git add .gitmodules &&
        git rm submod >actual 2>actual.err &&
        test_must_be_empty actual.err &&
-       ! test -d submod &&
-       ! test -f submod/.git &&
+       ! test_path_is_dir submod &&
+       ! test_path_is_file submod/.git &&
        git status -s -uno >actual &&
        test_cmp expect actual
 '

@@ -413,8 +413,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule
        echo "warning: Could not find section in .gitmodules where path=submod" >expect.err &&
        git rm submod >actual 2>actual.err &&
        test_i18ncmp expect.err actual.err &&
-       ! test -d submod &&
-       ! test -f submod/.git &&
+       ! test_path_is_dir submod &&
+       ! test_path_is_file submod/.git &&
        git status -s -uno >actual &&
        test_cmp expect actual
 '

@@ -424,8 +424,8 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
        git submodule update &&
        echo X >submod/empty &&
        test_must_fail git rm submod &&
-       test -d submod &&
-       test -f submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_file submod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test_cmp expect.modified_inside actual &&
        git rm -f submod &&

@@ -439,8 +439,8 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
        git submodule update &&
        echo X >submod/untracked &&
        test_must_fail git rm submod &&
-       test -d submod &&
-       test -f submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_file submod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test_cmp expect.modified_untracked actual &&
        git rm -f submod &&

@@ -493,8 +493,8 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
        git -C submod checkout HEAD^ &&
        test_must_fail git merge conflict2 &&
        test_must_fail git rm submod &&
-       test -d submod &&
-       test -f submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_file submod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test_cmp expect.conflict actual &&
        git rm -f submod &&

@@ -512,8 +512,8 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
        echo X >submod/empty &&
        test_must_fail git merge conflict2 &&
        test_must_fail git rm submod &&
-       test -d submod &&
-       test -f submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_file submod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test_cmp expect.conflict actual &&
        git rm -f submod &&

@@ -531,8 +531,8 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files
        echo X >submod/untracked &&
        test_must_fail git merge conflict2 &&
        test_must_fail git rm submod &&
-       test -d submod &&
-       test -f submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_file submod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test_cmp expect.conflict actual &&
        git rm -f submod &&

@@ -552,13 +552,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director
        ) &&
        test_must_fail git merge conflict2 &&
        test_must_fail git rm submod &&
-       test -d submod &&
-       test -d submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_dir submod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test_cmp expect.conflict actual &&
        test_must_fail git rm -f submod &&
-       test -d submod &&
-       test -d submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_dir submod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test_cmp expect.conflict actual &&
        git merge --abort &&

@@ -586,8 +586,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates
                rm -r ../.git/modules/sub
        ) &&
        git rm submod 2>output.err &&
-       ! test -d submod &&
-       ! test -d submod/.git &&
+       ! test_path_is_dir submod &&
+       ! test_path_is_dir submod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test -s actual &&
        test_i18ngrep Migrating output.err

@@ -624,8 +624,8 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
        git submodule update --recursive &&
        git -C submod/subsubmod checkout HEAD^ &&
        test_must_fail git rm submod &&
-       test -d submod &&
-       test -f submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_file submod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test_cmp expect.modified_inside actual &&
        git rm -f submod &&

@@ -639,8 +639,8 @@ test_expect_success 'rm of a populated nested submodule with nested modification
        git submodule update --recursive &&
        echo X >submod/subsubmod/empty &&
        test_must_fail git rm submod &&
-       test -d submod &&
-       test -f submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_file submod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test_cmp expect.modified_inside actual &&
        git rm -f submod &&

@@ -654,8 +654,8 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
        git submodule update --recursive &&
        echo X >submod/subsubmod/untracked &&
        test_must_fail git rm submod &&
-       test -d submod &&
-       test -f submod/.git &&
+       test_path_is_dir submod &&
+       test_path_is_file submod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test_cmp expect.modified_untracked actual &&
        git rm -f submod &&

@@ -673,8 +673,8 @@ test_expect_success "rm absorbs submodule's nested .git directory" '
                GIT_WORK_TREE=. git config --unset core.worktree
        ) &&
        git rm submod 2>output.err &&
-       ! test -d submod &&
-       ! test -d submod/subsubmod/.git &&
+       ! test_path_is_dir submod &&
+       ! test_path_is_dir submod/subsubmod/.git &&
        git status -s -uno --ignore-submodules=none >actual &&
        test -s actual &&
        test_i18ngrep Migrating output.err
--

gitgitgadget

Signature · 1 line

Duy

Rohit Ashiwal via GitGitGadget

author

2019-02-26 14:26 UTC #25 lore raw

v2 cover t3600: use test_path_is_dir and test_path_is_file

In reply to Rohit Ashiwal via GitGitGadget

Rohit Ashiwal (1):
t3600: use test_path_is_dir and test_path_is_file

 t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-152%2Fr1walz%2Frefactor-tests-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-152/r1walz/refactor-tests-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/152

Range-diff vs v1:

1: bf5eb04579 ! 1: fcafc87b38 tests: replace test -(d|f) with test_path_is_(dir|file)
@@ -1,12 +1,16 @@
Author: Rohit Ashiwal <[email protected]>

- tests: replace test -(d|f) with test_path_is_(dir|file)
+ t3600: use test_path_is_dir and test_path_is_file

- t3600-rm.sh: Previously we were using test -(d|f)
- to verify the presencee of a directory/file, but we
- already have helper functions, viz, test_path_is_dir
- and test_path_is_file with same functionality. This
- patch will replace test -(d|f) calls in t3600-rm.sh.
+ Previously we were using test -(d|f) to verify
+ the presence of a directory/file, but we already
+ have helper functions, viz, test_path_is_dir
+ and test_path_is_file with better functionality.
+ This patch will replace test -(d|f) calls in t3660.sh
+
+ These helper functions make code more readable
+ and informative to someone new to code, also
+ these functions have better error messages

Signed-off-by Rohit Ashiwal <[email protected]>

Signature · 1 line

gitgitgadget

Rohit Ashiwal via GitGitGadget

author

2019-02-26 14:26 UTC #26 lore raw

PATCH v2 1/1 t3600: use test_path_is_dir and test_path_is_file

From: Rohit Ashiwal <[email protected]>

Previously we were using test -(d|f) to verify the presence of a directory/file, but we already have helper functions, viz, test_path_is_dir and test_path_is_file with better functionality. This patch will replace test -(d|f) calls in t3660.sh

These helper functions make code more readable and informative to someone new to code, also these functions have better error messages

Signed-off-by Rohit Ashiwal <[email protected]>

---
 t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

t/t3600-rm.sh +48 ~~-48~~

index 04e5d42bd3..dcaa2ab4d6 100755
@@ -137,8 +137,8 @@ test_expect_success 'Re-add foo and baz' '
 test_expect_success 'Modify foo -- rm should refuse' '
 	echo >>foo &&
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '

@@ -159,8 +159,8 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '
 
 test_expect_success 'foo is different in index from HEAD -- rm should refuse' '
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '

@@ -194,21 +194,21 @@ test_expect_success 'Recursive test setup' '
 
 test_expect_success 'Recursive without -r fails' '
 	test_must_fail git rm frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r but dirty' '
 	echo qfwfq >>frotz/nitfol &&
 	test_must_fail git rm -r frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r -f' '
 	git rm -f -r frotz &&
-	! test -f frotz/nitfol &&
-	! test -d frotz
+	! test_path_is_file frotz/nitfol &&
+	! test_path_is_dir frotz
 '
 
 test_expect_success 'Remove nonexistent file returns nonzero exit status' '

@@ -254,7 +254,7 @@ test_expect_success 'rm removes subdirectories recursively' '
 	echo content >dir/subdir/subsubdir/file &&
 	git add dir/subdir/subsubdir/file &&
 	git rm -f dir/subdir/subsubdir/file &&
-	! test -d dir
+	! test_path_is_dir dir
 '
 
 cat >expect <<EOF

@@ -343,8 +343,8 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git submodule update &&
 	git -C submod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified actual &&
 	git rm -f submod &&

@@ -359,8 +359,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g
 	git reset --hard &&
 	git submodule update &&
 	git rm --cached submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.cached actual &&
 	git config -f .gitmodules submodule.sub.url &&

@@ -371,7 +371,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm -n submod &&
-	test -f submod/.git &&
+	test_path_is_file submod/.git &&
 	git diff-index --exit-code HEAD
 '

@@ -381,8 +381,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '
 	git rm .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.both_deleted actual
 '

@@ -393,14 +393,14 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta
 	git config -f .gitmodules foo.bar true &&
 	test_must_fail git rm submod >actual 2>actual.err &&
 	test -s actual.err &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git diff-files --quiet -- submod &&
 	git add .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '

@@ -413,8 +413,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule
 	echo "warning: Could not find section in .gitmodules where path=submod" >expect.err &&
 	git rm submod >actual 2>actual.err &&
 	test_i18ncmp expect.err actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '

@@ -424,8 +424,8 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	git submodule update &&
 	echo X >submod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&

@@ -439,8 +439,8 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	git submodule update &&
 	echo X >submod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&

@@ -493,8 +493,8 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git -C submod checkout HEAD^ &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&

@@ -512,8 +512,8 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
 	echo X >submod/empty &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&

@@ -531,8 +531,8 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files
 	echo X >submod/untracked &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&

@@ -552,13 +552,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director
 	) &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git merge --abort &&

@@ -586,8 +586,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates
 		rm -r ../.git/modules/sub
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test -s actual &&
 	test_i18ngrep Migrating output.err

@@ -624,8 +624,8 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	git submodule update --recursive &&
 	git -C submod/subsubmod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&

@@ -639,8 +639,8 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&

@@ -654,8 +654,8 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&

@@ -673,8 +673,8 @@ test_expect_success "rm absorbs submodule's nested .git directory" '
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/subsubmod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_dir submod/subsubmod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test -s actual &&
 	test_i18ngrep Migrating output.err

Signature · 1 line

gitgitgadget

Johannes Schindelin

2019-02-26 16:01 UTC #19 lore raw

Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)

In reply to Rohit Ashiwal via GitGitGadget

Hi Rohit,

the oneline suggests that this fixes all the tests, but it only fixes t3600. So maybe use "t3600:" instead of "tests:"?

Quoted 7 lines · Rohit Ashiwal via GitGitGadget

From: Rohit Ashiwal <[email protected]>
t3600-rm.sh: Previously we were using test -(d|f) to verify the presencee of a directory/file, but we already have helper functions, viz, test_path_is_dir and test_path_is_file with same functionality. This patch will replace test -(d|f) calls in t3600-rm.sh.

This answers a bit of the "what?", but little in the way of "how?".

Another thing to mention in the commit message is the "why?"... So far, a casual reader will not exactly understand what the benefit might be, and might even disagree that it is an improvement because

1. the new code will be slower (as it adds one level of indirection: a
shell function)

2. the new code is more verbose (test -d is shorter than
test_path_is_dir)

Obviously, the active Git developers do agree, though, that it is a good change (otherwise they would not have suggested it as a GSoC microproject), and I think it is because:

1. the new code is a lot more obvious to developers who are not fluent
in Unix shell scripting, and

2. the new code is a lot more informative in case of a breakage.

The patch looks fine,
Johannes

Quoted 272 lines

Signed-off-by Rohit Ashiwal <[email protected]>

---
 t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

t/t3600-rm.sh +48 ~~-48~~

index 04e5d42bd3..dcaa2ab4d6 100755
@@ -137,8 +137,8 @@ test_expect_success 'Re-add foo and baz' '
 test_expect_success 'Modify foo -- rm should refuse' '
 	echo >>foo &&
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '

@@ -159,8 +159,8 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '
 
 test_expect_success 'foo is different in index from HEAD -- rm should refuse' '
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '

@@ -194,21 +194,21 @@ test_expect_success 'Recursive test setup' '
 
 test_expect_success 'Recursive without -r fails' '
 	test_must_fail git rm frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r but dirty' '
 	echo qfwfq >>frotz/nitfol &&
 	test_must_fail git rm -r frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r -f' '
 	git rm -f -r frotz &&
-	! test -f frotz/nitfol &&
-	! test -d frotz
+	! test_path_is_file frotz/nitfol &&
+	! test_path_is_dir frotz
 '
 
 test_expect_success 'Remove nonexistent file returns nonzero exit status' '

@@ -254,7 +254,7 @@ test_expect_success 'rm removes subdirectories recursively' '
 	echo content >dir/subdir/subsubdir/file &&
 	git add dir/subdir/subsubdir/file &&
 	git rm -f dir/subdir/subsubdir/file &&
-	! test -d dir
+	! test_path_is_dir dir
 '
 
 cat >expect <<EOF

@@ -343,8 +343,8 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git submodule update &&
 	git -C submod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified actual &&
 	git rm -f submod &&

@@ -359,8 +359,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g
 	git reset --hard &&
 	git submodule update &&
 	git rm --cached submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.cached actual &&
 	git config -f .gitmodules submodule.sub.url &&

@@ -371,7 +371,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm -n submod &&
-	test -f submod/.git &&
+	test_path_is_file submod/.git &&
 	git diff-index --exit-code HEAD
 '

@@ -381,8 +381,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '
 	git rm .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.both_deleted actual
 '

@@ -393,14 +393,14 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta
 	git config -f .gitmodules foo.bar true &&
 	test_must_fail git rm submod >actual 2>actual.err &&
 	test -s actual.err &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git diff-files --quiet -- submod &&
 	git add .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '

@@ -413,8 +413,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule
 	echo "warning: Could not find section in .gitmodules where path=submod" >expect.err &&
 	git rm submod >actual 2>actual.err &&
 	test_i18ncmp expect.err actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '

@@ -424,8 +424,8 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	git submodule update &&
 	echo X >submod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&

@@ -439,8 +439,8 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	git submodule update &&
 	echo X >submod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&

@@ -493,8 +493,8 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git -C submod checkout HEAD^ &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&

@@ -512,8 +512,8 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
 	echo X >submod/empty &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&

@@ -531,8 +531,8 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files
 	echo X >submod/untracked &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&

@@ -552,13 +552,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director
 	) &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git merge --abort &&

@@ -586,8 +586,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates
 		rm -r ../.git/modules/sub
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test -s actual &&
 	test_i18ngrep Migrating output.err

@@ -624,8 +624,8 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	git submodule update --recursive &&
 	git -C submod/subsubmod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&

@@ -639,8 +639,8 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&

@@ -654,8 +654,8 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&

@@ -673,8 +673,8 @@ test_expect_success "rm absorbs submodule's nested .git directory" '
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/subsubmod/.git &&
+	! test_path_is_dir submod &&
+	! test_path_is_dir submod/subsubmod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test -s actual &&
 	test_i18ngrep Migrating output.err

Signature · 1 line

gitgitgadget

Ævar Arnfjörð Bjarmason

2019-02-26 16:10 UTC #3 lore raw

Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to Duy Nguyen

Quoted 5 lines · Duy Nguyen

Quoted 3 lines · Rohit Ashiwal via GitGitGadget
From: Rohit Ashiwal <[email protected]>

Quoted 8 lines

Quoted 4 lines
t3600-rm.sh: Previously we were using test -(d|f) to verify the presencee of a directory/file, but we already have helper functions, viz, test_path_is_dir and test_path_is_file with same functionality. This
It's not just the same (no point replacing then). It's better. When test_path_is_xxx fails, you get an error message. If "test -xxx" fails, you get a failed test with no clue what caused it.

I swear I'm not just on a mission to ruin everyone's GSOC projects. This patch definitely looks good, and given that we have this / document it makes sense.

However. I wonder in general if we've re-visited the utility of these wrappers and maybe other similar wrappers after -x was added.

Back when this was added in 2caf20c52b ("test-lib: user-friendly alternatives to test [-d|-f|-e]", 2010-08-10) we didn't have -x. So we'd at best fail like this:

    $ ./t0001-init.sh  -v -i
    Initialized empty Git repository in /home/avar/g/git/t/trash directory.t0001-init/.git/
    expecting success:
            test -d .git &&
            test -f doesnotexist &&
            test -f .git/config

    not ok 1 - check files
    #
    #               test -d .git &&
    #               test -f doesnotexist &&
    #               test -f .git/config
    #

At that point this was a definite improvement:

expecting success:
test_path_is_dir .git &&
test_path_is_file doesnotexist &&
test_path_is_file .git/config

File doesnotexist doesn't exist.
not ok 1 - check files

But 4 years after this was added in a136f6d8ff ("test-lib.sh: support -x option for shell-tracing", 2014-10-10) we got -x, and then with "-i -v -x":

expecting success:
test_path_is_dir .git &&
test_path_is_file doesnotexist &&
test_path_is_file .git/config

+ test_path_is_dir .git
+ test -d .git
+ test_path_is_file doesnotexist
+ test -f doesnotexist
+ echo File doesnotexist doesn't exist.
File doesnotexist doesn't exist.
+ false
error: last command exited with $?=1
not ok 1 - check files

But by just using "test -d/-e": the much shorter:

+ test -d .git
+ test -f doesnotexist
error: last command exited with $?=1
not ok 1 - check files

So I wonder if these days we shouldn't do this the other way around and get rid of these. Every test_* wrapper we add adds a bit of cognitive overload when you have to remember Git's specific shellscript dialect.

And at least to me whenever I have a test failure the first thing I do is try with -x (if I wasn't already using it). Under that the wrapper output is more verbose and no more helpful. It's immediately clear what's going on with:

+ test -f doesnotexist
error: last command exited with $?=1

Whereas:

+ test -f doesnotexist
+ echo File doesnotexist doesn't exist.
File doesnotexist doesn't exist.
+ false
error: last command exited with $?=1

Gives me the same thing, but I have to read 5 lines instead of 2 that ultimately don't tell me any more (and a bit of "huh, 'false' returned 1? Of course! Oh! It's faking things up and it's the 'echo' that matters...").

Looking over test-lib-functions.sh this patch would do it. I couldn't spot any other functions redundant to -x:

    diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
    index 80402a428f..b3a95b4968 100644
    --- a/t/test-lib-functions.sh
    +++ b/t/test-lib-functions.sh
    @@ -555,33 +555,6 @@ test_external_without_stderr () {
     	fi
     }

    -# debugging-friendly alternatives to "test [-f|-d|-e]"
    -# The commands test the existence or non-existence of $1. $2 can be
    -# given to provide a more precise diagnosis.
    -test_path_is_file () {
    -	if ! test -f "$1"
    -	then
    -		echo "File $1 doesn't exist. $2"
    -		false
    -	fi
    -}
    -
    -test_path_is_dir () {
    -	if ! test -d "$1"
    -	then
    -		echo "Directory $1 doesn't exist. $2"
    -		false
    -	fi
    -}
    -
    -test_path_exists () {
    -	if ! test -e "$1"
    -	then
    -		echo "Path $1 doesn't exist. $2"
    -		false
    -	fi
    -}
    -
     # Check if the directory exists and is empty as expected, barf otherwise.
     test_dir_is_empty () {
     	test_path_is_dir "$1" &&
    @@ -593,19 +566,6 @@ test_dir_is_empty () {
     	fi
     }

    -test_path_is_missing () {
    -	if test -e "$1"
    -	then
    -		echo "Path exists:"
    -		ls -ld "$1"
    -		if test $# -ge 1
    -		then
    -			echo "$*"
    -		fi
    -		false
    -	fi
    -}
    -
     # test_line_count checks that a file has the number of lines it
     # ought to. For example:
     #
    @@ -849,6 +809,9 @@ verbose () {
     # otherwise.

     test_must_be_empty () {
    +	# We don't want to remove this as noted in ec10b018e7 ("tests:
    +	# use 'test_must_be_empty' instead of '! test -s'",
    +	# 2018-08-19)
     	test_path_is_file "$1" &&
     	if test -s "$1"
     	then

Martin Ågren

2019-02-26 16:31 UTC #20 lore raw

Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)

In reply to Rohit Ashiwal via GitGitGadget

Quoted 6 lines · Rohit Ashiwal via GitGitGadget

t3600-rm.sh: Previously we were using test -(d|f) to verify the presencee of a directory/file, but we already have helper functions, viz, test_path_is_dir and test_path_is_file with same functionality. This patch will replace test -(d|f) calls in t3600-rm.sh.

I think this makes a lot of sense. If a test breaks, we'll get some helpful error message. Thank you for working on this.

Review

Quoted 2 lines

(quoted diff) +1 -1

-       ! test -d submod &&
+       ! test_path_is_dir submod &&

Now, here I wonder. This (and other changes like this) means that every time the test passes, we see "Directory submod doesn't exist.", which is perhaps not too irritating. But more importantly, when the test fails, we don't get any hint. So a failure is just as silent and "non-helpful" as before. I can think of a few approaches:

1 Teach test_path_is_dir and friends to handle "!" in a clever way, and
write these as test_path_is_dir ! foo. (We already have helpers that do this, see, e.g., test_i18ngrep.)

2 Don't be clever, and just introduce test_path_is_not_dir.

3 Don't bother, because this small change here doesn't make the error
case any worse.

4 Don't do this small change here, and leave cases like this for a
later change (something like 1 or 2 above).

What do you think?

There are a few of these "!". The other changes look good to me.

Cheers
Martin

SZEDER Gábor

2019-02-26 16:37 UTC #27 lore raw

Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file

In reply to Rohit Ashiwal via GitGitGadget

Hi and welcome!

Quoted 5 lines · Rohit Ashiwal via GitGitGadget

Previously we were using test -(d|f) to verify the presence of a directory/file, but we already have helper functions, viz, test_path_is_dir and test_path_is_file with better functionality. This patch will replace test -(d|f) calls in t3660.sh

We prefer to use imperative mode when talking about what a patch does, as if the author were to give orders to the code base. So e.g. instead of

This patch will ...

we would usually write something like this:

Replace 'test -(d|f)' calls in t3600 with the corresponding helper functions.

Quoted 8 lines

These helper functions make code more readable and informative to someone new to code, also these functions have better error messages
Signed-off-by Rohit Ashiwal <[email protected]>
---
 t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

The patch itself seems to be a straightforward application of

s/test -f/test_path_is_file/
s/test -d/test_path_is_dir/

so it looks good for the most part, but it has a few issues:

t/t3600-rm.sh test_expect_success 'Recursive test setup' ' · line 212

Quoted 7 lines

test_expect_success 'Recursive with -r -f' '

t/t3600-rm.sh +2 -2

 	git rm -f -r frotz &&
-	! test -f frotz/nitfol &&
-	! test -d frotz
+	! test_path_is_file frotz/nitfol &&
+	! test_path_is_dir frotz
 '

These should rather use the test_path_is_missing helper function.

However, if the directory 'frotz' is missing, then surely 'frotz/nitfol' could not possibly exist either, could it? I'm not sure why this test (and a couple of others) checks both, and wonder whether the redundant check for the file inside the supposedly non-existing directory could be removed.

Furthermore, there are a couple of place where the '!' is not in front of the whole 'test' command but is given as an argument, e.g.:

test ! -f file

Please convert those cases as well.

SZEDER Gábor

2019-02-26 17:04 UTC #4 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to Ævar Arnfjörð Bjarmason

Quoted 5 lines · Ævar Arnfjörð Bjarmason

However. I wonder in general if we've re-visited the utility of these wrappers and maybe other similar wrappers after -x was added.
But 4 years after this was added in a136f6d8ff ("test-lib.sh: support -x option for shell-tracing", 2014-10-10) we got -x, and then with "-i -v -x":

'-x' tracing doesn't work in all test scripts, unless it is run with a Bash version already supporting BASH_XTRACEFD, i.e. v4.1 or later. Notably the default Bash shipped in macOS is somewhere around v3.2.

Quoted 16 lines

And at least to me whenever I have a test failure the first thing I do is try with -x (if I wasn't already using it). Under that the wrapper output is more verbose and no more helpful. It's immediately clear what's going on with:
+ test -f doesnotexist
error: last command exited with $?=1
Whereas:
+ test -f doesnotexist
+ echo File doesnotexist doesn't exist.
File doesnotexist doesn't exist.
+ false
error: last command exited with $?=1

Quoted 4 lines

Gives me the same thing, but I have to read 5 lines instead of 2 that ultimately don't tell me any more (and a bit of "huh, 'false' returned 1? Of course! Oh! It's faking things up and it's the 'echo' that matters...").

I didn't find this to be an issue, but because of functions like 'test_seq' and 'test_must_fail' I've thought about suppressing '-x' output for test helpers (haven't actually done anything about it, though).

Quoted 14 lines

Looking over test-lib-functions.sh this patch would do it. I couldn't spot any other functions redundant to -x:

    diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
    index 80402a428f..b3a95b4968 100644
    --- a/t/test-lib-functions.sh
    +++ b/t/test-lib-functions.sh
    @@ -555,33 +555,6 @@ test_external_without_stderr () {
     	fi
     }

    -# debugging-friendly alternatives to "test [-f|-d|-e]"
    -# The commands test the existence or non-existence of $1. $2 can be
    -# given to provide a more precise diagnosis.

Note the second parameter; though, of course, you could argue that we use it so rarely that it wouldn't really be missed.

Quoted 28 lines

    -test_path_is_file () {
    -	if ! test -f "$1"
    -	then
    -		echo "File $1 doesn't exist. $2"
    -		false
    -	fi
    -}
    -
    -test_path_is_dir () {
    -	if ! test -d "$1"
    -	then
    -		echo "Directory $1 doesn't exist. $2"
    -		false
    -	fi
    -}
    -
    -test_path_exists () {
    -	if ! test -e "$1"
    -	then
    -		echo "Path $1 doesn't exist. $2"
    -		false
    -	fi
    -}
    -
     # Check if the directory exists and is empty as expected, barf otherwise.
     test_dir_is_empty () {
     	test_path_is_dir "$1" &&
    @@ -593,19 +566,6 @@ test_dir_is_empty () {

Quoted 8 lines

     	fi
     }

    -test_path_is_missing () {
    -	if test -e "$1"
    -	then
    -		echo "Path exists:"
    -		ls -ld "$1"

This 'ls' command gives a bit of additional info.

Quoted 10 lines

    -		if test $# -ge 1
    -		then
    -			echo "$*"
    -		fi
    -		false
    -	fi
    -}
    -
     # test_line_count checks that a file has the number of lines it
     # ought to. For example:

Quoted 8 lines

     #
    @@ -849,6 +809,9 @@ verbose () {
     # otherwise.

     test_must_be_empty () {
    +	# We don't want to remove this as noted in ec10b018e7 ("tests:
    +	# use 'test_must_be_empty' instead of '! test -s'",
    +	# 2018-08-19)

Indeed.

Quoted 1 line

     	test_path_is_file "$1" &&

This still uses 'test_path_is_file'.

Quoted 2 lines

     	if test -s "$1"
     	then

Jeff King

2019-02-26 17:35 UTC #14 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to Ævar Arnfjörð Bjarmason

Quoted 20 lines · Ævar Arnfjörð Bjarmason

But 4 years after this was added in a136f6d8ff ("test-lib.sh: support -x option for shell-tracing", 2014-10-10) we got -x, and then with "-i -v -x":
expecting success:
test_path_is_dir .git &&
test_path_is_file doesnotexist &&
test_path_is_file .git/config
+ test_path_is_dir .git
+ test -d .git
+ test_path_is_file doesnotexist
+ test -f doesnotexist
+ echo File doesnotexist doesn't exist.
File doesnotexist doesn't exist.
+ false
error: last command exited with $?=1
not ok 1 - check files
But by just using "test -d/-e": the much shorter:

Quoted 8 lines

+ test -d .git
+ test -f doesnotexist
error: last command exited with $?=1
not ok 1 - check files
So I wonder if these days we shouldn't do this the other way around and get rid of these. Every test_* wrapper we add adds a bit of cognitive overload when you have to remember Git's specific shellscript dialect.

I don't have a strong opinion, but I do agree that with "-x" it's nicer without the wrappers. I typically re-run with just "-v" on a failure, and only turn to "-x" if the verbose output isn't helpful. However, with the rise of multi-platform CI jobs which try to collect as much information as possible in the initial run, I do find myself looking at "-x" more often.

As Gábor notes, you can't run every script with "-x". But I find it's pretty consistent these days (and totally so if you have a recent bash). I dunno. Maybe people on other platforms (who might not have bash) would care more.

I had a vague notion that there was some reason (portability?) that we preferred to have the wrappers. But as your patch shows, they really are just calling "test" and nothing else.

Quoted 7 lines

     test_must_be_empty () {
    +	# We don't want to remove this as noted in ec10b018e7 ("tests:
    +	# use 'test_must_be_empty' instead of '! test -s'",
    +	# 2018-08-19)
     	test_path_is_file "$1" &&
     	if test -s "$1"
     	then

You'd still want it to become "test -f" though, right?

-Peff

Jeff King

2019-02-26 17:43 UTC #5 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to SZEDER Gábor

Quoted 13 lines · SZEDER Gábor

Quoted 8 lines
Whereas:
+ test -f doesnotexist
+ echo File doesnotexist doesn't exist.
File doesnotexist doesn't exist.
+ false
error: last command exited with $?=1
Quoted 4 lines
Gives me the same thing, but I have to read 5 lines instead of 2 that ultimately don't tell me any more (and a bit of "huh, 'false' returned 1? Of course! Oh! It's faking things up and it's the 'echo' that matters...").

Quoted 4 lines

I didn't find this to be an issue, but because of functions like 'test_seq' and 'test_must_fail' I've thought about suppressing '-x' output for test helpers (haven't actually done anything about it, though).

I'd be curious how you'd do that. We can wrap the function and redirect its stderr, but you'd still get a crufty line invoking the inner function (plus the outer function). That's better than seeing the inner details, but not as nice as just seeing the outer function invocation.

I don't think we can play games like the one we do in test_eval_(), because "set -x" will already be on.

-Peff

Matthieu Moy

2019-02-26 17:48 UTC #12 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to SZEDER Gábor

Quoted 10 lines · SZEDER Gábor

Quoted 5 lines · Ævar Arnfjörð Bjarmason
However. I wonder in general if we've re-visited the utility of these wrappers and maybe other similar wrappers after -x was added.
But 4 years after this was added in a136f6d8ff ("test-lib.sh: support -x option for shell-tracing", 2014-10-10) we got -x, and then with "-i -v -x":
'-x' tracing doesn't work in all test scripts, unless it is run with a Bash version already supporting BASH_XTRACEFD, i.e. v4.1 or later. Notably the default Bash shipped in macOS is somewhere around v3.2.

According to http://www.tldp.org/LDP/abs/html/bashver4.html#AEN21183,
bash 4.1 was released on May, 2010. Are you sure macOS is _that_ late?

I also tried with dash, and -x seems to work fine too (I use "works with dash" as a heuristic for "should word on any shell", but it doesn't always work).

If -x doesn't work in some setups, it may be a good reason to wait a bit before trashing test_path_is_*, but if it's clear enough that the vast majority of platforms get -x, then why not trash these wrappers indeed.

Signature · 2 lines

Matthieu Moy
https://matthieu-moy.fr/

Jeff King

2019-02-26 18:24 UTC #13 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

Quoted 6 lines · Matthieu Moy

Quoted 3 lines
'-x' tracing doesn't work in all test scripts, unless it is run with a Bash version already supporting BASH_XTRACEFD, i.e. v4.1 or later. Notably the default Bash shipped in macOS is somewhere around v3.2.
According to http://www.tldp.org/LDP/abs/html/bashver4.html#AEN21183,
bash 4.1 was released on May, 2010. Are you sure macOS is _that_ late?

It's not "late", it's "never". Bash 4 switched to GPLv3.

Quoted 3 lines

I also tried with dash, and -x seems to work fine too (I use "works with dash" as a heuristic for "should word on any shell", but it doesn't always work).

Yes, "-x" works everywhere. The problem is scripts which capture the stderr of subshells or functions, which then get polluted by "-x" output. You can fix that in two ways:

1. Use bash 4.1+, which works around that with BASH_XTRACEFD.

2. Don't do that. Gábor fixed most such instances already, except the ones in t1510. That one automatically disables "-x" tracing.

So I don't know what you tried exactly, but you should be able to successfully run with "-x" on any script. Including t1510, but you just won't get tracing output then.

Quoted 3 lines

If -x doesn't work in some setups, it may be a good reason to wait a bit before trashing test_path_is_*, but if it's clear enough that the vast majority of platforms get -x, then why not trash these wrappers indeed.

I do think it basically works everywhere these days.

-Peff

Rohit Ashiwal

2019-02-26 18:30 UTC #21 lore raw

Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)

In reply to Martin Ågren

Hi Martin

Quoted 11 lines · Martin Ågren

Review
Quoted 3 lines
(quoted diff) +1 -1
-       ! test -d submod &&
+       ! test_path_is_dir submod &&
Now, here I wonder. This (and other changes like this) means that every time the test passes, we see "Directory submod doesn't exist.", which is perhaps not too irritating. But more importantly, when the test fails, we don't get any hint. So a failure is just as silent and "non-helpful" as before. I can think of a few approaches:

Quoted 4 lines

1 Teach test_path_is_dir and friends to handle "!" in a clever way, and
write these as test_path_is_dir ! foo. (We already have helpers that do this, see, e.g., test_i18ngrep.)

Yes, I also think that it should be corrected and I think this(1) approach is good as it resonates well with the existing code. I'll start working on it and submit the patch as soon as possible.

Thanks
Rohit

Rohit Ashiwal

2019-02-26 18:41 UTC #28 lore raw

Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file

In reply to SZEDER Gábor

Hi SZEDER

Quoted 22 lines · SZEDER Gábor

We prefer to use imperative mode when talking about what a patch does, as if the author were to give orders to the code base. So e.g. instead of
This patch will ...
we would usually write something like this:
Replace 'test -(d|f)' calls in t3600 with the corresponding helper functions.
t/t3600-rm.sh test_expect_success 'Recursive test setup' ' · line 212
Quoted 7 lines
test_expect_success 'Recursive with -r -f' '
      git rm -f -r frotz &&
t/t3600-rm.sh +2 -2
-     ! test -f frotz/nitfol &&
-     ! test -d frotz
+     ! test_path_is_file frotz/nitfol &&
+     ! test_path_is_dir frotz
 '

Quoted 8 lines

These should rather use the test_path_is_missing helper function.
However, if the directory 'frotz' is missing, then surely 'frotz/nitfol' could not possibly exist either, could it? I'm not sure why this test (and a couple of others) checks both, and wonder whether the redundant check for the file inside the supposedly non-existing directory could be removed.

Okay! I'll scan through the file to check for redundancy like this and fix them.

Quoted 7 lines

Furthermore, there are a couple of place where the '!' is not in front of the whole 'test' command but is given as an argument, e.g.:
test ! -f file
Please convert those cases as well.

I think since I'm modifying test_path_is_{dir|file} functions to handle calls like ! test_path_is_dir well as mentioned in this thread[1]. I think we should replace ! test calls with test !, so that the changes are in agreement with each other. What do you say?

Thanks for advice
Rohit

[1]: https://public-inbox.org/git/CAN0heSqSp-a0zUKT5EaGLBYnRtESTnu9GKWtGARz2kaOAhc1HQ@mail.gmail.com/

SZEDER Gábor

2019-02-26 19:39 UTC #6 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to Jeff King

Quoted 15 lines · Jeff King

Quoted 13 lines · SZEDER Gábor
Quoted 12 lines
Whereas:
+ test -f doesnotexist
+ echo File doesnotexist doesn't exist.
File doesnotexist doesn't exist.
+ false
error: last command exited with $?=1
Gives me the same thing, but I have to read 5 lines instead of 2 that ultimately don't tell me any more (and a bit of "huh, 'false' returned 1? Of course! Oh! It's faking things up and it's the 'echo' that matters...").

Quoted 6 lines

Quoted 4 lines
I didn't find this to be an issue, but because of functions like 'test_seq' and 'test_must_fail' I've thought about suppressing '-x' output for test helpers (haven't actually done anything about it, though).
I'd be curious how you'd do that.

Well, I started replying with "Dunno" and explaining why I don't think that it can be done with 'test_must_fail'... but then got a bit of a lightbulb moment. Now look at this:

t/test-lib-functions.sh +14 -1

index 80402a428f..16adcd54c9 100644
@@ -664,7 +664,15 @@ list_contains () {
 #     Currently recognized signal names are: sigpipe, success.
 #     (Don't use 'success', use 'test_might_fail' instead.)
 
+restore_tracing () {
+	if test -n "$trace"
+	then
+		set -x
+	fi
+} 2>/dev/null 4>/dev/null
+
 test_must_fail () {
+	{ set +x ; } 2>/dev/null 4>/dev/null
 	case "$1" in
 	ok=*)
 		_test_ok=${1#ok=}

@@ -679,24 +687,29 @@ test_must_fail () {
 	if test $exit_code -eq 0 && ! list_contains "$_test_ok" success
 	then
 		echo >&4 "test_must_fail: command succeeded: $*"
+		restore_tracing
 		return 1
 	elif test_match_signal 13 $exit_code && list_contains "$_test_ok" sigpipe
 	then
+		restore_tracing
 		return 0
 	elif test $exit_code -gt 129 && test $exit_code -le 192
 	then
 		echo >&4 "test_must_fail: died by signal $(($exit_code - 128)): $*"
+		restore_tracing
 		return 1
 	elif test $exit_code -eq 127
 	then
 		echo >&4 "test_must_fail: command not found: $*"
+		restore_tracing
 		return 1
 	elif test $exit_code -eq 126
 	then
 		echo >&4 "test_must_fail: valgrind error: $*"
+		restore_tracing
 		return 1
 	fi
-	return 0
+	restore_tracing
 } 7>&2 2>&4
 
 # Similar to test_must_fail, but tolerates success, too.  This is

Yeah, it's a hassle, especially in a function with as many return paths as 'test_must_fail', but look at its output:

+ test_must_fail git rev-parse nope --
fatal: bad revision 'nope'
+ test_must_fail git rev-parse HEAD -- 48ab21c1a5972e0fa9d87da7c5da9982872b8db2
test_must_fail: command succeeded: git rev-parse HEAD --
+ return 1
error: last command exited with $?=1

Not even the 'set +x' shows up in the trace output! Unfortunately, that line is not particularly pleasing on the eyes, but I don't see any way around that...

Perhaps we could even go one step further with this 'restore_tracing' helper and add a parameter specifying its return code, so we could make it the last command invoked in the test helper function, and then even that 'return 1' would disappear from the trace output. Furthermore, this would be helpful in those functions where the last command's return code is relevant, e.g:

  test_cmp() {
        { set +x ; } 2>/dev/null 4>/dev/null
        $GIT_TEST_CMP "$@"
        restore_tracing $?
  }

There are a couple of tricky cases:

- Some test helper functions call other test helper functions, and in those cases tracing would be enabled upon returning from the inner helper function. This is not an issue with e.g. 'test_might_fail' or 'test_cmp_config', because the inner helper function is the last command anyway. However, there is 'test_must_be_empty', 'test_dir_is_empty', 'test_config', 'test_commit', etc. which call the other test helper functions right at the start or in the middle.

- && chains in test helper functions; we must make sure that the tracing is restored even in case of a failure.

Johannes Schindelin

2019-02-26 19:52 UTC #22 lore raw

Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)

In reply to Rohit Ashiwal

Hi,

Quoted 14 lines · Rohit Ashiwal

Hi Martin
Quoted 11 lines · Martin Ågren
Review
Quoted 3 lines
(quoted diff) +1 -1
-       ! test -d submod &&
+       ! test_path_is_dir submod &&
Now, here I wonder. This (and other changes like this) means that every time the test passes, we see "Directory submod doesn't exist.", which is perhaps not too irritating. But more importantly, when the test fails, we don't get any hint. So a failure is just as silent and "non-helpful" as before. I can think of a few approaches:

Quoted 8 lines

Quoted 4 lines
1 Teach test_path_is_dir and friends to handle "!" in a clever way, and
write these as test_path_is_dir ! foo. (We already have helpers that do this, see, e.g., test_i18ngrep.)
Yes, I also think that it should be corrected and I think this(1) approach is good as it resonates well with the existing code. I'll start working on it and submit the patch as soon as possible.

We already have test_path_is_missing. Why not use that instead of ! test -d or ! test -f?

Ciao,
Johannes

Johannes Schindelin

2019-02-26 19:59 UTC #15 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to Jeff King

Hi,

Quoted 3 lines · Jeff King

I had a vague notion that there was some reason (portability?) that we preferred to have the wrappers. But as your patch shows, they really are just calling "test" and nothing else.

Let's also not forget about the fact that test -f is actually not all that intuitive an interface. Whereas even somebody without training in software development (let alone Unix shell scripting) understands the meaning of

test_path_is_file this-file.txt

And even for a trained eye, the trace of test -f is sometimes hard to read, as you do *not* see the exit code in the trace, so you have to guess from circumstantial evidence whether it failed or succeeded.

Ciao,
Dscho

Rohit Ashiwal

2019-02-26 20:02 UTC #23 lore raw

Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)

In reply to Johannes Schindelin

Hey!

Quoted 4 lines · Johannes Schindelin

We already have test_path_is_missing. Why not use that instead of ! test -d or ! test -f?

Yes, I think this is better. It will satisfy all the requirements I guess.

Ciao
Rohit

Johannes Schindelin

2019-02-26 20:03 UTC #29 lore raw

Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file

In reply to Rohit Ashiwal

Hi Rohit,

Quoted 5 lines · Rohit Ashiwal

Quoted 3 lines · SZEDER Gábor
Furthermore, there are a couple of place where the '!' is not in front of the whole 'test' command but is given as an argument, e.g.:

Quoted 8 lines

Quoted 3 lines
test ! -f file
Please convert those cases as well.
I think since I'm modifying test_path_is_{dir|file} functions to handle calls like ! test_path_is_dir well as mentioned in this thread[1]. I think we should replace ! test calls with test !, so that the changes are in agreement with each other. What do you say?

I think what Gábor meant was that both test ! -f file and ! test -f file should be converted to test_path_is_missing file.

Ciao,
Johannes

Rohit Ashiwal

2019-02-26 20:06 UTC #30 lore raw

Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file

Hi Johannes

Quoted 4 lines · Johannes Schindelin

I think what Gábor meant was that both test ! -f file and ! test -f file should be converted to test_path_is_missing file.

I'll work over this and submit the patch.

Thanks for clarifying
Rohit

Jeff King

2019-02-26 21:01 UTC #7 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to SZEDER Gábor

t/test-lib-functions.sh list_contains () { · line 675

Quoted 12 lines · SZEDER Gábor

Quoted 6 lines
Quoted 4 lines
I didn't find this to be an issue, but because of functions like 'test_seq' and 'test_must_fail' I've thought about suppressing '-x' output for test helpers (haven't actually done anything about it, though).
I'd be curious how you'd do that.
Well, I started replying with "Dunno" and explaining why I don't think that it can be done with 'test_must_fail'... but then got a bit of a lightbulb moment. Now look at this:
[...]
t/test-lib-functions.sh +1 -0
+	{ set +x ; } 2>/dev/null 4>/dev/null

Ah, this is the magic. Doing:

set +x 2>/dev/null

will still show it, but doing the redirection in a wrapping block means that it is applied before the command inside the block is run. Clever.

I think this braces trick could be used in general to fix all of the remaining "you can't run this under -x" cases, though it might be ugly. It might also be possible to make test_eval_ a bit less subtle with it, though I think it is relying on the braces already (which makes me wonder if I just totally forgot about its existence today, or if I earlier somehow stumbled onto a working recipe because I wanted to run multiple redirected commands).

Quoted 10 lines

There are a couple of tricky cases:
- Some test helper functions call other test helper functions, and in those cases tracing would be enabled upon returning from the inner helper function. This is not an issue with e.g. 'test_might_fail' or 'test_cmp_config', because the inner helper function is the last command anyway. However, there is 'test_must_be_empty', 'test_dir_is_empty', 'test_config', 'test_commit', etc. which call the other test helper functions right at the start or in the middle.

Yeah, this is inherently a global flag that we're playing games with. It does seem like it would be easy to get it wrong. I guess the right model is considering it like a stack, like:

-- >8 --
#!/bin/sh

x_counter=0

pop_x() {
	ret=$?
	case "$x_counter" in
	0)
		echo >&2 "BUG: too many pops"
		exit 1
		;;
	1)
		x_counter=0
		set -x
		;;
	*)
		x_counter=$((x_counter - 1))
		;;
	esac
	{ return $ret; } 2>/dev/null
}

# you _must_ call this as "{ push_x; } 2>/dev/null" to avoid polluting
# trace output with the push call
push_x() {
	set +x 2>/dev/null
	x_counter=$((x_counter + 1))
}

bar() {
	{ push_x; } 2>/dev/null
	echo in bar
	pop_x
}

foo() {
{ push_x; } 2>/dev/null
echo in foo, before bar
bar
echo in foo, after bar
false
pop_x
}

set -x
foo
echo \$? is $?
-- 8< --

I wish there was a way to avoid having to do the block-and-redirect in the push_x calls in each function, though.

I dunno. I do like the output, but this is rapidly getting complex.

Quoted 2 lines

- && chains in test helper functions; we must make sure that the tracing is restored even in case of a failure.

Yeah, there is no "goto out" to help give a common exit point from the function. You could probably do it with a wrapper, like:

  foo() {
	{ push_x; } 2>/dev/null
	real_foo "$@"
	pop_x

}

and then real_foo() is free to return however it likes. I wonder if you could even wrap that up in a helper:

  disable_function_tracing () {
	# rename foo() to orig_foo(); this works in bash, but I'm not
	# sure if there's a portable way to do it (and ideally one that
	# wouldn't involve an extra process).
	eval "real_$1 () $(declare -f $1 | tail -n +2)"

	# and then install a wrapper which pushes/pops tracing
	eval "$1 () { { push_x; } 2>/dev/null; real_$1 \"\$@\"; pop_x; }"

}

foo () { .... }
disable_function_tracing foo

It would be easier if you could just declare the function body as an argument (and then it would be "declare_untraceable_function", where you do it all in one step). But then the function body has to be in single quotes, which is a pain. I think this is definitely pushing the limits of portable shell (and quite possibly the limits of good taste).

-Peff

Jeff King

2019-02-26 21:02 UTC #16 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to Johannes Schindelin

Quoted 11 lines · Johannes Schindelin

Quoted 3 lines · Jeff King
I had a vague notion that there was some reason (portability?) that we preferred to have the wrappers. But as your patch shows, they really are just calling "test" and nothing else.
Let's also not forget about the fact that test -f is actually not all that intuitive an interface. Whereas even somebody without training in software development (let alone Unix shell scripting) understands the meaning of

Quoted 5 lines

test_path_is_file this-file.txt
And even for a trained eye, the trace of test -f is sometimes hard to read, as you do *not* see the exit code in the trace, so you have to guess from circumstantial evidence whether it failed or succeeded.

True. For old-timers, I think "test -f" is idiomatic, but that is not true for everyone. Sometimes wrappers can cut both ways, making things harder for people who are used to the idioms. But "test_path_is_file" should be pretty readable for everyone, old and new alike, I would think.

-Peff

Rohit Ashiwal via GitGitGadget

author

2019-02-26 22:49 UTC #31 lore raw

v3 cover t3600: use test_path_is_* helper functions

In reply to Rohit Ashiwal via GitGitGadget

Replace test -(d|f|e) calls in t3600-rm.sh. Previously we were using test -(d|f|e) to verify the presence of a directory/file, but we already have helper functions, viz, test_path_is_dir, test_path_is_file and test_path_is_missing with better functionality.

Rohit Ashiwal (1):
t3600: use test_path_is_* functions

 t/t3600-rm.sh | 138 +++++++++++++++++++++++++-------------------------
 1 file changed, 69 insertions(+), 69 deletions(-)

base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-152%2Fr1walz%2Frefactor-tests-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-152/r1walz/refactor-tests-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/152

Range-diff vs v2:

1: fcafc87b38 ! 1: bfeba25c88 t3600: use test_path_is_dir and test_path_is_file

     @@ -1,12 +1,14 @@
      Author: Rohit Ashiwal <[email protected]>
      
     -    t3600: use test_path_is_dir and test_path_is_file
     +    t3600: use test_path_is_* functions
      
     -    Previously we were using `test -(d|f)` to verify
     +    Replace `test -(d|f|e)` calls in t3600-rm.sh
     +
     +    Previously we were using `test -(d|f|e)` to verify
          the presence of a directory/file, but we already
     -    have helper functions, viz, `test_path_is_dir`
     -    and `test_path_is_file` with better functionality.
     -    This patch will replace `test -(d|f)` calls in t3660.sh
     +    have helper functions, viz, `test_path_is_dir`,
     +    `test_path_is_file` and `test_path_is_missing`
     +    with better functionality.
      
          These helper functions make code more readable
          and informative to someone new to code, also
     @@ -18,6 +20,15 @@
       --- a/t/t3600-rm.sh
       +++ b/t/t3600-rm.sh
      @@
     + 
     + test_expect_success \
     +     'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
     +-    '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'
     ++    'test_path_is_missing bar && test_must_fail git ls-files --error-unmatch bar'
     + 
     + test_expect_success \
     +     'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \
     +@@
       test_expect_success 'Modify foo -- rm should refuse' '
       	echo >>foo &&
       	test_must_fail git rm foo baz &&
     @@ -28,6 +39,15 @@
       	git ls-files --error-unmatch foo baz
       '
       
     + test_expect_success 'Modified foo -- rm -f should work' '
     + 	git rm -f foo baz &&
     +-	test ! -f foo &&
     +-	test ! -f baz &&
     ++	test_path_is_missing foo &&
     ++	test_path_is_missing baz &&
     + 	test_must_fail git ls-files --error-unmatch foo &&
     + 	test_must_fail git ls-files --error-unmatch bar
     + '
      @@
       
       test_expect_success 'foo is different in index from HEAD -- rm should refuse' '
     @@ -39,6 +59,15 @@
       	git ls-files --error-unmatch foo baz
       '
       
     + test_expect_success 'but with -f it should work.' '
     + 	git rm -f foo baz &&
     +-	test ! -f foo &&
     +-	test ! -f baz &&
     ++	test_path_is_missing foo &&
     ++	test_path_is_missing baz &&
     + 	test_must_fail git ls-files --error-unmatch foo &&
     + 	test_must_fail git ls-files --error-unmatch baz
     + '
      @@
       
       test_expect_success 'Recursive without -r fails' '
     @@ -62,20 +91,56 @@
       	git rm -f -r frotz &&
      -	! test -f frotz/nitfol &&
      -	! test -d frotz
     -+	! test_path_is_file frotz/nitfol &&
     -+	! test_path_is_dir frotz
     ++	test_path_is_missing frotz/nitfol &&
     ++	test_path_is_missing frotz
       '
       
       test_expect_success 'Remove nonexistent file returns nonzero exit status' '
     +@@
     + 	git reset --hard &&
     + 	test-tool chmtime -86400 frotz/nitfol &&
     + 	git rm frotz/nitfol &&
     +-	test ! -f frotz/nitfol
     ++	test_path_is_missing frotz/nitfol
     + 
     + '
     + 
      @@
       	echo content >dir/subdir/subsubdir/file &&
       	git add dir/subdir/subsubdir/file &&
       	git rm -f dir/subdir/subsubdir/file &&
      -	! test -d dir
     -+	! test_path_is_dir dir
     ++	test_path_is_missing dir
       '
       
       cat >expect <<EOF
     +@@
     + 	git add .gitmodules &&
     + 	git commit -m "add submodule" &&
     + 	git rm submod &&
     +-	test ! -e submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual &&
     + 	test_must_fail git config -f .gitmodules submodule.sub.url &&
     +@@
     + 	git reset --hard &&
     + 	git submodule update &&
     + 	git rm submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual &&
     + 	test_must_fail git config -f .gitmodules submodule.sub.url &&
     +@@
     + 	git reset --hard &&
     + 	git submodule update &&
     + 	git rm submod/ &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual
     + '
      @@
       	git submodule update &&
       	git -C submod checkout HEAD^ &&
     @@ -87,6 +152,11 @@
       	git status -s -uno --ignore-submodules=none >actual &&
       	test_cmp expect.modified actual &&
       	git rm -f submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual &&
     + 	test_must_fail git config -f .gitmodules submodule.sub.url &&
      @@
       	git reset --hard &&
       	git submodule update &&
     @@ -113,8 +183,8 @@
       	test_must_be_empty actual.err &&
      -	! test -d submod &&
      -	! test -f submod/.git &&
     -+	! test_path_is_dir submod &&
     -+	! test_path_is_file submod/.git &&
     ++	test_path_is_missing submod &&
     ++	test_path_is_missing submod/.git &&
       	git status -s -uno >actual &&
       	test_cmp expect.both_deleted actual
       '
     @@ -132,8 +202,8 @@
       	test_must_be_empty actual.err &&
      -	! test -d submod &&
      -	! test -f submod/.git &&
     -+	! test_path_is_dir submod &&
     -+	! test_path_is_file submod/.git &&
     ++	test_path_is_missing submod &&
     ++	test_path_is_missing submod/.git &&
       	git status -s -uno >actual &&
       	test_cmp expect actual
       '
     @@ -143,8 +213,8 @@
       	test_i18ncmp expect.err actual.err &&
      -	! test -d submod &&
      -	! test -f submod/.git &&
     -+	! test_path_is_dir submod &&
     -+	! test_path_is_file submod/.git &&
     ++	test_path_is_missing submod &&
     ++	test_path_is_missing submod/.git &&
       	git status -s -uno >actual &&
       	test_cmp expect actual
       '
     @@ -159,6 +229,11 @@
       	git status -s -uno --ignore-submodules=none >actual &&
       	test_cmp expect.modified_inside actual &&
       	git rm -f submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual
     + '
      @@
       	git submodule update &&
       	echo X >submod/untracked &&
     @@ -170,6 +245,20 @@
       	git status -s -uno --ignore-submodules=none >actual &&
       	test_cmp expect.modified_untracked actual &&
       	git rm -f submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual
     + '
     +@@
     + 	git submodule update &&
     + 	test_must_fail git merge conflict2 &&
     + 	git rm submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual
     + '
      @@
       	git -C submod checkout HEAD^ &&
       	test_must_fail git merge conflict2 &&
     @@ -181,6 +270,11 @@
       	git status -s -uno --ignore-submodules=none >actual &&
       	test_cmp expect.conflict actual &&
       	git rm -f submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual &&
     + 	test_must_fail git config -f .gitmodules submodule.sub.url &&
      @@
       	echo X >submod/empty &&
       	test_must_fail git merge conflict2 &&
     @@ -192,6 +286,11 @@
       	git status -s -uno --ignore-submodules=none >actual &&
       	test_cmp expect.conflict actual &&
       	git rm -f submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual &&
     + 	test_must_fail git config -f .gitmodules submodule.sub.url &&
      @@
       	echo X >submod/untracked &&
       	test_must_fail git merge conflict2 &&
     @@ -203,6 +302,11 @@
       	git status -s -uno --ignore-submodules=none >actual &&
       	test_cmp expect.conflict actual &&
       	git rm -f submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual
     + '
      @@
       	) &&
       	test_must_fail git merge conflict2 &&
     @@ -221,18 +325,36 @@
       	git status -s -uno --ignore-submodules=none >actual &&
       	test_cmp expect.conflict actual &&
       	git merge --abort &&
     +@@
     + 	git reset --hard &&
     + 	test_must_fail git merge conflict2 &&
     + 	git rm submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual
     + '
      @@
       		rm -r ../.git/modules/sub
       	) &&
       	git rm submod 2>output.err &&
      -	! test -d submod &&
      -	! test -d submod/.git &&
     -+	! test_path_is_dir submod &&
     -+	! test_path_is_dir submod/.git &&
     ++	test_path_is_missing submod &&
     ++	test_path_is_missing submod/.git &&
       	git status -s -uno --ignore-submodules=none >actual &&
       	test -s actual &&
       	test_i18ngrep Migrating output.err
      @@
     + 
     + test_expect_success 'rm recursively removes work tree of unmodified submodules' '
     + 	git rm submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual
     + '
     +@@
       	git submodule update --recursive &&
       	git -C submod/subsubmod checkout HEAD^ &&
       	test_must_fail git rm submod &&
     @@ -243,6 +365,11 @@
       	git status -s -uno --ignore-submodules=none >actual &&
       	test_cmp expect.modified_inside actual &&
       	git rm -f submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual
     + '
      @@
       	git submodule update --recursive &&
       	echo X >submod/subsubmod/empty &&
     @@ -254,6 +381,11 @@
       	git status -s -uno --ignore-submodules=none >actual &&
       	test_cmp expect.modified_inside actual &&
       	git rm -f submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual
     + '
      @@
       	git submodule update --recursive &&
       	echo X >submod/subsubmod/untracked &&
     @@ -265,14 +397,19 @@
       	git status -s -uno --ignore-submodules=none >actual &&
       	test_cmp expect.modified_untracked actual &&
       	git rm -f submod &&
     +-	test ! -d submod &&
     ++	test_path_is_missing submod &&
     + 	git status -s -uno --ignore-submodules=none >actual &&
     + 	test_cmp expect actual
     + '
      @@
       		GIT_WORK_TREE=. git config --unset core.worktree
       	) &&
       	git rm submod 2>output.err &&
      -	! test -d submod &&
      -	! test -d submod/subsubmod/.git &&
     -+	! test_path_is_dir submod &&
     -+	! test_path_is_dir submod/subsubmod/.git &&
     ++	test_path_is_missing submod &&
     ++	test_path_is_missing submod/subsubmod/.git &&
       	git status -s -uno --ignore-submodules=none >actual &&
       	test -s actual &&
       	test_i18ngrep Migrating output.err

Signature · 1 line

gitgitgadget

Rohit Ashiwal via GitGitGadget

author

2019-02-26 22:49 UTC #32 lore raw

PATCH v3 1/1 t3600: use test_path_is_* functions

From: Rohit Ashiwal <[email protected]>

Replace test -(d|f|e) calls in t3600-rm.sh

Previously we were using test -(d|f|e) to verify the presence of a directory/file, but we already have helper functions, viz, test_path_is_dir, test_path_is_file and test_path_is_missing with better functionality.

These helper functions make code more readable and informative to someone new to code, also these functions have better error messages

Signed-off-by Rohit Ashiwal <[email protected]>

---
 t/t3600-rm.sh | 138 +++++++++++++++++++++++++-------------------------
 1 file changed, 69 insertions(+), 69 deletions(-)

t/t3600-rm.sh +69 ~~-69~~

index 04e5d42bd3..ad638490ac 100755
@@ -83,7 +83,7 @@ test_expect_success \
 
 test_expect_success \
     'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
-    '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'
+    'test_path_is_missing bar && test_must_fail git ls-files --error-unmatch bar'
 
 test_expect_success \
     'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \

@@ -137,15 +137,15 @@ test_expect_success 'Re-add foo and baz' '
 test_expect_success 'Modify foo -- rm should refuse' '
 	echo >>foo &&
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '
 
 test_expect_success 'Modified foo -- rm -f should work' '
 	git rm -f foo baz &&
-	test ! -f foo &&
-	test ! -f baz &&
+	test_path_is_missing foo &&
+	test_path_is_missing baz &&
 	test_must_fail git ls-files --error-unmatch foo &&
 	test_must_fail git ls-files --error-unmatch bar
 '

@@ -159,15 +159,15 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '
 
 test_expect_success 'foo is different in index from HEAD -- rm should refuse' '
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '
 
 test_expect_success 'but with -f it should work.' '
 	git rm -f foo baz &&
-	test ! -f foo &&
-	test ! -f baz &&
+	test_path_is_missing foo &&
+	test_path_is_missing baz &&
 	test_must_fail git ls-files --error-unmatch foo &&
 	test_must_fail git ls-files --error-unmatch baz
 '

@@ -194,21 +194,21 @@ test_expect_success 'Recursive test setup' '
 
 test_expect_success 'Recursive without -r fails' '
 	test_must_fail git rm frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r but dirty' '
 	echo qfwfq >>frotz/nitfol &&
 	test_must_fail git rm -r frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r -f' '
 	git rm -f -r frotz &&
-	! test -f frotz/nitfol &&
-	! test -d frotz
+	test_path_is_missing frotz/nitfol &&
+	test_path_is_missing frotz
 '
 
 test_expect_success 'Remove nonexistent file returns nonzero exit status' '

@@ -232,7 +232,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
 	git reset --hard &&
 	test-tool chmtime -86400 frotz/nitfol &&
 	git rm frotz/nitfol &&
-	test ! -f frotz/nitfol
+	test_path_is_missing frotz/nitfol
 
 '

@@ -254,7 +254,7 @@ test_expect_success 'rm removes subdirectories recursively' '
 	echo content >dir/subdir/subsubdir/file &&
 	git add dir/subdir/subsubdir/file &&
 	git rm -f dir/subdir/subsubdir/file &&
-	! test -d dir
+	test_path_is_missing dir
 '
 
 cat >expect <<EOF

@@ -292,7 +292,7 @@ test_expect_success 'rm removes empty submodules from work tree' '
 	git add .gitmodules &&
 	git commit -m "add submodule" &&
 	git rm submod &&
-	test ! -e submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&

@@ -314,7 +314,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&

@@ -325,7 +325,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '
 	git reset --hard &&
 	git submodule update &&
 	git rm submod/ &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -343,12 +343,12 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git submodule update &&
 	git -C submod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&

@@ -359,8 +359,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g
 	git reset --hard &&
 	git submodule update &&
 	git rm --cached submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.cached actual &&
 	git config -f .gitmodules submodule.sub.url &&

@@ -371,7 +371,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm -n submod &&
-	test -f submod/.git &&
+	test_path_is_file submod/.git &&
 	git diff-index --exit-code HEAD
 '

@@ -381,8 +381,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '
 	git rm .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.both_deleted actual
 '

@@ -393,14 +393,14 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta
 	git config -f .gitmodules foo.bar true &&
 	test_must_fail git rm submod >actual 2>actual.err &&
 	test -s actual.err &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git diff-files --quiet -- submod &&
 	git add .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '

@@ -413,8 +413,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule
 	echo "warning: Could not find section in .gitmodules where path=submod" >expect.err &&
 	git rm submod >actual 2>actual.err &&
 	test_i18ncmp expect.err actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '

@@ -424,12 +424,12 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	git submodule update &&
 	echo X >submod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -439,12 +439,12 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	git submodule update &&
 	echo X >submod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -481,7 +481,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '
 	git submodule update &&
 	test_must_fail git merge conflict2 &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -493,12 +493,12 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git -C submod checkout HEAD^ &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&

@@ -512,12 +512,12 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
 	echo X >submod/empty &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&

@@ -531,12 +531,12 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files
 	echo X >submod/untracked &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -552,13 +552,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director
 	) &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git merge --abort &&

@@ -570,7 +570,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
 	git reset --hard &&
 	test_must_fail git merge conflict2 &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -586,8 +586,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates
 		rm -r ../.git/modules/sub
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test -s actual &&
 	test_i18ngrep Migrating output.err

@@ -614,7 +614,7 @@ test_expect_success 'setup subsubmodule' '
 
 test_expect_success 'rm recursively removes work tree of unmodified submodules' '
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -624,12 +624,12 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	git submodule update --recursive &&
 	git -C submod/subsubmod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -639,12 +639,12 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -654,12 +654,12 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -673,8 +673,8 @@ test_expect_success "rm absorbs submodule's nested .git directory" '
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/subsubmod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/subsubmod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test -s actual &&
 	test_i18ngrep Migrating output.err

Signature · 1 line

gitgitgadget

Martin Ågren

2019-02-27 05:49 UTC #24 lore raw

Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)

In reply to Rohit Ashiwal

Quoted 11 lines · Rohit Ashiwal

Hey!
Quoted 4 lines · Johannes Schindelin
We already have test_path_is_missing. Why not use that instead of ! test -d or ! test -f?
Yes, I think this is better. It will satisfy all the requirements I guess.

Good suggestion, Johannes. That is probably what most (all) of these wanted to express.

Martin

Duy Nguyen

2019-02-27 10:02 UTC #17 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to Ævar Arnfjörð Bjarmason

Quoted 18 lines · Ævar Arnfjörð Bjarmason

Quoted 5 lines · Duy Nguyen
Quoted 3 lines · Rohit Ashiwal via GitGitGadget
From: Rohit Ashiwal <[email protected]>
Quoted 8 lines
Quoted 4 lines
t3600-rm.sh: Previously we were using test -(d|f) to verify the presencee of a directory/file, but we already have helper functions, viz, test_path_is_dir and test_path_is_file with same functionality. This
It's not just the same (no point replacing then). It's better. When test_path_is_xxx fails, you get an error message. If "test -xxx" fails, you get a failed test with no clue what caused it.

Quoted 6 lines

I swear I'm not just on a mission to ruin everyone's GSOC projects. This patch definitely looks good, and given that we have this / document it makes sense.
However. I wonder in general if we've re-visited the utility of these wrappers and maybe other similar wrappers after -x was added.

It's personal, but every time I have to use -x I curse a little. It's just often too much to read.

Besides what people have already said, there's another good potential for test_path_is_file and friends. You can make it support multiple arguments, so that you can check if many paths are file with just one line. Used properly, this could reduce repetition and shorten some test cases a bit without sacrificing readability.

Signature · 1 line

Duy

Duy Nguyen

2019-02-27 10:12 UTC #33 lore raw

Re: [PATCH v3 1/1] t3600: use test_path_is_* functions

In reply to Rohit Ashiwal via GitGitGadget

Quoted 25 lines · Rohit Ashiwal via GitGitGadget

From: Rohit Ashiwal <[email protected]>
Replace test -(d|f|e) calls in t3600-rm.sh
Previously we were using test -(d|f|e) to verify the presence of a directory/file, but we already have helper functions, viz, test_path_is_dir, test_path_is_file and test_path_is_missing with better functionality.
These helper functions make code more readable and informative to someone new to code, also these functions have better error messages
Signed-off-by Rohit Ashiwal <[email protected]>
---
 t/t3600-rm.sh | 138 +++++++++++++++++++++++++-------------------------
 1 file changed, 69 insertions(+), 69 deletions(-)
t/t3600-rm.sh +0 -0
index 04e5d42bd3..ad638490ac 100755

t/t3600-rm.sh test_expect_success \ · line 86

Quoted 5 lines

t/t3600-rm.sh +1 -1

@@ -83,7 +83,7 @@ test_expect_success \
 
 test_expect_success \
     'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
-    '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'
+    'test_path_is_missing bar && test_must_fail git ls-files --error-unmatch bar'

This line should be broken down in two. It was reasonably short before, but now getting long and two checks in one line seem easy to miss.

I was a bit worried that the "test ! something" could be incorrectly converted because for example, "test ! -d foo" is not always the same as "test_path_is_missing". If "foo" is intended to be a file, then the conversion is wrong.

But I don't think you made any wrong conversion here. All these negative "test" are preceded by "git rm" so the expectation is always "test ! -e".

Signature · 1 line

Duy

Rohit Ashiwal via GitGitGadget

author

2019-02-28 10:26 UTC #34 lore raw

v4 cover t3600: use test_path_is_* helper functions

In reply to Rohit Ashiwal via GitGitGadget

Rohit Ashiwal (1):
t3600: use test_path_is_* functions

 t/t3600-rm.sh | 160 ++++++++++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 76 deletions(-)

base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-152%2Fr1walz%2Frefactor-tests-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-152/r1walz/refactor-tests-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/152

Range-diff vs v3:

1: bfeba25c88 ! 1: f881f01e4f t3600: use test_path_is_* functions

     @@ -20,11 +20,48 @@
       --- a/t/t3600-rm.sh
       +++ b/t/t3600-rm.sh
      @@
     + "
       
       test_expect_success \
     -     'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
     +-    'Pre-check that foo exists and is in index before git rm foo' \
     +-    '[ -f foo ] && git ls-files --error-unmatch foo'
     ++    'Pre-check that foo exists and is in index before git rm foo' '
     ++	 test_path_is_file foo &&
     ++	 git ls-files --error-unmatch foo
     ++'
     + 
     + test_expect_success \
     +     'Test that git rm foo succeeds' \
     +@@
     +      git rm --cached -f foo'
     + 
     + test_expect_success \
     +-    'Post-check that foo exists but is not in index after git rm foo' \
     +-    '[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'
     ++    'Post-check that foo exists but is not in index after git rm foo' '
     ++	 test_path_is_file foo &&
     ++	 test_must_fail git ls-files --error-unmatch foo
     ++'
     + 
     + test_expect_success \
     +-    'Pre-check that bar exists and is in index before "git rm bar"' \
     +-    '[ -f bar ] && git ls-files --error-unmatch bar'
     ++    'Pre-check that bar exists and is in index before "git rm bar"' '
     ++	 test_path_is_file bar &&
     ++	 git ls-files --error-unmatch bar
     ++'
     + 
     + test_expect_success \
     +     'Test that "git rm bar" succeeds' \
     +     'git rm bar'
     + 
     + test_expect_success \
     +-    'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
      -    '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'
     -+    'test_path_is_missing bar && test_must_fail git ls-files --error-unmatch bar'
     ++    'Post-check that bar does not exist and is not in index after "git rm -f bar"' '
     ++	 test_path_is_missing bar &&
     ++	 test_must_fail git ls-files --error-unmatch bar
     ++'
       
       test_expect_success \
           'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \

Signature · 1 line

gitgitgadget

Rohit Ashiwal via GitGitGadget

author

2019-02-28 10:26 UTC #35 lore raw

PATCH v4 1/1 t3600: use test_path_is_* functions

From: Rohit Ashiwal <[email protected]>

Replace test -(d|f|e) calls in t3600-rm.sh

These helper functions make code more readable and informative to someone new to code, also these functions have better error messages

Signed-off-by Rohit Ashiwal <[email protected]>

---
 t/t3600-rm.sh | 160 ++++++++++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 76 deletions(-)

t/t3600-rm.sh +84 ~~-76~~

index 04e5d42bd3..3a5bd97df7 100755
@@ -27,8 +27,10 @@ embedded' &&
 "
 
 test_expect_success \
-    'Pre-check that foo exists and is in index before git rm foo' \
-    '[ -f foo ] && git ls-files --error-unmatch foo'
+    'Pre-check that foo exists and is in index before git rm foo' '
+	 test_path_is_file foo &&
+	 git ls-files --error-unmatch foo
+'
 
 test_expect_success \
     'Test that git rm foo succeeds' \

@@ -70,20 +72,26 @@ test_expect_success \
      git rm --cached -f foo'
 
 test_expect_success \
-    'Post-check that foo exists but is not in index after git rm foo' \
-    '[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'
+    'Post-check that foo exists but is not in index after git rm foo' '
+	 test_path_is_file foo &&
+	 test_must_fail git ls-files --error-unmatch foo
+'
 
 test_expect_success \
-    'Pre-check that bar exists and is in index before "git rm bar"' \
-    '[ -f bar ] && git ls-files --error-unmatch bar'
+    'Pre-check that bar exists and is in index before "git rm bar"' '
+	 test_path_is_file bar &&
+	 git ls-files --error-unmatch bar
+'
 
 test_expect_success \
     'Test that "git rm bar" succeeds' \
     'git rm bar'
 
 test_expect_success \
-    'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
-    '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'
+    'Post-check that bar does not exist and is not in index after "git rm -f bar"' '
+	 test_path_is_missing bar &&
+	 test_must_fail git ls-files --error-unmatch bar
+'
 
 test_expect_success \
     'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \

@@ -137,15 +145,15 @@ test_expect_success 'Re-add foo and baz' '
 test_expect_success 'Modify foo -- rm should refuse' '
 	echo >>foo &&
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '
 
 test_expect_success 'Modified foo -- rm -f should work' '
 	git rm -f foo baz &&
-	test ! -f foo &&
-	test ! -f baz &&
+	test_path_is_missing foo &&
+	test_path_is_missing baz &&
 	test_must_fail git ls-files --error-unmatch foo &&
 	test_must_fail git ls-files --error-unmatch bar
 '

@@ -159,15 +167,15 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '
 
 test_expect_success 'foo is different in index from HEAD -- rm should refuse' '
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '
 
 test_expect_success 'but with -f it should work.' '
 	git rm -f foo baz &&
-	test ! -f foo &&
-	test ! -f baz &&
+	test_path_is_missing foo &&
+	test_path_is_missing baz &&
 	test_must_fail git ls-files --error-unmatch foo &&
 	test_must_fail git ls-files --error-unmatch baz
 '

@@ -194,21 +202,21 @@ test_expect_success 'Recursive test setup' '
 
 test_expect_success 'Recursive without -r fails' '
 	test_must_fail git rm frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r but dirty' '
 	echo qfwfq >>frotz/nitfol &&
 	test_must_fail git rm -r frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r -f' '
 	git rm -f -r frotz &&
-	! test -f frotz/nitfol &&
-	! test -d frotz
+	test_path_is_missing frotz/nitfol &&
+	test_path_is_missing frotz
 '
 
 test_expect_success 'Remove nonexistent file returns nonzero exit status' '

@@ -232,7 +240,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
 	git reset --hard &&
 	test-tool chmtime -86400 frotz/nitfol &&
 	git rm frotz/nitfol &&
-	test ! -f frotz/nitfol
+	test_path_is_missing frotz/nitfol
 
 '

@@ -254,7 +262,7 @@ test_expect_success 'rm removes subdirectories recursively' '
 	echo content >dir/subdir/subsubdir/file &&
 	git add dir/subdir/subsubdir/file &&
 	git rm -f dir/subdir/subsubdir/file &&
-	! test -d dir
+	test_path_is_missing dir
 '
 
 cat >expect <<EOF

@@ -292,7 +300,7 @@ test_expect_success 'rm removes empty submodules from work tree' '
 	git add .gitmodules &&
 	git commit -m "add submodule" &&
 	git rm submod &&
-	test ! -e submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&

@@ -314,7 +322,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&

@@ -325,7 +333,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '
 	git reset --hard &&
 	git submodule update &&
 	git rm submod/ &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -343,12 +351,12 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git submodule update &&
 	git -C submod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&

@@ -359,8 +367,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g
 	git reset --hard &&
 	git submodule update &&
 	git rm --cached submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.cached actual &&
 	git config -f .gitmodules submodule.sub.url &&

@@ -371,7 +379,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm -n submod &&
-	test -f submod/.git &&
+	test_path_is_file submod/.git &&
 	git diff-index --exit-code HEAD
 '

@@ -381,8 +389,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '
 	git rm .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.both_deleted actual
 '

@@ -393,14 +401,14 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta
 	git config -f .gitmodules foo.bar true &&
 	test_must_fail git rm submod >actual 2>actual.err &&
 	test -s actual.err &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git diff-files --quiet -- submod &&
 	git add .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '

@@ -413,8 +421,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule
 	echo "warning: Could not find section in .gitmodules where path=submod" >expect.err &&
 	git rm submod >actual 2>actual.err &&
 	test_i18ncmp expect.err actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '

@@ -424,12 +432,12 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	git submodule update &&
 	echo X >submod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -439,12 +447,12 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	git submodule update &&
 	echo X >submod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -481,7 +489,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '
 	git submodule update &&
 	test_must_fail git merge conflict2 &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -493,12 +501,12 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git -C submod checkout HEAD^ &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&

@@ -512,12 +520,12 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
 	echo X >submod/empty &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&

@@ -531,12 +539,12 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files
 	echo X >submod/untracked &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -552,13 +560,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director
 	) &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git merge --abort &&

@@ -570,7 +578,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
 	git reset --hard &&
 	test_must_fail git merge conflict2 &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -586,8 +594,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates
 		rm -r ../.git/modules/sub
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test -s actual &&
 	test_i18ngrep Migrating output.err

@@ -614,7 +622,7 @@ test_expect_success 'setup subsubmodule' '
 
 test_expect_success 'rm recursively removes work tree of unmodified submodules' '
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -624,12 +632,12 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	git submodule update --recursive &&
 	git -C submod/subsubmod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -639,12 +647,12 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -654,12 +662,12 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '

@@ -673,8 +681,8 @@ test_expect_success "rm absorbs submodule's nested .git directory" '
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/subsubmod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/subsubmod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test -s actual &&
 	test_i18ngrep Migrating output.err

Signature · 1 line

gitgitgadget

Rohit Ashiwal

2019-02-28 19:03 UTC #36 lore raw

[GSoC] acknowledging mistakes

Hey people

I had a discussion with Rafael over the #git irc channel and Thanks to him I was able to find these minute mistakes:

1. Commit message was less than 50 chars which should be around 72 chars according to coding guide lines. Should I change this to match 72?

2. My changes had some uneven use of tabs and spaces, which I made considering that pre-existing code had them too. Is there a possibility to change the whole code according to CodingGuidelines? If yes should I only change my code according to guidelines or the whole file?

3. There is no helper function for test -s but Rafael suggested we can make use of other helper functions to provide similar functionality, if we can.

Open to suggestions and debate. These will be fixed in next revision accordingly.

Thanks
Rohit

Junio C Hamano

2019-03-01 02:51 UTC #37 lore raw

Re: [GSoC] acknowledging mistakes

Quoted 2 lines · Rohit Ashiwal

1. Commit message was less than 50 chars which should be around 72 chars according to coding guide lines. Should I change this to match 72?

Simple things do not need that many letters to tell ;-) The suggestion of 72 is about the maximum.

If you are doing something in a single patch that needs a longer title, it generally is a sign that you are trying to do too much in a single patch and should be splitting the patch into more digestable smaller steps. And the purpose of having a maximum is to nudge patch authors to realize that.

Quoted 5 lines

2. My changes had some uneven use of tabs and spaces, which I made considering that pre-existing code had them too. Is there a possibility to change the whole code according to CodingGuidelines? If yes should I only change my code according to guidelines or the whole file?

I think you are talking about t3600, which uses an ancient style. If this were a real project, then the preferred order would be

- A preliminary patch (or a series of patches) that modernizes existing tests in t3600. Just style updates and adding or removing nothing else.

- Update test that use "test -f" and friends to use the helpers in t3600.

Quoted 3 lines

3. There is no helper function for test -s but Rafael suggested we can make use of other helper functions to provide similar functionality, if we can.

If we often see if a path is an non-empty file in our tests (not limited to t3600), then it may make sense to add a new helper test_path_is_non_empty_file in t/test-lib-functions.sh next to where test_path_is_file and friends are defined.

Thanks.

[jch: I am still mostly offline til the next week, but I had a chance to sit in front of my mailbox long enough, so...]

Junio C Hamano

2019-03-01 02:53 UTC #18 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to Ævar Arnfjörð Bjarmason

Quoted 4 lines · Ævar Arnfjörð Bjarmason

I swear I'm not just on a mission to ruin everyone's GSOC projects. This patch definitely looks good, and given that we have this / document it makes sense.

Quoted 8 lines

However. I wonder in general if we've re-visited the utility of these wrappers and maybe other similar wrappers after -x was added.
Back when this was added in 2caf20c52b ("test-lib: user-friendly alternatives to test [-d|-f|-e]", 2010-08-10) we didn't have -x.
...
But 4 years after this was added in a136f6d8ff ("test-lib.sh: support -x option for shell-tracing", 2014-10-10) we got -x, and then with "-i -v -x":

I think two things need to be considered separately.

- Do the path-is-file and friends make the test source easier to read and undrstand? Special bonus if it helps us by making it harder to write a wrong test.

- Do these helpers make the output from the test execution easier to diagnose or harder?

If your primary compalint is the latter (which I think it is, and I share the same feeling to a certain degree), I think it is to throw the baby with bathwater to get rid of path-is-* family.

And as to the former question, I think we even are getting special bonus. Often when people write tests to ensure a fix that left an unwanted file behind would say "! test -f unwanted", but if we say "path-is-missing unwanted" that would catch not just a regular file but also catch other kinds of filesystem entities.

As to readablity, I do not think "test -f/-d" etc are unnecessary hard to read, but using path-is-* does not make it harder to read, so I'd say it would not give us much to revert to the bare "test -f" and friends.

Unless you are after squeezing the last cycle spent executing a shell builtin in the test scripts by using bare-bones "test -f", that is. But that is not among the two I said we need to consider separately, so I won't go there.

Thanks.

[jch: I am still mostly offline til the next week, but I had a chance to sit in front of my mailbox long enough, so...]

Rohit Ashiwal

2019-03-01 13:14 UTC #38 lore raw

Feeling confused a little bit

In reply to Junio C Hamano

Hey!

I'm a little confused as you never provide a clear indication to where shall I proceed? :-

Quoted 4 lines · On Fri, 01 Mar 2019 11:51:46 +0900 Junio C Hamano <[email protected]> wrote:

Simple things do not need that many letters to tell ;-) The suggestion of 72 is about the maximum.

Totally agree on this!

Quoted 11 lines

I think you are talking about t3600, which uses an ancient style. If this were a real project, then the preferred order would be
- A preliminary patch (or a series of patches) that modernizes existing tests in t3600. Just style updates and adding or removing nothing else.
- Update test that use "test -f" and friends to use the helpers in t3600.

Yes, this is a microproject after all. But I think I can work on this as if it were a real project, should I proceed according to this plan? (I have a lot of free time over this weekend)

Quoted 6 lines

If we often see if a path is an non-empty file in our tests (not limited to t3600), then it may make sense to add a new helper test_path_is_non_empty_file in t/test-lib-functions.sh next to where test_path_is_file and friends are defined.

Since my project does not deal with test-lib-functions.sh, I think I should not edit it anyway, but I'd be more than happy to add a new member to test_path_is_* family.

Thanks
Rohit

Rafael Ascensão

2019-03-02 04:24 UTC #39 lore raw

Re: Feeling confused a little bit

Quoted 7 lines · Rohit Ashiwal

Quoted 4 lines
Simple things do not need that many letters to tell ;-) The suggestion of 72 is about the maximum.
Totally agree on this!

I was bikeshedding the patch and mentioned that the commit message body is usually wrapped at 72 because I noticed you were wrapping the body at 50.

So to make things clear, when you're writing the subject, i.e. the first line, you should aim towards 50 and do not exceed 72.

The body, i.e. 3rd line until EOF is usually wrapped at 72.

There are exceptions, these are guidelines. Sometimes commits will break the first rule. (Merges are the most common example I can think of, but you won't be doing any as a contributor).
Pre-formatted content, like the output of a program, will break the second. Look at 3b41fb0cb217f4b4491f2e67ce4183e5d2a5d873 for an example.

But my nitpick wasn't necessarily because I didn't agree about the way you line wrapped the patch. It was about figuring out if you had a misconfigured editor (that could also be the cause of tabs and spaces mix), which you later mentioned was probably the case.

Cheers,
Rafael Ascensão

Thomas Gummerer

2019-03-02 14:46 UTC #40 lore raw

Re: Feeling confused a little bit

In reply to Rohit Ashiwal

Quoted 13 lines · Rohit Ashiwal

Hey!
I'm a little confused as you never provide a clear indication to where shall I proceed? :-
Quoted 7 lines · On Fri, 01 Mar 2019 11:51:46 +0900 Junio C Hamano <[email protected]> wrote:
I think you are talking about t3600, which uses an ancient style. If this were a real project, then the preferred order would be
- A preliminary patch (or a series of patches) that modernizes existing tests in t3600. Just style updates and adding or removing nothing else.

Quoted 7 lines

Quoted 3 lines
- Update test that use "test -f" and friends to use the helpers in t3600.
Yes, this is a microproject after all. But I think I can work on this as if it were a real project, should I proceed according to this plan? (I have a lot of free time over this weekend)

Yes, I think it would be good to make those changes, to try and get this merged. Having the microproject merged is not a requirement (its main purpose is to see how students communicate on the mailing list, and to get them familiar with the workflow ahead of GSoC), but it can be a nice achievement in itself.

That said, I would also encourage you to start thinking about a project proposal, as that is another important part that should be done for the application.

Quoted 10 lines

Quoted 6 lines
If we often see if a path is an non-empty file in our tests (not limited to t3600), then it may make sense to add a new helper test_path_is_non_empty_file in t/test-lib-functions.sh next to where test_path_is_file and friends are defined.
Since my project does not deal with test-lib-functions.sh, I think I should not edit it anyway, but I'd be more than happy to add a new member to test_path_is_* family.

It is up to you how far you would like to go with this. If you want to add the helper, to make further cleanups in t3600, I think that would be a good thing to do (after double checking that it would be useful in other test files as well), and should be done in a separate patch. Then you can use it in the same patch as using the helpers for "test -f" etc.

Quoted 2 lines

Thanks
Rohit

Rohit Ashiwal

2019-03-02 16:21 UTC #41 lore raw

[GSoC] Thanking

Hey! Thomas

Thank you for replying over my woes.

Quoted 8 lines

It is up to you how far you would like to go with this. If you want to add the helper, to make further cleanups in t3600, I think that would be a good thing to do (after double checking that it would be useful in other test files as well), and should be done in a separate patch. Then you can use it in the same patch as using the helpers for "test -f" etc.

I guess I should work on this one first. I checked and around 18 test files use test -s, it will be useful nonetheless.

Quoted 7 lines

Yes, I think it would be good to make those changes, to try and get this merged. Having the microproject merged is not a requirement (its main purpose is to see how students communicate on the mailing list, and to get them familiar with the workflow ahead of GSoC), but it can be a nice achievement in itself.

Yes, it is a nice experience to interact with people who "run" git over which most of the open source community depends for code sharing and collaboration.

Quoted 5 lines

That said, I would also encourage you to start thinking about a project proposal, as that is another important part that should be done for the application.

That is really encouraging, I'll try to finish my work as soon as possible and work on the proposal side by side!

Thanks
Rohit

SZEDER Gábor

2019-03-03 16:05 UTC #8 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to Jeff King

Quoted 71 lines · Jeff King

Quoted 7 lines · SZEDER Gábor
Quoted 4 lines
Quoted 4 lines
I didn't find this to be an issue, but because of functions like 'test_seq' and 'test_must_fail' I've thought about suppressing '-x' output for test helpers (haven't actually done anything about it, though).
There are a couple of tricky cases:
Quoted 8 lines
- Some test helper functions call other test helper functions, and in those cases tracing would be enabled upon returning from the inner helper function. This is not an issue with e.g. 'test_might_fail' or 'test_cmp_config', because the inner helper function is the last command anyway. However, there is 'test_must_be_empty', 'test_dir_is_empty', 'test_config', 'test_commit', etc. which call the other test helper functions right at the start or in the middle.
Yeah, this is inherently a global flag that we're playing games with. It does seem like it would be easy to get it wrong. I guess the right model is considering it like a stack, like:
-- >8 --
#!/bin/sh
x_counter=0
pop_x() {
	ret=$?
	case "$x_counter" in
	0)
		echo >&2 "BUG: too many pops"
		exit 1
		;;
	1)
		x_counter=0
		set -x
		;;
	*)
		x_counter=$((x_counter - 1))
		;;
	esac
	{ return $ret; } 2>/dev/null
}
# you _must_ call this as "{ push_x; } 2>/dev/null" to avoid polluting
# trace output with the push call
push_x() {
	set +x 2>/dev/null
	x_counter=$((x_counter + 1))
}
bar() {
	{ push_x; } 2>/dev/null
	echo in bar
	pop_x
}
foo() {
{ push_x; } 2>/dev/null
echo in foo, before bar
bar
echo in foo, after bar
false
pop_x
}
set -x
foo
echo \$? is $?
-- 8< --

Quoted 7 lines

I wish there was a way to avoid having to do the block-and-redirect in the push_x calls in each function, though.
I dunno. I do like the output, but this is rapidly getting complex.
Quoted 2 lines
- && chains in test helper functions; we must make sure that the tracing is restored even in case of a failure.

Actually, the && chain is not really an issue, because we can simply break the && chain at the very end:

  test_func () {
        { disable_tracing ; } 2>/dev/null 4>&2
        do this &&
        do that
        restore_tracing
  }

and make restore_tracing exit with $? (like you did above in pop_x()).

Quoted 2 lines

Yeah, there is no "goto out" to help give a common exit point from the function. You could probably do it with a wrapper, like:

Yeah, the wrapper works.
There are only a few test helper functions with multiple 'return' statements, and refactoring them to have a single 'return $ret' at the end worked, too.

Quoted 17 lines

  foo() {
	{ push_x; } 2>/dev/null
	real_foo "$@"
	pop_x

}

and then real_foo() is free to return however it likes. I wonder if you could even wrap that up in a helper:

  disable_function_tracing () {
	# rename foo() to orig_foo(); this works in bash, but I'm not
	# sure if there's a portable way to do it (and ideally one that
	# wouldn't involve an extra process).
	eval "real_$1 () $(declare -f $1 | tail -n +2)"

	# and then install a wrapper which pushes/pops tracing
	eval "$1 () { { push_x; } 2>/dev/null; real_$1 \"\$@\"; pop_x; }"

Quoted 4 lines

}
foo () { .... }
disable_function_tracing foo

We can wrap all functions at once:

  eval "$(declare -f \
                test_cmp \
                test_cmp_bin \
                <....> \
                write_script |
        sed -e 's%^\([a-zA-Z0-9_]*\) ()% \
                \1 () { \
                        { disable_tracing; } 2>/dev/null 4>/dev/null \
                        real_\1 \"\$@\" \
                        restore_tracing \
                } \
                real_\1 ()%')"

Yeah, not particularly pretty, but with the s/// command broken up into several lines it's not all that terrible either... And at least it doesn't need extra processes for each wrapped function.

We should also be careful and don't switch on tracing when returning from test helper functions invoked outside of tests, e.g. 'test_create_repo' while initializing the trash directory or 'test_set_port' while sourcing a daemon-specific lib.

Alas, 'declare' is Bash-only, and I don't see any way around that. Bummer.

On a mostly unrelated note, but I just noticed it while playing around with this: 't0000'-basic.sh' runs its internal tests with $SHELL_PATH instead of $TEST_SHELL_PATH. I'm not sure whether that's right or wrong.

SZEDER Gábor

2019-03-04 14:36 UTC #10 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to Jeff King

Quoted 8 lines · Jeff King

t/test-lib-functions.sh list_contains () { · line 675
Quoted 1 line
t/test-lib-functions.sh +1 -0
+	{ set +x ; } 2>/dev/null 4>/dev/null
Ah, this is the magic. Doing:
set +x 2>/dev/null
will still show it, but doing the redirection in a wrapping block means that it is applied before the command inside the block is run. Clever.

Yeah, clever, but unfortunately (and to me suprisingly) unportable:

  $ ksh
  $ set -x
  $ echo foo
  + echo foo
  foo
  $ set +x
  $

It doesn't show 'set +x', how convenient! :)
However:

  $ set -x
  $ echo foo 2>/dev/null
  + echo foo
  + 2> /dev/null
  foo
  $ { set +x; } 2>/dev/null
  + 2> /dev/null
  $

Apparently ksh, ksh93 and mksh show not only the executed commands but any redirections as well. It's already visible when running our tests with ksh and '-x':

  $ ksh ./t9999-test.sh -x
  Initialized empty Git repository in /home/szeder/src/git/t/trash directory.t9999-test/.git/
  expecting success: 
          true

+ true
+ 2> /dev/null ok 1 - first

  # passed all 1 test(s)
  1..1

NetBSD's sh:

  # set -x
  # echo foo
  + echo foo
  foo
  # echo foo 2>/dev/null
  + echo foo 2>/dev/null
  foo
  # { set +x; } 2>/dev/null
  + using redirections: 2>/dev/null do

Jeff King

2019-03-05 04:55 UTC #9 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to SZEDER Gábor

Quoted 6 lines · SZEDER Gábor

Quoted 2 lines
Quoted 2 lines
- && chains in test helper functions; we must make sure that the tracing is restored even in case of a failure.
Actually, the && chain is not really an issue, because we can simply break the && chain at the very end:

Quoted 8 lines

  test_func () {
        { disable_tracing ; } 2>/dev/null 4>&2
        do this &&
        do that
        restore_tracing
  }
and make restore_tracing exit with $? (like you did above in pop_x()).

Yeah, good point.

Quoted 7 lines

Quoted 2 lines
Yeah, there is no "goto out" to help give a common exit point from the function. You could probably do it with a wrapper, like:
Yeah, the wrapper works.
There are only a few test helper functions with multiple 'return' statements, and refactoring them to have a single 'return $ret' at the end worked, too.

Yeah, that might be less sneaky than this wrapper business. Or we could just do a few basic wrappers. The non-portable bit in my wrapper suggestion was the renaming of the old function. But if we accept just:

real_foo() {
... do stuff with multiple returns ...
}
disable_function_tracing real_foo foo

then that is pretty trivial to do with an eval. It does disallow your "wrap all functions at once", but I think that is OK. We might want to only do a subset anyway.

Quoted 4 lines

We should also be careful and don't switch on tracing when returning from test helper functions invoked outside of tests, e.g. 'test_create_repo' while initializing the trash directory or 'test_set_port' while sourcing a daemon-specific lib.

Yeah, it would probably make sense in the "push" half to check that we are actually tracing at that moment.

Quoted 4 lines

On a mostly unrelated note, but I just noticed it while playing around with this: 't0000'-basic.sh' runs its internal tests with $SHELL_PATH instead of $TEST_SHELL_PATH. I'm not sure whether that's right or wrong.

I'd say probably wrong, though it likely doesn't matter that much in practice.

-Peff

Jeff King

2019-03-05 04:58 UTC #11 lore raw

Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

In reply to SZEDER Gábor

Quoted 23 lines · SZEDER Gábor

Quoted 8 lines · Jeff King
t/test-lib-functions.sh list_contains () { · line 675
Quoted 1 line
t/test-lib-functions.sh +1 -0
+	{ set +x ; } 2>/dev/null 4>/dev/null
Ah, this is the magic. Doing:
set +x 2>/dev/null
will still show it, but doing the redirection in a wrapping block means that it is applied before the command inside the block is run. Clever.
Yeah, clever, but unfortunately (and to me suprisingly) unportable:
  $ ksh
  $ set -x
  $ echo foo
  + echo foo
  foo
  $ set +x
  $ 
It doesn't show 'set +x', how convenient! :)
However:

Quoted 8 lines

  $ set -x
  $ echo foo 2>/dev/null
  + echo foo
  + 2> /dev/null
  foo
  $ { set +x; } 2>/dev/null
  + 2> /dev/null
  $

Hmph. Good find. As you note, this is already a problem with "-x". I'm not sure if there's an easy way to fix this. We can't wrap it in a conditional function easily. I guess we could do something like:

if test "$SOMEHOW_WE_DETECT_KSH"
then
eval "set -x; run_the_test; set +x"
else

	eval "set -x; run_the_test; { set +x; } 2>/dev/null"

but I wonder if just ignoring it is a viable option here. We're talking about debugging output from the test suite, after all. As long as the test suite still _works_ on those shells, and as long as there are no developers on ksh-primary systems who can't bear to use another $TEST_SHELL_PATH, it's really not hurting anybody. The worst case is somebody reporting a test failure on NetBSD might have a slightly more verbose "-x" output.

-Peff