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