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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Apr 1, 2023, 02:39 UTC
Message-ID: <CAMP44s0eQqvDTevs_tNqnc4Z_ZaOb4PoEXdSFmXxJn9Nyd5Emg@mail.gmail.com>
In-Reply-To: <xmqqv8igbd8e.fsf@gitster.g>

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

Show 10 quoted lines

>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> >> 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.
> >
> > That is not true.
>
> We probably should agree to disagree.

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.

>Let me respond by picking the first example from your message and then stop.

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.

Show 11 quoted lines

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

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.

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

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

> But still, it *is* forcing our developers to learn one more rule.

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.

Show 6 quoted lines

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

zsh is the default in macOS.

Is macOS less important than some distros?

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

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.

Show 5 quoted lines

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

Nothing to do with the patch.

> I however do not see "don't use path or status or options or 0 as
> shell variable names" falls into the same category

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.

-- 
Felipe Contreras