Wrong parameter expansion with "brew shellenv"

[duxsco]

brew config output

HOMEBREW_VERSION: 3.5.8
ORIGIN: https://github.com/Homebrew/brew
HEAD: ff9c2500195cc357d4a06f99d462f8d8a5d37892
Last commit: 5 days ago
Core tap ORIGIN: https://github.com/Homebrew/homebrew-core
Core tap HEAD: 882dc1d6e541367fdd4a9d79452dbc1c54b53066
Core tap last commit: 18 hours ago
Core tap branch: master
HOMEBREW_PREFIX: /opt/homebrew
HOMEBREW_AUTO_UPDATE_SECS: 86400
HOMEBREW_CASK_OPTS: [--appdir=~/Applications, --fontdir=/Library/Fonts]
HOMEBREW_MAKE_JOBS: 10
HOMEBREW_NO_ANALYTICS: set
Homebrew Ruby: 2.6.8 => /System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/bin/ruby
CPU: 10-core 64-bit arm_firestorm_icestorm
Clang: 13.1.6 build 1316
Git: 2.37.1 => /opt/homebrew/bin/git
Curl: 7.79.1 => /usr/bin/curl
macOS: 12.5-arm64
CLT: 13.4.0.0.1.1651278267
Xcode: N/A
Rosetta 2: false

brew doctor output

Your system is ready to brew.

Verification

• I ran brew update and am still able to reproduce my issue.
• I have resolved all warnings from brew doctor and that did not fix my problem.

What were you trying to do (and why)?

I was following the installation steps to install.

What happened (include all command output)?

I got asked to add eval "$(/opt/homebrew/bin/brew shellenv)" to ~/.bash_profile or equivalent. I can't remember which file it was.

What did you expect to happen?

I expected for parameter expansion to be used correctly. See ${parameter:+word} from:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02

It seems, however, that parameter expansion is used wrongly:

brew/Library/Homebrew/cmd/shellenv.sh

Lines 48 to 49 in 044fefd

	echo "export PATH=\"${HOMEBREW_PREFIX}/bin:${HOMEBREW_PREFIX}/sbin\${PATH+:\$PATH}\";"
	echo "export MANPATH=\"${HOMEBREW_PREFIX}/share/man\${MANPATH+:\$MANPATH}:\";"

Step-by-step reproduction instructions (by running brew commands)

brew shellenv

[gromgit]

You're misreading the substitution. ${PATH+:$PATH} literally means:

• if PATH is not set, substitute nothing
• if PATH is set, substitute a colon, followed by the contents of PATH

What you seem to think is correct, i.e. ${PATH:+$PATH} is actually the wrong one. Simple demonstration:

$ echo $PATH
/usr/bin:/bin
$ echo /opt/homebrew/bin${PATH+:$PATH}
/opt/homebrew/bin:/usr/bin:/bin
$ echo /opt/homebrew/bin${PATH:+$PATH}
/opt/homebrew/bin/usr/bin:/bin

[carlocab]

Changing

${PATH+:$PATH}

to

${PATH:+:$PATH}

might be more POSIX-friendly.

Though I'd want to know which shells this fixes things for before changing anything.

[MikeMcQuaid]

might be more POSIX-friendly.

We only support Bash and ZSH as POSIX shells.

[gromgit]

Changing

${PATH+:$PATH}

to

${PATH:+:$PATH}

might be more POSIX-friendly.

POSIX actually defines both colon and non-colon variants, so that shouldn't be an issue.

However, I just realized that ${PATH+:$PATH} results in a trailing colon if $PATH is set but empty, which effectively adds the current directory to $PATH, i.e. it's equivalent to adding :., which is a security no-no.

So yeah, ${PATH:+:$PATH} is the correct substitution, even if the probability of $PATH being empty is low-to-no.

[duxsco]

What you seem to think is correct, i.e. ${PATH:+$PATH} is actually the wrong one. Simple demonstration:

With ${parameter:+word}, I was thinking of "PATH" for "parameter" and ":$PATH" for "word". So, ${PATH:+:$PATH} as @carlocab suggested should be the prefered way. In the 2nd table, it's the 2nd last row:
https://stackoverflow.com/a/16753536

The current approach is shown in the 2nd table in the last row.

[duxsco]

I still don't get why this issue got closed. PATH is obviously set wrongly as initially stated.

[carlocab]

Well, as initially stated, it wasn't at all clear, and definitely not obvious, what was wrong.

Upon further examination by @gromgit in #13674 (comment), maybe there is something wrong, but it's not really obvious either. It also applies to a really narrow edge case that I struggle to find worth keeping an issue open over.

If you could explain more clearly what exactly the consequences of the current usage of parameter expansion in brew shellenv is, and how changing the usage of parameter expansion avoids those consequences, preferably with information that hasn't yet been presented earlier in this issue, then maybe we can reconsider closing this.

[duxsco]

Well, as initially stated, it wasn't at all clear, and definitely not obvious, what was wrong.

... and the issue got closed.

Here is some more info:
https://www.gnu.org/savannah-checkouts/gnu/bash/manual/bash.html#index-PATH

[duxsco]

I'll just use:

eval "$(/opt/homebrew/bin/brew shellenv | sed -e 's/${PATH+:$PATH}/${PATH:+:$PATH}/' -e 's/${MANPATH+:$MANPATH}/${MANPATH:+:$MANPATH}/')"

[carlocab]

Links to documentation are really poor substitutes for explanations.

Do you regularly run brew shellenv in a shell where PATH and MANPATH are set to empty strings? If not, then it seems to me that you're incurring the overhead of calling sed to do absolutely nothing:

❯ diff -q <(echo foo) <(echo foo) && echo same
same
❯ diff -q <(echo foo) <(echo bar) && echo same
Files /dev/fd/13 and /dev/fd/14 differ
❯ diff -q <(echo ${PATH+:$PATH}) <(echo ${PATH:+:$PATH}) && echo same
same
❯ diff -q <(echo ${MANPATH+:$MANPATH}) <(echo ${MANPATH:+:$MANPATH}) && echo same
same

If you do set PATH and MANPATH to empty strings: why? What's the use case for this?

[duxsco]

Let me ask the other way around:
Why bothering with parameter expansion in the first place if only unset PATH is taken into consideration and not PATH set to empty value?
Instead of using ${PATH+:$PATH} the homebrew team could have just used :$PATH.

And, let me ask further:
What's the use case for mitigating only unset PATH and not PATH set to empty value?

[duxsco]

I am just arguing from the point of view where unset PATH is mitigated, but not PATH set to emtpy value. And, I just pointed out that the second case is not taken care of. Whether using parameter expansion here is overkill is another field of discussion.

[duxsco]

It's not like it's a big step to take care of booth loose ends.