Re: [Libguestfs] [libnbd PATCH v2] tests/requires: wrap source code at 80 characters
On 4/19/23 20:21, Eric Blake wrote:
On Wed, Apr 19, 2023 at 05:31:27PM +0200, Laszlo Ersek wrote:
> Embedding a shell script in a multi-line C string literal is an exercise
> in pain. I can see why the original author (whom I shall not look up with
> git-blame :) ) went for the easy route. Still, we want the source code to
> fit in 80 columns.
>
> At Eric's suggestion, also:
>
> - replace the non-portable control operator "|&" with the portable
> redirection operator "2>&1" and the portable control operator
"|";
>
> - replace the "if ...; then exit 1; else exit 0; fi" constructs with
"!
> ...".
Reviewed-by: Eric Blake <eblake(a)redhat.com>
Thanks!
> Notes:
> v2:
>
> - v1: https://listman.redhat.com/archives/libguestfs/2023-April/031298.html
>
> - incorporate Eric's recommendations (commit message, code)
>
> tests/requires.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tests/requires.c b/tests/requires.c
> index 199e30605473..2b7031532fe2 100644
> --- a/tests/requires.c
> +++ b/tests/requires.c
> @@ -57,7 +57,8 @@ requires_qemu_nbd_tls_support (const char *qemu_nbd)
> * interested in the error message that it prints.
> */
> snprintf (cmd, sizeof cmd,
> - "if %s --object tls-creds-x509,id=tls0 |& grep -sq 'TLS
credentials support requires GNUTLS'; then exit 1; else exit 0; fi",
> + "! %s --object tls-creds-x509,id=tls0 2>&1 \\\n"
> + " | grep -sq 'TLS credentials support requires
GNUTLS'\n",
The four bytes " \\\n " portion between the two lines are not
essential (the shell grammar doesn't care if there is a newline at
this point);
Indeed, but I wanted the shell script's line breaks to follow the C
source's line breaks. :)
in fact, if you put the | before \n instead of after, you
can use a newline without needing the \.
That's right, but personally I prefer
cmd1 \
| cmd2 \
| cmd3 \
| cmd4
over
cmd1 |
cmd2 |
cmd3 |
cmd4
because in the latter (especially if the commands are long / complex),
it's hard to see where a pipeline ends and a brand new pipeline starts.
Placing the pipe symbol at the front gives more visual cues.
(In C, things are different: whenever we need expressions spanning
multiple lines, we can easily keep the operators at the end of the line.
That's because we already have parens in place, e.g. because the
expression is the controlling expression of a statement, or because we
can simply add parens for the sake of indentation. But I've never seen
something like this, in shell:
{
cmd1 |
cmd2 |
cmd3 |
cmd4;
}
and anyway I think it would run the risk of the exact same mistake --
forgetting a pipe symbol somewhere would not cause a "compilation error"
but runtime misbehavior.)
But I don't see any point
changing this code yet again just to golf out a few more bytes. Let's
leave it as written in your v2.
Thanks! :) At least this time all of it was fully intended on my part.
Laszlo