Add a few environment variables important for editors

[thecaralice]

• Have you followed the guidelines in our Contributing document?

• Have you checked to ensure there aren't other open Pull Requests for the same change?
  Do not filter COLORTERM #13843 was mistakenly closed
  rel: Linux: brew edit don't work with graphical editor #6958

• Have you added an explanation of what your changes do and why you'd like us to include them?

• Have you written new tests for your changes? Here's an example.

  Other allowed variables are not covered by any tests, and I'm not good enough at writing anything in Ruby, let alone covering obscure code with tests.

• Have you successfully run brew style with your changes locally?

• Have you successfully run brew typecheck with your changes locally?

• Have you successfully run brew tests with your changes locally?

  I'm not going to decipher what Error: failed to install the 'bundler' gem. means and why does it happen, I'm no Ruby developer, let me use the important variables please.

---

• COLORTERM is provided by iTerm2, Alacritty, Konsole and others to indicate 24-bit color support, and some editors like Helix and Neovim rely on that to enable true color themes
• COLORFGBG is provided by rxvt-based terminal emulators and some others like iTerm2, and it is relied on by some editors like vim/MacVim and other utilities like broot (through a library)
• COMMAND_MODE is used much less frequently, but according to compat(5) its absence might cause some software to misbehave; as it is almost useless and I myself don't have a use case for it, I'm okay with removing this one.

[MikeMcQuaid]

• Do not filter COLORTERM #13843 was mistakenly closed

It wasn't closed by mistake, I don't see the value in increasing all of these variables for the benefits of a few editors.

Which editor are you actually using and which variables do you personally rely on being present?

These could be passed through as HOMEBREW_ variables and not allowlisted entirely.

[thecaralice]

• Do not filter COLORTERM #13843 was mistakenly closed

It wasn't closed by mistake, I don't see the value in increasing all of these variables for the benefits of a few editors.

From #13843:

So in this case you mean it's actually just your local configuration?

COLORTERM is not a "local configuration" issue, it is a de-facto standard (see https://github.com/termstandard/colors#checking-for-colorterm), as @equal-l2 said in #13843 (comment) which was ignored for some reason(?).
Neovim and Helix are not "a few editors", Neovim is actually the most used console editor along with vim and Helix is on the third place according to https://github.com/topics/text-editor. The next one, micro, relies on it as well. You can't just say that nearly every editor has "local configuration" issues because brew doesn't allow them to work properly.

---

Which editor are you actually using and which variables do you personally rely on being present?

I use helix which, as I have written in the PR description, relies on that variable.

EDITOR=hx brew edit bash looks like this:

How it looks like with this PR:

Helix does not rely on COLORFGBG but I can imagine other people having hard-to-debug (see below) issues with that.

---

These could be passed through as HOMEBREW_ variables and not allowlisted entirely.

Setting HOMEBREW_COLORTERM does not help, because it is not a standard variable everyone uses. Fixing that on the editor side would require sending PRs to each of the editors to make them treat HOMEBREW_COLORTERM equally to COLORTERM and that is harder than adding a variable to the brew allowlist.

---

On "hard-to-debug" above: finding the COLORTERM issue required me to

1. Figure out that the editor doesn't know about true color support
2. Try different ways of invoking brew edit with different verbosity levels with no result
3. Figure out that either brew edit serves as a MITM between the terminal and the editor, forwarding stdin/stdout while filtering some OSI commands, or it invokes the editor in a weird way
4. Write a script that would log all CLI arguments and environment variables and some other things on start and then exit
5. Compare the logs between running directly and running with EDITOR=mole brew edit to find a lot of differences in the list of environment variables
6. Find COLORTERM which seems to affect that behavior
7. Search through the source code of helix to find that exact variable being relied upon
8. Search for any homebrew-related issues mentioning COLORTERM only to find a single PR closed with no clear reason or evidence of anyone actually reading what the author wrote.

Due to less global behavior of COLORFGBG figuring out the actual issue would be much harder.

[thecaralice]

Side question: why does brew even need to filter environment variables when editing files?
I see #932 reasoning this for installing/testing things to sandbox the build process so packages don't break, but how does that apply to editing formulae at all?

[apainintheneck]

COLORTERM seems related to the TERM and TERMINFO environment variables that we already let through so I think it's reasonable to allow it too. The other ones can probably wait until they actually affect users.

[MikeMcQuaid]

Neovim and Helix are not "a few editors"

Two is "a few" (hence my wording).

I use helix which, as I have written in the PR description, relies on that variable.

Which variable: COLORTERM?

I know you wrote it in the PR description but: it wasn't clear (hence why I asked again).

Helix does not rely on COLORFGBG but I can imagine other people having hard-to-debug (see below) issues with that.

Let's let those folks open the relevant issues/PRs, please, and scope this one just down to the variable(s?) you personally need.

These could be passed through as HOMEBREW_ variables and not allowlisted entirely.

Setting HOMEBREW_COLORTERM does not help, because it is not a standard variable everyone uses. Fixing that on the editor side would require sending PRs to each of the editors to make them treat HOMEBREW_COLORTERM equally to COLORTERM and that is harder than adding a variable to the brew allowlist.

It does help but: I did not explain adequately, my apologies.

Rather than changing:

brew/bin/brew

Lines 206 to 209 in 78e7401

	ENV_VAR_NAMES=(
	HOME SHELL PATH TERM TERMINFO TERMINFO_DIRS COLUMNS DISPLAY LOGNAME USER CI SSH_AUTH_SOCK SUDO_ASKPASS
	http_proxy https_proxy ftp_proxy no_proxy all_proxy HTTPS_PROXY FTP_PROXY ALL_PROXY
	)

Instead add to:

brew/bin/brew

Lines 152 to 161 in 78e7401

	# We don't want to take the user's value for, e.g., `HOMEBREW_PATH` here!
	USED_BY_HOMEBREW_VARS=(
	CODESPACES
	DBUS_SESSION_BUS_ADDRESS
	PATH
	SSH_TTY
	SUDO_USER
	TMUX
	XDG_RUNTIME_DIR
	)

and then add a ENV["COLORTERM"] = ENV.fetch("HOMEBREW_COLORTERM", nil) to https://github.com/Homebrew/brew/blob/master/Library/Homebrew/dev-cmd/edit.rb

This scopes the variable to just this command rather than adding it everywhere, potentially breaking formulae installations.

Side question: why does brew even need to filter environment variables when editing files?

This change dramatically reduces the number of issues with Homebrew running and installing packages based on user-set environment variables. As a result: we want to be extremely conservative here.

COLORTERM seems related to the TERM and TERMINFO environment variables that we already let through so I think it's reasonable to allow it too. The other ones can probably wait until they actually affect users.

@apainintheneck I disagree. When it's only something needed for a specific command (or few commands): we should scope it to just that command. I would be unsurprised if passing through COLORTERM breaks some formulae building from source that currently work fine today.

[apainintheneck]

@MikeMcQuaid That makes sense. I didn't notice that code pattern until now. brew edit is probably the only place that this is relevant.

[thecaralice]

This scopes the variable to just this command rather than adding it everywhere, potentially breaking formulae installations.

Sounds pretty reasonable, will do.

This change dramatically reduces the number of issues with Homebrew running and installing packages based on user-set environment variables.

Yeah, I understand that; however, I'm convinced it should not affect editor experience in any way since brew edit does not build packages. I haven't read the code thoroughly yet, but I can only imagine some path finding logic for passing the correct filename to the editor and maybe some form of locking the package; neither are affected by non-Homebrew environment variables like COLORTERM. Maybe some XDG_ variables, alright, but nothing more than that. So the best editing UX would be to pass the environment as-is perhaps? Should I open a separate issue for that?

[MikeMcQuaid]

This scopes the variable to just this command rather than adding it everywhere, potentially breaking formulae installations.

Sounds pretty reasonable, will do.

Great, thanks 👍🏻

Yeah, I understand that; however, I'm convinced it should not affect editor experience in any way since brew edit does not build packages.

Sorry, to be clear: what I proposed with using HOMEBREW_* passthrough variable which is handled in brew edit will have no impact on building packages but passing through this/more/all variable(s) unconditionally definitely will have impact.

So the best editing UX would be to pass the environment as-is perhaps? Should I open a separate issue for that?

Not worth opening an issue. It may be the nicest UX but: we're not going to do that, sorry. It's possible to break Homebrew initialisation through random environment variables which would prevent you getting to brew edit. If you really really want to have this: instead do something like editor $(brew edit --print-path wget).

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[thecaralice]

Sorry for a delay, hope I did this right? Works right on my machine at least

[MikeMcQuaid]

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @thecaralice!