{"thread":{"id":"50631","subject":"[GSoC][PATCH 0/3] Use helper functions in test script","startedAt":"2019-03-03T12:29:28Z","lastAt":"2019-03-11T01:54:53Z","messageCount":37,"participants":["Rohit Ashiwal","Junio C Hamano","Thomas Gummerer","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"370510","messageId":"20190303122842.30380-1-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":null,"subject":"[GSoC][PATCH 0/3] Use helper functions in test script","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-03T12:28:39Z","receivedAt":"2019-03-03T12:29:28Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"This patch ultimately aims to replace `test -(d|f|e|s)` calls in t3600-rm.sh\nPreviously we were using these to verify the presence of diretory/file, but\nwe already have helper functions, viz, `test_path_is_dir`, `test_path_is_file`,\n`test_path_is_missing` and `test_file_not_empty` with better functionality\n\nHelper functions are better as they provide better error messages and\nimprove readability. They are friendly to someone new to code.\n\nNote: `test_file_not_empty` is implemented in [PATCH 1/3] of this mail\n\nRohit Ashiwal (3):\n test functions: Add new function `test_file_not_empty`\n t3600: refactor code according to contemporary guidelines\n t3600: use helper functions from test-lib-functions\n\n t/t3600-rm.sh | 281 +++++++++++++++++++++-------------------\n t/test-lib-functions.sh | 10 ++\n 2 files changed, 157 insertions(+), 134 deletions(-)\n\n-- \nThanks\nRohit\n\n"},{"id":"370511","messageId":"20190303122842.30380-2-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"20190303122842.30380-1-rohit.ashiwal265@gmail.com","subject":"[PATCH 1/3] test functions: Add new function `test_file_not_empty`","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-03T12:28:40Z","receivedAt":"2019-03-03T12:29:31Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"test-lib-functions: add a helper function that checks for a file and that\nthe file is not empty. The helper function will provide better error message\nin case of failure and improve readability\n\nSigned-off-by: Rohit Ashiwal \n---\n t/test-lib-functions.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 80402a428f..1302df63b6 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -593,6 +593,16 @@ test_dir_is_empty () {\n \tfi\n }\n \n+# Check if the file exists and has a size greater than zero\n+test_file_not_empty () {\n+\ttest_path_is_file \"$1\" &&\n+\tif ! test -s \"$1\"\n+\tthen\n+\t\techo \"'$1' is an empty file.\"\n+\t\tfalse\n+\tfi\n+}\n+\n test_path_is_missing () {\n \tif test -e \"$1\"\n \tthen\n-- \n\n"},{"id":"370512","messageId":"20190303122842.30380-3-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"20190303122842.30380-1-rohit.ashiwal265@gmail.com","subject":"[PATCH 2/3] t3600: refactor code according to contemporary guidelines","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-03T12:28:41Z","receivedAt":"2019-03-03T12:29:35Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Replace leading spaces with tabs\n\nThe previous code of `t3600-rm.sh` had a mix use of tabs and spaces with\ninstance of `not-so-recommended` way of writing `if-then` statement,\nreplace them so that the current version agrees with the coding guidelines\n\nSigned-off-by: Rohit Ashiwal \n---\n t/t3600-rm.sh | 131 ++++++++++++++++++++++++++------------------------\n 1 file changed, 68 insertions(+), 63 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 04e5d42bd3..ec4877bfec 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -9,89 +9,94 @@ test_description='Test of the various options to git rm.'\n \n # Setup some files to be removed, some with funny characters\n test_expect_success \\\n- 'Initialize test directory' \\\n- \"touch -- foo bar baz 'space embedded' -q &&\n- git add -- foo bar baz 'space embedded' -q &&\n- git commit -m 'add normal files'\"\n+\t'Initialize test directory' \"\n+\ttouch -- foo bar baz 'space embedded' -q &&\n+\tgit add -- foo bar baz 'space embedded' -q &&\n+\tgit commit -m 'add normal files'\n+\"\n \n-if test_have_prereq !FUNNYNAMES; then\n+if test_have_prereq !FUNNYNAMES\n+then\n \tsay 'Your filesystem does not allow tabs in filenames.'\n fi\n \n test_expect_success FUNNYNAMES 'add files with funny names' \"\n- touch -- 'tab\tembedded' 'newline\n+\ttouch -- 'tab\tembedded' 'newline\n embedded' &&\n- git add -- 'tab\tembedded' 'newline\n+\tgit add -- 'tab\tembedded' 'newline\n embedded' &&\n- git commit -m 'add files with tabs and newlines'\n+\tgit commit -m 'add files with tabs and newlines'\n \"\n \n test_expect_success \\\n- 'Pre-check that foo exists and is in index before git rm foo' \\\n- '[ -f foo ] && git ls-files --error-unmatch foo'\n+\t'Pre-check that foo exists and is in index before git rm foo' \\\n+\t'[ -f foo ] && git ls-files --error-unmatch foo'\n \n test_expect_success \\\n- 'Test that git rm foo succeeds' \\\n- 'git rm --cached foo'\n+\t'Test that git rm foo succeeds' \\\n+\t'git rm --cached foo'\n \n test_expect_success \\\n- 'Test that git rm --cached foo succeeds if the index matches the file' \\\n- 'echo content >foo &&\n- git add foo &&\n- git rm --cached foo'\n+\t'Test that git rm --cached foo succeeds if the index matches the file' '\n+\techo content >foo &&\n+\tgit add foo &&\n+\tgit rm --cached foo\n+'\n \n test_expect_success \\\n- 'Test that git rm --cached foo succeeds if the index matches the file' \\\n- 'echo content >foo &&\n- git add foo &&\n- git commit -m foo &&\n- echo \"other content\" >foo &&\n- git rm --cached foo'\n+\t'Test that git rm --cached foo succeeds if the index matches the file' '\n+\techo content >foo &&\n+\tgit add foo &&\n+\tgit commit -m foo &&\n+\techo \"other content\" >foo &&\n+\tgit rm --cached foo\n+'\n \n test_expect_success \\\n- 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '\n- echo content >foo &&\n- git add foo &&\n- git commit -m foo --allow-empty &&\n- echo \"other content\" >foo &&\n- git add foo &&\n- echo \"yet another content\" >foo &&\n- test_must_fail git rm --cached foo\n+\t'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '\n+\techo content >foo &&\n+\tgit add foo &&\n+\tgit commit -m foo --allow-empty &&\n+\techo \"other content\" >foo &&\n+\tgit add foo &&\n+\techo \"yet another content\" >foo &&\n+\ttest_must_fail git rm --cached foo\n '\n \n test_expect_success \\\n- 'Test that git rm --cached -f foo works in case where --cached only did not' \\\n- 'echo content >foo &&\n- git add foo &&\n- git commit -m foo --allow-empty &&\n- echo \"other content\" >foo &&\n- git add foo &&\n- echo \"yet another content\" >foo &&\n- git rm --cached -f foo'\n+\t'Test that git rm --cached -f foo works in case where --cached only did not' '\n+\techo content >foo &&\n+\tgit add foo &&\n+\tgit commit -m foo --allow-empty &&\n+\techo \"other content\" >foo &&\n+\tgit add foo &&\n+\techo \"yet another content\" >foo &&\n+\tgit rm --cached -f foo\n+'\n \n test_expect_success \\\n- 'Post-check that foo exists but is not in index after git rm foo' \\\n- '[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'\n+\t'Post-check that foo exists but is not in index after git rm foo' \\\n+\t'[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'\n \n test_expect_success \\\n- 'Pre-check that bar exists and is in index before \"git rm bar\"' \\\n- '[ -f bar ] && git ls-files --error-unmatch bar'\n+\t'Pre-check that bar exists and is in index before \"git rm bar\"' \\\n+\t'[ -f bar ] && git ls-files --error-unmatch bar'\n \n test_expect_success \\\n- 'Test that \"git rm bar\" succeeds' \\\n- 'git rm bar'\n+\t'Test that \"git rm bar\" succeeds' \\\n+\t'git rm bar'\n \n test_expect_success \\\n- 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' \\\n- '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'\n+\t'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' \\\n+\t'! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'\n \n test_expect_success \\\n- 'Test that \"git rm -- -q\" succeeds (remove a file that looks like an option)' \\\n- 'git rm -- -q'\n+\t'Test that \"git rm -- -q\" succeeds (remove a file that looks like an option)' \\\n+\t'git rm -- -q'\n \n test_expect_success FUNNYNAMES \\\n- \"Test that \\\"git rm -f\\\" succeeds with embedded space, tab, or newline characters.\" \\\n- \"git rm -f 'space embedded' 'tab\tembedded' 'newline\n+\t\"Test that \\\"git rm -f\\\" succeeds with embedded space, tab, or newline characters.\" \\\n+\t\"git rm -f 'space embedded' 'tab\tembedded' 'newline\n embedded'\"\n \n test_expect_success SANITY 'Test that \"git rm -f\" fails if its rm fails' '\n@@ -101,8 +106,8 @@ test_expect_success SANITY 'Test that \"git rm -f\" fails if its rm fails' '\n '\n \n test_expect_success \\\n- 'When the rm in \"git rm -f\" fails, it should not remove the file from the index' \\\n- 'git ls-files --error-unmatch baz'\n+\t'When the rm in \"git rm -f\" fails, it should not remove the file from the index' \\\n+\t'git ls-files --error-unmatch baz'\n \n test_expect_success 'Remove nonexistent file with --ignore-unmatch' '\n \tgit rm --ignore-unmatch nonexistent\n@@ -218,22 +223,22 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n test_expect_success 'Call \"rm\" from outside the work tree' '\n \tmkdir repo &&\n \t(cd repo &&\n-\t git init &&\n-\t echo something >somefile &&\n-\t git add somefile &&\n-\t git commit -m \"add a file\" &&\n-\t (cd .. &&\n-\t git --git-dir=repo/.git --work-tree=repo rm somefile) &&\n-\ttest_must_fail git ls-files --error-unmatch somefile)\n+\t\tgit init &&\n+\t\techo something >somefile &&\n+\t\tgit add somefile &&\n+\t\tgit commit -m \"add a file\" &&\n+\t\t(cd .. &&\n+\t\t\tgit --git-dir=repo/.git --work-tree=repo rm somefile\n+\t\t) &&\n+\t\ttest_must_fail git ls-files --error-unmatch somefile\n+\t)\n '\n \n test_expect_success 'refresh index before checking if it is up-to-date' '\n-\n \tgit reset --hard &&\n \ttest-tool chmtime -86400 frotz/nitfol &&\n \tgit rm frotz/nitfol &&\n \ttest ! -f frotz/nitfol\n-\n '\n \n test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n@@ -242,8 +247,8 @@ test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n \ti=0 &&\n \twhile test $i -lt 12000\n \tdo\n-\t echo \"100644 1234567890123456789012345678901234567890 0\tsome-file-$i\"\n-\t i=$(( $i + 1 ))\n+\t\techo \"100644 1234567890123456789012345678901234567890 0\tsome-file-$i\"\n+\t\ti=$(( $i + 1 ))\n \tdone | git update-index --index-info &&\n \tgit rm -n \"some-file-*\" | : &&\n \ttest_path_is_missing .git/index.lock\n-- \n\n"},{"id":"370513","messageId":"20190303122842.30380-4-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"20190303122842.30380-1-rohit.ashiwal265@gmail.com","subject":"[PATCH 3/3] t3600: use helper functions from test-lib-functions","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-03T12:28:42Z","receivedAt":"2019-03-03T12:29:38Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Replace `test -(d|f|e|s)` calls in `t3600-rm.sh`.\n\nPreviously we were using `test -(d|f|e|s)` to verify the presence of a\ndirectory/file, but we already have helper functions, viz,\n`test_path_is_dir`, `test_path_is_file`, `test_path_is_missing` and\n`test_file_not_empty` with better functionality.\n\nThese helper functions make code more readable and informative to someone\nnew, also these functions have better error messages.\n\nSigned-off-by: Rohit Ashiwal \n---\n t/t3600-rm.sh | 166 ++++++++++++++++++++++++++------------------------\n 1 file changed, 87 insertions(+), 79 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex ec4877bfec..c2391b7d56 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -29,8 +29,10 @@ embedded' &&\n \"\n \n test_expect_success \\\n-\t'Pre-check that foo exists and is in index before git rm foo' \\\n-\t'[ -f foo ] && git ls-files --error-unmatch foo'\n+\t'Pre-check that foo exists and is in index before git rm foo' '\n+\ttest_path_is_file foo &&\n+\tgit ls-files --error-unmatch foo\n+'\n \n test_expect_success \\\n \t'Test that git rm foo succeeds' \\\n@@ -75,20 +77,26 @@ test_expect_success \\\n '\n \n test_expect_success \\\n-\t'Post-check that foo exists but is not in index after git rm foo' \\\n-\t'[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'\n+\t'Post-check that foo exists but is not in index after git rm foo' '\n+\ttest_path_is_file foo &&\n+\ttest_must_fail git ls-files --error-unmatch foo\n+'\n \n test_expect_success \\\n-\t'Pre-check that bar exists and is in index before \"git rm bar\"' \\\n-\t'[ -f bar ] && git ls-files --error-unmatch bar'\n+\t'Pre-check that bar exists and is in index before \"git rm bar\"' '\n+\ttest_path_is_file bar &&\n+\tgit ls-files --error-unmatch bar\n+'\n \n test_expect_success \\\n \t'Test that \"git rm bar\" succeeds' \\\n \t'git rm bar'\n \n test_expect_success \\\n-\t'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' \\\n-\t'! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'\n+\t'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' '\n+\ttest_path_is_missing bar &&\n+\ttest_must_fail git ls-files --error-unmatch bar\n+'\n \n test_expect_success \\\n \t'Test that \"git rm -- -q\" succeeds (remove a file that looks like an option)' \\\n@@ -142,15 +150,15 @@ test_expect_success 'Re-add foo and baz' '\n test_expect_success 'Modify foo -- rm should refuse' '\n \techo >>foo &&\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n test_expect_success 'Modified foo -- rm -f should work' '\n \tgit rm -f foo baz &&\n-\ttest ! -f foo &&\n-\ttest ! -f baz &&\n+\ttest_path_is_missing foo &&\n+\ttest_path_is_missing baz &&\n \ttest_must_fail git ls-files --error-unmatch foo &&\n \ttest_must_fail git ls-files --error-unmatch bar\n '\n@@ -164,15 +172,15 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '\n \n test_expect_success 'foo is different in index from HEAD -- rm should refuse' '\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n test_expect_success 'but with -f it should work.' '\n \tgit rm -f foo baz &&\n-\ttest ! -f foo &&\n-\ttest ! -f baz &&\n+\ttest_path_is_missing foo &&\n+\ttest_path_is_missing baz &&\n \ttest_must_fail git ls-files --error-unmatch foo &&\n \ttest_must_fail git ls-files --error-unmatch baz\n '\n@@ -199,21 +207,21 @@ test_expect_success 'Recursive test setup' '\n \n test_expect_success 'Recursive without -r fails' '\n \ttest_must_fail git rm frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r but dirty' '\n \techo qfwfq >>frotz/nitfol &&\n \ttest_must_fail git rm -r frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r -f' '\n \tgit rm -f -r frotz &&\n-\t! test -f frotz/nitfol &&\n-\t! test -d frotz\n+\ttest_path_is_missing frotz/nitfol &&\n+\ttest_path_is_missing frotz\n '\n \n test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n@@ -238,7 +246,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '\n \tgit reset --hard &&\n \ttest-tool chmtime -86400 frotz/nitfol &&\n \tgit rm frotz/nitfol &&\n-\ttest ! -f frotz/nitfol\n+\ttest_path_is_missing frotz/nitfol\n '\n \n test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n@@ -259,7 +267,7 @@ test_expect_success 'rm removes subdirectories recursively' '\n \techo content >dir/subdir/subsubdir/file &&\n \tgit add dir/subdir/subsubdir/file &&\n \tgit rm -f dir/subdir/subsubdir/file &&\n-\t! test -d dir\n+\ttest_path_is_missing dir\n '\n \n cat >expect <actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -319,7 +327,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -330,7 +338,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm submod/ &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -348,12 +356,12 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles\n \tgit submodule update &&\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -364,8 +372,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm --cached submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.cached actual &&\n \tgit config -f .gitmodules submodule.sub.url &&\n@@ -376,7 +384,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm -n submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-index --exit-code HEAD\n '\n \n@@ -386,8 +394,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '\n \tgit rm .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.both_deleted actual\n '\n@@ -397,15 +405,15 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta\n \tgit submodule update &&\n \tgit config -f .gitmodules foo.bar true &&\n \ttest_must_fail git rm submod >actual 2>actual.err &&\n-\ttest -s actual.err &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_file_not_empty actual.err &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-files --quiet -- submod &&\n \tgit add .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -418,8 +426,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule\n \techo \"warning: Could not find section in .gitmodules where path=submod\" >expect.err &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_i18ncmp expect.err actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -429,12 +437,12 @@ test_expect_success 'rm of a populated submodule with modifications fails unless\n \tgit submodule update &&\n \techo X >submod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -444,12 +452,12 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \tgit submodule update &&\n \techo X >submod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -486,7 +494,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '\n \tgit submodule update &&\n \ttest_must_fail git merge conflict2 &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -498,12 +506,12 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -517,12 +525,12 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f\n \techo X >submod/empty &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -536,12 +544,12 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files\n \techo X >submod/untracked &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -557,13 +565,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director\n \t) &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \ttest_must_fail git rm -f submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit merge --abort &&\n@@ -575,7 +583,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '\n \tgit reset --hard &&\n \ttest_must_fail git merge conflict2 &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -591,10 +599,10 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates\n \t\trm -r ../.git/modules/sub\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest -s actual &&\n+\ttest_file_not_empty actual &&\n \ttest_i18ngrep Migrating output.err\n '\n \n@@ -619,7 +627,7 @@ test_expect_success 'setup subsubmodule' '\n \n test_expect_success 'rm recursively removes work tree of unmodified submodules' '\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -629,12 +637,12 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \tgit submodule update --recursive &&\n \tgit -C submod/subsubmod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -644,12 +652,12 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -659,12 +667,12 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -678,10 +686,10 @@ test_expect_success \"rm absorbs submodule's nested .git directory\" '\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/subsubmod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/subsubmod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest -s actual &&\n+\ttest_file_not_empty actual &&\n \ttest_i18ngrep Migrating output.err\n '\n \n-- \n\n"},{"id":"370515","messageId":"xmqq5zt014du.fsf@gitster-ct.c.googlers.com","threadId":"50631","inReplyTo":"20190303122842.30380-2-rohit.ashiwal265@gmail.com","subject":"Re: [PATCH 1/3] test functions: Add new function `test_file_not_empty`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-03T13:20:13Z","receivedAt":"2019-03-03T13:20:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rohit Ashiwal writes:\n\n> Subject: Re: [PATCH 1/3] test functions: Add new function `test_file_not_empty`\n\ns/Add/add/. Strictly speaking, you do not need to say \"new\", if you\nare already saying \"add\", then that's redundant.\n\n> test-lib-functions: add a helper function that checks for a file and that\n> the file is not empty. The helper function will provide better error message\n> in case of failure and improve readability\n>\n> Signed-off-by: Rohit Ashiwal \n> ---\n> t/test-lib-functions.sh | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 80402a428f..1302df63b6 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -593,6 +593,16 @@ test_dir_is_empty () {\n> \tfi\n> }\n> \n> +# Check if the file exists and has a size greater than zero\n> +test_file_not_empty () {\n> +\ttest_path_is_file \"$1\" &&\n> +\tif ! test -s \"$1\"\n\n\"test -s \" is true if resolves to an existing directory\nentry for a file that has a size greater than zero. Isn't it\nredundant and wasteful to have test_path_is_file before it, or is\nthere a situation where \"test -s\" alone won't give us what we want\nto check?\n\n> +\tthen\n> +\t\techo \"'$1' is an empty file.\"\n> +\t\tfalse\n> +\tfi\n> +}\n> +\n> test_path_is_missing () {\n> \tif test -e \"$1\"\n> \tthen\n"},{"id":"370516","messageId":"20190303132900.4618-1-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"xmqq5zt014du.fsf@gitster-ct.c.googlers.com","subject":"","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-03T13:29:00Z","receivedAt":"2019-03-03T13:29:34Z","isPatch":false,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Hey Junio\n\nOn 2019-03-03 13:20 UTC Junio C Hamano wrote:\n\n> s/Add/add/. Strictly speaking, you do not need to say \"new\", if you\n> are already saying \"add\", then that's redundant.\n\nOh, my mistake, I will change in coming revisions.\n\n> \"test -s \" is true if resolves to an existing directory\n> entry for a file that has a size greater than zero. Isn't it\n> redundant and wasteful to have test_path_is_file before it, or is\n> there a situation where \"test -s\" alone won't give us what we want\n> to check?\n\nJust to be clear of what caused the error:\n\t1. Path not being file, or\n\t2. File not being empty\nI am checking for both.\n\nRegards\nRohit\n\n"},{"id":"370517","messageId":"xmqqwolgytk5.fsf@gitster-ct.c.googlers.com","threadId":"50631","inReplyTo":"20190303122842.30380-3-rohit.ashiwal265@gmail.com","subject":"Re: [PATCH 2/3] t3600: refactor code according to contemporary guidelines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-03T13:30:02Z","receivedAt":"2019-03-03T13:30:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rohit Ashiwal writes:\n\n> Subject: Re: [PATCH 2/3] t3600: refactor code according to contemporary guidelines\n\nPlease do not overuse/abuse the verb \"refactor\" like this. What the\npatch does is only reformat---it does not do common \"refactoring\"\ntransformations like factoring out common/duplicated code into\nhelper functions, etc.\n\nIf we are doing this step, let's make sure all tests use the modern\nstyle correctly.\n\n> # Setup some files to be removed, some with funny characters\n> test_expect_success \\\n> - 'Initialize test directory' \\\n> - \"touch -- foo bar baz 'space embedded' -q &&\n> - git add -- foo bar baz 'space embedded' -q &&\n> - git commit -m 'add normal files'\"\n> +\t'Initialize test directory' \"\n> +\ttouch -- foo bar baz 'space embedded' -q &&\n> +\tgit add -- foo bar baz 'space embedded' -q &&\n> +\tgit commit -m 'add normal files'\n> +\"\n\nIn the modern style, we'd write this like so:\n\n\ttest_expect_success 'initialize test directory' '\n\t\ttouch -- foo bar baz \"space embedded\" -q &&\n\t\tgit add -- foo bar baz \"space embedded\" -q &&\n\t\tgit commit -m \"add normal files\"\n\t'\n\nIn addition to indenting with HT (not SP), two more points are\n\n - test title comes on the first line;\n\n - test body is enclosed in a single quote pair, opened on the first\n line and closed on the last line.\n\n> \n> -if test_have_prereq !FUNNYNAMES; then\n> +if test_have_prereq !FUNNYNAMES\n> +then\n\nThis is good.\n\n> \tsay 'Your filesystem does not allow tabs in filenames.'\n> fi\n> \n> test_expect_success FUNNYNAMES 'add files with funny names' \"\n\nThis has title on the first line, and opening quote of the body as\nwell, which is the modern style.\n\n> test_expect_success \\\n> - 'Pre-check that foo exists and is in index before git rm foo' \\\n> - '[ -f foo ] && git ls-files --error-unmatch foo'\n> +\t'Pre-check that foo exists and is in index before git rm foo' \\\n> +\t'[ -f foo ] && git ls-files --error-unmatch foo'\n\nWe prefer \"test ...\" over \"[ ... ]\" (Documentation/CodingGuidelines).\n\nThanks.\n"},{"id":"370518","messageId":"xmqqsgw4ytgl.fsf@gitster-ct.c.googlers.com","threadId":"50631","inReplyTo":"20190303122842.30380-4-rohit.ashiwal265@gmail.com","subject":"Re: [PATCH 3/3] t3600: use helper functions from test-lib-functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-03T13:32:10Z","receivedAt":"2019-03-03T13:32:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rohit Ashiwal writes:\n\n> Subject: Re: [PATCH 3/3] t3600: use helper functions from test-lib-functions\n\nThere are tons of helpers in that lib.\n\n> Replace `test -(d|f|e|s)` calls in `t3600-rm.sh`.\n\nThis one gives more useful information than the patch title.\n\nPerhaps\n\nSubject: [PATCH 3/3] t3600: use helpers to replace test -d/f/e/s \n"},{"id":"370519","messageId":"xmqqo96syte0.fsf@gitster-ct.c.googlers.com","threadId":"50631","inReplyTo":"20190303132900.4618-1-rohit.ashiwal265@gmail.com","subject":"Re: none","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-03T13:33:43Z","receivedAt":"2019-03-03T13:33:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rohit Ashiwal writes:\n\n> Just to be clear of what caused the error:\n> \t1. Path not being file, or\n> \t2. File not being empty\n> I am checking for both.\n\ntest -s makes sure is file; if it is not a file, then\nit won't yield true.\n\nSo why do you need to say test_path_is_file yourself, if you are\nasking \"test -s\"?\n"},{"id":"370520","messageId":"20190303140709.5561-1-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"xmqqo96syte0.fsf@gitster-ct.c.googlers.com","subject":"Clearing logic","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-03T14:07:09Z","receivedAt":"2019-03-03T14:07:45Z","isPatch":false,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"On 2019-03-03 13:33 UTC Junio C Hamano wrote:\n\n> test -s makes sure is file; if it is not a file, then\n> it won't yield true.\n\n> On 2019-03-03 13:20 UTC Rohit Ashiwal wrote:\n> > test_path_is_file \"$1\" &&\n> > \tif ! test -s \"$1\"\n\nAccording to the conditional if the path is not a file then we will get\nthe error \"file does not exist\" and then we will shortcircuit without checking\nthe second conditional, on the other hand, if path is a file then we will\nagain check if it has a size greater than zero, then error will be different\n(if any).\n\nRegards\nRohit\n\n"},{"id":"370521","messageId":"20190303141358.6479-1-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"xmqqwolgytk5.fsf@gitster-ct.c.googlers.com","subject":"Re: t3600: refactor code according to comtemporary guidelines","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-03T14:13:58Z","receivedAt":"2019-03-03T14:14:32Z","isPatch":false,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"I agree to all the points that you mentioned.\n\nOn 2019-03-03 13:30 UTC Junio C Hamano wrote:\n\n> We prefer \"test ...\" over \"[ ... ]\" (Documentation/CodingGuidelines).\n\nAt first I thought this should go in [PATCH 3/3] but now I think this is\nits real place.\n\nThanks\nRohit\n\n"},{"id":"370532","messageId":"20190303161946.GX6085@hank.intra.tgummerer.com","threadId":"50631","inReplyTo":"20190303140709.5561-1-rohit.ashiwal265@gmail.com","subject":"Re: Clearing logic","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-03T16:19:46Z","receivedAt":"2019-03-03T16:19:51Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/03, Rohit Ashiwal wrote:\n> On 2019-03-03 13:33 UTC Junio C Hamano wrote:\n> \n> > test -s makes sure is file; if it is not a file, then\n> > it won't yield true.\n> \n> > On 2019-03-03 13:20 UTC Rohit Ashiwal wrote:\n> > > test_path_is_file \"$1\" &&\n> > > \tif ! test -s \"$1\"\n> \n> According to the conditional if the path is not a file then we will get\n> the error \"file does not exist\" and then we will shortcircuit without checking\n> the second conditional, on the other hand, if path is a file then we will\n> again check if it has a size greater than zero, then error will be different\n> (if any).\n\nI do agree that the better error message is probably worth the\nadditional 'test_path_is_file' before the 'test -s'. Although it may\nbe better to only make that distinction in the 'if' (and then maybe\njust using 'test -f', which would explain better why we have an\nadditional call.\n\nEither way it would be nice to describe that reasoning in the commit\nmessage, as it's not 100% clear from the code what is going on here,\nwhich also lead to Junio's question.\n\n> Regards\n> Rohit\n> \n"},{"id":"370562","messageId":"20190303233750.6500-1-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"20190303122842.30380-1-rohit.ashiwal265@gmail.com","subject":"[GSoC][PATCH v2 0/3] Use helper functions in test script","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-03T23:37:47Z","receivedAt":"2019-03-03T23:38:27Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"This patch ultimately aims to replace `test -(d|f|e|s)` calls in t3600-rm.sh\nPreviously we were using these to verify the presence of diretory/file, but\nwe already have helper functions, viz, `test_path_is_dir`, `test_path_is_file`,\n`test_path_is_missing` and `test_file_not_empty` with better functionality\n\nHelper functions are better as they provide better error messages and\nimprove readability. They are friendly to someone new to code.\n\nThanks\nRohit\n\nPS: `test_file_not_empty` is implemented in [PATCH v2 1/3] of this mail\n\nRohit Ashiwal (3):\n test functions: add function `test_file_not_empty`\n t3600: restructure code according to contemporary guidelines\n t3600: use helpers to replace test -d/f/e/s \n\n t/t3600-rm.sh | 326 ++++++++++++++++++++--------------------\n t/test-lib-functions.sh | 15 ++\n 2 files changed, 180 insertions(+), 161 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"370563","messageId":"20190303233750.6500-2-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"20190303233750.6500-1-rohit.ashiwal265@gmail.com","subject":"[GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty`","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-03T23:37:48Z","receivedAt":"2019-03-03T23:38:30Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"test-lib-functions: add a helper function that checks for a file and that\nthe file is not empty. The helper function will provide better error message\nin case of failure and improve readability\n\nThe function `test_file_not_empty`, first checks if a file is provided,\nif it is not then an error message is printed, skipping the remaining\ncode, if is indeed a file then check `test -s` is applied to check\nif size of file is greater than zero, failing which another error message\nis printed.\n\nSigned-off-by: Rohit Ashiwal \n---\n t/test-lib-functions.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 80402a428f..f9fcd2e013 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -593,6 +593,21 @@ test_dir_is_empty () {\n \tfi\n }\n \n+# Check if the file exists and has a size greater than zero\n+test_file_not_empty () {\n+\tif ! test -f \"$1\"\n+\tthen\n+\t\techo \"'$1' does not exist or not a file.\"\n+\t\tfalse\n+\telse\n+\t\tif ! test -s \"$1\"\n+\t\tthen\n+\t\t\techo \"'$1' is an empty file.\"\n+\t\t\tfalse\n+\t\tfi\n+\tfi\n+}\n+\n test_path_is_missing () {\n \tif test -e \"$1\"\n \tthen\n-- \n2.17.1\n\n"},{"id":"370564","messageId":"20190303233750.6500-3-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"20190303233750.6500-1-rohit.ashiwal265@gmail.com","subject":"[GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-03T23:37:49Z","receivedAt":"2019-03-03T23:38:33Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Replace leading spaces with tabs\nPlace title on the same line as function\n\nThe previous code of `t3600-rm.sh` had a mixed use of tabs and spaces with\ninstance of `not-so-recommended` way of writing `if-then` statement, also\n`titles` were not on the same line as the function `test_expect_success`,\nreplace them so that the current version agrees with the coding guidelines\n\nSigned-off-by: Rohit Ashiwal \n---\n t/t3600-rm.sh | 184 ++++++++++++++++++++++++++------------------------\n 1 file changed, 94 insertions(+), 90 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 04e5d42bd3..f1afda21e9 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -8,91 +8,95 @@ test_description='Test of the various options to git rm.'\n . ./test-lib.sh\n \n # Setup some files to be removed, some with funny characters\n-test_expect_success \\\n- 'Initialize test directory' \\\n- \"touch -- foo bar baz 'space embedded' -q &&\n- git add -- foo bar baz 'space embedded' -q &&\n- git commit -m 'add normal files'\"\n+test_expect_success 'Initialize test directory' \"\n+\ttouch -- foo bar baz 'space embedded' -q &&\n+\tgit add -- foo bar baz 'space embedded' -q &&\n+\tgit commit -m 'add normal files'\n+\"\n \n-if test_have_prereq !FUNNYNAMES; then\n+if test_have_prereq !FUNNYNAMES\n+then\n \tsay 'Your filesystem does not allow tabs in filenames.'\n fi\n \n test_expect_success FUNNYNAMES 'add files with funny names' \"\n- touch -- 'tab\tembedded' 'newline\n+\ttouch -- 'tab\tembedded' 'newline\n embedded' &&\n- git add -- 'tab\tembedded' 'newline\n+\tgit add -- 'tab\tembedded' 'newline\n embedded' &&\n- git commit -m 'add files with tabs and newlines'\n+\tgit commit -m 'add files with tabs and newlines'\n \"\n \n-test_expect_success \\\n- 'Pre-check that foo exists and is in index before git rm foo' \\\n- '[ -f foo ] && git ls-files --error-unmatch foo'\n-\n-test_expect_success \\\n- 'Test that git rm foo succeeds' \\\n- 'git rm --cached foo'\n-\n-test_expect_success \\\n- 'Test that git rm --cached foo succeeds if the index matches the file' \\\n- 'echo content >foo &&\n- git add foo &&\n- git rm --cached foo'\n-\n-test_expect_success \\\n- 'Test that git rm --cached foo succeeds if the index matches the file' \\\n- 'echo content >foo &&\n- git add foo &&\n- git commit -m foo &&\n- echo \"other content\" >foo &&\n- git rm --cached foo'\n-\n-test_expect_success \\\n- 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '\n- echo content >foo &&\n- git add foo &&\n- git commit -m foo --allow-empty &&\n- echo \"other content\" >foo &&\n- git add foo &&\n- echo \"yet another content\" >foo &&\n- test_must_fail git rm --cached foo\n-'\n-\n-test_expect_success \\\n- 'Test that git rm --cached -f foo works in case where --cached only did not' \\\n- 'echo content >foo &&\n- git add foo &&\n- git commit -m foo --allow-empty &&\n- echo \"other content\" >foo &&\n- git add foo &&\n- echo \"yet another content\" >foo &&\n- git rm --cached -f foo'\n-\n-test_expect_success \\\n- 'Post-check that foo exists but is not in index after git rm foo' \\\n- '[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'\n-\n-test_expect_success \\\n- 'Pre-check that bar exists and is in index before \"git rm bar\"' \\\n- '[ -f bar ] && git ls-files --error-unmatch bar'\n-\n-test_expect_success \\\n- 'Test that \"git rm bar\" succeeds' \\\n- 'git rm bar'\n-\n-test_expect_success \\\n- 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' \\\n- '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'\n-\n-test_expect_success \\\n- 'Test that \"git rm -- -q\" succeeds (remove a file that looks like an option)' \\\n- 'git rm -- -q'\n-\n-test_expect_success FUNNYNAMES \\\n- \"Test that \\\"git rm -f\\\" succeeds with embedded space, tab, or newline characters.\" \\\n- \"git rm -f 'space embedded' 'tab\tembedded' 'newline\n-embedded'\"\n+test_expect_success 'Pre-check that foo exists and is in index before git rm foo' '\n+\ttest_path_is_file foo &&\n+\tgit ls-files --error-unmatch foo\n+'\n+\n+test_expect_success 'Test that git rm foo succeeds' '\n+\tgit rm --cached foo\n+'\n+\n+test_expect_success 'Test that git rm --cached foo succeeds if the index matches the file' '\n+\techo content >foo &&\n+\tgit add foo &&\n+\tgit rm --cached foo\n+'\n+\n+test_expect_success 'Test that git rm --cached foo succeeds if the index matches the file' '\n+\techo content >foo &&\n+\tgit add foo &&\n+\tgit commit -m foo &&\n+\techo \"other content\" >foo &&\n+\tgit rm --cached foo\n+'\n+\n+test_expect_success 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '\n+\techo content >foo &&\n+\tgit add foo &&\n+\tgit commit -m foo --allow-empty &&\n+\techo \"other content\" >foo &&\n+\tgit add foo &&\n+\techo \"yet another content\" >foo &&\n+\ttest_must_fail git rm --cached foo\n+'\n+\n+test_expect_success 'Test that git rm --cached -f foo works in case where --cached only did not' '\n+\techo content >foo &&\n+\tgit add foo &&\n+\tgit commit -m foo --allow-empty &&\n+\techo \"other content\" >foo &&\n+\tgit add foo &&\n+\techo \"yet another content\" >foo &&\n+\tgit rm --cached -f foo\n+'\n+\n+test_expect_success 'Post-check that foo exists but is not in index after git rm foo' '\n+\ttest_path_is_file foo &&\n+\ttest_must_fail git ls-files --error-unmatch foo\n+'\n+\n+test_expect_success 'Pre-check that bar exists and is in index before \"git rm bar\"' '\n+\ttest_path_is_file bar &&\n+\tgit ls-files --error-unmatch bar\n+'\n+\n+test_expect_success 'Test that \"git rm bar\" succeeds' '\n+\tgit rm bar\n+'\n+\n+test_expect_success 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' '\n+\ttest_path_is_missing bar &&\n+\ttest_must_fail git ls-files --error-unmatch bar\n+'\n+\n+test_expect_success 'Test that \"git rm -- -q\" succeeds (remove a file that looks like an option)' '\n+\tgit rm -- -q\n+'\n+\n+test_expect_success FUNNYNAMES \"Test that \\\"git rm -f\\\" succeeds with embedded space, tab, or newline characters.\" \"\n+\tgit rm -f 'space embedded' 'tab\tembedded' 'newline\n+embedded'\n+\"\n \n test_expect_success SANITY 'Test that \"git rm -f\" fails if its rm fails' '\n \ttest_when_finished \"chmod 775 .\" &&\n@@ -100,9 +104,9 @@ test_expect_success SANITY 'Test that \"git rm -f\" fails if its rm fails' '\n \ttest_must_fail git rm -f baz\n '\n \n-test_expect_success \\\n- 'When the rm in \"git rm -f\" fails, it should not remove the file from the index' \\\n- 'git ls-files --error-unmatch baz'\n+test_expect_success 'When the rm in \"git rm -f\" fails, it should not remove the file from the index' '\n+\tgit ls-files --error-unmatch baz\n+'\n \n test_expect_success 'Remove nonexistent file with --ignore-unmatch' '\n \tgit rm --ignore-unmatch nonexistent\n@@ -218,22 +222,22 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n test_expect_success 'Call \"rm\" from outside the work tree' '\n \tmkdir repo &&\n \t(cd repo &&\n-\t git init &&\n-\t echo something >somefile &&\n-\t git add somefile &&\n-\t git commit -m \"add a file\" &&\n-\t (cd .. &&\n-\t git --git-dir=repo/.git --work-tree=repo rm somefile) &&\n-\ttest_must_fail git ls-files --error-unmatch somefile)\n+\t\tgit init &&\n+\t\techo something >somefile &&\n+\t\tgit add somefile &&\n+\t\tgit commit -m \"add a file\" &&\n+\t\t(cd .. &&\n+\t\t\tgit --git-dir=repo/.git --work-tree=repo rm somefile\n+\t\t) &&\n+\t\ttest_must_fail git ls-files --error-unmatch somefile\n+\t)\n '\n \n test_expect_success 'refresh index before checking if it is up-to-date' '\n-\n \tgit reset --hard &&\n \ttest-tool chmtime -86400 frotz/nitfol &&\n \tgit rm frotz/nitfol &&\n \ttest ! -f frotz/nitfol\n-\n '\n \n test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n@@ -242,8 +246,8 @@ test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n \ti=0 &&\n \twhile test $i -lt 12000\n \tdo\n-\t echo \"100644 1234567890123456789012345678901234567890 0\tsome-file-$i\"\n-\t i=$(( $i + 1 ))\n+\t\techo \"100644 1234567890123456789012345678901234567890 0\tsome-file-$i\"\n+\t\ti=$(( $i + 1 ))\n \tdone | git update-index --index-info &&\n \tgit rm -n \"some-file-*\" | : &&\n \ttest_path_is_missing .git/index.lock\n-- \n2.17.1\n\n"},{"id":"370565","messageId":"20190303233750.6500-4-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"20190303233750.6500-1-rohit.ashiwal265@gmail.com","subject":"[GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s ","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-03T23:37:50Z","receivedAt":"2019-03-03T23:38:37Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Replace `test -(d|f|e|s)` calls in `t3600-rm.sh`.\n\nPreviously we were using `test -(d|f|e|s)` to verify the presence of a\ndirectory/file, but we already have helper functions, viz, `test_path_is_dir`,\n`test_path_is_file`, `test_path_is_missing` and `test_file_not_empty`\nwith better functionality.\n\nThese helper functions make code more readable and informative to someone\nnew, also these functions have better error messages.\n\nSigned-off-by: Rohit Ashiwal \n---\n t/t3600-rm.sh | 142 +++++++++++++++++++++++++-------------------------\n 1 file changed, 71 insertions(+), 71 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex f1afda21e9..9e1ada463c 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -141,15 +141,15 @@ test_expect_success 'Re-add foo and baz' '\n test_expect_success 'Modify foo -- rm should refuse' '\n \techo >>foo &&\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n test_expect_success 'Modified foo -- rm -f should work' '\n \tgit rm -f foo baz &&\n-\ttest ! -f foo &&\n-\ttest ! -f baz &&\n+\ttest_path_is_missing foo &&\n+\ttest_path_is_missing baz &&\n \ttest_must_fail git ls-files --error-unmatch foo &&\n \ttest_must_fail git ls-files --error-unmatch bar\n '\n@@ -163,15 +163,15 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '\n \n test_expect_success 'foo is different in index from HEAD -- rm should refuse' '\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n test_expect_success 'but with -f it should work.' '\n \tgit rm -f foo baz &&\n-\ttest ! -f foo &&\n-\ttest ! -f baz &&\n+\ttest_path_is_missing foo &&\n+\ttest_path_is_missing baz &&\n \ttest_must_fail git ls-files --error-unmatch foo &&\n \ttest_must_fail git ls-files --error-unmatch baz\n '\n@@ -198,21 +198,21 @@ test_expect_success 'Recursive test setup' '\n \n test_expect_success 'Recursive without -r fails' '\n \ttest_must_fail git rm frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r but dirty' '\n \techo qfwfq >>frotz/nitfol &&\n \ttest_must_fail git rm -r frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r -f' '\n \tgit rm -f -r frotz &&\n-\t! test -f frotz/nitfol &&\n-\t! test -d frotz\n+\ttest_path_is_missing frotz/nitfol &&\n+\ttest_path_is_missing frotz\n '\n \n test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n@@ -237,7 +237,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '\n \tgit reset --hard &&\n \ttest-tool chmtime -86400 frotz/nitfol &&\n \tgit rm frotz/nitfol &&\n-\ttest ! -f frotz/nitfol\n+\ttest_path_is_missing frotz/nitfol\n '\n \n test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n@@ -258,7 +258,7 @@ test_expect_success 'rm removes subdirectories recursively' '\n \techo content >dir/subdir/subsubdir/file &&\n \tgit add dir/subdir/subsubdir/file &&\n \tgit rm -f dir/subdir/subsubdir/file &&\n-\t! test -d dir\n+\ttest_path_is_missing dir\n '\n \n cat >expect <actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -318,7 +318,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -329,7 +329,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm submod/ &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -347,12 +347,12 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles\n \tgit submodule update &&\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -363,8 +363,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm --cached submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.cached actual &&\n \tgit config -f .gitmodules submodule.sub.url &&\n@@ -375,7 +375,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm -n submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-index --exit-code HEAD\n '\n \n@@ -385,8 +385,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '\n \tgit rm .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.both_deleted actual\n '\n@@ -396,15 +396,15 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta\n \tgit submodule update &&\n \tgit config -f .gitmodules foo.bar true &&\n \ttest_must_fail git rm submod >actual 2>actual.err &&\n-\ttest -s actual.err &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_file_not_empty actual.err &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-files --quiet -- submod &&\n \tgit add .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -417,8 +417,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule\n \techo \"warning: Could not find section in .gitmodules where path=submod\" >expect.err &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_i18ncmp expect.err actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -428,12 +428,12 @@ test_expect_success 'rm of a populated submodule with modifications fails unless\n \tgit submodule update &&\n \techo X >submod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -443,12 +443,12 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \tgit submodule update &&\n \techo X >submod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -485,7 +485,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '\n \tgit submodule update &&\n \ttest_must_fail git merge conflict2 &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -497,12 +497,12 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -516,12 +516,12 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f\n \techo X >submod/empty &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -535,12 +535,12 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files\n \techo X >submod/untracked &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -556,13 +556,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director\n \t) &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \ttest_must_fail git rm -f submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit merge --abort &&\n@@ -574,7 +574,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '\n \tgit reset --hard &&\n \ttest_must_fail git merge conflict2 &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -590,10 +590,10 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates\n \t\trm -r ../.git/modules/sub\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest -s actual &&\n+\ttest_file_not_empty actual &&\n \ttest_i18ngrep Migrating output.err\n '\n \n@@ -618,7 +618,7 @@ test_expect_success 'setup subsubmodule' '\n \n test_expect_success 'rm recursively removes work tree of unmodified submodules' '\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -628,12 +628,12 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \tgit submodule update --recursive &&\n \tgit -C submod/subsubmod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -643,12 +643,12 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -658,12 +658,12 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -677,10 +677,10 @@ test_expect_success \"rm absorbs submodule's nested .git directory\" '\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/subsubmod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/subsubmod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest -s actual &&\n+\ttest_file_not_empty actual &&\n \ttest_i18ngrep Migrating output.err\n '\n \n-- \n2.17.1\n\n"},{"id":"370568","messageId":"xmqqo96rxpyi.fsf@gitster-ct.c.googlers.com","threadId":"50631","inReplyTo":"20190303233750.6500-2-rohit.ashiwal265@gmail.com","subject":"Re: [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-04T03:45:25Z","receivedAt":"2019-03-04T03:45:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rohit Ashiwal writes:\n\n> test-lib-functions: add a helper function that checks for a file and that\n> the file is not empty. The helper function will provide better error message\n> in case of failure and improve readability\n\nAvoid making the log message into an enumerated list, when there\naren't that many things to enumerate to begin with (specifically,\nthe \"test-lib-functions:\" label is unsightly here). Finish the\nsentence with a full stop.\n\n\tAdd a helper function to ensure that a given path is a\n\tnon-empty file, and give an error message when it is not.\n\n\tGive separate messages for the case when the path is missing\n\tor a non-file, and for the case when the path is a file but\n\tis empty.\n\nshould be sufficient.\n\nI still do not see why the posted code is better than this\n\n\tif ! test -s \"$1\"\n\tthen\n\t\techo \"'$1' is not a non-empty file.'\n\tfi\n \nwhich is more to the point. After all, if we are truly aiming for\nfiner-grained diagnosis, there is no good reason to accept a single\nerror message \"does not exist or not a file\" for these two cases,\nbut you'd be writing more like\n\n\tif ! test -e \"$1\"\n\tthen\n\t\techo \"'$1' does not exist\"\n\telif ! test -f \"$1\"\n\tthen\n\t\techo \"'$1' is not a file\"\n\telif ! test -s \"$1\"\n\tthen\n\t\techo \"'$1' is not empty\"\n\telse\n\t\t: happy\n\t\treturn\n\tfi\n\tfalse\n\nBut I do not see much point in doing so, and I do not see much point\nin the version that makes an extra check only for \"test -f\", either.\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 80402a428f..f9fcd2e013 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -593,6 +593,21 @@ test_dir_is_empty () {\n> \tfi\n> }\n> \n> +# Check if the file exists and has a size greater than zero\n> +test_file_not_empty () {\n> +\tif ! test -f \"$1\"\n> +\tthen\n> +\t\techo \"'$1' does not exist or not a file.\"\n> +\t\tfalse\n> +\telse\n> +\t\tif ! test -s \"$1\"\n> +\t\tthen\n> +\t\t\techo \"'$1' is an empty file.\"\n> +\t\t\tfalse\n> +\t\tfi\n> +\tfi\n> +}\n\n\nIf I were writing this, I'd dedent it by turning this into\n\n\tif ! test -f ...\n\tthen\n\t\techo ...\n\telif ! test -s ...\n\tthen\n\t\techo ...\n\telse\n\t\t: happy\n\t\treturn\n\tfi\n\tfalse\n\nBut as I said, I do not see much point in the extra \"test -f\", so\nmore likely this is what I would write, if I were doing this step\nmyself:\n\n\tif test -s \"$1\"\n\tthen\n\t\t: happy\n\telse\n\t\techo \"'$1' is not a non-empty file\"\n\t\tfalse\n\tfi\n\n> +\n> test_path_is_missing () {\n> \tif test -e \"$1\"\n> \tthen\n"},{"id":"370571","messageId":"xmqqsgw3w9wg.fsf@gitster-ct.c.googlers.com","threadId":"50631","inReplyTo":"20190303233750.6500-3-rohit.ashiwal265@gmail.com","subject":"Re: [GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-04T04:17:35Z","receivedAt":"2019-03-04T04:17:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rohit Ashiwal writes:\n\n> Replace leading spaces with tabs\n> Place title on the same line as function\n>\n> The previous code of `t3600-rm.sh` had a mixed use of tabs and spaces with\n> instance of `not-so-recommended` way of writing `if-then` statement, also\n> `titles` were not on the same line as the function `test_expect_success`,\n> replace them so that the current version agrees with the coding guidelines\n\nStyles and conventions are different from project to project, but\naround here, we do _not_ start the log message with an itemized list\nof what was done. I can sort of see why some project might find it\nuseful, but we do not do that here.\n\nInstead we talk about the status-quo in present tense, point out\nproblems (which can be omitted when they are obvious from the\ndescription of the status-quo) and describe the approach to addres\nthe problems (again, which can be omitted when it is obvious from\nwhat is written already). We then summarize the solution in\nimperative mood, as if we are giving an order to the codebase to \"be\nlike so\" (you can think of it as giving a command to a patch monkey\nto \"make the code like so\").\n\n\tSubject: t3600: modernize style\n\n\tThe tests in t3600 were written long time ago, and has a lot\n\tof style violations, including the mixed use of tabs and\n\tspaces, not having the title and the opening quote of the\n\tbody on the first line of the tests, and other shell script\n\tstyle violations. Update it to match the CodingGuidelines.\n\nis probably what I would summarize this change as..\n\n> Signed-off-by: Rohit Ashiwal \n> ---\n> t/t3600-rm.sh | 184 ++++++++++++++++++++++++++------------------------\n> 1 file changed, 94 insertions(+), 90 deletions(-)\n>\n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> index 04e5d42bd3..f1afda21e9 100755\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n> @@ -8,91 +8,95 @@ test_description='Test of the various options to git rm.'\n> . ./test-lib.sh\n> \n> # Setup some files to be removed, some with funny characters\n> -test_expect_success \\\n> - 'Initialize test directory' \\\n> - \"touch -- foo bar baz 'space embedded' -q &&\n> - git add -- foo bar baz 'space embedded' -q &&\n> - git commit -m 'add normal files'\"\n> +test_expect_success 'Initialize test directory' \"\n> +\ttouch -- foo bar baz 'space embedded' -q &&\n> +\tgit add -- foo bar baz 'space embedded' -q &&\n> +\tgit commit -m 'add normal files'\n> +\"\n\nSwap '' and \"\"; it is very rare that use of double-quotes around the\ntest body is justifiable (for one, any $variable reference would be\nexpanded _before_ the test runs, which is almost always not what you\nwant, if you used double-quote around the test body). \n\nThere are many other instances of this in the remainder of this\npatch, which I won't mention.\n\n> -if test_have_prereq !FUNNYNAMES; then\n> +if test_have_prereq !FUNNYNAMES\n> +then\n\nGood.\n\n> -test_expect_success FUNNYNAMES \\\n> - \"Test that \\\"git rm -f\\\" succeeds with embedded space, tab, or newline characters.\" \\\n> - \"git rm -f 'space embedded' 'tab\tembedded' 'newline\n> -embedded'\"\n> +test_expect_success 'Pre-check that foo exists and is in index before git rm foo' '\n> +\ttest_path_is_file foo &&\n\nThe point of having 2/3 and 3/3 as separate steps is because 3/3 is\nabout using the test-path-is... helpers, while 2/3 is about modernizing\nthe codebase before doing 3/3 so that the it can be reviewed more easily\nwithout distracting changes 2/3 needs to make.\n\nSo you would want to turn the \"[ -f foo ]\" into \"test -f foo\" in\nthis step, and then you will further turn it in the next step into\n\"test_path_is_file foo\".\n\nIt would not show in the end result, but paying attention to this\nkind of detail shows how careful the author was when future readers\nread the patch, so I try to be careful when I am structuring a\nseries like this myself.\n\n> +test_expect_success 'Post-check that foo exists but is not in index after git rm foo' '\n> +\ttest_path_is_file foo &&\n> +\ttest_must_fail git ls-files --error-unmatch foo\n> +'\n\nLikewise.\n\n> +test_expect_success 'Pre-check that bar exists and is in index before \"git rm bar\"' '\n> +\ttest_path_is_file bar &&\n> +\tgit ls-files --error-unmatch bar\n> +'\n\nLikewise (I'll stop pointing these out from here on).\n\n> +test_expect_success FUNNYNAMES \"Test that \\\"git rm -f\\\" succeeds with embedded space, tab, or newline characters.\" \"\n> +\tgit rm -f 'space embedded' 'tab\tembedded' 'newline\n> +embedded'\n> +\"\n\nAgain, swap \"\" and '' around; that way you can lose the backslash.\n\nConsider using $LF that is defined in t/test-lib.sh for exactly a\ncase like this one.\n\n\tgit rm -f \"space embedded\" \"tab\tembedded\" \"newline${LF}embedded\"\n\nThat may make the test body even easier to follow.\n\n> @@ -218,22 +222,22 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n> test_expect_success 'Call \"rm\" from outside the work tree' '\n> \tmkdir repo &&\n> \t(cd repo &&\n\nInspect the output from\n\n\tgit grep 'cd ' 't/t[0-9][0-9][0-9][0-9]-*.sh'\n\nand see which is prevalent; I think this line may want to become\n\n\t(\n\t\tcd repo &&\n\nbut I did not count.\n\n> -\t git init &&\n> -\t echo something >somefile &&\n> -\t git add somefile &&\n> -\t git commit -m \"add a file\" &&\n> -\t (cd .. &&\n> -\t git --git-dir=repo/.git --work-tree=repo rm somefile) &&\n> -\ttest_must_fail git ls-files --error-unmatch somefile)\n> +\t\tgit init &&\n> +\t\techo something >somefile &&\n> +\t\tgit add somefile &&\n> +\t\tgit commit -m \"add a file\" &&\n> +\t\t(cd .. &&\n> +\t\t\tgit --git-dir=repo/.git --work-tree=repo rm somefile\n> +\t\t) &&\n> +\t\ttest_must_fail git ls-files --error-unmatch somefile\n> +\t)\n> '\n\nLikewise.\n\n> test_expect_success 'refresh index before checking if it is up-to-date' '\n> -\n> \tgit reset --hard &&\n> \ttest-tool chmtime -86400 frotz/nitfol &&\n> \tgit rm frotz/nitfol &&\n> \ttest ! -f frotz/nitfol\n> -\n> '\n\nGood.\n\nThanks for working on this.\n"},{"id":"370592","messageId":"20190304120801.28763-1-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"20190303122842.30380-1-rohit.ashiwal265@gmail.com","subject":"[GSoC][PATCH v3 0/3] Use helper functions in test script","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-04T12:07:58Z","receivedAt":"2019-03-04T12:08:40Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"This patch ultimately aims to replace `test -(d|f|e|s)` calls in t3600-rm.sh\nPreviously we were using these to verify the presence of diretory/file, but\nwe already have helper functions, viz, `test_path_is_dir`, `test_path_is_file`,\n`test_path_is_missing` and `test_file_not_empty` with better functionality\n\nHelper functions are better as they provide better error messages and\nimprove readability. They are friendly to someone new to code.\n\nRohit Ashiwal (3):\n test functions: add function `test_file_not_empty`\n t3600: modernize style\n t3600: use helpers to replace test -d/f/e/s \n\n t/t3600-rm.sh | 349 ++++++++++++++++++++--------------------\n t/test-lib-functions.sh | 9 ++\n 2 files changed, 187 insertions(+), 171 deletions(-)\n\n-- \nRohit\n"},{"id":"370593","messageId":"20190304120801.28763-2-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"20190304120801.28763-1-rohit.ashiwal265@gmail.com","subject":"[GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty`","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-04T12:07:59Z","receivedAt":"2019-03-04T12:08:43Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Add a helper function to ensure that a given path is a non-empty file,\nand give an error message when it is not.\n\nSigned-off-by: Rohit Ashiwal \n---\n t/test-lib-functions.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 80402a428f..681c41ba32 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -593,6 +593,15 @@ test_dir_is_empty () {\n \tfi\n }\n \n+# Check if the file exists and has a size greater than zero\n+test_file_not_empty () {\n+\tif ! test -s \"$1\"\n+\tthen\n+\t\techo \"'$1' is not a non-empty file.\"\n+\t\tfalse\n+\tfi\n+}\n+\n test_path_is_missing () {\n \tif test -e \"$1\"\n \tthen\n-- \nRohit\n"},{"id":"370594","messageId":"20190304120801.28763-3-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"20190304120801.28763-1-rohit.ashiwal265@gmail.com","subject":"[GSoC][PATCH v3 2/3] t3600: modernize style","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-04T12:08:00Z","receivedAt":"2019-03-04T12:08:47Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"The tests in `t3600-rm.sh` were written long time ago, and has a lot of\nstyle violations, including the mixed use of tabs and spaces, not having\nthe title and the opening quote of the body on the first line of the\ntests, and other shell script style violations. Update it to match the\nCodingGuidelines.\n\nSigned-off-by: Rohit Ashiwal \n---\n t/t3600-rm.sh | 207 ++++++++++++++++++++++++++------------------------\n 1 file changed, 107 insertions(+), 100 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 04e5d42bd3..8b03897a65 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -8,91 +8,92 @@ test_description='Test of the various options to git rm.'\n . ./test-lib.sh\n \n # Setup some files to be removed, some with funny characters\n-test_expect_success \\\n- 'Initialize test directory' \\\n- \"touch -- foo bar baz 'space embedded' -q &&\n- git add -- foo bar baz 'space embedded' -q &&\n- git commit -m 'add normal files'\"\n+test_expect_success 'Initialize test directory' '\n+\ttouch -- foo bar baz \"space embedded\" -q &&\n+\tgit add -- foo bar baz \"space embedded\" -q &&\n+\tgit commit -m \"add normal files\"\n+'\n \n-if test_have_prereq !FUNNYNAMES; then\n+if test_have_prereq !FUNNYNAMES\n+then\n \tsay 'Your filesystem does not allow tabs in filenames.'\n fi\n \n-test_expect_success FUNNYNAMES 'add files with funny names' \"\n- touch -- 'tab\tembedded' 'newline\n-embedded' &&\n- git add -- 'tab\tembedded' 'newline\n-embedded' &&\n- git commit -m 'add files with tabs and newlines'\n-\"\n-\n-test_expect_success \\\n- 'Pre-check that foo exists and is in index before git rm foo' \\\n- '[ -f foo ] && git ls-files --error-unmatch foo'\n-\n-test_expect_success \\\n- 'Test that git rm foo succeeds' \\\n- 'git rm --cached foo'\n-\n-test_expect_success \\\n- 'Test that git rm --cached foo succeeds if the index matches the file' \\\n- 'echo content >foo &&\n- git add foo &&\n- git rm --cached foo'\n-\n-test_expect_success \\\n- 'Test that git rm --cached foo succeeds if the index matches the file' \\\n- 'echo content >foo &&\n- git add foo &&\n- git commit -m foo &&\n- echo \"other content\" >foo &&\n- git rm --cached foo'\n-\n-test_expect_success \\\n- 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '\n- echo content >foo &&\n- git add foo &&\n- git commit -m foo --allow-empty &&\n- echo \"other content\" >foo &&\n- git add foo &&\n- echo \"yet another content\" >foo &&\n- test_must_fail git rm --cached foo\n-'\n-\n-test_expect_success \\\n- 'Test that git rm --cached -f foo works in case where --cached only did not' \\\n- 'echo content >foo &&\n- git add foo &&\n- git commit -m foo --allow-empty &&\n- echo \"other content\" >foo &&\n- git add foo &&\n- echo \"yet another content\" >foo &&\n- git rm --cached -f foo'\n-\n-test_expect_success \\\n- 'Post-check that foo exists but is not in index after git rm foo' \\\n- '[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'\n-\n-test_expect_success \\\n- 'Pre-check that bar exists and is in index before \"git rm bar\"' \\\n- '[ -f bar ] && git ls-files --error-unmatch bar'\n-\n-test_expect_success \\\n- 'Test that \"git rm bar\" succeeds' \\\n- 'git rm bar'\n-\n-test_expect_success \\\n- 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' \\\n- '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'\n-\n-test_expect_success \\\n- 'Test that \"git rm -- -q\" succeeds (remove a file that looks like an option)' \\\n- 'git rm -- -q'\n-\n-test_expect_success FUNNYNAMES \\\n- \"Test that \\\"git rm -f\\\" succeeds with embedded space, tab, or newline characters.\" \\\n- \"git rm -f 'space embedded' 'tab\tembedded' 'newline\n-embedded'\"\n+test_expect_success FUNNYNAMES 'add files with funny names' '\n+\ttouch -- \"tab\tembedded\" \"newline${LF}embedded\" &&\n+\tgit add -- \"tab\tembedded\" \"newline${LF}embedded\" &&\n+\tgit commit -m \"add files with tabs and newlines\"\n+'\n+\n+test_expect_success 'Pre-check that foo exists and is in index before git rm foo' '\n+\ttest -f foo &&\n+\tgit ls-files --error-unmatch foo\n+'\n+\n+test_expect_success 'Test that git rm foo succeeds' '\n+\tgit rm --cached foo\n+'\n+\n+test_expect_success 'Test that git rm --cached foo succeeds if the index matches the file' '\n+\techo content >foo &&\n+\tgit add foo &&\n+\tgit rm --cached foo\n+'\n+\n+test_expect_success 'Test that git rm --cached foo succeeds if the index matches the file' '\n+\techo content >foo &&\n+\tgit add foo &&\n+\tgit commit -m foo &&\n+\techo \"other content\" >foo &&\n+\tgit rm --cached foo\n+'\n+\n+test_expect_success 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '\n+\techo content >foo &&\n+\tgit add foo &&\n+\tgit commit -m foo --allow-empty &&\n+\techo \"other content\" >foo &&\n+\tgit add foo &&\n+\techo \"yet another content\" >foo &&\n+\ttest_must_fail git rm --cached foo\n+'\n+\n+test_expect_success 'Test that git rm --cached -f foo works in case where --cached only did not' '\n+\techo content >foo &&\n+\tgit add foo &&\n+\tgit commit -m foo --allow-empty &&\n+\techo \"other content\" >foo &&\n+\tgit add foo &&\n+\techo \"yet another content\" >foo &&\n+\tgit rm --cached -f foo\n+'\n+\n+test_expect_success 'Post-check that foo exists but is not in index after git rm foo' '\n+\ttest -f foo &&\n+\ttest_must_fail git ls-files --error-unmatch foo\n+'\n+\n+test_expect_success 'Pre-check that bar exists and is in index before \"git rm bar\"' '\n+\ttest -f bar &&\n+\tgit ls-files --error-unmatch bar\n+'\n+\n+test_expect_success 'Test that \"git rm bar\" succeeds' '\n+\tgit rm bar\n+'\n+\n+test_expect_success 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' '\n+\t! test -f bar &&\n+\ttest_must_fail git ls-files --error-unmatch bar\n+'\n+\n+test_expect_success 'Test that \"git rm -- -q\" succeeds (remove a file that looks like an option)' '\n+\tgit rm -- -q\n+'\n+\n+test_expect_success FUNNYNAMES 'Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters.' '\n+\tgit rm -f \"space embedded\" \"tab\tembedded\" \"newline${LF}embedded\"\n+'\n \n test_expect_success SANITY 'Test that \"git rm -f\" fails if its rm fails' '\n \ttest_when_finished \"chmod 775 .\" &&\n@@ -100,9 +101,9 @@ test_expect_success SANITY 'Test that \"git rm -f\" fails if its rm fails' '\n \ttest_must_fail git rm -f baz\n '\n \n-test_expect_success \\\n- 'When the rm in \"git rm -f\" fails, it should not remove the file from the index' \\\n- 'git ls-files --error-unmatch baz'\n+test_expect_success 'When the rm in \"git rm -f\" fails, it should not remove the file from the index' '\n+\tgit ls-files --error-unmatch baz\n+'\n \n test_expect_success 'Remove nonexistent file with --ignore-unmatch' '\n \tgit rm --ignore-unmatch nonexistent\n@@ -217,23 +218,25 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n \n test_expect_success 'Call \"rm\" from outside the work tree' '\n \tmkdir repo &&\n-\t(cd repo &&\n-\t git init &&\n-\t echo something >somefile &&\n-\t git add somefile &&\n-\t git commit -m \"add a file\" &&\n-\t (cd .. &&\n-\t git --git-dir=repo/.git --work-tree=repo rm somefile) &&\n-\ttest_must_fail git ls-files --error-unmatch somefile)\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\t\techo something >somefile &&\n+\t\tgit add somefile &&\n+\t\tgit commit -m \"add a file\" &&\n+\t\t(\n+\t\t\tcd .. &&\n+\t\t\tgit --git-dir=repo/.git --work-tree=repo rm somefile\n+\t\t) &&\n+\t\ttest_must_fail git ls-files --error-unmatch somefile\n+\t)\n '\n \n test_expect_success 'refresh index before checking if it is up-to-date' '\n-\n \tgit reset --hard &&\n \ttest-tool chmtime -86400 frotz/nitfol &&\n \tgit rm frotz/nitfol &&\n \ttest ! -f frotz/nitfol\n-\n '\n \n test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n@@ -242,8 +245,8 @@ test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n \ti=0 &&\n \twhile test $i -lt 12000\n \tdo\n-\t echo \"100644 1234567890123456789012345678901234567890 0\tsome-file-$i\"\n-\t i=$(( $i + 1 ))\n+\t\techo \"100644 1234567890123456789012345678901234567890 0\tsome-file-$i\"\n+\t\ti=$(( $i + 1 ))\n \tdone | git update-index --index-info &&\n \tgit rm -n \"some-file-*\" | : &&\n \ttest_path_is_missing .git/index.lock\n@@ -545,7 +548,8 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director\n \tgit checkout conflict1 &&\n \tgit reset --hard &&\n \tgit submodule update &&\n-\t(cd submod &&\n+\t(\n+\t\tcd submod &&\n \t\trm .git &&\n \t\tcp -R ../.git/modules/sub .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n@@ -579,7 +583,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates\n \tgit checkout -f master &&\n \tgit reset --hard &&\n \tgit submodule update &&\n-\t(cd submod &&\n+\t(\n+\t\tcd submod &&\n \t\trm .git &&\n \t\tcp -R ../.git/modules/sub .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree &&\n@@ -600,7 +605,8 @@ EOF\n test_expect_success 'setup subsubmodule' '\n \tgit reset --hard &&\n \tgit submodule update &&\n-\t(cd submod &&\n+\t(\n+\t\tcd submod &&\n \t\tgit update-index --add --cacheinfo 160000 $(git rev-parse HEAD) subsubmod &&\n \t\tgit config -f .gitmodules submodule.sub.url ../. &&\n \t\tgit config -f .gitmodules submodule.sub.path subsubmod &&\n@@ -667,7 +673,8 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n test_expect_success \"rm absorbs submodule's nested .git directory\" '\n \tgit reset --hard &&\n \tgit submodule update --recursive &&\n-\t(cd submod/subsubmod &&\n+\t(\n+\t\tcd submod/subsubmod &&\n \t\trm .git &&\n \t\tmv ../../.git/modules/sub/modules/sub .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n-- \nRohit\n"},{"id":"370595","messageId":"20190304120801.28763-4-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"20190304120801.28763-1-rohit.ashiwal265@gmail.com","subject":"[GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s ","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-04T12:08:01Z","receivedAt":"2019-03-04T12:08:51Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Previously we were using `test -(d|f|e|s)` to verify the presence of a\ndirectory/file, but we already have helper functions, viz, `test_path_is_dir`,\n`test_path_is_file`, `test_path_is_missing` and `test_file_not_empty`\nwith better functionality.\n\nThese helper functions make code more readable and informative to someone new,\nalso these functions have better error messages.\n\nSigned-off-by: Rohit Ashiwal \n---\n t/t3600-rm.sh | 150 +++++++++++++++++++++++++-------------------------\n 1 file changed, 75 insertions(+), 75 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 8b03897a65..85ae7dc1e4 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -26,7 +26,7 @@ test_expect_success FUNNYNAMES 'add files with funny names' '\n '\n \n test_expect_success 'Pre-check that foo exists and is in index before git rm foo' '\n-\ttest -f foo &&\n+\ttest_path_is_file foo &&\n \tgit ls-files --error-unmatch foo\n '\n \n@@ -69,12 +69,12 @@ test_expect_success 'Test that git rm --cached -f foo works in case where --cach\n '\n \n test_expect_success 'Post-check that foo exists but is not in index after git rm foo' '\n-\ttest -f foo &&\n+\ttest_path_is_file foo &&\n \ttest_must_fail git ls-files --error-unmatch foo\n '\n \n test_expect_success 'Pre-check that bar exists and is in index before \"git rm bar\"' '\n-\ttest -f bar &&\n+\ttest_path_is_file bar &&\n \tgit ls-files --error-unmatch bar\n '\n \n@@ -83,7 +83,7 @@ test_expect_success 'Test that \"git rm bar\" succeeds' '\n '\n \n test_expect_success 'Post-check that bar does not exist and is not in index after \"git rm -f bar\"' '\n-\t! test -f bar &&\n+\ttest_path_is_missing bar &&\n \ttest_must_fail git ls-files --error-unmatch bar\n '\n \n@@ -138,15 +138,15 @@ test_expect_success 'Re-add foo and baz' '\n test_expect_success 'Modify foo -- rm should refuse' '\n \techo >>foo &&\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n test_expect_success 'Modified foo -- rm -f should work' '\n \tgit rm -f foo baz &&\n-\ttest ! -f foo &&\n-\ttest ! -f baz &&\n+\ttest_path_is_missing foo &&\n+\ttest_path_is_missing baz &&\n \ttest_must_fail git ls-files --error-unmatch foo &&\n \ttest_must_fail git ls-files --error-unmatch bar\n '\n@@ -160,15 +160,15 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '\n \n test_expect_success 'foo is different in index from HEAD -- rm should refuse' '\n \ttest_must_fail git rm foo baz &&\n-\ttest -f foo &&\n-\ttest -f baz &&\n+\ttest_path_is_file foo &&\n+\ttest_path_is_file baz &&\n \tgit ls-files --error-unmatch foo baz\n '\n \n test_expect_success 'but with -f it should work.' '\n \tgit rm -f foo baz &&\n-\ttest ! -f foo &&\n-\ttest ! -f baz &&\n+\ttest_path_is_missing foo &&\n+\ttest_path_is_missing baz &&\n \ttest_must_fail git ls-files --error-unmatch foo &&\n \ttest_must_fail git ls-files --error-unmatch baz\n '\n@@ -195,21 +195,21 @@ test_expect_success 'Recursive test setup' '\n \n test_expect_success 'Recursive without -r fails' '\n \ttest_must_fail git rm frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r but dirty' '\n \techo qfwfq >>frotz/nitfol &&\n \ttest_must_fail git rm -r frotz &&\n-\ttest -d frotz &&\n-\ttest -f frotz/nitfol\n+\ttest_path_is_dir frotz &&\n+\ttest_path_is_file frotz/nitfol\n '\n \n test_expect_success 'Recursive with -r -f' '\n \tgit rm -f -r frotz &&\n-\t! test -f frotz/nitfol &&\n-\t! test -d frotz\n+\ttest_path_is_missing frotz/nitfol &&\n+\ttest_path_is_missing frotz\n '\n \n test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n@@ -236,7 +236,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '\n \tgit reset --hard &&\n \ttest-tool chmtime -86400 frotz/nitfol &&\n \tgit rm frotz/nitfol &&\n-\ttest ! -f frotz/nitfol\n+\ttest_path_is_missing frotz/nitfol\n '\n \n test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n@@ -257,7 +257,7 @@ test_expect_success 'rm removes subdirectories recursively' '\n \techo content >dir/subdir/subsubdir/file &&\n \tgit add dir/subdir/subsubdir/file &&\n \tgit rm -f dir/subdir/subsubdir/file &&\n-\t! test -d dir\n+\ttest_path_is_missing dir\n '\n \n cat >expect <actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -317,7 +317,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -328,7 +328,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm submod/ &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -346,12 +346,12 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles\n \tgit submodule update &&\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -362,8 +362,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm --cached submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.cached actual &&\n \tgit config -f .gitmodules submodule.sub.url &&\n@@ -374,7 +374,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm -n submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-index --exit-code HEAD\n '\n \n@@ -384,8 +384,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '\n \tgit rm .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect.both_deleted actual\n '\n@@ -395,15 +395,15 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta\n \tgit submodule update &&\n \tgit config -f .gitmodules foo.bar true &&\n \ttest_must_fail git rm submod >actual 2>actual.err &&\n-\ttest -s actual.err &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_file_not_empty actual.err &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit diff-files --quiet -- submod &&\n \tgit add .gitmodules &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_must_be_empty actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -416,8 +416,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule\n \techo \"warning: Could not find section in .gitmodules where path=submod\" >expect.err &&\n \tgit rm submod >actual 2>actual.err &&\n \ttest_i18ncmp expect.err actual.err &&\n-\t! test -d submod &&\n-\t! test -f submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno >actual &&\n \ttest_cmp expect actual\n '\n@@ -427,12 +427,12 @@ test_expect_success 'rm of a populated submodule with modifications fails unless\n \tgit submodule update &&\n \techo X >submod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -442,12 +442,12 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \tgit submodule update &&\n \techo X >submod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -484,7 +484,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '\n \tgit submodule update &&\n \ttest_must_fail git merge conflict2 &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -496,12 +496,12 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD\n \tgit -C submod checkout HEAD^ &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -515,12 +515,12 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f\n \techo X >submod/empty &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n@@ -534,12 +534,12 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files\n \techo X >submod/untracked &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -556,13 +556,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director\n \t) &&\n \ttest_must_fail git merge conflict2 &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \ttest_must_fail git rm -f submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_dir submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.conflict actual &&\n \tgit merge --abort &&\n@@ -574,7 +574,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '\n \tgit reset --hard &&\n \ttest_must_fail git merge conflict2 &&\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -591,10 +591,10 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates\n \t\trm -r ../.git/modules/sub\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest -s actual &&\n+\ttest_file_not_empty actual &&\n \ttest_i18ngrep Migrating output.err\n '\n \n@@ -620,7 +620,7 @@ test_expect_success 'setup subsubmodule' '\n \n test_expect_success 'rm recursively removes work tree of unmodified submodules' '\n \tgit rm submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -630,12 +630,12 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \tgit submodule update --recursive &&\n \tgit -C submod/subsubmod checkout HEAD^ &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -645,12 +645,12 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/empty &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -660,12 +660,12 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \tgit submodule update --recursive &&\n \techo X >submod/subsubmod/untracked &&\n \ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -f submod/.git &&\n+\ttest_path_is_dir submod &&\n+\ttest_path_is_file submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n-\ttest ! -d submod &&\n+\ttest_path_is_missing submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n \ttest_cmp expect actual\n '\n@@ -680,10 +680,10 @@ test_expect_success \"rm absorbs submodule's nested .git directory\" '\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \tgit rm submod 2>output.err &&\n-\t! test -d submod &&\n-\t! test -d submod/subsubmod/.git &&\n+\ttest_path_is_missing submod &&\n+\ttest_path_is_missing submod/subsubmod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest -s actual &&\n+\ttest_file_not_empty actual &&\n \ttest_i18ngrep Migrating output.err\n '\n \n-- \nRohit\n"},{"id":"370649","messageId":"CAPig+cSsAufCnHPJfjQd8A778UNAsXEst1m+ekQ7T83=2mMUnw@mail.gmail.com","threadId":"50631","inReplyTo":"20190304120801.28763-1-rohit.ashiwal265@gmail.com","subject":"Re: [GSoC][PATCH v3 0/3] Use helper functions in test script","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-05T00:09:00Z","receivedAt":"2019-03-05T00:09:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal wrote:\n> This patch ultimately aims to replace `test -(d|f|e|s)` calls in t3600-rm.sh\n> Previously we were using these to verify the presence of diretory/file, but\n> we already have helper functions, viz, `test_path_is_dir`, `test_path_is_file`,\n> `test_path_is_missing` and `test_file_not_empty` with better functionality\n>\n> Helper functions are better as they provide better error messages and\n> improve readability. They are friendly to someone new to code.\n\nAs an aid to reviewers, please use the cover-letter to explain what\nchanged since the previous version of the patch series. Also, to\nfurther help reviewers, consider using the --range-diff or --interdiff\noptions with \"git format-patch\" to visually show the changes since the\nprevious attempt (in addition to your prose explanation).\n\nFinally, it is a good idea to provide a link, like this[1], to the\nprevious round in order to jog the memory of existing reviewers and to\nprovide context for people new to the review of the series.\n\n[1]: https://public-inbox.org/git/20190303233750.6500-1-rohit.ashiwal265@gmail.com/\n"},{"id":"370650","messageId":"CAPig+cTTJgXERud0=svc5b+ctwQxoQ6cmpiA7WHMa7TUZ37BgQ@mail.gmail.com","threadId":"50631","inReplyTo":"20190304120801.28763-2-rohit.ashiwal265@gmail.com","subject":"Re: [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-05T00:17:50Z","receivedAt":"2019-03-05T00:18:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal wrote:\n> Add a helper function to ensure that a given path is a non-empty file,\n> and give an error message when it is not.\n>\n> Signed-off-by: Rohit Ashiwal \n> ---\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> @@ -593,6 +593,15 @@ test_dir_is_empty () {\n> +# Check if the file exists and has a size greater than zero\n> +test_file_not_empty () {\n> + if ! test -s \"$1\"\n> + then\n> + echo \"'$1' is not a non-empty file.\"\n\nAlthough not incorrect, the double-negative is hard to digest. I had\nto read it a few times to convince myself that it matched the intent\nof the new function. I wonder if a message such as\n\n echo \"'$1' is unexpectedly empty\"\n\nwould be better. (Subjective, and not at all worth a re-roll.)\n\n> + false\n> + fi\n> +}\n> test_path_is_missing () {\n\nMuch later in this same file, a function named test_must_be_empty() is\ndefined, which is the complement of your new test_file_not_empty()\nfunction. The dissimilar names may cause confusion, so choosing a name\nmore like the existing function might be warranted.\n\nAlso, it might be a good idea to add this new function as a neighbor\nof test_must_be_empty() rather than defining it a couple hundred lines\nearlier in the file. Alternately, perhaps a preparatory patch could\nmove test_must_be_empty() closer to the other similar functions\n(test_path_is_missing() and cousins).\n"},{"id":"370651","messageId":"CAPig+cQ=Uoa3G0mvJ6MGfEM=W6bpghS-+Ub32UtmdoC0OAZD7w@mail.gmail.com","threadId":"50631","inReplyTo":"20190304120801.28763-3-rohit.ashiwal265@gmail.com","subject":"Re: [GSoC][PATCH v3 2/3] t3600: modernize style","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-05T00:36:48Z","receivedAt":"2019-03-05T00:37:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal wrote:\n> The tests in `t3600-rm.sh` were written long time ago, and has a lot of\n> style violations, including the mixed use of tabs and spaces, not having\n> the title and the opening quote of the body on the first line of the\n> tests, and other shell script style violations. Update it to match the\n> CodingGuidelines.\n\nMany of the words in this commit message are separated by multiple\nspaces. Please fold out the excess so there is only a single space\nbetween words.\n\n> Signed-off-by: Rohit Ashiwal \n> ---\n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> @@ -217,23 +218,25 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n> test_expect_success 'Call \"rm\" from outside the work tree' '\n> mkdir repo &&\n> + (\n> + cd repo &&\n> + git init &&\n> + echo something >somefile &&\n> + git add somefile &&\n> + git commit -m \"add a file\" &&\n> + (\n> + cd .. &&\n> + git --git-dir=repo/.git --work-tree=repo rm somefile\n> + ) &&\n> + test_must_fail git ls-files --error-unmatch somefile\n> + )\n> '\n\nThis test is unusual in that it first cd's into a subdirectory and\nthen cd's back out with \"cd ..\". And, while the use of subshells is\ncorrect to ensure that all 'cd' commands are undone at the end of the\ntest (whether successful or not), the entire construction is\nunnecessarily confusing. This is not the sort of issue which should be\nfixed in this style-fix patch, however, it is something which could be\ncleaned up with a follow-up patch. For instance, the test might be\nreworked like this:\n\n git init repo &&\n (\n cd repo &&\n echo something >somefile &&\n git add somefile &&\n git commit -m \"add a file\"\n ) &&\n git --git-dir=repo/.git --work-tree=repo rm somefile &&\n test_must_fail git -C repo ls-files --error-unmatch somefile\n\nIt's up to you whether you actually want to include such a follow-up\npatch in your series; it's certainly not a requirement.\n"},{"id":"370652","messageId":"CAPig+cSKOSC+CckNbjr7HahT5jXkp47WuOxbDov_KQi4XNnbQQ@mail.gmail.com","threadId":"50631","inReplyTo":"20190304120801.28763-4-rohit.ashiwal265@gmail.com","subject":"Re: [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s ","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-05T00:42:53Z","receivedAt":"2019-03-05T00:43:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 4, 2019 at 7:09 AM Rohit Ashiwal wrote:\n> Previously we were using `test -(d|f|e|s)` to verify the presence of a\n> directory/file, but we already have helper functions, viz, `test_path_is_dir`,\n> `test_path_is_file`, `test_path_is_missing` and `test_file_not_empty`\n> with better functionality.\n\nAs with the commit message of 2/3, many of the words in this message\nare separated by multiple spaced. Please fold out the excess so there\nis only a single space between words.\n\nAlso, no need to say \"previously\" since readers know that the patch is\nchanging something. Rewrite in imperative mood:\n\n Take advantage of helper functions test_path_is_dir(),\n test_path_is_missing(), etc. to replace `test -d|f|e|s` since the\n functions make the code more readable and have better error\n messages.\n\n> These helper functions make code more readable and informative to someone new,\n> also these functions have better error messages.\n>\n> Signed-off-by: Rohit Ashiwal \n"},{"id":"370704","messageId":"xmqqmum9v6dx.fsf@gitster-ct.c.googlers.com","threadId":"50631","inReplyTo":"CAPig+cTTJgXERud0=svc5b+ctwQxoQ6cmpiA7WHMa7TUZ37BgQ@mail.gmail.com","subject":"Re: [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-05T12:43:22Z","receivedAt":"2019-03-05T12:43:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> +test_file_not_empty () {\n>> + if ! test -s \"$1\"\n>> + then\n>> + echo \"'$1' is not a non-empty file.\"\n>\n> Although not incorrect, the double-negative is hard to digest. I had\n> to read it a few times to convince myself that it matched the intent\n> of the new function. I wonder if a message such as\n>\n> echo \"'$1' is unexpectedly empty\"\n>\n> would be better. (Subjective, and not at all worth a re-roll.)\n\nYeah, that is subjective. The expectation of the test is \"not-empty\",\nso I do not see this double-negation as being too bad, though.\n\n> Much later in this same file, a function named test_must_be_empty() is\n> defined, which is the complement of your new test_file_not_empty()\n> function. The dissimilar names may cause confusion, so choosing a name\n> more like the existing function might be warranted.\n>\n> Also, it might be a good idea to add this new function as a neighbor\n> of test_must_be_empty() rather than defining it a couple hundred lines\n> earlier in the file. Alternately, perhaps a preparatory patch could\n> move test_must_be_empty() closer to the other similar functions\n> (test_path_is_missing() and cousins).\n\nVery good suggestions. Looking at neighbouring helpers around\nmust-be-empty, it seems to me that the latter, i.e. moving it to sit\nnext to other \"path\" helpers, would make the most sense.\n\nThanks.\n"},{"id":"370705","messageId":"xmqqimwxv6bo.fsf@gitster-ct.c.googlers.com","threadId":"50631","inReplyTo":"CAPig+cQ=Uoa3G0mvJ6MGfEM=W6bpghS-+Ub32UtmdoC0OAZD7w@mail.gmail.com","subject":"Re: [GSoC][PATCH v3 2/3] t3600: modernize style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-05T12:44:43Z","receivedAt":"2019-03-05T12:44:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> This test is unusual in that it first cd's into a subdirectory and\n> then cd's back out with \"cd ..\". And, while the use of subshells is\n> correct to ensure that all 'cd' commands are undone at the end of the\n> test (whether successful or not), the entire construction is\n> unnecessarily confusing. This is not the sort of issue which should be\n> fixed in this style-fix patch, however, it is something which could be\n> cleaned up with a follow-up patch. For instance, the test might be\n> reworked like this:\n>\n> git init repo &&\n> (\n> cd repo &&\n> echo something >somefile &&\n> git add somefile &&\n> git commit -m \"add a file\"\n> ) &&\n> git --git-dir=repo/.git --work-tree=repo rm somefile &&\n> test_must_fail git -C repo ls-files --error-unmatch somefile\n>\n> It's up to you whether you actually want to include such a follow-up\n> patch in your series; it's certainly not a requirement.\n\nI missed that. As you said, it can be left for further clean-up.\n"},{"id":"370711","messageId":"20190305132701.9657-1-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"CAPig+cTTJgXERud0=svc5b+ctwQxoQ6cmpiA7WHMa7TUZ37BgQ@mail.gmail.com","subject":"Re: [GSoc][PATCH v3 1/3] test functions: add function `test_file_not_empty`","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-05T13:27:01Z","receivedAt":"2019-03-05T13:27:42Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Hello Eric\n\nOn 2019-03-04 19:17:50 -0500 Eric Sunshine wrote:\n> On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal wrote:\n> > if ! test -s \"$1\"\n> > then\n> > \techo \"'$1' is not a non-empty file.\"\n>\n> Although not incorrect, the double-negative is hard to digest. I had\n> to read it a few times to convince myself that it matched the intent\n> of the new function. I wonder if a message such as\n>\n> echo \"'$1' is unexpectedly empty\"\n>\n> would be better. (Subjective, and not at all worth a re-roll.)\n\nI think the current message is more accurate as it implies both:\n\t1. There is no file, and\n\t2. If there is, it is not empty\n\n\"unexpectedly empty\" may imply that there is a directory which is not empty\nand that is not the intention of the function.\n\n> Also, it might be a good idea to add this new function as a neighbor\n> of test_must_be_empty() rather than defining it a couple hundred lines\n> earlier in the file. Alternately, perhaps a preparatory patch could\n> move test_must_be_empty() closer to the other similar functions\n> (test_path_is_missing() and cousins).\n\nI think we should relocate the function `test_must_be_empty` in a separate\npatch as this patch deals with a different issue. \n\nThanks\nRohit\n\n"},{"id":"370714","messageId":"20190305134259.10962-1-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"CAPig+cSKOSC+CckNbjr7HahT5jXkp47WuOxbDov_KQi4XNnbQQ@mail.gmail.com","subject":"Re: [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s ","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-05T13:42:59Z","receivedAt":"2019-03-05T13:43:35Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Hey Eric\n\nOn 2019-03-05 0:42 Eric Sunshine wrote:\n> As with the commit message of 2/3, many of the words in this message\n> are separated by multiple spaced. Please fold out the excess so there\n> is only a single space between words.\n>\n> Also, no need to say \"previously\" since readers know that the patch is\n> changing something. Rewrite in imperative mood:\n\nOkay, I'll keep that in mind from next time onwards. The spaces were\nprovided to make the commit message look aesthetically pleasing.\n\nThese changes aside, is there anything you would like to add to the review?\nor is it good to go for a merge?\n\nThanks for advice\nRohit\n\n"},{"id":"370717","messageId":"CAPig+cR3b=jk4W=9SF4XJQyqAfFHiG8MduypD75RL1=T_qY0Hg@mail.gmail.com","threadId":"50631","inReplyTo":"20190305134259.10962-1-rohit.ashiwal265@gmail.com","subject":"Re: [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s ","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-05T14:03:45Z","receivedAt":"2019-03-05T14:03:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 5, 2019 at 8:43 AM Rohit Ashiwal wrote:\n> On 2019-03-05 0:42 Eric Sunshine wrote:\n> > As with the commit message of 2/3, many of the words in this message\n> > are separated by multiple spaced. Please fold out the excess so there\n> > is only a single space between words.\n> >\n> > Also, no need to say \"previously\" since readers know that the patch is\n> > changing something. Rewrite in imperative mood:\n>\n> Okay, I'll keep that in mind from next time onwards. The spaces were\n> provided to make the commit message look aesthetically pleasing.\n>\n> These changes aside, is there anything you would like to add to the review?\n> or is it good to go for a merge?\n\nI don't understand your question.\n"},{"id":"370719","messageId":"20190305142149.13671-1-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"CAPig+cR3b=jk4W=9SF4XJQyqAfFHiG8MduypD75RL1=T_qY0Hg@mail.gmail.com","subject":"Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s ","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-05T14:21:49Z","receivedAt":"2019-03-05T14:22:25Z","isPatch":true,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Eric\n\nI was asking if this patch is good enough to be added to the existing code? Does this patch look good?\n\n\nRegards\nRohit\n\n"},{"id":"370720","messageId":"CAPig+cT_YT-1=ymAYiTpjgRQEe8906Y6yyBU=XuP_wbw+ixxiQ@mail.gmail.com","threadId":"50631","inReplyTo":"20190305142149.13671-1-rohit.ashiwal265@gmail.com","subject":"Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s ","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-05T14:57:40Z","receivedAt":"2019-03-05T14:57:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 5, 2019 at 9:22 AM Rohit Ashiwal wrote:\n> I was asking if this patch is good enough to be added to the\n> existing code? Does this patch look good?\n\nI didn't review the patch with a critical-enough eye to be able to say\nthat every change maintains fidelity with the original code. As\nmentioned in [1]:\n\n [...] an important reason for limiting the scope of this change\n [...] is to ease the burden on people who review your submission.\n Large patch series tend to tax reviewers heavily, even (and often)\n when repetitive and simple, like replacing `test -d` with\n `test_path_is_dir()`. The shorter and more concise a patch series\n is, the more likely that it will receive quality reviews.\n\nThis patch, due to its length and repetitive nature, falls under the\ncategory of being tedious to review, which makes it all the more\nlikely that a reviewer will overlook a problem.\n\nAnd, it's not always obvious at a glance that a change is correct. For\ninstance, taking a look at the final patch band:\n\n - ! test -d submod &&\n - ! test -d submod/subsubmod/.git &&\n + test_path_is_missing submod &&\n + test_path_is_missing submod/subsubmod/.git &&\n\nSuperficially, the transformation seems straightforward. However, that\ndoesn't mean it maintains fidelity with the original or even means the\nsame thing. To review this change properly requires understanding the\noriginal intent of \"! test -d\".\n\nThe meaning of that expression can vary depending upon the context. Is\nit checking that that path is not a directory (but it is okay if a\nplain file exists there)? Or does it merely care about existence\n(neither directory nor any other type of entry)? If the latter, then\nthe transformation is probably correct, however, if the former, then\nit likely isn't correct. So, understanding the overall context of the\ntest is important for judging if a particular change is correct, and\nmany (volunteer) reviewers simply don't have the time to delve that\ndeeply to make a proper judgment.\n\n[1]: https://public-inbox.org/git/CAPig+cSZZaCT0G3hysmjn_tNvZmYGp=5cXpZHkdphbWXnONSVQ@mail.gmail.com/\n"},{"id":"370766","messageId":"20190305233825.5327-1-rohit.ashiwal265@gmail.com","threadId":"50631","inReplyTo":"CAPig+cT_YT-1=ymAYiTpjgRQEe8906Y6yyBU=XuP_wbw+ixxiQ@mail.gmail.com","subject":"Re:","fromName":"Rohit Ashiwal","fromEmail":"rohit.ashiwal265@gmail.com","sentAt":"2019-03-05T23:38:25Z","receivedAt":"2019-03-05T23:39:02Z","isPatch":false,"sender":{"key":"rohit.ashiwal265@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31043830?v=4"},"body":"Hey Eric\n\nOn Tue, 5 Mar 2019 09:57:40 -0500 Eric Sunshine wrote:\n> This patch, due to its length and repetitive nature, falls under the\n> category of being tedious to review, which makes it all the more\n> likely that a reviewer will overlook a problem.\n\nYes, I clearly understand that this patch has become too big to review.\nIt will require time to carefully review and reviewers are doing their\nbest to maintain the utmost quality of code.\n\n> And, it's not always obvious at a glance that a change is correct. For\n> instance, taking a look at the final patch band:\n>\n> - ! test -d submod &&\n> - ! test -d submod/subsubmod/.git &&\n> + test_path_is_missing submod &&\n> + test_path_is_missing submod/subsubmod/.git &&\n\nDuy actually confirms that this transformation is correct in this[1] email.\n(I know that, it was given as an example, but I'll leave the link anyway).\n\nThanks\nRohit\n\n[1]: https://public-inbox.org/git/CACsJy8BYeLvB7BSM_Jt4vwfGsEBuhaCZfzGPOHe=B=7cvnRwrg@mail.gmail.com/\n\n"},{"id":"370941","messageId":"xmqq7edancws.fsf@gitster-ct.c.googlers.com","threadId":"50631","inReplyTo":"CAPig+cT_YT-1=ymAYiTpjgRQEe8906Y6yyBU=XuP_wbw+ixxiQ@mail.gmail.com","subject":"Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-08T05:38:43Z","receivedAt":"2019-03-08T05:38:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> This patch, due to its length and repetitive nature, falls under the\n> category of being tedious to review, which makes it all the more\n> likely that a reviewer will overlook a problem.\n>\n> And, it's not always obvious at a glance that a change is correct. For\n> instance, taking a look at the final patch band:\n>\n> - ! test -d submod &&\n> - ! test -d submod/subsubmod/.git &&\n> + test_path_is_missing submod &&\n> + test_path_is_missing submod/subsubmod/.git &&\n>\n> Superficially, the transformation seems straightforward. However, that\n> doesn't mean it maintains fidelity with the original or even means the\n> same thing. To review this change properly requires understanding the\n> original intent of \"! test -d\".\n>\n> ... , and\n> many (volunteer) reviewers simply don't have the time to delve that\n> deeply to make a proper judgment.\n\nTrue. The microproject was supposed to be a gentle introduction to\nand a practice session of the process of modifying, committing,\nsubmitting, and responding to reviews. Learning the usual Git\ncontributor workflow, without spending too much community resources\nlike reviewers' time (as opposed to the real \"here is my itch; let's\nimprove the system, and please help me doing so\").\n\nIn any case, I have spent some time with the patch and I think the\nchanges are generaly OK; some (like using \"test_path_is_missing foo\"\ninstead of \"test ! -f foo\" after \"git rm foo\" to ensure the path no\nlonger exists) are improvements.\n\nAn unrelated tangent, but what do you think of this patch? In the\ncontext of testing \"git rm\", if foo is a dangling symbolic link,\n\"git rm foo && test_path_is_missing foo\" would need something like\nthis to work correctly, I would think.\n\n t/test-lib-functions.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 681c41ba32..dfe0d4aff4 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -603,7 +603,7 @@ test_file_not_empty () {\n }\n \n test_path_is_missing () {\n-\tif test -e \"$1\"\n+\tif test -e \"$1\" || test -L \"$1\"\n \tthen\n \t\techo \"Path exists:\"\n \t\tls -ld \"$1\"\n\n\n\n\n\n\n"},{"id":"370949","messageId":"CAPig+cQFMNFTMfMz5EnMZxnXGLWnKYZ5_=D3eiNsX24hdZSPRw@mail.gmail.com","threadId":"50631","inReplyTo":"xmqq7edancws.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s ","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-08T09:51:59Z","receivedAt":"2019-03-08T09:52:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 8, 2019 at 12:38 AM Junio C Hamano wrote:\n> An unrelated tangent, but what do you think of this patch? In the\n> context of testing \"git rm\", if foo is a dangling symbolic link,\n> \"git rm foo && test_path_is_missing foo\" would need something like\n> this to work correctly, I would think.\n>\n> test_path_is_missing () {\n> - if test -e \"$1\"\n> + if test -e \"$1\" || test -L \"$1\"\n> then\n> echo \"Path exists:\"\n> ls -ld \"$1\"\n\nMakes sense. Won't we also want:\n\n test_path_exists () {\n - if ! test -e \"$1\"\n + if ! test -e \"$1\" && ! test -L \"$1\"\n then\n echo \"Path $1 doesn't exist. $2\"\n\nor something like that?\n"},{"id":"371095","messageId":"xmqqa7i2mazb.fsf@gitster-ct.c.googlers.com","threadId":"50631","inReplyTo":"CAPig+cQFMNFTMfMz5EnMZxnXGLWnKYZ5_=D3eiNsX24hdZSPRw@mail.gmail.com","subject":"Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-11T01:54:48Z","receivedAt":"2019-03-11T01:54:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Fri, Mar 8, 2019 at 12:38 AM Junio C Hamano wrote:\n>> An unrelated tangent, but what do you think of this patch? In the\n>> context of testing \"git rm\", if foo is a dangling symbolic link,\n>> \"git rm foo && test_path_is_missing foo\" would need something like\n>> this to work correctly, I would think.\n>>\n>> test_path_is_missing () {\n>> - if test -e \"$1\"\n>> + if test -e \"$1\" || test -L \"$1\"\n>> then\n>> echo \"Path exists:\"\n>> ls -ld \"$1\"\n>\n> Makes sense. Won't we also want:\n>\n> test_path_exists () {\n> - if ! test -e \"$1\"\n> + if ! test -e \"$1\" && ! test -L \"$1\"\n> then\n> echo \"Path $1 doesn't exist. $2\"\n>\n> or something like that?\n\nThat would make them symmetric, but what I was driving at with \"In\nthe context of testing git rm\" was that I highly suspect that among\nother existing users of test_path_is_missing there are some that\nwant to consider a dangling symbolic link as if it is not there (and\nvice versa for test_path_exists), and it may not be a good idea to\nunconditionally declare that, unlike the underlying \"test\" command\nthat dereferences symlinks for most operations, our wrapper does not\ndereference symbolic links, which is what the \"what do you think?\"\npatch and your addtion do.\n\n\n"}]}