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

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

[Chris Lasell]

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, Apple Peeler, MacTeam
Pixar Animation Studios, Emeryville, CA

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

[Junio C Hamano]

Chris Lasell <chrisl@pixar.com> writes:

> 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.

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...

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

[Junio C Hamano]

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

> 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...

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

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

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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...
>
> Well, I'll take that back.  I misread p.use_shell line.

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])

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

[Junio C Hamano]

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

> Perhaps a single-liner patch like this would work?
>
> -	system([editor, template_file])
> +	system(["sh", "-c", editor, template_file])

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.

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

[Jonathan Nieder]

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

>> Perhaps a single-liner patch like this would work?
>>
>> -	system([editor, template_file])
>> +	system(["sh", "-c", editor, template_file])
>
> Nah, scratch that.  The list should be more like
>
> 	["sh", "-c", "%s %s" % (editor, shlex.quote(template_file))]

*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.

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> run-command.c does
>
> 	system(["sh", "-c", ('%s "$@"' % editor), editor, template_file])
>
> to avoid having to figure out how to shell-quote template_file.

Yup, thanks.

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

[Chris Lasell]

> On May 4, 2015, at 3:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> 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])
> 

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