Rohit Ashiwal <rohit.ashiwal...@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.
