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

3 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:47:59

Kevin Leung wrote:
read_pipe() returns "\n". We need to remove it before passing it
to system().

Signed-off-by: Kevin Leung <redacted>
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.
quoted hunk
---
 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

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

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:47:59

Jonathan Nieder wrote:
Kevin Leung wrote:
quoted
read_pipe() returns "\n". We need to remove it before passing it
to system().

Signed-off-by: Kevin Leung <redacted>
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 <redacted>
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

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

From: Kevin Leung <hidden>
Date: 2016-06-15 22:47:59

On Sun, Jan 10, 2010 at 7:14 PM, Jonathan Nieder [off-list ref] wrote:
Jonathan Nieder wrote:
quoted
Kevin Leung wrote:
quoted
read_pipe() returns "\n". We need to remove it before passing it
to system().

Signed-off-by: Kevin Leung <redacted>
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 <redacted>
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help