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

From: Eric Sunshine <sunshine@sunshineco.com>
To: matvore@google.com
Cc: Git List <git@vger.kernel.org>, Jeff King <peff@peff.net>,
	Jonathan Tan <jonathantanmy@google.com>,
	Junio C Hamano <gitster@pobox.com>,
	Jonathan Nieder <jrn@google.com>
Subject: Re: [PATCH v2 1/6] CodingGuidelines: add shell piping guidelines
Date: Mon, 17 Sep 2018 20:16:46 -0400	[thread overview]
Message-ID: <CAPig+cSzddcS+8mx=GMbJ5BP+=fPtza+7UdA5ugN+83NuOHyiw@mail.gmail.com> (raw)
In-Reply-To: <c625bfe2205d51b3158ef71e4bf472708642c146.1537223021.git.matvore@google.com>

On Mon, Sep 17, 2018 at 6:24 PM Matthew DeVore <matvore@google.com> wrote:
> 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.)

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