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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Sep 24, 2018, 21:03 UTC
Message-ID: <20180924210314.GE27036@localhost>
In-Reply-To: <7dd9a1120726dee34aeebdfa4bf45a232c185500.1537493611.git.matvore@google.com>

On Thu, Sep 20, 2018 at 06:43:27PM -0700, Matthew DeVore wrote:

Show 36 quoted lines

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

Show 8 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

>  For C programs:
>  
> -- 
> 2.19.0.444.g18242da7ef-goog
>