Re: [PATCH 15/15] t3040 (subprojects-basic): modernize style
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Dec 7, 2011, 22:21 UTC
- Message-ID
- <20111207222126.GF2911@elie.hsd1.il.comcast.net>
- In-Reply-To
- <1323286611-4806-16-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Put the opening quote starting each test on the same line as the > test_expect_* invocation. While at it:
[...]
> - Use <<\-EOF in preference to <<EOF to save readers the trouble of > looking for variable interpolations.
I think you mean <<-\EOF. :)
[...]
Show 5 quoted lines
> - Chain commands with &&. Breaks in a test assertion's && chain can > potentially hide failures from earlier commands in the chain. > > - Use test_expect_code() in preference to checking the exit status of > various statements by hand.
I guess these two are the motivation?
> Inspired-by: Jonathan Nieder <jrnieder@gmail.com>
Oh, dear.
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
[...]
Show 13 quoted lines
> --- a/t/t3040-subprojects-basic.sh > +++ b/t/t3040-subprojects-basic.sh > @@ -3,81 +3,81 @@ > test_description='Basic subproject functionality' > . ./test-lib.sh > > -test_expect_success 'Super project creation' \ > - ': >Makefile && > - git add Makefile && > - git commit -m "Superproject created"' > - > - > -cat >expected <<EOF
[...]
It would be easier to read if each preimage test assertion were next to the corresponding postimage test assertion. Does "git diff --patience" do better?
Show 12 quoted lines
> -test_expect_success 'Super project creation' \ > - ': >Makefile && > - git add Makefile && > - git commit -m "Superproject created"' > - > - > +test_expect_success 'setup: create superproject' ' > + : >Makefile && > + git add Makefile && > + git commit -m "Superproject created" > +' > +
Ok, makes sense.
Show 12 quoted lines
> -cat >expected <<EOF > -:000000 160000 00000... A sub1 > -:000000 160000 00000... A sub2 > -EOF > -test_expect_success 'create subprojects' \ > - 'mkdir sub1 && > - ( cd sub1 && git init && : >Makefile && git add * && > - git commit -q -m "subproject 1" ) && > +test_expect_success 'setup: create subprojects' ' > + mkdir sub1 && > + ( cd sub1 && git init && : >Makefile && git add * && > + git commit -q -m "subproject 1" ) &&
If cleaning up the style anyway, I would write this as
mkdir sub1 && ( cd sub1 && git init && >Makefile && git add Makefile && git commit -m "subproject 1" )
Or
mkdir sub1 && ( cd sub1 && git init && test_commit subproject-1 Makefile )
But leaving it alone like you did is probably better. ;-)
[...]
Show 14 quoted lines
> - git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current && > - test_cmp expected current' > - > -git branch save HEAD > - > + git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current && > + git branch save HEAD && > + cat >expected <<-\EOF && > + :000000 160000 00000... A sub1 > + :000000 160000 00000... A sub2 > + EOF > + test_cmp expected current > +' > +
At first I wondered if "git branch save HEAD" is logically part of the same test. After all, it's just meant as a baseline for use by later tests.
After thinking about it for a few seconds, though, that's exactly what this test is about. Maybe it would have been clearer if the two setup tests were combined into one (but please don't take this advice too seriously; I'm just musing).
Show 5 quoted lines
> -test_expect_success 'check if fsck ignores the subprojects' \ > - 'git fsck --full' > +test_expect_success 'check if fsck ignores the subprojects' ' > + git fsck --full > +'
Does this test imply that one of the subprojects is broken somehow?
Show 17 quoted lines
> -
> -test_expect_success 'check if commit in a subproject detected' \
> - '( cd sub1 &&
> - echo "all:" >>Makefile &&
> - echo " true" >>Makefile &&
> - git commit -q -a -m "make all" ) && {
> - git diff-files --exit-code
> - test $? = 1
> - }'
> +
> +test_expect_success 'check if commit in a subproject detected' '
> + ( cd sub1 &&
> + echo "all:" >>Makefile &&
> + echo " true" >>Makefile &&
> + git commit -q -a -m "make all" ) &&
> + test_expect_code 1 git diff-files --exit-code
> +'Nice. Style again: I'd be tempted to reformat as
( cd sub1 && echo "all:" >>Makefile && ... ) && test_expect_code 1 git diff-files --exit-code
to make the subshell scope a little clearer, but exercising restraint like you did may be better.
[...]
Show 10 quoted lines
> > # the index must contain the object name the HEAD of the > # subproject sub1 was at the point "save" > -test_expect_success 'checkout in superproject' \ > - 'git checkout save && > - git diff-index --exit-code --raw --cached save -- sub1' > +test_expect_success 'checkout in superproject' ' > + git checkout save && > + git diff-index --exit-code --raw --cached save -- sub1 > +'
Thanks much. The result really is a little easier on the eyes, and the changes look safe.