Support Editor env with options in config edit

[haooodev]

Generally the EDITOR env is a single string referring to a program but
nothing prevents user from attaching options within it. To fully conform
to the convention pip config edit should split the env first.

Fixes #7392.

[uranusjr]

Could you provide reference whether this is the right thing to do? I briefly searched, but all references I found to the EDITOR environ only mentions setting it to a command, not command + arguments. The current implementation also wouldn’t work well on Windows, where editors are usually installed with a space in the path (e.g. C:\Program Files).

[chrahunt]

This could be a reasonable approach that addresses the point raised by @uranusjr:

def get_editor_cmd():
    editor = os.environ["EDITOR"]
    result = shutil.which(editor)
    if result:
        return [result]
    if os.path.exists(editor):
        return [editor]
    return shlex.split(editor)

Then if we want to deprecate non-shell-syntax in the future we could warn if os.path.exists(editor) and ([editor] != shlex.split(editor)).

We would also need tests for this.

(discussion on whether to do this is on #7392).

[pradyunsg]

Thanks for experimenting with this @chrahunt! So, we should do this.

I think we should pass shell=True into the subprocess call, instead of trying to figure out what to invoke directly -- it'll be the responsibility of the user to do the escaping properly, but at least, the invocation would be similar to how svn seems to be doing it.

[pfmoore]

Be careful to test shell=True on Windows. Cross platform argv handling is very tricky, and switching from shell=False to shell=True could easily trigger regressions.

I'm not saying don't use shell=True, just that unless there's a good reason to change, it may not be worth the risk. I'd rather we split the EDITOR variable and passed it as arguments, rather than pass it uninterpreted to the shell.

Actually, there may be other issues too - has anyone considered how switching to shell=True would handle EDITOR='vi $(rm -rf /)' for example?

[chrahunt]

I think we should pass shell=True into the subprocess call, instead of trying to figure out what to invoke directly -- it'll be the responsibility of the user to do the escaping properly, but at least, the invocation would be similar to how svn seems to be doing it.

This would be backwards-incompatible and may be less obvious for users who just want to set their editor path directly. I updated my previous comment with an example implementation in case that makes it clearer how much code we're talking about.

Actually, there may be other issues too - has anyone considered how switching to shell=True would handle EDITOR='vi $(rm -rf /)' for example?

I don't think using shell=True changes the risk. With the current implementation that only accepts a path to a single executable, someone could set EDITOR=/script/that/has/rm-rf-slash for example.

[uranusjr]

@chrahunt’s suggestion looks good to me, except shutil.which is not available on 2.7 :(

[chrahunt]

It has been some time without movement on this, so I will close it. I've left a comment on #7392 with details for anyone that wants to pick this back up.