[PATCH 0/6] local VAR="VAL"

[PATCH 0/6] local VAR="VAL"

[Junio C Hamano]

* Update coding guidelines and test-lint script so that we quote the
  right hand side of assignment used with "local", which is buggy on
  certain versions of dash.

The first patch is not about the theme of the topic, but to document
a rule enforced by the test-lint script that is not written down in
the coding guidelines.

The second patch gives guidance to avoid the dash bug.

Patches [3/6], [4/6], and [5/6] are to adjust the existing tests. I
think many of them are currently safe because the values they assign
are $IFS safe, but some may be real workarounds for the dash bug.

The last patch introduces the test-lint pattern.

Junio C Hamano (6):
  CodingGuidelines: describe "export VAR=VAL" rule
  CodingGuidelines: quote assigned value in 'local var=$val'
  t: local VAR="VAL" (quote positional parameters)
  t: local VAR="VAL" (quote command substitution)
  t: local VAR="VAL" (quote ${magic-reference})
  t: teach lint that RHS of 'local VAR=VAL' needs to be quoted

 Documentation/CodingGuidelines | 20 ++++++++++++++++++++
 t/check-non-portable-shell.pl  |  2 ++
 t/lib-parallel-checkout.sh     |  2 +-
 t/t2400-worktree-add.sh        |  2 +-
 t/t4011-diff-symlink.sh        |  4 ++--
 t/t4210-log-i18n.sh            |  4 ++--
 t/test-lib-functions.sh        | 12 ++++++------
 7 files changed, 34 insertions(+), 12 deletions(-)

-- 
2.44.0-501-g19981daefd

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

[Junio C Hamano]

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>
---
 Documentation/CodingGuidelines | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 9495df835d..0a39205c48 100644
--- 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.
+
  - 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.
-- 
2.44.0-501-g19981daefd

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

[Eric Sunshine]

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

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

s/separae/separate/

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

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

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

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

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

Very true.  I thought I'd move some to the log message, but it turns
out that enough is already described there.

Thanks.

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

[Andreas Schwab]

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

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

Correct.

But we are talking about working around sub-standard (read: buggy)
implementations and it is of dubious value to assume a compliant
implementation when devising a workaround.

It is easily imaginable that a sub-standard implementation uses a
symbol table with a single "is it exported?" bit in addition to
(name, value), without a way to say "this parameter is not set
(yet)" (IOW, never value==NULL), and such an implementation would
not be capable to have "this name is exported but nobody set the
value to it yet".  Using an assignment to make sure it is known
before setting the exported bit is safer to protect against such an
implementation.

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

[Eric Sunshine]

On Sat, Apr 6, 2024 at 5:15 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> 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.

Yes, I know, but it is customary in this code-base to write it as:

    VAR=VAL &&
    export VAR

not the other way around, so it makes sense for CodingGuidelines to
illustrate it in a fashion consistent with its use in the project.

[PATCH 2/6] CodingGuidelines: quote assigned value in 'local var=$val'

[Junio C Hamano]

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>
---
 Documentation/CodingGuidelines | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/CodingGuidelines b/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.
-- 
2.44.0-501-g19981daefd

RE: [PATCH 2/6] CodingGuidelines: quote assigned value in 'local var=$val'

[rsbecker]

On Friday, April 5, 2024 8:09 PM, Junio C Hamano wrote:
>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>
>---
> Documentation/CodingGuidelines | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
>diff --git a/Documentation/CodingGuidelines
b/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.

I can confirm, at least for the set of platforms I work on, that printf with
hex values is definitely not portable.
--Randall

Re: [PATCH 2/6] CodingGuidelines: quote assigned value in 'local var=$val'

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

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

Sure, that is why we have that rule that we see in the context.

Re: [PATCH 2/6] CodingGuidelines: quote assigned value in 'local var=$val'

[Eric Sunshine]

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

Re: [PATCH 2/6] CodingGuidelines: quote assigned value in 'local var=$val'

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

Sure.  That sounds like a good idea.

[PATCH 3/6] t: local VAR="VAL" (quote positional parameters)

[Junio C Hamano]

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

Re: [PATCH 3/6] t: local VAR="VAL" (quote positional parameters)

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3237 bytes --]

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 any case, changing all of these to be quoted feels like the right
thing to do regardless of whether or not it happens to work with the
current values of "$1". Otherwise it's simply a confusing failure
waiting to happen.

Patrick

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/6] t: local VAR="VAL" (quote positional parameters)

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

Correct.

> In any case, changing all of these to be quoted feels like the right
> thing to do regardless of whether or not it happens to work with the
> current values of "$1". Otherwise it's simply a confusing failure
> waiting to happen.

Again, agreed.  That is where my "Future-proof" comes from.

The true objective of this change is so that the last patch does not
have to learn too much exceptions ;-)  As long as expected_workers
is expected to be a number (an unbroken sequence of digits), even if
we add more callers to this helper in the future, $1 we see here is
expected to be $IFS safe.  So in that sense my "future-proof" is a
white lie.

[PATCH 4/6] t: local VAR="VAL" (quote command substitution)

[Junio C Hamano]

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 $(command substitution).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4011-diff-symlink.sh | 4 ++--
 t/test-lib-functions.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index d7a5f7ae78..bc8ba88719 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -13,13 +13,13 @@ TEST_PASSES_SANITIZE_LEAK=true
 
 # Print the short OID of a symlink with the given name.
 symlink_oid () {
-	local oid=$(printf "%s" "$1" | git hash-object --stdin) &&
+	local oid="$(printf "%s" "$1" | git hash-object --stdin)" &&
 	git rev-parse --short "$oid"
 }
 
 # Print the short OID of the given file.
 short_oid () {
-	local oid=$(git hash-object "$1") &&
+	local oid="$(git hash-object "$1")" &&
 	git rev-parse --short "$oid"
 }
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3204afbafb..3dc638f7dc 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1764,7 +1764,7 @@ test_subcommand () {
 		shift
 	fi
 
-	local expr=$(printf '"%s",' "$@")
+	local expr="$(printf '"%s",' "$@")"
 	expr="${expr%,}"
 
 	if test -n "$negate"
-- 
2.44.0-501-g19981daefd

[PATCH 5/6] t: local VAR="VAL" (quote ${magic-reference})

[Junio C Hamano]

Future-proof test scripts that do

	local VAR=VAL

without quoting VAL (which is OK in POSIX but broken in some shells)
that is ${magic-"reference to a parameter"}.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib-functions.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3dc638f7dc..029cb31ffe 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -330,7 +330,7 @@ test_commit () {
 		shift
 	done &&
 	indir=${indir:+"$indir"/} &&
-	local file=${2:-"$1.t"} &&
+	local file="${2:-"$1.t"}" &&
 	if test -n "$append"
 	then
 		$echo "${3-$1}" >>"$indir$file"
@@ -1672,7 +1672,7 @@ test_oid () {
 # Insert a slash into an object ID so it can be used to reference a location
 # under ".git/objects".  For example, "deadbeef..." becomes "de/adbeef..".
 test_oid_to_path () {
-	local basename=${1#??}
+	local basename="${1#??}"
 	echo "${1%$basename}/$basename"
 }
 
@@ -1840,7 +1840,7 @@ test_readlink () {
 # An optional increment to the magic timestamp may be specified as second
 # argument.
 test_set_magic_mtime () {
-	local inc=${2:-0} &&
+	local inc="${2:-0}" &&
 	local mtime=$((1234567890 + $inc)) &&
 	test-tool chmtime =$mtime "$1" &&
 	test_is_magic_mtime "$1" $inc
@@ -1853,7 +1853,7 @@ test_set_magic_mtime () {
 # argument.  Usually, this should be the same increment which was used for
 # the associated test_set_magic_mtime.
 test_is_magic_mtime () {
-	local inc=${2:-0} &&
+	local inc="${2:-0}" &&
 	local mtime=$((1234567890 + $inc)) &&
 	echo $mtime >.git/test-mtime-expect &&
 	test-tool chmtime --get "$1" >.git/test-mtime-actual &&
-- 
2.44.0-501-g19981daefd

[PATCH 6/6] t: teach lint that RHS of 'local VAR=VAL' needs to be quoted

[Junio C Hamano]

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

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/check-non-portable-shell.pl | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index dd8107cd7d..b2b28c2ced 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -47,6 +47,8 @@ sub err {
 	/\bgrep\b.*--file\b/ and err 'grep --file FILE is not portable (use grep -f FILE)';
 	/\b[ef]grep\b/ and err 'egrep/fgrep obsolescent (use grep -E/-F)';
 	/\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
 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
 	$line = '';
-- 
2.44.0-501-g19981daefd

Re: [PATCH 6/6] t: teach lint that RHS of 'local VAR=VAL' needs to be quoted

[Jeff King]

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.

-Peff

Re: [PATCH 6/6] t: teach lint that RHS of 'local VAR=VAL' needs to be quoted

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

Re: [PATCH 6/6] t: teach lint that RHS of 'local VAR=VAL' needs to be quoted

[Jeff King]

On Mon, Apr 08, 2024 at 10:31:34AM -0700, Junio C Hamano wrote:

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

Yeah, I think a regex is probably going to end up with either false
positives or false negatives. It probably does not matter too much which
way we err, if we expect them to be rare on either side.

My thinking was mostly that false negatives are worse, because they only
matter on old buggy versions of dash (and only if the tests actually
pass a value with spaces). And so most developers will not notice them
immediately. Whereas false positives, while annoying, are reported to
them immediately by the linter. And generally, dealing with problems
closer to the time of writing means less work overall.

But I am happy to take your series as-is and we can see which cases (if
any!) we miss in practice.

I do hope that eventually we could just say "that buggy version of dash
does not matter anymore", but I think it is too soon for that (it sounds
like it is still being used in CI).

-Peff

[PATCH 7/6] t0610: local VAR="VAL" fix

[Junio C Hamano]

The series was based on maint and fixes all the tests that exist
there, but we have acquired a few more.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0610-reftable-basics.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git i/t/t0610-reftable-basics.sh w/t/t0610-reftable-basics.sh
index 686781192e..c8074ebab2 100755
--- i/t/t0610-reftable-basics.sh
+++ w/t/t0610-reftable-basics.sh
@@ -83,7 +83,7 @@ test_expect_success 'init: reinitializing reftable with files backend fails' '
 test_expect_perms () {
 	local perms="$1"
 	local file="$2"
-	local actual=$(ls -l "$file") &&
+	local actual="$(ls -l "$file")" &&
 
 	case "$actual" in
 	$perms*)

[PATCH 8/6] t1016: local VAR="VAL" fix

[Junio C Hamano]

The series was based on maint and fixes all the tests that exist
there, but we have acquired a few more.

I suspect that the values assigned in many of these places are $IFS
safe, and this is primarily to squelch the linter than adding a
necessary workaround for buggy dash.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t1016-compatObjectFormat.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git c/t/t1016-compatObjectFormat.sh w/t/t1016-compatObjectFormat.sh
index 8132cd37b8..be3206a16f 100755
--- c/t/t1016-compatObjectFormat.sh
+++ w/t/t1016-compatObjectFormat.sh
@@ -79,7 +79,7 @@ commit2_oid () {
 }
 
 del_sigcommit () {
-    local delete=$1
+    local delete="$1"
 
     if test "$delete" = "sha256" ; then
 	local pattern="gpgsig-sha256"
@@ -91,8 +91,8 @@ del_sigcommit () {
 
 
 del_sigtag () {
-    local storage=$1
-    local delete=$2
+    local storage="$1"
+    local delete="$2"
 
     if test "$storage" = "$delete" ; then
 	local pattern="trailer"
@@ -181,7 +181,7 @@ done
 cd "$base"
 
 compare_oids () {
-    test "$#" = 5 && { local PREREQ=$1; shift; } || PREREQ=
+    test "$#" = 5 && { local PREREQ="$1"; shift; } || PREREQ=
     local type="$1"
     local name="$2"
     local sha1_oid="$3"
@@ -193,8 +193,8 @@ compare_oids () {
 
     git --git-dir=repo-sha1/.git rev-parse --output-object-format=sha256 ${sha1_oid} > ${name}_sha1_sha256_found
     git --git-dir=repo-sha256/.git rev-parse --output-object-format=sha1 ${sha256_oid} > ${name}_sha256_sha1_found
-    local sha1_sha256_oid=$(cat ${name}_sha1_sha256_found)
-    local sha256_sha1_oid=$(cat ${name}_sha256_sha1_found)
+    local sha1_sha256_oid="$(cat ${name}_sha1_sha256_found)"
+    local sha256_sha1_oid="$(cat ${name}_sha256_sha1_found)"
 
     test_expect_success $PREREQ "Verify ${type} ${name}'s sha1 oid" '
 	git --git-dir=repo-sha256/.git rev-parse --output-object-format=sha1 ${sha256_oid} > ${name}_sha1 &&