* [GSoC][PATCH 0/3] Use helper functions in test script @ 2019-03-03 12:28 Rohit Ashiwal 2019-03-03 12:28 ` [PATCH 1/3] test functions: Add new function `test_file_not_empty` Rohit Ashiwal ` (4 more replies) 0 siblings, 5 replies; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-03 12:28 UTC (permalink / raw) To: git Cc: Johannes.Schindelin, gitster, t.gummerer, christian.couder, Rohit Ashiwal This patch ultimately aims to replace `test -(d|f|e|s)` calls in t3600-rm.sh Previously we were using these to verify the presence of diretory/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 Helper functions are better as they provide better error messages and improve readability. They are friendly to someone new to code. Note: `test_file_not_empty` is implemented in [PATCH 1/3] of this mail Rohit Ashiwal (3): test functions: Add new function `test_file_not_empty` t3600: refactor code according to contemporary guidelines t3600: use helper functions from test-lib-functions t/t3600-rm.sh | 281 +++++++++++++++++++++------------------- t/test-lib-functions.sh | 10 ++ 2 files changed, 157 insertions(+), 134 deletions(-) -- Thanks Rohit ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 1/3] test functions: Add new function `test_file_not_empty` 2019-03-03 12:28 [GSoC][PATCH 0/3] Use helper functions in test script Rohit Ashiwal @ 2019-03-03 12:28 ` Rohit Ashiwal 2019-03-03 13:20 ` Junio C Hamano 2019-03-03 12:28 ` [PATCH 2/3] t3600: refactor code according to contemporary guidelines Rohit Ashiwal ` (3 subsequent siblings) 4 siblings, 1 reply; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-03 12:28 UTC (permalink / raw) To: git Cc: Johannes.Schindelin, gitster, t.gummerer, christian.couder, Rohit Ashiwal test-lib-functions: add a helper function that checks for a file and that the file is not empty. The helper function will provide better error message in case of failure and improve readability Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> --- t/test-lib-functions.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 80402a428f..1302df63b6 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -593,6 +593,16 @@ test_dir_is_empty () { fi } +# Check if the file exists and has a size greater than zero +test_file_not_empty () { + test_path_is_file "$1" && + if ! test -s "$1" + then + echo "'$1' is an empty file." + false + fi +} + test_path_is_missing () { if test -e "$1" then -- ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 1/3] test functions: Add new function `test_file_not_empty` 2019-03-03 12:28 ` [PATCH 1/3] test functions: Add new function `test_file_not_empty` Rohit Ashiwal @ 2019-03-03 13:20 ` Junio C Hamano 2019-03-03 13:29 ` Rohit Ashiwal 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2019-03-03 13:20 UTC (permalink / raw) To: Rohit Ashiwal; +Cc: git, Johannes.Schindelin, t.gummerer, christian.couder Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes: > Subject: Re: [PATCH 1/3] test functions: Add new function `test_file_not_empty` s/Add/add/. Strictly speaking, you do not need to say "new", if you are already saying "add", then that's redundant. > test-lib-functions: add a helper function that checks for a file and that > the file is not empty. The helper function will provide better error message > in case of failure and improve readability > > Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > --- > t/test-lib-functions.sh | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > index 80402a428f..1302df63b6 100644 > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -593,6 +593,16 @@ test_dir_is_empty () { > fi > } > > +# Check if the file exists and has a size greater than zero > +test_file_not_empty () { > + test_path_is_file "$1" && > + if ! test -s "$1" "test -s <path>" is true if <path> resolves to an existing directory entry for a file that has a size greater than zero. Isn't it redundant and wasteful to have test_path_is_file before it, or is there a situation where "test -s" alone won't give us what we want to check? > + then > + echo "'$1' is an empty file." > + false > + fi > +} > + > test_path_is_missing () { > if test -e "$1" > then ^ permalink raw reply [flat|nested] 37+ messages in thread
* (no subject) 2019-03-03 13:20 ` Junio C Hamano @ 2019-03-03 13:29 ` Rohit Ashiwal 2019-03-03 13:33 ` none Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-03 13:29 UTC (permalink / raw) To: gitster Cc: Johannes.Schindelin, christian.couder, git, rohit.ashiwal265, t.gummerer Hey Junio On 2019-03-03 13:20 UTC Junio C Hamano <gitster@pobox.com> wrote: > s/Add/add/. Strictly speaking, you do not need to say "new", if you > are already saying "add", then that's redundant. Oh, my mistake, I will change in coming revisions. > "test -s <path>" is true if <path> resolves to an existing directory > entry for a file that has a size greater than zero. Isn't it > redundant and wasteful to have test_path_is_file before it, or is > there a situation where "test -s" alone won't give us what we want > to check? Just to be clear of what caused the error: 1. Path not being file, or 2. File not being empty I am checking for both. Regards Rohit ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: none 2019-03-03 13:29 ` Rohit Ashiwal @ 2019-03-03 13:33 ` Junio C Hamano 2019-03-03 14:07 ` Clearing logic Rohit Ashiwal 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2019-03-03 13:33 UTC (permalink / raw) To: Rohit Ashiwal; +Cc: Johannes.Schindelin, christian.couder, git, t.gummerer Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes: > Just to be clear of what caused the error: > 1. Path not being file, or > 2. File not being empty > I am checking for both. test -s <path> makes sure <path> is file; if it is not a file, then it won't yield true. So why do you need to say test_path_is_file yourself, if you are asking "test -s"? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Clearing logic 2019-03-03 13:33 ` none Junio C Hamano @ 2019-03-03 14:07 ` Rohit Ashiwal 2019-03-03 16:19 ` Thomas Gummerer 0 siblings, 1 reply; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-03 14:07 UTC (permalink / raw) To: gitster Cc: Johannes.Schindelin, christian.couder, git, rohit.ashiwal265, t.gummerer On 2019-03-03 13:33 UTC Junio C Hamano <gitster@pobox.com> wrote: > test -s <path> makes sure <path> is file; if it is not a file, then > it won't yield true. > On 2019-03-03 13:20 UTC Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote: > > test_path_is_file "$1" && > > if ! test -s "$1" According to the conditional if the path is not a file then we will get the error "file does not exist" and then we will shortcircuit without checking the second conditional, on the other hand, if path is a file then we will again check if it has a size greater than zero, then error will be different (if any). Regards Rohit ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Clearing logic 2019-03-03 14:07 ` Clearing logic Rohit Ashiwal @ 2019-03-03 16:19 ` Thomas Gummerer 0 siblings, 0 replies; 37+ messages in thread From: Thomas Gummerer @ 2019-03-03 16:19 UTC (permalink / raw) To: Rohit Ashiwal; +Cc: gitster, Johannes.Schindelin, christian.couder, git On 03/03, Rohit Ashiwal wrote: > On 2019-03-03 13:33 UTC Junio C Hamano <gitster@pobox.com> wrote: > > > test -s <path> makes sure <path> is file; if it is not a file, then > > it won't yield true. > > > On 2019-03-03 13:20 UTC Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote: > > > test_path_is_file "$1" && > > > if ! test -s "$1" > > According to the conditional if the path is not a file then we will get > the error "file does not exist" and then we will shortcircuit without checking > the second conditional, on the other hand, if path is a file then we will > again check if it has a size greater than zero, then error will be different > (if any). I do agree that the better error message is probably worth the additional 'test_path_is_file' before the 'test -s'. Although it may be better to only make that distinction in the 'if' (and then maybe just using 'test -f', which would explain better why we have an additional call. Either way it would be nice to describe that reasoning in the commit message, as it's not 100% clear from the code what is going on here, which also lead to Junio's question. > Regards > Rohit > ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 2/3] t3600: refactor code according to contemporary guidelines 2019-03-03 12:28 [GSoC][PATCH 0/3] Use helper functions in test script Rohit Ashiwal 2019-03-03 12:28 ` [PATCH 1/3] test functions: Add new function `test_file_not_empty` Rohit Ashiwal @ 2019-03-03 12:28 ` Rohit Ashiwal 2019-03-03 13:30 ` Junio C Hamano 2019-03-03 12:28 ` [PATCH 3/3] t3600: use helper functions from test-lib-functions Rohit Ashiwal ` (2 subsequent siblings) 4 siblings, 1 reply; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-03 12:28 UTC (permalink / raw) To: git Cc: Johannes.Schindelin, gitster, t.gummerer, christian.couder, Rohit Ashiwal Replace leading spaces with tabs 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, replace them so that the current version agrees with the coding guidelines Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> --- t/t3600-rm.sh | 131 ++++++++++++++++++++++++++------------------------ 1 file changed, 68 insertions(+), 63 deletions(-) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 04e5d42bd3..ec4877bfec 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -9,89 +9,94 @@ test_description='Test of the various options to git rm.' # Setup some files to be removed, some with funny characters test_expect_success \ - 'Initialize test directory' \ - "touch -- foo bar baz 'space embedded' -q && - git add -- foo bar baz 'space embedded' -q && - git commit -m 'add normal files'" + 'Initialize test directory' " + touch -- foo bar baz 'space embedded' -q && + git add -- foo bar baz 'space embedded' -q && + git commit -m 'add normal files' +" -if test_have_prereq !FUNNYNAMES; then +if test_have_prereq !FUNNYNAMES +then say 'Your filesystem does not allow tabs in filenames.' fi test_expect_success FUNNYNAMES 'add files with funny names' " - touch -- 'tab embedded' 'newline + touch -- 'tab embedded' 'newline embedded' && - git add -- 'tab embedded' 'newline + git add -- 'tab embedded' 'newline embedded' && - git commit -m 'add files with tabs and newlines' + git commit -m 'add files with tabs and newlines' " 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' \ + '[ -f foo ] && git ls-files --error-unmatch foo' test_expect_success \ - 'Test that git rm foo succeeds' \ - 'git rm --cached foo' + 'Test that git rm foo succeeds' \ + 'git rm --cached foo' test_expect_success \ - 'Test that git rm --cached foo succeeds if the index matches the file' \ - 'echo content >foo && - git add foo && - git rm --cached foo' + 'Test that git rm --cached foo succeeds if the index matches the file' ' + echo content >foo && + git add foo && + git rm --cached foo +' test_expect_success \ - 'Test that git rm --cached foo succeeds if the index matches the file' \ - 'echo content >foo && - git add foo && - git commit -m foo && - echo "other content" >foo && - git rm --cached foo' + 'Test that git rm --cached foo succeeds if the index matches the file' ' + echo content >foo && + git add foo && + git commit -m foo && + echo "other content" >foo && + git rm --cached foo +' test_expect_success \ - 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' ' - echo content >foo && - git add foo && - git commit -m foo --allow-empty && - echo "other content" >foo && - git add foo && - echo "yet another content" >foo && - test_must_fail git rm --cached foo + 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' ' + echo content >foo && + git add foo && + git commit -m foo --allow-empty && + echo "other content" >foo && + git add foo && + echo "yet another content" >foo && + test_must_fail git rm --cached foo ' test_expect_success \ - 'Test that git rm --cached -f foo works in case where --cached only did not' \ - 'echo content >foo && - git add foo && - git commit -m foo --allow-empty && - echo "other content" >foo && - git add foo && - echo "yet another content" >foo && - git rm --cached -f foo' + 'Test that git rm --cached -f foo works in case where --cached only did not' ' + echo content >foo && + git add foo && + git commit -m foo --allow-empty && + echo "other content" >foo && + git add foo && + echo "yet another content" >foo && + 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' \ + '[ -f 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"' \ + '[ -f bar ] && git ls-files --error-unmatch bar' test_expect_success \ - 'Test that "git rm bar" succeeds' \ - 'git rm bar' + '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"' \ + '! [ -f 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)' \ - 'git rm -- -q' + 'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \ + 'git rm -- -q' test_expect_success FUNNYNAMES \ - "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." \ - "git rm -f 'space embedded' 'tab embedded' 'newline + "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." \ + "git rm -f 'space embedded' 'tab embedded' 'newline embedded'" test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' ' @@ -101,8 +106,8 @@ test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' ' ' test_expect_success \ - 'When the rm in "git rm -f" fails, it should not remove the file from the index' \ - 'git ls-files --error-unmatch baz' + 'When the rm in "git rm -f" fails, it should not remove the file from the index' \ + 'git ls-files --error-unmatch baz' test_expect_success 'Remove nonexistent file with --ignore-unmatch' ' git rm --ignore-unmatch nonexistent @@ -218,22 +223,22 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' ' test_expect_success 'Call "rm" from outside the work tree' ' mkdir repo && (cd repo && - git init && - echo something >somefile && - git add somefile && - git commit -m "add a file" && - (cd .. && - git --git-dir=repo/.git --work-tree=repo rm somefile) && - test_must_fail git ls-files --error-unmatch somefile) + git init && + echo something >somefile && + git add somefile && + git commit -m "add a file" && + (cd .. && + git --git-dir=repo/.git --work-tree=repo rm somefile + ) && + test_must_fail git ls-files --error-unmatch somefile + ) ' 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_expect_success 'choking "git rm" should not let it die with cruft' ' @@ -242,8 +247,8 @@ test_expect_success 'choking "git rm" should not let it die with cruft' ' i=0 && while test $i -lt 12000 do - echo "100644 1234567890123456789012345678901234567890 0 some-file-$i" - i=$(( $i + 1 )) + echo "100644 1234567890123456789012345678901234567890 0 some-file-$i" + i=$(( $i + 1 )) done | git update-index --index-info && git rm -n "some-file-*" | : && test_path_is_missing .git/index.lock -- ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 2/3] t3600: refactor code according to contemporary guidelines 2019-03-03 12:28 ` [PATCH 2/3] t3600: refactor code according to contemporary guidelines Rohit Ashiwal @ 2019-03-03 13:30 ` Junio C Hamano 2019-03-03 14:13 ` t3600: refactor code according to comtemporary guidelines Rohit Ashiwal 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2019-03-03 13:30 UTC (permalink / raw) To: Rohit Ashiwal; +Cc: git, Johannes.Schindelin, t.gummerer, christian.couder Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes: > Subject: Re: [PATCH 2/3] t3600: refactor code according to contemporary guidelines Please do not overuse/abuse the verb "refactor" like this. What the patch does is only reformat---it does not do common "refactoring" transformations like factoring out common/duplicated code into helper functions, etc. If we are doing this step, let's make sure all tests use the modern style correctly. > # Setup some files to be removed, some with funny characters > test_expect_success \ > - 'Initialize test directory' \ > - "touch -- foo bar baz 'space embedded' -q && > - git add -- foo bar baz 'space embedded' -q && > - git commit -m 'add normal files'" > + 'Initialize test directory' " > + touch -- foo bar baz 'space embedded' -q && > + git add -- foo bar baz 'space embedded' -q && > + git commit -m 'add normal files' > +" In the modern style, we'd write this like so: test_expect_success 'initialize test directory' ' touch -- foo bar baz "space embedded" -q && git add -- foo bar baz "space embedded" -q && git commit -m "add normal files" ' In addition to indenting with HT (not SP), two more points are - test title comes on the first line; - test body is enclosed in a single quote pair, opened on the first line and closed on the last line. > > -if test_have_prereq !FUNNYNAMES; then > +if test_have_prereq !FUNNYNAMES > +then This is good. > say 'Your filesystem does not allow tabs in filenames.' > fi > > test_expect_success FUNNYNAMES 'add files with funny names' " This has title on the first line, and opening quote of the body as well, which is the modern style. > 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' \ > + '[ -f foo ] && git ls-files --error-unmatch foo' We prefer "test ..." over "[ ... ]" (Documentation/CodingGuidelines). Thanks. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: t3600: refactor code according to comtemporary guidelines 2019-03-03 13:30 ` Junio C Hamano @ 2019-03-03 14:13 ` Rohit Ashiwal 0 siblings, 0 replies; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-03 14:13 UTC (permalink / raw) To: gitster Cc: Johannes.Schindelin, christian.couder, git, rohit.ashiwal265, t.gummerer I agree to all the points that you mentioned. On 2019-03-03 13:30 UTC Junio C Hamano <gitster@pobox.com> wrote: > We prefer "test ..." over "[ ... ]" (Documentation/CodingGuidelines). At first I thought this should go in [PATCH 3/3] but now I think this is its real place. Thanks Rohit ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 3/3] t3600: use helper functions from test-lib-functions 2019-03-03 12:28 [GSoC][PATCH 0/3] Use helper functions in test script Rohit Ashiwal 2019-03-03 12:28 ` [PATCH 1/3] test functions: Add new function `test_file_not_empty` Rohit Ashiwal 2019-03-03 12:28 ` [PATCH 2/3] t3600: refactor code according to contemporary guidelines Rohit Ashiwal @ 2019-03-03 12:28 ` Rohit Ashiwal 2019-03-03 13:32 ` Junio C Hamano 2019-03-03 23:37 ` [GSoC][PATCH v2 0/3] Use helper functions in test script Rohit Ashiwal 2019-03-04 12:07 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Rohit Ashiwal 4 siblings, 1 reply; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-03 12:28 UTC (permalink / raw) To: git Cc: Johannes.Schindelin, gitster, t.gummerer, christian.couder, Rohit Ashiwal 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> --- t/t3600-rm.sh | 166 ++++++++++++++++++++++++++------------------------ 1 file changed, 87 insertions(+), 79 deletions(-) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index ec4877bfec..c2391b7d56 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -29,8 +29,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' \ @@ -75,20 +77,26 @@ test_expect_success \ ' 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)' \ @@ -142,15 +150,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 ' @@ -164,15 +172,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 ' @@ -199,21 +207,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' ' @@ -238,7 +246,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 ' test_expect_success 'choking "git rm" should not let it die with cruft' ' @@ -259,7 +267,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 @@ -297,7 +305,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 && @@ -319,7 +327,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 && @@ -330,7 +338,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 ' @@ -348,12 +356,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 && @@ -364,8 +372,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 && @@ -376,7 +384,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 ' @@ -386,8 +394,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 ' @@ -397,15 +405,15 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta git submodule update && 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_file_not_empty actual.err && + 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 ' @@ -418,8 +426,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 ' @@ -429,12 +437,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 ' @@ -444,12 +452,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 ' @@ -486,7 +494,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 ' @@ -498,12 +506,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 && @@ -517,12 +525,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 && @@ -536,12 +544,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 ' @@ -557,13 +565,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 && @@ -575,7 +583,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 ' @@ -591,10 +599,10 @@ 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_file_not_empty actual && test_i18ngrep Migrating output.err ' @@ -619,7 +627,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 ' @@ -629,12 +637,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 ' @@ -644,12 +652,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 ' @@ -659,12 +667,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 ' @@ -678,10 +686,10 @@ 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_file_not_empty actual && test_i18ngrep Migrating output.err ' -- ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 3/3] t3600: use helper functions from test-lib-functions 2019-03-03 12:28 ` [PATCH 3/3] t3600: use helper functions from test-lib-functions Rohit Ashiwal @ 2019-03-03 13:32 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2019-03-03 13:32 UTC (permalink / raw) To: Rohit Ashiwal; +Cc: git, Johannes.Schindelin, t.gummerer, christian.couder Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes: > Subject: Re: [PATCH 3/3] t3600: use helper functions from test-lib-functions There are tons of helpers in that lib. > Replace `test -(d|f|e|s)` calls in `t3600-rm.sh`. This one gives more useful information than the patch title. Perhaps Subject: [PATCH 3/3] t3600: use helpers to replace test -d/f/e/s <path> ^ permalink raw reply [flat|nested] 37+ messages in thread
* [GSoC][PATCH v2 0/3] Use helper functions in test script 2019-03-03 12:28 [GSoC][PATCH 0/3] Use helper functions in test script Rohit Ashiwal ` (2 preceding siblings ...) 2019-03-03 12:28 ` [PATCH 3/3] t3600: use helper functions from test-lib-functions Rohit Ashiwal @ 2019-03-03 23:37 ` Rohit Ashiwal 2019-03-03 23:37 ` [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal ` (2 more replies) 2019-03-04 12:07 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Rohit Ashiwal 4 siblings, 3 replies; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-03 23:37 UTC (permalink / raw) To: rohit.ashiwal265 Cc: Johannes.Schindelin, christian.couder, git, gitster, t.gummerer This patch ultimately aims to replace `test -(d|f|e|s)` calls in t3600-rm.sh Previously we were using these to verify the presence of diretory/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 Helper functions are better as they provide better error messages and improve readability. They are friendly to someone new to code. Thanks Rohit PS: `test_file_not_empty` is implemented in [PATCH v2 1/3] of this mail Rohit Ashiwal (3): test functions: add function `test_file_not_empty` t3600: restructure code according to contemporary guidelines t3600: use helpers to replace test -d/f/e/s <path> t/t3600-rm.sh | 326 ++++++++++++++++++++-------------------- t/test-lib-functions.sh | 15 ++ 2 files changed, 180 insertions(+), 161 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty` 2019-03-03 23:37 ` [GSoC][PATCH v2 0/3] Use helper functions in test script Rohit Ashiwal @ 2019-03-03 23:37 ` Rohit Ashiwal 2019-03-04 3:45 ` Junio C Hamano 2019-03-03 23:37 ` [GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines Rohit Ashiwal 2019-03-03 23:37 ` [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> Rohit Ashiwal 2 siblings, 1 reply; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-03 23:37 UTC (permalink / raw) To: rohit.ashiwal265 Cc: Johannes.Schindelin, christian.couder, git, gitster, t.gummerer test-lib-functions: add a helper function that checks for a file and that the file is not empty. The helper function will provide better error message in case of failure and improve readability The function `test_file_not_empty`, first checks if a file is provided, if it is not then an error message is printed, skipping the remaining code, if <path> is indeed a file then check `test -s` is applied to check if size of file is greater than zero, failing which another error message is printed. Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> --- t/test-lib-functions.sh | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 80402a428f..f9fcd2e013 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -593,6 +593,21 @@ test_dir_is_empty () { fi } +# Check if the file exists and has a size greater than zero +test_file_not_empty () { + if ! test -f "$1" + then + echo "'$1' does not exist or not a file." + false + else + if ! test -s "$1" + then + echo "'$1' is an empty file." + false + fi + fi +} + test_path_is_missing () { if test -e "$1" then -- 2.17.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty` 2019-03-03 23:37 ` [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal @ 2019-03-04 3:45 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2019-03-04 3:45 UTC (permalink / raw) To: Rohit Ashiwal; +Cc: Johannes.Schindelin, christian.couder, git, t.gummerer Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes: > test-lib-functions: add a helper function that checks for a file and that > the file is not empty. The helper function will provide better error message > in case of failure and improve readability Avoid making the log message into an enumerated list, when there aren't that many things to enumerate to begin with (specifically, the "test-lib-functions:" label is unsightly here). Finish the sentence with a full stop. Add a helper function to ensure that a given path is a non-empty file, and give an error message when it is not. Give separate messages for the case when the path is missing or a non-file, and for the case when the path is a file but is empty. should be sufficient. I still do not see why the posted code is better than this if ! test -s "$1" then echo "'$1' is not a non-empty file.' fi which is more to the point. After all, if we are truly aiming for finer-grained diagnosis, there is no good reason to accept a single error message "does not exist or not a file" for these two cases, but you'd be writing more like if ! test -e "$1" then echo "'$1' does not exist" elif ! test -f "$1" then echo "'$1' is not a file" elif ! test -s "$1" then echo "'$1' is not empty" else : happy return fi false But I do not see much point in doing so, and I do not see much point in the version that makes an extra check only for "test -f", either. > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > index 80402a428f..f9fcd2e013 100644 > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -593,6 +593,21 @@ test_dir_is_empty () { > fi > } > > +# Check if the file exists and has a size greater than zero > +test_file_not_empty () { > + if ! test -f "$1" > + then > + echo "'$1' does not exist or not a file." > + false > + else > + if ! test -s "$1" > + then > + echo "'$1' is an empty file." > + false > + fi > + fi > +} If I were writing this, I'd dedent it by turning this into if ! test -f ... then echo ... elif ! test -s ... then echo ... else : happy return fi false But as I said, I do not see much point in the extra "test -f", so more likely this is what I would write, if I were doing this step myself: if test -s "$1" then : happy else echo "'$1' is not a non-empty file" false fi > + > test_path_is_missing () { > if test -e "$1" > then ^ permalink raw reply [flat|nested] 37+ messages in thread
* [GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines 2019-03-03 23:37 ` [GSoC][PATCH v2 0/3] Use helper functions in test script Rohit Ashiwal 2019-03-03 23:37 ` [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal @ 2019-03-03 23:37 ` Rohit Ashiwal 2019-03-04 4:17 ` Junio C Hamano 2019-03-03 23:37 ` [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> Rohit Ashiwal 2 siblings, 1 reply; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-03 23:37 UTC (permalink / raw) To: rohit.ashiwal265 Cc: Johannes.Schindelin, christian.couder, git, gitster, t.gummerer Replace leading spaces with tabs Place title on the same line as function The previous code of `t3600-rm.sh` had a mixed use of tabs and spaces with instance of `not-so-recommended` way of writing `if-then` statement, also `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> --- t/t3600-rm.sh | 184 ++++++++++++++++++++++++++------------------------ 1 file changed, 94 insertions(+), 90 deletions(-) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 04e5d42bd3..f1afda21e9 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -8,91 +8,95 @@ test_description='Test of the various options to git rm.' . ./test-lib.sh # Setup some files to be removed, some with funny characters -test_expect_success \ - 'Initialize test directory' \ - "touch -- foo bar baz 'space embedded' -q && - git add -- foo bar baz 'space embedded' -q && - git commit -m 'add normal files'" +test_expect_success 'Initialize test directory' " + touch -- foo bar baz 'space embedded' -q && + git add -- foo bar baz 'space embedded' -q && + git commit -m 'add normal files' +" -if test_have_prereq !FUNNYNAMES; then +if test_have_prereq !FUNNYNAMES +then say 'Your filesystem does not allow tabs in filenames.' fi test_expect_success FUNNYNAMES 'add files with funny names' " - touch -- 'tab embedded' 'newline + touch -- 'tab embedded' 'newline embedded' && - git add -- 'tab embedded' 'newline + git add -- 'tab embedded' 'newline embedded' && - git commit -m 'add files with tabs and newlines' + git commit -m 'add files with tabs and newlines' " -test_expect_success \ - 'Pre-check that foo exists and is in index before git rm foo' \ - '[ -f foo ] && git ls-files --error-unmatch foo' - -test_expect_success \ - 'Test that git rm foo succeeds' \ - 'git rm --cached foo' - -test_expect_success \ - 'Test that git rm --cached foo succeeds if the index matches the file' \ - 'echo content >foo && - git add foo && - git rm --cached foo' - -test_expect_success \ - 'Test that git rm --cached foo succeeds if the index matches the file' \ - 'echo content >foo && - git add foo && - git commit -m foo && - echo "other content" >foo && - git rm --cached foo' - -test_expect_success \ - 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' ' - echo content >foo && - git add foo && - git commit -m foo --allow-empty && - echo "other content" >foo && - git add foo && - echo "yet another content" >foo && - test_must_fail git rm --cached foo -' - -test_expect_success \ - 'Test that git rm --cached -f foo works in case where --cached only did not' \ - 'echo content >foo && - git add foo && - git commit -m foo --allow-empty && - echo "other content" >foo && - git add foo && - echo "yet another content" >foo && - 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' - -test_expect_success \ - 'Pre-check that bar exists and is in index before "git rm bar"' \ - '[ -f 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_expect_success \ - 'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \ - 'git rm -- -q' - -test_expect_success FUNNYNAMES \ - "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." \ - "git rm -f 'space embedded' 'tab embedded' 'newline -embedded'" +test_expect_success '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 foo +' + +test_expect_success 'Test that git rm --cached foo succeeds if the index matches the file' ' + echo content >foo && + git add foo && + git rm --cached foo +' + +test_expect_success 'Test that git rm --cached foo succeeds if the index matches the file' ' + echo content >foo && + git add foo && + git commit -m foo && + echo "other content" >foo && + git rm --cached foo +' + +test_expect_success 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' ' + echo content >foo && + git add foo && + git commit -m foo --allow-empty && + echo "other content" >foo && + git add foo && + echo "yet another content" >foo && + test_must_fail git rm --cached foo +' + +test_expect_success 'Test that git rm --cached -f foo works in case where --cached only did not' ' + echo content >foo && + git add foo && + git commit -m foo --allow-empty && + echo "other content" >foo && + git add foo && + echo "yet another content" >foo && + git rm --cached -f foo +' + +test_expect_success '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"' ' + 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"' ' + 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)' ' + git rm -- -q +' + +test_expect_success FUNNYNAMES "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." " + git rm -f 'space embedded' 'tab embedded' 'newline +embedded' +" test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' ' test_when_finished "chmod 775 ." && @@ -100,9 +104,9 @@ test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' ' test_must_fail git rm -f baz ' -test_expect_success \ - 'When the rm in "git rm -f" fails, it should not remove the file from the index' \ - 'git ls-files --error-unmatch baz' +test_expect_success 'When the rm in "git rm -f" fails, it should not remove the file from the index' ' + git ls-files --error-unmatch baz +' test_expect_success 'Remove nonexistent file with --ignore-unmatch' ' git rm --ignore-unmatch nonexistent @@ -218,22 +222,22 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' ' test_expect_success 'Call "rm" from outside the work tree' ' mkdir repo && (cd repo && - git init && - echo something >somefile && - git add somefile && - git commit -m "add a file" && - (cd .. && - git --git-dir=repo/.git --work-tree=repo rm somefile) && - test_must_fail git ls-files --error-unmatch somefile) + git init && + echo something >somefile && + git add somefile && + git commit -m "add a file" && + (cd .. && + git --git-dir=repo/.git --work-tree=repo rm somefile + ) && + test_must_fail git ls-files --error-unmatch somefile + ) ' 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_expect_success 'choking "git rm" should not let it die with cruft' ' @@ -242,8 +246,8 @@ test_expect_success 'choking "git rm" should not let it die with cruft' ' i=0 && while test $i -lt 12000 do - echo "100644 1234567890123456789012345678901234567890 0 some-file-$i" - i=$(( $i + 1 )) + echo "100644 1234567890123456789012345678901234567890 0 some-file-$i" + i=$(( $i + 1 )) done | git update-index --index-info && git rm -n "some-file-*" | : && test_path_is_missing .git/index.lock -- 2.17.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines 2019-03-03 23:37 ` [GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines Rohit Ashiwal @ 2019-03-04 4:17 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2019-03-04 4:17 UTC (permalink / raw) To: Rohit Ashiwal; +Cc: Johannes.Schindelin, christian.couder, git, t.gummerer Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes: > Replace leading spaces with tabs > Place title on the same line as function > > The previous code of `t3600-rm.sh` had a mixed use of tabs and spaces with > instance of `not-so-recommended` way of writing `if-then` statement, also > `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 Styles and conventions are different from project to project, but around here, we do _not_ start the log message with an itemized list of what was done. I can sort of see why some project might find it useful, but we do not do that here. Instead we talk about the status-quo in present tense, point out problems (which can be omitted when they are obvious from the description of the status-quo) and describe the approach to addres the problems (again, which can be omitted when it is obvious from what is written already). We then summarize the solution in imperative mood, as if we are giving an order to the codebase to "be like so" (you can think of it as giving a command to a patch monkey to "make the code like so"). Subject: t3600: modernize style The tests in t3600 were written long time ago, and has a lot of style violations, including the mixed use of tabs and spaces, not having the title and the opening quote of the body on the first line of the tests, and other shell script style violations. Update it to match the CodingGuidelines. is probably what I would summarize this change as.. > Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > --- > t/t3600-rm.sh | 184 ++++++++++++++++++++++++++------------------------ > 1 file changed, 94 insertions(+), 90 deletions(-) > > diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh > index 04e5d42bd3..f1afda21e9 100755 > --- a/t/t3600-rm.sh > +++ b/t/t3600-rm.sh > @@ -8,91 +8,95 @@ test_description='Test of the various options to git rm.' > . ./test-lib.sh > > # Setup some files to be removed, some with funny characters > -test_expect_success \ > - 'Initialize test directory' \ > - "touch -- foo bar baz 'space embedded' -q && > - git add -- foo bar baz 'space embedded' -q && > - git commit -m 'add normal files'" > +test_expect_success 'Initialize test directory' " > + touch -- foo bar baz 'space embedded' -q && > + git add -- foo bar baz 'space embedded' -q && > + git commit -m 'add normal files' > +" Swap '' and ""; it is very rare that use of double-quotes around the test body is justifiable (for one, any $variable reference would be expanded _before_ the test runs, which is almost always not what you want, if you used double-quote around the test body). There are many other instances of this in the remainder of this patch, which I won't mention. > -if test_have_prereq !FUNNYNAMES; then > +if test_have_prereq !FUNNYNAMES > +then Good. > -test_expect_success FUNNYNAMES \ > - "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." \ > - "git rm -f 'space embedded' 'tab embedded' 'newline > -embedded'" > +test_expect_success 'Pre-check that foo exists and is in index before git rm foo' ' > + test_path_is_file foo && The point of having 2/3 and 3/3 as separate steps is because 3/3 is about using the test-path-is... helpers, while 2/3 is about modernizing the codebase before doing 3/3 so that the it can be reviewed more easily without distracting changes 2/3 needs to make. So you would want to turn the "[ -f foo ]" into "test -f foo" in this step, and then you will further turn it in the next step into "test_path_is_file foo". It would not show in the end result, but paying attention to this kind of detail shows how careful the author was when future readers read the patch, so I try to be careful when I am structuring a series like this myself. > +test_expect_success '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 > +' Likewise. > +test_expect_success 'Pre-check that bar exists and is in index before "git rm bar"' ' > + test_path_is_file bar && > + git ls-files --error-unmatch bar > +' Likewise (I'll stop pointing these out from here on). > +test_expect_success FUNNYNAMES "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." " > + git rm -f 'space embedded' 'tab embedded' 'newline > +embedded' > +" Again, swap "" and '' around; that way you can lose the backslash. Consider using $LF that is defined in t/test-lib.sh for exactly a case like this one. git rm -f "space embedded" "tab embedded" "newline${LF}embedded" That may make the test body even easier to follow. > @@ -218,22 +222,22 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' ' > test_expect_success 'Call "rm" from outside the work tree' ' > mkdir repo && > (cd repo && Inspect the output from git grep 'cd ' 't/t[0-9][0-9][0-9][0-9]-*.sh' and see which is prevalent; I think this line may want to become ( cd repo && but I did not count. > - git init && > - echo something >somefile && > - git add somefile && > - git commit -m "add a file" && > - (cd .. && > - git --git-dir=repo/.git --work-tree=repo rm somefile) && > - test_must_fail git ls-files --error-unmatch somefile) > + git init && > + echo something >somefile && > + git add somefile && > + git commit -m "add a file" && > + (cd .. && > + git --git-dir=repo/.git --work-tree=repo rm somefile > + ) && > + test_must_fail git ls-files --error-unmatch somefile > + ) > ' Likewise. > 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 > - > ' Good. Thanks for working on this. ^ permalink raw reply [flat|nested] 37+ messages in thread
* [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> 2019-03-03 23:37 ` [GSoC][PATCH v2 0/3] Use helper functions in test script Rohit Ashiwal 2019-03-03 23:37 ` [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal 2019-03-03 23:37 ` [GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines Rohit Ashiwal @ 2019-03-03 23:37 ` Rohit Ashiwal 2 siblings, 0 replies; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-03 23:37 UTC (permalink / raw) To: rohit.ashiwal265 Cc: Johannes.Schindelin, christian.couder, git, gitster, t.gummerer 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> --- t/t3600-rm.sh | 142 +++++++++++++++++++++++++------------------------- 1 file changed, 71 insertions(+), 71 deletions(-) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index f1afda21e9..9e1ada463c 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -141,15 +141,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 ' @@ -163,15 +163,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 ' @@ -198,21 +198,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' ' @@ -237,7 +237,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 ' test_expect_success 'choking "git rm" should not let it die with cruft' ' @@ -258,7 +258,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 @@ -296,7 +296,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 && @@ -318,7 +318,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 && @@ -329,7 +329,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 ' @@ -347,12 +347,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 && @@ -363,8 +363,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 && @@ -375,7 +375,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 ' @@ -385,8 +385,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 ' @@ -396,15 +396,15 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta git submodule update && 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_file_not_empty actual.err && + 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 ' @@ -417,8 +417,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 ' @@ -428,12 +428,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 ' @@ -443,12 +443,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 ' @@ -485,7 +485,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 ' @@ -497,12 +497,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 && @@ -516,12 +516,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 && @@ -535,12 +535,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 ' @@ -556,13 +556,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 && @@ -574,7 +574,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 ' @@ -590,10 +590,10 @@ 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_file_not_empty actual && test_i18ngrep Migrating output.err ' @@ -618,7 +618,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 ' @@ -628,12 +628,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 ' @@ -643,12 +643,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 ' @@ -658,12 +658,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 ' @@ -677,10 +677,10 @@ 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_file_not_empty actual && test_i18ngrep Migrating output.err ' -- 2.17.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [GSoC][PATCH v3 0/3] Use helper functions in test script 2019-03-03 12:28 [GSoC][PATCH 0/3] Use helper functions in test script Rohit Ashiwal ` (3 preceding siblings ...) 2019-03-03 23:37 ` [GSoC][PATCH v2 0/3] Use helper functions in test script Rohit Ashiwal @ 2019-03-04 12:07 ` Rohit Ashiwal 2019-03-04 12:07 ` [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal ` (3 more replies) 4 siblings, 4 replies; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-04 12:07 UTC (permalink / raw) To: rohit.ashiwal265 Cc: Johannes.Schindelin, christian.couder, git, gitster, t.gummerer This patch ultimately aims to replace `test -(d|f|e|s)` calls in t3600-rm.sh Previously we were using these to verify the presence of diretory/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 Helper functions are better as they provide better error messages and improve readability. They are friendly to someone new to code. Rohit Ashiwal (3): test functions: add function `test_file_not_empty` t3600: modernize style t3600: use helpers to replace test -d/f/e/s <path> t/t3600-rm.sh | 349 ++++++++++++++++++++-------------------- t/test-lib-functions.sh | 9 ++ 2 files changed, 187 insertions(+), 171 deletions(-) -- Rohit ^ permalink raw reply [flat|nested] 37+ messages in thread
* [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty` 2019-03-04 12:07 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Rohit Ashiwal @ 2019-03-04 12:07 ` Rohit Ashiwal 2019-03-05 0:17 ` Eric Sunshine 2019-03-04 12:08 ` [GSoC][PATCH v3 2/3] t3600: modernize style Rohit Ashiwal ` (2 subsequent siblings) 3 siblings, 1 reply; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-04 12:07 UTC (permalink / raw) To: rohit.ashiwal265 Cc: Johannes.Schindelin, christian.couder, git, gitster, t.gummerer Add a helper function to ensure that a given path is a non-empty file, and give an error message when it is not. Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> --- t/test-lib-functions.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 80402a428f..681c41ba32 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -593,6 +593,15 @@ test_dir_is_empty () { fi } +# Check if the file exists and has a size greater than zero +test_file_not_empty () { + if ! test -s "$1" + then + echo "'$1' is not a non-empty file." + false + fi +} + test_path_is_missing () { if test -e "$1" then -- Rohit ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty` 2019-03-04 12:07 ` [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal @ 2019-03-05 0:17 ` Eric Sunshine 2019-03-05 12:43 ` Junio C Hamano 2019-03-05 13:27 ` [GSoc][PATCH " Rohit Ashiwal 0 siblings, 2 replies; 37+ messages in thread From: Eric Sunshine @ 2019-03-05 0:17 UTC (permalink / raw) To: Rohit Ashiwal Cc: Johannes Schindelin, Christian Couder, Git List, Junio C Hamano, Thomas Gummerer On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote: > Add a helper function to ensure that a given path is a non-empty file, > and give an error message when it is not. > > Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > --- > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > @@ -593,6 +593,15 @@ test_dir_is_empty () { > +# Check if the file exists and has a size greater than zero > +test_file_not_empty () { > + if ! test -s "$1" > + then > + echo "'$1' is not a non-empty file." Although not incorrect, the double-negative is hard to digest. I had to read it a few times to convince myself that it matched the intent of the new function. I wonder if a message such as echo "'$1' is unexpectedly empty" would be better. (Subjective, and not at all worth a re-roll.) > + false > + fi > +} > test_path_is_missing () { Much later in this same file, a function named test_must_be_empty() is defined, which is the complement of your new test_file_not_empty() function. The dissimilar names may cause confusion, so choosing a name more like the existing function might be warranted. Also, it might be a good idea to add this new function as a neighbor of test_must_be_empty() rather than defining it a couple hundred lines earlier in the file. Alternately, perhaps a preparatory patch could move test_must_be_empty() closer to the other similar functions (test_path_is_missing() and cousins). ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty` 2019-03-05 0:17 ` Eric Sunshine @ 2019-03-05 12:43 ` Junio C Hamano 2019-03-05 13:27 ` [GSoc][PATCH " Rohit Ashiwal 1 sibling, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2019-03-05 12:43 UTC (permalink / raw) To: Eric Sunshine Cc: Rohit Ashiwal, Johannes Schindelin, Christian Couder, Git List, Thomas Gummerer Eric Sunshine <sunshine@sunshineco.com> writes: >> +test_file_not_empty () { >> + if ! test -s "$1" >> + then >> + echo "'$1' is not a non-empty file." > > Although not incorrect, the double-negative is hard to digest. I had > to read it a few times to convince myself that it matched the intent > of the new function. I wonder if a message such as > > echo "'$1' is unexpectedly empty" > > would be better. (Subjective, and not at all worth a re-roll.) Yeah, that is subjective. The expectation of the test is "not-empty", so I do not see this double-negation as being too bad, though. > Much later in this same file, a function named test_must_be_empty() is > defined, which is the complement of your new test_file_not_empty() > function. The dissimilar names may cause confusion, so choosing a name > more like the existing function might be warranted. > > Also, it might be a good idea to add this new function as a neighbor > of test_must_be_empty() rather than defining it a couple hundred lines > earlier in the file. Alternately, perhaps a preparatory patch could > move test_must_be_empty() closer to the other similar functions > (test_path_is_missing() and cousins). Very good suggestions. Looking at neighbouring helpers around must-be-empty, it seems to me that the latter, i.e. moving it to sit next to other "path" helpers, would make the most sense. Thanks. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [GSoc][PATCH v3 1/3] test functions: add function `test_file_not_empty` 2019-03-05 0:17 ` Eric Sunshine 2019-03-05 12:43 ` Junio C Hamano @ 2019-03-05 13:27 ` Rohit Ashiwal 1 sibling, 0 replies; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-05 13:27 UTC (permalink / raw) To: sunshine Cc: Johannes.Schindelin, christian.couder, git, gitster, rohit.ashiwal265, t.gummerer Hello Eric On 2019-03-04 19:17:50 -0500 Eric Sunshine <sunshine@sunshineco.com> wrote: > On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote: > > if ! test -s "$1" > > then > > echo "'$1' is not a non-empty file." > > Although not incorrect, the double-negative is hard to digest. I had > to read it a few times to convince myself that it matched the intent > of the new function. I wonder if a message such as > > echo "'$1' is unexpectedly empty" > > would be better. (Subjective, and not at all worth a re-roll.) I think the current message is more accurate as it implies both: 1. There is no file, and 2. If there is, it is not empty "unexpectedly empty" may imply that there is a directory which is not empty and that is not the intention of the function. > Also, it might be a good idea to add this new function as a neighbor > of test_must_be_empty() rather than defining it a couple hundred lines > earlier in the file. Alternately, perhaps a preparatory patch could > move test_must_be_empty() closer to the other similar functions > (test_path_is_missing() and cousins). I think we should relocate the function `test_must_be_empty` in a separate patch as this patch deals with a different issue. Thanks Rohit ^ permalink raw reply [flat|nested] 37+ messages in thread
* [GSoC][PATCH v3 2/3] t3600: modernize style 2019-03-04 12:07 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Rohit Ashiwal 2019-03-04 12:07 ` [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal @ 2019-03-04 12:08 ` Rohit Ashiwal 2019-03-05 0:36 ` Eric Sunshine 2019-03-04 12:08 ` [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path> Rohit Ashiwal 2019-03-05 0:09 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Eric Sunshine 3 siblings, 1 reply; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-04 12:08 UTC (permalink / raw) To: rohit.ashiwal265 Cc: Johannes.Schindelin, christian.couder, git, gitster, t.gummerer The tests in `t3600-rm.sh` were written long time ago, and has a lot of style violations, including the mixed use of tabs and spaces, not having the title and the opening quote of the body on the first line of the tests, and other shell script style violations. Update it to match the CodingGuidelines. Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> --- t/t3600-rm.sh | 207 ++++++++++++++++++++++++++------------------------ 1 file changed, 107 insertions(+), 100 deletions(-) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 04e5d42bd3..8b03897a65 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -8,91 +8,92 @@ test_description='Test of the various options to git rm.' . ./test-lib.sh # Setup some files to be removed, some with funny characters -test_expect_success \ - 'Initialize test directory' \ - "touch -- foo bar baz 'space embedded' -q && - git add -- foo bar baz 'space embedded' -q && - git commit -m 'add normal files'" +test_expect_success 'Initialize test directory' ' + touch -- foo bar baz "space embedded" -q && + git add -- foo bar baz "space embedded" -q && + git commit -m "add normal files" +' -if test_have_prereq !FUNNYNAMES; then +if test_have_prereq !FUNNYNAMES +then say 'Your filesystem does not allow tabs in filenames.' fi -test_expect_success FUNNYNAMES 'add files with funny names' " - touch -- 'tab embedded' 'newline -embedded' && - git add -- 'tab embedded' 'newline -embedded' && - git commit -m 'add files with tabs and newlines' -" - -test_expect_success \ - 'Pre-check that foo exists and is in index before git rm foo' \ - '[ -f foo ] && git ls-files --error-unmatch foo' - -test_expect_success \ - 'Test that git rm foo succeeds' \ - 'git rm --cached foo' - -test_expect_success \ - 'Test that git rm --cached foo succeeds if the index matches the file' \ - 'echo content >foo && - git add foo && - git rm --cached foo' - -test_expect_success \ - 'Test that git rm --cached foo succeeds if the index matches the file' \ - 'echo content >foo && - git add foo && - git commit -m foo && - echo "other content" >foo && - git rm --cached foo' - -test_expect_success \ - 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' ' - echo content >foo && - git add foo && - git commit -m foo --allow-empty && - echo "other content" >foo && - git add foo && - echo "yet another content" >foo && - test_must_fail git rm --cached foo -' - -test_expect_success \ - 'Test that git rm --cached -f foo works in case where --cached only did not' \ - 'echo content >foo && - git add foo && - git commit -m foo --allow-empty && - echo "other content" >foo && - git add foo && - echo "yet another content" >foo && - 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' - -test_expect_success \ - 'Pre-check that bar exists and is in index before "git rm bar"' \ - '[ -f 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_expect_success \ - 'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \ - 'git rm -- -q' - -test_expect_success FUNNYNAMES \ - "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." \ - "git rm -f 'space embedded' 'tab embedded' 'newline -embedded'" +test_expect_success FUNNYNAMES 'add files with funny names' ' + touch -- "tab embedded" "newline${LF}embedded" && + git add -- "tab embedded" "newline${LF}embedded" && + git commit -m "add files with tabs and newlines" +' + +test_expect_success 'Pre-check that foo exists and is in index before git rm foo' ' + test -f foo && + git ls-files --error-unmatch foo +' + +test_expect_success 'Test that git rm foo succeeds' ' + git rm --cached foo +' + +test_expect_success 'Test that git rm --cached foo succeeds if the index matches the file' ' + echo content >foo && + git add foo && + git rm --cached foo +' + +test_expect_success 'Test that git rm --cached foo succeeds if the index matches the file' ' + echo content >foo && + git add foo && + git commit -m foo && + echo "other content" >foo && + git rm --cached foo +' + +test_expect_success 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' ' + echo content >foo && + git add foo && + git commit -m foo --allow-empty && + echo "other content" >foo && + git add foo && + echo "yet another content" >foo && + test_must_fail git rm --cached foo +' + +test_expect_success 'Test that git rm --cached -f foo works in case where --cached only did not' ' + echo content >foo && + git add foo && + git commit -m foo --allow-empty && + echo "other content" >foo && + git add foo && + echo "yet another content" >foo && + git rm --cached -f foo +' + +test_expect_success 'Post-check that foo exists but is not in index after git rm foo' ' + test -f 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"' ' + test -f 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"' ' + ! test -f 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)' ' + git rm -- -q +' + +test_expect_success FUNNYNAMES 'Test that "git rm -f" succeeds with embedded space, tab, or newline characters.' ' + git rm -f "space embedded" "tab embedded" "newline${LF}embedded" +' test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' ' test_when_finished "chmod 775 ." && @@ -100,9 +101,9 @@ test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' ' test_must_fail git rm -f baz ' -test_expect_success \ - 'When the rm in "git rm -f" fails, it should not remove the file from the index' \ - 'git ls-files --error-unmatch baz' +test_expect_success 'When the rm in "git rm -f" fails, it should not remove the file from the index' ' + git ls-files --error-unmatch baz +' test_expect_success 'Remove nonexistent file with --ignore-unmatch' ' git rm --ignore-unmatch nonexistent @@ -217,23 +218,25 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' ' test_expect_success 'Call "rm" from outside the work tree' ' mkdir repo && - (cd repo && - git init && - echo something >somefile && - git add somefile && - git commit -m "add a file" && - (cd .. && - git --git-dir=repo/.git --work-tree=repo rm somefile) && - test_must_fail git ls-files --error-unmatch somefile) + ( + cd repo && + git init && + echo something >somefile && + git add somefile && + git commit -m "add a file" && + ( + cd .. && + git --git-dir=repo/.git --work-tree=repo rm somefile + ) && + test_must_fail git ls-files --error-unmatch somefile + ) ' 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_expect_success 'choking "git rm" should not let it die with cruft' ' @@ -242,8 +245,8 @@ test_expect_success 'choking "git rm" should not let it die with cruft' ' i=0 && while test $i -lt 12000 do - echo "100644 1234567890123456789012345678901234567890 0 some-file-$i" - i=$(( $i + 1 )) + echo "100644 1234567890123456789012345678901234567890 0 some-file-$i" + i=$(( $i + 1 )) done | git update-index --index-info && git rm -n "some-file-*" | : && test_path_is_missing .git/index.lock @@ -545,7 +548,8 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director git checkout conflict1 && git reset --hard && git submodule update && - (cd submod && + ( + cd submod && rm .git && cp -R ../.git/modules/sub .git && GIT_WORK_TREE=. git config --unset core.worktree @@ -579,7 +583,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates git checkout -f master && git reset --hard && git submodule update && - (cd submod && + ( + cd submod && rm .git && cp -R ../.git/modules/sub .git && GIT_WORK_TREE=. git config --unset core.worktree && @@ -600,7 +605,8 @@ EOF test_expect_success 'setup subsubmodule' ' git reset --hard && git submodule update && - (cd submod && + ( + cd submod && git update-index --add --cacheinfo 160000 $(git rev-parse HEAD) subsubmod && git config -f .gitmodules submodule.sub.url ../. && git config -f .gitmodules submodule.sub.path subsubmod && @@ -667,7 +673,8 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi test_expect_success "rm absorbs submodule's nested .git directory" ' git reset --hard && git submodule update --recursive && - (cd submod/subsubmod && + ( + cd submod/subsubmod && rm .git && mv ../../.git/modules/sub/modules/sub .git && GIT_WORK_TREE=. git config --unset core.worktree -- Rohit ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v3 2/3] t3600: modernize style 2019-03-04 12:08 ` [GSoC][PATCH v3 2/3] t3600: modernize style Rohit Ashiwal @ 2019-03-05 0:36 ` Eric Sunshine 2019-03-05 12:44 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Eric Sunshine @ 2019-03-05 0:36 UTC (permalink / raw) To: Rohit Ashiwal Cc: Johannes Schindelin, Christian Couder, Git List, Junio C Hamano, Thomas Gummerer On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote: > The tests in `t3600-rm.sh` were written long time ago, and has a lot of > style violations, including the mixed use of tabs and spaces, not having > the title and the opening quote of the body on the first line of the > tests, and other shell script style violations. Update it to match the > CodingGuidelines. Many of the words in this commit message are separated by multiple spaces. Please fold out the excess so there is only a single space between words. > Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > --- > diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh > @@ -217,23 +218,25 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' ' > test_expect_success 'Call "rm" from outside the work tree' ' > mkdir repo && > + ( > + cd repo && > + git init && > + echo something >somefile && > + git add somefile && > + git commit -m "add a file" && > + ( > + cd .. && > + git --git-dir=repo/.git --work-tree=repo rm somefile > + ) && > + test_must_fail git ls-files --error-unmatch somefile > + ) > ' This test is unusual in that it first cd's into a subdirectory and then cd's back out with "cd ..". And, while the use of subshells is correct to ensure that all 'cd' commands are undone at the end of the test (whether successful or not), the entire construction is unnecessarily confusing. This is not the sort of issue which should be fixed in this style-fix patch, however, it is something which could be cleaned up with a follow-up patch. For instance, the test might be reworked like this: git init repo && ( cd repo && echo something >somefile && git add somefile && git commit -m "add a file" ) && git --git-dir=repo/.git --work-tree=repo rm somefile && test_must_fail git -C repo ls-files --error-unmatch somefile It's up to you whether you actually want to include such a follow-up patch in your series; it's certainly not a requirement. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v3 2/3] t3600: modernize style 2019-03-05 0:36 ` Eric Sunshine @ 2019-03-05 12:44 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2019-03-05 12:44 UTC (permalink / raw) To: Eric Sunshine Cc: Rohit Ashiwal, Johannes Schindelin, Christian Couder, Git List, Thomas Gummerer Eric Sunshine <sunshine@sunshineco.com> writes: > This test is unusual in that it first cd's into a subdirectory and > then cd's back out with "cd ..". And, while the use of subshells is > correct to ensure that all 'cd' commands are undone at the end of the > test (whether successful or not), the entire construction is > unnecessarily confusing. This is not the sort of issue which should be > fixed in this style-fix patch, however, it is something which could be > cleaned up with a follow-up patch. For instance, the test might be > reworked like this: > > git init repo && > ( > cd repo && > echo something >somefile && > git add somefile && > git commit -m "add a file" > ) && > git --git-dir=repo/.git --work-tree=repo rm somefile && > test_must_fail git -C repo ls-files --error-unmatch somefile > > It's up to you whether you actually want to include such a follow-up > patch in your series; it's certainly not a requirement. I missed that. As you said, it can be left for further clean-up. ^ permalink raw reply [flat|nested] 37+ messages in thread
* [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path> 2019-03-04 12:07 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Rohit Ashiwal 2019-03-04 12:07 ` [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal 2019-03-04 12:08 ` [GSoC][PATCH v3 2/3] t3600: modernize style Rohit Ashiwal @ 2019-03-04 12:08 ` Rohit Ashiwal 2019-03-05 0:42 ` Eric Sunshine 2019-03-05 0:09 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Eric Sunshine 3 siblings, 1 reply; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-04 12:08 UTC (permalink / raw) To: rohit.ashiwal265 Cc: Johannes.Schindelin, christian.couder, git, gitster, t.gummerer 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> --- t/t3600-rm.sh | 150 +++++++++++++++++++++++++------------------------- 1 file changed, 75 insertions(+), 75 deletions(-) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 8b03897a65..85ae7dc1e4 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -26,7 +26,7 @@ test_expect_success FUNNYNAMES 'add files with funny names' ' ' test_expect_success 'Pre-check that foo exists and is in index before git rm foo' ' - test -f foo && + test_path_is_file foo && git ls-files --error-unmatch foo ' @@ -69,12 +69,12 @@ test_expect_success 'Test that git rm --cached -f foo works in case where --cach ' test_expect_success 'Post-check that foo exists but is not in index after git rm foo' ' - test -f 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"' ' - test -f bar && + test_path_is_file bar && git ls-files --error-unmatch bar ' @@ -83,7 +83,7 @@ test_expect_success 'Test that "git rm bar" succeeds' ' ' test_expect_success 'Post-check that bar does not exist and is not in index after "git rm -f bar"' ' - ! test -f bar && + test_path_is_missing bar && test_must_fail git ls-files --error-unmatch bar ' @@ -138,15 +138,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 ' @@ -160,15 +160,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 ' @@ -195,21 +195,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' ' @@ -236,7 +236,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 ' test_expect_success 'choking "git rm" should not let it die with cruft' ' @@ -257,7 +257,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 @@ -295,7 +295,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 && @@ -317,7 +317,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 && @@ -328,7 +328,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 ' @@ -346,12 +346,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 && @@ -362,8 +362,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 && @@ -374,7 +374,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 ' @@ -384,8 +384,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 ' @@ -395,15 +395,15 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta git submodule update && 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_file_not_empty actual.err && + 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 ' @@ -416,8 +416,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 ' @@ -427,12 +427,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 ' @@ -442,12 +442,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 ' @@ -484,7 +484,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 ' @@ -496,12 +496,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 && @@ -515,12 +515,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 && @@ -534,12 +534,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 ' @@ -556,13 +556,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 && @@ -574,7 +574,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 ' @@ -591,10 +591,10 @@ 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_file_not_empty actual && test_i18ngrep Migrating output.err ' @@ -620,7 +620,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 ' @@ -630,12 +630,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 ' @@ -645,12 +645,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 ' @@ -660,12 +660,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 ' @@ -680,10 +680,10 @@ 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_file_not_empty actual && test_i18ngrep Migrating output.err ' -- Rohit ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path> 2019-03-04 12:08 ` [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path> Rohit Ashiwal @ 2019-03-05 0:42 ` Eric Sunshine 2019-03-05 13:42 ` Rohit Ashiwal 0 siblings, 1 reply; 37+ messages in thread From: Eric Sunshine @ 2019-03-05 0:42 UTC (permalink / raw) To: Rohit Ashiwal Cc: Johannes Schindelin, Christian Couder, Git List, Junio C Hamano, Thomas Gummerer On Mon, Mar 4, 2019 at 7:09 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote: > 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. As with the commit message of 2/3, many of the words in this message are separated by multiple spaced. Please fold out the excess so there is only a single space between words. Also, no need to say "previously" since readers know that the patch is changing something. Rewrite in imperative mood: Take advantage of helper functions test_path_is_dir(), test_path_is_missing(), etc. to replace `test -d|f|e|s` since the functions make the code more readable and have better error messages. > 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> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path> 2019-03-05 0:42 ` Eric Sunshine @ 2019-03-05 13:42 ` Rohit Ashiwal 2019-03-05 14:03 ` Eric Sunshine 0 siblings, 1 reply; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-05 13:42 UTC (permalink / raw) To: sunshine Cc: Johannes.Schindelin, christian.couder, git, gitster, rohit.ashiwal265, t.gummerer Hey Eric On 2019-03-05 0:42 Eric Sunshine <sunshine@sunshineco.com> wrote: > As with the commit message of 2/3, many of the words in this message > are separated by multiple spaced. Please fold out the excess so there > is only a single space between words. > > Also, no need to say "previously" since readers know that the patch is > changing something. Rewrite in imperative mood: Okay, I'll keep that in mind from next time onwards. The spaces were provided to make the commit message look aesthetically pleasing. These changes aside, is there anything you would like to add to the review? or is it good to go for a merge? Thanks for advice Rohit ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path> 2019-03-05 13:42 ` Rohit Ashiwal @ 2019-03-05 14:03 ` Eric Sunshine 2019-03-05 14:21 ` [GSoC][PATCH v2 " Rohit Ashiwal 0 siblings, 1 reply; 37+ messages in thread From: Eric Sunshine @ 2019-03-05 14:03 UTC (permalink / raw) To: Rohit Ashiwal Cc: Johannes Schindelin, Christian Couder, Git List, Junio C Hamano, Thomas Gummerer On Tue, Mar 5, 2019 at 8:43 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote: > On 2019-03-05 0:42 Eric Sunshine <sunshine@sunshineco.com> wrote: > > As with the commit message of 2/3, many of the words in this message > > are separated by multiple spaced. Please fold out the excess so there > > is only a single space between words. > > > > Also, no need to say "previously" since readers know that the patch is > > changing something. Rewrite in imperative mood: > > Okay, I'll keep that in mind from next time onwards. The spaces were > provided to make the commit message look aesthetically pleasing. > > These changes aside, is there anything you would like to add to the review? > or is it good to go for a merge? I don't understand your question. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> 2019-03-05 14:03 ` Eric Sunshine @ 2019-03-05 14:21 ` Rohit Ashiwal 2019-03-05 14:57 ` Eric Sunshine 0 siblings, 1 reply; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-05 14:21 UTC (permalink / raw) To: sunshine Cc: Johannes.Schindelin, christian.couder, git, gitster, rohit.ashiwal265, t.gummerer Eric I was asking if this patch is good enough to be added to the existing code? Does this patch look good? Regards Rohit ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> 2019-03-05 14:21 ` [GSoC][PATCH v2 " Rohit Ashiwal @ 2019-03-05 14:57 ` Eric Sunshine 2019-03-05 23:38 ` Rohit Ashiwal 2019-03-08 5:38 ` [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> Junio C Hamano 0 siblings, 2 replies; 37+ messages in thread From: Eric Sunshine @ 2019-03-05 14:57 UTC (permalink / raw) To: Rohit Ashiwal Cc: Johannes Schindelin, Christian Couder, Git List, Junio C Hamano, Thomas Gummerer On Tue, Mar 5, 2019 at 9:22 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote: > I was asking if this patch is good enough to be added to the > existing code? Does this patch look good? I didn't review the patch with a critical-enough eye to be able to say that every change maintains fidelity with the original code. As mentioned in [1]: [...] an important reason for limiting the scope of this change [...] is to ease the burden on people who review your submission. Large patch series tend to tax reviewers heavily, even (and often) when repetitive and simple, like replacing `test -d` with `test_path_is_dir()`. The shorter and more concise a patch series is, the more likely that it will receive quality reviews. This patch, due to its length and repetitive nature, falls under the category of being tedious to review, which makes it all the more likely that a reviewer will overlook a problem. And, it's not always obvious at a glance that a change is correct. For instance, taking a look at the final patch band: - ! test -d submod && - ! test -d submod/subsubmod/.git && + test_path_is_missing submod && + test_path_is_missing submod/subsubmod/.git && Superficially, the transformation seems straightforward. However, that doesn't mean it maintains fidelity with the original or even means the same thing. To review this change properly requires understanding the original intent of "! test -d". The meaning of that expression can vary depending upon the context. Is it checking that that path is not a directory (but it is okay if a plain file exists there)? Or does it merely care about existence (neither directory nor any other type of entry)? If the latter, then the transformation is probably correct, however, if the former, then it likely isn't correct. So, understanding the overall context of the test is important for judging if a particular change is correct, and many (volunteer) reviewers simply don't have the time to delve that deeply to make a proper judgment. [1]: https://public-inbox.org/git/CAPig+cSZZaCT0G3hysmjn_tNvZmYGp=5cXpZHkdphbWXnONSVQ@mail.gmail.com/ ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: 2019-03-05 14:57 ` Eric Sunshine @ 2019-03-05 23:38 ` Rohit Ashiwal 2019-03-08 5:38 ` [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> Junio C Hamano 1 sibling, 0 replies; 37+ messages in thread From: Rohit Ashiwal @ 2019-03-05 23:38 UTC (permalink / raw) To: sunshine Cc: Johannes.Schindelin, christian.couder, git, gitster, rohit.ashiwal265, t.gummerer Hey Eric On Tue, 5 Mar 2019 09:57:40 -0500 Eric Sunshine <sunshine@sunshineco.com> wrote: > This patch, due to its length and repetitive nature, falls under the > category of being tedious to review, which makes it all the more > likely that a reviewer will overlook a problem. Yes, I clearly understand that this patch has become too big to review. It will require time to carefully review and reviewers are doing their best to maintain the utmost quality of code. > And, it's not always obvious at a glance that a change is correct. For > instance, taking a look at the final patch band: > > - ! test -d submod && > - ! test -d submod/subsubmod/.git && > + test_path_is_missing submod && > + test_path_is_missing submod/subsubmod/.git && Duy actually confirms that this transformation is correct in this[1] email. (I know that, it was given as an example, but I'll leave the link anyway). Thanks Rohit [1]: https://public-inbox.org/git/CACsJy8BYeLvB7BSM_Jt4vwfGsEBuhaCZfzGPOHe=B=7cvnRwrg@mail.gmail.com/ ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> 2019-03-05 14:57 ` Eric Sunshine 2019-03-05 23:38 ` Rohit Ashiwal @ 2019-03-08 5:38 ` Junio C Hamano 2019-03-08 9:51 ` Eric Sunshine 1 sibling, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2019-03-08 5:38 UTC (permalink / raw) To: Eric Sunshine Cc: Rohit Ashiwal, Johannes Schindelin, Christian Couder, Git List, Thomas Gummerer Eric Sunshine <sunshine@sunshineco.com> writes: > This patch, due to its length and repetitive nature, falls under the > category of being tedious to review, which makes it all the more > likely that a reviewer will overlook a problem. > > And, it's not always obvious at a glance that a change is correct. For > instance, taking a look at the final patch band: > > - ! test -d submod && > - ! test -d submod/subsubmod/.git && > + test_path_is_missing submod && > + test_path_is_missing submod/subsubmod/.git && > > Superficially, the transformation seems straightforward. However, that > doesn't mean it maintains fidelity with the original or even means the > same thing. To review this change properly requires understanding the > original intent of "! test -d". > > ... , and > many (volunteer) reviewers simply don't have the time to delve that > deeply to make a proper judgment. True. The microproject was supposed to be a gentle introduction to and a practice session of the process of modifying, committing, submitting, and responding to reviews. Learning the usual Git contributor workflow, without spending too much community resources like reviewers' time (as opposed to the real "here is my itch; let's improve the system, and please help me doing so"). In any case, I have spent some time with the patch and I think the changes are generaly OK; some (like using "test_path_is_missing foo" instead of "test ! -f foo" after "git rm foo" to ensure the path no longer exists) are improvements. An unrelated tangent, but what do you think of this patch? In the context of testing "git rm", if foo is a dangling symbolic link, "git rm foo && test_path_is_missing foo" would need something like this to work correctly, I would think. t/test-lib-functions.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 681c41ba32..dfe0d4aff4 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -603,7 +603,7 @@ test_file_not_empty () { } test_path_is_missing () { - if test -e "$1" + if test -e "$1" || test -L "$1" then echo "Path exists:" ls -ld "$1" ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> 2019-03-08 5:38 ` [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> Junio C Hamano @ 2019-03-08 9:51 ` Eric Sunshine 2019-03-11 1:54 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Eric Sunshine @ 2019-03-08 9:51 UTC (permalink / raw) To: Junio C Hamano Cc: Rohit Ashiwal, Johannes Schindelin, Christian Couder, Git List, Thomas Gummerer On Fri, Mar 8, 2019 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote: > An unrelated tangent, but what do you think of this patch? In the > context of testing "git rm", if foo is a dangling symbolic link, > "git rm foo && test_path_is_missing foo" would need something like > this to work correctly, I would think. > > test_path_is_missing () { > - if test -e "$1" > + if test -e "$1" || test -L "$1" > then > echo "Path exists:" > ls -ld "$1" Makes sense. Won't we also want: test_path_exists () { - if ! test -e "$1" + if ! test -e "$1" && ! test -L "$1" then echo "Path $1 doesn't exist. $2" or something like that? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> 2019-03-08 9:51 ` Eric Sunshine @ 2019-03-11 1:54 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2019-03-11 1:54 UTC (permalink / raw) To: Eric Sunshine Cc: Rohit Ashiwal, Johannes Schindelin, Christian Couder, Git List, Thomas Gummerer Eric Sunshine <sunshine@sunshineco.com> writes: > On Fri, Mar 8, 2019 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote: >> An unrelated tangent, but what do you think of this patch? In the >> context of testing "git rm", if foo is a dangling symbolic link, >> "git rm foo && test_path_is_missing foo" would need something like >> this to work correctly, I would think. >> >> test_path_is_missing () { >> - if test -e "$1" >> + if test -e "$1" || test -L "$1" >> then >> echo "Path exists:" >> ls -ld "$1" > > Makes sense. Won't we also want: > > test_path_exists () { > - if ! test -e "$1" > + if ! test -e "$1" && ! test -L "$1" > then > echo "Path $1 doesn't exist. $2" > > or something like that? That would make them symmetric, but what I was driving at with "In the context of testing git rm" was that I highly suspect that among other existing users of test_path_is_missing there are some that want to consider a dangling symbolic link as if it is not there (and vice versa for test_path_exists), and it may not be a good idea to unconditionally declare that, unlike the underlying "test" command that dereferences symlinks for most operations, our wrapper does not dereference symbolic links, which is what the "what do you think?" patch and your addtion do. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [GSoC][PATCH v3 0/3] Use helper functions in test script 2019-03-04 12:07 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Rohit Ashiwal ` (2 preceding siblings ...) 2019-03-04 12:08 ` [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path> Rohit Ashiwal @ 2019-03-05 0:09 ` Eric Sunshine 3 siblings, 0 replies; 37+ messages in thread From: Eric Sunshine @ 2019-03-05 0:09 UTC (permalink / raw) To: Rohit Ashiwal Cc: Johannes Schindelin, Christian Couder, Git List, Junio C Hamano, Thomas Gummerer On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote: > This patch ultimately aims to replace `test -(d|f|e|s)` calls in t3600-rm.sh > Previously we were using these to verify the presence of diretory/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 > > Helper functions are better as they provide better error messages and > improve readability. They are friendly to someone new to code. As an aid to reviewers, please use the cover-letter to explain what changed since the previous version of the patch series. Also, to further help reviewers, consider using the --range-diff or --interdiff options with "git format-patch" to visually show the changes since the previous attempt (in addition to your prose explanation). Finally, it is a good idea to provide a link, like this[1], to the previous round in order to jog the memory of existing reviewers and to provide context for people new to the review of the series. [1]: https://public-inbox.org/git/20190303233750.6500-1-rohit.ashiwal265@gmail.com/ ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2019-03-11 1:54 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-03 12:28 [GSoC][PATCH 0/3] Use helper functions in test script Rohit Ashiwal 2019-03-03 12:28 ` [PATCH 1/3] test functions: Add new function `test_file_not_empty` Rohit Ashiwal 2019-03-03 13:20 ` Junio C Hamano 2019-03-03 13:29 ` Rohit Ashiwal 2019-03-03 13:33 ` none Junio C Hamano 2019-03-03 14:07 ` Clearing logic Rohit Ashiwal 2019-03-03 16:19 ` Thomas Gummerer 2019-03-03 12:28 ` [PATCH 2/3] t3600: refactor code according to contemporary guidelines Rohit Ashiwal 2019-03-03 13:30 ` Junio C Hamano 2019-03-03 14:13 ` t3600: refactor code according to comtemporary guidelines Rohit Ashiwal 2019-03-03 12:28 ` [PATCH 3/3] t3600: use helper functions from test-lib-functions Rohit Ashiwal 2019-03-03 13:32 ` Junio C Hamano 2019-03-03 23:37 ` [GSoC][PATCH v2 0/3] Use helper functions in test script Rohit Ashiwal 2019-03-03 23:37 ` [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal 2019-03-04 3:45 ` Junio C Hamano 2019-03-03 23:37 ` [GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines Rohit Ashiwal 2019-03-04 4:17 ` Junio C Hamano 2019-03-03 23:37 ` [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> Rohit Ashiwal 2019-03-04 12:07 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Rohit Ashiwal 2019-03-04 12:07 ` [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal 2019-03-05 0:17 ` Eric Sunshine 2019-03-05 12:43 ` Junio C Hamano 2019-03-05 13:27 ` [GSoc][PATCH " Rohit Ashiwal 2019-03-04 12:08 ` [GSoC][PATCH v3 2/3] t3600: modernize style Rohit Ashiwal 2019-03-05 0:36 ` Eric Sunshine 2019-03-05 12:44 ` Junio C Hamano 2019-03-04 12:08 ` [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path> Rohit Ashiwal 2019-03-05 0:42 ` Eric Sunshine 2019-03-05 13:42 ` Rohit Ashiwal 2019-03-05 14:03 ` Eric Sunshine 2019-03-05 14:21 ` [GSoC][PATCH v2 " Rohit Ashiwal 2019-03-05 14:57 ` Eric Sunshine 2019-03-05 23:38 ` Rohit Ashiwal 2019-03-08 5:38 ` [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> Junio C Hamano 2019-03-08 9:51 ` Eric Sunshine 2019-03-11 1:54 ` Junio C Hamano 2019-03-05 0:09 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Eric Sunshine
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.