Re* [PATCH v3 4/4] add-patch: render hunks through the pager

Commit Message

Junio C Hamano July 22, 2024, 9:06 p.m. UTC

Junio C Hamano <[e-mail]> writes:

> Rubén Justo <[e-mail]> writes:
>
>>> It's a very convincing theory but it does not seem to match my
>>> observation.  Is there a difference in shells used, or something?
>>
>> Have you tried your tweak in the "linux-gcc (ubuntu-20.04)" test
>> environment where the problem was detected?  In that environment, the
>> value of GIT_PAGER is not passed to Git in that test. 
>
> So, we may have a shell that does not behave like others ;-)  Do you
> know what shell is being used?

So we have an answer:

  https://github.com/git/git/actions/runs/10047627546/job/27769808515

tells us that the problematic shell is used in the job.

It is

ii  dash           0.5.10.2-6     amd64        POSIX-compliant shell

running on Ubuntu 20.04 that is "too POSIXly correct"[*] and behaves
differently from what the tests expect.

Somebody should write this combination down somewhere in the
documentation so that we can answer (better yet, we do not have to
answer) when somebody wonders if we know of a version of shell that
refuses to do an one-shot export for shell functions as we naïvely
expect.


[Reference]

 * https://lore.kernel.org/git/[e-mail]/


----- >8 --------- >8 --------- >8 --------- >8 ----
CodingGuidelines: give an example shell that "fails" "VAR=VAL shell_func"

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

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

Signed-off-by: Junio C Hamano <[e-mail]>
---
 Documentation/CodingGuidelines | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Rubén Justo July 22, 2024, 10 p.m. UTC | #1

On Mon, Jul 22, 2024 at 02:06:37PM -0700, Junio C Hamano wrote:

> So we have an answer:
> 
>   https://github.com/git/git/actions/runs/10047627546/job/27769808515
> 
> tells us that the problematic shell is used in the job.
> 
> It is
> 
> ii  dash           0.5.10.2-6     amd64        POSIX-compliant shell
> 
> running on Ubuntu 20.04 that is "too POSIXly correct"[*] and behaves
> differently from what the tests expect.
> 
> Somebody should write this combination down somewhere in the
> documentation so that we can answer (better yet, we do not have to
> answer) when somebody wonders if we know of a version of shell that
> refuses to do an one-shot export for shell functions as we naïvely
> expect.
> 
> 
> [Reference]
> 
>  * https://lore.kernel.org/git/[e-mail]/
> 
> 
> ----- >8 --------- >8 --------- >8 --------- >8 ----
> CodingGuidelines: give an example shell that "fails" "VAR=VAL shell_func"
> 
> Over the years, we accumulated the community wisdom to avoid the
> common "one-short export" construct for shell functions, but seem to
> have lost on which exact platform it is known to fail.  Now during
> an investigation on a breakage for a recent topic, let's document
> one example of failing shell.
> 
> This does *not* mean that we can freely start using the construct
> once Ubuntu 20.04 is retired.  But it does mean that we cannot use
> the construct until Ubuntu 20.04 is fully retired from the machines
> that matter.
> 
> Signed-off-by: Junio C Hamano <[e-mail]>
> ---
>  Documentation/CodingGuidelines | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
> index 1d92b2da03..a3ecb4ac5a 100644
> --- c/Documentation/CodingGuidelines
> +++ w/Documentation/CodingGuidelines
> @@ -204,6 +204,29 @@ For shell scripts specifically (not exhaustive):
>  	local variable="$value"
>  	local variable="$(command args)"
>  
> + - The common construct
> +
> +	VAR=VAL command args
> +
> +   to temporarily set and export environment variable VAR only while
> +   "command args" is running is handy, but some versions of dash (like
> +   0.5.10.2-6 found on Ubuntu 20.04) makes a temporary assignment
> +   without exporting the variable, when command is *not* an external
> +   command.  We often have to resort to subshell with explicit export,
> +   i.e.
> +
> +	(incorrect)
> +	VAR=VAL func args
> +
> +	(correct)
> +	(
> +		VAR=VAL && export VAR &&
> +		func args

Just a small comment; maybe it's worth adding an extra line to make the
example clearer:  

		VAR=VAL &&
		export VAR &&
		func args

> +	)
> +
> +   but be careful that the effect "func" makes to the variables in the
> +   current shell will be lost across the subshell boundary.
> +
>   - Use octal escape sequences (e.g. "\302\242"), not hexadecimal (e.g.
>     "\xc2\xa2") in printf format strings, since hexadecimal escape
>     sequences are not portable.

Thank you for digging into the test to find an explanation and for
adding the comment to the documentation.

Kyle Lippincott July 22, 2024, 11:12 p.m. UTC | #2

On Mon, Jul 22, 2024 at 2:06 PM Junio C Hamano <[e-mail]> wrote:
>
> Junio C Hamano <[e-mail]> writes:
>
> > Rubén Justo <[e-mail]> writes:
> >
> >>> It's a very convincing theory but it does not seem to match my
> >>> observation.  Is there a difference in shells used, or something?
> >>
> >> Have you tried your tweak in the "linux-gcc (ubuntu-20.04)" test
> >> environment where the problem was detected?  In that environment, the
> >> value of GIT_PAGER is not passed to Git in that test.
> >
> > So, we may have a shell that does not behave like others ;-)  Do you
> > know what shell is being used?
>
> So we have an answer:
>
>   https://github.com/git/git/actions/runs/10047627546/job/27769808515
>
> tells us that the problematic shell is used in the job.
>
> It is
>
> ii  dash           0.5.10.2-6     amd64        POSIX-compliant shell
>
> running on Ubuntu 20.04 that is "too POSIXly correct"[*] and behaves
> differently from what the tests expect.
>
> Somebody should write this combination down somewhere in the
> documentation so that we can answer (better yet, we do not have to
> answer) when somebody wonders if we know of a version of shell that
> refuses to do an one-shot export for shell functions as we naïvely
> expect.
>
>
> [Reference]
>
>  * https://lore.kernel.org/git/[e-mail]/
>
>
> ----- >8 --------- >8 --------- >8 --------- >8 ----
> CodingGuidelines: give an example shell that "fails" "VAR=VAL shell_func"
>
> Over the years, we accumulated the community wisdom to avoid the
> common "one-short export" construct for shell functions, but seem to
> have lost on which exact platform it is known to fail.  Now during
> an investigation on a breakage for a recent topic, let's document
> one example of failing shell.
>
> This does *not* mean that we can freely start using the construct
> once Ubuntu 20.04 is retired.  But it does mean that we cannot use
> the construct until Ubuntu 20.04 is fully retired from the machines
> that matter.
>
> Signed-off-by: Junio C Hamano <[e-mail]>
> ---
>  Documentation/CodingGuidelines | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
> index 1d92b2da03..a3ecb4ac5a 100644
> --- c/Documentation/CodingGuidelines
> +++ w/Documentation/CodingGuidelines
> @@ -204,6 +204,29 @@ For shell scripts specifically (not exhaustive):
>         local variable="$value"
>         local variable="$(command args)"
>
> + - The common construct
> +
> +       VAR=VAL command args
> +
> +   to temporarily set and export environment variable VAR only while
> +   "command args" is running is handy, but some versions of dash (like
> +   0.5.10.2-6 found on Ubuntu 20.04) makes a temporary assignment

I was also able to reproduce both aspects of this behavior (doesn't
export, value is retained) with ksh (sh (AT&T Research) 93u+m/1.0.8
2024-01-01), which is the current version on debian testing. So maybe
"some versions of ksh (tested: 93u+m/1.0.8 2024-01-01) and dash
(0.5.10.2-6)"? Or maybe we move the 'some versions' around, because I
think it's probably all versions of ksh :)

I don't know how easily discoverable this is, though. I think I'd
still want some linkage between t/check-non-portable-shell.pl and this
section of this file? I probably wouldn't think to look here if I
received that error from the check-non-portable-shell.pl linter.

Otherwise, looks good.

> +   without exporting the variable, when command is *not* an external
> +   command.  We often have to resort to subshell with explicit export,
> +   i.e.
> +
> +       (incorrect)
> +       VAR=VAL func args
> +
> +       (correct)
> +       (
> +               VAR=VAL && export VAR &&
> +               func args
> +       )
> +
> +   but be careful that the effect "func" makes to the variables in the
> +   current shell will be lost across the subshell boundary.
> +
>   - Use octal escape sequences (e.g. "\302\242"), not hexadecimal (e.g.
>     "\xc2\xa2") in printf format strings, since hexadecimal escape
>     sequences are not portable.

Junio C Hamano July 22, 2024, 11:28 p.m. UTC | #3

Kyle Lippincott <[e-mail]> writes:

> I don't know how easily discoverable this is, though. I think I'd
> still want some linkage between t/check-non-portable-shell.pl and this
> section of this file?

Patches welcome.  I personally think CodingGuidelines would be more
promiment source of information than a linter script, and comments
in the lint script cannot grow too elaborate, so that is the reason
why I did this patch first.  Once we have something to refer to in a
section in an authoritative and canonical document, it should be
easy to point at the section from other places, like the linter
script.

Thanks.

Junio C Hamano July 23, 2024, 2:31 a.m. UTC | #4

Kyle Lippincott <[e-mail]> writes:

> I was also able to reproduce both aspects of this behavior (doesn't
> export, value is retained) with ksh (sh (AT&T Research) 93u+m/1.0.8
> 2024-01-01), which is the current version on debian testing. So maybe
> "some versions of ksh (tested: 93u+m/1.0.8 2024-01-01) and dash
> (0.5.10.2-6)"? Or maybe we move the 'some versions' around, because I
> think it's probably all versions of ksh :)

Makes sense, but I think "POSIX guarantees that the behaviour is
something you should not rely on by telling us that these are
unspecified", which you found,  is a much better rationale to
explicitly forbid "VAR=VAL shell_func" construct.

Besides, as another thread recently discussed, our test scripts,
with really heavy uses of "local", do not work at all with AT&T ksh
(other ksh clones are reported to be OK, though).  So it may be OK
to write it off as "unusuable to run our tests", at least for now.

Patch

diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
index 1d92b2da03..a3ecb4ac5a 100644
--- c/Documentation/CodingGuidelines
+++ w/Documentation/CodingGuidelines
@@ -204,6 +204,29 @@  For shell scripts specifically (not exhaustive):
 	local variable="$value"
 	local variable="$(command args)"
 
+ - The common construct
+
+	VAR=VAL command args
+
+   to temporarily set and export environment variable VAR only while
+   "command args" is running is handy, but some versions of dash (like
+   0.5.10.2-6 found on Ubuntu 20.04) makes a temporary assignment
+   without exporting the variable, when command is *not* an external
+   command.  We often have to resort to subshell with explicit export,
+   i.e.
+
+	(incorrect)
+	VAR=VAL func args
+
+	(correct)
+	(
+		VAR=VAL && export VAR &&
+		func args
+	)
+
+   but be careful that the effect "func" makes to the variables in the
+   current shell will be lost across the subshell boundary.
+
  - Use octal escape sequences (e.g. "\302\242"), not hexadecimal (e.g.
    "\xc2\xa2") in printf format strings, since hexadecimal escape
    sequences are not portable.