diff mbox series

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

Message ID 20190303233750.6500-3-rohit.ashiwal265@gmail.com (mailing list archive)
State New, archived
Headers show
Series Use helper functions in test script | expand

Commit Message

Rohit Ashiwal March 3, 2019, 11:37 p.m. UTC 
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(-)

Comments

Junio C Hamano March 4, 2019, 4:17 a.m. UTC | #1 
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.
diff mbox series

Patch

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