Re: [GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 4, 2019, 04:17 UTC
- Message-ID
- <xmqqsgw3w9wg.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20190303233750.6500-3-rohit.ashiwal265@gmail.com>
Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:
Show 7 quoted lines
> 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..
Show 23 quoted lines
> 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.
Show 6 quoted lines
> -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.
Show 17 quoted lines
> - 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.
Show 8 quoted lines
> 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.