[PATCH 0/4] improve one-shot variable detection with shell function

[PATCH 0/4] improve one-shot variable detection with shell function

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

This series addresses a blind-spot of check-non-portable-shell's
detection of one-shot environment variable assignment with shell
functions. In particular, although it correctly detects:

    VAR=val shell-func

it will miss invocations such as:

    echo X | VAR=val shell-func

References:
https://lore.kernel.org/git/CAPig+cRyj8J7MZEufu34NUzwOL2n=w35nT1Ug7FGRwMC0=Qpwg@mail.gmail.com/
https://lore.kernel.org/git/bc1b9cce-d04d-4a79-8fab-55ec3c8bae30@gmail.com/

Eric Sunshine (4):
  t3430: modernize one-shot "VAR=val shell-func" invocation
  t4034: fix use of one-shot variable assignment with shell function
  check-non-portable-shell: improve `VAR=val shell-func` detection
  check-non-portable-shell: suggest alternative for `VAR=val shell-func`

 t/check-non-portable-shell.pl | 4 ++--
 t/t3430-rebase-merges.sh      | 4 ++--
 t/t4034-diff-words.sh         | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.45.2

[PATCH 1/4] t3430: modernize one-shot "VAR=val shell-func" invocation

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

Unlike "VAR=val cmd" one-shot environment variable assignments which
exist only for the invocation of 'cmd', those assigned by "VAR=val
shell-func" exist within the running shell and continue to do so until
the process exits (or are explicitly unset). check-non-portable-shell.pl
warns when it detects such usage since, more often than not, the author
who writes such an invocation is unaware of the undesirable behavior.

A common way to work around the problem is to wrap a subshell around the
variable assignments and function call, thus ensuring that the
assignments are short-lived. However, these days, a more ergonomic
approach is to employ test_env() which is tailor-made for this specific
use-case.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t3430-rebase-merges.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 36ca126bcd..e851ede4f9 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -392,8 +392,8 @@ test_expect_success 'refuse to merge ancestors of HEAD' '
 
 test_expect_success 'root commits' '
 	git checkout --orphan unrelated &&
-	(GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
-	 test_commit second-root) &&
+	test_env GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
+		test_commit second-root &&
 	test_commit third-root &&
 	cat >script-from-scratch <<-\EOF &&
 	pick third-root
-- 
2.45.2

[PATCH 2/4] t4034: fix use of one-shot variable assignment with shell function

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

Unlike "VAR=val cmd" one-shot environment variable assignments which
exist only for the invocation of 'cmd', those assigned by "VAR=val
shell-func" exist within the running shell and continue to do so until
the process exits (or are explicitly unset). In most cases, it is
unlikely that this behavior was intended by the test author, and, even
if those leaked assignments do not impact other tests today, they can
negatively impact tests added later by authors unaware that the variable
assignments are still hanging around. Address this shortcoming by
ensuring that the assignments are short-lived.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4034-diff-words.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 74586f3813..4dcd7e9925 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -70,7 +70,7 @@ test_language_driver () {
 		word_diff --color-words
 	'
 	test_expect_success "diff driver '$lang' in Islandic" '
-		LANG=is_IS.UTF-8 LANGUAGE=is LC_ALL="$is_IS_locale" \
+		test_env LANG=is_IS.UTF-8 LANGUAGE=is LC_ALL="$is_IS_locale" \
 		word_diff --color-words
 	'
 }
-- 
2.45.2

[PATCH 3/4] check-non-portable-shell: improve `VAR=val shell-func` detection

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

Unlike "VAR=val cmd" one-shot environment variable assignments which
exist only for the invocation of 'cmd', those assigned by "VAR=val
shell-func" exist within the running shell and continue to do so until
the process exits. check-non-portable-shell.pl warns when it detects
such usage since, more often than not, the author who writes such an
invocation is unaware of the undesirable behavior.

However, a limitation of the check is that it only detects such
invocations when variable assignment (i.e. `VAR=val`) is the first
thing on the line. Thus, it can easily be fooled by an invocation such
as:

    echo X | VAR=val shell-func

Address this shortcoming by loosening the check so that the variable
assignment can be recognized even when not at the beginning of the line.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/check-non-portable-shell.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b2b28c2ced..44b23d6ddd 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -49,7 +49,7 @@ sub err {
 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
 	/\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
 		err q(quote "$val" in 'local var=$val');
-	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
+	/\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and
 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
 	$line = '';
 	# this resets our $. for each file
-- 
2.45.2

[PATCH 4/4] check-non-portable-shell: suggest alternative for `VAR=val shell-func`

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

Most problems reported by check-non-portable-shell are accompanied by
advice suggesting how the test author can repair the problem. For
instance:

    error: egrep/fgrep obsolescent (use grep -E/-F)

However, when one-shot variable assignment is detected when calling a
shell function (i.e. `VAR=val shell-func`), the problem is reported, but
no advice is given. The lack of advice is particularly egregious since
neither the problem nor the workaround are likely well-known by
newcomers to the project writing tests for the first time. Address this
shortcoming by recommending the use of test_env() which is tailor made
for this specific use-case.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/check-non-portable-shell.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 44b23d6ddd..56db7cc6ed 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -50,7 +50,7 @@ sub err {
 	/\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
 		err q(quote "$val" in 'local var=$val');
 	/\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and
-		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
+		err '"FOO=bar shell_func" assignment extends beyond "shell_func" (use test_env FOO=bar shell_func)';
 	$line = '';
 	# this resets our $. for each file
 	close ARGV if eof;
-- 
2.45.2

Re: [PATCH 3/4] check-non-portable-shell: improve `VAR=val shell-func` detection

[Rubén Justo]

On Mon, Jul 22, 2024 at 02:59:13AM -0400, Eric Sunshine wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
> 
> Unlike "VAR=val cmd" one-shot environment variable assignments which
> exist only for the invocation of 'cmd', those assigned by "VAR=val
> shell-func" exist within the running shell and continue to do so until
> the process exits. check-non-portable-shell.pl warns when it detects
> such usage since, more often than not, the author who writes such an
> invocation is unaware of the undesirable behavior.
> 
> However, a limitation of the check is that it only detects such
> invocations when variable assignment (i.e. `VAR=val`) is the first
> thing on the line. Thus, it can easily be fooled by an invocation such
> as:
> 
>     echo X | VAR=val shell-func
> 
> Address this shortcoming by loosening the check so that the variable
> assignment can be recognized even when not at the beginning of the line.
> 
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/check-non-portable-shell.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
> index b2b28c2ced..44b23d6ddd 100755
> --- a/t/check-non-portable-shell.pl
> +++ b/t/check-non-portable-shell.pl
> @@ -49,7 +49,7 @@ sub err {
>  	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
>  	/\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
>  		err q(quote "$val" in 'local var=$val');
> -	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
> +	/\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and

Losing "^\s*" means we'll cause false positives, such as:

    # VAR=VAL shell-func
    echo VAR=VAL shell-func

Regardless of that, the regex will continue to pose problems with:

  VAR=$OTHER_VALUE shell-func
  VAR=$(cmd) shell-func
  VAR=VAL\ UE shell-func
  VAR="\"val\" shell-func UE" non-shell-func 

Which, of course, should be cases that should be written in a more
orthodox way. 

But we will start to detect errors like the ones mentioned in the
message, which are more likely to happen.

I think this change is a good step forward, and I'm happy with it as it
is.

Thanks

>  		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
>  	$line = '';
>  	# this resets our $. for each file
> -- 
> 2.45.2
>  

Re: [PATCH 4/4] check-non-portable-shell: suggest alternative for `VAR=val shell-func`

[Rubén Justo]

On 7/22/24 8:59 AM, Eric Sunshine wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
> 
> Most problems reported by check-non-portable-shell are accompanied by
> advice suggesting how the test author can repair the problem. For
> instance:
> 
>     error: egrep/fgrep obsolescent (use grep -E/-F)
> 
> However, when one-shot variable assignment is detected when calling a
> shell function (i.e. `VAR=val shell-func`), the problem is reported, but
> no advice is given. The lack of advice is particularly egregious since
> neither the problem nor the workaround are likely well-known by
> newcomers to the project writing tests for the first time. Address this
> shortcoming by recommending the use of test_env() which is tailor made
> for this specific use-case.
> 
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/check-non-portable-shell.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
> index 44b23d6ddd..56db7cc6ed 100755
> --- a/t/check-non-portable-shell.pl
> +++ b/t/check-non-portable-shell.pl
> @@ -50,7 +50,7 @@ sub err {
>  	/\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
>  		err q(quote "$val" in 'local var=$val');
>  	/\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and
> -		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
> +		err '"FOO=bar shell_func" assignment extends beyond "shell_func" (use test_env FOO=bar shell_func)';

This is a nice improvement.

>  	$line = '';
>  	# this resets our $. for each file
>  	close ARGV if eof;

Re: [PATCH 0/4] improve one-shot variable detection with shell function

[Rubén Justo]

On Mon, Jul 22, 2024 at 02:59:10AM -0400, Eric Sunshine wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
> 
> This series addresses a blind-spot of check-non-portable-shell's
> detection of one-shot environment variable assignment with shell
> functions. In particular, although it correctly detects:
> 
>     VAR=val shell-func
> 
> it will miss invocations such as:
> 
>     echo X | VAR=val shell-func
> 
> References:
> https://lore.kernel.org/git/CAPig+cRyj8J7MZEufu34NUzwOL2n=w35nT1Ug7FGRwMC0=Qpwg@mail.gmail.com/
> https://lore.kernel.org/git/bc1b9cce-d04d-4a79-8fab-55ec3c8bae30@gmail.com/
> 
> Eric Sunshine (4):
>   t3430: modernize one-shot "VAR=val shell-func" invocation
>   t4034: fix use of one-shot variable assignment with shell function
>   check-non-portable-shell: improve `VAR=val shell-func` detection
>   check-non-portable-shell: suggest alternative for `VAR=val shell-func`

All these changes look good to me.

Thanks.

> 
>  t/check-non-portable-shell.pl | 4 ++--
>  t/t3430-rebase-merges.sh      | 4 ++--
>  t/t4034-diff-words.sh         | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> -- 
> 2.45.2
> 

Re: [PATCH 1/4] t3430: modernize one-shot "VAR=val shell-func" invocation

[Phillip Wood]

Hi Eric

On 22/07/2024 07:59, Eric Sunshine wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
> 
> Unlike "VAR=val cmd" one-shot environment variable assignments which
> exist only for the invocation of 'cmd', those assigned by "VAR=val
> shell-func" exist within the running shell and continue to do so until
> the process exits (or are explicitly unset).

I'm not sure I follow. If I run

sh -c 'f() {
     echo "f: HELLO=$HELLO"
     env | grep HELLO
}
HELLO=x f; echo "HELLO=$HELLO"'

Then I see

f: HELLO=x
HELLO=x
HELLO=

which seems to contradict the commit message as $HELLO is unset when the 
function returns. I see the same result if I replace "sh" (which is bash 
on my system) with an explicit "bash", "dash" or "zsh".

I'm also confused as to why this caused a problem for Rubén's test as 
$HELLO is set in the environment so I'm don't understand why git wasn't 
picking up the right pager.

> check-non-portable-shell.pl
> warns when it detects such usage since, more often than not, the author
> who writes such an invocation is unaware of the undesirable behavior.
> 
> A common way to work around the problem is to wrap a subshell around the
> variable assignments and function call, thus ensuring that the
> assignments are short-lived. However, these days, a more ergonomic
> approach is to employ test_env() which is tailor-made for this specific
> use-case.

Oh, that sounds useful, I didn't know it existed.

Best Wishes

Phillip

> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>   t/t3430-rebase-merges.sh | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index 36ca126bcd..e851ede4f9 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -392,8 +392,8 @@ test_expect_success 'refuse to merge ancestors of HEAD' '
>   
>   test_expect_success 'root commits' '
>   	git checkout --orphan unrelated &&
> -	(GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
> -	 test_commit second-root) &&
> +	test_env GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
> +		test_commit second-root &&
>   	test_commit third-root &&
>   	cat >script-from-scratch <<-\EOF &&
>   	pick third-root

Re: [PATCH 3/4] check-non-portable-shell: improve `VAR=val shell-func` detection

[Kyle Lippincott]

On Mon, Jul 22, 2024 at 12:01 AM Eric Sunshine <ericsunshine@charter.net> wrote:
>
> From: Eric Sunshine <sunshine@sunshineco.com>
>
> Unlike "VAR=val cmd" one-shot environment variable assignments which
> exist only for the invocation of 'cmd', those assigned by "VAR=val
> shell-func" exist within the running shell and continue to do so until
> the process exits. check-non-portable-shell.pl warns when it detects
> such usage since, more often than not, the author who writes such an
> invocation is unaware of the undesirable behavior.
>
> However, a limitation of the check is that it only detects such
> invocations when variable assignment (i.e. `VAR=val`) is the first
> thing on the line. Thus, it can easily be fooled by an invocation such
> as:
>
>     echo X | VAR=val shell-func
>
> Address this shortcoming by loosening the check so that the variable
> assignment can be recognized even when not at the beginning of the line.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/check-non-portable-shell.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
> index b2b28c2ced..44b23d6ddd 100755
> --- a/t/check-non-portable-shell.pl
> +++ b/t/check-non-portable-shell.pl
> @@ -49,7 +49,7 @@ sub err {
>         /\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
>         /\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
>                 err q(quote "$val" in 'local var=$val');
> -       /^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
> +       /\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and
>                 err '"FOO=bar shell_func" assignment extends beyond "shell_func"';

Is there an example of a shell on Linux that has this behavior that I
can observe, and/or reproduction steps? `bash --posix` does not do
this, as far as I can tell. I tried:

```
bash --posix
echo_hi() { echo hi; }
FOO=BAR echo_hi
echo $FOO
<no output>
```

and the simpler:

```
bash --posix
FOO=BAR echo hi
echo $FOO
```

Both attempts were done interactively, not via a script.

I'm asking mostly because of the recent "platform support document"
patch series - this is a very surprising behavior (which is presumably
why it was added to this test), and it's possible this was just a bug
in a shell that isn't really used anymore. Having documentation on how
to reproduce the issue lets us know when we can remove these kinds of
restrictions on our codebase that we took in the name of
compatibility, possibly over a decade ago.

>         $line = '';
>         # this resets our $. for each file
> --
> 2.45.2
>
>

Re: [PATCH 3/4] check-non-portable-shell: improve `VAR=val shell-func` detection

[Junio C Hamano]

Kyle Lippincott <spectral@google.com> writes:

> Is there an example of a shell on Linux that has this behavior that I
> can observe, and/or reproduction steps?

Every once in a while this comes up and we fix, e.g.

https://lore.kernel.org/git/528CE716.8060307@ramsay1.demon.co.uk/
https://lore.kernel.org/git/c6efda03848abc00cf8bf8d84fc34ef0d652b64c.1264151435.git.mhagger@alum.mit.edu/
https://lore.kernel.org/git/Koa4iojOlOQ_YENPwWXKt7G8Aa1x6UaBnFFtliKdZmpcrrqOBhY7NQ@cipher.nrlssc.navy.mil/
https://lore.kernel.org/git/20180713055205.32351-2-sunshine@sunshineco.com/
https://lore.kernel.org/git/574E27A4.6040804@ramsayjones.plus.com/

which is from a query

    https://lore.kernel.org/git/?q=one-shot+export+shell+function

but unfortunately we do not document which exact shell the observed
breakage happened with.

The closest article I found that is suitable as a discussion
reignitor talks about what POSIX requires, which may be more
relevant:

  https://lore.kernel.org/git/4B5027B8.2090507@viscovery.net/

Re: [PATCH 1/4] t3430: modernize one-shot "VAR=val shell-func" invocation

[Junio C Hamano]

Eric Sunshine <ericsunshine@charter.net> writes:

> A common way to work around the problem is to wrap a subshell around the
> variable assignments and function call, thus ensuring that the
> assignments are short-lived. However, these days, a more ergonomic
> approach is to employ test_env() which is tailor-made for this specific
> use-case.

OK.  I am not sure if that is "ergonomic", though.  An explict
subshell has a good documentation value that even though we call
test_commit there, we do not care about the committer timestamps
subsequent commits would record, and we do not mind losing the
effect of test_tick from this invocation of test_commit.  Hiding
all of that behind test_env loses the documentation value.

We could resurrect it by explicitly passing "--no-tick" to
test_commit, though ;-).

> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/t3430-rebase-merges.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index 36ca126bcd..e851ede4f9 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -392,8 +392,8 @@ test_expect_success 'refuse to merge ancestors of HEAD' '
>  
>  test_expect_success 'root commits' '
>  	git checkout --orphan unrelated &&
> -	(GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
> -	 test_commit second-root) &&
> +	test_env GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
> +		test_commit second-root &&
>  	test_commit third-root &&
>  	cat >script-from-scratch <<-\EOF &&
>  	pick third-root

Re: [PATCH 3/4] check-non-portable-shell: improve `VAR=val shell-func` detection

[Kyle Lippincott]

On Mon, Jul 22, 2024 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Kyle Lippincott <spectral@google.com> writes:
>
> > Is there an example of a shell on Linux that has this behavior that I
> > can observe, and/or reproduction steps?
>
> Every once in a while this comes up and we fix, e.g.
>
> https://lore.kernel.org/git/528CE716.8060307@ramsay1.demon.co.uk/
> https://lore.kernel.org/git/c6efda03848abc00cf8bf8d84fc34ef0d652b64c.1264151435.git.mhagger@alum.mit.edu/
> https://lore.kernel.org/git/Koa4iojOlOQ_YENPwWXKt7G8Aa1x6UaBnFFtliKdZmpcrrqOBhY7NQ@cipher.nrlssc.navy.mil/
> https://lore.kernel.org/git/20180713055205.32351-2-sunshine@sunshineco.com/

Thanks, this one leads to
https://lore.kernel.org/git/20180713055205.32351-1-sunshine@sunshineco.com/,
which references
https://public-inbox.org/git/xmqqefg8w73c.fsf@gitster-ct.c.googlers.com/T/,
which claims that `dash` has this behavior 6 years ago. The version of
`dash` I have on my machine right now doesn't seem to have this issue,
but I can believe some older version does.

> https://lore.kernel.org/git/574E27A4.6040804@ramsayjones.plus.com/
>
> which is from a query
>
>     https://lore.kernel.org/git/?q=one-shot+export+shell+function
>
> but unfortunately we do not document which exact shell the observed
> breakage happened with.
>
> The closest article I found that is suitable as a discussion
> reignitor talks about what POSIX requires, which may be more
> relevant:
>
>   https://lore.kernel.org/git/4B5027B8.2090507@viscovery.net/

This claims that `ksh` "gets it right", and I can confirm that ksh
does behave this way on my Linux machine.

Having just looked at the POSIX standard (I don't think I'm allowed to
copy from this document), the POSIX standard (POSIX.1-2024, at least)
explicitly leaves it unspecified whether the variable assignments
remain in effect after function execution.

Thanks for indulging my curiosity; should we include a statement in
the linter along the lines of `# POSIX.1-2024 explicitly does not
specify if variable assignment persists after executing a shell
function; some shells, such as ksh, have these variables remain.`?

Re: [PATCH 3/4] check-non-portable-shell: improve `VAR=val shell-func` detection

[Junio C Hamano]

Kyle Lippincott <spectral@google.com> writes:

> Having just looked at the POSIX standard (I don't think I'm allowed to
> copy from this document), the POSIX standard (POSIX.1-2024, at least)
> explicitly leaves it unspecified whether the variable assignments
> remain in effect after function execution.

True.

https://pubs.opengroup.org/onlinepubs/9799919799/utilities/V3_chap02.html#tag_19_09_01_02

also says that it is unspecified if the variable gets exported, and
older version of dash that comes on Ubuntu 20.04 chooses *not* to
export, which was the test breakage that triggered this whole
discussion.  The thread can be seen here:

https://lore.kernel.org/git/xmqqbk2p9lwi.fsf_-_@gitster.g/

> Thanks for indulging my curiosity; should we include a statement in
> the linter along the lines of `# POSIX.1-2024 explicitly does not
> specify if variable assignment persists after executing a shell
> function; some shells, such as ksh, have these variables remain.`?

Giving a review (either positive or negative is fine, as long as it
is constructive) on the update to CodingGuidelines

  https://lore.kernel.org/git/xmqqbk2p9lwi.fsf_-_@gitster.g/

may be a good place to start.

Thanks.

Re: [PATCH 1/4] t3430: modernize one-shot "VAR=val shell-func" invocation

[Phillip Wood]

On 22/07/2024 16:09, Phillip Wood wrote:
> Hi Eric
> 
> On 22/07/2024 07:59, Eric Sunshine wrote:
>> From: Eric Sunshine <sunshine@sunshineco.com>
>>
>> Unlike "VAR=val cmd" one-shot environment variable assignments which
>> exist only for the invocation of 'cmd', those assigned by "VAR=val
>> shell-func" exist within the running shell and continue to do so until
>> the process exits (or are explicitly unset).

Having seen the parallel discussion about the behavior of hash this 
construct is non-portable because the behavior differs between shells so 
perhaps the commit message could say something like

Unlike "VAR=val cmd" one-shot environment variable assignments which 
only exist for the invocation of external command "cmd", the behavior of 
"VAR=val func" where "func" is a shell function or builtin command 
varies between shells and so we should not use it in our test suite.

Best Wishes

Phillip

> I'm not sure I follow. If I run
> 
> sh -c 'f() {
>      echo "f: HELLO=$HELLO"
>      env | grep HELLO
> }
> HELLO=x f; echo "HELLO=$HELLO"'
> 
> Then I see
> 
> f: HELLO=x
> HELLO=x
> HELLO=
> 
> which seems to contradict the commit message as $HELLO is unset when the 
> function returns. I see the same result if I replace "sh" (which is bash 
> on my system) with an explicit "bash", "dash" or "zsh".
> 
> I'm also confused as to why this caused a problem for Rubén's test as 
> $HELLO is set in the environment so I'm don't understand why git wasn't 
> picking up the right pager.
> 
>> check-non-portable-shell.pl
>> warns when it detects such usage since, more often than not, the author
>> who writes such an invocation is unaware of the undesirable behavior.
>>
>> A common way to work around the problem is to wrap a subshell around the
>> variable assignments and function call, thus ensuring that the
>> assignments are short-lived. However, these days, a more ergonomic
>> approach is to employ test_env() which is tailor-made for this specific
>> use-case.
> 
> Oh, that sounds useful, I didn't know it existed.
> 
> Best Wishes
> 
> Phillip
> 
>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>> ---
>>   t/t3430-rebase-merges.sh | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
>> index 36ca126bcd..e851ede4f9 100755
>> --- a/t/t3430-rebase-merges.sh
>> +++ b/t/t3430-rebase-merges.sh
>> @@ -392,8 +392,8 @@ test_expect_success 'refuse to merge ancestors of 
>> HEAD' '
>>   test_expect_success 'root commits' '
>>       git checkout --orphan unrelated &&
>> -    (GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
>> -     test_commit second-root) &&
>> +    test_env GIT_AUTHOR_NAME="Parsnip" 
>> GIT_AUTHOR_EMAIL="root@example.com" \
>> +        test_commit second-root &&
>>       test_commit third-root &&
>>       cat >script-from-scratch <<-\EOF &&
>>       pick third-root

Re: [PATCH 1/4] t3430: modernize one-shot "VAR=val shell-func" invocation

[Eric Sunshine]

On Mon, Jul 22, 2024 at 11:10 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 22/07/2024 07:59, Eric Sunshine wrote:
> > Unlike "VAR=val cmd" one-shot environment variable assignments which
> > exist only for the invocation of 'cmd', those assigned by "VAR=val
> > shell-func" exist within the running shell and continue to do so until
> > the process exits (or are explicitly unset).
>
> I'm not sure I follow. If I run
>
> sh -c 'f() {
>      echo "f: HELLO=$HELLO"
>      env | grep HELLO
> }
> HELLO=x f; echo "HELLO=$HELLO"'
>
> Then I see
>
> f: HELLO=x
> HELLO=x
> HELLO=
>
> which seems to contradict the commit message as $HELLO is unset when the
> function returns. I see the same result if I replace "sh" (which is bash
> on my system) with an explicit "bash", "dash" or "zsh".

I believe downstream discussion[1][2] established that the behavior is
inconsistent between various shells and versions of shells, and is
considered undefined by POSIX.

[1]: https://lore.kernel.org/git/xmqq34o1cn6b.fsf@gitster.g/
[2]: https://lore.kernel.org/git/xmqqbk2p9lwi.fsf_-_@gitster.g/

> I'm also confused as to why this caused a problem for Rubén's test as
> $HELLO is set in the environment so I'm don't understand why git wasn't
> picking up the right pager.

Junio summarized the problem and explanation[3].

[3]: https://lore.kernel.org/git/xmqq7cdd9l0m.fsf@gitster.g/

> > A common way to work around the problem is to wrap a subshell around the
> > variable assignments and function call, thus ensuring that the
> > assignments are short-lived. However, these days, a more ergonomic
> > approach is to employ test_env() which is tailor-made for this specific
> > use-case.
>
> Oh, that sounds useful, I didn't know it existed.

I didn't know about it either, and only discovered it upon my initial
attempt at making check-non-portable-shell.pl recognize the case Rubén
identified, at which point it started showing false-positives on
`test_env` invocations. Actually, considering that I was involved[4]
in the conversation which led to the introduction[5] of `test_env` by
Peff, it may be that I did know about it but forgot.

[4]: https://lore.kernel.org/git/CAPig+cR989yU4+JNTFREaeXqY61nusUOhufeBGGVCi29tR1P5w@mail.gmail.com/
[5]: https://lore.kernel.org/git/20160601070425.GA13648@sigill.intra.peff.net/

Re: [PATCH 1/4] t3430: modernize one-shot "VAR=val shell-func" invocation

[Eric Sunshine]

On Mon, Jul 22, 2024 at 2:24 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <ericsunshine@charter.net> writes:
> > A common way to work around the problem is to wrap a subshell around the
> > variable assignments and function call, thus ensuring that the
> > assignments are short-lived. However, these days, a more ergonomic
> > approach is to employ test_env() which is tailor-made for this specific
> > use-case.
>
> OK.  I am not sure if that is "ergonomic", though.  An explict
> subshell has a good documentation value that even though we call
> test_commit there, we do not care about the committer timestamps
> subsequent commits would record, and we do not mind losing the
> effect of test_tick from this invocation of test_commit.  Hiding
> all of that behind test_env loses the documentation value.
>
> We could resurrect it by explicitly passing "--no-tick" to
> test_commit, though ;-).

Your mention of `test_commit` reminded me that, these days, it accepts
an --author switch which seems tailor-made for what this chunk of code
in this test is actually checking: namely, that the root commit of the
orphan branch has "Parsnip <root@example.com>" as its author. So an
even simpler approach is to change it to:

    test_commit --author "Parsnip <root@example.com>" second-root &&

That conveys precisely that the test is interested in overriding the
author for that one commit.

Re: [PATCH 1/4] t3430: modernize one-shot "VAR=val shell-func" invocation

[Eric Sunshine]

On Tue, Jul 23, 2024 at 5:26 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 22/07/2024 16:09, Phillip Wood wrote:
> > On 22/07/2024 07:59, Eric Sunshine wrote:
> >> Unlike "VAR=val cmd" one-shot environment variable assignments which
> >> exist only for the invocation of 'cmd', those assigned by "VAR=val
> >> shell-func" exist within the running shell and continue to do so until
> >> the process exits (or are explicitly unset).
>
> Having seen the parallel discussion about the behavior of hash this
> construct is non-portable because the behavior differs between shells so
> perhaps the commit message could say something like
>
> Unlike "VAR=val cmd" one-shot environment variable assignments which
> only exist for the invocation of external command "cmd", the behavior of
> "VAR=val func" where "func" is a shell function or builtin command
> varies between shells and so we should not use it in our test suite.

Indeed, given all the subsequent discussion, it is now apparent that
multiple undesirable behaviors have been experienced, not just the one
mentioned by this commit message, and that POSIX states that the
behavior is undefined.

Re: [PATCH 3/4] check-non-portable-shell: improve `VAR=val shell-func` detection

[Eric Sunshine]

On Mon, Jul 22, 2024 at 10:46 AM Rubén Justo <rjusto@gmail.com> wrote:
> On Mon, Jul 22, 2024 at 02:59:13AM -0400, Eric Sunshine wrote:
> > -     /^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
> > +     /\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and
>
> Losing "^\s*" means we'll cause false positives, such as:
>
>     # VAR=VAL shell-func
>     echo VAR=VAL shell-func

True, though, considering that "shell-func" in these examples must
match the name of a function actually defined in one of the input
files, one would expect (or at least hope) that this sort of
false-positive will be exceedingly rare. Indeed, there are no such
false-positives in the existing test scripts. Of course, we can always
tighten the regex later if it proves to be problematic.

> Regardless of that, the regex will continue to pose problems with:
>
>   VAR=$OTHER_VALUE shell-func
>   VAR=$(cmd) shell-func
>   VAR=VAL\ UE shell-func
>   VAR="\"val\" shell-func UE" non-shell-func
>
> Which, of course, should be cases that should be written in a more
> orthodox way.

Yes, it can be difficult to be thorough when "linting" a programming
language merely via regular-expressions, and this particular
expression is already almost unreadable. The effort involved in trying
to make it perfect may very well outweigh the potential gain in
coverage.

> But we will start to detect errors like the ones mentioned in the
> message, which are more likely to happen.

Indeed.

[PATCH v2 0/5] improve one-shot variable detection with shell function

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

This is a reroll of [1] which improves check-non-portable-shell's
detection of one-shot environment variable assignment with shell
functions. Changes since v1:

* commit messages now state the behavior is undefined according to POSIX
  rather than focusing only on the original reason given (that the
  assignments could outlive the shell function invocation)

* t3430 simplified by dropping the subshell altogether in favor of
  `test_commit --author`

* new commit to improve the error message when one-shot assignment with
  shell function is detected

[1]: https://lore.kernel.org/git/20240722065915.80760-1-ericsunshine@charter.net/

Eric Sunshine (5):
  t3430: drop unnecessary one-shot "VAR=val shell-func" invocation
  t4034: fix use of one-shot variable assignment with shell function
  check-non-portable-shell: loosen one-shot assignment error message
  check-non-portable-shell: suggest alternative for `VAR=val shell-func`
  check-non-portable-shell: improve `VAR=val shell-func` detection

 t/check-non-portable-shell.pl | 4 ++--
 t/t3430-rebase-merges.sh      | 3 +--
 t/t4034-diff-words.sh         | 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

Range-diff against v1:
1:  5bb6811f68 ! 1:  0d3c0593c9 t3430: modernize one-shot "VAR=val shell-func" invocation
    @@ Metadata
     Author: Eric Sunshine <sunshine@sunshineco.com>
     
      ## Commit message ##
    -    t3430: modernize one-shot "VAR=val shell-func" invocation
    +    t3430: drop unnecessary one-shot "VAR=val shell-func" invocation
     
    -    Unlike "VAR=val cmd" one-shot environment variable assignments which
    -    exist only for the invocation of 'cmd', those assigned by "VAR=val
    -    shell-func" exist within the running shell and continue to do so until
    -    the process exits (or are explicitly unset). check-non-portable-shell.pl
    -    warns when it detects such usage since, more often than not, the author
    -    who writes such an invocation is unaware of the undesirable behavior.
    +    The behavior of a one-shot environment variable assignment of the form
    +    "VAR=val cmd" is undefined according to POSIX when "cmd" is a shell
    +    function. Indeed the behavior differs between shell implementations and
    +    even different versions of the same shell. One such ill-defined behavior
    +    is that, with some shells, the assignment will outlive the invocation of
    +    the function, thus may potentially impact subsequent commands in the
    +    test, as well as subsequent tests. A common way to work around the
    +    problem is to wrap a subshell around the one-shot assignment, thus
    +    ensuring that the assignment is short-lived.
     
    -    A common way to work around the problem is to wrap a subshell around the
    -    variable assignments and function call, thus ensuring that the
    -    assignments are short-lived. However, these days, a more ergonomic
    -    approach is to employ test_env() which is tailor-made for this specific
    -    use-case.
    +    In this test, the subshell is employed precisely for this purpose; other
    +    side-effects of the subshell, such as losing the effect of `test_tick`
    +    which is invoked by `test_commit`, are immaterial.
    +
    +    These days, we can take advantage of `test_commit --author` to more
    +    clearly convey that the test is interested only in overriding the author
    +    of the commit.
     
         Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
     
    @@ t/t3430-rebase-merges.sh: test_expect_success 'refuse to merge ancestors of HEAD
      	git checkout --orphan unrelated &&
     -	(GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
     -	 test_commit second-root) &&
    -+	test_env GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
    -+		test_commit second-root &&
    ++	test_commit --author "Parsnip <root@example.com>" second-root &&
      	test_commit third-root &&
      	cat >script-from-scratch <<-\EOF &&
      	pick third-root
2:  1f35449847 ! 2:  19ee8295ef t4034: fix use of one-shot variable assignment with shell function
    @@ Metadata
      ## Commit message ##
         t4034: fix use of one-shot variable assignment with shell function
     
    -    Unlike "VAR=val cmd" one-shot environment variable assignments which
    -    exist only for the invocation of 'cmd', those assigned by "VAR=val
    -    shell-func" exist within the running shell and continue to do so until
    -    the process exits (or are explicitly unset). In most cases, it is
    -    unlikely that this behavior was intended by the test author, and, even
    -    if those leaked assignments do not impact other tests today, they can
    -    negatively impact tests added later by authors unaware that the variable
    -    assignments are still hanging around. Address this shortcoming by
    -    ensuring that the assignments are short-lived.
    +    The behavior of a one-shot environment variable assignment of the form
    +    "VAR=val cmd" is undefined according to POSIX when "cmd" is a shell
    +    function. Indeed the behavior differs between shell implementations and
    +    even different versions of the same shell, thus should be avoided.
     
         Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
     
-:  ---------- > 3:  220ca26d4f check-non-portable-shell: loosen one-shot assignment error message
-:  ---------- > 4:  4910756aab check-non-portable-shell: suggest alternative for `VAR=val shell-func`
3:  89621f72a2 ! 5:  7a15553a5a check-non-portable-shell: improve `VAR=val shell-func` detection
    @@ Metadata
      ## Commit message ##
         check-non-portable-shell: improve `VAR=val shell-func` detection
     
    -    Unlike "VAR=val cmd" one-shot environment variable assignments which
    -    exist only for the invocation of 'cmd', those assigned by "VAR=val
    -    shell-func" exist within the running shell and continue to do so until
    -    the process exits. check-non-portable-shell.pl warns when it detects
    -    such usage since, more often than not, the author who writes such an
    -    invocation is unaware of the undesirable behavior.
    +    The behavior of a one-shot environment variable assignment of the form
    +    "VAR=val cmd" is undefined according to POSIX when "cmd" is a shell
    +    function. Indeed the behavior differs between shell implementations and
    +    even different versions of the same shell, thus should be avoided.
     
    +    As such, check-non-portable-shell.pl warns when it detects such usage.
         However, a limitation of the check is that it only detects such
    -    invocations when variable assignment (i.e. `VAR=val`) is the first
    -    thing on the line. Thus, it can easily be fooled by an invocation such
    -    as:
    +    invocations when variable assignment (i.e. `VAR=val`) is the first thing
    +    on the line. Thus, it can easily be fooled by an invocation such as:
     
             echo X | VAR=val shell-func
     
    @@ t/check-non-portable-shell.pl: sub err {
      		err q(quote "$val" in 'local var=$val');
     -	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
     +	/\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and
    - 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
    + 		err '"FOO=bar shell_func" is not portable (use test_env FOO=bar shell_func)';
      	$line = '';
      	# this resets our $. for each file
4:  7b2e1dd895 < -:  ---------- check-non-portable-shell: suggest alternative for `VAR=val shell-func`
-- 
2.45.2

[PATCH v2 1/5] t3430: drop unnecessary one-shot "VAR=val shell-func" invocation

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

The behavior of a one-shot environment variable assignment of the form
"VAR=val cmd" is undefined according to POSIX when "cmd" is a shell
function. Indeed the behavior differs between shell implementations and
even different versions of the same shell. One such ill-defined behavior
is that, with some shells, the assignment will outlive the invocation of
the function, thus may potentially impact subsequent commands in the
test, as well as subsequent tests. A common way to work around the
problem is to wrap a subshell around the one-shot assignment, thus
ensuring that the assignment is short-lived.

In this test, the subshell is employed precisely for this purpose; other
side-effects of the subshell, such as losing the effect of `test_tick`
which is invoked by `test_commit`, are immaterial.

These days, we can take advantage of `test_commit --author` to more
clearly convey that the test is interested only in overriding the author
of the commit.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t3430-rebase-merges.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 36ca126bcd..2aa8593f77 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -392,8 +392,7 @@ test_expect_success 'refuse to merge ancestors of HEAD' '
 
 test_expect_success 'root commits' '
 	git checkout --orphan unrelated &&
-	(GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
-	 test_commit second-root) &&
+	test_commit --author "Parsnip <root@example.com>" second-root &&
 	test_commit third-root &&
 	cat >script-from-scratch <<-\EOF &&
 	pick third-root
-- 
2.45.2

[PATCH v2 2/5] t4034: fix use of one-shot variable assignment with shell function

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

The behavior of a one-shot environment variable assignment of the form
"VAR=val cmd" is undefined according to POSIX when "cmd" is a shell
function. Indeed the behavior differs between shell implementations and
even different versions of the same shell, thus should be avoided.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4034-diff-words.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 74586f3813..4dcd7e9925 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -70,7 +70,7 @@ test_language_driver () {
 		word_diff --color-words
 	'
 	test_expect_success "diff driver '$lang' in Islandic" '
-		LANG=is_IS.UTF-8 LANGUAGE=is LC_ALL="$is_IS_locale" \
+		test_env LANG=is_IS.UTF-8 LANGUAGE=is LC_ALL="$is_IS_locale" \
 		word_diff --color-words
 	'
 }
-- 
2.45.2

[PATCH v2 3/5] check-non-portable-shell: loosen one-shot assignment error message

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

When a0a630192d (t/check-non-portable-shell: detect "FOO=bar
shell_func", 2018-07-13) added the check for one-shot environment
variable assignment for shell functions, the primary reason given for
avoiding them was that, under some shells, the assignment outlives the
invocation of the shell function, thus could potentially negatively
impact subsequent commands in the same test, as well as subsequent
tests.

However, it has recently become apparent that this is not the only
potential problem with one-shot assignments and shell functions. Another
problem is that some shells do not actually export the variable to
commands which the function invokes[1]. More significantly, however, the
behavior of one-shot assignments with shell functions is considered
undefined by POSIX[2].

Given this new understanding, the presented error message ("assignment
extends beyond 'shell_func'") is too specific and potentially
misleading. Address this by emitting a less specific error message.

(Note that the wording "is not portable" is chosen over the more
specific "has undefined behavior according to POSIX" for consistency
with almost all other error message issued by this "lint" script.)

[1]: https://lore.kernel.org/git/xmqqbk2p9lwi.fsf_-_@gitster.g/
[2]: https://lore.kernel.org/git/xmqq34o19jj1.fsf@gitster.g/

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/check-non-portable-shell.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b2b28c2ced..179efaa39d 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -50,7 +50,7 @@ sub err {
 	/\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
 		err q(quote "$val" in 'local var=$val');
 	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
-		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
+		err '"FOO=bar shell_func" is not portable';
 	$line = '';
 	# this resets our $. for each file
 	close ARGV if eof;
-- 
2.45.2

[PATCH v2 4/5] check-non-portable-shell: suggest alternative for `VAR=val shell-func`

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

Most problems reported by check-non-portable-shell are accompanied by
advice suggesting how the test author can repair the problem. For
instance:

    error: egrep/fgrep obsolescent (use grep -E/-F)

However, when one-shot variable assignment is detected when calling a
shell function (i.e. `VAR=val shell-func`), the problem is reported, but
no advice is given. The lack of advice is particularly egregious since
neither the problem nor the workaround are likely well-known by
newcomers to the project writing tests for the first time. Address this
shortcoming by recommending the use of `test_env` which is tailor made
for this specific use-case.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/check-non-portable-shell.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 179efaa39d..903af14294 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -50,7 +50,7 @@ sub err {
 	/\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
 		err q(quote "$val" in 'local var=$val');
 	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
-		err '"FOO=bar shell_func" is not portable';
+		err '"FOO=bar shell_func" is not portable (use test_env FOO=bar shell_func)';
 	$line = '';
 	# this resets our $. for each file
 	close ARGV if eof;
-- 
2.45.2

[PATCH v2 5/5] check-non-portable-shell: improve `VAR=val shell-func` detection

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

The behavior of a one-shot environment variable assignment of the form
"VAR=val cmd" is undefined according to POSIX when "cmd" is a shell
function. Indeed the behavior differs between shell implementations and
even different versions of the same shell, thus should be avoided.

As such, check-non-portable-shell.pl warns when it detects such usage.
However, a limitation of the check is that it only detects such
invocations when variable assignment (i.e. `VAR=val`) is the first thing
on the line. Thus, it can easily be fooled by an invocation such as:

    echo X | VAR=val shell-func

Address this shortcoming by loosening the check so that the variable
assignment can be recognized even when not at the beginning of the line.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/check-non-portable-shell.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 903af14294..6ee7700eb4 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -49,7 +49,7 @@ sub err {
 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
 	/\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
 		err q(quote "$val" in 'local var=$val');
-	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
+	/\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and
 		err '"FOO=bar shell_func" is not portable (use test_env FOO=bar shell_func)';
 	$line = '';
 	# this resets our $. for each file
-- 
2.45.2

Re: [PATCH v2 4/5] check-non-portable-shell: suggest alternative for `VAR=val shell-func`

[Rubén Justo]

On Fri, Jul 26, 2024 at 04:15:21AM -0400, Eric Sunshine wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
> 
> Most problems reported by check-non-portable-shell are accompanied by
> advice suggesting how the test author can repair the problem. For
> instance:
> 
>     error: egrep/fgrep obsolescent (use grep -E/-F)
> 
> However, when one-shot variable assignment is detected when calling a
> shell function (i.e. `VAR=val shell-func`), the problem is reported, but
> no advice is given. The lack of advice is particularly egregious since
> neither the problem nor the workaround are likely well-known by
> newcomers to the project writing tests for the first time. Address this
> shortcoming by recommending the use of `test_env` which is tailor made
> for this specific use-case.
> 
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/check-non-portable-shell.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
> index 179efaa39d..903af14294 100755
> --- a/t/check-non-portable-shell.pl
> +++ b/t/check-non-portable-shell.pl
> @@ -50,7 +50,7 @@ sub err {
>  	/\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
>  		err q(quote "$val" in 'local var=$val');
>  	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
> -		err '"FOO=bar shell_func" is not portable';
> +		err '"FOO=bar shell_func" is not portable (use test_env FOO=bar shell_func)';

When someone blames this line in the future, the message of this commit
will appear and be informative.  However, I think the message of the
previous patch [3/5], which also touches this line, would also be
relevant for this context.  And it won't be so obvious to get to that
message.  Therefore, it might be worth combining this commit with the
previous one.  But I'm not sure the change is worth it to have a new
iteration of this series.

Anyway, the change in the err() is a convenient improvement.

>  	$line = '';
>  	# this resets our $. for each file
>  	close ARGV if eof;
> -- 
> 2.45.2
> 

Re: [PATCH 3/4] check-non-portable-shell: improve `VAR=val shell-func` detection

[Rubén Justo]

On Fri, Jul 26, 2024 at 02:45:59AM -0400, Eric Sunshine wrote:
> On Mon, Jul 22, 2024 at 10:46 AM Rubén Justo <rjusto@gmail.com> wrote:
> > On Mon, Jul 22, 2024 at 02:59:13AM -0400, Eric Sunshine wrote:
> > > -     /^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
> > > +     /\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and
> >
> > Losing "^\s*" means we'll cause false positives, such as:
> >
> >     # VAR=VAL shell-func
> >     echo VAR=VAL shell-func
> 
> True, though, considering that "shell-func" in these examples must
> match the name of a function actually defined in one of the input
> files, one would expect (or at least hope) that this sort of
> false-positive will be exceedingly rare. Indeed, there are no such
> false-positives in the existing test scripts. Of course, we can always
> tighten the regex later if it proves to be problematic.
> 
> > Regardless of that, the regex will continue to pose problems with:
> >
> >   VAR=$OTHER_VALUE shell-func
> >   VAR=$(cmd) shell-func
> >   VAR=VAL\ UE shell-func
> >   VAR="\"val\" shell-func UE" non-shell-func
> >
> > Which, of course, should be cases that should be written in a more
> > orthodox way.
> 
> Yes, it can be difficult to be thorough when "linting" a programming
> language merely via regular-expressions, and this particular
> expression is already almost unreadable. The effort involved in trying
> to make it perfect may very well outweigh the potential gain in
> coverage.

I tried to be exhaustive in the analysis of the change and explicit in
the conclusions so that it is clear, and documented in the list, that we
acknowledge the magnitude of the change.

I agree.  I don't think it's worth refining the regex any further.  It
might even be counterproductive.  It covers the cases it was already
covering and the new ones that have occurred.

A simple 'Acked-by: Rubén Justo <rjusto@gmail.com>' didn't seem
sufficient to me :), but perhaps it would have been clearer.

I have the same positive opinion of your new iteration:
20240722065915.80760-5-ericsunshine@charter.net

> 
> > But we will start to detect errors like the ones mentioned in the
> > message, which are more likely to happen.
> 
> Indeed.

Re: [PATCH v2 0/5] improve one-shot variable detection with shell function

[Junio C Hamano]

Eric Sunshine <ericsunshine@charter.net> writes:

>     @@ t/t3430-rebase-merges.sh: test_expect_success 'refuse to merge ancestors of HEAD
>       	git checkout --orphan unrelated &&
>      -	(GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
>      -	 test_commit second-root) &&
>     -+	test_env GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
>     -+		test_commit second-root &&
>     ++	test_commit --author "Parsnip <root@example.com>" second-root &&

Very pleasing to the eyes.

>     @@ t/check-non-portable-shell.pl: sub err {
>       		err q(quote "$val" in 'local var=$val');
>      -	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
>      +	/\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and
>     - 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
>     + 		err '"FOO=bar shell_func" is not portable (use test_env FOO=bar shell_func)';

OK.

Thanks.  Will queue.

Re: [PATCH v2 1/5] t3430: drop unnecessary one-shot "VAR=val shell-func" invocation

[Junio C Hamano]

Eric Sunshine <ericsunshine@charter.net> writes:

> From: Eric Sunshine <sunshine@sunshineco.com>
>
> The behavior of a one-shot environment variable assignment of the form
> "VAR=val cmd" is undefined according to POSIX when "cmd" is a shell

Please use the right word to describe what the standard says.

Throughout the topic's discussion, you seem to be repeating
"undefined", but the word POSIX uses for this particular unportable
behaviour is "unspecified".  The differences are subtle, and for
programs that want to be conformant, there is no practical
difference (in other words, we should not rely on the existence or
validity of the value or behaviour if we wanted to be portable).

The former is what results from use of an invalid construct or
feeding an invalid data input.  The implementation can do whatever
it wants to do once you trigger an undefined behaviour.  The latter
is what results from use of a valid construct or valid data input,
but outcome may differ across implementations.  An "unspecified"
behaviour often are still consistent and sensible within a single
conformant implementation.

Re: [PATCH v2 4/5] check-non-portable-shell: suggest alternative for `VAR=val shell-func`

[Eric Sunshine]

On Fri, Jul 26, 2024 at 9:11 AM Rubén Justo <rjusto@gmail.com> wrote:
> On Fri, Jul 26, 2024 at 04:15:21AM -0400, Eric Sunshine wrote:
> > -             err '"FOO=bar shell_func" is not portable';
> > +             err '"FOO=bar shell_func" is not portable (use test_env FOO=bar shell_func)';
>
> When someone blames this line in the future, the message of this commit
> will appear and be informative.  However, I think the message of the
> previous patch [3/5], which also touches this line, would also be
> relevant for this context.  And it won't be so obvious to get to that
> message.  Therefore, it might be worth combining this commit with the
> previous one.  But I'm not sure the change is worth it to have a new
> iteration of this series.

I did consider combining the two patches but decided against it.
Despite the fact that both patches touch the same line/message, they
really are two distinct "fixes" as evidenced by the fact that the
explanation provided by each commit message is entirely orthogonal to
the other.

Re: [PATCH v2 1/5] t3430: drop unnecessary one-shot "VAR=val shell-func" invocation

[Eric Sunshine]

On Fri, Jul 26, 2024 at 2:50 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <ericsunshine@charter.net> writes:
> > The behavior of a one-shot environment variable assignment of the form
> > "VAR=val cmd" is undefined according to POSIX when "cmd" is a shell
>
> Please use the right word to describe what the standard says.
>
> Throughout the topic's discussion, you seem to be repeating
> "undefined", but the word POSIX uses for this particular unportable
> behaviour is "unspecified".  The differences are subtle, and for
> programs that want to be conformant, there is no practical
> difference (in other words, we should not rely on the existence or
> validity of the value or behaviour if we wanted to be portable).
>
> The former is what results from use of an invalid construct or
> feeding an invalid data input.  The implementation can do whatever
> it wants to do once you trigger an undefined behaviour.  The latter
> is what results from use of a valid construct or valid data input,
> but outcome may differ across implementations.  An "unspecified"
> behaviour often are still consistent and sensible within a single
> conformant implementation.

Makes sense. Will adjust the commit messages.

[PATCH v3 0/5] improve one-shot variable detection with shell function

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

This is a reroll of [1] which improves check-non-portable-shell's
detection of one-shot environment variable assignment with shell
functions.

The only difference from v2 is that the commit messages have been
adjusted to use more accurate terminology[2]. In particular, they now
say that the behavior of one-shot variable assignment with a
shell-function is _unspecified_, not _undefined_.

[1]: https://lore.kernel.org/git/20240726081522.28015-1-ericsunshine@charter.net/
[2]: https://lore.kernel.org/git/xmqqplr0t2bo.fsf@gitster.g/

Eric Sunshine (5):
  t3430: drop unnecessary one-shot "VAR=val shell-func" invocation
  t4034: fix use of one-shot variable assignment with shell function
  check-non-portable-shell: loosen one-shot assignment error message
  check-non-portable-shell: suggest alternative for `VAR=val shell-func`
  check-non-portable-shell: improve `VAR=val shell-func` detection

 t/check-non-portable-shell.pl | 4 ++--
 t/t3430-rebase-merges.sh      | 3 +--
 t/t4034-diff-words.sh         | 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

Range-diff against v2:
1:  0d3c0593c9 ! 1:  3bf12762a5 t3430: drop unnecessary one-shot "VAR=val shell-func" invocation
    @@ Commit message
         t3430: drop unnecessary one-shot "VAR=val shell-func" invocation
     
         The behavior of a one-shot environment variable assignment of the form
    -    "VAR=val cmd" is undefined according to POSIX when "cmd" is a shell
    +    "VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell
         function. Indeed the behavior differs between shell implementations and
    -    even different versions of the same shell. One such ill-defined behavior
    +    even different versions of the same shell. One such problematic behavior
         is that, with some shells, the assignment will outlive the invocation of
         the function, thus may potentially impact subsequent commands in the
         test, as well as subsequent tests. A common way to work around the
2:  19ee8295ef ! 2:  cb77c3dc66 t4034: fix use of one-shot variable assignment with shell function
    @@ Commit message
         t4034: fix use of one-shot variable assignment with shell function
     
         The behavior of a one-shot environment variable assignment of the form
    -    "VAR=val cmd" is undefined according to POSIX when "cmd" is a shell
    +    "VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell
         function. Indeed the behavior differs between shell implementations and
         even different versions of the same shell, thus should be avoided.
     
3:  220ca26d4f ! 3:  0b3716cfb3 check-non-portable-shell: loosen one-shot assignment error message
    @@ Commit message
         potential problem with one-shot assignments and shell functions. Another
         problem is that some shells do not actually export the variable to
         commands which the function invokes[1]. More significantly, however, the
    -    behavior of one-shot assignments with shell functions is considered
    -    undefined by POSIX[2].
    +    behavior of one-shot assignments with shell functions is not specified
    +    by POSIX[2].
     
         Given this new understanding, the presented error message ("assignment
         extends beyond 'shell_func'") is too specific and potentially
         misleading. Address this by emitting a less specific error message.
     
         (Note that the wording "is not portable" is chosen over the more
    -    specific "has undefined behavior according to POSIX" for consistency
    -    with almost all other error message issued by this "lint" script.)
    +    specific "behavior not specified by POSIX" for consistency with almost
    +    all other error message issued by this "lint" script.)
     
         [1]: https://lore.kernel.org/git/xmqqbk2p9lwi.fsf_-_@gitster.g/
         [2]: https://lore.kernel.org/git/xmqq34o19jj1.fsf@gitster.g/
4:  4910756aab = 4:  24ae9be947 check-non-portable-shell: suggest alternative for `VAR=val shell-func`
5:  7a15553a5a ! 5:  38cd3556c5 check-non-portable-shell: improve `VAR=val shell-func` detection
    @@ Commit message
         check-non-portable-shell: improve `VAR=val shell-func` detection
     
         The behavior of a one-shot environment variable assignment of the form
    -    "VAR=val cmd" is undefined according to POSIX when "cmd" is a shell
    +    "VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell
         function. Indeed the behavior differs between shell implementations and
         even different versions of the same shell, thus should be avoided.
     
-- 
2.45.2

[PATCH v3 1/5] t3430: drop unnecessary one-shot "VAR=val shell-func" invocation

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

The behavior of a one-shot environment variable assignment of the form
"VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell
function. Indeed the behavior differs between shell implementations and
even different versions of the same shell. One such problematic behavior
is that, with some shells, the assignment will outlive the invocation of
the function, thus may potentially impact subsequent commands in the
test, as well as subsequent tests. A common way to work around the
problem is to wrap a subshell around the one-shot assignment, thus
ensuring that the assignment is short-lived.

In this test, the subshell is employed precisely for this purpose; other
side-effects of the subshell, such as losing the effect of `test_tick`
which is invoked by `test_commit`, are immaterial.

These days, we can take advantage of `test_commit --author` to more
clearly convey that the test is interested only in overriding the author
of the commit.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t3430-rebase-merges.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 36ca126bcd..2aa8593f77 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -392,8 +392,7 @@ test_expect_success 'refuse to merge ancestors of HEAD' '
 
 test_expect_success 'root commits' '
 	git checkout --orphan unrelated &&
-	(GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
-	 test_commit second-root) &&
+	test_commit --author "Parsnip <root@example.com>" second-root &&
 	test_commit third-root &&
 	cat >script-from-scratch <<-\EOF &&
 	pick third-root
-- 
2.45.2

[PATCH v3 2/5] t4034: fix use of one-shot variable assignment with shell function

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

The behavior of a one-shot environment variable assignment of the form
"VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell
function. Indeed the behavior differs between shell implementations and
even different versions of the same shell, thus should be avoided.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4034-diff-words.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 74586f3813..4dcd7e9925 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -70,7 +70,7 @@ test_language_driver () {
 		word_diff --color-words
 	'
 	test_expect_success "diff driver '$lang' in Islandic" '
-		LANG=is_IS.UTF-8 LANGUAGE=is LC_ALL="$is_IS_locale" \
+		test_env LANG=is_IS.UTF-8 LANGUAGE=is LC_ALL="$is_IS_locale" \
 		word_diff --color-words
 	'
 }
-- 
2.45.2

[PATCH v3 3/5] check-non-portable-shell: loosen one-shot assignment error message

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

When a0a630192d (t/check-non-portable-shell: detect "FOO=bar
shell_func", 2018-07-13) added the check for one-shot environment
variable assignment for shell functions, the primary reason given for
avoiding them was that, under some shells, the assignment outlives the
invocation of the shell function, thus could potentially negatively
impact subsequent commands in the same test, as well as subsequent
tests.

However, it has recently become apparent that this is not the only
potential problem with one-shot assignments and shell functions. Another
problem is that some shells do not actually export the variable to
commands which the function invokes[1]. More significantly, however, the
behavior of one-shot assignments with shell functions is not specified
by POSIX[2].

Given this new understanding, the presented error message ("assignment
extends beyond 'shell_func'") is too specific and potentially
misleading. Address this by emitting a less specific error message.

(Note that the wording "is not portable" is chosen over the more
specific "behavior not specified by POSIX" for consistency with almost
all other error message issued by this "lint" script.)

[1]: https://lore.kernel.org/git/xmqqbk2p9lwi.fsf_-_@gitster.g/
[2]: https://lore.kernel.org/git/xmqq34o19jj1.fsf@gitster.g/

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/check-non-portable-shell.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b2b28c2ced..179efaa39d 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -50,7 +50,7 @@ sub err {
 	/\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
 		err q(quote "$val" in 'local var=$val');
 	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
-		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
+		err '"FOO=bar shell_func" is not portable';
 	$line = '';
 	# this resets our $. for each file
 	close ARGV if eof;
-- 
2.45.2

[PATCH v3 4/5] check-non-portable-shell: suggest alternative for `VAR=val shell-func`

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

Most problems reported by check-non-portable-shell are accompanied by
advice suggesting how the test author can repair the problem. For
instance:

    error: egrep/fgrep obsolescent (use grep -E/-F)

However, when one-shot variable assignment is detected when calling a
shell function (i.e. `VAR=val shell-func`), the problem is reported, but
no advice is given. The lack of advice is particularly egregious since
neither the problem nor the workaround are likely well-known by
newcomers to the project writing tests for the first time. Address this
shortcoming by recommending the use of `test_env` which is tailor made
for this specific use-case.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/check-non-portable-shell.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 179efaa39d..903af14294 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -50,7 +50,7 @@ sub err {
 	/\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
 		err q(quote "$val" in 'local var=$val');
 	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
-		err '"FOO=bar shell_func" is not portable';
+		err '"FOO=bar shell_func" is not portable (use test_env FOO=bar shell_func)';
 	$line = '';
 	# this resets our $. for each file
 	close ARGV if eof;
-- 
2.45.2

[PATCH v3 5/5] check-non-portable-shell: improve `VAR=val shell-func` detection

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

The behavior of a one-shot environment variable assignment of the form
"VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell
function. Indeed the behavior differs between shell implementations and
even different versions of the same shell, thus should be avoided.

As such, check-non-portable-shell.pl warns when it detects such usage.
However, a limitation of the check is that it only detects such
invocations when variable assignment (i.e. `VAR=val`) is the first thing
on the line. Thus, it can easily be fooled by an invocation such as:

    echo X | VAR=val shell-func

Address this shortcoming by loosening the check so that the variable
assignment can be recognized even when not at the beginning of the line.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/check-non-portable-shell.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 903af14294..6ee7700eb4 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -49,7 +49,7 @@ sub err {
 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
 	/\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
 		err q(quote "$val" in 'local var=$val');
-	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
+	/\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and
 		err '"FOO=bar shell_func" is not portable (use test_env FOO=bar shell_func)';
 	$line = '';
 	# this resets our $. for each file
-- 
2.45.2