Re: [PATCH] git-prompt: use builtin test

From: Jeff King <peff@peff.net>
Date: Jun 21, 2022, 06:56 UTC
Message-ID: <YrFrsZj8w0i6PPiz@coredump.intra.peff.net>
In-Reply-To: <Yq57MP47M5fAzkFC@bnjf.id.au>

On Sun, Jun 19, 2022 at 01:26:08AM +0000, Brad Forschinger wrote:

Show 8 quoted lines

> > But at some point we may say "you have made the environment too hostile
> > for us to function". Is redefining "test" to something that doesn't
> > behave the same way such a case? Part of me wants to say yes. :)
> 
> I'd be inclined to agree!  But disregarding a user with malicious
> intent, these environment changes can also be unintentional: I came
> across it when I stubbed out a quick test() function while prototyping
> something unrelated.

I kind of wonder what else would have trouble with your accidental breakage. I poked at the bash-completion package as the only other prominent "source this into your bash script" package I could think of. They do indeed seem to mostly avoid "test", though there are a few cases in program-specific completions.

In the general case, though, I think it's an infinite rabbit hole. I can similarly redefine "declare" or "local" and cause all sorts of trouble. And there's no real defense for scripts there.

Show 11 quoted lines

> >   - my biggest concern on cost is that this is an unusual style for our
> >     project (which usually writes in POSIX shell, though of course this
> >     file is meant to be bash/zsh specific). Will it be a maintenance
> >     burden going forward?
> 
> 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"? 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.

-Peff