threads / discuss / 61776

[Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

Subject: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

## tl;dr

18 messages between Jul 13, 2024 and Jul 15, 2024.

replies: 17people: 4as markdown or json

rsbecker@nexbridge.com· Jul 13, 2024, 16:38 UTC · lore

Here is the breakage. I included the prior test log in case there was a dependency.

checking prerequisite: TTY
mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-TTY" &&
(
        cd "$TRASH_DIRECTORY/prereq-test-dir-TTY" &&
        test_have_prereq PERL &&
        # Reading from the pty master seems to get stuck _sometimes_
        # on Mac OS X 10.5.0, using Perl 5.10.0 or 5.8.9.
        #
        # Reproduction recipe: run
        #
        #       i=0
        #       while ./test-terminal.perl echo hi $i
        #       do
        #               : $((i = $i + 1))
        #       done
        #
        # After 2000 iterations or so it hangs.
        # https://rt.cpan.org/Ticket/Display.html?id=65692
        #
        test "$(uname -s)" != Darwin &&
        perl "$TEST_DIRECTORY"/test-terminal.perl \
                sh -c "test -t 1 && test -t 2"
)
+ mkdir -p /home/randall/git/t/trash
directory.t0021-conversion/prereq-test-dir-TTY
+ cd /home/randall/git/t/trash
directory.t0021-conversion/prereq-test-dir-TTY
+ test_have_prereq PERL
+ uname -s
+ test NONSTOP_KERNEL != Darwin
+ perl /home/randall/git/t/test-terminal.perl sh -c test -t 1 && test -t 2
Can't locate IO/Pty.pm in @INC (you may need to install the IO::Pty module)
(@INC contains: /usr/lib/perl5/site_perl/5.30.3/nonstop_kernel
/usr/lib/perl5/site_perl/5.30.3 /usr/lib/perl5/5.30.3/nonstop_kernel
/usr/lib/perl5/5.30.3 /usr/lib/perl5/site_perl) at
/home/randall/git/t/test-terminal.perl line 5.
BEGIN failed--compilation aborted at /home/randall/git/t/test-terminal.perl
line 5.
error: last command exited with $?=162
prerequisite TTY not satisfied
ok 34 # skip delayed checkout shows progress by default on tty (pathspec
checkout) (missing TTY of PERL,TTY)
expecting success of 0021.35 'delayed checkout ommits progress on non-tty
(pathspec checkout)':
                test_delayed_checkout_progress ! git checkout $opt
+ test_delayed_checkout_progress ! git checkout .
test_delayed_checkout_progress: test: argument expected
error: last command exited with $?=127
not ok 35 - delayed checkout ommits progress on non-tty (pathspec checkout)
#
#                       test_delayed_checkout_progress ! git checkout $opt
#
1..35

-- Brief whoami: NonStop&UNIX developer since approximately UNIX(421664400) NonStop(211288444200000000) -- In real life, I talk too much.

Junio C Hamano· Jul 14, 2024, 02:53 UTC · re: rsbecker@nexbridge.com · lore

Re: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

<rsbecker@nexbridge.com> writes:
Show 9 quoted lines
> ok 34 # skip delayed checkout shows progress by default on tty (pathspec
> checkout) (missing TTY of PERL,TTY)
>
> expecting success of 0021.35 'delayed checkout ommits progress on non-tty
> (pathspec checkout)':
>                 test_delayed_checkout_progress ! git checkout $opt
>
> + test_delayed_checkout_progress ! git checkout .
> test_delayed_checkout_progress: test: argument expected
This "test: argument expected" is curious.

The shell function reads like so and it hasn't changed since 7a132c62 (checkout: make delayed checkout respect --quiet and --no-progress, 2021-08-26) introduced it, and there are 3 uses of "test" builtin function.

        test_delayed_checkout_progress () {
                if test "$1" = "!"
                then
                        local expect_progress=N &&
                        shift
                else
                        local expect_progress=
                fi &&
                if test $# -lt 1
                then
                        BUG "no command given to test_delayed_checkout_progress"
                fi &&
                (
                        cd progress &&
                        GIT_PROGRESS_DELAY=0 &&
                        export GIT_PROGRESS_DELAY &&
                        rm -f *.a delay-progress.log &&
                        "$@" 2>err &&
                        grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log &&
                        if test "$expect_progress" = N
                        then
                                ! grep "Filtering content" err
                        else
                                grep "Filtering content" err
                        fi
                )
        }

I do not see how any of these tests can be fed a list of zero or more argument that causes it to barf with "argument expected", when

	test_delayed_checkout_progress ! git checkout .
is run.  The first "are we negated?" test gets
	test "!" = "!"

and shifts that "!" away, the second test that checks the number of arguments do not need any extra quoting (in other words, $# can never be an empty string, so

	test $# -lt 1
cannot become "test -lt 1" that would barf, like so:
	$ ksh -c 'test -lt 1'
	ksh: test: argument expected

And whether expect_progress gets an empty string or N (we should have got N in there, if the shell is so broken that the first comparison between "!" and "!" were botched and the variable got assigned an empty string, or "local" were botched and the variable were somehow left uninitialized, expect_progress could be a value we do not expect), the last test to switch between !grep and grep can never break like so:

	$ ksh -c 'test = N'
	ksh: test: argument expected
because $expect_progress is inside a pair of double quotes.

In the above "thinking out aloud", I invoked ksh because bash and dash gives different error messages.

Another remotely curious thing is that I am not sure how "local" in the outer shell influences the use of the variable in the subshell (read: now I am speculating if the above code uses some constructs that are so tricky to implement correctly and an unfortunate bug gets triggered in your shell), but even if such a bug caused a nonsense value to be assigned to $expect_progress, I do not see how it can cause "test" to barf with "argument expected".

So, I am not sure what is going on at all.

In any case, this does not sound anything recent. Do you know that this used to work in previous releases? Could this be caused by changes in your environment (like which shell is used)?

rsbecker@nexbridge.com· Jul 14, 2024, 13:29 UTC · re: Junio C Hamano · lore

RE: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

On Saturday, July 13, 2024 10:54 PM, Junio C Hamano wrote:
Show 17 quoted lines
><rsbecker@nexbridge.com> writes:
>
>> ok 34 # skip delayed checkout shows progress by default on tty
>> (pathspec
>> checkout) (missing TTY of PERL,TTY)
>>
>> expecting success of 0021.35 'delayed checkout ommits progress on
>> non-tty (pathspec checkout)':
>>                 test_delayed_checkout_progress ! git checkout $opt
>>
>> + test_delayed_checkout_progress ! git checkout .
>> test_delayed_checkout_progress: test: argument expected
>
>This "test: argument expected" is curious.
>
>The shell function reads like so and it hasn't changed since
>7a132c62 (checkout: make delayed checkout respect --quiet and
--no-progress,
Show 14 quoted lines
>2021-08-26) introduced it, and there are 3 uses of "test" builtin function.
>
>        test_delayed_checkout_progress () {
>                if test "$1" = "!"
>                then
>                        local expect_progress=N &&
>                        shift
>                else
>                        local expect_progress=
>                fi &&
>
>                if test $# -lt 1
>                then
>                        BUG "no command given to
test_delayed_checkout_progress"
Show 10 quoted lines
>                fi &&
>
>                (
>                        cd progress &&
>                        GIT_PROGRESS_DELAY=0 &&
>                        export GIT_PROGRESS_DELAY &&
>                        rm -f *.a delay-progress.log &&
>
>                        "$@" 2>err &&
>                        grep "IN: smudge test-delay10.a .* \\[DELAYED\\]"
delay-progress.log
Show 11 quoted lines
>&&
>                        if test "$expect_progress" = N
>                        then
>                                ! grep "Filtering content" err
>                        else
>                                grep "Filtering content" err
>                        fi
>                )
>        }
>
>I do not see how any of these tests can be fed a list of zero or more
argument that
Show 9 quoted lines
>causes it to barf with "argument expected", when
>
>	test_delayed_checkout_progress ! git checkout .
>
>is run.  The first "are we negated?" test gets
>
>	test "!" = "!"
>
>and shifts that "!" away, the second test that checks the number of
arguments do
>not need any extra quoting (in other words, $# can never be an empty
string, so
Show 9 quoted lines
>
>	test $# -lt 1
>
>cannot become "test -lt 1" that would barf, like so:
>
>	$ ksh -c 'test -lt 1'
>	ksh: test: argument expected
>
>And whether expect_progress gets an empty string or N (we should have got N
in
>there, if the shell is so broken that the first comparison between "!" and
"!" were
>botched and the variable got assigned an empty string, or "local" were
botched and
>the variable were somehow left uninitialized, expect_progress could be a
value we
>do not expect), the last test to switch between !grep and grep can never
break like
Show 8 quoted lines
>so:
>
>	$ ksh -c 'test = N'
>	ksh: test: argument expected
>
>because $expect_progress is inside a pair of double quotes.
>
>In the above "thinking out aloud", I invoked ksh because bash and dash
gives
>different error messages.
>
>Another remotely curious thing is that I am not sure how "local" in the
outer shell
>influences the use of the variable in the subshell
>(read: now I am speculating if the above code uses some constructs that are
so
>tricky to implement correctly and an unfortunate bug gets triggered in your
shell),
>but even if such a bug caused a nonsense value to be assigned to
$expect_progress,
Show 5 quoted lines
>I do not see how it can cause "test" to barf with "argument expected".
>
>So, I am not sure what is going on at all.
>
>In any case, this does not sound anything recent.  Do you know that this
used to
>work in previous releases?  Could this be caused by changes in your
environment
>(like which shell is used)?

This looks like a different between ksh and bash. Under bash, the test works. I can live with that but will have to force bash to be used as the shebang #!/bin/sh defaults to ksh on this box.

Thanks for looking.
--Randall
Junio C Hamano· Jul 14, 2024, 17:00 UTC · re: rsbecker@nexbridge.com · lore

Re: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

<rsbecker@nexbridge.com> writes:
> This looks like a different between ksh and bash. Under bash, the test
> works. I can live with that but will have to force bash to be used as the
> shebang #!/bin/sh defaults to ksh on this box.

It turns out that the version of ksh I used in my description does not seem to grok "local" at all. I vaguely recall that we've written off various hobbist reimplementation of ksh as unusable enough, but this one is ksh93 direct from AT&T Research.

I guess when we said "as long as we limit our use to a simple 'this variable has visibility limited to the function and its children' and nothing else, it is portable enough across practically everybody we care about", we have written off the real ksh, too.

In the meantime, we may want to document this in a more prominent way. Perhaps like so:

-------- >8 --------------- >8 --------------- >8 --------
Subject: doc: guide to use of "local" shell language construct

The scripted Porcelain commands do not allow use of "local" because it is not universally supported, but we use it liberally in our test scripts, which means some POSIX compliant shells (like "ksh93") can not be used to run our tests.

Document the status quo, and hint that we might want to change the situation in the fiture.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 4 +++-
 t/README                       | 8 ++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)
diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
index 1d92b2da03..68b7210f48 100644
--- c/Documentation/CodingGuidelines
+++ w/Documentation/CodingGuidelines
@@ -186,7 +186,9 @@ For shell scripts specifically (not exhaustive):
  - Even though "local" is not part of POSIX, we make heavy use of it
    in our test suite.  We do not use it in scripted Porcelains, and
    hopefully nobody starts using "local" before they are reimplemented
-   in C ;-)
+   in C ;-) Notably, ksh (not just reimplementations but the real one
+   from AT&T Research) does not support "local" and cannot be used,
+   which we might want to reconsider.
 
  - Some versions of shell do not understand "export variable=value",
    so we write "variable=value" and then "export variable" on two
diff --git c/t/README w/t/README
index d9e0e07506..1d39d8cfd5 100644
--- c/t/README
+++ w/t/README
@@ -850,6 +850,14 @@ And here are the "don'ts:"
    but the best indication is to just run the tests with prove(1),
    it'll complain if anything is amiss.
 
+ - Don't overuse "local"
+
+   Because strictly POSIX-compliant shells do not have to support
+   "local", we avoid using it in our scripted Porcelain scripts, but
+   we have allowed use of "local" in test scripts.  We may want to
+   reconsider this and rewrite our tests to also run on shells like
+   ksh93.  Do not add new use of "local" unnecessarily.
+
 
 Skipping tests
 --------------
rsbecker@nexbridge.com· Jul 14, 2024, 17:29 UTC · re: Junio C Hamano · lore

RE: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

On Sunday, July 14, 2024 1:00 PM, Junio C Hamano wrote:
Show 7 quoted lines
><rsbecker@nexbridge.com> writes:
>
>> This looks like a different between ksh and bash. Under bash, the test
>> works. I can live with that but will have to force bash to be used as
>> the shebang #!/bin/sh defaults to ksh on this box.
>
>It turns out that the version of ksh I used in my description does not seem
to grok
>"local" at all. I vaguely recall that we've written off various hobbist
>reimplementation of ksh as unusable enough, but this one is ksh93 direct
from
>AT&T Research.
>
>I guess when we said "as long as we limit our use to a simple 'this
variable has
>visibility limited to the function and its children'
>and nothing else, it is portable enough across practically everybody we
care about",
Show 9 quoted lines
>we have written off the real ksh, too.
>
>In the meantime, we may want to document this in a more prominent way.
>Perhaps like so:
>
>-------- >8 --------------- >8 --------------- >8 --------
>Subject: doc: guide to use of "local" shell language construct
>
>The scripted Porcelain commands do not allow use of "local" because it is
not
>universally supported, but we use it liberally in our test scripts, which
means some
>POSIX compliant shells (like "ksh93") can not be used to run our tests.
>
>Document the status quo, and hint that we might want to change the
situation in
Show 9 quoted lines
>the fiture.
>
>Signed-off-by: Junio C Hamano <gitster@pobox.com>
>---
> Documentation/CodingGuidelines | 4 +++-
> t/README                       | 8 ++++++++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
>diff --git c/Documentation/CodingGuidelines
w/Documentation/CodingGuidelines
Show 14 quoted lines
>index 1d92b2da03..68b7210f48 100644
>--- c/Documentation/CodingGuidelines
>+++ w/Documentation/CodingGuidelines
>@@ -186,7 +186,9 @@ For shell scripts specifically (not exhaustive):
>  - Even though "local" is not part of POSIX, we make heavy use of it
>    in our test suite.  We do not use it in scripted Porcelains, and
>    hopefully nobody starts using "local" before they are reimplemented
>-   in C ;-)
>+   in C ;-) Notably, ksh (not just reimplementations but the real one
>+   from AT&T Research) does not support "local" and cannot be used,
>+   which we might want to reconsider.
>
>  - Some versions of shell do not understand "export variable=value",
>    so we write "variable=value" and then "export variable" on two diff
--git
Show 18 quoted lines
>c/t/README w/t/README index d9e0e07506..1d39d8cfd5 100644
>--- c/t/README
>+++ w/t/README
>@@ -850,6 +850,14 @@ And here are the "don'ts:"
>    but the best indication is to just run the tests with prove(1),
>    it'll complain if anything is amiss.
>
>+ - Don't overuse "local"
>+
>+   Because strictly POSIX-compliant shells do not have to support
>+   "local", we avoid using it in our scripted Porcelain scripts, but
>+   we have allowed use of "local" in test scripts.  We may want to
>+   reconsider this and rewrite our tests to also run on shells like
>+   ksh93.  Do not add new use of "local" unnecessarily.
>+
>
> Skipping tests
> --------------

Thanks. I approve. I'm currently working on trying to get the test suite to run under bash. It looks like TEST_SHELL_PATH is not propagated to the inner make -C test. My current approach is to run the inner make without the outer make. Otherwise I am forced to use ksh, which is known not to work. Will advice when this runs - I have to rebuild, and that takes about an hour.

brian m. carlson· Jul 14, 2024, 18:15 UTC · re: Junio C Hamano · lore

Re: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

On 2024-07-14 at 17:00:12, Junio C Hamano wrote:
Show 29 quoted lines
> <rsbecker@nexbridge.com> writes:
> 
> > This looks like a different between ksh and bash. Under bash, the test
> > works. I can live with that but will have to force bash to be used as the
> > shebang #!/bin/sh defaults to ksh on this box.
> 
> It turns out that the version of ksh I used in my description does
> not seem to grok "local" at all. I vaguely recall that we've written
> off various hobbist reimplementation of ksh as unusable enough, but
> this one is ksh93 direct from AT&T Research.
> 
> I guess when we said "as long as we limit our use to a simple 'this
> variable has visibility limited to the function and its children'
> and nothing else, it is portable enough across practically everybody
> we care about", we have written off the real ksh, too.
> 
> In the meantime, we may want to document this in a more prominent
> way.  Perhaps like so:
> 
> -------- >8 --------------- >8 --------------- >8 --------
> Subject: doc: guide to use of "local" shell language construct
> 
> The scripted Porcelain commands do not allow use of "local" because
> it is not universally supported, but we use it liberally in our test
> scripts, which means some POSIX compliant shells (like "ksh93") can
> not be used to run our tests.
> 
> Document the status quo, and hint that we might want to change the
> situation in the fiture.

I don't think this is the right approach. Every version of ksh _except_ AT&T ksh works just fine here. pdksh, mksh, lksh, OpenBSD's ksh (which is also its /bin/sh) work fine, as do bash, dash, FreeBSD's sh (ash), Busybox's sh (also ash), and zsh (when run in sh mode with 5.9 or newer). AT&T ksh is considering adding local in a newer version for this reason.

Literally only AT&T ksh is not supported here, and so anyone can set SHELL_PATH to any suitable shell. I don't think it's useful to get rid of local when there are a variety of acceptable and portable options.

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA
rsbecker@nexbridge.com· Jul 14, 2024, 18:28 UTC · re: brian m. carlson · lore

RE: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

On Sunday, July 14, 2024 2:16 PM, brian m. carlson wrote:
Show 44 quoted lines
>To: Junio C Hamano <gitster@pobox.com>
>Cc: rsbecker@nexbridge.com; git@vger.kernel.org
>Subject: Re: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop
>
>On 2024-07-14 at 17:00:12, Junio C Hamano wrote:
>> <rsbecker@nexbridge.com> writes:
>>
>> > This looks like a different between ksh and bash. Under bash, the
>> > test works. I can live with that but will have to force bash to be
>> > used as the shebang #!/bin/sh defaults to ksh on this box.
>>
>> It turns out that the version of ksh I used in my description does not
>> seem to grok "local" at all. I vaguely recall that we've written off
>> various hobbist reimplementation of ksh as unusable enough, but this
>> one is ksh93 direct from AT&T Research.
>>
>> I guess when we said "as long as we limit our use to a simple 'this
>> variable has visibility limited to the function and its children'
>> and nothing else, it is portable enough across practically everybody
>> we care about", we have written off the real ksh, too.
>>
>> In the meantime, we may want to document this in a more prominent way.
>> Perhaps like so:
>>
>> -------- >8 --------------- >8 --------------- >8 --------
>> Subject: doc: guide to use of "local" shell language construct
>>
>> The scripted Porcelain commands do not allow use of "local" because it
>> is not universally supported, but we use it liberally in our test
>> scripts, which means some POSIX compliant shells (like "ksh93") can
>> not be used to run our tests.
>>
>> Document the status quo, and hint that we might want to change the
>> situation in the fiture.
>
>I don't think this is the right approach.  Every version of ksh _except_ AT&T ksh
>works just fine here.  pdksh, mksh, lksh, OpenBSD's ksh (which is also its /bin/sh)
>work fine, as do bash, dash, FreeBSD's sh (ash), Busybox's sh (also ash), and zsh
>(when run in sh mode with 5.9 or newer).  AT&T ksh is considering adding local in a
>newer version for this reason.
>
>Literally only AT&T ksh is not supported here, and so anyone can set SHELL_PATH to
>any suitable shell.  I don't think it's useful to get rid of local when there are a variety
>of acceptable and portable options.
We can add NonStop's ksh to the list of not supported. I'm using TEST_SHELL_PATH while running make all in the t directory. Test passes when I use bash. For some reason (maybe GNUMake 4.1, which is what I have in my POSIX environment, I don't get TEST_SHELL_PATH passed down from the outer Makefile, but I can work with that. t0021 is now passing in my current CI stream using bash 5.0.18.
--Randall
brian m. carlson· Jul 14, 2024, 22:01 UTC · re: rsbecker@nexbridge.com · lore

Re: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

On 2024-07-14 at 18:28:56, rsbecker@nexbridge.com wrote:
Show 12 quoted lines
> On Sunday, July 14, 2024 2:16 PM, brian m. carlson wrote:
> >I don't think this is the right approach.  Every version of ksh _except_ AT&T ksh
> >works just fine here.  pdksh, mksh, lksh, OpenBSD's ksh (which is also its /bin/sh)
> >work fine, as do bash, dash, FreeBSD's sh (ash), Busybox's sh (also ash), and zsh
> >(when run in sh mode with 5.9 or newer).  AT&T ksh is considering adding local in a
> >newer version for this reason.
> >
> >Literally only AT&T ksh is not supported here, and so anyone can set SHELL_PATH to
> >any suitable shell.  I don't think it's useful to get rid of local when there are a variety
> >of acceptable and portable options.
> 
> We can add NonStop's ksh to the list of not supported. I'm using TEST_SHELL_PATH while running make all in the t directory. Test passes when I use bash. For some reason (maybe GNUMake 4.1, which is what I have in my POSIX environment, I don't get TEST_SHELL_PATH passed down from the outer Makefile, but I can work with that. t0021 is now passing in my current CI stream using bash 5.0.18.

I think we had discussed that you were using AT&T ksh on NonStop, which would explain the situation. That's the most common version of ksh on proprietary Unix systems, and you can usually detect it with something like this:

% ksh -c 'echo $KSH_VERSION' Version AJM 93u+m/1.0.8 2024-01-01

Variants of pdksh look like this (this one from OpenBSD):

% ksh -c 'echo $KSH_VERSION' @(#)PD KSH v5.2.14 99/07/13.2

And mksh and lksh (which are the same shell, just compiled differently, look like this:

% mksh -c 'echo $KSH_VERSION' @(#)MIRBSD KSH R59 2024/02/01 +Debian

% lksh -c 'echo $KSH_VERSION' @(#)LEGACY KSH R59 2024/02/01 +Debian

I think using bash as a workaround is the right choice here if all you have is AT&T ksh.

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA
rsbecker@nexbridge.com· Jul 14, 2024, 22:14 UTC · re: brian m. carlson · lore

RE: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

On Sunday, July 14, 2024 6:02 PM, brian m. carlson wrote:
Show 43 quoted lines
>On 2024-07-14 at 18:28:56, rsbecker@nexbridge.com wrote:
>> On Sunday, July 14, 2024 2:16 PM, brian m. carlson wrote:
>> >I don't think this is the right approach.  Every version of ksh
>> >_except_ AT&T ksh works just fine here.  pdksh, mksh, lksh, OpenBSD's
>> >ksh (which is also its /bin/sh) work fine, as do bash, dash,
>> >FreeBSD's sh (ash), Busybox's sh (also ash), and zsh (when run in sh
>> >mode with 5.9 or newer).  AT&T ksh is considering adding local in a newer
>version for this reason.
>> >
>> >Literally only AT&T ksh is not supported here, and so anyone can set
>> >SHELL_PATH to any suitable shell.  I don't think it's useful to get
>> >rid of local when there are a variety of acceptable and portable options.
>>
>> We can add NonStop's ksh to the list of not supported. I'm using
>TEST_SHELL_PATH while running make all in the t directory. Test passes when I use
>bash. For some reason (maybe GNUMake 4.1, which is what I have in my POSIX
>environment, I don't get TEST_SHELL_PATH passed down from the outer Makefile,
>but I can work with that. t0021 is now passing in my current CI stream using bash
>5.0.18.
>
>I think we had discussed that you were using AT&T ksh on NonStop, which would
>explain the situation.  That's the most common version of ksh on proprietary Unix
>systems, and you can usually detect it with something like this:
>
>% ksh -c 'echo $KSH_VERSION'
>Version AJM 93u+m/1.0.8 2024-01-01
>
>Variants of pdksh look like this (this one from OpenBSD):
>
>% ksh -c 'echo $KSH_VERSION'
>@(#)PD KSH v5.2.14 99/07/13.2
>
>And mksh and lksh (which are the same shell, just compiled differently, look like
>this:
>
>% mksh -c 'echo $KSH_VERSION'
>@(#)MIRBSD KSH R59 2024/02/01 +Debian
>
>% lksh -c 'echo $KSH_VERSION'
>@(#)LEGACY KSH R59 2024/02/01 +Debian
>
>I think using bash as a workaround is the right choice here if all you have is AT&T
>ksh.
We did discuss the ksh issue on NonStop but the check for whether this is an AT&T ksh is non-functional (KSH_VERSION is not set). What we did was change SHELL=/usr/coreutils/bin/bash and TEST_LINT= to get the tests to execute. Unfortunately, the GNU Make we have (4.1) does not pass TEST_SHELL_PATH (also set) down to inner make processes, so we still get stuck with ksh. I changed our CI system to run the test make on its own, which is now working on this test.

Regards, Randall

Junio C Hamano· Jul 15, 2024, 15:20 UTC · re: brian m. carlson · lore

Re: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> I think we had discussed that you were using AT&T ksh on NonStop, which
> would explain the situation.  That's the most common version of ksh on
> proprietary Unix systems, and you can usually detect it with something
> like this:

What is sad is that we have this as literally the very first thing in our test suite, as t0000.1, but ...

        try_local_xy () {
                local x="local" y="alsolocal" &&
                echo "$x $y"
        }
        # Check whether the shell supports the "local" keyword. "local" is not
        # POSIX-standard, but it is very widely supported by POSIX-compliant
        # shells, and we rely on it within Git's test framework.
        #
        # If your shell fails this test, the results of other tests may be
        # unreliable. You may wish to report the problem to the Git mailing
        # list <git@vger.kernel.org>, as it could cause us to reconsider
        # relying on "local".
        test_expect_success 'verify that the running shell supports "local"' '
                x="notlocal" &&
                y="alsonotlocal" &&
                echo "local alsolocal" >expected1 &&
                try_local_xy >actual1 &&
                test_cmp expected1 actual1 &&
                echo "notlocal alsonotlocal" >expected2 &&
                echo "$x $y" >actual2 &&
                test_cmp expected2 actual2
        '

... apparently it is just like any other test failure, so unless the tester is running

	$ shell t0000-basic.sh -i

reading the output, *AND* goes to the test script to read that comment, the helpful comment can easily be missed.

I am wondering if it is worth doing something like this.
 t/t0000-basic.sh | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git c/t/t0000-basic.sh w/t/t0000-basic.sh
index 98b81e4d63..3cb8243cb4 100755
--- c/t/t0000-basic.sh
+++ w/t/t0000-basic.sh
@@ -34,7 +34,7 @@ try_local_xy () {
 # unreliable. You may wish to report the problem to the Git mailing
 # list <git@vger.kernel.org>, as it could cause us to reconsider
 # relying on "local".
-test_expect_success 'verify that the running shell supports "local"' '
+test_lazy_prereeq WORKING_LOCAL '
 	x="notlocal" &&
 	y="alsonotlocal" &&
 	echo "local alsolocal" >expected1 &&
@@ -45,6 +45,17 @@ test_expect_success 'verify that the running shell supports "local"' '
 	test_cmp expected2 actual2
 '
 
+if ! test_have_prereq WORKING_LOCAL
+then
+	skip_all='
+		Your shell has no working "local", no tests will work.
+		You may wish to report the problem to the Git mailing
+		list <git@vger.kernel.org>, unless it is AT&T ksh,
+		which we know lacks "local".  In the meantime, use
+		shells that support "local", like dash, bash, pdksh...'
+	test_done
+fi
+
 ################################################################
 # git init has been done in an empty repository.
 # make sure it is empty.
rsbecker@nexbridge.com· Jul 15, 2024, 15:32 UTC · re: Junio C Hamano · lore

RE: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

On Monday, July 15, 2024 11:20 AM, Junio C Hamano wrote:
Show 8 quoted lines
>"brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> I think we had discussed that you were using AT&T ksh on NonStop,
>> which would explain the situation.  That's the most common version of
>> ksh on proprietary Unix systems, and you can usually detect it with
>> something like this:
>
>What is sad is that we have this as literally the very first thing in our
test suite, as
Show 8 quoted lines
>t0000.1, but ...
>
>        try_local_xy () {
>                local x="local" y="alsolocal" &&
>                echo "$x $y"
>        }
>
>        # Check whether the shell supports the "local" keyword. "local" is
not
>        # POSIX-standard, but it is very widely supported by
POSIX-compliant
Show 7 quoted lines
>        # shells, and we rely on it within Git's test framework.
>        #
>        # If your shell fails this test, the results of other tests may be
>        # unreliable. You may wish to report the problem to the Git mailing
>        # list <git@vger.kernel.org>, as it could cause us to reconsider
>        # relying on "local".
>        test_expect_success 'verify that the running shell supports
"local"' '
Show 11 quoted lines
>                x="notlocal" &&
>                y="alsonotlocal" &&
>                echo "local alsolocal" >expected1 &&
>                try_local_xy >actual1 &&
>                test_cmp expected1 actual1 &&
>                echo "notlocal alsonotlocal" >expected2 &&
>                echo "$x $y" >actual2 &&
>                test_cmp expected2 actual2
>        '
>
>... apparently it is just like any other test failure, so unless the tester
is running
>
>	$ shell t0000-basic.sh -i
>
>reading the output, *AND* goes to the test script to read that comment, the
helpful
Show 9 quoted lines
>comment can easily be missed.
>
>I am wondering if it is worth doing something like this.
>
>
> t/t0000-basic.sh | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
>diff --git c/t/t0000-basic.sh w/t/t0000-basic.sh index
98b81e4d63..3cb8243cb4
Show 5 quoted lines
>100755
>--- c/t/t0000-basic.sh
>+++ w/t/t0000-basic.sh
>@@ -34,7 +34,7 @@ try_local_xy () {
> # unreliable. You may wish to report the problem to the Git mailing  #
list
><git@vger.kernel.org>, as it could cause us to reconsider  # relying on
"local".
Show 6 quoted lines
>-test_expect_success 'verify that the running shell supports "local"' '
>+test_lazy_prereeq WORKING_LOCAL '
> 	x="notlocal" &&
> 	y="alsonotlocal" &&
> 	echo "local alsolocal" >expected1 &&
>@@ -45,6 +45,17 @@ test_expect_success 'verify that the running shell
supports
Show 18 quoted lines
>"local"' '
> 	test_cmp expected2 actual2
> '
>
>+if ! test_have_prereq WORKING_LOCAL
>+then
>+	skip_all='
>+		Your shell has no working "local", no tests will work.
>+		You may wish to report the problem to the Git mailing
>+		list <git@vger.kernel.org>, unless it is AT&T ksh,
>+		which we know lacks "local".  In the meantime, use
>+		shells that support "local", like dash, bash, pdksh...'
>+	test_done
>+fi
>+
> ################################################################
> # git init has been done in an empty repository.
> # make sure it is empty.

What is strange is that when running on NonStop using ksh, t0000.1 has never failed. I think the situation is subtly different from what we are solving. My take is that there is a difference in the local vs. non-local variable set semantic, rather than just accepting the keyword. I would propose that we need a more comprehensive local test to verify the actual expected semantics rather than just testing the syntax.

Junio C Hamano· Jul 15, 2024, 16:41 UTC · re: rsbecker@nexbridge.com · lore

Re: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

<rsbecker@nexbridge.com> writes:
Show 6 quoted lines
> What is strange is that when running on NonStop using ksh, t0000.1 has never
> failed. I think the situation is subtly different from what we are solving.
> My take is that there is a difference in the local vs. non-local variable
> set semantic, rather than just accepting the keyword. I would propose that
> we need a more comprehensive local test to verify the actual expected
> semantics rather than just testing the syntax.

It is possible that I may be misreading that first test, but as far as I can tell, it is testing not just the syntax but tests how the variables declared "local" behaves and should notice if they are not localized. It checks that "local" assignments in try_local_xy does take effect, and (more importantly) after try_local_xy returns, the original values are restored.

As I speculated earlier in an earlier message, the breakage you reported may have to do with interaction between "local" and use of a subshell, and perhaps we can also check that pattern in the test.

Thanks.
rsbecker@nexbridge.com· Jul 15, 2024, 17:39 UTC · re: Junio C Hamano · lore

RE: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

On Monday, July 15, 2024 12:42 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>> What is strange is that when running on NonStop using ksh, t0000.1 has
>> never failed. I think the situation is subtly different from what we are
solving.
Show 6 quoted lines
>> My take is that there is a difference in the local vs. non-local
>> variable set semantic, rather than just accepting the keyword. I would
>> propose that we need a more comprehensive local test to verify the
>> actual expected semantics rather than just testing the syntax.
>
>It is possible that I may be misreading that first test, but as far as I
can tell, it is
>testing not just the syntax but tests how the variables declared "local"
behaves and
>should notice if they are not localized.  It checks that "local"
assignments in
>try_local_xy does take effect, and (more importantly) after try_local_xy
returns, the
>original values are restored.
>
>As I speculated earlier in an earlier message, the breakage you reported
may have to
>do with interaction between "local" and use of a subshell, and perhaps we
can also
>check that pattern in the test.
That is that I am also suggesting but did not say it as precisely. Thanks.
Junio C Hamano· Jul 15, 2024, 18:33 UTC · re: rsbecker@nexbridge.com · lore

Re: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

<rsbecker@nexbridge.com> writes:
Show 7 quoted lines
>>As I speculated earlier in an earlier message, the breakage you reported
> may have to
>>do with interaction between "local" and use of a subshell, and perhaps we
> can also
>>check that pattern in the test.
>
> That is that I am also suggesting but did not say it as precisely. Thanks.

Oh, I see. Are you volunteering to come up with a minimum addition to t0000.1 then?

Thanks.
rsbecker@nexbridge.com· Jul 15, 2024, 19:03 UTC · re: Junio C Hamano · lore

RE: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

On Monday, July 15, 2024 2:34 PM, Junio C Hamano wrote:
Show 11 quoted lines
><rsbecker@nexbridge.com> writes:
>
>>>As I speculated earlier in an earlier message, the breakage you
>>>reported
>> may have to
>>>do with interaction between "local" and use of a subshell, and perhaps
>>>we
>> can also
>>>check that pattern in the test.
>>
>> That is that I am also suggesting but did not say it as precisely.
Thanks.
>
>Oh, I see.  Are you volunteering to come up with a minimum addition to
t0000.1
>then?

I'll have to give this some thought on the best way to verify local that does not break the various other shells. I only have a limited set (ksh, bash). Maybe someone with a linux system might be in a better position.

Junio C Hamano· Jul 15, 2024, 15:05 UTC · re: brian m. carlson · lore

Re: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> I don't think this is the right approach.  Every version of ksh
> _except_ AT&T ksh works just fine here. ... AT&T ksh is
> considering adding local in a newer version for this reason.

Thanks. That is nice to know, though unfortunately I didn't see signs of them making much progress.

Let's not make or change any policy but just document what we found to help the next person.

------ >8 ----------- >8 ----------- >8 ------
Subject: [PATCH] doc: note that AT&T ksh does not work with our test suite

The scripted Porcelain commands do not allow use of "local" because it is not universally supported, but we use it liberally in our test scripts, which means some POSIX compliant shells (like "ksh93") can not be used to run our tests.

Document the status quo, to help the next person who gets perplexed seeing our tests fail.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 1d92b2da03..94ca5cf7c0 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -185,8 +185,8 @@ For shell scripts specifically (not exhaustive):
 
  - Even though "local" is not part of POSIX, we make heavy use of it
    in our test suite.  We do not use it in scripted Porcelains, and
-   hopefully nobody starts using "local" before they are reimplemented
-   in C ;-)
+   hopefully nobody starts using "local" before all shells that matter
+   support it (notably, ksh from AT&T Research does not support it yet).
 
  - Some versions of shell do not understand "export variable=value",
    so we write "variable=value" and then "export variable" on two

-- 
2.46.0-rc0-140-g824782812f
Eric Sunshine· Jul 14, 2024, 21:27 UTC · re: Junio C Hamano · lore

Re: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

On Sun, Jul 14, 2024 at 1:00 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> Subject: doc: guide to use of "local" shell language construct
>
> The scripted Porcelain commands do not allow use of "local" because
> it is not universally supported, but we use it liberally in our test
> scripts, which means some POSIX compliant shells (like "ksh93") can
> not be used to run our tests.
>
> Document the status quo, and hint that we might want to change the
> situation in the fiture.
s/fiture/future/
Show 11 quoted lines
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
> @@ -186,7 +186,9 @@ For shell scripts specifically (not exhaustive):
>   - Even though "local" is not part of POSIX, we make heavy use of it
>     in our test suite.  We do not use it in scripted Porcelains, and
>     hopefully nobody starts using "local" before they are reimplemented
> -   in C ;-)
> +   in C ;-) Notably, ksh (not just reimplementations but the real one
> +   from AT&T Research) does not support "local" and cannot be used,
> +   which we might want to reconsider.

The last bit ("which we might want to reconsider") probably belongs in the commit message rather than here in the actual documentation. Saying "we might want to reconsider" doesn't help people new to the project who are looking for guidance _today_.

Show 9 quoted lines
> diff --git c/t/README w/t/README
> @@ -850,6 +850,14 @@ And here are the "don'ts:"
> + - Don't overuse "local"
> +
> +   Because strictly POSIX-compliant shells do not have to support
> +   "local", we avoid using it in our scripted Porcelain scripts, but
> +   we have allowed use of "local" in test scripts.  We may want to
> +   reconsider this and rewrite our tests to also run on shells like
> +   ksh93.  Do not add new use of "local" unnecessarily.

Same comment regarding "We may want to reconsider..."; it probably belongs in the commit message, not here.

rsbecker@nexbridge.com· Jul 14, 2024, 13:58 UTC · re: Junio C Hamano · lore

RE: [Test Breakage 2.46.0-rc0] Test t0021.35 fails on NonStop

On Saturday, July 13, 2024 10:54 PM, Junio C Hamano wrote:
Show 17 quoted lines
><rsbecker@nexbridge.com> writes:
>
>> ok 34 # skip delayed checkout shows progress by default on tty
>> (pathspec
>> checkout) (missing TTY of PERL,TTY)
>>
>> expecting success of 0021.35 'delayed checkout ommits progress on
>> non-tty (pathspec checkout)':
>>                 test_delayed_checkout_progress ! git checkout $opt
>>
>> + test_delayed_checkout_progress ! git checkout .
>> test_delayed_checkout_progress: test: argument expected
>
>This "test: argument expected" is curious.
>
>The shell function reads like so and it hasn't changed since
>7a132c62 (checkout: make delayed checkout respect --quiet and
--no-progress,
Show 14 quoted lines
>2021-08-26) introduced it, and there are 3 uses of "test" builtin function.
>
>        test_delayed_checkout_progress () {
>                if test "$1" = "!"
>                then
>                        local expect_progress=N &&
>                        shift
>                else
>                        local expect_progress=
>                fi &&
>
>                if test $# -lt 1
>                then
>                        BUG "no command given to
test_delayed_checkout_progress"
Show 10 quoted lines
>                fi &&
>
>                (
>                        cd progress &&
>                        GIT_PROGRESS_DELAY=0 &&
>                        export GIT_PROGRESS_DELAY &&
>                        rm -f *.a delay-progress.log &&
>
>                        "$@" 2>err &&
>                        grep "IN: smudge test-delay10.a .* \\[DELAYED\\]"
delay-progress.log
Show 11 quoted lines
>&&
>                        if test "$expect_progress" = N
>                        then
>                                ! grep "Filtering content" err
>                        else
>                                grep "Filtering content" err
>                        fi
>                )
>        }
>
>I do not see how any of these tests can be fed a list of zero or more
argument that
Show 9 quoted lines
>causes it to barf with "argument expected", when
>
>	test_delayed_checkout_progress ! git checkout .
>
>is run.  The first "are we negated?" test gets
>
>	test "!" = "!"
>
>and shifts that "!" away, the second test that checks the number of
arguments do
>not need any extra quoting (in other words, $# can never be an empty
string, so
Show 9 quoted lines
>
>	test $# -lt 1
>
>cannot become "test -lt 1" that would barf, like so:
>
>	$ ksh -c 'test -lt 1'
>	ksh: test: argument expected
>
>And whether expect_progress gets an empty string or N (we should have got N
in
>there, if the shell is so broken that the first comparison between "!" and
"!" were
>botched and the variable got assigned an empty string, or "local" were
botched and
>the variable were somehow left uninitialized, expect_progress could be a
value we
>do not expect), the last test to switch between !grep and grep can never
break like
Show 8 quoted lines
>so:
>
>	$ ksh -c 'test = N'
>	ksh: test: argument expected
>
>because $expect_progress is inside a pair of double quotes.
>
>In the above "thinking out aloud", I invoked ksh because bash and dash
gives
>different error messages.
>
>Another remotely curious thing is that I am not sure how "local" in the
outer shell
>influences the use of the variable in the subshell
>(read: now I am speculating if the above code uses some constructs that are
so
>tricky to implement correctly and an unfortunate bug gets triggered in your
shell),
>but even if such a bug caused a nonsense value to be assigned to
$expect_progress,
Show 5 quoted lines
>I do not see how it can cause "test" to barf with "argument expected".
>
>So, I am not sure what is going on at all.
>
>In any case, this does not sound anything recent.  Do you know that this
used to
>work in previous releases?  Could this be caused by changes in your
environment
>(like which shell is used)?

What is weird is that this worked fine in previous releases. We did not change operating system versions or shells since...well, this test box was built a year ago. The only significant change recently was the TZ variable. Prior to that, back last fall was the introduction of IPv6. Other than that, things are pretty stable. What I did add to our CI script just prior to 2.46.0-rc0 is GIT_TEST_DEFAULT_REF_FORMAT=reftable GIT_TEST_CLONE_2GB=true to get the related tests to run. I specifically need to test 2Gb clones because that is happening a lot on platform.

The semantics of local in ksh is different than in bash, so it looks like I have to force down the bash path. However, my CI script also runs make test with TEST_LINT= SHELL=/usr/coreutils/bin/bash. My assumption was that this would force bash globally. Apparently that is not the case and ksh is being used inside. I don't like having to run the test manually afterwards.

--Randall

← back to recent threads