Bug: git-p4 edit_template() and P4EDITOR w/options

Hello Git Folks!

I recently had a user who was unable to perform a 'git p4 submit', with the following traceback:

========
Traceback (most recent call last):
  File "/usr/local/pixar/Cellar/git/2.2.2/libexec/git-core/git-p4", line 3302, in <module>
    main()
  File "/usr/local/pixar/Cellar/git/2.2.2/libexec/git-core/git-p4", line 3296, in main
    if not cmd.run(args):
  File "/usr/local/pixar/Cellar/git/2.2.2/libexec/git-core/git-p4", line 1723, in run
    ok = self.applyCommit(commit)
  File "/usr/local/pixar/Cellar/git/2.2.2/libexec/git-core/git-p4", line 1477, in applyCommit
    if self.edit_template(fileName):
  File "/usr/local/pixar/Cellar/git/2.2.2/libexec/git-core/git-p4", line 1228, in edit_template
    system([editor, template_file])
  File "/usr/local/pixar/Cellar/git/2.2.2/libexec/git-core/git-p4", line 196, in system
    retcode = subprocess.call(cmd, shell=expand)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 524, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 711, in __init__
    errread, errwrite)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1308, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory
==========

This is running on OS X 10.9.5, with git 2.2.2 installed via homebrew, but we’ve replicated it with the version of git that comes with XCode (which I believe is older, 1.9.x)

We chased this down to the fact that his P4EDITOR ( and GIT_EDITOR) values have a command-line option in them:  /path/to/mate -w,

The edit_template() function passes that value as-is to system(), and then on to subprocess.call(), in such a way that the entirety of P4EDITOR is treated as the command name.

I have confirmed this by modifying the system() call in edit_template()  thus, and it works:

==========
if editor.split()[-1][0] == "-":
    cmd = editor.split() + [template_file]
    system(cmd)
 else:
    system([editor, template_file])
==========

Which results, if the last word of 'editor' begins with a hyphen, in [ "/path/to/mate", "-w", "/path/to/template"] being passed to system() rather than the non-funcional [ "/path/to/mate -w", "/path/to/template"].

Clearly my hack is not the best way to address this, especially since I’m not much of a python coder, nor am I familiar with the git-p4 source. I was just confirming my suspicion and trying to get things working for the user.

Thank you! -Chris Lasell

PS: 'mate' is the CLI invocation of the TextMate GUI editor for OS X. The -w is required in this instance, or the 'mate' command would return 
instantly when the document opens. The -w causes it to wait until the document window is closed.

I have noticed that the help output for mate says: ======= By default mate will wait for files to be closed if the command name has a "_wait" suffix (e.g. via a symbolic link) =======

and I have instructed my user to do just that for now. However, options to EDITOR environment variables are not uncommon, so I wanted to address this to the right people

Chris Lasell <chrisl@pixar.com> writes:

I think that is not merely "for now" but is the way the command and the environment variable are designed to be used. A quick websearch for [$EDITOR environment with parameter] found this one

  http://superuser.com/questions/521070/unix-environment-variables-with-arguments

which seems to be talking about a similar issue (unrelated to Git).

The relevant part of git-p4 is this:

        # invoke the editor
        if os.environ.has_key("P4EDITOR") and (os.environ.get("P4EDITOR") != ""):
            editor = os.environ.get("P4EDITOR")
        else:
            editor = read_pipe("git var GIT_EDITOR").strip()
        system([editor, template_file])

It grabs $EDITOR (or $GIT_EDITOR) and treats it as the path to the editor executable, without letting shell to split that into words at whitespace boundaries, so that you can say things like

	EDITOR="/User/me/My Programs/nano"

The way we spawn EDITOR in our core codepaths matches what git-p4 does, too:

	const char *args[] = { editor, real_path(path), NULL };
	struct child_process p = CHILD_PROCESS_INIT;
	int ret, sig;

	p.argv = args;
	p.env = env;
	p.use_shell = 1;
	if (start_command(&p) < 0)
		return error("unable to start editor '%s'", editor);
	...

So...

Junio C Hamano <gitster@pobox.com> writes:

Well, I'll take that back.  I misread p.use_shell line.

Junio C Hamano <gitster@pobox.com> writes:

So, use_shell==true codepath grabs that array with "editor", "path" and turns it into an equivalent to

	sh -c "$EDITOR" "$path"

when $EDITOR has a "magic" character (including whitespace) in it. So what git-p4 does is *not* in line with how we use the environment variable.

Perhaps a single-liner patch like this would work?

- system([editor, template_file]) + system(["sh", "-c", editor, template_file])

Junio C Hamano <gitster@pobox.com> writes:

Nah, scratch that.  The list should be more like

	["sh", "-c", "%s %s" % (editor, shlex.quote(template_file))]

if your Python has shlex.quote, that is; it may be pipes.quote instead.

Junio C Hamano wrote:

*nod*

run-command.c does

	system(["sh", "-c", ('%s "$@"' % editor), editor, template_file])

to avoid having to figure out how to shell-quote template_file.

Jonathan Nieder <jrnieder@gmail.com> writes:

Yup, thanks.

Thanks for the input Junio!

I’ll try it out, but leave the decision about patching  for the maintainers :-)

Though I do notice that their "system()" function does do shell expansion if it’s arg is a string, but not if it’s a list:

def system(cmd):
    expand = isinstance(cmd,basestring)
    if verbose:
        sys.stderr.write("executing %s\n" % str(cmd))
    retcode = subprocess.call(cmd, shell=expand)
    if retcode:
        raise CalledProcessError(retcode, cmd)

and the edit_template() function is passing a list

I had thought about writing a tiny wrapper script as mentioned in that first link you sent, but that felt even more out of line, especially since there are many references on the net to using parameters in EDITOR vars, particularly with mate.

FWIW, my user is all set now, but I’m happy to participate in this conversation if appropriate :-)

Cheers, -Chris