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

From: Chris Lasell <chrisl@pixar.com>
Date: May 4, 2015, 22:46 UTC
Message-ID: <8AC0C799-2402-4AAB-8C9D-271FBF14910D@pixar.com>
In-Reply-To: <xmqqzj5knf2t.fsf@gitster.dls.corp.google.com>

Show 18 quoted lines

> 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