{"thread":{"id":"40229","subject":"[PATCH] git-p4: add \"--path-encoding\" option","startedAt":"2015-08-31T15:40:15Z","lastAt":"2015-08-31T20:09:55Z","messageCount":5,"participants":["larsxschneider@gmail.com","Junio C Hamano","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"268996","messageId":"1441035616-39128-1-git-send-email-larsxschneider@gmail.com","threadId":"40229","inReplyTo":null,"subject":"[PATCH] git-p4: add \"--path-encoding\" option","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-08-31T15:40:15Z","receivedAt":"2015-08-31T15:40:15Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nHi,\n\nI think I discovered a path encoding issue if you migrate P4 repositories that contain path names generated with Windows. I added a test case to prove my point. Character encoding is a complicated topic. Feedback is highly appreciated.\n\nThanks,\nLars\n\nLars Schneider (1):\n git-p4: add \"--path-encoding\" option\n\n Documentation/git-p4.txt | 4 ++++\n git-p4.py | 6 ++++++\n t/t9821-git-p4-path-encoding.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 48 insertions(+)\n create mode 100755 t/t9821-git-p4-path-encoding.sh\n\n--\n2.5.1.1.g36ff854\n"},{"id":"268997","messageId":"1441035616-39128-2-git-send-email-larsxschneider@gmail.com","threadId":"40229","inReplyTo":"1441035616-39128-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH] git-p4: add \"--path-encoding\" option","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-08-31T15:40:16Z","receivedAt":"2015-08-31T15:40:16Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nSigned-off-by: Lars Schneider \n---\n Documentation/git-p4.txt | 4 ++++\n git-p4.py | 6 ++++++\n t/t9821-git-p4-path-encoding.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 48 insertions(+)\n create mode 100755 t/t9821-git-p4-path-encoding.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex 82aa5d6..98b6c0f 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -252,6 +252,10 @@ Git repository:\n \tUse a client spec to find the list of interesting files in p4.\n \tSee the \"CLIENT SPEC\" section below.\n \n+----path-encoding ::\n+\tThe encoding to use when reading p4 client paths. With this option\n+\tnon ASCII paths are properly stored in Git. For example, the encoding 'cp1252' is often used on Windows systems.\n+\n -/ ::\n \tExclude selected depot paths when cloning or syncing.\n \ndiff --git a/git-p4.py b/git-p4.py\nindex 073f87b..2b3bfc4 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1981,6 +1981,8 @@ class P4Sync(Command, P4UserMap):\n optparse.make_option(\"--silent\", dest=\"silent\", action=\"store_true\"),\n optparse.make_option(\"--detect-labels\", dest=\"detectLabels\", action=\"store_true\"),\n optparse.make_option(\"--import-labels\", dest=\"importLabels\", action=\"store_true\"),\n+ optparse.make_option(\"--path-encoding\", dest=\"pathEncoding\", type=\"string\",\n+ help=\"Encoding to use for paths\"),\n optparse.make_option(\"--import-local\", dest=\"importIntoRemotes\", action=\"store_false\",\n help=\"Import into refs/heads/ , not refs/remotes\"),\n optparse.make_option(\"--max-changes\", dest=\"maxChanges\",\n@@ -2025,6 +2027,7 @@ class P4Sync(Command, P4UserMap):\n self.clientSpecDirs = None\n self.tempBranches = []\n self.tempBranchLocation = \"git-p4-tmp\"\n+ self.pathEncoding = None\n \n if gitConfig(\"git-p4.syncFromOrigin\") == \"false\":\n self.syncWithOrigin = False\n@@ -2213,6 +2216,9 @@ class P4Sync(Command, P4UserMap):\n text = regexp.sub(r'$\\1$', text)\n contents = [ text ]\n \n+ if self.pathEncoding:\n+ relPath = relPath.decode(self.pathEncoding).encode('utf8', 'replace')\n+\n self.gitStream.write(\"M %s inline %s\\n\" % (git_mode, relPath))\n \n # total length...\ndiff --git a/t/t9821-git-p4-path-encoding.sh b/t/t9821-git-p4-path-encoding.sh\nnew file mode 100755\nindex 0000000..f6bb79c\n--- /dev/null\n+++ b/t/t9821-git-p4-path-encoding.sh\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+\n+test_description='Clone repositories with non ASCII paths'\n+\n+. ./lib-git-p4.sh\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'Create a repo containing cp1251 encoded paths' '\n+\tcd \"$cli\" &&\n+\n+\tFILENAME=\"$(echo \"a-ä_o-ö_u-ü.txt\" | iconv -f utf-8 -t cp1252)\" &&\n+\t>\"$FILENAME\" &&\n+\tp4 add \"$FILENAME\" &&\n+\tp4 submit -d \"test\"\n+'\n+\n+test_expect_success 'Clone repo containing cp1251 encoded paths' '\n+\tgit p4 clone --destination=\"$git\" --path-encoding=cp1252 //depot &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit init . &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\t\"a-\\303\\244_o-\\303\\266_u-\\303\\274.txt\"\n+\t\tEOF\n+\t\tgit ls-files >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'kill p4d' '\n+\tkill_p4d\n+'\n+\n+test_done\n-- \n2.5.1.1.g36ff854\n"},{"id":"269008","messageId":"xmqqpp23we4d.fsf@gitster.mtv.corp.google.com","threadId":"40229","inReplyTo":"1441035616-39128-2-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH] git-p4: add \"--path-encoding\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-31T17:40:18Z","receivedAt":"2015-08-31T17:40:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> From: Lars Schneider \n>\n> Signed-off-by: Lars Schneider \n> ---\n> Documentation/git-p4.txt | 4 ++++\n> git-p4.py | 6 ++++++\n> t/t9821-git-p4-path-encoding.sh | 38 ++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 48 insertions(+)\n> create mode 100755 t/t9821-git-p4-path-encoding.sh\n>\n> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n> index 82aa5d6..98b6c0f 100644\n> --- a/Documentation/git-p4.txt\n> +++ b/Documentation/git-p4.txt\n> @@ -252,6 +252,10 @@ Git repository:\n> \tUse a client spec to find the list of interesting files in p4.\n> \tSee the \"CLIENT SPEC\" section below.\n> \n> +----path-encoding ::\n> +\tThe encoding to use when reading p4 client paths. With this option\n> +\tnon ASCII paths are properly stored in Git. For example, the encoding 'cp1252' is often used on Windows systems.\n> +\n\nThis line is overly long. Let AsciiDoc wrap it upon output and keep\nthe source within a reasonable limit (see existing lines around the\nnew text to see what is considered reasonable).\n\nDo I see too many dashes before the option name, by the way, or is\nit my e-mail client tricking my eyes?\n\n> diff --git a/t/t9821-git-p4-path-encoding.sh b/t/t9821-git-p4-path-encoding.sh\n> new file mode 100755\n> index 0000000..f6bb79c\n> --- /dev/null\n> +++ b/t/t9821-git-p4-path-encoding.sh\n> @@ -0,0 +1,38 @@\n> +#!/bin/sh\n> +\n> +test_description='Clone repositories with non ASCII paths'\n> +\n> +. ./lib-git-p4.sh\n> +\n> +test_expect_success 'start p4d' '\n> +\tstart_p4d\n> +'\n> +\n> +test_expect_success 'Create a repo containing cp1251 encoded paths' '\n> +\tcd \"$cli\" &&\n> +\n> +\tFILENAME=\"$(echo \"a-�_o-�_u-�.txt\" | iconv -f utf-8 -t cp1252)\" &&\n\nHmm, we'd be better off not having a bare UTF-8 sequence in the\nsource like this, especially when you already have the same thing\nbackslash-escaped in the \"expect\" file below. Perhaps\n\n\tNAME=\"a-\\303\\244_o-\\303\\266_u-\\303\\274.txt\" &&\n\n\tUTF8=$(printf \"$NAME\") &&\n CP1252=$(printf \"$NAME\" | iconv -t cp1252) &&\n echo \"\\\"$UTF8\\\"\" >expect &&\n\n >\"$CP1252\" &&\n p4 add \"$CP1252\" &&\n ...\n\nor something along that line?\n\n> +\t>\"$FILENAME\" &&\n> +\tp4 add \"$FILENAME\" &&\n> +\tp4 submit -d \"test\"\n> +'\n> +\n> +test_expect_success 'Clone repo containing cp1251 encoded paths' '\n> +\tgit p4 clone --destination=\"$git\" --path-encoding=cp1252 //depot &&\n> +\ttest_when_finished cleanup_git &&\n> +\t(\n> +\t\tcd \"$git\" &&\n> +\t\tgit init . &&\n> +\t\tcat >expect <<-\\EOF &&\n> +\t\t\"a-\\303\\244_o-\\303\\266_u-\\303\\274.txt\"\n> +\t\tEOF\n> +\t\tgit ls-files >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> +test_expect_success 'kill p4d' '\n> +\tkill_p4d\n> +'\n> +\n> +test_done\n"},{"id":"269034","messageId":"55E4A965.8040305@web.de","threadId":"40229","inReplyTo":"xmqqpp23we4d.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-p4: add \"--path-encoding\" option","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-08-31T19:22:13Z","receivedAt":"2015-08-31T19:22:13Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-08-31 19.40, Junio C Hamano wrote:\n> larsxschneider@gmail.com writes:\n\n>> +test_expect_success 'Create a repo containing cp1251 encoded paths' '\n>> +\tcd \"$cli\" &&\n>> +\n>> +\tFILENAME=\"$(echo \"a-¤_o-¶_u-¼.txt\" | iconv -f utf-8 -t cp1252)\" &&\n> \n> Hmm, we'd be better off not having a bare UTF-8 sequence in the\n> source like this, especially when you already have the same thing\n> backslash-escaped in the \"expect\" file below. Perhaps\n> \n> \tNAME=\"a-\\303\\244_o-\\303\\266_u-\\303\\274.txt\" &&\n> \n> \tUTF8=$(printf \"$NAME\") &&\n> CP1252=$(printf \"$NAME\" | iconv -t cp1252) &&\n> echo \"\\\"$UTF8\\\"\" >expect &&\n> \n> >\"$CP1252\" &&\n> p4 add \"$CP1252\" &&\n> ...\n> \nUsing file names and iconv like this may not be portable:\n- cp1252 may be called CP1252 (or may not be available)\n- reading from stdin is not necessarily supported by iconv\n- creating files in CP1252 may not be supported under Mac OS\n (Not sure about Windows)\n\n\nOne solution could be to use ISO-8859-1, convert into UTF-8,\nand \"convert into UTF-8\" one more time.\n\nWe can skip using iconv in the test case completely, and use\nsomething like this:\n(Fully untested)\n\nUTF8=$(printf '\\303\\203\\302\\204')\nNAME=$(printf '\\303\\204')\n"},{"id":"269043","messageId":"xmqqa8t7usmk.fsf@gitster.mtv.corp.google.com","threadId":"40229","inReplyTo":"55E4A965.8040305@web.de","subject":"Re: [PATCH] git-p4: add \"--path-encoding\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-31T20:09:55Z","receivedAt":"2015-08-31T20:09:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> On 2015-08-31 19.40, Junio C Hamano wrote:\n>> larsxschneider@gmail.com writes:\n>\n>>> +test_expect_success 'Create a repo containing cp1251 encoded paths' '\n>>> +\tcd \"$cli\" &&\n>>> +\n>>> +\tFILENAME=\"$(echo \"a-¤_o-¶_u-¼.txt\" | iconv -f utf-8 -t cp1252)\" &&\n>> ...\n> Using file names and iconv like this may not be portable:\n> - cp1252 may be called CP1252 (or may not be available)\n\n\"git grep 'cp[0-9]' t/\" does tell us that we refrain from using them\nand I am sure the portability worries is a big reason. Thank you\nfor pointing it out.\n\n> - reading from stdin is not necessarily supported by iconv\n\n\"git grep '| iconv' t/\" tells me that this is irrelevant; we already\nheavily depend on it.\n\n> - creating files in CP1252 may not be supported under Mac OS\n> (Not sure about Windows)\n\nThe same as the first point, which is a good thing to worry about.\n\n> One solution could be to use ISO-8859-1, convert into UTF-8,\n> and \"convert into UTF-8\" one more time.\n\nI do not quite get it; do you need to do anything more than just\nreplacing cp1252 with iso-8859-1 in the patch being discussed?\n"}]}