Re: [PATCH v3 1/5] CodingGuidelines: add shell piping guidelines

From: Matthew DeVore <matvore@google.com>
Date: Sep 25, 2018, 21:58 UTC
Message-ID: <CAMfpvhJ-chi7OMRKjjk79r0uqCqW67Vj9J=tT7Kz-XUmw41H5A@mail.gmail.com>
In-Reply-To: <20180924210314.GE27036@localhost>

On Mon, Sep 24, 2018 at 2:03 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:

Show 7 quoted lines

> > + - 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.

Show 14 quoted lines

> > + - 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.

Show 14 quoted lines

> > +   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 <matvore@google.com>

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 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
> >