{"thread":{"id":"28383","subject":"[PATCH] git-p4: import utf16 file properly","startedAt":"2011-09-13T21:33:14Z","lastAt":"2011-09-18T01:19:42Z","messageCount":6,"participants":["Chris Li","Luke Diamand","Pete Wyckoff"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"175420","messageId":"CANeU7QndA0yv1OzU3vta5B8r8nCRdBSqTy0Rboc_bbpst+1pcw@mail.gmail.com","threadId":"28383","inReplyTo":null,"subject":"[PATCH] git-p4: import utf16 file properly","fromName":"Chris Li","fromEmail":"git@chrisli.org","sentAt":"2011-09-13T21:33:14Z","receivedAt":"2011-09-13T21:33:14Z","isPatch":true,"sender":{"key":"git@chrisli.org","avatar":null},"body":"The current git-p4 does not handle utf16 files properly.\nThe \"p4 print\" command, when output to stdout, converts the\nutf16 file into utf8. That effectively imported the utf16 file\nas utf8 for git. In other words, git-p4 import a different\nfile compare to file check out by perforce. This breakes my\nwindows build in the company project.\n\nThe fix is simple, just ask perforce to print the depot\nfile into a real file. This way perforce will not performe\nthe utf16 to utf8 conversion. Git can import the exact same\nfile as perforce checkout.\n---\n contrib/fast-import/git-p4 | 5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 6b9de9e..5fb1ac7 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -1239,6 +1239,11 @@ class P4Sync(Command, P4UserMap):\n contents = map(lambda text:\nre.sub(r'(?i)\\$(Id|Header):[^$]*\\$',r'$\\1$', text), contents)\n elif file['type'] in ('text+k', 'ktext', 'kxtext',\n'unicode+k', 'binary+k'):\n contents = map(lambda text:\nre.sub(r'\\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$\\n]*\\$',r'$\\1$',\ntext), contents)\n+ elif file['type'] == 'utf16':\n+ tmpFile = tempfile.NamedTemporaryFile()\n+ p4CmdList(\"print -o %s %s\"%(tmpFile.name, file['depotFile']))\n+ contents = [ open(tmpFile.name).read() ]\n+ tmpFile.close()\n\n self.gitStream.write(\"M %s inline %s\\n\" % (mode, relPath))\n\n-- \n1.7.6\n"},{"id":"175456","messageId":"4E705DF8.1040508@diamand.org","threadId":"28383","inReplyTo":"CANeU7QndA0yv1OzU3vta5B8r8nCRdBSqTy0Rboc_bbpst+1pcw@mail.gmail.com","subject":"Re: [PATCH] git-p4: import utf16 file properly","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2011-09-14T07:55:36Z","receivedAt":"2011-09-14T07:55:36Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 13/09/11 22:33, Chris Li wrote:\n> The current git-p4 does not handle utf16 files properly.\n> The \"p4 print\" command, when output to stdout, converts the\n> utf16 file into utf8. That effectively imported the utf16 file\n> as utf8 for git. In other words, git-p4 import a different\n> file compare to file check out by perforce. This breakes my\n> windows build in the company project.\n>\n> The fix is simple, just ask perforce to print the depot\n> file into a real file. This way perforce will not performe\n> the utf16 to utf8 conversion. Git can import the exact same\n> file as perforce checkout.\n\nDoes this change do the right thing with RCS keywords in UTF16 files?\n\nIf p4CmdList() fails, e.g. due to running out of diskspace, will this \njust happily import a truncated/corrupt file?\n\n(And I could be wrong about this, but does you patch have newline \ndamage? It didn't seem to apply for me).\n\nRegards!\nLuke\n\n> ---\n> contrib/fast-import/git-p4 | 5 +++++\n> 1 files changed, 5 insertions(+), 0 deletions(-)\n>\n> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\n> index 6b9de9e..5fb1ac7 100755\n> --- a/contrib/fast-import/git-p4\n> +++ b/contrib/fast-import/git-p4\n> @@ -1239,6 +1239,11 @@ class P4Sync(Command, P4UserMap):\n> contents = map(lambda text:\n> re.sub(r'(?i)\\$(Id|Header):[^$]*\\$',r'$\\1$', text), contents)\n> elif file['type'] in ('text+k', 'ktext', 'kxtext',\n> 'unicode+k', 'binary+k'):\n> contents = map(lambda text:\n> re.sub(r'\\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$\\n]*\\$',r'$\\1$',\n> text), contents)\n> + elif file['type'] == 'utf16':\n> + tmpFile = tempfile.NamedTemporaryFile()\n> + p4CmdList(\"print -o %s %s\"%(tmpFile.name, file['depotFile']))\n> + contents = [ open(tmpFile.name).read() ]\n> + tmpFile.close()\n>\n> self.gitStream.write(\"M %s inline %s\\n\" % (mode, relPath))\n>\n"},{"id":"175487","messageId":"CANeU7QnW5kSni0W9M9q-FTWv4p_qc67LG3mA6BQj_U-wxNuZeQ@mail.gmail.com","threadId":"28383","inReplyTo":"4E705DF8.1040508@diamand.org","subject":"Re: [PATCH] git-p4: import utf16 file properly","fromName":"Chris Li","fromEmail":"git@chrisli.org","sentAt":"2011-09-14T18:29:26Z","receivedAt":"2011-09-14T18:29:26Z","isPatch":true,"sender":{"key":"git@chrisli.org","avatar":null},"body":"On Wed, Sep 14, 2011 at 12:55 AM, Luke Diamand wrote:\n> On 13/09/11 22:33, Chris Li wrote:\n>> The fix is simple, just ask perforce to print the depot\n>> file into a real file. This way perforce will not performe\n>> the utf16 to utf8 conversion. Git can import the exact same\n>> file as perforce checkout.\n>\n> Does this change do the right thing with RCS keywords in UTF16 files?\n\nI don't know what is the rules about the RCS keyword in UTF16 files.\nI look at the current git-p4, it does not do any keyword replacement in\nutf16 files. So this patch did not change that. It should be a separate issue.\n\nThe way I see it, this patch is a straight enhancement compare to the\ncurrent git-p4 because the current git-p4 *corrupts* the utf16 files.\n\n>\n> If p4CmdList() fails, e.g. due to running out of diskspace, will this just\n> happily import a truncated/corrupt file?\n\nGood point. I add the error check and attach the new patch.\n\n> (And I could be wrong about this, but does you patch have newline damage? It\n> didn't seem to apply for me).\n\nGmail dmage the white space. I should always use the attachment.\nDoes the attached patch work for you?\n\nThanks\n\nChris\n\n\nFrom 06de9cfdcd89e8bfb6575f40d36fdfcefe1a1985 Mon Sep 17 00:00:00 2001\nFrom: Chris Li \nDate: Tue, 13 Sep 2011 13:57:31 -0700\nSubject: [PATCH] git-p4: import utf16 file properly\n\nThe current git-p4 does not handle utf16 files properly.\nThe \"p4 print\" command, when output to stdout, convert the\nutf16 file into utf8. That effectively imported the utf16 file\nas utf8 for git. In other words, git-p4 import a different\nfile compare to file check out by perforce. This breaks my\nwindows build in the company project.\n\nThe fix is simple, just ask perforce to print the depot\nfile into a real file. This way perforce will not perform\nthe utf16 to utf8 conversion. Git can import the exact same\nfile as perforce checkout.\n---\n contrib/fast-import/git-p4 | 8 ++++++++\n 1 files changed, 8 insertions(+), 0 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 6b9de9e..c111cad 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -1239,6 +1239,14 @@ class P4Sync(Command, P4UserMap):\n contents = map(lambda text: re.sub(r'(?i)\\$(Id|Header):[^$]*\\$',r'$\\1$', text), contents)\n elif file['type'] in ('text+k', 'ktext', 'kxtext', 'unicode+k', 'binary+k'):\n contents = map(lambda text: re.sub(r'\\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$\\n]*\\$',r'$\\1$', text), contents)\n+ elif file['type'] == 'utf16':\n+ tmpFile = tempfile.NamedTemporaryFile()\n+ cmd = \"print -o %s %s\"%(tmpFile.name, file['depotFile'])\n+ result = p4Cmd(cmd)\n+ if \"p4ExitCode\" in result:\n+ die(\"Problems executing p4 %s\"%cmd)\n+ contents = [ open(tmpFile.name).read() ]\n+ tmpFile.close()\n \n self.gitStream.write(\"M %s inline %s\\n\" % (mode, relPath))\n \n-- \n1.7.6\n\n"},{"id":"175492","messageId":"CANeU7QnPqJ+igcmS1JX_vasCXr+Wpcx2b4Z-sy_=0qKEkG+v_w@mail.gmail.com","threadId":"28383","inReplyTo":"CANeU7QnW5kSni0W9M9q-FTWv4p_qc67LG3mA6BQj_U-wxNuZeQ@mail.gmail.com","subject":"Re: [PATCH] git-p4: import utf16 file properly","fromName":"Chris Li","fromEmail":"git@chrisli.org","sentAt":"2011-09-14T18:39:18Z","receivedAt":"2011-09-14T18:39:18Z","isPatch":true,"sender":{"key":"git@chrisli.org","avatar":null},"body":"On Wed, Sep 14, 2011 at 11:29 AM, Chris Li wrote:\n>> Does this change do the right thing with RCS keywords in UTF16 files?\n>\n> I don't know what is the rules about the RCS keyword in UTF16 files.\n\nI did a little bit research and found this:\n\nhttp://www.perforce.com/perforce/doc.current/manuals/p4guide/ab_filetypes.html\n\nRCS keyword expand should only happen for \"+k\" or \"+ko\" modifiers.\nThere for, \"utf16\" files without modifier should not be converted.\nIn that regard, the patch is correct. But both the original and patched version\ndid not handle \"utf16+k\" type of files. I still consider it as a separate issue.\n\nChris\n"},{"id":"175496","messageId":"4E70F8DB.8080008@diamand.org","threadId":"28383","inReplyTo":"CANeU7QnW5kSni0W9M9q-FTWv4p_qc67LG3mA6BQj_U-wxNuZeQ@mail.gmail.com","subject":"Re: [PATCH] git-p4: import utf16 file properly","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2011-09-14T18:56:27Z","receivedAt":"2011-09-14T18:56:27Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 14/09/11 19:29, Chris Li wrote:\n> On Wed, Sep 14, 2011 at 12:55 AM, Luke Diamand wrote:\n>> On 13/09/11 22:33, Chris Li wrote:\n>>> The fix is simple, just ask perforce to print the depot\n>>> file into a real file. This way perforce will not performe\n>>> the utf16 to utf8 conversion. Git can import the exact same\n>>> file as perforce checkout.\n>>\n>> Does this change do the right thing with RCS keywords in UTF16 files?\n>\n> I don't know what is the rules about the RCS keyword in UTF16 files.\n> I look at the current git-p4, it does not do any keyword replacement in\n> utf16 files. So this patch did not change that. It should be a separate issue.\n>\n> The way I see it, this patch is a straight enhancement compare to the\n> current git-p4 because the current git-p4 *corrupts* the utf16 files.\n>\n>>\n>> If p4CmdList() fails, e.g. due to running out of diskspace, will this just\n>> happily import a truncated/corrupt file?\n>\n> Good point. I add the error check and attach the new patch.\n>\n>> (And I could be wrong about this, but does you patch have newline damage? It\n>> didn't seem to apply for me).\n\nLooks good to me. I think you're right about the RCS keywords not being \nrelevant here.\n\n\n\n>\n> Gmail dmage the white space. I should always use the attachment.\n> Does the attached patch work for you?\n>\n> Thanks\n>\n> Chris\n"},{"id":"175701","messageId":"20110918011942.GA13702@arf.padd.com","threadId":"28383","inReplyTo":"CANeU7QnPqJ+igcmS1JX_vasCXr+Wpcx2b4Z-sy_=0qKEkG+v_w@mail.gmail.com","subject":"Re: [PATCH] git-p4: import utf16 file properly","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2011-09-18T01:19:42Z","receivedAt":"2011-09-18T01:19:42Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"git@chrisli.org wrote on Wed, 14 Sep 2011 11:39 -0700:\n> On Wed, Sep 14, 2011 at 11:29 AM, Chris Li wrote:\n> >> Does this change do the right thing with RCS keywords in UTF16 files?\n> >\n> > I don't know what is the rules about the RCS keyword in UTF16 files.\n> \n> I did a little bit research and found this:\n> \n> http://www.perforce.com/perforce/doc.current/manuals/p4guide/ab_filetypes.html\n> \n> RCS keyword expand should only happen for \"+k\" or \"+ko\" modifiers.\n> There for, \"utf16\" files without modifier should not be converted.\n> In that regard, the patch is correct. But both the original and patched version\n> did not handle \"utf16+k\" type of files. I still consider it as a separate issue.\n\nYour patch looks good and this all makes sense. I redid\nit, adding a test case, and some more patches to clean up\nsome of the filetype detection code. I'll send it out for\nreview soon here.\n\nLuke: thanks for the comments; they prompted me to think\nabout keywords and beyond.\n\n\t\t-- Pete\n"}]}