Re: [PATCH 0/6] test: make the test suite work with zsh

From: Felipe Contreras <felipe.contreras@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: "brian m. carlson" <sandals@crustytoothpaste.net>,
	git@vger.kernel.org, "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>
Subject: Re: [PATCH 0/6] test: make the test suite work with zsh
Date: Wed, 29 Mar 2023 05:19:27 -0600	[thread overview]
Message-ID: <CAMP44s1z6PBS8whv6+nhbzQr-H2+QYTDw=WHf0AkY2mbJDZfMA@mail.gmail.com> (raw)
In-Reply-To: <230329.861ql7x4k4.gmgdl@evledraar.gmail.com>

On Wed, Mar 29, 2023 at 3:58 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Tue, Mar 28 2023, Felipe Contreras wrote:
>
> > On Tue, Mar 28, 2023 at 6:57 PM brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> >>
> >> On 2023-03-28 at 17:39:26, Felipe Contreras wrote:
> >> > It's not difficult to make the testing library work for zsh, so I did that in
> >> > the first patch.
> >> >
> >> > The rest of the patches are basically to deal with some variables that are
> >> > special in zsh, workaround a bug, and a minor discrepancy.
> >>
> >> There was a point at which the tests worked entirely in sh mode with
> >> zsh.  I know because I fixed a handful of tests there, ending with
> >> c64368e3a2a47, and I patched zsh to run all commands in a pipeline in a
> >> subshell in sh mode to fix the remaining tests.
> >>
> >> If I symlink zsh (zsh 5.9 (x86_64-debian-linux-gnu)) to sh in a
> >> temporary directory and use it in SHELL_PATH, I get only the following
> >> failures:
> >
> > That would defeat my motivation behind the patches, which is to be
> > able to run one test file in zsh.
>
> "One" as in one specific file you have in mind, or a "one-off run"?

One specific file.

I just did a quick porting of the code in my fork, and this should
give you a good idea (it's preliminary):

https://github.com/felipec/git/blob/zsh/tests/t/t9904-completion-zsh.sh

I had already sent versions of this test file that run in bash, but
it's much better to fix test-lib.sh so that it works for zsh and they
can be run directly.

> The
> 1/6 here looks like it fixes most of the issues, but e.g. the
> test-lib.sh fix in 2/6 would be needed by any test that reached that
> code, wouldn't it?

Yes, but my test file doesn't reach that code.

> Some details on all of this in an updated commit message would be most
> welcome...

Well, those details depended on the response.

Since nobody seems to be interested in the tests running with vanilla
zsh, I can just drop them and the explanation.

> > Only the first patch is needed for that, the rest were in case anyone
> > cared to run all the tests.
> >
> >> I don't care a lot of other folks want to make zsh run the testsuite in
> >> zsh mode, but I'd think that using sh mode would be simpler and less
> >> likely to break in general, and would avoid us needing to carry a lot of
> >> patches to work around various variables that are special in zsh mode.
> >
> > We don't need to carry the patches if the patches are applied.
>
> But we do need to carry some hacks going forward, some of it seems
> pretty isolated & easy to spot, but e.g. the 6/6 fix of:
>
> -               if test "$c" = " "
> +               if test "$c" = " " || test -z "$c"
>
> Is quite subtle, you might look at that and be convinced that the RHS is
> redundant, and be right, but only because you assume POSIX semantics.
>
> If we are going to include this I think the relevant t/README and
> Documentation/CodingGuidelines parts should be updated to note that
> we're not targeting POSIX shellscripts anymore, but the subset of it
> that zsh is happy with.

There's no point in that. I consider it a bug in zsh, along with 5/6,
so presumably at some point it's going to be fixed.

And if nobody cares about running these tests in zsh, it doesn't matter anyway.

Cheers.

-- 
Felipe Contreras