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

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

[Rohit Ashiwal via GitGitGadget]

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
-- 
gitgitgadget

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

[Rohit Ashiwal via GitGitGadget]

From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>

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 <rohit.ashiwal265@gmail.com>
---
 t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 04e5d42bd3..dcaa2ab4d6 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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

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

[Duy Nguyen]

On Tue, Feb 26, 2019 at 8:42 PM Rohit Ashiwal via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
>
> 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.

> patch will replace `test -(d|f)` calls in t3600-rm.sh.
>
> Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> ---
>  t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------
>  1 file changed, 48 insertions(+), 48 deletions(-)
>
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index 04e5d42bd3..dcaa2ab4d6 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -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



-- 
Duy

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

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 26 2019, Duy Nguyen wrote:

> On Tue, Feb 26, 2019 at 8:42 PM Rohit Ashiwal via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
>>
>> 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

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

[SZEDER Gábor]

On Tue, Feb 26, 2019 at 05:10:30PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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.

> 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...").

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).

> 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.

>     -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"

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

>     -		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)

Indeed.

>      	test_path_is_file "$1" &&

This still uses 'test_path_is_file'.

>      	if test -s "$1"
>      	then

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

[Jeff King]

On Tue, Feb 26, 2019 at 06:04:00PM +0100, SZEDER Gábor wrote:

> > 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...").
> 
> 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

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

[SZEDER Gábor]

On Tue, Feb 26, 2019 at 12:43:17PM -0500, Jeff King wrote:
> On Tue, Feb 26, 2019 at 06:04:00PM +0100, SZEDER Gábor wrote:
> 
> > > 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...").
> > 
> > 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:


diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 80402a428f..16adcd54c9 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -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.

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

[Jeff King]

On Tue, Feb 26, 2019 at 08:39:12PM +0100, SZEDER Gábor wrote:

> > > 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:
> [...]
> +	{ 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).

> 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.

>   - && 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

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

[SZEDER Gábor]

On Tue, Feb 26, 2019 at 04:01:01PM -0500, Jeff King wrote:
> On Tue, Feb 26, 2019 at 08:39:12PM +0100, SZEDER Gábor wrote:
> 
> > > > 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:
> > 
> >   - 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.
> 
> >   - && 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()).

> 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.

>   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

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.

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

[Jeff King]

On Sun, Mar 03, 2019 at 05:04:59PM +0100, SZEDER Gábor wrote:

> > >   - && 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()).

Yeah, good point.

> > 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.

> 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.

> 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

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

[SZEDER Gábor]

On Tue, Feb 26, 2019 at 04:01:01PM -0500, Jeff King wrote:
> > +	{ 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

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

[Jeff King]

On Mon, Mar 04, 2019 at 03:36:33PM +0100, SZEDER Gábor wrote:

> On Tue, Feb 26, 2019 at 04:01:01PM -0500, Jeff King wrote:
> > > +	{ 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
>   $ 

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"
  fi

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

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

[Matthieu Moy]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Tue, Feb 26, 2019 at 05:10:30PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 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.

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

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

[Jeff King]

On Tue, Feb 26, 2019 at 06:48:43PM +0100, Matthieu Moy wrote:

> > '-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.

> 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.

> 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

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

[Jeff King]

On Tue, Feb 26, 2019 at 05:10:30PM +0100, Ævar Arnfjörð Bjarmason wrote:

> 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.

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.

>      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

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

[Johannes Schindelin]

Hi,

On Tue, 26 Feb 2019, Jeff King wrote:

> 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

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

[Jeff King]

On Tue, Feb 26, 2019 at 08:58:43PM +0100, Johannes Schindelin wrote:

> On Tue, 26 Feb 2019, Jeff King wrote:
> 
> > 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.

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

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

[Duy Nguyen]

On Tue, Feb 26, 2019 at 11:10 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Tue, Feb 26 2019, Duy Nguyen wrote:
>
> > On Tue, Feb 26, 2019 at 8:42 PM Rohit Ashiwal via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> >>
> >> 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.

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.
-- 
Duy

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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.
> ...
> 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...]

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

[Johannes Schindelin]

Hi Rohit,

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

On Tue, 26 Feb 2019, Rohit Ashiwal via GitGitGadget wrote:

> From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> 
> 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

> 
> Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> ---
>  t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------
>  1 file changed, 48 insertions(+), 48 deletions(-)
> 
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index 04e5d42bd3..dcaa2ab4d6 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -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
> 

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

[Martin Ågren]

On Tue, 26 Feb 2019 at 14:43, Rohit Ashiwal via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.

> -       ! 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

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

[Rohit Ashiwal]

Hi Martin

On Tue, Feb 26, 2019 at 10:01 PM Martin Ågren <martin.agren@gmail.com> wrote:
>
> > -       ! 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`.)
>

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

Hi,

On Tue, 26 Feb 2019, Rohit Ashiwal wrote:

> Hi Martin
> 
> On Tue, Feb 26, 2019 at 10:01 PM Martin Ågren <martin.agren@gmail.com> wrote:
> >
> > > -       ! 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`.)
> >
> 
> 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

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

[Rohit Ashiwal]

Hey!

On Wed, Feb 27, 2019 at 1:22 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> 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

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

[Martin Ågren]

On Tue, 26 Feb 2019 at 21:02, Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
>
> Hey!
>
> On Wed, Feb 27, 2019 at 1:22 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > 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

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

[Rohit Ashiwal via GitGitGadget]

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):
  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 <rohit.ashiwal265@gmail.com>
      
     -    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 <rohit.ashiwal265@gmail.com>
      

-- 
gitgitgadget

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

[Rohit Ashiwal via GitGitGadget]

From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>

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 <rohit.ashiwal265@gmail.com>
---
 t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 04e5d42bd3..dcaa2ab4d6 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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

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

[SZEDER Gábor]

Hi and welcome!

On Tue, Feb 26, 2019 at 06:26:09AM -0800, Rohit Ashiwal via GitGitGadget wrote:
> 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.

> 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 <rohit.ashiwal265@gmail.com>
> ---
>  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:

>  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
>  '

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.

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

[Rohit Ashiwal]

Hi SZEDER

On Tue, Feb 26, 2019 at 10:07 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> 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.
>

>
> >  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
> >  '
>
> 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.

> 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/

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

Hi Rohit,

On Wed, 27 Feb 2019, Rohit Ashiwal wrote:

> On Tue, Feb 26, 2019 at 10:07 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> > 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?

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

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

[Rohit Ashiwal]

Hi Johannes

On Wed, Feb 27, 2019 at 1:33 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> 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

[PATCH v3 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions

[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 <rohit.ashiwal265@gmail.com>
      
     -    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

-- 
gitgitgadget

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

[Rohit Ashiwal via GitGitGadget]

From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>

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 <rohit.ashiwal265@gmail.com>
---
 t/t3600-rm.sh | 138 +++++++++++++++++++++++++-------------------------
 1 file changed, 69 insertions(+), 69 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 04e5d42bd3..ad638490ac 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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
-- 
gitgitgadget

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

[Duy Nguyen]

On Wed, Feb 27, 2019 at 5:49 AM Rohit Ashiwal via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
>
> 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 <rohit.ashiwal265@gmail.com>
> ---
>  t/t3600-rm.sh | 138 +++++++++++++++++++++++++-------------------------
>  1 file changed, 69 insertions(+), 69 deletions(-)
>
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index 04e5d42bd3..ad638490ac 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -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".
-- 
Duy

[PATCH v4 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions

[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 | 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)' \

-- 
gitgitgadget

[PATCH v4 1/1] t3600: use test_path_is_* functions

[Rohit Ashiwal via GitGitGadget]

From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>

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 <rohit.ashiwal265@gmail.com>
---
 t/t3600-rm.sh | 160 ++++++++++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 76 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 04e5d42bd3..3a5bd97df7 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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
-- 
gitgitgadget

[GSoC] acknowledging mistakes

[Rohit Ashiwal]

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

Re: [GSoC] acknowledging mistakes

[Junio C Hamano]

Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:

> 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.

> 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.

> 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...]

Feeling confused a little bit

[Rohit Ashiwal]

Hey!

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

On Fri, 01 Mar 2019 11:51:46 +0900 Junio C Hamano <gitster@pobox.com> wrote:

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

Totally agree on this!

>
> 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)

>
> 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

Re: Feeling confused a little bit

[Rafael Ascensão]

On Fri, Mar 01, 2019 at 06:43:26PM +0530, Rohit Ashiwal wrote:
> >
> > 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

Re: Feeling confused a little bit

[Thomas Gummerer]

On 03/01, Rohit Ashiwal wrote:
> Hey!
> 
> I'm a little confused as you never provide a clear indication to
> where shall I proceed? :-
> 
> On Fri, 01 Mar 2019 11:51:46 +0900 Junio C Hamano <gitster@pobox.com> 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.
> >
> >  - 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.

> >
> > 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.

> Thanks
> Rohit

[GSoC] Thanking

[Rohit Ashiwal]

Hey! Thomas

Thank you for replying over my woes.

>
> 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.

>
> 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.

>
> 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