On Tue, Sep 25, 2018 at 02:58:08PM -0700, Matthew DeVore wrote: > Here is the new commit with updated message (I will wait for a day or > two before I send a reroll): > > Documentation: add shell guidelines > > Add the following guideline to Documentation/CodingGuidelines: > > &&, ||, and | should appear at the end of lines, not the > beginning, and the \ line continuation character should be > omitted > > And the following to t/README (since it is specific to writing tests): > > pipes and $(git ...) should be avoided when they swallow exit > codes of Git processes > > Signed-off-by: Matthew DeVore <matvore@xxxxxxxxxx> > > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines > index 48aa4edfb..3d2cfea9b 100644 > --- a/Documentation/CodingGuidelines > +++ b/Documentation/CodingGuidelines > @@ -118,6 +118,24 @@ For shell scripts specifically (not exhaustive): > do this > fi > > + - If a command sequence joined with && or || or | spans multiple > + lines, put each command on a separate line and put && and || and | > + operators at the end of each line, rather than the start. This > + means you don't need to use \ to join lines, since the above > + operators imply the sequence isn't finished. > + > + (incorrect) > + grep blob verify_pack_result \ > + | awk -f print_1.awk \ > + | sort >actual && > + ... > + > + (correct) > + grep blob verify_pack_result | > + awk -f print_1.awk | > + sort >actual && > + ... > + > - We prefer "test" over "[ ... ]". > > - We do not write the noiseword "function" in front of shell > @@ -163,7 +181,6 @@ For shell scripts specifically (not exhaustive): > > does not have such a problem. > > - > For C programs: > > - We use tabs to indent, and interpret tabs as taking up to > diff --git a/t/README b/t/README > index 9028b47d9..3e28b72c4 100644 > --- a/t/README > +++ b/t/README > @@ -461,6 +461,32 @@ Don't: > platform commands; just use '! cmd'. We are not in the business > of verifying that the world given to us sanely works. > > + - Use Git upstream in the non-final position in a piped chain, as in: Note the starting upper case 'U'. > + > + git -C repo ls-files | > + xargs -n 1 basename | > + grep foo > + > + which will discard git's exit code and may mask a crash. In the > + above example, all exit codes are ignored except grep's. > + > + Instead, write the output of that command to a temporary > + file with ">" or assign it to a variable with "x=$(git ...)" rather > + than pipe it. > + > + - Use command substitution in a way that discards git's exit code. 'U' again. > + When assigning to a variable, the exit code is not discarded, e.g.: > + > + x=$(git cat-file -p $sha) && > + ... > + > + is OK because a crash in "git cat-file" will cause the "&&" chain > + to fail, but: > + > + test_cmp expect $(git cat-file -p $sha) > + > + is not OK and a crash in git could go undetected. Well, this is not OK indeed, because it doesn't make any sense in the first place :) 'test_cmp' requires two paths as argumens, but the output of 'git cat-file -p' is the whole _content_ of the given object. > - use perl without spelling it as "$PERL_PATH". This is to help our Note the starting lower case 'u'. This is because these are the continuation of the "Don't:" some lines earlier, so your new points should start with a lower case 'u' as well. Sidenote: I think we should consider reformatting this whole section as: - Don't do this. - Don't do that. because it grew so much that when I look at the last points, then that starting "Don't:" has already scrolled out of my screen. > friends on Windows where the platform Perl often adds CR before > the end of line, and they bundle Git with a version of Perl that > > > > > > > > These last two points, however, are specific to test scripts, > > therefore I think they would be better placed in 't/README', where the > > rest of the test-specific guidelines are. > > > > > For C programs: > > > > > > -- > > > 2.19.0.444.g18242da7ef-goog > > >
