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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2019, 13:30 UTC
Message-ID: <xmqqwolgytk5.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190303122842.30380-3-rohit.ashiwal265@gmail.com>

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.

Show 11 quoted lines

>  # 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.

Show 5 quoted lines

>  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.