git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: Brad Forschinger <bnjf@bnjf.id.au>,
	Brad Forschinger via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH] git-prompt: use builtin test
Date: Tue, 21 Jun 2022 10:35:10 -0700	[thread overview]
Message-ID: <xmqqmte5hqcx.fsf@gitster.g> (raw)
In-Reply-To: YrFrsZj8w0i6PPiz@coredump.intra.peff.net

Jeff King <peff@peff.net> writes:

>> That's possible, but I suspect the burden is minimal.  As you said, this
>> is bash and zsh specific, and for those shell coders who only write
>> Bourne dialect it's to be read as a "strong" left square bracket.  For
>> example, to minimize any shock to the eyeballs I've intentionally not
>> re-written string operations `[ a = b ] && [ c = d ]` to `[[ a == b && c
>> == d ]]`.  I promise it wasn't mere laziness!
>
> I guess my concern was less about doing it once, and more about: is this
> something we want to continue enforcing as time goes on? That is, would
> we want to catch it in review and complain about people using "test"?

Yes, if this is just a mental exercise burning off excess calories
and too much spare time and nothing else, I'm hesitant to burden our
reviewers and casual contributors, who do not have excess calories
or too much spare time to burn off just to play nice with the result
of this patch, with an issue that is apparently only theoretical
like this one, with which nobody actually is hurt in practice.

It's not like local variables used in prompt and completion leaking
out, which can hurt real users (as it is quite normal to use throw
away variables in your interactive sessions).  It's not even like
these scripts being "set -u" clean, that may hurt those who use "set
-u" in their interactive sessions (what for?) but nobody else.  

If you write your own shell function "test" that is grossly
incompatible with everybody else's "test", where do you do that?
Not in your .bashrc, or you would break more than just prompt and
completion.

If this came with additions to t9903 that redefines "test" and tries
to make sure we consistently use bashism [[...]] and such a change
to "test" would not break it, I might have been more sympathetic,
but I dunno.  I sort-of like the purity of mind when we say "this is
a #!/bin/bash script and we maximally write in a way a bash-script
expert would", but many of our shell scripts outside these two
contrib/ scripts HAVE to be portable and free of bashisms, so...

> That's a subtle thing to remember to look for, though I guess we could
> automate it via the tests. Or would we rely on people who cared to
> notice new instances and submit patches? That's how we deal with some
> other portability issues (if nobody is screaming, how broken could it
> be?). But it sounds from your description like this was a one-off even
> for you.
>
> So I dunno. I'm not really opposed, but I'm not convinced it's really
> accomplishing much here.

I guess I am mostly on the same page here.

Thanks.

      reply	other threads:[~2022-06-21 17:35 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-14  9:09 [PATCH] git-prompt: use builtin test Brad Forschinger via GitGitGadget
2022-06-16 22:37 ` Jeff King
2022-06-19  1:26   ` Brad Forschinger
2022-06-21  6:56     ` Jeff King
2022-06-21 17:35       ` Junio C Hamano [this message]

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

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

  git send-email \
    --in-reply-to=xmqqmte5hqcx.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=bnjf@bnjf.id.au \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=peff@peff.net \
    /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.
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 NNTP newsgroup(s).