[GSoC][PATCH 0/3] Use helper functions in test script

[GSoC][PATCH 0/3] Use helper functions in test script

[Rohit Ashiwal]

This patch ultimately aims to replace `test -(d|f|e|s)` calls in t3600-rm.sh
Previously we were using these to verify the presence of diretory/file, but
we already have helper functions, viz, `test_path_is_dir`, `test_path_is_file`,
`test_path_is_missing` and `test_file_not_empty` with better functionality

Helper functions are better as they provide better error messages and
improve readability. They are friendly to someone new to code.

Note: `test_file_not_empty` is implemented in [PATCH 1/3] of this mail

Rohit Ashiwal (3):
  test functions: Add new function `test_file_not_empty`
  t3600: refactor code according to contemporary guidelines
  t3600: use helper functions from test-lib-functions

 t/t3600-rm.sh           | 281 +++++++++++++++++++++-------------------
 t/test-lib-functions.sh |  10 ++
 2 files changed, 157 insertions(+), 134 deletions(-)

-- 
Thanks
Rohit

[PATCH 1/3] test functions: Add new function `test_file_not_empty`

[Rohit Ashiwal]

test-lib-functions: add a helper function that checks for a file and that
the file is not empty. The helper function will provide better error message
in case of failure and improve readability

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
---
 t/test-lib-functions.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 80402a428f..1302df63b6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -593,6 +593,16 @@ test_dir_is_empty () {
 	fi
 }
 
+# Check if the file exists and has a size greater than zero
+test_file_not_empty () {
+	test_path_is_file "$1" &&
+	if ! test -s "$1"
+	then
+		echo "'$1' is an empty file."
+		false
+	fi
+}
+
 test_path_is_missing () {
 	if test -e "$1"
 	then
-- 

Re: [PATCH 1/3] test functions: Add new function `test_file_not_empty`

[Junio C Hamano]

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

> Subject: Re: [PATCH 1/3] test functions: Add new function `test_file_not_empty`

s/Add/add/.  Strictly speaking, you do not need to say "new", if you
are already saying "add", then that's redundant.

> test-lib-functions: add a helper function that checks for a file and that
> the file is not empty. The helper function will provide better error message
> in case of failure and improve readability
>
> Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> ---
>  t/test-lib-functions.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 80402a428f..1302df63b6 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -593,6 +593,16 @@ test_dir_is_empty () {
>  	fi
>  }
>  
> +# Check if the file exists and has a size greater than zero
> +test_file_not_empty () {
> +	test_path_is_file "$1" &&
> +	if ! test -s "$1"

"test -s <path>" is true if <path> resolves to an existing directory
entry for a file that has a size greater than zero.  Isn't it
redundant and wasteful to have test_path_is_file before it, or is
there a situation where "test -s" alone won't give us what we want
to check?

> +	then
> +		echo "'$1' is an empty file."
> +		false
> +	fi
> +}
> +
>  test_path_is_missing () {
>  	if test -e "$1"
>  	then

(no subject)

[Rohit Ashiwal]

Hey Junio

On 2019-03-03 13:20 UTC Junio C Hamano <gitster@pobox.com> wrote:

> s/Add/add/.  Strictly speaking, you do not need to say "new", if you
> are already saying "add", then that's redundant.

Oh, my mistake, I will change in coming revisions.

> "test -s <path>" is true if <path> resolves to an existing directory
> entry for a file that has a size greater than zero.  Isn't it
> redundant and wasteful to have test_path_is_file before it, or is
> there a situation where "test -s" alone won't give us what we want
> to check?

Just to be clear of what caused the error:
	1. Path not being file, or
	2. File not being empty
I am checking for both.

Regards
Rohit

Re: none

[Junio C Hamano]

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

> Just to be clear of what caused the error:
> 	1. Path not being file, or
> 	2. File not being empty
> I am checking for both.

test -s <path> makes sure <path> is file; if it is not a file, then
it won't yield true.

So why do you need to say test_path_is_file yourself, if you are
asking "test -s"?

Clearing logic

[Rohit Ashiwal]

On 2019-03-03 13:33 UTC Junio C Hamano <gitster@pobox.com> wrote:

> test -s <path> makes sure <path> is file; if it is not a file, then
> it won't yield true.

> On 2019-03-03 13:20 UTC Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> > test_path_is_file "$1" &&
> > 	if ! test -s "$1"

According to the conditional if the path is not a file then we will get
the error "file does not exist" and then we will shortcircuit without checking
the second conditional, on the other hand, if path is a file then we will
again check if it has a size greater than zero, then error will be different
(if any).

Regards
Rohit

Re: Clearing logic

[Thomas Gummerer]

On 03/03, Rohit Ashiwal wrote:
> On 2019-03-03 13:33 UTC Junio C Hamano <gitster@pobox.com> wrote:
> 
> > test -s <path> makes sure <path> is file; if it is not a file, then
> > it won't yield true.
> 
> > On 2019-03-03 13:20 UTC Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> > > test_path_is_file "$1" &&
> > > 	if ! test -s "$1"
> 
> According to the conditional if the path is not a file then we will get
> the error "file does not exist" and then we will shortcircuit without checking
> the second conditional, on the other hand, if path is a file then we will
> again check if it has a size greater than zero, then error will be different
> (if any).

I do agree that the better error message is probably worth the
additional 'test_path_is_file' before the 'test -s'.  Although it may
be better to only make that distinction in the 'if' (and then maybe
just using 'test -f', which would explain better why we have an
additional call.

Either way it would be nice to describe that reasoning in the commit
message, as it's not 100% clear from the code what is going on here,
which also lead to Junio's question.

> Regards
> Rohit
> 

[PATCH 2/3] t3600: refactor code according to contemporary guidelines

[Rohit Ashiwal]

Replace leading spaces with tabs

The previous code of `t3600-rm.sh` had a mix use of tabs and spaces with
instance of `not-so-recommended` way of writing `if-then` statement,
replace them so that the current version agrees with the coding guidelines

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
---
 t/t3600-rm.sh | 131 ++++++++++++++++++++++++++------------------------
 1 file changed, 68 insertions(+), 63 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 04e5d42bd3..ec4877bfec 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -9,89 +9,94 @@ test_description='Test of the various options to git rm.'
 
 # Setup some files to be removed, some with funny characters
 test_expect_success \
-    'Initialize test directory' \
-    "touch -- foo bar baz 'space embedded' -q &&
-     git add -- foo bar baz 'space embedded' -q &&
-     git commit -m 'add normal files'"
+	'Initialize test directory' "
+	touch -- foo bar baz 'space embedded' -q &&
+	git add -- foo bar baz 'space embedded' -q &&
+	git commit -m 'add normal files'
+"
 
-if test_have_prereq !FUNNYNAMES; then
+if test_have_prereq !FUNNYNAMES
+then
 	say 'Your filesystem does not allow tabs in filenames.'
 fi
 
 test_expect_success FUNNYNAMES 'add files with funny names' "
-     touch -- 'tab	embedded' 'newline
+	touch -- 'tab	embedded' 'newline
 embedded' &&
-     git add -- 'tab	embedded' 'newline
+	git add -- 'tab	embedded' 'newline
 embedded' &&
-     git commit -m 'add files with tabs and newlines'
+	git commit -m 'add files with tabs and newlines'
 "
 
 test_expect_success \
-    'Pre-check that foo exists and is in index before git rm foo' \
-    '[ -f foo ] && git ls-files --error-unmatch foo'
+	'Pre-check that foo exists and is in index before git rm foo' \
+	'[ -f foo ] && git ls-files --error-unmatch foo'
 
 test_expect_success \
-    'Test that git rm foo succeeds' \
-    'git rm --cached foo'
+	'Test that git rm foo succeeds' \
+	'git rm --cached foo'
 
 test_expect_success \
-    'Test that git rm --cached foo succeeds if the index matches the file' \
-    'echo content >foo &&
-     git add foo &&
-     git rm --cached foo'
+	'Test that git rm --cached foo succeeds if the index matches the file' '
+	echo content >foo &&
+	git add foo &&
+	git rm --cached foo
+'
 
 test_expect_success \
-    'Test that git rm --cached foo succeeds if the index matches the file' \
-    'echo content >foo &&
-     git add foo &&
-     git commit -m foo &&
-     echo "other content" >foo &&
-     git rm --cached foo'
+	'Test that git rm --cached foo succeeds if the index matches the file' '
+	echo content >foo &&
+	git add foo &&
+	git commit -m foo &&
+	echo "other content" >foo &&
+	git rm --cached foo
+'
 
 test_expect_success \
-    'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '
-     echo content >foo &&
-     git add foo &&
-     git commit -m foo --allow-empty &&
-     echo "other content" >foo &&
-     git add foo &&
-     echo "yet another content" >foo &&
-     test_must_fail git rm --cached foo
+	'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '
+	echo content >foo &&
+	git add foo &&
+	git commit -m foo --allow-empty &&
+	echo "other content" >foo &&
+	git add foo &&
+	echo "yet another content" >foo &&
+	test_must_fail git rm --cached foo
 '
 
 test_expect_success \
-    'Test that git rm --cached -f foo works in case where --cached only did not' \
-    'echo content >foo &&
-     git add foo &&
-     git commit -m foo --allow-empty &&
-     echo "other content" >foo &&
-     git add foo &&
-     echo "yet another content" >foo &&
-     git rm --cached -f foo'
+	'Test that git rm --cached -f foo works in case where --cached only did not' '
+	echo content >foo &&
+	git add foo &&
+	git commit -m foo --allow-empty &&
+	echo "other content" >foo &&
+	git add foo &&
+	echo "yet another content" >foo &&
+	git rm --cached -f foo
+'
 
 test_expect_success \
-    'Post-check that foo exists but is not in index after git rm foo' \
-    '[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'
+	'Post-check that foo exists but is not in index after git rm foo' \
+	'[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'
 
 test_expect_success \
-    'Pre-check that bar exists and is in index before "git rm bar"' \
-    '[ -f bar ] && git ls-files --error-unmatch bar'
+	'Pre-check that bar exists and is in index before "git rm bar"' \
+	'[ -f bar ] && git ls-files --error-unmatch bar'
 
 test_expect_success \
-    'Test that "git rm bar" succeeds' \
-    'git rm bar'
+	'Test that "git rm bar" succeeds' \
+	'git rm bar'
 
 test_expect_success \
-    'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
-    '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'
+	'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
+	'! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'
 
 test_expect_success \
-    'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \
-    'git rm -- -q'
+	'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \
+	'git rm -- -q'
 
 test_expect_success FUNNYNAMES \
-    "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." \
-    "git rm -f 'space embedded' 'tab	embedded' 'newline
+	"Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." \
+	"git rm -f 'space embedded' 'tab	embedded' 'newline
 embedded'"
 
 test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' '
@@ -101,8 +106,8 @@ test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' '
 '
 
 test_expect_success \
-    'When the rm in "git rm -f" fails, it should not remove the file from the index' \
-    'git ls-files --error-unmatch baz'
+	'When the rm in "git rm -f" fails, it should not remove the file from the index' \
+	'git ls-files --error-unmatch baz'
 
 test_expect_success 'Remove nonexistent file with --ignore-unmatch' '
 	git rm --ignore-unmatch nonexistent
@@ -218,22 +223,22 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' '
 test_expect_success 'Call "rm" from outside the work tree' '
 	mkdir repo &&
 	(cd repo &&
-	 git init &&
-	 echo something >somefile &&
-	 git add somefile &&
-	 git commit -m "add a file" &&
-	 (cd .. &&
-	  git --git-dir=repo/.git --work-tree=repo rm somefile) &&
-	test_must_fail git ls-files --error-unmatch somefile)
+		git init &&
+		echo something >somefile &&
+		git add somefile &&
+		git commit -m "add a file" &&
+		(cd .. &&
+			git --git-dir=repo/.git --work-tree=repo rm somefile
+		) &&
+		test_must_fail git ls-files --error-unmatch somefile
+	)
 '
 
 test_expect_success 'refresh index before checking if it is up-to-date' '
-
 	git reset --hard &&
 	test-tool chmtime -86400 frotz/nitfol &&
 	git rm frotz/nitfol &&
 	test ! -f frotz/nitfol
-
 '
 
 test_expect_success 'choking "git rm" should not let it die with cruft' '
@@ -242,8 +247,8 @@ test_expect_success 'choking "git rm" should not let it die with cruft' '
 	i=0 &&
 	while test $i -lt 12000
 	do
-	    echo "100644 1234567890123456789012345678901234567890 0	some-file-$i"
-	    i=$(( $i + 1 ))
+		echo "100644 1234567890123456789012345678901234567890 0	some-file-$i"
+		i=$(( $i + 1 ))
 	done | git update-index --index-info &&
 	git rm -n "some-file-*" | : &&
 	test_path_is_missing .git/index.lock
-- 

Re: [PATCH 2/3] t3600: refactor code according to contemporary guidelines

[Junio C Hamano]

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

> Subject: Re: [PATCH 2/3] t3600: refactor code according to contemporary guidelines

Please do not overuse/abuse the verb "refactor" like this.  What the
patch does is only reformat---it does not do common "refactoring"
transformations like factoring out common/duplicated code into
helper functions, etc.

If we are doing this step, let's make sure all tests use the modern
style correctly.

>  # Setup some files to be removed, some with funny characters
>  test_expect_success \
> -    'Initialize test directory' \
> -    "touch -- foo bar baz 'space embedded' -q &&
> -     git add -- foo bar baz 'space embedded' -q &&
> -     git commit -m 'add normal files'"
> +	'Initialize test directory' "
> +	touch -- foo bar baz 'space embedded' -q &&
> +	git add -- foo bar baz 'space embedded' -q &&
> +	git commit -m 'add normal files'
> +"

In the modern style, we'd write this like so:

	test_expect_success 'initialize test directory' '
		touch -- foo bar baz "space embedded" -q &&
		git add -- foo bar baz "space embedded" -q &&
		git commit -m "add normal files"
	'

In addition to indenting with HT (not SP), two more points are

 - test title comes on the first line;

 - test body is enclosed in a single quote pair, opened on the first
   line and closed on the last line.

>  
> -if test_have_prereq !FUNNYNAMES; then
> +if test_have_prereq !FUNNYNAMES
> +then

This is good.

>  	say 'Your filesystem does not allow tabs in filenames.'
>  fi
>  
>  test_expect_success FUNNYNAMES 'add files with funny names' "

This has title on the first line, and opening quote of the body as
well, which is the modern style.

>  test_expect_success \
> -    'Pre-check that foo exists and is in index before git rm foo' \
> -    '[ -f foo ] && git ls-files --error-unmatch foo'
> +	'Pre-check that foo exists and is in index before git rm foo' \
> +	'[ -f foo ] && git ls-files --error-unmatch foo'

We prefer "test ..." over "[ ... ]" (Documentation/CodingGuidelines).

Thanks.

Re: t3600: refactor code according to comtemporary guidelines

[Rohit Ashiwal]

I agree to all the points that you mentioned.

On 2019-03-03 13:30 UTC Junio C Hamano <gitster@pobox.com> wrote:

> We prefer "test ..." over "[ ... ]" (Documentation/CodingGuidelines).

At first I thought this should go in [PATCH 3/3] but now I think this is
its real place.

Thanks
Rohit

[PATCH 3/3] t3600: use helper functions from test-lib-functions

[Rohit Ashiwal]

Replace `test -(d|f|e|s)` calls in `t3600-rm.sh`.

Previously we were using `test -(d|f|e|s)` to verify the presence of a
directory/file, but we already have helper functions, viz,
`test_path_is_dir`, `test_path_is_file`, `test_path_is_missing` and
`test_file_not_empty` with better functionality.

These helper functions make code more readable and informative to someone
new, also these functions have better error messages.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
---
 t/t3600-rm.sh | 166 ++++++++++++++++++++++++++------------------------
 1 file changed, 87 insertions(+), 79 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index ec4877bfec..c2391b7d56 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -29,8 +29,10 @@ embedded' &&
 "
 
 test_expect_success \
-	'Pre-check that foo exists and is in index before git rm foo' \
-	'[ -f foo ] && git ls-files --error-unmatch foo'
+	'Pre-check that foo exists and is in index before git rm foo' '
+	test_path_is_file foo &&
+	git ls-files --error-unmatch foo
+'
 
 test_expect_success \
 	'Test that git rm foo succeeds' \
@@ -75,20 +77,26 @@ test_expect_success \
 '
 
 test_expect_success \
-	'Post-check that foo exists but is not in index after git rm foo' \
-	'[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'
+	'Post-check that foo exists but is not in index after git rm foo' '
+	test_path_is_file foo &&
+	test_must_fail git ls-files --error-unmatch foo
+'
 
 test_expect_success \
-	'Pre-check that bar exists and is in index before "git rm bar"' \
-	'[ -f bar ] && git ls-files --error-unmatch bar'
+	'Pre-check that bar exists and is in index before "git rm bar"' '
+	test_path_is_file bar &&
+	git ls-files --error-unmatch bar
+'
 
 test_expect_success \
 	'Test that "git rm bar" succeeds' \
 	'git rm bar'
 
 test_expect_success \
-	'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
-	'! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'
+	'Post-check that bar does not exist and is not in index after "git rm -f bar"' '
+	test_path_is_missing bar &&
+	test_must_fail git ls-files --error-unmatch bar
+'
 
 test_expect_success \
 	'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \
@@ -142,15 +150,15 @@ test_expect_success 'Re-add foo and baz' '
 test_expect_success 'Modify foo -- rm should refuse' '
 	echo >>foo &&
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '
 
 test_expect_success 'Modified foo -- rm -f should work' '
 	git rm -f foo baz &&
-	test ! -f foo &&
-	test ! -f baz &&
+	test_path_is_missing foo &&
+	test_path_is_missing baz &&
 	test_must_fail git ls-files --error-unmatch foo &&
 	test_must_fail git ls-files --error-unmatch bar
 '
@@ -164,15 +172,15 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '
 
 test_expect_success 'foo is different in index from HEAD -- rm should refuse' '
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '
 
 test_expect_success 'but with -f it should work.' '
 	git rm -f foo baz &&
-	test ! -f foo &&
-	test ! -f baz &&
+	test_path_is_missing foo &&
+	test_path_is_missing baz &&
 	test_must_fail git ls-files --error-unmatch foo &&
 	test_must_fail git ls-files --error-unmatch baz
 '
@@ -199,21 +207,21 @@ test_expect_success 'Recursive test setup' '
 
 test_expect_success 'Recursive without -r fails' '
 	test_must_fail git rm frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r but dirty' '
 	echo qfwfq >>frotz/nitfol &&
 	test_must_fail git rm -r frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r -f' '
 	git rm -f -r frotz &&
-	! test -f frotz/nitfol &&
-	! test -d frotz
+	test_path_is_missing frotz/nitfol &&
+	test_path_is_missing frotz
 '
 
 test_expect_success 'Remove nonexistent file returns nonzero exit status' '
@@ -238,7 +246,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
 	git reset --hard &&
 	test-tool chmtime -86400 frotz/nitfol &&
 	git rm frotz/nitfol &&
-	test ! -f frotz/nitfol
+	test_path_is_missing frotz/nitfol
 '
 
 test_expect_success 'choking "git rm" should not let it die with cruft' '
@@ -259,7 +267,7 @@ test_expect_success 'rm removes subdirectories recursively' '
 	echo content >dir/subdir/subsubdir/file &&
 	git add dir/subdir/subsubdir/file &&
 	git rm -f dir/subdir/subsubdir/file &&
-	! test -d dir
+	test_path_is_missing dir
 '
 
 cat >expect <<EOF
@@ -297,7 +305,7 @@ test_expect_success 'rm removes empty submodules from work tree' '
 	git add .gitmodules &&
 	git commit -m "add submodule" &&
 	git rm submod &&
-	test ! -e submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -319,7 +327,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -330,7 +338,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '
 	git reset --hard &&
 	git submodule update &&
 	git rm submod/ &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -348,12 +356,12 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git submodule update &&
 	git -C submod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -364,8 +372,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g
 	git reset --hard &&
 	git submodule update &&
 	git rm --cached submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.cached actual &&
 	git config -f .gitmodules submodule.sub.url &&
@@ -376,7 +384,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm -n submod &&
-	test -f submod/.git &&
+	test_path_is_file submod/.git &&
 	git diff-index --exit-code HEAD
 '
 
@@ -386,8 +394,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '
 	git rm .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.both_deleted actual
 '
@@ -397,15 +405,15 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta
 	git submodule update &&
 	git config -f .gitmodules foo.bar true &&
 	test_must_fail git rm submod >actual 2>actual.err &&
-	test -s actual.err &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_file_not_empty actual.err &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git diff-files --quiet -- submod &&
 	git add .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '
@@ -418,8 +426,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule
 	echo "warning: Could not find section in .gitmodules where path=submod" >expect.err &&
 	git rm submod >actual 2>actual.err &&
 	test_i18ncmp expect.err actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '
@@ -429,12 +437,12 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	git submodule update &&
 	echo X >submod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -444,12 +452,12 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	git submodule update &&
 	echo X >submod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -486,7 +494,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '
 	git submodule update &&
 	test_must_fail git merge conflict2 &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -498,12 +506,12 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git -C submod checkout HEAD^ &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -517,12 +525,12 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
 	echo X >submod/empty &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -536,12 +544,12 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files
 	echo X >submod/untracked &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -557,13 +565,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director
 	) &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git merge --abort &&
@@ -575,7 +583,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
 	git reset --hard &&
 	test_must_fail git merge conflict2 &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -591,10 +599,10 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates
 		rm -r ../.git/modules/sub
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test -s actual &&
+	test_file_not_empty actual &&
 	test_i18ngrep Migrating output.err
 '
 
@@ -619,7 +627,7 @@ test_expect_success 'setup subsubmodule' '
 
 test_expect_success 'rm recursively removes work tree of unmodified submodules' '
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -629,12 +637,12 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	git submodule update --recursive &&
 	git -C submod/subsubmod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -644,12 +652,12 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -659,12 +667,12 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -678,10 +686,10 @@ test_expect_success "rm absorbs submodule's nested .git directory" '
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/subsubmod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/subsubmod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test -s actual &&
+	test_file_not_empty actual &&
 	test_i18ngrep Migrating output.err
 '
 
-- 

Re: [PATCH 3/3] t3600: use helper functions from test-lib-functions

[Junio C Hamano]

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

> Subject: Re: [PATCH 3/3] t3600: use helper functions from test-lib-functions

There are tons of helpers in that lib.

> Replace `test -(d|f|e|s)` calls in `t3600-rm.sh`.

This one gives more useful information than the patch title.

Perhaps

Subject: [PATCH 3/3] t3600: use helpers to replace test -d/f/e/s <path>

[GSoC][PATCH v2 0/3] Use helper functions in test script

[Rohit Ashiwal]

This patch ultimately aims to replace `test -(d|f|e|s)` calls in t3600-rm.sh
Previously we were using these to verify the presence of diretory/file, but
we already have helper functions, viz, `test_path_is_dir`, `test_path_is_file`,
`test_path_is_missing` and `test_file_not_empty` with better functionality

Helper functions are better as they provide better error messages and
improve readability. They are friendly to someone new to code.

Thanks
Rohit

PS: `test_file_not_empty` is implemented in [PATCH v2 1/3] of this mail

Rohit Ashiwal (3):
  test functions: add function `test_file_not_empty`
  t3600: restructure code according to contemporary guidelines
  t3600: use helpers to replace test -d/f/e/s <path>

 t/t3600-rm.sh           | 326 ++++++++++++++++++++--------------------
 t/test-lib-functions.sh |  15 ++
 2 files changed, 180 insertions(+), 161 deletions(-)

-- 
2.17.1

[GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty`

[Rohit Ashiwal]

test-lib-functions: add a helper function that checks for a file and that
the file is not empty. The helper function will provide better error message
in case of failure and improve readability

The function `test_file_not_empty`, first checks if a file is provided,
if it is not then an error message is printed, skipping the remaining
code, if <path> is indeed a file then check `test -s` is applied to check
if size of file is greater than zero, failing which another error message
is printed.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
---
 t/test-lib-functions.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 80402a428f..f9fcd2e013 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -593,6 +593,21 @@ test_dir_is_empty () {
 	fi
 }
 
+# Check if the file exists and has a size greater than zero
+test_file_not_empty () {
+	if ! test -f "$1"
+	then
+		echo "'$1' does not exist or not a file."
+		false
+	else
+		if ! test -s "$1"
+		then
+			echo "'$1' is an empty file."
+			false
+		fi
+	fi
+}
+
 test_path_is_missing () {
 	if test -e "$1"
 	then
-- 
2.17.1

Re: [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty`

[Junio C Hamano]

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

> test-lib-functions: add a helper function that checks for a file and that
> the file is not empty. The helper function will provide better error message
> in case of failure and improve readability

Avoid making the log message into an enumerated list, when there
aren't that many things to enumerate to begin with (specifically,
the "test-lib-functions:" label is unsightly here).   Finish the
sentence with a full stop.

	Add a helper function to ensure that a given path is a
	non-empty file, and give an error message when it is not.

	Give separate messages for the case when the path is missing
	or a non-file, and for the case when the path is a file but
	is empty.

should be sufficient.

I still do not see why the posted code is better than this

	if ! test -s "$1"
	then
		echo "'$1' is not a non-empty file.'
	fi
 
which is more to the point.  After all, if we are truly aiming for
finer-grained diagnosis, there is no good reason to accept a single
error message "does not exist or not a file" for these two cases,
but you'd be writing more like

	if ! test -e "$1"
	then
		echo "'$1' does not exist"
	elif ! test -f "$1"
	then
		echo "'$1' is not a file"
	elif ! test -s "$1"
	then
		echo "'$1' is not empty"
	else
		: happy
		return
	fi
	false

But I do not see much point in doing so, and I do not see much point
in the version that makes an extra check only for "test -f", either.

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 80402a428f..f9fcd2e013 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -593,6 +593,21 @@ test_dir_is_empty () {
>  	fi
>  }
>  
> +# Check if the file exists and has a size greater than zero
> +test_file_not_empty () {
> +	if ! test -f "$1"
> +	then
> +		echo "'$1' does not exist or not a file."
> +		false
> +	else
> +		if ! test -s "$1"
> +		then
> +			echo "'$1' is an empty file."
> +			false
> +		fi
> +	fi
> +}


If I were writing this, I'd dedent it by turning this into

	if ! test -f ...
	then
		echo ...
	elif ! test -s ...
	then
		echo ...
	else
		: happy
		return
	fi
	false

But as I said, I do not see much point in the extra "test -f", so
more likely this is what I would write, if I were doing this step
myself:

	if test -s "$1"
	then
		: happy
	else
		echo "'$1' is not a non-empty file"
		false
	fi

> +
>  test_path_is_missing () {
>  	if test -e "$1"
>  	then

[GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines

[Rohit Ashiwal]

Replace leading spaces with tabs
Place title on the same line as function

The previous code of `t3600-rm.sh` had a mixed use of tabs and spaces with
instance of `not-so-recommended` way of writing `if-then` statement, also
`titles` were not on the same line as the function `test_expect_success`,
replace them so that the current version agrees with the coding guidelines

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
---
 t/t3600-rm.sh | 184 ++++++++++++++++++++++++++------------------------
 1 file changed, 94 insertions(+), 90 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 04e5d42bd3..f1afda21e9 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -8,91 +8,95 @@ test_description='Test of the various options to git rm.'
 . ./test-lib.sh
 
 # Setup some files to be removed, some with funny characters
-test_expect_success \
-    'Initialize test directory' \
-    "touch -- foo bar baz 'space embedded' -q &&
-     git add -- foo bar baz 'space embedded' -q &&
-     git commit -m 'add normal files'"
+test_expect_success 'Initialize test directory' "
+	touch -- foo bar baz 'space embedded' -q &&
+	git add -- foo bar baz 'space embedded' -q &&
+	git commit -m 'add normal files'
+"
 
-if test_have_prereq !FUNNYNAMES; then
+if test_have_prereq !FUNNYNAMES
+then
 	say 'Your filesystem does not allow tabs in filenames.'
 fi
 
 test_expect_success FUNNYNAMES 'add files with funny names' "
-     touch -- 'tab	embedded' 'newline
+	touch -- 'tab	embedded' 'newline
 embedded' &&
-     git add -- 'tab	embedded' 'newline
+	git add -- 'tab	embedded' 'newline
 embedded' &&
-     git commit -m 'add files with tabs and newlines'
+	git commit -m 'add files with tabs and newlines'
 "
 
-test_expect_success \
-    'Pre-check that foo exists and is in index before git rm foo' \
-    '[ -f foo ] && git ls-files --error-unmatch foo'
-
-test_expect_success \
-    'Test that git rm foo succeeds' \
-    'git rm --cached foo'
-
-test_expect_success \
-    'Test that git rm --cached foo succeeds if the index matches the file' \
-    'echo content >foo &&
-     git add foo &&
-     git rm --cached foo'
-
-test_expect_success \
-    'Test that git rm --cached foo succeeds if the index matches the file' \
-    'echo content >foo &&
-     git add foo &&
-     git commit -m foo &&
-     echo "other content" >foo &&
-     git rm --cached foo'
-
-test_expect_success \
-    'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '
-     echo content >foo &&
-     git add foo &&
-     git commit -m foo --allow-empty &&
-     echo "other content" >foo &&
-     git add foo &&
-     echo "yet another content" >foo &&
-     test_must_fail git rm --cached foo
-'
-
-test_expect_success \
-    'Test that git rm --cached -f foo works in case where --cached only did not' \
-    'echo content >foo &&
-     git add foo &&
-     git commit -m foo --allow-empty &&
-     echo "other content" >foo &&
-     git add foo &&
-     echo "yet another content" >foo &&
-     git rm --cached -f foo'
-
-test_expect_success \
-    'Post-check that foo exists but is not in index after git rm foo' \
-    '[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'
-
-test_expect_success \
-    'Pre-check that bar exists and is in index before "git rm bar"' \
-    '[ -f bar ] && git ls-files --error-unmatch bar'
-
-test_expect_success \
-    'Test that "git rm bar" succeeds' \
-    'git rm bar'
-
-test_expect_success \
-    'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
-    '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'
-
-test_expect_success \
-    'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \
-    'git rm -- -q'
-
-test_expect_success FUNNYNAMES \
-    "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." \
-    "git rm -f 'space embedded' 'tab	embedded' 'newline
-embedded'"
+test_expect_success 'Pre-check that foo exists and is in index before git rm foo' '
+	test_path_is_file foo &&
+	git ls-files --error-unmatch foo
+'
+
+test_expect_success 'Test that git rm foo succeeds' '
+	git rm --cached foo
+'
+
+test_expect_success 'Test that git rm --cached foo succeeds if the index matches the file' '
+	echo content >foo &&
+	git add foo &&
+	git rm --cached foo
+'
+
+test_expect_success 'Test that git rm --cached foo succeeds if the index matches the file' '
+	echo content >foo &&
+	git add foo &&
+	git commit -m foo &&
+	echo "other content" >foo &&
+	git rm --cached foo
+'
+
+test_expect_success 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '
+	echo content >foo &&
+	git add foo &&
+	git commit -m foo --allow-empty &&
+	echo "other content" >foo &&
+	git add foo &&
+	echo "yet another content" >foo &&
+	test_must_fail git rm --cached foo
+'
+
+test_expect_success 'Test that git rm --cached -f foo works in case where --cached only did not' '
+	echo content >foo &&
+	git add foo &&
+	git commit -m foo --allow-empty &&
+	echo "other content" >foo &&
+	git add foo &&
+	echo "yet another content" >foo &&
+	git rm --cached -f foo
+'
+
+test_expect_success 'Post-check that foo exists but is not in index after git rm foo' '
+	test_path_is_file foo &&
+	test_must_fail git ls-files --error-unmatch foo
+'
+
+test_expect_success 'Pre-check that bar exists and is in index before "git rm bar"' '
+	test_path_is_file bar &&
+	git ls-files --error-unmatch bar
+'
+
+test_expect_success 'Test that "git rm bar" succeeds' '
+	git rm bar
+'
+
+test_expect_success 'Post-check that bar does not exist and is not in index after "git rm -f bar"' '
+	test_path_is_missing bar &&
+	test_must_fail git ls-files --error-unmatch bar
+'
+
+test_expect_success 'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' '
+	git rm -- -q
+'
+
+test_expect_success FUNNYNAMES "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." "
+	git rm -f 'space embedded' 'tab	embedded' 'newline
+embedded'
+"
 
 test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' '
 	test_when_finished "chmod 775 ." &&
@@ -100,9 +104,9 @@ test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' '
 	test_must_fail git rm -f baz
 '
 
-test_expect_success \
-    'When the rm in "git rm -f" fails, it should not remove the file from the index' \
-    'git ls-files --error-unmatch baz'
+test_expect_success 'When the rm in "git rm -f" fails, it should not remove the file from the index' '
+	git ls-files --error-unmatch baz
+'
 
 test_expect_success 'Remove nonexistent file with --ignore-unmatch' '
 	git rm --ignore-unmatch nonexistent
@@ -218,22 +222,22 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' '
 test_expect_success 'Call "rm" from outside the work tree' '
 	mkdir repo &&
 	(cd repo &&
-	 git init &&
-	 echo something >somefile &&
-	 git add somefile &&
-	 git commit -m "add a file" &&
-	 (cd .. &&
-	  git --git-dir=repo/.git --work-tree=repo rm somefile) &&
-	test_must_fail git ls-files --error-unmatch somefile)
+		git init &&
+		echo something >somefile &&
+		git add somefile &&
+		git commit -m "add a file" &&
+		(cd .. &&
+			git --git-dir=repo/.git --work-tree=repo rm somefile
+		) &&
+		test_must_fail git ls-files --error-unmatch somefile
+	)
 '
 
 test_expect_success 'refresh index before checking if it is up-to-date' '
-
 	git reset --hard &&
 	test-tool chmtime -86400 frotz/nitfol &&
 	git rm frotz/nitfol &&
 	test ! -f frotz/nitfol
-
 '
 
 test_expect_success 'choking "git rm" should not let it die with cruft' '
@@ -242,8 +246,8 @@ test_expect_success 'choking "git rm" should not let it die with cruft' '
 	i=0 &&
 	while test $i -lt 12000
 	do
-	    echo "100644 1234567890123456789012345678901234567890 0	some-file-$i"
-	    i=$(( $i + 1 ))
+		echo "100644 1234567890123456789012345678901234567890 0	some-file-$i"
+		i=$(( $i + 1 ))
 	done | git update-index --index-info &&
 	git rm -n "some-file-*" | : &&
 	test_path_is_missing .git/index.lock
-- 
2.17.1

Re: [GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines

[Junio C Hamano]

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

> Replace leading spaces with tabs
> Place title on the same line as function
>
> The previous code of `t3600-rm.sh` had a mixed use of tabs and spaces with
> instance of `not-so-recommended` way of writing `if-then` statement, also
> `titles` were not on the same line as the function `test_expect_success`,
> replace them so that the current version agrees with the coding guidelines

Styles and conventions are different from project to project, but
around here, we do _not_ start the log message with an itemized list
of what was done.  I can sort of see why some project might find it
useful, but we do not do that here.

Instead we talk about the status-quo in present tense, point out
problems (which can be omitted when they are obvious from the
description of the status-quo) and describe the approach to addres
the problems (again, which can be omitted when it is obvious from
what is written already).  We then summarize the solution in
imperative mood, as if we are giving an order to the codebase to "be
like so" (you can think of it as giving a command to a patch monkey
to "make the code like so").

	Subject: t3600: modernize style

	The tests in t3600 were written long time ago, and has a lot
	of style violations, including the mixed use of tabs and
	spaces, not having the title and the opening quote of the
	body on the first line of the tests, and other shell script
	style violations.  Update it to match the CodingGuidelines.

is probably what I would summarize this change as..

> Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> ---
>  t/t3600-rm.sh | 184 ++++++++++++++++++++++++++------------------------
>  1 file changed, 94 insertions(+), 90 deletions(-)
>
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index 04e5d42bd3..f1afda21e9 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -8,91 +8,95 @@ test_description='Test of the various options to git rm.'
>  . ./test-lib.sh
>  
>  # Setup some files to be removed, some with funny characters
> -test_expect_success \
> -    'Initialize test directory' \
> -    "touch -- foo bar baz 'space embedded' -q &&
> -     git add -- foo bar baz 'space embedded' -q &&
> -     git commit -m 'add normal files'"
> +test_expect_success 'Initialize test directory' "
> +	touch -- foo bar baz 'space embedded' -q &&
> +	git add -- foo bar baz 'space embedded' -q &&
> +	git commit -m 'add normal files'
> +"

Swap '' and ""; it is very rare that use of double-quotes around the
test body is justifiable (for one, any $variable reference would be
expanded _before_ the test runs, which is almost always not what you
want, if you used double-quote around the test body).  

There are many other instances of this in the remainder of this
patch, which I won't mention.

> -if test_have_prereq !FUNNYNAMES; then
> +if test_have_prereq !FUNNYNAMES
> +then

Good.

> -test_expect_success FUNNYNAMES \
> -    "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." \
> -    "git rm -f 'space embedded' 'tab	embedded' 'newline
> -embedded'"
> +test_expect_success 'Pre-check that foo exists and is in index before git rm foo' '
> +	test_path_is_file foo &&

The point of having 2/3 and 3/3 as separate steps is because 3/3 is
about using the test-path-is... helpers, while 2/3 is about modernizing
the codebase before doing 3/3 so that the it can be reviewed more easily
without distracting changes 2/3 needs to make.

So you would want to turn the "[ -f foo ]" into "test -f foo" in
this step, and then you will further turn it in the next step into
"test_path_is_file foo".

It would not show in the end result, but paying attention to this
kind of detail shows how careful the author was when future readers
read the patch, so I try to be careful when I am structuring a
series like this myself.

> +test_expect_success 'Post-check that foo exists but is not in index after git rm foo' '
> +	test_path_is_file foo &&
> +	test_must_fail git ls-files --error-unmatch foo
> +'

Likewise.

> +test_expect_success 'Pre-check that bar exists and is in index before "git rm bar"' '
> +	test_path_is_file bar &&
> +	git ls-files --error-unmatch bar
> +'

Likewise (I'll stop pointing these out from here on).

> +test_expect_success FUNNYNAMES "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." "
> +	git rm -f 'space embedded' 'tab	embedded' 'newline
> +embedded'
> +"

Again, swap "" and '' around; that way you can lose the backslash.

Consider using $LF that is defined in t/test-lib.sh for exactly a
case like this one.

	git rm -f "space embedded" "tab	embedded" "newline${LF}embedded"

That may make the test body even easier to follow.

> @@ -218,22 +222,22 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' '
>  test_expect_success 'Call "rm" from outside the work tree' '
>  	mkdir repo &&
>  	(cd repo &&

Inspect the output from

	git grep 'cd ' 't/t[0-9][0-9][0-9][0-9]-*.sh'

and see which is prevalent; I think this line may want to become

	(
		cd repo &&

but I did not count.

> -	 git init &&
> -	 echo something >somefile &&
> -	 git add somefile &&
> -	 git commit -m "add a file" &&
> -	 (cd .. &&
> -	  git --git-dir=repo/.git --work-tree=repo rm somefile) &&
> -	test_must_fail git ls-files --error-unmatch somefile)
> +		git init &&
> +		echo something >somefile &&
> +		git add somefile &&
> +		git commit -m "add a file" &&
> +		(cd .. &&
> +			git --git-dir=repo/.git --work-tree=repo rm somefile
> +		) &&
> +		test_must_fail git ls-files --error-unmatch somefile
> +	)
>  '

Likewise.

>  test_expect_success 'refresh index before checking if it is up-to-date' '
> -
>  	git reset --hard &&
>  	test-tool chmtime -86400 frotz/nitfol &&
>  	git rm frotz/nitfol &&
>  	test ! -f frotz/nitfol
> -
>  '

Good.

Thanks for working on this.

[GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path>

[Rohit Ashiwal]

Replace `test -(d|f|e|s)` calls in `t3600-rm.sh`.

Previously we were using `test -(d|f|e|s)` to verify the presence of a
directory/file, but we already have helper functions, viz, `test_path_is_dir`,
`test_path_is_file`, `test_path_is_missing` and `test_file_not_empty`
with better functionality.

These helper functions make code more readable and informative to someone
new, also these functions have better error messages.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
---
 t/t3600-rm.sh | 142 +++++++++++++++++++++++++-------------------------
 1 file changed, 71 insertions(+), 71 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index f1afda21e9..9e1ada463c 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -141,15 +141,15 @@ test_expect_success 'Re-add foo and baz' '
 test_expect_success 'Modify foo -- rm should refuse' '
 	echo >>foo &&
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '
 
 test_expect_success 'Modified foo -- rm -f should work' '
 	git rm -f foo baz &&
-	test ! -f foo &&
-	test ! -f baz &&
+	test_path_is_missing foo &&
+	test_path_is_missing baz &&
 	test_must_fail git ls-files --error-unmatch foo &&
 	test_must_fail git ls-files --error-unmatch bar
 '
@@ -163,15 +163,15 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '
 
 test_expect_success 'foo is different in index from HEAD -- rm should refuse' '
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '
 
 test_expect_success 'but with -f it should work.' '
 	git rm -f foo baz &&
-	test ! -f foo &&
-	test ! -f baz &&
+	test_path_is_missing foo &&
+	test_path_is_missing baz &&
 	test_must_fail git ls-files --error-unmatch foo &&
 	test_must_fail git ls-files --error-unmatch baz
 '
@@ -198,21 +198,21 @@ test_expect_success 'Recursive test setup' '
 
 test_expect_success 'Recursive without -r fails' '
 	test_must_fail git rm frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r but dirty' '
 	echo qfwfq >>frotz/nitfol &&
 	test_must_fail git rm -r frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r -f' '
 	git rm -f -r frotz &&
-	! test -f frotz/nitfol &&
-	! test -d frotz
+	test_path_is_missing frotz/nitfol &&
+	test_path_is_missing frotz
 '
 
 test_expect_success 'Remove nonexistent file returns nonzero exit status' '
@@ -237,7 +237,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
 	git reset --hard &&
 	test-tool chmtime -86400 frotz/nitfol &&
 	git rm frotz/nitfol &&
-	test ! -f frotz/nitfol
+	test_path_is_missing frotz/nitfol
 '
 
 test_expect_success 'choking "git rm" should not let it die with cruft' '
@@ -258,7 +258,7 @@ test_expect_success 'rm removes subdirectories recursively' '
 	echo content >dir/subdir/subsubdir/file &&
 	git add dir/subdir/subsubdir/file &&
 	git rm -f dir/subdir/subsubdir/file &&
-	! test -d dir
+	test_path_is_missing dir
 '
 
 cat >expect <<EOF
@@ -296,7 +296,7 @@ test_expect_success 'rm removes empty submodules from work tree' '
 	git add .gitmodules &&
 	git commit -m "add submodule" &&
 	git rm submod &&
-	test ! -e submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -318,7 +318,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -329,7 +329,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '
 	git reset --hard &&
 	git submodule update &&
 	git rm submod/ &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -347,12 +347,12 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git submodule update &&
 	git -C submod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -363,8 +363,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g
 	git reset --hard &&
 	git submodule update &&
 	git rm --cached submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.cached actual &&
 	git config -f .gitmodules submodule.sub.url &&
@@ -375,7 +375,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm -n submod &&
-	test -f submod/.git &&
+	test_path_is_file submod/.git &&
 	git diff-index --exit-code HEAD
 '
 
@@ -385,8 +385,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '
 	git rm .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.both_deleted actual
 '
@@ -396,15 +396,15 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta
 	git submodule update &&
 	git config -f .gitmodules foo.bar true &&
 	test_must_fail git rm submod >actual 2>actual.err &&
-	test -s actual.err &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_file_not_empty actual.err &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git diff-files --quiet -- submod &&
 	git add .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '
@@ -417,8 +417,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule
 	echo "warning: Could not find section in .gitmodules where path=submod" >expect.err &&
 	git rm submod >actual 2>actual.err &&
 	test_i18ncmp expect.err actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '
@@ -428,12 +428,12 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	git submodule update &&
 	echo X >submod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -443,12 +443,12 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	git submodule update &&
 	echo X >submod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -485,7 +485,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '
 	git submodule update &&
 	test_must_fail git merge conflict2 &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -497,12 +497,12 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git -C submod checkout HEAD^ &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -516,12 +516,12 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
 	echo X >submod/empty &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -535,12 +535,12 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files
 	echo X >submod/untracked &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -556,13 +556,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director
 	) &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git merge --abort &&
@@ -574,7 +574,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
 	git reset --hard &&
 	test_must_fail git merge conflict2 &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -590,10 +590,10 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates
 		rm -r ../.git/modules/sub
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test -s actual &&
+	test_file_not_empty actual &&
 	test_i18ngrep Migrating output.err
 '
 
@@ -618,7 +618,7 @@ test_expect_success 'setup subsubmodule' '
 
 test_expect_success 'rm recursively removes work tree of unmodified submodules' '
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -628,12 +628,12 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	git submodule update --recursive &&
 	git -C submod/subsubmod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -643,12 +643,12 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -658,12 +658,12 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -677,10 +677,10 @@ test_expect_success "rm absorbs submodule's nested .git directory" '
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/subsubmod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/subsubmod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test -s actual &&
+	test_file_not_empty actual &&
 	test_i18ngrep Migrating output.err
 '
 
-- 
2.17.1

[GSoC][PATCH v3 0/3] Use helper functions in test script

[Rohit Ashiwal]

This patch ultimately aims to replace `test -(d|f|e|s)` calls in t3600-rm.sh
Previously we were using these to verify the presence of diretory/file, but
we already have helper functions, viz, `test_path_is_dir`, `test_path_is_file`,
`test_path_is_missing` and `test_file_not_empty` with better functionality

Helper functions are better as they provide better error messages and
improve readability. They are friendly to someone new to code.

Rohit Ashiwal (3):
  test functions: add function `test_file_not_empty`
  t3600: modernize style
  t3600: use helpers to replace test -d/f/e/s <path>

 t/t3600-rm.sh           | 349 ++++++++++++++++++++--------------------
 t/test-lib-functions.sh |   9 ++
 2 files changed, 187 insertions(+), 171 deletions(-)

-- 
Rohit

[GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty`

[Rohit Ashiwal]

Add a helper function to ensure that a given path is a non-empty file,
and give an error message when it is not.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
---
 t/test-lib-functions.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 80402a428f..681c41ba32 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -593,6 +593,15 @@ test_dir_is_empty () {
 	fi
 }
 
+# Check if the file exists and has a size greater than zero
+test_file_not_empty () {
+	if ! test -s "$1"
+	then
+		echo "'$1' is not a non-empty file."
+		false
+	fi
+}
+
 test_path_is_missing () {
 	if test -e "$1"
 	then
-- 
Rohit

Re: [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty`

[Eric Sunshine]

On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> Add a helper function to ensure that a given path is a non-empty file,
> and give an error message when it is not.
>
> Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -593,6 +593,15 @@ test_dir_is_empty () {
> +# Check if the file exists and has a size greater than zero
> +test_file_not_empty () {
> +       if ! test -s "$1"
> +       then
> +               echo "'$1' is not a non-empty file."

Although not incorrect, the double-negative is hard to digest. I had
to read it a few times to convince myself that it matched the intent
of the new function. I wonder if a message such as

    echo "'$1' is unexpectedly empty"

would be better. (Subjective, and not at all worth a re-roll.)

> +               false
> +       fi
> +}
>  test_path_is_missing () {

Much later in this same file, a function named test_must_be_empty() is
defined, which is the complement of your new test_file_not_empty()
function. The dissimilar names may cause confusion, so choosing a name
more like the existing function might be warranted.

Also, it might be a good idea to add this new function as a neighbor
of test_must_be_empty() rather than defining it a couple hundred lines
earlier in the file. Alternately, perhaps a preparatory patch could
move test_must_be_empty() closer to the other similar functions
(test_path_is_missing() and cousins).

Re: [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty`

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +test_file_not_empty () {
>> +       if ! test -s "$1"
>> +       then
>> +               echo "'$1' is not a non-empty file."
>
> Although not incorrect, the double-negative is hard to digest. I had
> to read it a few times to convince myself that it matched the intent
> of the new function. I wonder if a message such as
>
>     echo "'$1' is unexpectedly empty"
>
> would be better. (Subjective, and not at all worth a re-roll.)

Yeah, that is subjective.  The expectation of the test is "not-empty",
so I do not see this double-negation as being too bad, though.

> Much later in this same file, a function named test_must_be_empty() is
> defined, which is the complement of your new test_file_not_empty()
> function. The dissimilar names may cause confusion, so choosing a name
> more like the existing function might be warranted.
>
> Also, it might be a good idea to add this new function as a neighbor
> of test_must_be_empty() rather than defining it a couple hundred lines
> earlier in the file. Alternately, perhaps a preparatory patch could
> move test_must_be_empty() closer to the other similar functions
> (test_path_is_missing() and cousins).

Very good suggestions.  Looking at neighbouring helpers around
must-be-empty, it seems to me that the latter, i.e. moving it to sit
next to other "path" helpers, would make the most sense.

Thanks.

Re: [GSoc][PATCH v3 1/3] test functions: add function `test_file_not_empty`

[Rohit Ashiwal]

Hello Eric

On 2019-03-04 19:17:50 -0500 Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> > if ! test -s "$1"
> > then
> > 	echo "'$1' is not a non-empty file."
>
> Although not incorrect, the double-negative is hard to digest. I had
> to read it a few times to convince myself that it matched the intent
> of the new function. I wonder if a message such as
>
>    echo "'$1' is unexpectedly empty"
>
> would be better. (Subjective, and not at all worth a re-roll.)

I think the current message is more accurate as it implies both:
	1. There is no file, and
	2. If there is, it is not empty

"unexpectedly empty" may imply that there is a directory which is not empty
and that is not the intention of the function.

> Also, it might be a good idea to add this new function as a neighbor
> of test_must_be_empty() rather than defining it a couple hundred lines
> earlier in the file. Alternately, perhaps a preparatory patch could
> move test_must_be_empty() closer to the other similar functions
> (test_path_is_missing() and cousins).

I think we should relocate the function `test_must_be_empty` in a separate
patch as this patch deals with a different issue. 

Thanks
Rohit

[GSoC][PATCH v3 2/3] t3600: modernize style

[Rohit Ashiwal]

The tests in `t3600-rm.sh` were written  long time ago, and has a lot of
style violations, including the mixed use of tabs and spaces, not having
the title  and the  opening quote of the body on  the first line of  the
tests, and other  shell script  style violations. Update it to match the
CodingGuidelines.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
---
 t/t3600-rm.sh | 207 ++++++++++++++++++++++++++------------------------
 1 file changed, 107 insertions(+), 100 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 04e5d42bd3..8b03897a65 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -8,91 +8,92 @@ test_description='Test of the various options to git rm.'
 . ./test-lib.sh
 
 # Setup some files to be removed, some with funny characters
-test_expect_success \
-    'Initialize test directory' \
-    "touch -- foo bar baz 'space embedded' -q &&
-     git add -- foo bar baz 'space embedded' -q &&
-     git commit -m 'add normal files'"
+test_expect_success 'Initialize test directory' '
+	touch -- foo bar baz "space embedded" -q &&
+	git add -- foo bar baz "space embedded" -q &&
+	git commit -m "add normal files"
+'
 
-if test_have_prereq !FUNNYNAMES; then
+if test_have_prereq !FUNNYNAMES
+then
 	say 'Your filesystem does not allow tabs in filenames.'
 fi
 
-test_expect_success FUNNYNAMES 'add files with funny names' "
-     touch -- 'tab	embedded' 'newline
-embedded' &&
-     git add -- 'tab	embedded' 'newline
-embedded' &&
-     git commit -m 'add files with tabs and newlines'
-"
-
-test_expect_success \
-    'Pre-check that foo exists and is in index before git rm foo' \
-    '[ -f foo ] && git ls-files --error-unmatch foo'
-
-test_expect_success \
-    'Test that git rm foo succeeds' \
-    'git rm --cached foo'
-
-test_expect_success \
-    'Test that git rm --cached foo succeeds if the index matches the file' \
-    'echo content >foo &&
-     git add foo &&
-     git rm --cached foo'
-
-test_expect_success \
-    'Test that git rm --cached foo succeeds if the index matches the file' \
-    'echo content >foo &&
-     git add foo &&
-     git commit -m foo &&
-     echo "other content" >foo &&
-     git rm --cached foo'
-
-test_expect_success \
-    'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '
-     echo content >foo &&
-     git add foo &&
-     git commit -m foo --allow-empty &&
-     echo "other content" >foo &&
-     git add foo &&
-     echo "yet another content" >foo &&
-     test_must_fail git rm --cached foo
-'
-
-test_expect_success \
-    'Test that git rm --cached -f foo works in case where --cached only did not' \
-    'echo content >foo &&
-     git add foo &&
-     git commit -m foo --allow-empty &&
-     echo "other content" >foo &&
-     git add foo &&
-     echo "yet another content" >foo &&
-     git rm --cached -f foo'
-
-test_expect_success \
-    'Post-check that foo exists but is not in index after git rm foo' \
-    '[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'
-
-test_expect_success \
-    'Pre-check that bar exists and is in index before "git rm bar"' \
-    '[ -f bar ] && git ls-files --error-unmatch bar'
-
-test_expect_success \
-    'Test that "git rm bar" succeeds' \
-    'git rm bar'
-
-test_expect_success \
-    'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
-    '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'
-
-test_expect_success \
-    'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \
-    'git rm -- -q'
-
-test_expect_success FUNNYNAMES \
-    "Test that \"git rm -f\" succeeds with embedded space, tab, or newline characters." \
-    "git rm -f 'space embedded' 'tab	embedded' 'newline
-embedded'"
+test_expect_success FUNNYNAMES 'add files with funny names' '
+	touch -- "tab	embedded" "newline${LF}embedded" &&
+	git add -- "tab	embedded" "newline${LF}embedded" &&
+	git commit -m "add files with tabs and newlines"
+'
+
+test_expect_success 'Pre-check that foo exists and is in index before git rm foo' '
+	test -f foo &&
+	git ls-files --error-unmatch foo
+'
+
+test_expect_success 'Test that git rm foo succeeds' '
+	git rm --cached foo
+'
+
+test_expect_success 'Test that git rm --cached foo succeeds if the index matches the file' '
+	echo content >foo &&
+	git add foo &&
+	git rm --cached foo
+'
+
+test_expect_success 'Test that git rm --cached foo succeeds if the index matches the file' '
+	echo content >foo &&
+	git add foo &&
+	git commit -m foo &&
+	echo "other content" >foo &&
+	git rm --cached foo
+'
+
+test_expect_success 'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '
+	echo content >foo &&
+	git add foo &&
+	git commit -m foo --allow-empty &&
+	echo "other content" >foo &&
+	git add foo &&
+	echo "yet another content" >foo &&
+	test_must_fail git rm --cached foo
+'
+
+test_expect_success 'Test that git rm --cached -f foo works in case where --cached only did not' '
+	echo content >foo &&
+	git add foo &&
+	git commit -m foo --allow-empty &&
+	echo "other content" >foo &&
+	git add foo &&
+	echo "yet another content" >foo &&
+	git rm --cached -f foo
+'
+
+test_expect_success 'Post-check that foo exists but is not in index after git rm foo' '
+	test -f foo &&
+	test_must_fail git ls-files --error-unmatch foo
+'
+
+test_expect_success 'Pre-check that bar exists and is in index before "git rm bar"' '
+	test -f bar &&
+	git ls-files --error-unmatch bar
+'
+
+test_expect_success 'Test that "git rm bar" succeeds' '
+	git rm bar
+'
+
+test_expect_success 'Post-check that bar does not exist and is not in index after "git rm -f bar"' '
+	! test -f bar &&
+	test_must_fail git ls-files --error-unmatch bar
+'
+
+test_expect_success 'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' '
+	git rm -- -q
+'
+
+test_expect_success FUNNYNAMES 'Test that "git rm -f" succeeds with embedded space, tab, or newline characters.' '
+	git rm -f "space embedded" "tab	embedded" "newline${LF}embedded"
+'
 
 test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' '
 	test_when_finished "chmod 775 ." &&
@@ -100,9 +101,9 @@ test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' '
 	test_must_fail git rm -f baz
 '
 
-test_expect_success \
-    'When the rm in "git rm -f" fails, it should not remove the file from the index' \
-    'git ls-files --error-unmatch baz'
+test_expect_success 'When the rm in "git rm -f" fails, it should not remove the file from the index' '
+	git ls-files --error-unmatch baz
+'
 
 test_expect_success 'Remove nonexistent file with --ignore-unmatch' '
 	git rm --ignore-unmatch nonexistent
@@ -217,23 +218,25 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' '
 
 test_expect_success 'Call "rm" from outside the work tree' '
 	mkdir repo &&
-	(cd repo &&
-	 git init &&
-	 echo something >somefile &&
-	 git add somefile &&
-	 git commit -m "add a file" &&
-	 (cd .. &&
-	  git --git-dir=repo/.git --work-tree=repo rm somefile) &&
-	test_must_fail git ls-files --error-unmatch somefile)
+	(
+		cd repo &&
+		git init &&
+		echo something >somefile &&
+		git add somefile &&
+		git commit -m "add a file" &&
+		(
+			cd .. &&
+			git --git-dir=repo/.git --work-tree=repo rm somefile
+		) &&
+		test_must_fail git ls-files --error-unmatch somefile
+	)
 '
 
 test_expect_success 'refresh index before checking if it is up-to-date' '
-
 	git reset --hard &&
 	test-tool chmtime -86400 frotz/nitfol &&
 	git rm frotz/nitfol &&
 	test ! -f frotz/nitfol
-
 '
 
 test_expect_success 'choking "git rm" should not let it die with cruft' '
@@ -242,8 +245,8 @@ test_expect_success 'choking "git rm" should not let it die with cruft' '
 	i=0 &&
 	while test $i -lt 12000
 	do
-	    echo "100644 1234567890123456789012345678901234567890 0	some-file-$i"
-	    i=$(( $i + 1 ))
+		echo "100644 1234567890123456789012345678901234567890 0	some-file-$i"
+		i=$(( $i + 1 ))
 	done | git update-index --index-info &&
 	git rm -n "some-file-*" | : &&
 	test_path_is_missing .git/index.lock
@@ -545,7 +548,8 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director
 	git checkout conflict1 &&
 	git reset --hard &&
 	git submodule update &&
-	(cd submod &&
+	(
+		cd submod &&
 		rm .git &&
 		cp -R ../.git/modules/sub .git &&
 		GIT_WORK_TREE=. git config --unset core.worktree
@@ -579,7 +583,8 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates
 	git checkout -f master &&
 	git reset --hard &&
 	git submodule update &&
-	(cd submod &&
+	(
+		cd submod &&
 		rm .git &&
 		cp -R ../.git/modules/sub .git &&
 		GIT_WORK_TREE=. git config --unset core.worktree &&
@@ -600,7 +605,8 @@ EOF
 test_expect_success 'setup subsubmodule' '
 	git reset --hard &&
 	git submodule update &&
-	(cd submod &&
+	(
+		cd submod &&
 		git update-index --add --cacheinfo 160000 $(git rev-parse HEAD) subsubmod &&
 		git config -f .gitmodules submodule.sub.url ../. &&
 		git config -f .gitmodules submodule.sub.path subsubmod &&
@@ -667,7 +673,8 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 test_expect_success "rm absorbs submodule's nested .git directory" '
 	git reset --hard &&
 	git submodule update --recursive &&
-	(cd submod/subsubmod &&
+	(
+		cd submod/subsubmod &&
 		rm .git &&
 		mv ../../.git/modules/sub/modules/sub .git &&
 		GIT_WORK_TREE=. git config --unset core.worktree
-- 
Rohit

Re: [GSoC][PATCH v3 2/3] t3600: modernize style

[Eric Sunshine]

On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> The tests in `t3600-rm.sh` were written  long time ago, and has a lot of
> style violations, including the mixed use of tabs and spaces, not having
> the title  and the  opening quote of the body on  the first line of  the
> tests, and other  shell script  style violations. Update it to match the
> CodingGuidelines.

Many of the words in this commit message are separated by multiple
spaces. Please fold out the excess so there is only a single space
between words.

> Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> ---
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> @@ -217,23 +218,25 @@ test_expect_success 'Remove nonexistent file returns nonzero exit status' '
>  test_expect_success 'Call "rm" from outside the work tree' '
>         mkdir repo &&
> +       (
> +               cd repo &&
> +               git init &&
> +               echo something >somefile &&
> +               git add somefile &&
> +               git commit -m "add a file" &&
> +               (
> +                       cd .. &&
> +                       git --git-dir=repo/.git --work-tree=repo rm somefile
> +               ) &&
> +               test_must_fail git ls-files --error-unmatch somefile
> +       )
>  '

This test is unusual in that it first cd's into a subdirectory and
then cd's back out with "cd ..". And, while the use of subshells is
correct to ensure that all 'cd' commands are undone at the end of the
test (whether successful or not), the entire construction is
unnecessarily confusing. This is not the sort of issue which should be
fixed in this style-fix patch, however, it is something which could be
cleaned up with a follow-up patch. For instance, the test might be
reworked like this:

    git init repo &&
    (
        cd repo &&
        echo something >somefile &&
        git add somefile &&
        git commit -m "add a file"
    ) &&
    git --git-dir=repo/.git --work-tree=repo rm somefile &&
    test_must_fail git -C repo ls-files --error-unmatch somefile

It's up to you whether you actually want to include such a follow-up
patch in your series; it's certainly not a requirement.

Re: [GSoC][PATCH v3 2/3] t3600: modernize style

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> This test is unusual in that it first cd's into a subdirectory and
> then cd's back out with "cd ..". And, while the use of subshells is
> correct to ensure that all 'cd' commands are undone at the end of the
> test (whether successful or not), the entire construction is
> unnecessarily confusing. This is not the sort of issue which should be
> fixed in this style-fix patch, however, it is something which could be
> cleaned up with a follow-up patch. For instance, the test might be
> reworked like this:
>
>     git init repo &&
>     (
>         cd repo &&
>         echo something >somefile &&
>         git add somefile &&
>         git commit -m "add a file"
>     ) &&
>     git --git-dir=repo/.git --work-tree=repo rm somefile &&
>     test_must_fail git -C repo ls-files --error-unmatch somefile
>
> It's up to you whether you actually want to include such a follow-up
> patch in your series; it's certainly not a requirement.

I missed that.  As you said, it can be left for further clean-up.

[GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path>

[Rohit Ashiwal]

Previously  we  were  using  `test -(d|f|e|s)`  to  verify  the  presence of a
directory/file, but we already have helper functions, viz, `test_path_is_dir`,
`test_path_is_file`,    `test_path_is_missing`    and    `test_file_not_empty`
with better functionality.

These helper functions make code more readable and informative to someone new,
also these functions have better error messages.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
---
 t/t3600-rm.sh | 150 +++++++++++++++++++++++++-------------------------
 1 file changed, 75 insertions(+), 75 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 8b03897a65..85ae7dc1e4 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -26,7 +26,7 @@ test_expect_success FUNNYNAMES 'add files with funny names' '
 '
 
 test_expect_success 'Pre-check that foo exists and is in index before git rm foo' '
-	test -f foo &&
+	test_path_is_file foo &&
 	git ls-files --error-unmatch foo
 '
 
@@ -69,12 +69,12 @@ test_expect_success 'Test that git rm --cached -f foo works in case where --cach
 '
 
 test_expect_success 'Post-check that foo exists but is not in index after git rm foo' '
-	test -f foo &&
+	test_path_is_file foo &&
 	test_must_fail git ls-files --error-unmatch foo
 '
 
 test_expect_success 'Pre-check that bar exists and is in index before "git rm bar"' '
-	test -f bar &&
+	test_path_is_file bar &&
 	git ls-files --error-unmatch bar
 '
 
@@ -83,7 +83,7 @@ test_expect_success 'Test that "git rm bar" succeeds' '
 '
 
 test_expect_success 'Post-check that bar does not exist and is not in index after "git rm -f bar"' '
-	! test -f bar &&
+	test_path_is_missing bar &&
 	test_must_fail git ls-files --error-unmatch bar
 '
 
@@ -138,15 +138,15 @@ test_expect_success 'Re-add foo and baz' '
 test_expect_success 'Modify foo -- rm should refuse' '
 	echo >>foo &&
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '
 
 test_expect_success 'Modified foo -- rm -f should work' '
 	git rm -f foo baz &&
-	test ! -f foo &&
-	test ! -f baz &&
+	test_path_is_missing foo &&
+	test_path_is_missing baz &&
 	test_must_fail git ls-files --error-unmatch foo &&
 	test_must_fail git ls-files --error-unmatch bar
 '
@@ -160,15 +160,15 @@ test_expect_success 'Re-add foo and baz for HEAD tests' '
 
 test_expect_success 'foo is different in index from HEAD -- rm should refuse' '
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '
 
 test_expect_success 'but with -f it should work.' '
 	git rm -f foo baz &&
-	test ! -f foo &&
-	test ! -f baz &&
+	test_path_is_missing foo &&
+	test_path_is_missing baz &&
 	test_must_fail git ls-files --error-unmatch foo &&
 	test_must_fail git ls-files --error-unmatch baz
 '
@@ -195,21 +195,21 @@ test_expect_success 'Recursive test setup' '
 
 test_expect_success 'Recursive without -r fails' '
 	test_must_fail git rm frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r but dirty' '
 	echo qfwfq >>frotz/nitfol &&
 	test_must_fail git rm -r frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r -f' '
 	git rm -f -r frotz &&
-	! test -f frotz/nitfol &&
-	! test -d frotz
+	test_path_is_missing frotz/nitfol &&
+	test_path_is_missing frotz
 '
 
 test_expect_success 'Remove nonexistent file returns nonzero exit status' '
@@ -236,7 +236,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
 	git reset --hard &&
 	test-tool chmtime -86400 frotz/nitfol &&
 	git rm frotz/nitfol &&
-	test ! -f frotz/nitfol
+	test_path_is_missing frotz/nitfol
 '
 
 test_expect_success 'choking "git rm" should not let it die with cruft' '
@@ -257,7 +257,7 @@ test_expect_success 'rm removes subdirectories recursively' '
 	echo content >dir/subdir/subsubdir/file &&
 	git add dir/subdir/subsubdir/file &&
 	git rm -f dir/subdir/subsubdir/file &&
-	! test -d dir
+	test_path_is_missing dir
 '
 
 cat >expect <<EOF
@@ -295,7 +295,7 @@ test_expect_success 'rm removes empty submodules from work tree' '
 	git add .gitmodules &&
 	git commit -m "add submodule" &&
 	git rm submod &&
-	test ! -e submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -317,7 +317,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -328,7 +328,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '
 	git reset --hard &&
 	git submodule update &&
 	git rm submod/ &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -346,12 +346,12 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git submodule update &&
 	git -C submod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -362,8 +362,8 @@ test_expect_success 'rm --cached leaves work tree of populated submodules and .g
 	git reset --hard &&
 	git submodule update &&
 	git rm --cached submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.cached actual &&
 	git config -f .gitmodules submodule.sub.url &&
@@ -374,7 +374,7 @@ test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm -n submod &&
-	test -f submod/.git &&
+	test_path_is_file submod/.git &&
 	git diff-index --exit-code HEAD
 '
 
@@ -384,8 +384,8 @@ test_expect_success 'rm does not complain when no .gitmodules file is found' '
 	git rm .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.both_deleted actual
 '
@@ -395,15 +395,15 @@ test_expect_success 'rm will error out on a modified .gitmodules file unless sta
 	git submodule update &&
 	git config -f .gitmodules foo.bar true &&
 	test_must_fail git rm submod >actual 2>actual.err &&
-	test -s actual.err &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_file_not_empty actual.err &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git diff-files --quiet -- submod &&
 	git add .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '
@@ -416,8 +416,8 @@ test_expect_success 'rm issues a warning when section is not found in .gitmodule
 	echo "warning: Could not find section in .gitmodules where path=submod" >expect.err &&
 	git rm submod >actual 2>actual.err &&
 	test_i18ncmp expect.err actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '
@@ -427,12 +427,12 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	git submodule update &&
 	echo X >submod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -442,12 +442,12 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	git submodule update &&
 	echo X >submod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -484,7 +484,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '
 	git submodule update &&
 	test_must_fail git merge conflict2 &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -496,12 +496,12 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git -C submod checkout HEAD^ &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -515,12 +515,12 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
 	echo X >submod/empty &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -534,12 +534,12 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files
 	echo X >submod/untracked &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -556,13 +556,13 @@ test_expect_success 'rm of a conflicted populated submodule with a .git director
 	) &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git merge --abort &&
@@ -574,7 +574,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
 	git reset --hard &&
 	test_must_fail git merge conflict2 &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -591,10 +591,10 @@ test_expect_success 'rm of a populated submodule with a .git directory migrates
 		rm -r ../.git/modules/sub
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test -s actual &&
+	test_file_not_empty actual &&
 	test_i18ngrep Migrating output.err
 '
 
@@ -620,7 +620,7 @@ test_expect_success 'setup subsubmodule' '
 
 test_expect_success 'rm recursively removes work tree of unmodified submodules' '
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -630,12 +630,12 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	git submodule update --recursive &&
 	git -C submod/subsubmod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -645,12 +645,12 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -660,12 +660,12 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -680,10 +680,10 @@ test_expect_success "rm absorbs submodule's nested .git directory" '
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/subsubmod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/subsubmod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test -s actual &&
+	test_file_not_empty actual &&
 	test_i18ngrep Migrating output.err
 '
 
-- 
Rohit

Re: [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path>

[Eric Sunshine]

On Mon, Mar 4, 2019 at 7:09 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> Previously  we  were  using  `test -(d|f|e|s)`  to  verify  the  presence of a
> directory/file, but we already have helper functions, viz, `test_path_is_dir`,
> `test_path_is_file`,    `test_path_is_missing`    and    `test_file_not_empty`
> with better functionality.

As with the commit message of 2/3, many of the words in this message
are separated by multiple spaced. Please fold out the excess so there
is only a single space between words.

Also, no need to say "previously" since readers know that the patch is
changing something. Rewrite in imperative mood:

    Take advantage of helper functions test_path_is_dir(),
    test_path_is_missing(), etc. to replace `test -d|f|e|s` since the
    functions make the code more readable and have better error
    messages.

> These helper functions make code more readable and informative to someone new,
> also these functions have better error messages.
>
> Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>

Re: [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path>

[Rohit Ashiwal]

Hey Eric

On 2019-03-05 0:42 Eric Sunshine <sunshine@sunshineco.com> wrote:
> As with the commit message of 2/3, many of the words in this message
> are separated by multiple spaced. Please fold out the excess so there
> is only a single space between words.
>
> Also, no need to say "previously" since readers know that the patch is
> changing something. Rewrite in imperative mood:

Okay, I'll keep that in mind from next time onwards. The spaces were
provided to make the commit message look aesthetically pleasing.

These changes aside, is there anything you would like to add to the review?
or is it good to go for a merge?

Thanks for advice
Rohit

Re: [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path>

[Eric Sunshine]

On Tue, Mar 5, 2019 at 8:43 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> On 2019-03-05 0:42 Eric Sunshine <sunshine@sunshineco.com> wrote:
> > As with the commit message of 2/3, many of the words in this message
> > are separated by multiple spaced. Please fold out the excess so there
> > is only a single space between words.
> >
> > Also, no need to say "previously" since readers know that the patch is
> > changing something. Rewrite in imperative mood:
>
> Okay, I'll keep that in mind from next time onwards. The spaces were
> provided to make the commit message look aesthetically pleasing.
>
> These changes aside, is there anything you would like to add to the review?
> or is it good to go for a merge?

I don't understand your question.

Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path>

[Rohit Ashiwal]

Eric

I was asking if this patch is good enough to be added to the existing code? Does this patch look good?


Regards
Rohit

Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path>

[Eric Sunshine]

On Tue, Mar 5, 2019 at 9:22 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> I was asking if this patch is good enough to be added to the
> existing code? Does this patch look good?

I didn't review the patch with a critical-enough eye to be able to say
that every change maintains fidelity with the original code. As
mentioned in [1]:

    [...] an important reason for limiting the scope of this change
    [...] is to ease the burden on people who review your submission.
    Large patch series tend to tax reviewers heavily, even (and often)
    when repetitive and simple, like replacing `test -d` with
    `test_path_is_dir()`. The shorter and more concise a patch series
    is, the more likely that it will receive quality reviews.

This patch, due to its length and repetitive nature, falls under the
category of being tedious to review, which makes it all the more
likely that a reviewer will overlook a problem.

And, it's not always obvious at a glance that a change is correct. For
instance, taking a look at the final patch band:

    - ! test -d submod &&
    - ! test -d submod/subsubmod/.git &&
    + test_path_is_missing submod &&
    + test_path_is_missing submod/subsubmod/.git &&

Superficially, the transformation seems straightforward. However, that
doesn't mean it maintains fidelity with the original or even means the
same thing. To review this change properly requires understanding the
original intent of "! test -d".

The meaning of that expression can vary depending upon the context. Is
it checking that that path is not a directory (but it is okay if a
plain file exists there)? Or does it merely care about existence
(neither directory nor any other type of entry)? If the latter, then
the transformation is probably correct, however, if the former, then
it likely isn't correct. So, understanding the overall context of the
test is important for judging if a particular change is correct, and
many (volunteer) reviewers simply don't have the time to delve that
deeply to make a proper judgment.

[1]: https://public-inbox.org/git/CAPig+cSZZaCT0G3hysmjn_tNvZmYGp=5cXpZHkdphbWXnONSVQ@mail.gmail.com/

Re:

[Rohit Ashiwal]

Hey Eric

On Tue, 5 Mar 2019 09:57:40 -0500 Eric Sunshine <sunshine@sunshineco.com> wrote:
> This patch, due to its length and repetitive nature, falls under the
> category of being tedious to review, which makes it all the more
> likely that a reviewer will overlook a problem.

Yes, I clearly understand that this patch has become too big to review.
It will require time to carefully review and reviewers are doing their
best to maintain the utmost quality of code.

> And, it's not always obvious at a glance that a change is correct. For
> instance, taking a look at the final patch band:
>
>     - ! test -d submod &&
>     - ! test -d submod/subsubmod/.git &&
>     + test_path_is_missing submod &&
>     + test_path_is_missing submod/subsubmod/.git &&

Duy actually confirms that this transformation is correct in this[1] email.
(I know that, it was given as an example, but I'll leave the link anyway).

Thanks
Rohit

[1]: https://public-inbox.org/git/CACsJy8BYeLvB7BSM_Jt4vwfGsEBuhaCZfzGPOHe=B=7cvnRwrg@mail.gmail.com/

Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path>

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> This patch, due to its length and repetitive nature, falls under the
> category of being tedious to review, which makes it all the more
> likely that a reviewer will overlook a problem.
>
> And, it's not always obvious at a glance that a change is correct. For
> instance, taking a look at the final patch band:
>
>     - ! test -d submod &&
>     - ! test -d submod/subsubmod/.git &&
>     + test_path_is_missing submod &&
>     + test_path_is_missing submod/subsubmod/.git &&
>
> Superficially, the transformation seems straightforward. However, that
> doesn't mean it maintains fidelity with the original or even means the
> same thing. To review this change properly requires understanding the
> original intent of "! test -d".
>
> ... , and
> many (volunteer) reviewers simply don't have the time to delve that
> deeply to make a proper judgment.

True.  The microproject was supposed to be a gentle introduction to
and a practice session of the process of modifying, committing,
submitting, and responding to reviews.  Learning the usual Git
contributor workflow, without spending too much community resources
like reviewers' time (as opposed to the real "here is my itch; let's
improve the system, and please help me doing so").

In any case, I have spent some time with the patch and I think the
changes are generaly OK; some (like using "test_path_is_missing foo"
instead of "test ! -f foo" after "git rm foo" to ensure the path no
longer exists) are improvements.

An unrelated tangent, but what do you think of this patch?  In the
context of testing "git rm", if foo is a dangling symbolic link,
"git rm foo && test_path_is_missing foo" would need something like
this to work correctly, I would think.

 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 681c41ba32..dfe0d4aff4 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -603,7 +603,7 @@ test_file_not_empty () {
 }
 
 test_path_is_missing () {
-	if test -e "$1"
+	if test -e "$1" || test -L "$1"
 	then
 		echo "Path exists:"
 		ls -ld "$1"

Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path>

[Eric Sunshine]

On Fri, Mar 8, 2019 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote:
> An unrelated tangent, but what do you think of this patch?  In the
> context of testing "git rm", if foo is a dangling symbolic link,
> "git rm foo && test_path_is_missing foo" would need something like
> this to work correctly, I would think.
>
>  test_path_is_missing () {
> -       if test -e "$1"
> +       if test -e "$1" || test -L "$1"
>         then
>                 echo "Path exists:"
>                 ls -ld "$1"

Makes sense. Won't we also want:

    test_path_exists () {
    -    if ! test -e "$1"
    +   if ! test -e "$1" && ! test -L "$1"
       then
            echo "Path $1 doesn't exist. $2"

or something like that?

Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path>

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, Mar 8, 2019 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote:
>> An unrelated tangent, but what do you think of this patch?  In the
>> context of testing "git rm", if foo is a dangling symbolic link,
>> "git rm foo && test_path_is_missing foo" would need something like
>> this to work correctly, I would think.
>>
>>  test_path_is_missing () {
>> -       if test -e "$1"
>> +       if test -e "$1" || test -L "$1"
>>         then
>>                 echo "Path exists:"
>>                 ls -ld "$1"
>
> Makes sense. Won't we also want:
>
>     test_path_exists () {
>     -    if ! test -e "$1"
>     +   if ! test -e "$1" && ! test -L "$1"
>        then
>             echo "Path $1 doesn't exist. $2"
>
> or something like that?

That would make them symmetric, but what I was driving at with "In
the context of testing git rm" was that I highly suspect that among
other existing users of test_path_is_missing there are some that
want to consider a dangling symbolic link as if it is not there (and
vice versa for test_path_exists), and it may not be a good idea to
unconditionally declare that, unlike the underlying "test" command
that dereferences symlinks for most operations, our wrapper does not
dereference symbolic links, which is what the "what do you think?"
patch and your addtion do.

Re: [GSoC][PATCH v3 0/3] Use helper functions in test script

[Eric Sunshine]

On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> This patch ultimately aims to replace `test -(d|f|e|s)` calls in t3600-rm.sh
> Previously we were using these to verify the presence of diretory/file, but
> we already have helper functions, viz, `test_path_is_dir`, `test_path_is_file`,
> `test_path_is_missing` and `test_file_not_empty` with better functionality
>
> Helper functions are better as they provide better error messages and
> improve readability. They are friendly to someone new to code.

As an aid to reviewers, please use the cover-letter to explain what
changed since the previous version of the patch series. Also, to
further help reviewers, consider using the --range-diff or --interdiff
options with "git format-patch" to visually show the changes since the
previous attempt (in addition to your prose explanation).

Finally, it is a good idea to provide a link, like this[1], to the
previous round in order to jog the memory of existing reviewers and to
provide context for people new to the review of the series.

[1]: https://public-inbox.org/git/20190303233750.6500-1-rohit.ashiwal265@gmail.com/