git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Felipe Contreras <felipe.contreras@gmail.com>
Cc: "brian m. carlson" <sandals@crustytoothpaste.net>,
	git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"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: Fri, 31 Mar 2023 18:30:41 -0700	[thread overview]
Message-ID: <xmqqv8igbd8e.fsf@gitster.g> (raw)
In-Reply-To: CAMP44s3xVL0UHCHh2Ei=STTx=grkpvUTfj6o9roe3tL35GhG4Q@mail.gmail.com

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.  Let me respond by picking the
first example from your message and then stop.

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

  parent reply	other threads:[~2023-04-01  1:31 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-28 17:39 [PATCH 0/6] test: make the test suite work with zsh Felipe Contreras
2023-03-28 17:39 ` [PATCH 1/6] test: fix build for zsh Felipe Contreras
2023-03-28 17:39 ` [PATCH 2/6] test: avoid `stat` variable Felipe Contreras
2023-03-29  9:48   ` Ævar Arnfjörð Bjarmason
2023-04-01  0:05   ` Taylor Blau
2023-04-01  0:25     ` Felipe Contreras
2023-03-28 17:39 ` [PATCH 3/6] test: avoid `options` variable Felipe Contreras
2023-03-28 17:39 ` [PATCH 4/6] test: avoid `path` variable Felipe Contreras
2023-03-28 17:39 ` [PATCH 5/6] test: hack for zsh Felipe Contreras
2023-03-30  8:15   ` Felipe Contreras
2023-03-28 17:39 ` [PATCH 6/6] mergetools: vimdiff: check for empty fields Felipe Contreras
2023-03-29  0:57 ` [PATCH 0/6] test: make the test suite work with zsh brian m. carlson
2023-03-29  1:57   ` Felipe Contreras
2023-03-29  9:51     ` Ævar Arnfjörð Bjarmason
2023-03-29 11:19       ` Felipe Contreras
2023-03-30 13:00         ` Felipe Contreras
2023-03-29 15:34   ` Junio C Hamano
2023-03-29 21:54     ` Felipe Contreras
2023-03-30 10:15       ` Junio C Hamano
2023-03-30 14:19         ` Felipe Contreras
2023-04-01  0:04           ` Taylor Blau
2023-04-01  0:59             ` Felipe Contreras
2023-04-01  1:30           ` Junio C Hamano [this message]
2023-04-01  2:39             ` Felipe Contreras
2023-04-01  0:00         ` Taylor Blau
2023-04-01  0:50           ` Felipe Contreras
2023-03-29 22:14     ` brian m. carlson
2023-03-30  3:15       ` Junio C Hamano
2023-03-30  7:47         ` Felipe Contreras

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqv8igbd8e.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=sandals@crustytoothpaste.net \
    --cc=sunshine@sunshineco.com \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).