{"thread":{"id":"26067","subject":"[PATCH] git-p4: Fix 'p4 opened' in git-p4 for names with spaces","startedAt":"2010-12-14T20:56:04Z","lastAt":"2011-01-15T14:35:32Z","messageCount":8,"participants":["Jerzy Kozera","Junio C Hamano","Reece Dunn","Andreas Schwab","Pete Wyckoff"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"158092","messageId":"1292360165-26771-1-git-send-email-jerzy.kozera@gmail.com","threadId":"26067","inReplyTo":null,"subject":"[PATCH] git-p4: Fix 'p4 opened' in git-p4 for names with spaces","fromName":"Jerzy Kozera","fromEmail":"jerzy.kozera@gmail.com","sentAt":"2010-12-14T20:56:04Z","receivedAt":"2010-12-14T20:56:04Z","isPatch":true,"sender":{"key":"jerzy.kozera@gmail.com","avatar":"https://gravatar.com/avatar/f9b73794f2e389bd9b490213b1aced0cfced339c940e2f657ab43fedb21291e8?d=mp&s=160"},"body":"There is problem with git-p4 when trying to submit changes to file containing spaces in name - submit fails with \"Command failed: p4 opened [name with spaces here]\"\n\nIt's caused by not quoting name for p4 opened, and the patch attached fixes it.\n\nJerzy Kozera (1):\n  Fix 'p4 opened' in git-p4 for names with spaces\n\n contrib/fast-import/git-p4 |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n"},{"id":"158091","messageId":"1292360165-26771-2-git-send-email-jerzy.kozera@gmail.com","threadId":"26067","inReplyTo":"1292360165-26771-1-git-send-email-jerzy.kozera@gmail.com","subject":"[PATCH] git-p4: Fix 'p4 opened' in git-p4 for names with spaces","fromName":"Jerzy Kozera","fromEmail":"jerzy.kozera@gmail.com","sentAt":"2010-12-14T20:56:05Z","receivedAt":"2010-12-14T20:56:05Z","isPatch":true,"sender":{"key":"jerzy.kozera@gmail.com","avatar":"https://gravatar.com/avatar/f9b73794f2e389bd9b490213b1aced0cfced339c940e2f657ab43fedb21291e8?d=mp&s=160"},"body":"Signed-off-by: Jerzy Kozera <jerzy.kozera@gmail.com>\n---\n contrib/fast-import/git-p4 |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 04ce7e3..a5297e7 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -144,7 +144,7 @@ def setP4ExecBit(file, mode):\n def getP4OpenedType(file):\n     # Returns the perforce file type for the given file.\n \n-    result = p4_read_pipe(\"opened %s\" % file)\n+    result = p4_read_pipe(\"opened \\\"%s\\\"\" % file)\n     match = re.match(\".*\\((.+)\\)\\r?$\", result)\n     if match:\n         return match.group(1)\n-- \n1.6.5.2\n"},{"id":"158121","messageId":"7vvd2wq72l.fsf@alter.siamese.dyndns.org","threadId":"26067","inReplyTo":"1292360165-26771-2-git-send-email-jerzy.kozera@gmail.com","subject":"Re: [PATCH] git-p4: Fix 'p4 opened' in git-p4 for names with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-14T23:16:02Z","receivedAt":"2010-12-14T23:16:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerzy Kozera <jerzy.kozera@gmail.com> writes:\n\n> Signed-off-by: Jerzy Kozera <jerzy.kozera@gmail.com>\n> ---\n>  contrib/fast-import/git-p4 |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\n> index 04ce7e3..a5297e7 100755\n> --- a/contrib/fast-import/git-p4\n> +++ b/contrib/fast-import/git-p4\n> @@ -144,7 +144,7 @@ def setP4ExecBit(file, mode):\n>  def getP4OpenedType(file):\n>      # Returns the perforce file type for the given file.\n>  \n> -    result = p4_read_pipe(\"opened %s\" % file)\n> +    result = p4_read_pipe(\"opened \\\"%s\\\"\" % file)\n\nDon't you need a lot more than that?  What if file has \" or \\ in it?\n"},{"id":"158124","messageId":"AANLkTi=Cp=FCuJdthr7JfML6jdNzUiDAUPjrWpTQfWGk@mail.gmail.com","threadId":"26067","inReplyTo":"7vvd2wq72l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-p4: Fix 'p4 opened' in git-p4 for names with spaces","fromName":"Reece Dunn","fromEmail":"msclrhd@googlemail.com","sentAt":"2010-12-14T23:36:56Z","receivedAt":"2010-12-14T23:36:56Z","isPatch":true,"sender":{"key":"msclrhd@googlemail.com","avatar":null},"body":"On 14 December 2010 23:16, Junio C Hamano <gitster@pobox.com> wrote:\n> Jerzy Kozera <jerzy.kozera@gmail.com> writes:\n>\n>> Signed-off-by: Jerzy Kozera <jerzy.kozera@gmail.com>\n>> ---\n>>  contrib/fast-import/git-p4 |    2 +-\n>>  1 files changed, 1 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\n>> index 04ce7e3..a5297e7 100755\n>> --- a/contrib/fast-import/git-p4\n>> +++ b/contrib/fast-import/git-p4\n>> @@ -144,7 +144,7 @@ def setP4ExecBit(file, mode):\n>>  def getP4OpenedType(file):\n>>      # Returns the perforce file type for the given file.\n>>\n>> -    result = p4_read_pipe(\"opened %s\" % file)\n>> +    result = p4_read_pipe(\"opened \\\"%s\\\"\" % file)\n>\n> Don't you need a lot more than that?  What if file has \" or \\ in it?\n\nThose are invalid characters for a filename on Windows, so cannot be\nentered/present in the filename. On Linux, they are accepted, but\ndon't get put into the filename, so it all depends on where the data\nfor file comes from (API call or user/external source). Not sure how\nMac/BSD/Solaris handle those characters.\n\nThis looks fine to me, but I wonder if there are other places\nreferencing file paths that require quoting to correctly handle\nspaces.\n\nAlso, escaping the quote characters can be avoided by using single\nquoted string literals:\n\n+    result = p4_read_pipe('opened \"%s\"' % file)\n\n- Reece\n"},{"id":"159449","messageId":"1294944715-5647-1-git-send-email-jerzy.kozera@gmail.com","threadId":"26067","inReplyTo":"AANLkTi=Cp=FCuJdthr7JfML6jdNzUiDAUPjrWpTQfWGk@mail.gmail.com","subject":"[PATCH] git-p4: Fixed handling of file names with spaces","fromName":"Jerzy Kozera","fromEmail":"jerzy.kozera@gmail.com","sentAt":"2011-01-13T18:51:55Z","receivedAt":"2011-01-13T18:51:55Z","isPatch":true,"sender":{"key":"jerzy.kozera@gmail.com","avatar":"https://gravatar.com/avatar/f9b73794f2e389bd9b490213b1aced0cfced339c940e2f657ab43fedb21291e8?d=mp&s=160"},"body":"Hi,\n\nI've noticed the same issue in reopen and rm calls - not saying these three are all occurences of this problem, but I guess fixing them is a good start.\n\nI'm using \\\" instead of '' quoting for consistency with rest of the file.\n\nRegards,\nJerzy\n---\n contrib/fast-import/git-p4 |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 04ce7e3..2147315 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -139,12 +139,12 @@ def setP4ExecBit(file, mode):\n         if p4Type[-1] == \"+\":\n             p4Type = p4Type[0:-1]\n \n-    p4_system(\"reopen -t %s %s\" % (p4Type, file))\n+    p4_system(\"reopen -t %s \\\"%s\\\"\" % (p4Type, file))\n \n def getP4OpenedType(file):\n     # Returns the perforce file type for the given file.\n \n-    result = p4_read_pipe(\"opened %s\" % file)\n+    result = p4_read_pipe(\"opened \\\"%s\\\"\" % file)\n     match = re.match(\".*\\((.+)\\)\\r?$\", result)\n     if match:\n         return match.group(1)\n@@ -666,7 +666,7 @@ class P4Submit(Command):\n                 for f in editedFiles:\n                     p4_system(\"revert \\\"%s\\\"\" % f);\n                 for f in filesToAdd:\n-                    system(\"rm %s\" %f)\n+                    system(\"rm \\\"%s\\\"\" % f)\n                 return\n             elif response == \"a\":\n                 os.system(applyPatchCmd)\n-- \n1.7.1\n"},{"id":"159511","messageId":"m28vyncffu.fsf@igel.home","threadId":"26067","inReplyTo":"1294944715-5647-1-git-send-email-jerzy.kozera@gmail.com","subject":"Re: [PATCH] git-p4: Fixed handling of file names with spaces","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-01-14T22:01:57Z","receivedAt":"2011-01-14T22:01:57Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Jerzy Kozera <jerzy.kozera@gmail.com> writes:\n\n> I've noticed the same issue in reopen and rm calls - not saying these three are all occurences of this problem, but I guess fixing them is a good start.\n\nCan those file names also include a double quote or a backquote or a\ndollar sign?\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"159513","messageId":"A0F152FE-C659-4F9B-9625-505AA5DAF942@gmail.com","threadId":"26067","inReplyTo":"m28vyncffu.fsf@igel.home","subject":"Re: [PATCH] git-p4: Fixed handling of file names with spaces","fromName":"Jerzy Kozera","fromEmail":"jerzy.kozera@gmail.com","sentAt":"2011-01-14T22:45:45Z","receivedAt":"2011-01-14T22:45:45Z","isPatch":true,"sender":{"key":"jerzy.kozera@gmail.com","avatar":"https://gravatar.com/avatar/f9b73794f2e389bd9b490213b1aced0cfced339c940e2f657ab43fedb21291e8?d=mp&s=160"},"body":"On 14 Jan 2011, at 22:01, Andreas Schwab wrote:\n> Can those file names also include a double quote or a backquote or a\n> dollar sign?\n\n\nDouble quote and backquote get escaped by git so they are not a problem:\n$ git diff-tree -r HEAD^ HEAD\n:000000 100644 0000000000000000000000000000000000000000 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 A\t\"\\\" \\\\ $\"\n\nBut as you can see above, the dollar sign remains intact, so it needs to be handled as well - patch below takes it into account.\n\nThanks,\nJerzy\n\n---\n contrib/fast-import/git-p4 |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 04ce7e3..d930908 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -47,7 +47,7 @@ def p4_build_cmd(cmd):\n     real_cmd += \"%s\" % (cmd)\n     if verbose:\n         print real_cmd\n-    return real_cmd\n+    return real_cmd.replace('$', '\\\\$')\n \n def chdir(dir):\n     if os.name == 'nt':\n@@ -139,12 +139,12 @@ def setP4ExecBit(file, mode):\n         if p4Type[-1] == \"+\":\n             p4Type = p4Type[0:-1]\n \n-    p4_system(\"reopen -t %s %s\" % (p4Type, file))\n+    p4_system(\"reopen -t %s \\\"%s\\\"\" % (p4Type, file))\n \n def getP4OpenedType(file):\n     # Returns the perforce file type for the given file.\n \n-    result = p4_read_pipe(\"opened %s\" % file)\n+    result = p4_read_pipe(\"opened \\\"%s\\\"\" % file)\n     match = re.match(\".*\\((.+)\\)\\r?$\", result)\n     if match:\n         return match.group(1)\n@@ -666,7 +666,7 @@ class P4Submit(Command):\n                 for f in editedFiles:\n                     p4_system(\"revert \\\"%s\\\"\" % f);\n                 for f in filesToAdd:\n-                    system(\"rm %s\" %f)\n+                    system(\"rm \\\"%s\\\"\" % f.replace('$', '\\\\$'))\n                 return\n             elif response == \"a\":\n                 os.system(applyPatchCmd)\n-- \n1.7.1\n"},{"id":"159529","messageId":"20110115143532.GB31622@arf.padd.com","threadId":"26067","inReplyTo":"A0F152FE-C659-4F9B-9625-505AA5DAF942@gmail.com","subject":"Re: [PATCH] git-p4: Fixed handling of file names with spaces","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2011-01-15T14:35:32Z","receivedAt":"2011-01-15T14:35:32Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"jerzy.kozera@gmail.com wrote on Fri, 14 Jan 2011 22:45 +0000:\n> On 14 Jan 2011, at 22:01, Andreas Schwab wrote:\n> > Can those file names also include a double quote or a backquote or a\n> > dollar sign?\n> \n> \n> Double quote and backquote get escaped by git so they are not a problem:\n> $ git diff-tree -r HEAD^ HEAD\n> :000000 100644 0000000000000000000000000000000000000000 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 A\t\"\\\" \\\\ $\"\n> \n> But as you can see above, the dollar sign remains intact, so it needs to be handled as well - patch below takes it into account.\n[..]\n> -    p4_system(\"reopen -t %s %s\" % (p4Type, file))\n> +    p4_system(\"reopen -t %s \\\"%s\\\"\" % (p4Type, file))\n\nThese changes are important for correctness.  Thanks for fixing\nthem.\n\nIt is kind of ugly to have to do file escaping all over the\nsource.  I'd rather see all the os.system() calls go away, in\nfavor of subprocess.Popen().  You can use the latter without\ngoing through the shell at all, hence no escapes are needed.\nIf you feel ambitious, this would be a nice fix.\n\nSpaces can happen in depot paths too.  That isn't handled\ncurrent.  All the p4Cmd and p4CmdList calls that work on\ndepotPaths should avoid going through the shell too.\n\nBut at least what you have done already should go in.  If you\nfeel adventurous, addressing these other space-related issues\nwould be nice too.\n\n\t\t-- Pete\n"}]}