patch, 6 partstest: make the test suite work with zsh

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.

Felipe Contreras (6):
  test: fix build for zsh
  test: avoid `stat` variable
  test: avoid `options` variable
  test: avoid `path` variable
  test: hack for zsh
  mergetools: vimdiff: check for empty fields

 mergetools/vimdiff               |  4 +--
 t/annotate-tests.sh              | 10 +++----
 t/lib-bash.sh                    |  2 +-
 t/t0001-init.sh                  |  4 +--
 t/t0003-attributes.sh            | 16 +++++------
 t/t1450-fsck.sh                  | 16 +++++------
 t/t3305-notes-fanout.sh          | 12 ++++----
 t/t3432-rebase-fast-forward.sh   |  4 +--
 t/t4013-diff-various.sh          |  6 ++--
 t/t4046-diff-unmerged.sh         | 49 ++++++++++++++++----------------
 t/t4051-diff-function-context.sh |  4 +--
 t/t5329-pack-objects-cruft.sh    |  8 +++---
 t/t5512-ls-remote.sh             |  4 +--
 t/t5516-fetch-push.sh            |  8 +++---
 t/t9300-fast-import.sh           |  4 +--
 t/test-lib.sh                    | 24 ++++++++++------
 16 files changed, 92 insertions(+), 83 deletions(-)

It has a special meaning in zsh.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/annotate-tests.sh              | 10 +++++-----
 t/t4051-diff-function-context.sh |  4 ++--
 t/t9300-fast-import.sh           |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/lib-bash.sh |  2 +-
 t/test-lib.sh | 20 ++++++++++++++------
 2 files changed, 15 insertions(+), 7 deletions(-)

See: https://www.zsh.org/mla/workers/2023/msg00336.html

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t4046-diff-unmerged.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On Tue, Mar 28, 2023 at 11:39 AM Felipe Contreras <felipe.contreras@gmail.com> wrote:

In case anyone is interested this has just been fixed:

https://github.com/zsh-users/zsh/commit/12e5db145b098a62ff11b88eea26f473ea2ecdcf

In zsh 'foo#bar#' produces an empty field at the end.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

It has special meaning in zsh.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t0001-init.sh                | 4 ++--
 t/t3432-rebase-fast-forward.sh | 4 ++--
 t/t4013-diff-various.sh        | 6 +++---
 t/t5512-ls-remote.sh           | 4 ++--
 t/t5516-fetch-push.sh          | 8 ++++----
 t/test-lib.sh                  | 4 ++--
 6 files changed, 15 insertions(+), 15 deletions(-)

On Tue, Mar 28 2023, Felipe Contreras wrote:

Nit: If we are going to rename these I'd prefer (just from grepping
existing examples) $exit_code, $code, $ret etc. a "$stat" at a glance
would seem to refer to stat() somehow, which is confusing.

On Tue, Mar 28, 2023 at 11:39:28AM -0600, Felipe Contreras wrote:

Maybe I'm misreading your patch or its subject line, but all of these hunks convert from something (usually "status") _to_ "stat". Is the point to avoid "status", not "stat"? Or the other way around?

Thanks, Taylor

On Fri, Mar 31, 2023 at 6:05 PM Taylor Blau <me@ttaylorr.com> wrote:

No, the title is wrong; the purpose is to avoid `status`, which in zsh it's linked to $?

In zsh it's linked to `PATH`.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t0003-attributes.sh         | 16 ++++++------
 t/t1450-fsck.sh               | 16 ++++++------
 t/t3305-notes-fanout.sh       | 12 ++++-----
 t/t4046-diff-unmerged.sh      | 48 +++++++++++++++++------------------
 t/t5329-pack-objects-cruft.sh |  8 +++---
 5 files changed, 50 insertions(+), 50 deletions(-)

On 2023-03-28 at 17:39:26, Felipe Contreras wrote:

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:

  t4046-diff-unmerged.sh                           (Wstat: 256 (exited 1) Tests: 6 Failed: 5)
    Failed tests:  2-6
    Non-zero exit status: 1
  t7609-mergetool--lib.sh                          (Wstat: 256 (exited 1) Tests: 1 Failed: 1)
    Failed test:  1
    Non-zero exit status: 1

I haven't investigated further.

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. It would also be easier to potentially test in the testsuite as well.

On Tue, Mar 28, 2023 at 6:57 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

That would defeat my motivation behind the patches, which is to be able to run one test file in zsh.

Only the first patch is needed for that, the rest were in case anyone cared to run all the tests.

We don't need to carry the patches if the patches are applied.

On Tue, Mar 28 2023, Felipe Contreras wrote:

"One" as in one specific file you have in mind, or a "one-off run"? 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?

If it's the latter, I don't really see the point of making just some of it work with zsh's native mode (if I'm getting this correctly).

But for that case, wouldn't:

	zsh --some-options t0001-init.sh

Or whatever work, which just from skimming the help might be some of the --posix options? But I can see that being more of a hassle, presumably you want to use it as /bin/sh, and to have it pick up the script and have it Just Work.

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

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.

And, to avoid the inevitable re-breakage have this tested in CI, usurping 1/2 OSX jobs for that might be a good target (or one of the other linux jobs)...

On Wed, Mar 29, 2023 at 3:58 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

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.

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

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.

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.

On Wed, Mar 29, 2023 at 5:19 AM Felipe Contreras <felipe.contreras@gmail.com> wrote:

Actually, that isn't even true (see below).

But in this particular case the exact opposite is true: the script is *not* POSIX, it just happens to work on bash and other shells.

You *assume* it's POSIX because it works on bash and it doesn't work on zsh, but in this particular case bash is the non-POSIX one, zsh is following POSIX correctly.

Actually, no. I've changed my mind.

I was going to report to the zsh dev mailing list the fact that this created an extra empty field at the end (in sh mode):

  IFS=, ; str='foo,bar,,roo,'; printf '"%s"\n' $str

But then I read the POSIX specification, and the section 2.6.5 Field Splitting [1] is very clear on what should happen.

What muddles the waters is the distinction between `IFS white space` characters (newline, space and tab), and non-`IFS white space` characters (all the other).

If we ignore all the shite space stuff and concentrate on the rules for non-`IFS white space` characters (as comma is), then we arrive at this subitem:

3.b. "Each occurrence in the input of an IFS character that is not IFS white space, along with any adjacent IFS white space, shall delimit a field, as described previously."

In other words: each occurence of a non-`IFS white space` character shall delimit a field. Or: each occurence of a comma should delimit a field.

The script only works if the last delimiter does *not* delimit a field, and thus it's not following POSIX, it just happens to work on most shells. My patch does make it align with POSIX.

I've reported bash's non-compliance with POSIX to their mailing list [2].

But I bet nobody here will care, because POSIX is just an excuse to segregate the shells the main developers want to make work, from the ones they are not (Brian even used the language of certain shells being one of "the good ones").

Cheers.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_05 [2] https://lists.gnu.org/archive/html/bug-bash/2023-03/msg00152.html

-- Felipe Contreras

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

While these patches may make it "work" with zsh in its native mode, because zsh that is running in its native mode is sufficiently distant from the more POSIXy portable variants of Bourne shells like dash and bash, I find it hard to justify the cost of maintaining the resulting codebase to "work" that way.

We need to force our developers to pay attention to rules set by zsh, like "$0 is not the way to spell the name of the script", "$status is not usable by end-users", etc. Making the problem worse, the rules are enforced only by zsh. The situation is similar to the "when to use $(pwd) and when to use $PWD?" rules, which folks like me without Windows have to spend extra minutes to remember every time they use one of these constructs, but the number of rules seem to be much larger as these patches seem to show.

Thanks.

On Wed, Mar 29, 2023 at 9:34 AM Junio C Hamano <gitster@pobox.com> wrote:

Why do we follow POSIX anyway?

Is it a) because following POSIX is just good? Or b) because POSIX is a standard that many shells attempt to follow and thus by following POSIX we maximize the amount of code that works on most shells?

A quick non-exhaustive search of the history of Git, shows these:

74ec8cf674 Conform to POSIX (works in mksh/lksh) c5c39f4e34 Fix for dash 7d661e5ed1 Conform with POSIX (works with dash and mksh) ddf3a1152d Remove a warning in bash 9b67c9942e Workaround for ksh 5c63920190 Consideration for some shells (e.g. ksh93) e9980419cb Fails in ksh88 268ef4d3d0 Aligns with POSIX 8e98b35f87 ksh88 on AIX considers it a syntax error 8f92c7755e Code would break under mksh b082687cba Not allowed in mksh 661bfd13b4 Fails in xpdg4/sh and ksh f5799e05c0 Public domain ksh and ksh on IRIX 6.5 have a problem 365c2aaafc Doesn't work in pdksh, POSIX unclear 482ce70e14 Doesn't work in dash and ash 77e572653b Doesn't work in dash 0e418e568f Workaround for ksh 49a43f5468 ksh doesn't like this c289c315c2 Doesn't work in xpg4/sh 933e4f090d Workaround possible dash bug c7053aa88f Doesn't seem to be supported by dash

It does seem like in only around 15% of these patches the consideration was "this aligns with POSIX", in most cases the patch was to make the code work for a particular shell.

In most cases the mention of POSIX is used as a justification for the proposed code, but it was clear the intention was to make it work for a specific shell (usually ksh).

In the few cases where POSIX was the main rationale, the shells in which the current code doesn't work are mentioned. Not a single patch says "this aligns with POSIX" without mentioning any shell.

No, we don't. $0 is not something a test script needs. In fact, there doesn't seem to be a single instance in which $0 is used except test infrastructure (e.g. test-lib.sh)

This sounds like a rationalization post-decision rather than a reason for the decision.

Looking at the history it seems the test code has been more than happy to make accommodations for xpg4/sh, pdksh, ash, and even mksh, even when the code was already POSIX-compliant.

In truth all the patches regarding shell portability have been along the lines of: "this code makes $x shell work, doesn't break other shells, and isn't against POSIX". In some cases even when the Austin group disagreed on what POSIX actually said, we did whatever worked in most shells.

Is there some sort of predisposition against zsh?

Cheers.

Felipe Contreras <felipe.contreras@gmail.com> writes:

It is not what we follow that is at the primary issue.

The criteria is more about what our developers are expected to be familiar with, and what is reasonable to force our developers to become sufficiently familiar with. Even among POSIXy Bourne variants, dash being stricter than bash already gives some new folks, who only know bash, things to learn to avoid. But at least what they learn through such an effort would help them write more portable scripts. It is my impression, however, that zsh in its native mode is even further out and away, pushing it on the other side of the line of being reasonable to force our develoerps to adjust to. What they will learn through such an effort would be more of "what to do when you are forced to use zsh" than "how to write your shell script portably".

One aspect that is missing in the above is the extra burden on our developers.

There isn't. If somebody makes the tests to also work with csh or /bin/sh on SunOS, I would pretty much say the same thing. The end-result may work perfectly well, but the cost to maintain it working over the long haul may be too high for our developers.

On Thu, Mar 30, 2023 at 4:15 AM Junio C Hamano <gitster@pobox.com> wrote:

That is not true.

This patch 77e572653b (t0050: fix printf format strings for portability, 2010-12-21) fixed a problem that was specific with dash. Did our developers have to learn the details of such issue? No.

How about f5799e05c0 (git-submodule.sh: separate parens by a space to avoid confusing some shells, 2011-05-26)? Did anybody have to learn that `((foo` causes problems in ksh in order to apply the patch?

No, these patches fixed specific issues for specific shells, and did not affect what our developers were expected to be familiar with.

Just because it's your impression doesn't mean it's true.

No, they don't have to learn anything, just like they don't have to learn that ksh93 returns signals that are 256+n: 0e418e568f (t0005: work around strange $? in ksh when program terminated by a signal, 2010-07-09).

You just picked the patch that fixed a specific problem for a specific shell, and we all forgot about it.

Except you did pick the patches that fixed specific issues for these shells when the existing code was already compliant with POSIX.

-- Felipe Contreras

On Thu, Mar 30, 2023 at 08:19:08AM -0600, Felipe Contreras wrote:

Sure, but zsh's incompatibility with bash is clear evidence that it is further out from the POSIX standard. But I think the argument is missing the point, anyway, which is that it's incompatible with POSIX.

Is it worth adapting our explicitly POSIX-compatible test suite to work with a non-POSIX shell? I think not.

Thanks, Taylor

On Fri, Mar 31, 2023 at 6:04 PM Taylor Blau <me@ttaylorr.com> wrote:

No, the POSIX standard is not whatever bash does; bash does many things that are not part of POSIX, and it even has a POSIX mode precisely because it does not follow it to a tee: `bash --posix`.

If `ksh` does something that bash does differently, is that evidence that `ksh` is further out from the POSIX standard?

This argument does not make sense.

So is bash.

All shells in the world are incompatible with the POSIX standard in one way or another.

POSIX doesn't say anything about the $COLUMNS variable, and yet bash sets it (and many other variables). Does that mean bash is "non-POSIX"?

Of course not; POSIX doesn't say "thou shall not set the $COLUMNS environment variable", so it's OK for bash to do that, it's still compatible with posix (as long as you use `--posix`).

This's just not how standards work.

All shells are non-POSIX.

Felipe Contreras <felipe.contreras@gmail.com> writes:

We probably should agree to disagree. Let me respond by picking the first example from your message and then stop.

The code before that change was feeding "\xc3\xa4" to printf, expecting that it would be an acceptable way to spell hexadecimal byte values, which was wrong. The commit improved portability by rewriting them to spell the same byte values in octal.

Yes, our developers have to learn to avoid hexadecimal byte values and the commit serves as a reminder for them to learn from. When a developer writes printf format with '\xCC' hexadecimal, reviewers would need to catch it as a mistake, and that commit makes a good reference why we insist on such a rule.

By learning that practice, our developers will be trained to write scripts that not just work with bash but also with dash, which is a small good thing. But still, it *is* forcing our developers to learn one more rule.

There is a trade off: is it worth supporting dash by forcing our developers to stick to the rule to write octal and not hex? dash is used as the default for some distros and considered one of the standard ones, and is worth supporting even if we need to stay away from some stuff people may have picked up from other shells like bash.

If it were a different shell, the equation may have been different. If industry standards like POSIX.1 required supporting hex literals, the equation may also have been different.

	Note: though, we do not blindly say "it is in POSIX, your
        shell behaves differently and we won't support it".

In any case, once we declare that "we aim for our scripts and tests to work with dash and bash and these other shells", our developers are forced to stick to intersection of these supported ones.

It takes a judgement call. And "don't write literals you feed printf in hex, instead do so in octal, because printf built into some shells do not like it" is something reasonable to force our developers to stick to (as it allows "dash" to be thrown into the set of supported shells). I however do not see "don't use path or status or options or 0 as shell variable names" falls into the same category (even though it may allow "zsh" in native mode to be included into the supported shells). Do benefits outweigh downsides?

And somebody has to draw that line. Judgement calls are just that. There are no absolutely right or wrong answers and they will not please everybody. Some folks may not agree with where the line gets drawn. Tough.

But the job of maintainer is not about being loved by everybody. Just drawing the line somewhere in order to allow folks to move on, without having everybody dragged into and getting stuck in endless arguments, in which there is no absolute right or wrong. That is what needs to be done, and that is what I just did.

On Fri, Mar 31, 2023 at 7:30 PM Junio C Hamano <gitster@pobox.com> wrote:

You can decide to disagree with a fact, but it's still a fact.

It is a fact that portability fixes have been merged to the code with zero consideration to what "our developers" would have to be "sufficiently familiar with" (which is very little, if anything).

I provided dozens of examples, and in this particular response chose to only pick two.

Sure, but that's just *one* example of many that prove what I'm saying is a fact, and I explain below it doesn't apply.

Even if that were true (which it isn't), that has *absolutely nothing* to do with the patch in question.

Developers would have to avoid that practice because it breaks dash, not because you picked the patch.

This is a red herring: merging or not merging the patch makes no difference to the practice.

And this is a prescriptive claim from you. *You* say that our developers have to learn that practice, and that's your opinion of what our developers should do. But that doesn't mean that our developers did actually learn that.

Yes, according to you, and regardless of whether or not you merge the patch.

Not true. The fact that such code breaks dash is what is forcing our developers to learn that rule, not you merging the patch.

Most developers would not even notice that you merged the patch. And such practice would not be shared in a memo, notes from the maintainer, or update of the CodingGuidelines.

You may be expecting such practice from developers after you merge such patch, but that's very different from developers actually knowing that they have to adopt this practice.

zsh is the default in macOS.

Is macOS less important than some distros?

Once again: red herring. Merging a patch has absolutely nothing to do with you declaring such an aim.

And if that were true, there would be a declaration of "Git now supports dash in its test suite".

When did such a declaration take place?

Moreover, the declaration has nothing to do with the patch.

Nothing to do with the patch.

Another red herring: because this is not something that anybody is suggesting to actually do.

---

The fact is that $0 is used nowhere in the tests.

You can decide to disagree with what `git grep '$0' t/t????-*.sh` shows, but it's an unobjectionable fact.

Not that it matters, because I just sent another patch with `emulate sh -o POSIX_ARGZERO`, which doesn't require changing $0, so you'll need another rationale to not support zsh.

Cheers.

On Thu, Mar 30, 2023 at 03:15:48AM -0700, Junio C Hamano wrote:

Well said.

Having to remember that we need to write "$ARGZERO" instead of "$0" is yet another burden to place on developers who want to make changes to the test suite.

Is that a big deal? Probably not. But it's a slippery slope, and a weird gotcha to remember when dealing with our otherwise POSIX-y test suite.

So I'm not convinced that it's a worthwhile price to pay for zsh support in our test suite.

Thanks, Taylor

On Fri, Mar 31, 2023 at 6:00 PM Taylor Blau <me@ttaylorr.com> wrote:

When have you ever used "$0" in the test suite?

A quick grep shows zero results (`git log --author=me@ttaylorr.com -S'$0'`), so I think you are talking about a hypothetical, not something that would actually happen in reality.

Sometimes preemptive optimization pays off in the future, but other times it doesn't, and it's just wasted mental effort.

This is one of those times when worrying about a future that will never happen in reality does not pay off.

You won't have to remember that because you'll never use $0 in the test suite. Nobody does use it, and nobody ever will.

*If* for some weird reason somebody needs to use $0, we can worry about it *then*.

---

But this is a red herring. The reality is that developers do not have to worry about every little aspect of the test suite. When somebody uses `seq`, somebody else reminds them to use `test_seq` instead, and if for some reason a `seq` slips by and it breaks the test in some obscure platform, the test is updated to fix that. It's not a big deal.

Why are many tests using `chmod` instead of `test_chmod`? Did the introduction of `test_chmod` imply an extra burden to "our developers" to remember using that instead of `chmod`? No, *in reality* what everyone cares about is that the test runs on the platforms of the real world.

If we could run the test suite on 100 hypothetical platforms that don't exist in the real world, it would break all over the place.

In reality all the portability considerations of the test suite are geared towards certain platforms that exist in the real world. Nobody cares that the test suite doesn't run on some hypothetical platforms.

So no, nobody needs to remember to use test_chmod, or test_seq, and nobody will ever have to remember to use $ARGZERO instead of $0, and if some hypothetical person does use that in some hypothetical test and that breaks the tests for zsh in native mode, nobody will care, except the person running those tests (likely me), which would promptly fix that single hypothetical instance of $0.

Nobody will die if a instance of $0 slipped by (which will never happen).

This is simply not a real consideration.

Cheers.

On 2023-03-29 at 15:34:26, Junio C Hamano wrote:

I think that's a fine position to take. We have found some fun bugs in our testsuite via zsh's sh mode, and zsh (in sh mode) is a valid shell according to Debian's /bin/sh rules, so I think it's fair to address shell fixes for it in sh mode where we can, even though it sounds like we'd prefer to skip the zsh mode.

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Yeah, as long as the sh mode is sufficiently compatible, I would be happy to see the shell supported as one of the /bin/sh we can use, and I agree that it is fair to address shell portability fixes for zsh running in the sh compatible mode.

If the native mode didn't impose too much burden on our developers' to maintain, it would have been nicer, but judging from the contents of these patches, I am afraid that it falls on the other side of the line.

Thanks.

On Wed, Mar 29, 2023 at 9:15 PM Junio C Hamano <gitster@pobox.com> wrote:

There is zero burden to maintain.

What burden did this fix specific for ksh93 cause to "our developers" 0e418e568f (t0005: work around strange $? in ksh when program terminated by a signal, 2010-07-09)?