Re: [PATCH v2 1/6] CodingGuidelines: add shell piping guidelines

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 18, 2018, 00:16 UTC
Message-ID: <CAPig+cSzddcS+8mx=GMbJ5BP+=fPtza+7UdA5ugN+83NuOHyiw@mail.gmail.com>
In-Reply-To: <c625bfe2205d51b3158ef71e4bf472708642c146.1537223021.git.matvore@google.com>

On Mon, Sep 17, 2018 at 6:24 PM Matthew DeVore <matvore@google.com> wrote:

Show 14 quoted lines

> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> @@ -163,6 +163,35 @@ For shell scripts specifically (not exhaustive):
> + - In a piped sequence which spans multiple lines, put each statement
> +   on a separate line and put pipes on the end of each line, rather
> +   than the start. This means you don't need to use \ to join lines,
> +   since | implies a join already. Also, do not indent subsequent
> +   lines; if you need a sequence to visually stand apart from the
> +   surrounding code, use a blank line before and/or after the piped
> +   sequence.
> +
> +       (incorrect)
> +       [...]
> +       (correct)
> +       echo '...' > expected

Existing tests seem to favor the name "expect" over "expected", so perhaps use that instead.

    $ git grep '>expect\b' -- t | wc -l
    2674
    $ git grep '>expected\b' -- t | wc -l
    1406

> +       git ls-files -s file.1 file.2 file.3 file.4 file.5 |
> +       awk '{print $1}' |
> +       sort >observed

This is not a great example since it flatly contradicts the very next bit of advice added by this patch about not placing a Git command upstream in a pipe. Perhaps come up with an example which doesn't suffer this shortcoming.

I've seen the advice earlier in the thread of not indenting the sub-commands in a pipe, but I find that the result makes it far more difficult to see which commands are part of the pipe sequence than with them indented, so I'm not convinced that this advice should be in the guidelines. (But that just my opinion.)

Show 5 quoted lines

> + - In a pipe, any non-zero exit codes returned by processes besides
> +   the last will be ignored. If there is any possibility some
> +   non-final command in the pipe will raise an error, prefer writing
> +   the output of that command to a temporary file with '>' rather than
> +   pipe it.

It's not so much that we care about losing a non-zero exit code (which might be perfectly acceptable depending upon the context) but that we care about missing a Git command which outright crashes. So, it might make sense to make this text more specific by saying that ("exit code indicating a crash" and "Git command") rather than being generic in saying only "exit code" and "command".

Also, what about expression like $(git foo) by which a crash of a Git command can also be lost? Do we want to talk about that, as well?