From: Jeff King <peff@peff.net>
To: Brad Forschinger via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Brad Forschinger <bnjf@bnjf.id.au>
Subject: Re: [PATCH] git-prompt: use builtin test
Date: Thu, 16 Jun 2022 18:37:27 -0400 [thread overview]
Message-ID: <YquwpxEoAVWen8tZ@coredump.intra.peff.net> (raw)
In-Reply-To: <pull.1260.git.1655197751403.gitgitgadget@gmail.com>
On Tue, Jun 14, 2022 at 09:09:11AM +0000, Brad Forschinger via GitGitGadget wrote:
> From: Brad Forschinger <bnjf@bnjf.id.au>
>
> The test and [ commands are used throughout the prompt generation. They
> also happen to be valid function names that can be defined, leading to
> unintentional results. Prevent the somewhat unusual case of this
> happening by simply using [[, which is reserved.
Hmm. I do think we need to be a bit more paranoid about style in the
prompt and completion code, because they are sourced into the user's
shell alongside whatever other weird customizations they'd have. So we
already have adjustments to work under "set -u", and so forth.
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. :)
That said, if it's not _hard_ to support, maybe it is worth doing to be
on the cautious side? A few thoughts:
- 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?
- this only changes git-prompt.sh; doesn't the completion code have
the same issue?
- I don't write much bash-specific code, but I seem to recall that
"[[" has some subtle differences to "[". Is it sufficiently a
superset that these conversions are all equivalent?
I think some like:
> - if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
> + if [[ $pcmode = yes ]] && [[ $ps1_expanded = yes ]]; then
are not equivalent, but it's an actual improvement (bash's builtin
"[[" isn't confused by unquoted empty variables), but I don't know
if there may be other gotchas.
(I doubt this is an actual bug in the current code, as $pcmode
always seems to be set, but just a more defensive style).
-Peff
next prev parent reply other threads:[~2022-06-16 22:37 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 [this message]
2022-06-19 1:26 ` Brad Forschinger
2022-06-21 6:56 ` Jeff King
2022-06-21 17:35 ` Junio C Hamano
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=YquwpxEoAVWen8tZ@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=bnjf@bnjf.id.au \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@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).