{"thread":{"id":"39247","subject":"Bug: git-p4 edit_template() and P4EDITOR w/options","startedAt":"2015-05-04T21:26:03Z","lastAt":"2015-05-05T00:22:46Z","messageCount":8,"participants":["Chris Lasell","Junio C Hamano","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"260555","messageId":"5D2E2EAF-FFE7-437F-A716-E152E865E634@pixar.com","threadId":"39247","inReplyTo":null,"subject":"Bug: git-p4 edit_template() and P4EDITOR w/options","fromName":"Chris Lasell","fromEmail":"chrisl@pixar.com","sentAt":"2015-05-04T21:26:03Z","receivedAt":"2015-05-04T21:26:03Z","isPatch":false,"sender":{"key":"chrisl@pixar.com","avatar":null},"body":"Hello Git Folks!\n\nI recently had a user who was unable to perform a 'git p4 submit', with the following traceback:\n\n========\nTraceback (most recent call last):\n File \"/usr/local/pixar/Cellar/git/2.2.2/libexec/git-core/git-p4\", line 3302, in \n main()\n File \"/usr/local/pixar/Cellar/git/2.2.2/libexec/git-core/git-p4\", line 3296, in main\n if not cmd.run(args):\n File \"/usr/local/pixar/Cellar/git/2.2.2/libexec/git-core/git-p4\", line 1723, in run\n ok = self.applyCommit(commit)\n File \"/usr/local/pixar/Cellar/git/2.2.2/libexec/git-core/git-p4\", line 1477, in applyCommit\n if self.edit_template(fileName):\n File \"/usr/local/pixar/Cellar/git/2.2.2/libexec/git-core/git-p4\", line 1228, in edit_template\n system([editor, template_file])\n File \"/usr/local/pixar/Cellar/git/2.2.2/libexec/git-core/git-p4\", line 196, in system\n retcode = subprocess.call(cmd, shell=expand)\n File \"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py\", line 524, in call\n return Popen(*popenargs, **kwargs).wait()\n File \"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py\", line 711, in __init__\n errread, errwrite)\n File \"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py\", line 1308, in _execute_child\n raise child_exception\nOSError: [Errno 2] No such file or directory\n==========\n\nThis 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\n(which I believe is older, 1.9.x)\n\nWe chased this down to the fact that his P4EDITOR ( and GIT_EDITOR) values have a command-line option in them: /path/to/mate -w,\n\nThe 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 \ntreated as the command name.\n\nI have confirmed this by modifying the system() call in edit_template() thus, and it works:\n\n==========\nif editor.split()[-1][0] == \"-\":\n cmd = editor.split() + [template_file]\n system(cmd)\n else:\n system([editor, template_file])\n==========\n\nWhich results, if the last word of 'editor' begins with a hyphen, in [ \"/path/to/mate\", \"-w\", \"/path/to/template\"] being passed to system() rather\nthan the non-funcional [ \"/path/to/mate -w\", \"/path/to/template\"].\n\nClearly 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.\nI was just confirming my suspicion and trying to get things working for the user.\n\nThank you!\n-Chris Lasell\n\nPS: '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 \ninstantly when the document opens. The -w causes it to wait until the document window is closed.\n\nI have noticed that the help output for mate says:\n=======\nBy default mate will wait for files to be closed if the command name\nhas a \"_wait\" suffix (e.g. via a symbolic link)\n=======\n\nand I have instructed my user to do just that for now. However, options to EDITOR environment variables are not uncommon, so I wanted\nto address this to the right people\n\n-- \nChris Lasell, Apple Peeler, MacTeam\nPixar Animation Studios, Emeryville, CA\n"},{"id":"260561","messageId":"xmqqh9rsovd4.fsf@gitster.dls.corp.google.com","threadId":"39247","inReplyTo":"5D2E2EAF-FFE7-437F-A716-E152E865E634@pixar.com","subject":"Re: Bug: git-p4 edit_template() and P4EDITOR w/options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-04T22:01:27Z","receivedAt":"2015-05-04T22:01:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Lasell writes:\n\n> PS: 'mate' is the CLI invocation of the TextMate GUI editor for OS\n> X. The -w is required in this instance, or the 'mate' command\n> would return instantly when the document opens. The -w causes it\n> to wait until the document window is closed.\n>\n> I have noticed that the help output for mate says:\n> =======\n> By default mate will wait for files to be closed if the command name\n> has a \"_wait\" suffix (e.g. via a symbolic link)\n> =======\n>\n> and I have instructed my user to do just that for now.\n\nI think that is not merely \"for now\" but is the way the command and\nthe environment variable are designed to be used. A quick websearch\nfor [$EDITOR environment with parameter] found this one\n\n http://superuser.com/questions/521070/unix-environment-variables-with-arguments\n\nwhich seems to be talking about a similar issue (unrelated to Git).\n\nThe relevant part of git-p4 is this:\n\n # invoke the editor\n if os.environ.has_key(\"P4EDITOR\") and (os.environ.get(\"P4EDITOR\") != \"\"):\n editor = os.environ.get(\"P4EDITOR\")\n else:\n editor = read_pipe(\"git var GIT_EDITOR\").strip()\n system([editor, template_file])\n\nIt grabs $EDITOR (or $GIT_EDITOR) and treats it as the path to the\neditor executable, without letting shell to split that into words at\nwhitespace boundaries, so that you can say things like\n\n\tEDITOR=\"/User/me/My Programs/nano\"\n\nThe way we spawn EDITOR in our core codepaths matches what git-p4\ndoes, too:\n\n\tconst char *args[] = { editor, real_path(path), NULL };\n\tstruct child_process p = CHILD_PROCESS_INIT;\n\tint ret, sig;\n\n\tp.argv = args;\n\tp.env = env;\n\tp.use_shell = 1;\n\tif (start_command(&p) < 0)\n\t\treturn error(\"unable to start editor '%s'\", editor);\n\t...\n\nSo...\n"},{"id":"260565","messageId":"xmqq4mnsottj.fsf@gitster.dls.corp.google.com","threadId":"39247","inReplyTo":"xmqqh9rsovd4.fsf@gitster.dls.corp.google.com","subject":"Re: Bug: git-p4 edit_template() and P4EDITOR w/options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-04T22:34:48Z","receivedAt":"2015-05-04T22:34:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The relevant part of git-p4 is this:\n>\n> # invoke the editor\n> if os.environ.has_key(\"P4EDITOR\") and (os.environ.get(\"P4EDITOR\") != \"\"):\n> editor = os.environ.get(\"P4EDITOR\")\n> else:\n> editor = read_pipe(\"git var GIT_EDITOR\").strip()\n> system([editor, template_file])\n>\n> It grabs $EDITOR (or $GIT_EDITOR) and treats it as the path to the\n> editor executable, without letting shell to split that into words at\n> whitespace boundaries, so that you can say things like\n>\n> \tEDITOR=\"/User/me/My Programs/nano\"\n>\n> The way we spawn EDITOR in our core codepaths matches what git-p4\n> does, too:\n>\n> \tconst char *args[] = { editor, real_path(path), NULL };\n> \tstruct child_process p = CHILD_PROCESS_INIT;\n> \tint ret, sig;\n>\n> \tp.argv = args;\n> \tp.env = env;\n> \tp.use_shell = 1;\n> \tif (start_command(&p) < 0)\n> \t\treturn error(\"unable to start editor '%s'\", editor);\n> \t...\n>\n> So...\n\nWell, I'll take that back. I misread p.use_shell line.\n"},{"id":"260566","messageId":"xmqqzj5knf2t.fsf@gitster.dls.corp.google.com","threadId":"39247","inReplyTo":"xmqq4mnsottj.fsf@gitster.dls.corp.google.com","subject":"Re: Bug: git-p4 edit_template() and P4EDITOR w/options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-04T22:38:34Z","receivedAt":"2015-05-04T22:38:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> The relevant part of git-p4 is this:\n>>\n>> # invoke the editor\n>> if os.environ.has_key(\"P4EDITOR\") and (os.environ.get(\"P4EDITOR\") != \"\"):\n>> editor = os.environ.get(\"P4EDITOR\")\n>> else:\n>> editor = read_pipe(\"git var GIT_EDITOR\").strip()\n>> system([editor, template_file])\n>>\n>> It grabs $EDITOR (or $GIT_EDITOR) and treats it as the path to the\n>> editor executable, without letting shell to split that into words at\n>> whitespace boundaries, so that you can say things like\n>>\n>> \tEDITOR=\"/User/me/My Programs/nano\"\n>>\n>> The way we spawn EDITOR in our core codepaths matches what git-p4\n>> does, too:\n>>\n>> \tconst char *args[] = { editor, real_path(path), NULL };\n>> \tstruct child_process p = CHILD_PROCESS_INIT;\n>> \tint ret, sig;\n>>\n>> \tp.argv = args;\n>> \tp.env = env;\n>> \tp.use_shell = 1;\n>> \tif (start_command(&p) < 0)\n>> \t\treturn error(\"unable to start editor '%s'\", editor);\n>> \t...\n>>\n>> So...\n>\n> Well, I'll take that back. I misread p.use_shell line.\n\nSo, use_shell==true codepath grabs that array with \"editor\", \"path\"\nand turns it into an equivalent to\n\n\tsh -c \"$EDITOR\" \"$path\"\n\nwhen $EDITOR has a \"magic\" character (including whitespace) in it.\nSo what git-p4 does is *not* in line with how we use the environment\nvariable.\n\nPerhaps a single-liner patch like this would work?\n\n-\tsystem([editor, template_file])\n+\tsystem([\"sh\", \"-c\", editor, template_file])\n"},{"id":"260567","messageId":"xmqqvbg8netq.fsf@gitster.dls.corp.google.com","threadId":"39247","inReplyTo":"xmqqzj5knf2t.fsf@gitster.dls.corp.google.com","subject":"Re: Bug: git-p4 edit_template() and P4EDITOR w/options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-04T22:44:01Z","receivedAt":"2015-05-04T22:44:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Perhaps a single-liner patch like this would work?\n>\n> -\tsystem([editor, template_file])\n> +\tsystem([\"sh\", \"-c\", editor, template_file])\n\nNah, scratch that. The list should be more like\n\n\t[\"sh\", \"-c\", \"%s %s\" % (editor, shlex.quote(template_file))]\n\nif your Python has shlex.quote, that is; it may be pipes.quote\ninstead.\n"},{"id":"260568","messageId":"8AC0C799-2402-4AAB-8C9D-271FBF14910D@pixar.com","threadId":"39247","inReplyTo":"xmqqzj5knf2t.fsf@gitster.dls.corp.google.com","subject":"Re: Bug: git-p4 edit_template() and P4EDITOR w/options","fromName":"Chris Lasell","fromEmail":"chrisl@pixar.com","sentAt":"2015-05-04T22:46:35Z","receivedAt":"2015-05-04T22:46:35Z","isPatch":false,"sender":{"key":"chrisl@pixar.com","avatar":null},"body":"\n> On May 4, 2015, at 3:38 PM, Junio C Hamano wrote:\n> \n> Junio C Hamano writes:\n> \n> So, use_shell==true codepath grabs that array with \"editor\", \"path\"\n> and turns it into an equivalent to\n> \n> \tsh -c \"$EDITOR\" \"$path\"\n> \n> when $EDITOR has a \"magic\" character (including whitespace) in it.\n> So what git-p4 does is *not* in line with how we use the environment\n> variable.\n> \n> Perhaps a single-liner patch like this would work?\n> \n> -\tsystem([editor, template_file])\n> +\tsystem([\"sh\", \"-c\", editor, template_file])\n> \n\nThanks for the input Junio!\n\nI’ll try it out, but leave the decision about patching for the maintainers :-)\n\nThough 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:\n\ndef system(cmd):\n expand = isinstance(cmd,basestring)\n if verbose:\n sys.stderr.write(\"executing %s\\n\" % str(cmd))\n retcode = subprocess.call(cmd, shell=expand)\n if retcode:\n raise CalledProcessError(retcode, cmd)\n\nand the edit_template() function is passing a list\n\nI 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.\n\nFWIW, my user is all set now, but I’m happy to participate in this conversation if appropriate :-)\n\nCheers,\n-Chris\n"},{"id":"260569","messageId":"20150504231136.GY5467@google.com","threadId":"39247","inReplyTo":"xmqqvbg8netq.fsf@gitster.dls.corp.google.com","subject":"Re: Bug: git-p4 edit_template() and P4EDITOR w/options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2015-05-04T23:11:36Z","receivedAt":"2015-05-04T23:11:36Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Junio C Hamano writes:\n\n>> Perhaps a single-liner patch like this would work?\n>>\n>> -\tsystem([editor, template_file])\n>> +\tsystem([\"sh\", \"-c\", editor, template_file])\n>\n> Nah, scratch that. The list should be more like\n>\n> \t[\"sh\", \"-c\", \"%s %s\" % (editor, shlex.quote(template_file))]\n\n*nod*\n\nrun-command.c does\n\n\tsystem([\"sh\", \"-c\", ('%s \"$@\"' % editor), editor, template_file])\n\nto avoid having to figure out how to shell-quote template_file.\n"},{"id":"260573","messageId":"xmqq8ud3g9ex.fsf@gitster.dls.corp.google.com","threadId":"39247","inReplyTo":"20150504231136.GY5467@google.com","subject":"Re: Bug: git-p4 edit_template() and P4EDITOR w/options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-05T00:22:46Z","receivedAt":"2015-05-05T00:22:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> run-command.c does\n>\n> \tsystem([\"sh\", \"-c\", ('%s \"$@\"' % editor), editor, template_file])\n>\n> to avoid having to figure out how to shell-quote template_file.\n\nYup, thanks.\n"}]}