patch, 6 partslocal VAR="VAL"

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

> 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

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

> On Fri, Apr 5, 2024 at 8:09 PM Junio C Hamano <gitster@pobox.com> wrote:
>> 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".

>> I suspect you meant:
>>
>>    ... and suggest us to instead write it as "VAR=VAL" followed by
>>    "export VAR".
>
> There is no difference between them.  The export command only marks the
> variable for export, independent of the current or future value of the
> variable.  The exported value is always the last assigned one.

> On Apr 06 2024, Eric Sunshine wrote:
> > On Fri, Apr 5, 2024 at 8:09 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> 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".
>
> There is no difference between them.  The export command only marks the
> variable for export, independent of the current or future value of the
> variable.  The exported value is always the last assigned one.

> Future-proof test scripts that do
> 
> 	local VAR=VAL
> 
> without quoting VAL (which is OK in POSIX but broken in some shells)
> that is a positional parameter, e.g. $4.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/lib-parallel-checkout.sh | 2 +-
>  t/t2400-worktree-add.sh    | 2 +-
>  t/t4210-log-i18n.sh        | 4 ++--
>  t/test-lib-functions.sh    | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh
> index acaee9cbb6..8324d6c96d 100644
> --- a/t/lib-parallel-checkout.sh
> +++ b/t/lib-parallel-checkout.sh
> @@ -20,7 +20,7 @@ test_checkout_workers () {
>  		BUG "too few arguments to test_checkout_workers"
>  	fi &&
>  
> -	local expected_workers=$1 &&
> +	local expected_workers="$1" &&
>  	shift &&

>  	local trace_file=trace-test-checkout-workers &&
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index 051363acbb..5851e07290 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -404,7 +404,7 @@ test_expect_success '"add" worktree with orphan branch, lock, and reason' '
>  # Note: Quoted arguments containing spaces are not supported.
>  test_wt_add_orphan_hint () {
>  	local context="$1" &&
> -	local use_branch=$2 &&
> +	local use_branch="$2" &&
>  	shift 2 &&
>  	local opts="$*" &&
>  	test_expect_success "'worktree add' show orphan hint in bad/orphan HEAD w/ $context" '
> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
> index d2dfcf164e..75216f19ce 100755
> --- a/t/t4210-log-i18n.sh
> +++ b/t/t4210-log-i18n.sh
> @@ -64,7 +64,7 @@ test_expect_success 'log --grep does not find non-reencoded values (latin1)' '
>  '
>  
>  triggers_undefined_behaviour () {
> -	local engine=$1
> +	local engine="$1"
>  
>  	case $engine in
>  	fixed)
> @@ -85,7 +85,7 @@ triggers_undefined_behaviour () {
>  }
>  
>  mismatched_git_log () {
> -	local pattern=$1
> +	local pattern="$1"
>  
>  	LC_ALL=$is_IS_locale git log --encoding=ISO-8859-1 --format=%s \
>  		--grep=$pattern
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 2f8868caa1..3204afbafb 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1689,7 +1689,7 @@ test_parse_ls_tree_oids () {
>  # Choose a port number based on the test script's number and store it in
>  # the given variable name, unless that variable already contains a number.
>  test_set_port () {
> -	local var=$1 port
> +	local var="$1" port
>  
>  	if test $# -ne 1 || test -z "$var"
>  	then
> -- 
> 2.44.0-501-g19981daefd
> 
> 

> On Fri, Apr 05, 2024 at 05:08:59PM -0700, Junio C Hamano wrote:
>> Future-proof test scripts that do
>> 
>> 	local VAR=VAL
>> 
>> without quoting VAL (which is OK in POSIX but broken in some shells)
>> that is a positional parameter, e.g. $4.
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  t/lib-parallel-checkout.sh | 2 +-
>>  t/t2400-worktree-add.sh    | 2 +-
>>  t/t4210-log-i18n.sh        | 4 ++--
>>  t/test-lib-functions.sh    | 2 +-
>>  4 files changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh
>> index acaee9cbb6..8324d6c96d 100644
>> --- a/t/lib-parallel-checkout.sh
>> +++ b/t/lib-parallel-checkout.sh
>> @@ -20,7 +20,7 @@ test_checkout_workers () {
>>  		BUG "too few arguments to test_checkout_workers"
>>  	fi &&
>>  
>> -	local expected_workers=$1 &&
>> +	local expected_workers="$1" &&
>>  	shift &&
>
> I was wondering a bit why this is a problem in t0610, but not over here.
> As far as I understand it these statements are fine in practice because
> the expanded values cannot be split, right? So if "$1" expanded to
> something with spaces in between things would start to break.

>in tests, 2021-06-06).
>
>Signed-off-by: Junio C Hamano <gitster@pobox.com>
>---
> Documentation/CodingGuidelines | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
>diff --git a/Documentation/CodingGuidelines

>index 0a39205c48..1cb77a871b 100644
>--- a/Documentation/CodingGuidelines
>+++ b/Documentation/CodingGuidelines
>@@ -194,6 +194,20 @@ For shell scripts specifically (not exhaustive):
>    have changed since then.  We'd need to re-evaluate this rule,
>    together with the rule in t/check-non-portable-shell.pl script.
>
>+ - Some versions of dash have broken variable assignment when prefixed
>+   with "local", "export", and "readonly", in that the value to be
>+   assigned goes through field splitting at $IFS unless quoted.
>+
>+   DO NOT write:
>+
>+     local variable=$value           ;# wrong
>+     local variable=$(command args)  ;# wrong
>+
>+   and instead write:
>+
>+     local variable="$value"
>+     local variable="$(command args)"
>+
>  - 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.

>>  - 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.
>
> I can confirm, at least for the set of platforms I work on, that printf with
> hex values is definitely not portable.

> Dash bug https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097
> lets the shell erroneously perform field splitting on the expansion
> of a command substitution during declaration of a local or an extern
> variable.
>
> The explanation was stolen from ebee5580 (parallel-checkout: avoid
> dash local bug in tests, 2021-06-06).
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> @@ -194,6 +194,20 @@ For shell scripts specifically (not exhaustive):
> + - Some versions of dash have broken variable assignment when prefixed
> +   with "local", "export", and "readonly", in that the value to be
> +   assigned goes through field splitting at $IFS unless quoted.
> +
> +   DO NOT write:
> +
> +     local variable=$value           ;# wrong
> +     local variable=$(command args)  ;# wrong
> +
> +   and instead write:
> +
> +     local variable="$value"
> +     local variable="$(command args)"
> +

> Every other example in the shell-script section of this document is
> written like this:
>
>     (incorrect)
>     local variable=$value
>     local variable=$(command args)
>
>     (correct)
>     local variable="$value"
>     local variable="$(command args)"
>
> Should this patch follow suit for consistency?

> Teach t/check-non-portable-shell.pl that right hand side of the
> assignment done with "local VAR=VAL" need to be quoted.  We
> deliberately target only VAL that begins with $ so that we can catch
> 
>  - $variable_reference and positional parameter reference like $4
>  - $(command substitution)
>  - ${variable_reference-with_magic}
> 
> while excluding
> 
>  - $'\n' that is a bash-ism freely usable in t990[23]
>  - $(( arithmetic )) whose result should be $IFS safe.
>  - $? that also is $IFS safe

> On Fri, Apr 05, 2024 at 05:09:02PM -0700, Junio C Hamano wrote:
>
>> Teach t/check-non-portable-shell.pl that right hand side of the
>> assignment done with "local VAR=VAL" need to be quoted.  We
>> deliberately target only VAL that begins with $ so that we can catch
>> 
>>  - $variable_reference and positional parameter reference like $4
>>  - $(command substitution)
>>  - ${variable_reference-with_magic}
>> 
>> while excluding
>> 
>>  - $'\n' that is a bash-ism freely usable in t990[23]
>>  - $(( arithmetic )) whose result should be $IFS safe.
>>  - $? that also is $IFS safe
>
> Hmm. Just porting over my comment from the other thread (before I
> realized you'd written this series), this misses:
>
>   local foo=bar/$1
>
> etc. Should we look for the "$" anywhere on the line? I doubt we can get
> things foolproof, but requiring somebody to quote:
>
>   local foo=$((1+2))
>
> does not seem like the worst outcome. I dunno.

> > Hmm. Just porting over my comment from the other thread (before I
> > realized you'd written this series), this misses:
> >
> >   local foo=bar/$1
> >
> > etc. Should we look for the "$" anywhere on the line? I doubt we can get
> > things foolproof, but requiring somebody to quote:
> >
> >   local foo=$((1+2))
> >
> > does not seem like the worst outcome. I dunno.
> 
> Looking at the output from
> 
>     $ git grep -E -e 'local [a-zA-Z0-9_]+=[^"]*[$]' t/
> 
> the listed ones in the proposed commit log message are the false
> positives.  Luckily we didn't have anything that tries to
> concatenate parameter reference to something else.
> 
> But with the pattern we do miss
> 
>     local var=$*
> 
> and possibly many others.  So I am not sure.  The false positives
> do look moderately bad, so I'd rather start with the simplest one
> proposed in the patch.