pip config and $EDITOR containing arguments

[haooodev]

Environment

• pip version: 19.2.3
• Python version: 3.8
• OS: Linux

Description

pip config edit is supposed to open the program specified by environment variable Editor if it's set. Actually if the env is a program with its options, pip will treat the whole thing as the program name and throws a FileNotFoundError.

Expected behavior

pip config edit open the editor with options set.

How to Reproduce

1. export EDITOR="nvim --noplugin"; pip config edit

Output

ERROR: Exception:
Traceback (most recent call last):
  File "/usr/lib/python3.8/site-packages/pip/_internal/cli/base_command.py", line 188, in main
    status = self.run(options, args)
  File "/usr/lib/python3.8/site-packages/pip/_internal/commands/configuration.py", line 136, in run
    handlers[action](options, args[1:])
  File "/usr/lib/python3.8/site-packages/pip/_internal/commands/configuration.py", line 216, in open_in_editor
    subprocess.check_call([editor, fname])
  File "/usr/lib/python3.8/subprocess.py", line 359, in check_call
    retcode = call(*popenargs, **kwargs)
  File "/usr/lib/python3.8/subprocess.py", line 340, in call
    with Popen(*popenargs, **kwargs) as p:
  File "/usr/lib/python3.8/subprocess.py", line 854, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.8/subprocess.py", line 1702, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'nvim  #--noplugin'

My Fix #7391

[chrahunt]

There is some discussion on the associated PR #7391.

[pradyunsg]

I remember researching this. There wasn't any reference material for $editor that involved arguments. That said, if git and svn (both) support this form of invocation, that's a strong argument in support of allowing this. If either doesn't, I don't know how we should go about this.

Regardless, we should definitely print a nicer error message here IMO.

[chrahunt]

SVN_EDITOR supports it per this snippet:

The value of [EDITOR] is the beginning of a command line to be executed by the shell. Subversion appends to that command line a space and the pathname of a temporary file to be edited.

A quick test shows git supports it too:

#!/bin/bash
cd "$(mktemp -d)"
mkdir "example path"
cd "example path"

git init
cat <<'EOF' > script
#!/bin/sh
printf "%s\n" "$@" > args.txt
EOF
chmod +x script
git add .
EDITOR="'$PWD/script' hello world" git commit

echo ========= Result =========
cat args.txt

yields

Initialized empty Git repository in /tmp/user/1000/tmp.WJdV4vgEQd/example path/.git/
Aborting commit due to empty commit message.
========= Result =========
hello
world
/tmp/user/1000/tmp.WJdV4vgEQd/example path/.git/COMMIT_EDITMSG

I would be in favor of this depending on the approach. We can discuss that on #7391.

[chrahunt]

For anyone that wants to implement this, please see #7391 which shows where the change needs to happen and #7391 (comment) which has advice to improve that approach. We would be happy to review a PR that implements this taking those into account.

[gutsytechster]

I would like to resolve this issue.

[chrahunt]

Hi @gutsytechster! My previous comment still stands.

The first place I would start is figuring out a way to get shutil.which-compatible behavior on Python 2 (like mentioned in #7391 (comment). If having a shutil.which-compatible version in Python 2 looks like too much work, then we can either make it work in Python 3 only or wait until we drop Python 2 support. I would not spend too much time trying to get a Python 2 version to work nicely, maybe 15 minutes before doing Python 3 only. Feel free to drop a comment with what you find out, or submit a PR if you're comfortable with your approach!

[gutsytechster]

An alternative to shutil.which for Python2 can be implemented as

>>> def which(pgm):
    path=os.getenv('PATH')
    for p in path.split(os.path.pathsep):
        p=os.path.join(p,pgm)
        if os.path.exists(p) and os.access(p,os.X_OK):
            return p


>>> os.which=which
>>> os.which('ls.exe')
'C:\\GNUwin32\\bin\\ls.exe'

A quick search over the web leaded to me this implementation. Reference:- https://stackoverflow.com/questions/9877462/is-there-a-python-equivalent-to-the-which-command.

What do you think about this @chrahunt?

[chrahunt]

On Windows it would also need to check PATHEXT. And we'd need tests for the same. Given that, I would no longer implement this for Python 2. We can just use the existing behavior for Python 2 and handle args in Python 3.

[gutsytechster]

I see. Thanks, I'll then put up a PR to implement this on Python3.

[gutsytechster]

@chrahunt, In the suggested implementation here, https://github.com/pypa/pip/pull/7391#pullrequestreview-335613001, what would be the case when the code under if os.path.exists(editor) would execute? To me this condition and the first condition i.e. shutil.which(editor) looks similar.

I mean, if the path would exist, then the first condition would already return True, won't it?

[gutsytechster]

Also currently, the way that code executes is like this.

| input | Python 2 | Python 3 |
| ------- |--------------- |--------------|
| vim| [vim] | [/usr/bin/vim] |
| vim --nofork | [vim --nofork] | [vim, --nofork]|

Since the behaviour is not changed for Python 2, the output is similar to what is being provided to it as an editor. However for Python 3, if a single argument ie the application is provided, then it returns an absolute path to the executable but when an application is provided with arguments, the arguments are separated by space.

Should it behave like that?

This is w.r.t #8612

[McSinyx]

I've been reading the discussion over GH-7391 and I also favor passing the raw EDITOR with shell=True, for the following reasons:

• It would just works as the user would expect/experience in other CLI tools, with all kind of obscure values for EDITOR
• We won't have to deal with the pain of shell unquoting, e.g. EDITOR="vim -S 'some weird path to additional rc'".

[uranusjr]

I believe shlex.split() already deals with quoting and unquoting correctly. You can pass the splitted result directly into subprocess and it would just work.

How do other tools (e.g. SVN, Git) handle this environment variable?

[McSinyx]

I believe shlex.split() already deals with quoting and unquoting correctly. You can pass the splitted result directly into subprocess and it would just work.

You're right, I've just checked this again.

How do other tools (e.g. SVN, Git) handle this environment variable?

git seems to handle it out-of-box, e.g. with

#!/bin/sh
cd $(mktemp -d)
mkdir example\ path
cp ~/.vim/vimrc example\ path
git init
git add .
EDITOR="vim -S 'example path/vimrc'" git commit

I don't understand why we need shutil.which though.

[uranusjr]

I should be clearer. I was wondering how other tools implement this feature, e.g. do they use something analogous to shell=True?

[McSinyx]

I've just taken a peak at git's source and it uses shell too.

[gutsytechster]

So, how do we want to proceed with it? This question https://github.com/pypa/pip/issues/7392#issuecomment-663652892 still stands.

[gutsytechster]

ping^^

[uranusjr]

I feel we should yield and just use shell=True if needed. I still think it’s wrong, but it is what users expect because everybody is doing it.

[DanielShaulov]

I'll just add a popular use case to the discussion - using vscode as a git editor.

[DanielShaulov]

@gutsytechster - About https://github.com/pypa/pip/issues/7392#issuecomment-663652892, what if we change the logic to:

...
result = shutil.which(editor)
if result or os.path.exists(editor):
    return [editor]
return shlex.split(editor)

So if the users command works - we just use that without changing it. This removes the odd behavior of changing to absolute path, but keeps everything else working.