{"thread":{"id":"58194","subject":"[PATCH] git-p4: fix crlf handling for utf16 files on Windows","startedAt":"2022-07-20T05:28:09Z","lastAt":"2022-07-20T18:42:43Z","messageCount":6,"participants":["Moritz Baumann via GitGitGadget","Junio C Hamano","Baumann, Moritz"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"459496","messageId":"pull.1294.git.git.1658294873702.gitgitgadget@gmail.com","threadId":"58194","inReplyTo":null,"subject":"[PATCH] git-p4: fix crlf handling for utf16 files on Windows","fromName":"Moritz Baumann via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-20T05:27:53Z","receivedAt":"2022-07-20T05:28:09Z","isPatch":true,"sender":{"key":"name:Moritz Baumann","avatar":null},"body":"From: Moritz Baumann <moritz.baumann@sap.com>\n\nSigned-off-by: Moritz Baumann <moritz.baumann@sap.com>\n---\n    git-p4: fix crlf handling for utf16 files on Windows\n    \n    Signed-off-by: Moritz Baumann moritz.baumann@sap.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1294%2Fmbs-c%2Ffix-crlf-conversion-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1294/mbs-c/fix-crlf-conversion-v1\nPull-Request: https://github.com/git/git/pull/1294\n\n git-p4.py | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8fbf6eb1fe3..0a9d7e2ed7c 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -3148,7 +3148,7 @@ class P4Sync(Command, P4UserMap):\n                     raise e\n             else:\n                 if p4_version_string().find('/NT') >= 0:\n-                    text = text.replace(b'\\r\\n', b'\\n')\n+                    text = text.replace(b'\\x0d\\x00\\x0a\\x00', b'\\x0a\\x00')\n                 contents = [text]\n \n         if type_base == \"apple\":\n\nbase-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca\n-- \ngitgitgadget\n"},{"id":"459510","messageId":"xmqqilnr4vhn.fsf@gitster.g","threadId":"58194","inReplyTo":"pull.1294.git.git.1658294873702.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-p4: fix crlf handling for utf16 files on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-20T16:08:52Z","receivedAt":"2022-07-20T16:08:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Moritz Baumann via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Moritz Baumann <moritz.baumann@sap.com>\n\nCan you describe briefly what problem is being solved and how the\nchange solves it in this place above your Sign-off?  The title says\n\"fix\", without saying how the behaviour by the current code is\n\"broken\", so that is one thing you can describe.  It talks about\n\"UTF-16 files on Windows\", but does it mean git-p4 running on\nWindows or git-p4 running anywhere that (over the wire) talks with\nP4 running on Windows?  IOW, would the same problem trigger if you\nare on macOS but the contents of the file you exchange with P4\nhappens to be in UTF-16?\n\nThese are the things you can describe to help those who are not you\n(i.e. without access to an environment similar to what you saw the\nproblem on) understand the issue and help them convince themselves\nthat the patch they are seeing is a sensible solution.  Without any,\nit is hard to evaluate.\n\n> Signed-off-by: Moritz Baumann <moritz.baumann@sap.com>\n> ---\n\n> diff --git a/git-p4.py b/git-p4.py\n> index 8fbf6eb1fe3..0a9d7e2ed7c 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -3148,7 +3148,7 @@ class P4Sync(Command, P4UserMap):\n>                      raise e\n>              else:\n>                  if p4_version_string().find('/NT') >= 0:\n> -                    text = text.replace(b'\\r\\n', b'\\n')\n> +                    text = text.replace(b'\\x0d\\x00\\x0a\\x00', b'\\x0a\\x00')\n>                  contents = [text]\n>  \n>          if type_base == \"apple\":\n\nOK, the part being touched is inside this context:\n\n        if type_base == \"utf16\":\n            # ...\n            # But ascii text saved as -t utf16 is completely mangled.\n            # Invoke print -o to get the real contents.\n            #\n            # On windows, the newlines will always be mangled by print, so put\n            # them back too.  This is not needed to the cygwin windows version,\n            # just the native \"NT\" type.\n            #\n\n            try:\n                text = ...\n            except Exception as e:\n                ...\n            else:\n                if p4_version_string().find('/NT') >= 0:\n                    text = text.replace(b'\\r\\n', b'\\n')\n                contents = [text]\n\nSo the intent of the existing code is \"we know we are dealing with\nUTF-16 text, and after successfully reading 'text' without\nexception, we need to convert CRLF back to LF if we are on 'the\nnative NT type'\".  Presumably 'text' that came from\np4_read_pipe(... raw=True) is not unicode string but just a bunch of\nbytes, so each \"char\" is represented as two-byte sequence in UTF-16?\n\nWith that (speculative) understanding, I can guess that the patch\nmakes sense, but the patch should not make readers guess.\n\nThanks.\n"},{"id":"459514","messageId":"AS1PR02MB8185D1041A96B0384554EE32948E9@AS1PR02MB8185.eurprd02.prod.outlook.com","threadId":"58194","inReplyTo":"xmqqilnr4vhn.fsf@gitster.g","subject":"RE: [PATCH] git-p4: fix crlf handling for utf16 files on Windows","fromName":"Baumann, Moritz","fromEmail":"moritz.baumann@sap.com","sentAt":"2022-07-20T16:32:51Z","receivedAt":"2022-07-20T16:33:00Z","isPatch":true,"sender":{"key":"moritz.baumann@sap.com","avatar":"https://avatars.githubusercontent.com/u/67415316?v=4"},"body":"Hi Junio,\n\nThank you for your notes. I assumed the intent of the original code would be clear, in which case the fix should also be clear, but I am happy to elaborate.\n\n> Can you describe briefly what problem is being solved and how the change\n> solves it in this place above your Sign-off?  […] It talks about\n> \"UTF-16 files on Windows\", but does it mean git-p4 running on Windows or\n> git-p4 running anywhere that (over the wire) talks with\n> P4 running on Windows?  IOW, would the same problem trigger if you are on\n> macOS but the contents of the file you exchange with P4 happens to be in\n> UTF-16?\n\nThe potential problem that the original code was trying to solve is the following: If a file is marked as utf16 in Perforce, and if the Perforce client is on Windows, then Perforce will replace all LF line endings with CRLF when the file is synced. This is different from git's autocrlf behavior, which ignores UTF-16 encoded files and always treats them as binary files. Without special handling, this can lead to git-p4 creating files with different hashes when run on Windows. (Which is how I stumbled upon this issue.)\n\nTherefore, git-p4 checks the Perforce \"file type\" and tries to undo the line endings changes.\n\n> So the intent of the existing code is \"we know we are dealing with\n> UTF-16 text, and after successfully reading 'text' without exception, we need\n> to convert CRLF back to LF if we are on 'the native NT type'\".  Presumably\n> 'text' that came from p4_read_pipe(... raw=True) is not unicode string but just\n> a bunch of bytes, so each \"char\" is represented as two-byte sequence in UTF-\n> 16?\n\nExactly. The original code tried to do the right thing to ensure stable hashes that are independent of the operating system git-p4 is run on, but failed to do so successfully. With my fix, I finally got deterministic hashes on my test repository.\n\n> With that (speculative) understanding, I can guess that the patch makes sense,\n> but the patch should not make readers guess.\n\nDo you need me to resubmit the patch with an explanatory description? If so, I can try to summarize the above.\n\nBest regards,\nMoritz\n"},{"id":"459521","messageId":"xmqqh73b3doo.fsf@gitster.g","threadId":"58194","inReplyTo":"AS1PR02MB8185D1041A96B0384554EE32948E9@AS1PR02MB8185.eurprd02.prod.outlook.com","subject":"Re: [PATCH] git-p4: fix crlf handling for utf16 files on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-20T17:18:47Z","receivedAt":"2022-07-20T17:18:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Baumann, Moritz\" <moritz.baumann@sap.com> writes:\n\n> Do you need me to resubmit the patch with an explanatory\n> description? If so, I can try to summarize the above.\n\nYup.\n\nReview comments are not a request to the authors to explain their\npatches to reviewers.  Their primary purpose is to point out what\npotential issues readers of the commit that would result by the\nproposed patches may have.  So answering in your response to see if\nyour clarifications are understandable is very good, but please\nconsider it a preparation to write a better version (i.e. [PATCH\nv2]).\n\nThanks for working on this fix.\n\n"},{"id":"459523","messageId":"pull.1294.v2.git.git.1658341065221.gitgitgadget@gmail.com","threadId":"58194","inReplyTo":"pull.1294.git.git.1658294873702.gitgitgadget@gmail.com","subject":"[PATCH v2] git-p4: fix CR LF handling for utf16 files","fromName":"Moritz Baumann via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-20T18:17:45Z","receivedAt":"2022-07-20T18:17:52Z","isPatch":true,"sender":{"key":"name:Moritz Baumann","avatar":null},"body":"From: Moritz Baumann <moritz.baumann@sap.com>\n\nPerforce silently replaces LF with CR LF for \"utf16\" files if the client\nis a native Windows client. Since git's autocrlf logic does not undo\nthis transformation for UTF-16 encoded files, git-p4 replaces CR LF with\nLF during the sync if the file type \"utf16\" is detected and the Perforce\nclient platform indicates that this conversion is performed.\n\nWindows only runs on little-endian architectures, therefore the encoding\nof the byte stream received from the Perforce client is UTF-16-LE and\nthe relevant byte sequence is 0D 00 0A 00.\n\nSigned-off-by: Moritz Baumann <moritz.baumann@sap.com>\n---\n    git-p4: fix crlf handling for utf16 files on Windows\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1294%2Fmbs-c%2Ffix-crlf-conversion-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1294/mbs-c/fix-crlf-conversion-v2\nPull-Request: https://github.com/git/git/pull/1294\n\nRange-diff vs v1:\n\n 1:  4a7a14eec28 ! 1:  4d0043712d3 git-p4: fix crlf handling for utf16 files on Windows\n     @@ Metadata\n      Author: Moritz Baumann <moritz.baumann@sap.com>\n      \n       ## Commit message ##\n     -    git-p4: fix crlf handling for utf16 files on Windows\n     +    git-p4: fix CR LF handling for utf16 files\n     +\n     +    Perforce silently replaces LF with CR LF for \"utf16\" files if the client\n     +    is a native Windows client. Since git's autocrlf logic does not undo\n     +    this transformation for UTF-16 encoded files, git-p4 replaces CR LF with\n     +    LF during the sync if the file type \"utf16\" is detected and the Perforce\n     +    client platform indicates that this conversion is performed.\n     +\n     +    Windows only runs on little-endian architectures, therefore the encoding\n     +    of the byte stream received from the Perforce client is UTF-16-LE and\n     +    the relevant byte sequence is 0D 00 0A 00.\n      \n          Signed-off-by: Moritz Baumann <moritz.baumann@sap.com>\n      \n\n\n git-p4.py | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8fbf6eb1fe3..0a9d7e2ed7c 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -3148,7 +3148,7 @@ class P4Sync(Command, P4UserMap):\n                     raise e\n             else:\n                 if p4_version_string().find('/NT') >= 0:\n-                    text = text.replace(b'\\r\\n', b'\\n')\n+                    text = text.replace(b'\\x0d\\x00\\x0a\\x00', b'\\x0a\\x00')\n                 contents = [text]\n \n         if type_base == \"apple\":\n\nbase-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca\n-- \ngitgitgadget\n"},{"id":"459531","messageId":"xmqq7d4739sy.fsf@gitster.g","threadId":"58194","inReplyTo":"pull.1294.v2.git.git.1658341065221.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] git-p4: fix CR LF handling for utf16 files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-20T18:42:37Z","receivedAt":"2022-07-20T18:42:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Moritz Baumann via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Moritz Baumann <moritz.baumann@sap.com>\n>\n> Perforce silently replaces LF with CR LF for \"utf16\" files if the client\n> is a native Windows client. Since git's autocrlf logic does not undo\n> this transformation for UTF-16 encoded files, git-p4 replaces CR LF with\n> LF during the sync if the file type \"utf16\" is detected and the Perforce\n> client platform indicates that this conversion is performed.\n>\n> Windows only runs on little-endian architectures, therefore the encoding\n> of the byte stream received from the Perforce client is UTF-16-LE and\n> the relevant byte sequence is 0D 00 0A 00.\n>\n> Signed-off-by: Moritz Baumann <moritz.baumann@sap.com>\n> ---\n\nWill queue.  Thanks.\n"}]}