Re: [PATCH 1/6] CodingGuidelines: describe "export VAR=VAL" rule

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 6, 2024, 05:11 UTC
Message-ID: <CAPig+cRjqe-rgYf5UZr9KXmfSw98ZoYjPo5PKhwzRaC-svwshA@mail.gmail.com>
In-Reply-To: <20240406000902.3082301-2-gitster@pobox.com>

On Fri, Apr 5, 2024 at 8:09 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

> https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/
> resulted in 9968ffff (test-lint: detect 'export FOO=bar',
> 2013-07-08) to add a rule to t/check-non-portable-shell.pl script to
> reject
>
>         export VAR=VAL
>
> and suggest us to instead write it as "export VAR" followed by
> "VAR=VAL".  This however was not spelled out in the CodingGuidelines
> document.

I suspect you meant:

   ... and suggest us to instead write it as "VAR=VAL" followed by
   "export VAR".

Show 13 quoted lines

> We may want to re-evaluate the rule since it is from ages ago, but
> for now, let's make the written rule and what the automation enforces
> consistent.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> @@ -188,6 +188,12 @@ For shell scripts specifically (not exhaustive):
>     hopefully nobody starts using "local" before they are reimplemented
>     in C ;-)
>
> + - Some versions of shell do not understand "export variable=value",
> +   so we write "export variable" and "variable=value" on separae

s/separae/separate/

Here too, it might be clearer to swap around the pieces:

    ... so we write "variable=value" and "export variable" on...

> +   lines.  Note that this was reported in 2013 and the situation might
> +   have changed since then.  We'd need to re-evaluate this rule,
> +   together with the rule in t/check-non-portable-shell.pl script.

The bit starting at "Note that..." seems more appropriate for the commit message (which is already the case) or a To-Do list. People reading this document are likely newcomers looking for concrete instructions about how to code for this project, and this sort of To-Do item isn't going to help them. (If anything, it might confuse them into ignoring the advice to split `export foo=bar` into two statements, which will result in reviewers asking them to reroll.)