Editor is not passed to the shell

[bk2204]

Describe the bug

The editor, whether acquired from core.editor, $EDITOR, or $VISUAL, must be passed to the shell for expansion because the values can contain arbitrary shell expressions. However, gh only performs shell splitting, which means complex invocations don't work properly as they do with Git and other standard Unix utilities (e.g., crontab).

While it is very tempting to try to avoid invoking the shell through various mechanisms and parsing, it is actually required for compatibility with Git and can't be avoided. People actually do stuff shell expressions in those variables to use different editors based on things like whether they have an X11 session or the editors available on the system (e.g., use VS Code if available, otherwise Vim).

See mislav/hub#2214 for more details and a PR for hub.

$ gh --version
gh version 0.8.0-37-ge28e609 (2020-05-14)
https://github.com/cli/cli/releases/latest

Steps to reproduce the behavior

1. Type VISUAL='f() { vim "$@"; };f' gh issue create
2. Specify a title.
3. Choose to edit the body with your editor.

Expected vs actual behavior

Expected behavior is that vim is invoked. Actual behavior is that the operation fails.

Logs

% VISUAL='f() { vim "$@"; };f' ~/checkouts/cli/bin/gh issue create

Creating issue in bk2204/test-repo

? Title Foo
? Body [(e) to launch f() { vim "$@"; };f] could not collect title and/or body: could not prompt: exec: "f()": executable file not found in $PATH

[mislav]

Thanks for re-raising this!

Do you know how common is the practice of putting shell expressions in environment variables? I have never seen it in the wild.

I did see embedding other environment variables in, such as $HOME, but expanding those is arguably easier and does not require passing through a shell interpreter.

[bk2204]

I can't say for certain how common this is. I would use it for scripting automatic messages when opening a PR by extracting, reformatting, and unwrapping commit messages, and I expect other reasonably advanced folks will as well. Since I'd be using command substitutions, it would necessarily need to be passed to the shell.

We had to fix a similar issue in Git LFS (with GIT_SSH_COMMAND) because our attempts to monkey patch it again and again, even with shell splitting and environment variable expansion, were ultimately unsuccessful, since even with that people found places where it didn't work properly. I'm pretty sure that trying to do a "good enough" job will result in a similar problem here. mislav/hub#1482, for example, outlines a situation where nothing but a full shell expansion will work.