# Re: [PATCH] git-p4: Fix empty submit template when editor fires up

3 messages from 2010-01-10 to 2010-01-10. Participants: Jonathan Nieder, Kevin Leung.
Thread: https://gitlist.dev/t/22151

## Jonathan Nieder, 2010-01-10 11:11

Subject: Re: [PATCH] git-p4: Fix empty submit template when editor fires up
Message-ID: <20100110111153.GA19612@progeny.tock>
URL: https://gitlist.dev/e/20100110111153.GA19612%40progeny.tock
In-Reply-To: <1262235876-1239-1-git-send-email-kevinlsk@gmail.com>

```
Kevin Leung wrote:
> read_pipe() returns "\n". We need to remove it before passing it
> to system().
> 
> Signed-off-by: Kevin Leung <kevinlsk@gmail.com>

If I understand correctly, this is a cosmetic change: os.system()
calls system(3), which uses 'sh -c', which has no problem coping with
an extra newline at the end.  So 'need' seems too strong a word.
Still, the change sounds sensible.

> ---
>  contrib/fast-import/git-p4 |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 0cef242..04bf4f4 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -732,8 +732,8 @@ class P4Submit(Command):
>              if os.environ.has_key("P4EDITOR"):
>                  editor = os.environ.get("P4EDITOR")
>              else:
> -                editor = read_pipe("git var GIT_EDITOR")
> -            system(editor + " " + fileName)
> +                editor = read_pipe("git var GIT_EDITOR").strip()
> +            system("%s %s" % (editor, fileName))
>  
>              response = "y"
>              if os.stat(fileName).st_mtime <= mtime:
> -- 

What is the rationale for the rewritten system() line?  I would have
understood a change to

	os.spawnlp("sh", "-c", editor + " \"$@\"", fileName)

for better behavior when TMPDIR contains shell metacharacters, but
even this has nothing to do with read_pipe() returning a trailing
newline.

Just my two cents,
Jonathan

```

## Jonathan Nieder, 2010-01-10 11:14

Subject: Re: [PATCH] git-p4: Fix empty submit template when editor fires up
Message-ID: <20100110111440.GB19612@progeny.tock>
URL: https://gitlist.dev/e/20100110111440.GB19612%40progeny.tock
In-Reply-To: <20100110111153.GA19612@progeny.tock>

```
Jonathan Nieder wrote:
> Kevin Leung wrote:
> > read_pipe() returns "\n". We need to remove it before passing it
> > to system().
> > 
> > Signed-off-by: Kevin Leung <kevinlsk@gmail.com>
> 
> If I understand correctly, this is a cosmetic change:

... and of course I didn't the subject.  Sorry about that.  Thanks
for cleaning up my mess.

Acked-by: Jonathan Nieder <jrnieder@gmail.com>

> What is the rationale for the rewritten system() line?  I would have
> understood a change to
> 
> 	os.spawnlp("sh", "-c", editor + " \"$@\"", fileName)

I am still curious about this, though it is not so important.

Jonathan

```

## Kevin Leung, 2010-01-10 13:52

Subject: Re: [PATCH] git-p4: Fix empty submit template when editor fires up
Message-ID: <e66701d41001100552p6cf2fde8m6463613cfcc4315b@mail.gmail.com>
URL: https://gitlist.dev/e/e66701d41001100552p6cf2fde8m6463613cfcc4315b%40mail.gmail.com
In-Reply-To: <20100110111440.GB19612@progeny.tock>

```
On Sun, Jan 10, 2010 at 7:14 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
>> Kevin Leung wrote:
>> > read_pipe() returns "\n". We need to remove it before passing it
>> > to system().
>> >
>> > Signed-off-by: Kevin Leung <kevinlsk@gmail.com>
>>
>> If I understand correctly, this is a cosmetic change:
>
> ... and of course I didn't the subject.  Sorry about that.  Thanks
> for cleaning up my mess.
>
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
>
>> What is the rationale for the rewritten system() line?  I would have
>> understood a change to
>>
>>       os.spawnlp("sh", "-c", editor + " \"$@\"", fileName)
>
> I am still curious about this, though it is not so important.

You are right, the system() line is not so important. I can revert
that line of change.

Kevin

```