[PATCH v3 1/5] CodingGuidelines: add shell piping guidelines
Add two guidelines: - pipe characters should appear at the end of lines, and not cause indentation - pipes should be avoided when they swallow exit codes that can potentially fail --- Documentation/CodingGuidelines | 27 +++ 1 file changed, 27 insertions(+) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 48aa4edfb..6d265327c 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,6 +181,15 @@ For shell scripts specifically (not exhaustive): does not have such a problem. + - In a piped chain such as "grep blob objects | sort", the exit codes + returned by processes besides the last are ignored. This means that + if git crashes at the beginning or middle of a chain, it may go + undetected. Prefer writing the output of that command to a + temporary file with '>' rather than pipe it. + + - The $(git ...) construct also discards git's exit code, so if the + goal is to test that particular command, redirect its output to a + temporary file rather than wrap it with $( ). For C programs: -- 2.19.0.444.g18242da7ef-goog
Re: [PATCH v3 1/5] CodingGuidelines: add shell piping guidelines
On Thu, Sep 20, 2018 at 9:43 PM Matthew DeVore wrote: > Add two guidelines: Probably s/two/three/ or s/two/several/ since the patch now adds three guidelines. > - pipe characters should appear at the end of lines, and not cause >indentation The "not cause indentation" bit is outdated since the added guideline no longer says this. > - pipes should be avoided when they swallow exit codes that can >potentially fail And: - $(git ...) should be avoided ...
Re: [PATCH v3 1/5] CodingGuidelines: add shell piping guidelines
On Thu, Sep 20, 2018 at 7:06 PM Eric Sunshine wrote: > > On Thu, Sep 20, 2018 at 9:43 PM Matthew DeVore wrote: > > Add two guidelines: > > Probably s/two/three/ or s/two/several/ since the patch now adds three > guidelines. > > > - pipe characters should appear at the end of lines, and not cause > >indentation > > The "not cause indentation" bit is outdated since the added guideline > no longer says this. > > > - pipes should be avoided when they swallow exit codes that can > >potentially fail > > And: > > - $(git ...) should be avoided ... Thank you for pointing this out - I obviously forgot to update the commit message. Here is the revised message: CodingGuidelines: add shell piping guidelines Add the following guidelines: - pipe characters should appear at the end of lines, not the beginning, and the \ line continuation character should be omitted - pipes and $(git ...) should be avoided when they swallow exit codes of processes that can potentially fail
Re: [PATCH v3 1/5] CodingGuidelines: add shell piping guidelines
On Thu, Sep 20, 2018 at 06:43:27PM -0700, Matthew DeVore wrote: > Add two guidelines: > > - pipe characters should appear at the end of lines, and not cause >indentation > - pipes should be avoided when they swallow exit codes that can >potentially fail > --- > Documentation/CodingGuidelines | 27 +++ > 1 file changed, 27 insertions(+) > > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines > index 48aa4edfb..6d265327c 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 && > + ... > + The above are general shell coding style guidelines, so it makes sense to include them in 'Documentation/CodingGuidelines'. > - We prefer "test" over "[ ... ]". > > - We do not write the noiseword "function" in front of shell > @@ -163,6 +181,15 @@ For shell scripts specifically (not exhaustive): > > does not have such a problem. > > + - In a piped chain such as "grep blob objects | sort", the exit codes Let's make an example with git in it, e.g. something like this: git cmd | grep important | sort since just two lines below the new text mentions git crashing. > + returned by processes besides the last are ignored. This means that > + if git crashes at the beginning or middle of a chain, it may go > + undetected. Prefer writing the output of that command to a > + temporary file with '>' rather than pipe it. > + > + - The $(git ...) construct also discards git's exit code, so if the This contruct is called command substitution, and it does preserve the command's exit code, when the expanded text is assigned to a variable: $ var=$(exit 42) ; echo $? 42 Note, however, that even in that case only the exit code of the last command substitution is preserved: $ var=$(exit 1)foo$(exit 2)bar$(exit 3) ; echo $? 3 > + goal is to test that particular command, redirect its output to a > + temporary file rather than wrap it with $( ). I find this a bit vague, and to me it implies that ignoring the exit code of a git command that is not the main focus of the given test is acceptable, e.g. (made up pseudo example): test_expect_success 'fetch gets what it should' ' git fetch $remote && test "$(git rev-parse just-fetched)" = $expected_oid ' In my opinion no tests should ignore the exit code of any git command, ever. 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 >
Re: [PATCH v3 1/5] CodingGuidelines: add shell piping guidelines
On Mon, Sep 24, 2018 at 2:03 PM SZEDER Gábor wrote: > > + - In a piped chain such as "grep blob objects | sort", the exit codes > > Let's make an example with git in it, e.g. something like this: > > git cmd | grep important | sort > > since just two lines below the new text mentions git crashing. Done. > > + - The $(git ...) construct also discards git's exit code, so if the > > This contruct is called command substitution, and it does preserve the > command's exit code, when the expanded text is assigned to a variable: > > $ var=$(exit 42) ; echo $? > 42 > > Note, however, that even in that case only the exit code of the last > command substitution is preserved: > > $ var=$(exit 1)foo$(exit 2)bar$(exit 3) ; echo $? > 3 > OK, I've changed this guideline to allow for setting a variable with command substitution, but not in other contexts. It's worded sufficiently openly such that your latter example will be forbidden. > > + goal is to test that particular command, redirect its output to a > > + temporary file rather than wrap it with $( ). > > I find this a bit vague, and to me it implies that ignoring the exit > code of a git command that is not the main focus of the given test is > acceptable, e.g. (made up pseudo example): > > test_expect_success 'fetch gets what it should' ' > git fetch $remote && > test "$(git rev-parse just-fetched)" = $expected_oid > ' > > In my opinion no tests should ignore the exit code of any git > command, ever. This seems like a pretty strong assertion, but something very similar is written in t/README (in the "don't" section): - use '! git cmd' when you want to make sure the git command exits with failure in a controlled way by calling "die()". Instead, use 'test_must_fail git cmd'. This will signal a failure if git dies in an unexpected way (e.g. segfault). So I've changed this to basically say you should never ignore git's exit code. 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 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: + + 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. + 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. + - use perl without spelling it as "$PERL_PATH". This is to help our 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 tw
Re: [PATCH v3 1/5] CodingGuidelines: add shell piping guidelines
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 > > 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 > > >
Re: [PATCH v3 1/5] CodingGuidelines: add shell piping guidelines
On Thu, Sep 27, 2018 at 2:18 PM SZEDER Gábor wrote: > > On Tue, Sep 25, 2018 at 02:58:08PM -0700, Matthew DeVore wrote: > > + - Use Git upstream in the non-final position in a piped chain, as in: > > Note the starting upper case 'U'. > > > + - Use command substitution in a way that discards git's exit code. > > 'U' again. > ... > > + > > + 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. I've replaced the example and added "Don't" in front of each new item (explanation for this below). Here is a new diff for this file - the rest of the commit is the same: platform commands; just use '! cmd'. We are not in the business of verifying that the world given to us sanely works. + - Don't use Git upstream in the non-final position in a piped chain, as + in: + + 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. + + - Don't use command substitution in a way that discards git's exit + code. 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 "refs/heads/foo" = "$(git symbolic-ref HEAD)" + + is not OK and a crash in git could go undetected. + - Don't use perl without spelling it as "$PERL_PATH". This is to help our 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 > > > - 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. > I didn't like how easy it was to mistake a "Don't" item for a "Do" (although temporarily until you read the first sentence, but it's still confusing). So I added a new commit to clean this section up. Here it is: Author: Matthew DeVore Date: Mon Oct 1 15:30:49 2018 -0700 t/README: reformat Do, Don't, Keep in mind lists The list of Don'ts for test writing has grown large such that it is hard to see at a glance which section an item is in. In other words, if I ignore a little bit of surrounding context, the "don'ts" look like "do's." To make the list more readable, prefix "Don't" in front of every first sentence in the items. Also, the "Keep in mind" list is out of place and awkward, because it was a very short "list" beneath two very long ones, and it seemed easy to miss under the list of "don'ts," and it only had one item. So move this item to the list of "do's" and phrase as "Remember..." Signed-off-by: Matthew DeVore diff --git a/t/README b/t/README index 9028b47d9..85024aba6 100644 --- a/t/README +++ b/t/README @@ -393,13 +393,13 @@ This test harness library does the following things: consistently when command line arguments --verbose (or -v), --debug (or -d), and --immediate (or -i) is given. -Do's, don'ts & things to keep in mind +Do's & don'ts - Here are a few examples of things you probably should and shouldn't do when writing tests. -Do: +Here are the "do's:" - Put all code inside test_expect_success and other assertions. @@ -444,16 +444,21 @@ Do: Windows, where the shell (MSYS bash) mangles absolute path names. For details, see the commit message of 4114156ae9. -Don't: + - Remember that inside the
