* [PATCH 0/1] [GSoC][PATCH] tests: replace test -(d|f) with test_path_is_(dir|file) @ 2019-02-26 13:42 Rohit Ashiwal via GitGitGadget 2019-02-26 13:42 ` [PATCH 1/1] tests: replace `test -(d|f)` " Rohit Ashiwal via GitGitGadget 2019-02-26 14:26 ` [PATCH v2 0/1] [GSoC][PATCH] t3600: use test_path_is_dir and test_path_is_file Rohit Ashiwal via GitGitGadget 0 siblings, 2 replies; 42+ messages in thread From: Rohit Ashiwal via GitGitGadget @ 2019-02-26 13:42 UTC (permalink / raw) To: git; +Cc: Junio C Hamano 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file) 2019-02-26 13:42 [PATCH 0/1] [GSoC][PATCH] tests: replace test -(d|f) with test_path_is_(dir|file) Rohit Ashiwal via GitGitGadget @ 2019-02-26 13:42 ` Rohit Ashiwal via GitGitGadget 2019-02-26 14:04 ` Duy Nguyen ` (2 more replies) 2019-02-26 14:26 ` [PATCH v2 0/1] [GSoC][PATCH] t3600: use test_path_is_dir and test_path_is_file Rohit Ashiwal via GitGitGadget 1 sibling, 3 replies; 42+ messages in thread From: Rohit Ashiwal via GitGitGadget @ 2019-02-26 13:42 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Rohit Ashiwal 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file) 2019-02-26 13:42 ` [PATCH 1/1] tests: replace `test -(d|f)` " Rohit Ashiwal via GitGitGadget @ 2019-02-26 14:04 ` Duy Nguyen 2019-02-26 16:10 ` Do test-path_is_{file,dir,exists} make sense anymore with -x? Ævar Arnfjörð Bjarmason 2019-02-26 16:01 ` [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file) Johannes Schindelin 2019-02-26 16:30 ` Martin Ågren 2 siblings, 1 reply; 42+ messages in thread From: Duy Nguyen @ 2019-02-26 14:04 UTC (permalink / raw) To: Rohit Ashiwal via GitGitGadget Cc: Git Mailing List, Junio C Hamano, Rohit Ashiwal 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 14:04 ` Duy Nguyen @ 2019-02-26 16:10 ` Ævar Arnfjörð Bjarmason 2019-02-26 17:04 ` SZEDER Gábor ` (3 more replies) 0 siblings, 4 replies; 42+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-02-26 16:10 UTC (permalink / raw) To: Duy Nguyen Cc: Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, SZEDER Gábor, Jeff King, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 16:10 ` Do test-path_is_{file,dir,exists} make sense anymore with -x? Ævar Arnfjörð Bjarmason @ 2019-02-26 17:04 ` SZEDER Gábor 2019-02-26 17:43 ` Jeff King 2019-02-26 17:48 ` Matthieu Moy 2019-02-26 17:35 ` Jeff King ` (2 subsequent siblings) 3 siblings, 2 replies; 42+ messages in thread From: SZEDER Gábor @ 2019-02-26 17:04 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, Jeff King, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 17:04 ` SZEDER Gábor @ 2019-02-26 17:43 ` Jeff King 2019-02-26 19:39 ` SZEDER Gábor 2019-02-26 17:48 ` Matthieu Moy 1 sibling, 1 reply; 42+ messages in thread From: Jeff King @ 2019-02-26 17:43 UTC (permalink / raw) To: SZEDER Gábor Cc: Ævar Arnfjörð Bjarmason, Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 17:43 ` Jeff King @ 2019-02-26 19:39 ` SZEDER Gábor 2019-02-26 21:01 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: SZEDER Gábor @ 2019-02-26 19:39 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, Matthieu Moy 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. ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 19:39 ` SZEDER Gábor @ 2019-02-26 21:01 ` Jeff King 2019-03-03 16:04 ` SZEDER Gábor 2019-03-04 14:36 ` SZEDER Gábor 0 siblings, 2 replies; 42+ messages in thread From: Jeff King @ 2019-02-26 21:01 UTC (permalink / raw) To: SZEDER Gábor Cc: Ævar Arnfjörð Bjarmason, Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 21:01 ` Jeff King @ 2019-03-03 16:04 ` SZEDER Gábor 2019-03-05 4:55 ` Jeff King 2019-03-04 14:36 ` SZEDER Gábor 1 sibling, 1 reply; 42+ messages in thread From: SZEDER Gábor @ 2019-03-03 16:04 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, Matthieu Moy 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. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-03-03 16:04 ` SZEDER Gábor @ 2019-03-05 4:55 ` Jeff King 0 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2019-03-05 4:55 UTC (permalink / raw) To: SZEDER Gábor Cc: Ævar Arnfjörð Bjarmason, Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 21:01 ` Jeff King 2019-03-03 16:04 ` SZEDER Gábor @ 2019-03-04 14:36 ` SZEDER Gábor 2019-03-05 4:58 ` Jeff King 1 sibling, 1 reply; 42+ messages in thread From: SZEDER Gábor @ 2019-03-04 14:36 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-03-04 14:36 ` SZEDER Gábor @ 2019-03-05 4:58 ` Jeff King 0 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2019-03-05 4:58 UTC (permalink / raw) To: SZEDER Gábor Cc: Ævar Arnfjörð Bjarmason, Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 17:04 ` SZEDER Gábor 2019-02-26 17:43 ` Jeff King @ 2019-02-26 17:48 ` Matthieu Moy 2019-02-26 18:24 ` Jeff King 1 sibling, 1 reply; 42+ messages in thread From: Matthieu Moy @ 2019-02-26 17:48 UTC (permalink / raw) To: SZEDER Gábor Cc: Ævar Arnfjörð Bjarmason, Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, Jeff King, 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/ ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 17:48 ` Matthieu Moy @ 2019-02-26 18:24 ` Jeff King 0 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2019-02-26 18:24 UTC (permalink / raw) To: Matthieu Moy Cc: SZEDER Gábor, Ævar Arnfjörð Bjarmason, Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 16:10 ` Do test-path_is_{file,dir,exists} make sense anymore with -x? Ævar Arnfjörð Bjarmason 2019-02-26 17:04 ` SZEDER Gábor @ 2019-02-26 17:35 ` Jeff King 2019-02-26 19:58 ` Johannes Schindelin 2019-02-27 10:01 ` Duy Nguyen 2019-03-01 2:52 ` Junio C Hamano 3 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2019-02-26 17:35 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, SZEDER Gábor, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 17:35 ` Jeff King @ 2019-02-26 19:58 ` Johannes Schindelin 2019-02-26 21:02 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: Johannes Schindelin @ 2019-02-26 19:58 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, SZEDER Gábor, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 19:58 ` Johannes Schindelin @ 2019-02-26 21:02 ` Jeff King 0 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2019-02-26 21:02 UTC (permalink / raw) To: Johannes Schindelin Cc: Ævar Arnfjörð Bjarmason, Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, SZEDER Gábor, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 16:10 ` Do test-path_is_{file,dir,exists} make sense anymore with -x? Ævar Arnfjörð Bjarmason 2019-02-26 17:04 ` SZEDER Gábor 2019-02-26 17:35 ` Jeff King @ 2019-02-27 10:01 ` Duy Nguyen 2019-03-01 2:52 ` Junio C Hamano 3 siblings, 0 replies; 42+ messages in thread From: Duy Nguyen @ 2019-02-27 10:01 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Rohit Ashiwal, SZEDER Gábor, Jeff King, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Do test-path_is_{file,dir,exists} make sense anymore with -x? 2019-02-26 16:10 ` Do test-path_is_{file,dir,exists} make sense anymore with -x? Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2019-02-27 10:01 ` Duy Nguyen @ 2019-03-01 2:52 ` Junio C Hamano 3 siblings, 0 replies; 42+ messages in thread From: Junio C Hamano @ 2019-03-01 2:52 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Duy Nguyen, Rohit Ashiwal via GitGitGadget, Git Mailing List, Rohit Ashiwal, SZEDER Gábor, Jeff King, Matthieu Moy Æ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...] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file) 2019-02-26 13:42 ` [PATCH 1/1] tests: replace `test -(d|f)` " Rohit Ashiwal via GitGitGadget 2019-02-26 14:04 ` Duy Nguyen @ 2019-02-26 16:01 ` Johannes Schindelin 2019-02-26 16:30 ` Martin Ågren 2 siblings, 0 replies; 42+ messages in thread From: Johannes Schindelin @ 2019-02-26 16:01 UTC (permalink / raw) To: Rohit Ashiwal via GitGitGadget; +Cc: git, Junio C Hamano, Rohit Ashiwal 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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file) 2019-02-26 13:42 ` [PATCH 1/1] tests: replace `test -(d|f)` " Rohit Ashiwal via GitGitGadget 2019-02-26 14:04 ` Duy Nguyen 2019-02-26 16:01 ` [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file) Johannes Schindelin @ 2019-02-26 16:30 ` Martin Ågren 2019-02-26 18:29 ` Rohit Ashiwal 2 siblings, 1 reply; 42+ messages in thread From: Martin Ågren @ 2019-02-26 16:30 UTC (permalink / raw) To: Rohit Ashiwal via GitGitGadget Cc: Git Mailing List, Junio C Hamano, Rohit Ashiwal 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file) 2019-02-26 16:30 ` Martin Ågren @ 2019-02-26 18:29 ` Rohit Ashiwal 2019-02-26 19:52 ` Johannes Schindelin 0 siblings, 1 reply; 42+ messages in thread From: Rohit Ashiwal @ 2019-02-26 18:29 UTC (permalink / raw) To: Martin Ågren Cc: Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Johannes Schindelin, pclouds 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file) 2019-02-26 18:29 ` Rohit Ashiwal @ 2019-02-26 19:52 ` Johannes Schindelin 2019-02-26 20:01 ` Rohit Ashiwal 0 siblings, 1 reply; 42+ messages in thread From: Johannes Schindelin @ 2019-02-26 19:52 UTC (permalink / raw) To: Rohit Ashiwal Cc: Martin Ågren, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, pclouds [-- 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file) 2019-02-26 19:52 ` Johannes Schindelin @ 2019-02-26 20:01 ` Rohit Ashiwal 2019-02-27 5:49 ` Martin Ågren 0 siblings, 1 reply; 42+ messages in thread From: Rohit Ashiwal @ 2019-02-26 20:01 UTC (permalink / raw) To: Johannes Schindelin Cc: Martin Ågren, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, pclouds 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file) 2019-02-26 20:01 ` Rohit Ashiwal @ 2019-02-27 5:49 ` Martin Ågren 0 siblings, 0 replies; 42+ messages in thread From: Martin Ågren @ 2019-02-27 5:49 UTC (permalink / raw) To: Rohit Ashiwal Cc: Johannes Schindelin, Rohit Ashiwal via GitGitGadget, Git Mailing List, Junio C Hamano, Nguyễn Thái Ngọc Duy 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 0/1] [GSoC][PATCH] t3600: use test_path_is_dir and test_path_is_file 2019-02-26 13:42 [PATCH 0/1] [GSoC][PATCH] tests: replace test -(d|f) with test_path_is_(dir|file) Rohit Ashiwal via GitGitGadget 2019-02-26 13:42 ` [PATCH 1/1] tests: replace `test -(d|f)` " Rohit Ashiwal via GitGitGadget @ 2019-02-26 14:26 ` Rohit Ashiwal via GitGitGadget 2019-02-26 14:26 ` [PATCH v2 1/1] " Rohit Ashiwal via GitGitGadget 2019-02-26 22:48 ` [PATCH v3 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions Rohit Ashiwal via GitGitGadget 1 sibling, 2 replies; 42+ messages in thread From: Rohit Ashiwal via GitGitGadget @ 2019-02-26 14:26 UTC (permalink / raw) To: git; +Cc: Junio C Hamano 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file 2019-02-26 14:26 ` [PATCH v2 0/1] [GSoC][PATCH] t3600: use test_path_is_dir and test_path_is_file Rohit Ashiwal via GitGitGadget @ 2019-02-26 14:26 ` Rohit Ashiwal via GitGitGadget 2019-02-26 16:37 ` SZEDER Gábor 2019-02-26 22:48 ` [PATCH v3 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions Rohit Ashiwal via GitGitGadget 1 sibling, 1 reply; 42+ messages in thread From: Rohit Ashiwal via GitGitGadget @ 2019-02-26 14:26 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Rohit Ashiwal 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file 2019-02-26 14:26 ` [PATCH v2 1/1] " Rohit Ashiwal via GitGitGadget @ 2019-02-26 16:37 ` SZEDER Gábor 2019-02-26 18:40 ` Rohit Ashiwal 0 siblings, 1 reply; 42+ messages in thread From: SZEDER Gábor @ 2019-02-26 16:37 UTC (permalink / raw) To: Rohit Ashiwal via GitGitGadget; +Cc: git, Junio C Hamano, Rohit Ashiwal 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. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file 2019-02-26 16:37 ` SZEDER Gábor @ 2019-02-26 18:40 ` Rohit Ashiwal 2019-02-26 20:02 ` Johannes Schindelin 0 siblings, 1 reply; 42+ messages in thread From: Rohit Ashiwal @ 2019-02-26 18:40 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Rohit Ashiwal via GitGitGadget, git, Junio C Hamano 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/ ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file 2019-02-26 18:40 ` Rohit Ashiwal @ 2019-02-26 20:02 ` Johannes Schindelin 2019-02-26 20:05 ` Rohit Ashiwal 0 siblings, 1 reply; 42+ messages in thread From: Johannes Schindelin @ 2019-02-26 20:02 UTC (permalink / raw) To: Rohit Ashiwal Cc: SZEDER Gábor, Rohit Ashiwal via GitGitGadget, git, Junio C Hamano [-- 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file 2019-02-26 20:02 ` Johannes Schindelin @ 2019-02-26 20:05 ` Rohit Ashiwal 0 siblings, 0 replies; 42+ messages in thread From: Rohit Ashiwal @ 2019-02-26 20:05 UTC (permalink / raw) To: Johannes Schindelin Cc: SZEDER Gábor, Rohit Ashiwal via GitGitGadget, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions 2019-02-26 14:26 ` [PATCH v2 0/1] [GSoC][PATCH] t3600: use test_path_is_dir and test_path_is_file Rohit Ashiwal via GitGitGadget 2019-02-26 14:26 ` [PATCH v2 1/1] " Rohit Ashiwal via GitGitGadget @ 2019-02-26 22:48 ` Rohit Ashiwal via GitGitGadget 2019-02-26 22:48 ` [PATCH v3 1/1] t3600: use test_path_is_* functions Rohit Ashiwal via GitGitGadget 2019-02-28 10:26 ` [PATCH v4 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions Rohit Ashiwal via GitGitGadget 1 sibling, 2 replies; 42+ messages in thread From: Rohit Ashiwal via GitGitGadget @ 2019-02-26 22:48 UTC (permalink / raw) To: git; +Cc: Junio C Hamano 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3 1/1] t3600: use test_path_is_* functions 2019-02-26 22:48 ` [PATCH v3 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions Rohit Ashiwal via GitGitGadget @ 2019-02-26 22:48 ` Rohit Ashiwal via GitGitGadget 2019-02-27 10:12 ` Duy Nguyen 2019-02-28 10:26 ` [PATCH v4 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions Rohit Ashiwal via GitGitGadget 1 sibling, 1 reply; 42+ messages in thread From: Rohit Ashiwal via GitGitGadget @ 2019-02-26 22:48 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Rohit Ashiwal 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v3 1/1] t3600: use test_path_is_* functions 2019-02-26 22:48 ` [PATCH v3 1/1] t3600: use test_path_is_* functions Rohit Ashiwal via GitGitGadget @ 2019-02-27 10:12 ` Duy Nguyen 0 siblings, 0 replies; 42+ messages in thread From: Duy Nguyen @ 2019-02-27 10:12 UTC (permalink / raw) To: Rohit Ashiwal via GitGitGadget Cc: Git Mailing List, Junio C Hamano, Rohit Ashiwal 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions 2019-02-26 22:48 ` [PATCH v3 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions Rohit Ashiwal via GitGitGadget 2019-02-26 22:48 ` [PATCH v3 1/1] t3600: use test_path_is_* functions Rohit Ashiwal via GitGitGadget @ 2019-02-28 10:26 ` Rohit Ashiwal via GitGitGadget 2019-02-28 10:26 ` [PATCH v4 1/1] t3600: use test_path_is_* functions Rohit Ashiwal via GitGitGadget 1 sibling, 1 reply; 42+ messages in thread From: Rohit Ashiwal via GitGitGadget @ 2019-02-28 10:26 UTC (permalink / raw) To: git; +Cc: Junio C Hamano 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4 1/1] t3600: use test_path_is_* functions 2019-02-28 10:26 ` [PATCH v4 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions Rohit Ashiwal via GitGitGadget @ 2019-02-28 10:26 ` Rohit Ashiwal via GitGitGadget 2019-02-28 19:02 ` [GSoC] acknowledging mistakes Rohit Ashiwal 0 siblings, 1 reply; 42+ messages in thread From: Rohit Ashiwal via GitGitGadget @ 2019-02-28 10:26 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Rohit Ashiwal 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [GSoC] acknowledging mistakes 2019-02-28 10:26 ` [PATCH v4 1/1] t3600: use test_path_is_* functions Rohit Ashiwal via GitGitGadget @ 2019-02-28 19:02 ` Rohit Ashiwal 2019-03-01 2:51 ` Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Rohit Ashiwal @ 2019-02-28 19:02 UTC (permalink / raw) Cc: git, gitster, rohit.ashiwal265, martin.agren, pclouds, peff, szeder.dev, git, Johannes.Schindelin 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [GSoC] acknowledging mistakes 2019-02-28 19:02 ` [GSoC] acknowledging mistakes Rohit Ashiwal @ 2019-03-01 2:51 ` Junio C Hamano 2019-03-01 13:13 ` Feeling confused a little bit Rohit Ashiwal 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2019-03-01 2:51 UTC (permalink / raw) To: Rohit Ashiwal 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...] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Feeling confused a little bit 2019-03-01 2:51 ` Junio C Hamano @ 2019-03-01 13:13 ` Rohit Ashiwal 2019-03-02 4:24 ` Rafael Ascensão 2019-03-02 14:46 ` Thomas Gummerer 0 siblings, 2 replies; 42+ messages in thread From: Rohit Ashiwal @ 2019-03-01 13:13 UTC (permalink / raw) To: gitster Cc: Johannes.Schindelin, git, git, martin.agren, pclouds, peff, szeder.dev, 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Feeling confused a little bit 2019-03-01 13:13 ` Feeling confused a little bit Rohit Ashiwal @ 2019-03-02 4:24 ` Rafael Ascensão 2019-03-02 14:46 ` Thomas Gummerer 1 sibling, 0 replies; 42+ messages in thread From: Rafael Ascensão @ 2019-03-02 4:24 UTC (permalink / raw) To: Rohit Ashiwal Cc: gitster, Johannes.Schindelin, git, git, martin.agren, pclouds, peff, szeder.dev 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Feeling confused a little bit 2019-03-01 13:13 ` Feeling confused a little bit Rohit Ashiwal 2019-03-02 4:24 ` Rafael Ascensão @ 2019-03-02 14:46 ` Thomas Gummerer 2019-03-02 16:21 ` [GSoC] Thanking Rohit Ashiwal 1 sibling, 1 reply; 42+ messages in thread From: Thomas Gummerer @ 2019-03-02 14:46 UTC (permalink / raw) To: Rohit Ashiwal Cc: gitster, Johannes.Schindelin, git, git, martin.agren, pclouds, peff, szeder.dev 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [GSoC] Thanking 2019-03-02 14:46 ` Thomas Gummerer @ 2019-03-02 16:21 ` Rohit Ashiwal 0 siblings, 0 replies; 42+ messages in thread From: Rohit Ashiwal @ 2019-03-02 16:21 UTC (permalink / raw) To: t.gummerer Cc: Johannes.Schindelin, git, git, gitster, martin.agren, pclouds, peff, rohit.ashiwal265, szeder.dev 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2019-03-05 4:58 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-26 13:42 [PATCH 0/1] [GSoC][PATCH] tests: replace test -(d|f) with test_path_is_(dir|file) Rohit Ashiwal via GitGitGadget 2019-02-26 13:42 ` [PATCH 1/1] tests: replace `test -(d|f)` " Rohit Ashiwal via GitGitGadget 2019-02-26 14:04 ` Duy Nguyen 2019-02-26 16:10 ` Do test-path_is_{file,dir,exists} make sense anymore with -x? Ævar Arnfjörð Bjarmason 2019-02-26 17:04 ` SZEDER Gábor 2019-02-26 17:43 ` Jeff King 2019-02-26 19:39 ` SZEDER Gábor 2019-02-26 21:01 ` Jeff King 2019-03-03 16:04 ` SZEDER Gábor 2019-03-05 4:55 ` Jeff King 2019-03-04 14:36 ` SZEDER Gábor 2019-03-05 4:58 ` Jeff King 2019-02-26 17:48 ` Matthieu Moy 2019-02-26 18:24 ` Jeff King 2019-02-26 17:35 ` Jeff King 2019-02-26 19:58 ` Johannes Schindelin 2019-02-26 21:02 ` Jeff King 2019-02-27 10:01 ` Duy Nguyen 2019-03-01 2:52 ` Junio C Hamano 2019-02-26 16:01 ` [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file) Johannes Schindelin 2019-02-26 16:30 ` Martin Ågren 2019-02-26 18:29 ` Rohit Ashiwal 2019-02-26 19:52 ` Johannes Schindelin 2019-02-26 20:01 ` Rohit Ashiwal 2019-02-27 5:49 ` Martin Ågren 2019-02-26 14:26 ` [PATCH v2 0/1] [GSoC][PATCH] t3600: use test_path_is_dir and test_path_is_file Rohit Ashiwal via GitGitGadget 2019-02-26 14:26 ` [PATCH v2 1/1] " Rohit Ashiwal via GitGitGadget 2019-02-26 16:37 ` SZEDER Gábor 2019-02-26 18:40 ` Rohit Ashiwal 2019-02-26 20:02 ` Johannes Schindelin 2019-02-26 20:05 ` Rohit Ashiwal 2019-02-26 22:48 ` [PATCH v3 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions Rohit Ashiwal via GitGitGadget 2019-02-26 22:48 ` [PATCH v3 1/1] t3600: use test_path_is_* functions Rohit Ashiwal via GitGitGadget 2019-02-27 10:12 ` Duy Nguyen 2019-02-28 10:26 ` [PATCH v4 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions Rohit Ashiwal via GitGitGadget 2019-02-28 10:26 ` [PATCH v4 1/1] t3600: use test_path_is_* functions Rohit Ashiwal via GitGitGadget 2019-02-28 19:02 ` [GSoC] acknowledging mistakes Rohit Ashiwal 2019-03-01 2:51 ` Junio C Hamano 2019-03-01 13:13 ` Feeling confused a little bit Rohit Ashiwal 2019-03-02 4:24 ` Rafael Ascensão 2019-03-02 14:46 ` Thomas Gummerer 2019-03-02 16:21 ` [GSoC] Thanking Rohit Ashiwal
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).