git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Cc: Johannes.Schindelin@gmx.de, christian.couder@gmail.com,
	git@vger.kernel.org, t.gummerer@gmail.com
Subject: Re: [GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines
Date: Mon, 04 Mar 2019 13:17:35 +0900	[thread overview]
Message-ID: <xmqqsgw3w9wg.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190303233750.6500-3-rohit.ashiwal265@gmail.com> (Rohit Ashiwal's message of "Mon, 4 Mar 2019 05:07:49 +0530")

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.

  reply	other threads:[~2019-03-04  4:17 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-03 12:28 [GSoC][PATCH 0/3] Use helper functions in test script Rohit Ashiwal
2019-03-03 12:28 ` [PATCH 1/3] test functions: Add new function `test_file_not_empty` Rohit Ashiwal
2019-03-03 13:20   ` Junio C Hamano
2019-03-03 13:29     ` Rohit Ashiwal
2019-03-03 13:33       ` none Junio C Hamano
2019-03-03 14:07         ` Clearing logic Rohit Ashiwal
2019-03-03 16:19           ` Thomas Gummerer
2019-03-03 12:28 ` [PATCH 2/3] t3600: refactor code according to contemporary guidelines Rohit Ashiwal
2019-03-03 13:30   ` Junio C Hamano
2019-03-03 14:13     ` t3600: refactor code according to comtemporary guidelines Rohit Ashiwal
2019-03-03 12:28 ` [PATCH 3/3] t3600: use helper functions from test-lib-functions Rohit Ashiwal
2019-03-03 13:32   ` Junio C Hamano
2019-03-03 23:37 ` [GSoC][PATCH v2 0/3] Use helper functions in test script Rohit Ashiwal
2019-03-03 23:37   ` [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal
2019-03-04  3:45     ` Junio C Hamano
2019-03-03 23:37   ` [GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines Rohit Ashiwal
2019-03-04  4:17     ` Junio C Hamano [this message]
2019-03-03 23:37   ` [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> Rohit Ashiwal
2019-03-04 12:07 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Rohit Ashiwal
2019-03-04 12:07   ` [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal
2019-03-05  0:17     ` Eric Sunshine
2019-03-05 12:43       ` Junio C Hamano
2019-03-05 13:27       ` [GSoc][PATCH " Rohit Ashiwal
2019-03-04 12:08   ` [GSoC][PATCH v3 2/3] t3600: modernize style Rohit Ashiwal
2019-03-05  0:36     ` Eric Sunshine
2019-03-05 12:44       ` Junio C Hamano
2019-03-04 12:08   ` [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path> Rohit Ashiwal
2019-03-05  0:42     ` Eric Sunshine
2019-03-05 13:42       ` Rohit Ashiwal
2019-03-05 14:03         ` Eric Sunshine
2019-03-05 14:21           ` [GSoC][PATCH v2 " Rohit Ashiwal
2019-03-05 14:57             ` Eric Sunshine
2019-03-05 23:38               ` Rohit Ashiwal
2019-03-08  5:38               ` [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> Junio C Hamano
2019-03-08  9:51                 ` Eric Sunshine
2019-03-11  1:54                   ` Junio C Hamano
2019-03-05  0:09   ` [GSoC][PATCH v3 0/3] Use helper functions in test script Eric Sunshine

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqsgw3w9wg.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=rohit.ashiwal265@gmail.com \
    --cc=t.gummerer@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).