[GSoC][PATCH] t3600: use test_path_is_* helper functions #152
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Welcome to GitGitGadgetHi @r1walz, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that this Pull Request has a good description, as it will be used as cover letter. Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions. If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via: curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt |
| /allow r1walz |
| User r1walz is now allowed to use GitGitGadget. |
| User r1walz already allowed to use GitGitGadget. |
dscho requested changes Feb 26, 2019
| git ls-files --error-unmatch foo baz | ||
| ' | ||
| | ||
| test_expect_success 'Modify foo -- rm should refuse' ' |
There was a problem hiding this comment.
Since you heeded @chriscool's advice to focus on only one test script, maybe it would make more sense to use the one-line "t3600: use test_path_is_dir and test_path_is_file"?
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
- the new code will be slower (as it adds one level of indirection: a shell function)
- the new code is more verbose (
test -dis shorter thantest_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:
- the new code is a lot more obvious to developers who are not fluent in Unix shell scripting, and
- the new code is a lot more informative in case of a breakage.
| Yeah, I agree that the code changes look good. The goal of a micro-project anyway is to see how the student interacts with the mailing list, so I think it is ok if you send it like this and further improvements are discussed on the mailing list. |
| /submit |
| Submitted as pull.152.git.gitgitgadget@gmail.com |
| Error: bf5eb04 was already submitted |
r1walz changed the title test -(d|f) with test_path_is_(dir|file) r1walz force-pushed the refactor-tests branch from bf5eb04 to 4ba0b7f Compare ![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| git ls-files --error-unmatch foo baz | ||
| ' | ||
| | ||
| test_expect_success 'Modify foo -- rm should refuse' ' |
There was a problem hiding this comment.
On the Git mailing list, Duy Nguyen wrote (reply to this):
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 There was a problem hiding this comment.
On the Git mailing list, =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason wrote (reply to this):
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 There was a problem hiding this comment.
On the Git mailing list, SZEDER =?utf-8?B?R8OhYm9y?= wrote (reply to this):
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 There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
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 There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
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 There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Æ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...] There was a problem hiding this comment.
On the Git mailing list, SZEDER =?utf-8?B?R8OhYm9y?= wrote (reply to this):
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. There was a problem hiding this comment.
On the Git mailing list, SZEDER =?utf-8?B?R8OhYm9y?= wrote (reply to this):
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 There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
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 There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
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 4ba0b7f to fcafc87 Compare | /submit |
| Submitted as pull.152.v2.git.gitgitgadget@gmail.com |
| git ls-files --error-unmatch foo baz | ||
| ' | ||
| | ||
| test_expect_success 'Modify foo -- rm should refuse' ' |
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
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 > | git ls-files --error-unmatch foo baz | ||
| ' | ||
| | ||
| test_expect_success 'Modify foo -- rm should refuse' ' |
There was a problem hiding this comment.
On the Git mailing list, =?UTF-8?Q?Martin_=C3=85gren?= wrote (reply to this):
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 There was a problem hiding this comment.
On the Git mailing list, Rohit Ashiwal wrote (reply to this):
Hi Martin On Tue, Feb 26, 2019 at 10:01 PM Martin =C3=85gren <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, an= d > 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 There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323328-628463197-1551210737=:41 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT 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 --8323328-628463197-1551210737=:41-- There was a problem hiding this comment.
On the Git mailing list, Rohit Ashiwal wrote (reply to this):
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 There was a problem hiding this comment.
On the Git mailing list, =?UTF-8?Q?Martin_=C3=85gren?= wrote (reply to this):
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 | git ls-files --error-unmatch foo baz | ||
| ' | ||
| | ||
| test_expect_success 'Modify foo -- rm should refuse' ' |
There was a problem hiding this comment.
On the Git mailing list, SZEDER =?utf-8?B?R8OhYm9y?= wrote (reply to this):
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. There was a problem hiding this comment.
On the Git mailing list, Rohit Ashiwal wrote (reply to this):
Hi SZEDER On Tue, Feb 26, 2019 at 10:07 PM SZEDER G=C3=A1bor <szeder.dev@gmail.com> w= rote: > > 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-a0zUKT5EaGLBYnRtESTnu9GKWtGARz= 2kaOAhc1HQ@mail.gmail.com/ There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323328-613778193-1551211382=:41 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT 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 --8323328-613778193-1551211382=:41-- There was a problem hiding this comment.
On the Git mailing list, Rohit Ashiwal wrote (reply to this):
Hi Johannes On Wed, Feb 27, 2019 at 1:33 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > I think what G=C3=A1bor 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 fcafc87 to bfeba25 Compare r1walz changed the title use test_path_is_* helper functions | /submit |
| Submitted as pull.152.v3.git.gitgitgadget@gmail.com |
t/t3600-rm.sh Outdated
| test_expect_success \ | ||
| 'Test that "git rm bar" succeeds' \ | ||
| 'git rm bar' | ||
| |
There was a problem hiding this comment.
On the Git mailing list, Duy Nguyen wrote (reply to this):
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 bfeba25 to f881f01 Compare | /submit |
| Submitted as pull.152.v4.git.gitgitgadget@gmail.com |
| Hey Duy On the Git mailing list, Duy Nguyen wrote (reply to this): 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". Sorry for the late reply. Yes, I thought about it earlier and made changes thinking this only. Also when I was going through the code again, I replaced other conditionals `[ -f ]` with test_path_is_file etc. Ciao Rohit |
r1walz force-pushed the refactor-tests branch 3 times, most recently from c4cb47a to 40aaf87 Compare 
test-lib-functions: add a helper function that checks for a file and that the file is not empey. The helper function will provide better error message in case of failuer and improve readability The function `test_file_not_empty`, first calls `test_path_is_file` to check verify if a file is provided, if it is not then an error message is printed, short-circuiting the remaining code, if <path> is indeed a file then check `test -s` is applied to check is size of file is more than zero, failing which another error message is returned. Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Replace leading spaces with tabs, title on same line as function The previous code of `t3600-rm.sh` had a mix use of tabs and spaces with instance of `not-so-recommended` way of writing `if-then` statement, and `titles` were not on the same line as the function `test_expect_success`, replace them so that the current version agrees with the coding guidelines Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Replace `test -(d|f|e|s)` calls in `t3600-rm.sh`. Previously we were using `test -(d|f|e|s)` to verify the presence of a directory/file, but we already have helper functions, viz, `test_path_is_dir`, `test_path_is_file`, `test_path_is_missing` and `test_file_not_empty` with better functionality. These helper functions make code more readable and informative to someone new, also these functions have better error messages. Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
| Why did you close this PR, @r1walz? |
| @dscho Now that I am familiar with the mailing list, I don't think I will be needing this. GitGitGadget was a nice experience though. |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Replace
test -(d|f|e)calls in t3600-rm.sh. Previously we were usingtest -(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.