threads / patch / 61824

v2CodingGuidelines: document a shell that "fails" "VAR=VAL shell_func"

Subject: [PATCH v2] CodingGuidelines: document a shell that "fails" "VAR=VAL shell_func"

## tl;dr

7 messages between Jul 22, 2024 and Jul 23, 2024. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Junio C Hamano· Jul 22, 2024, 23:10 UTC · lore

Over the years, we accumulated the community wisdom to avoid the common "one-short export" construct for shell functions, but seem to have lost on which exact platform it is known to fail. Now during an investigation on a breakage for a recent topic, we found one example of failing shell. Let's document that.

This does *not* mean that we can freely start using the construct once Ubuntu 20.04 is retired. But it does mean that we cannot use the construct until Ubuntu 20.04 is fully retired from the machines that matter.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
Show changes to Documentation/CodingGuidelines +24 −0
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 1d92b2da03..203ef49364 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -204,6 +204,30 @@ For shell scripts specifically (not exhaustive):
 	local variable="$value"
 	local variable="$(command args)"
 
+ - The common construct
+
+	VAR=VAL command args
+
+   to temporarily set and export environment variable VAR only while
+   "command args" is running is handy, but some versions of dash (like
+   0.5.10.2-6 found on Ubuntu 20.04) makes a temporary assignment
+   without exporting the variable, when command is *not* an external
+   command.  Do not use it for shell functions.  A common workaround
+   is to do an explicit export in a subshell, like so:
+
+	(incorrect)
+	VAR=VAL func args
+
+	(correct)
+	(
+		VAR=VAL &&
+		export VAR &&
+		func args
+	)
+
+   but be careful that the effect "func" makes to the variables in the
+   current shell will be lost across the subshell boundary.
+
  - Use octal escape sequences (e.g. "\302\242"), not hexadecimal (e.g.
    "\xc2\xa2") in printf format strings, since hexadecimal escape
    sequences are not portable.

Range-diff:
1:  78a3847e80 ! 1:  75d07c05c7 CodingGuidelines: give an example shell that "fails" "VAR=VAL shell_func"
    @@ Metadata
     Author: Junio C Hamano <gitster@pobox.com>
     
      ## Commit message ##
    -    CodingGuidelines: give an example shell that "fails" "VAR=VAL shell_func"
    +    CodingGuidelines: document a shell that "fails" "VAR=VAL shell_func"
     
         Over the years, we accumulated the community wisdom to avoid the
         common "one-short export" construct for shell functions, but seem to
         have lost on which exact platform it is known to fail.  Now during
    -    an investigation on a breakage for a recent topic, let's document
    -    one example of failing shell.
    +    an investigation on a breakage for a recent topic, we found one
    +    example of failing shell.  Let's document that.
     
         This does *not* mean that we can freely start using the construct
         once Ubuntu 20.04 is retired.  But it does mean that we cannot use
    @@ Documentation/CodingGuidelines: For shell scripts specifically (not exhaustive):
     +
     +	(correct)
     +	(
    -+		VAR=VAL && export VAR &&
    ++		VAR=VAL &&
    ++		export VAR &&
     +		func args
     +	)
     +
-- 
2.46.0-rc1-52-g816ffef0a1
Junio C Hamano· Jul 23, 2024, 00:04 UTC · re: Junio C Hamano · lore

[PATCH v3] CodingGuidelines: document a shell that "fails" "VAR=VAL shell_func"

Over the years, we accumulated the community wisdom to avoid the common "one-short export" construct for shell functions, but seem to have lost on which exact platform it is known to fail. Now during an investigation on a breakage for a recent topic, we found one example of failing shell. Let's document that.

This does *not* mean that we can freely start using the construct once Ubuntu 20.04 is retired. But it does mean that we cannot use the construct until Ubuntu 20.04 is fully retired from the machines that matter. Moreover, posix explicitly says that the behaviour for the construct is unspecified.

Helped-by: Kyle Lippincott <spectral@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
Show changes to Documentation/CodingGuidelines +27 −0
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 1d92b2da03..ad71c26152 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -204,6 +204,33 @@ For shell scripts specifically (not exhaustive):
 	local variable="$value"
 	local variable="$(command args)"
 
+ - The common construct
+
+	VAR=VAL command args
+
+   to temporarily set and export environment variable VAR only while
+   "command args" is running is handy, but this triggers an
+   unspecified behaviour accoreding to POSIX when used for a command
+   that is not an external command (like shell functions).  Indeed,
+   some versions of dash (like 0.5.10.2-6 found on Ubuntu 20.04) and
+   AT&T ksh do make a temporary assignment without exporting the
+   variable, in such a case.  Do not use it for shell functions.  A
+   common workaround is to do an explicit export in a subshell, like
+   so:
+
+	(incorrect)
+	VAR=VAL func args
+
+	(correct)
+	(
+		VAR=VAL &&
+		export VAR &&
+		func args
+	)
+
+   but be careful that the effect "func" makes to the variables in the
+   current shell will be lost across the subshell boundary.
+
  - Use octal escape sequences (e.g. "\302\242"), not hexadecimal (e.g.
    "\xc2\xa2") in printf format strings, since hexadecimal escape
    sequences are not portable.

Range-diff:
1:  75d07c05c7 ! 1:  8462cbb740 CodingGuidelines: document a shell that "fails" "VAR=VAL shell_func"
    @@ Commit message
         This does *not* mean that we can freely start using the construct
         once Ubuntu 20.04 is retired.  But it does mean that we cannot use
         the construct until Ubuntu 20.04 is fully retired from the machines
    -    that matter.
    +    that matter.  Moreover, posix explicitly says that the behaviour for
    +    the construct is unspecified.
     
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    @@ Documentation/CodingGuidelines: For shell scripts specifically (not exhaustive):
     +	VAR=VAL command args
     +
     +   to temporarily set and export environment variable VAR only while
    -+   "command args" is running is handy, but some versions of dash (like
    -+   0.5.10.2-6 found on Ubuntu 20.04) makes a temporary assignment
    -+   without exporting the variable, when command is *not* an external
    -+   command.  Do not use it for shell functions.  A common workaround
    -+   is to do an explicit export in a subshell, like so:
    ++   "command args" is running is handy, but this triggers an
    ++   unspecified behaviour accoreding to POSIX when used for a command
    ++   that is not an external command (like shell functions).  Indeed,
    ++   some versions of dash (like 0.5.10.2-6 found on Ubuntu 20.04) and
    ++   AT&T ksh do make a temporary assignment without exporting the
    ++   variable, in such a case.  Do not use it for shell functions.  A
    ++   common workaround is to do an explicit export in a subshell, like
    ++   so:
     +
     +	(incorrect)
     +	VAR=VAL func args
-- 
2.46.0-rc1-52-g816ffef0a1
Eric Sunshine· Jul 23, 2024, 00:10 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] CodingGuidelines: document a shell that "fails" "VAR=VAL shell_func"

On Mon, Jul 22, 2024 at 8:05 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> Over the years, we accumulated the community wisdom to avoid the
> common "one-short export" construct for shell functions, but seem to
> have lost on which exact platform it is known to fail.  Now during
> an investigation on a breakage for a recent topic, we found one
> example of failing shell.  Let's document that.
> [...]
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> @@ -204,6 +204,33 @@ For shell scripts specifically (not exhaustive):
> + - The common construct
> +
> +       VAR=VAL command args
> +
> +   to temporarily set and export environment variable VAR only while
> +   "command args" is running is handy, but this triggers an
> +   unspecified behaviour accoreding to POSIX when used for a command
s/accoreding/according/
Show 19 quoted lines
> +   that is not an external command (like shell functions).  Indeed,
> +   some versions of dash (like 0.5.10.2-6 found on Ubuntu 20.04) and
> +   AT&T ksh do make a temporary assignment without exporting the
> +   variable, in such a case.  Do not use it for shell functions.  A
> +   common workaround is to do an explicit export in a subshell, like
> +   so:
> +
> +       (incorrect)
> +       VAR=VAL func args
> +
> +       (correct)
> +       (
> +               VAR=VAL &&
> +               export VAR &&
> +               func args
> +       )
> +
> +   but be careful that the effect "func" makes to the variables in the
> +   current shell will be lost across the subshell boundary.
Junio C Hamano· Jul 23, 2024, 00:23 UTC · re: Eric Sunshine · lore

Re: [PATCH v3] CodingGuidelines: document a shell that "fails" "VAR=VAL shell_func"

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 5 quoted lines
>> +   to temporarily set and export environment variable VAR only while
>> +   "command args" is running is handy, but this triggers an
>> +   unspecified behaviour accoreding to POSIX when used for a command
>
> s/accoreding/according/
Thanks.
Jeff King· Jul 23, 2024, 03:34 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] CodingGuidelines: document a shell that "fails" "VAR=VAL shell_func"

On Mon, Jul 22, 2024 at 04:10:41PM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> Over the years, we accumulated the community wisdom to avoid the
> common "one-short export" construct for shell functions, but seem to
> have lost on which exact platform it is known to fail.  Now during
> an investigation on a breakage for a recent topic, we found one
> example of failing shell.  Let's document that.

My recollection was that FreeBSD's /bin/sh was the culprit, but I couldn't find any mention digging in the archive. However, I just checked on a FreeBSD 13 VM, and it does have the same problem (that the one-shot variable is not exported). I don't think that changes anything for your patch, but just reinforces this part:

> This does *not* mean that we can freely start using the construct
> once Ubuntu 20.04 is retired.  But it does mean that we cannot use
> the construct until Ubuntu 20.04 is fully retired from the machines
> that matter.
since now we have one other instance.

I thought it also had the issue that the variable would remain set in the caller after the function returned, but it does not seem to do so now (if it ever did).

-Peff
Junio C Hamano· Jul 23, 2024, 15:28 UTC · re: Jeff King · lore

Re: [PATCH v2] CodingGuidelines: document a shell that "fails" "VAR=VAL shell_func"

Jeff King <peff@peff.net> writes:
Show 24 quoted lines
> On Mon, Jul 22, 2024 at 04:10:41PM -0700, Junio C Hamano wrote:
>
>> Over the years, we accumulated the community wisdom to avoid the
>> common "one-short export" construct for shell functions, but seem to
>> have lost on which exact platform it is known to fail.  Now during
>> an investigation on a breakage for a recent topic, we found one
>> example of failing shell.  Let's document that.
>
> My recollection was that FreeBSD's /bin/sh was the culprit, but I
> couldn't find any mention digging in the archive. However, I just
> checked on a FreeBSD 13 VM, and it does have the same problem (that the
> one-shot variable is not exported). I don't think that changes anything
> for your patch, but just reinforces this part:
>
>> This does *not* mean that we can freely start using the construct
>> once Ubuntu 20.04 is retired.  But it does mean that we cannot use
>> the construct until Ubuntu 20.04 is fully retired from the machines
>> that matter.
>
> since now we have one other instance.
>
> I thought it also had the issue that the variable would remain set in
> the caller after the function returned, but it does not seem to do so
> now (if it ever did).

Yeah, that one is also what POSIX leaves to the implementation, if I recall what I read there.

So here is how the part looks like in my tree right now.  
Thanks.
Show changes to diff +5 −5
diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
index 2151ec51b8..52afb2725f 100644
--- c/Documentation/CodingGuidelines
+++ w/Documentation/CodingGuidelines
@@ -212,11 +212,11 @@ For shell scripts specifically (not exhaustive):
    "command args" is running is handy, but this triggers an
    unspecified behaviour according to POSIX when used for a command
    that is not an external command (like shell functions).  Indeed,
-   some versions of dash (like 0.5.10.2-6 found on Ubuntu 20.04) and
-   AT&T ksh do make a temporary assignment without exporting the
-   variable, in such a case.  Do not use it for shell functions.  A
-   common workaround is to do an explicit export in a subshell, like
-   so:
+   dash 0.5.10.2-6 on Ubuntu 20.04, /bin/sh on FreeBSD 13, and AT&T
+   ksh all make a temporary assignment without exporting the variable,
+   in such a case.  As it does not work portably across shells, do not
+   use this syntax for shell functions.  A common workaround is to do
+   an explicit export in a subshell, like so:
 
 	(incorrect)
 	VAR=VAL func args
Rubén Justo· Jul 23, 2024, 21:55 UTC · re: Jeff King · lore

Re: [PATCH v2] CodingGuidelines: document a shell that "fails" "VAR=VAL shell_func"

On Mon, Jul 22, 2024 at 11:34:18PM -0400, Jeff King wrote:
Show 18 quoted lines
> On Mon, Jul 22, 2024 at 04:10:41PM -0700, Junio C Hamano wrote:
> 
> > Over the years, we accumulated the community wisdom to avoid the
> > common "one-short export" construct for shell functions, but seem to
> > have lost on which exact platform it is known to fail.  Now during
> > an investigation on a breakage for a recent topic, we found one
> > example of failing shell.  Let's document that.
> 
> My recollection was that FreeBSD's /bin/sh was the culprit, but I
> couldn't find any mention digging in the archive. However, I just
> checked on a FreeBSD 13 VM, and it does have the same problem (that the
> one-shot variable is not exported). I don't think that changes anything
> for your patch, but just reinforces this part:
> 
> > This does *not* mean that we can freely start using the construct
> > once Ubuntu 20.04 is retired.  But it does mean that we cannot use
> > the construct until Ubuntu 20.04 is fully retired from the machines
> > that matter.

And adding more confusion, which further reinforces the need to avoid the construct and the usefulness of Eric's change [1], it sometimes appears to function differently:

   ** dash 0.5.10.2-6 **
   $ f() { echo $A; bash -c 'echo $A'; }
   $ A=2
   $ A=1 f
   1
   
   $ export A
   $ f
   2
   2
   $ A=1 f
   1
   1
 1.- https://lore.kernel.org/git/2e1c8fc6-86f0-404f-bef6-9502aa0d31d0@gmail.com/T/#m3470fc6c1df59d29312bbe3de0444f1f608f3611
Show 8 quoted lines
> 
> since now we have one other instance.
> 
> I thought it also had the issue that the variable would remain set in
> the caller after the function returned, but it does not seem to do so
> now (if it ever did).
> 
> -Peff

← back to recent threads