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

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

[rsbecker]

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.

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

[Junio C Hamano]

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

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

[rsbecker]

On Saturday, July 13, 2024 10:54 PM, Junio C Hamano wrote:
><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,
>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)?

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

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

[Junio C Hamano]

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

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

[rsbecker]

On Sunday, July 14, 2024 1:00 PM, 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.
>
>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
> --------------

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.

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

[brian m. carlson]

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

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.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

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

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

[rsbecker]

On Sunday, July 14, 2024 2:16 PM, brian m. carlson wrote:
>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

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

[brian m. carlson]

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

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.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

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

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

[rsbecker]

On Sunday, July 14, 2024 6:02 PM, brian m. carlson wrote:
>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

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

[Junio C Hamano]

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

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

[rsbecker]

On Monday, July 15, 2024 11:20 AM, Junio C Hamano wrote:
>"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.

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.

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

[Junio C Hamano]

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

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

[rsbecker]

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

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

[Junio C Hamano]

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

Thanks.

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

[rsbecker]

On Monday, July 15, 2024 2:34 PM, Junio C Hamano wrote:
><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.

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

[Junio C Hamano]

"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

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

[Eric Sunshine]

On Sun, Jul 14, 2024 at 1:00 PM Junio C Hamano <gitster@pobox.com> wrote:
> 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/

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

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

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

[rsbecker]

On Saturday, July 13, 2024 10:54 PM, Junio C Hamano wrote:
><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,
>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)?

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