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

From: Jeff King <peff@peff.net>
Date: Jun 16, 2022, 22:37 UTC
Message-ID: <YquwpxEoAVWen8tZ@coredump.intra.peff.net>
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:

Show 6 quoted lines

> 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