{"thread":{"id":"22151","subject":"Re: [PATCH] git-p4: Fix empty submit template when editor fires up","startedAt":"2010-01-10T11:11:53Z","lastAt":"2010-01-10T13:52:53Z","messageCount":3,"participants":["Jonathan Nieder","Kevin Leung"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"131216","messageId":"20100110111153.GA19612@progeny.tock","threadId":"22151","inReplyTo":"1262235876-1239-1-git-send-email-kevinlsk@gmail.com","subject":"Re: [PATCH] git-p4: Fix empty submit template when editor fires up","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-01-10T11:11:53Z","receivedAt":"2010-01-10T11:11:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Kevin Leung wrote:\n> read_pipe() returns \"\\n\". We need to remove it before passing it\n> to system().\n> \n> Signed-off-by: Kevin Leung <kevinlsk@gmail.com>\n\nIf I understand correctly, this is a cosmetic change: os.system()\ncalls system(3), which uses 'sh -c', which has no problem coping with\nan extra newline at the end.  So 'need' seems too strong a word.\nStill, the change sounds sensible.\n\n> ---\n>  contrib/fast-import/git-p4 |    4 ++--\n>  1 files changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\n> index 0cef242..04bf4f4 100755\n> --- a/contrib/fast-import/git-p4\n> +++ b/contrib/fast-import/git-p4\n> @@ -732,8 +732,8 @@ class P4Submit(Command):\n>              if os.environ.has_key(\"P4EDITOR\"):\n>                  editor = os.environ.get(\"P4EDITOR\")\n>              else:\n> -                editor = read_pipe(\"git var GIT_EDITOR\")\n> -            system(editor + \" \" + fileName)\n> +                editor = read_pipe(\"git var GIT_EDITOR\").strip()\n> +            system(\"%s %s\" % (editor, fileName))\n>  \n>              response = \"y\"\n>              if os.stat(fileName).st_mtime <= mtime:\n> -- \n\nWhat is the rationale for the rewritten system() line?  I would have\nunderstood a change to\n\n\tos.spawnlp(\"sh\", \"-c\", editor + \" \\\"$@\\\"\", fileName)\n\nfor better behavior when TMPDIR contains shell metacharacters, but\neven this has nothing to do with read_pipe() returning a trailing\nnewline.\n\nJust my two cents,\nJonathan\n"},{"id":"131218","messageId":"20100110111440.GB19612@progeny.tock","threadId":"22151","inReplyTo":"20100110111153.GA19612@progeny.tock","subject":"Re: [PATCH] git-p4: Fix empty submit template when editor fires up","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-01-10T11:14:40Z","receivedAt":"2010-01-10T11:14:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Kevin Leung wrote:\n> > read_pipe() returns \"\\n\". We need to remove it before passing it\n> > to system().\n> > \n> > Signed-off-by: Kevin Leung <kevinlsk@gmail.com>\n> \n> If I understand correctly, this is a cosmetic change:\n\n... and of course I didn't the subject.  Sorry about that.  Thanks\nfor cleaning up my mess.\n\nAcked-by: Jonathan Nieder <jrnieder@gmail.com>\n\n> What is the rationale for the rewritten system() line?  I would have\n> understood a change to\n> \n> \tos.spawnlp(\"sh\", \"-c\", editor + \" \\\"$@\\\"\", fileName)\n\nI am still curious about this, though it is not so important.\n\nJonathan\n"},{"id":"131232","messageId":"e66701d41001100552p6cf2fde8m6463613cfcc4315b@mail.gmail.com","threadId":"22151","inReplyTo":"20100110111440.GB19612@progeny.tock","subject":"Re: [PATCH] git-p4: Fix empty submit template when editor fires up","fromName":"Kevin Leung","fromEmail":"kevinlsk@gmail.com","sentAt":"2010-01-10T13:52:53Z","receivedAt":"2010-01-10T13:52:53Z","isPatch":true,"sender":{"key":"kevinlsk@gmail.com","avatar":null},"body":"On Sun, Jan 10, 2010 at 7:14 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Jonathan Nieder wrote:\n>> Kevin Leung wrote:\n>> > read_pipe() returns \"\\n\". We need to remove it before passing it\n>> > to system().\n>> >\n>> > Signed-off-by: Kevin Leung <kevinlsk@gmail.com>\n>>\n>> If I understand correctly, this is a cosmetic change:\n>\n> ... and of course I didn't the subject.  Sorry about that.  Thanks\n> for cleaning up my mess.\n>\n> Acked-by: Jonathan Nieder <jrnieder@gmail.com>\n>\n>> What is the rationale for the rewritten system() line?  I would have\n>> understood a change to\n>>\n>>       os.spawnlp(\"sh\", \"-c\", editor + \" \\\"$@\\\"\", fileName)\n>\n> I am still curious about this, though it is not so important.\n\nYou are right, the system() line is not so important. I can revert\nthat line of change.\n\nKevin\n"}]}