{"thread":{"id":"55479","subject":"[PATCH 0/2] git-p4: encoding of data from perforce","startedAt":"2021-04-12T09:18:23Z","lastAt":"2021-05-05T04:34:38Z","messageCount":13,"participants":["Andrew Oakley","Tzadik Vanderhoof","Luke Diamand","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"421640","messageId":"20210412085251.51475-1-andrew@adoakley.name","threadId":"55479","inReplyTo":null,"subject":"[PATCH 0/2] git-p4: encoding of data from perforce","fromName":"Andrew Oakley","fromEmail":"andrew@adoakley.name","sentAt":"2021-04-12T08:52:49Z","receivedAt":"2021-04-12T09:18:23Z","isPatch":true,"sender":{"key":"andrew@adoakley.name","avatar":"https://avatars.githubusercontent.com/u/1107440?v=4"},"body":"When using python3, git-p4 fails to handle data from perforce which is\nnot valid UTF-8.  In large repositories it's very likely that such data\nwill exist - perforce itself does no validation of the data by default.\n\nHistorically git-p4 has just passed whatever bytes it got from perforce\ninto git.  This seems like a sensible approach - git-p4 has no idea what\nencoding may have been used and it seems likely that different encodings\nare used within a repository.\n\nI was trying to do a more thorough job, moving more of git-p4 over to\nusing bytes.  Unfortunately the changes end up being large and hard to\nreview.  In most cases it's probably sufficient to just avoid decoding\nthe commit messages.\n\nThere have been a couple of previous proposals around trying to decode\nthis data using a user-configured encoding:\nhttp://public-inbox.org/git/CAE5ih7-F9efsiV5AQmw3ocjiy+BT6ZAT5fA0Lx0OSkVTO8Kqjg@mail.gmail.com/T/\nhttp://public-inbox.org/git/20210409153815.7joohvmlnh6itczc@tb-raspi4/T/\n\n\n"},{"id":"421641","messageId":"20210412085251.51475-3-andrew@adoakley.name","threadId":"55479","inReplyTo":"20210412085251.51475-1-andrew@adoakley.name","subject":"[PATCH 2/2] git-p4: do not decode data from perforce by default","fromName":"Andrew Oakley","fromEmail":"andrew@adoakley.name","sentAt":"2021-04-12T08:52:51Z","receivedAt":"2021-04-12T09:18:28Z","isPatch":true,"sender":{"key":"andrew@adoakley.name","avatar":"https://avatars.githubusercontent.com/u/1107440?v=4"},"body":"This commit is not intended to change behaviour, any we still attempt to\ndecode values that might not be valid unicode.  It's not clear that all\nof these values are safe to decode, but it's now more obvious which data\nis decoded.\n---\n git-p4.py | 16 ++++++++++------\n 1 file changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8407ec5c7a..8a97ff3dd2 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -764,15 +764,19 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n         while True:\n             entry = marshal.load(p4.stdout)\n             if bytes is not str:\n-                # Decode unmarshalled dict to use str keys and values, except\n-                # for cases where the values may not be valid UTF-8.\n-                binary_keys = ('data', 'path', 'clientFile', 'Description',\n-                               'desc', 'Email', 'FullName', 'Owner', 'time',\n-                               'user', 'User')\n+                # Decode unmarshalled dict to use str keys and values where it\n+                # is expected that the data is always valid UTF-8.\n+                text_keys = ('action', 'change', 'Change', 'Client', 'code',\n+                             'fileSize', 'headAction', 'headRev', 'headType',\n+                             'Jobs', 'label', 'options', 'perm', 'rev', 'Root',\n+                             'Status', 'type', 'Update')\n+                text_key_prefixes = ('action', 'File', 'job', 'rev', 'type',\n+                                     'View')\n                 decoded_entry = {}\n                 for key, value in entry.items():\n                     key = key.decode()\n-                    if isinstance(value, bytes) and not (key in binary_keys or key.startswith('depotFile')):\n+                    if isinstance(value, bytes) and (key in text_keys or\n+                            any(filter(key.startswith, text_key_prefixes))):\n                         value = value.decode()\n                     decoded_entry[key] = value\n                 # Parse out data if it's an error response\n-- \n2.26.3\n\n"},{"id":"421642","messageId":"20210412085251.51475-2-andrew@adoakley.name","threadId":"55479","inReplyTo":"20210412085251.51475-1-andrew@adoakley.name","subject":"[PATCH 1/2] git-p4: avoid decoding more data from perforce","fromName":"Andrew Oakley","fromEmail":"andrew@adoakley.name","sentAt":"2021-04-12T08:52:50Z","receivedAt":"2021-04-12T09:18:28Z","isPatch":true,"sender":{"key":"andrew@adoakley.name","avatar":"https://avatars.githubusercontent.com/u/1107440?v=4"},"body":"Perforce does not validate or store the encoding of user submitted data\nby default (although this can be enabled).  In large repositories it is\ntherefore very likely that some data will not be valid UTF-8.\n\nHistorically (with python2) git-p4 did not attempt to decode the data\nfrom the perforce server - it just passed bytes from perforce to git,\npreserving whatever was stored in perforce.  This seems like a sensible\napproach - it avoids any loss of data, and there is no way to determine\nthe intended encoding for any invalid data from perforce.\n\nThis change updates git-p4 to avoid decoding changelist descriptions,\nuser and time information.  The time data is almost certainly valid\nunicode, but as they are processed with the user information it is more\nconvenient for them to be handled as bytes.\n\nSigned-off-by: Andrew Oakley <andrew@adoakley.name>\n---\n git-p4.py                          | 57 +++++++++++++++---------------\n t/t9835-git-p4-message-encoding.sh | 48 +++++++++++++++++++++++++\n 2 files changed, 77 insertions(+), 28 deletions(-)\n create mode 100755 t/t9835-git-p4-message-encoding.sh\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 09c9e93ac4..8407ec5c7a 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -764,13 +764,15 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n         while True:\n             entry = marshal.load(p4.stdout)\n             if bytes is not str:\n-                # Decode unmarshalled dict to use str keys and values, except for:\n-                #   - `data` which may contain arbitrary binary data\n-                #   - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text\n+                # Decode unmarshalled dict to use str keys and values, except\n+                # for cases where the values may not be valid UTF-8.\n+                binary_keys = ('data', 'path', 'clientFile', 'Description',\n+                               'desc', 'Email', 'FullName', 'Owner', 'time',\n+                               'user', 'User')\n                 decoded_entry = {}\n                 for key, value in entry.items():\n                     key = key.decode()\n-                    if isinstance(value, bytes) and not (key in ('data', 'path', 'clientFile') or key.startswith('depotFile')):\n+                    if isinstance(value, bytes) and not (key in binary_keys or key.startswith('depotFile')):\n                         value = value.decode()\n                     decoded_entry[key] = value\n                 # Parse out data if it's an error response\n@@ -949,11 +951,11 @@ def gitConfigInt(key):\n             _gitConfig[key] = None\n     return _gitConfig[key]\n \n-def gitConfigList(key):\n+def gitConfigList(key, raw=False):\n     if key not in _gitConfig:\n-        s = read_pipe([\"git\", \"config\", \"--get-all\", key], ignore_error=True)\n+        s = read_pipe([\"git\", \"config\", \"--get-all\", key], ignore_error=True, raw=raw)\n         _gitConfig[key] = s.strip().splitlines()\n-        if _gitConfig[key] == ['']:\n+        if _gitConfig[key] == [''] or _gitConfig[key] == [b'']:\n             _gitConfig[key] = []\n     return _gitConfig[key]\n \n@@ -1499,35 +1501,35 @@ def getUserMapFromPerforceServer(self):\n         for output in p4CmdList(\"users\"):\n             if \"User\" not in output:\n                 continue\n-            self.users[output[\"User\"]] = output[\"FullName\"] + \" <\" + output[\"Email\"] + \">\"\n+            self.users[output[\"User\"]] = output[\"FullName\"] + b\" <\" + output[\"Email\"] + b\">\"\n             self.emails[output[\"Email\"]] = output[\"User\"]\n \n-        mapUserConfigRegex = re.compile(r\"^\\s*(\\S+)\\s*=\\s*(.+)\\s*<(\\S+)>\\s*$\", re.VERBOSE)\n-        for mapUserConfig in gitConfigList(\"git-p4.mapUser\"):\n+        mapUserConfigRegex = re.compile(br\"^\\s*(\\S+)\\s*=\\s*(.+)\\s*<(\\S+)>\\s*$\", re.VERBOSE)\n+        for mapUserConfig in gitConfigList(\"git-p4.mapUser\", raw=True):\n             mapUser = mapUserConfigRegex.findall(mapUserConfig)\n             if mapUser and len(mapUser[0]) == 3:\n                 user = mapUser[0][0]\n                 fullname = mapUser[0][1]\n                 email = mapUser[0][2]\n-                self.users[user] = fullname + \" <\" + email + \">\"\n+                self.users[user] = fullname + b\" <\" + email + b\">\"\n                 self.emails[email] = user\n \n-        s = ''\n+        s = b''\n         for (key, val) in self.users.items():\n-            s += \"%s\\t%s\\n\" % (key.expandtabs(1), val.expandtabs(1))\n+            s += b\"%s\\t%s\\n\" % (key.expandtabs(1), val.expandtabs(1))\n \n-        open(self.getUserCacheFilename(), 'w').write(s)\n+        open(self.getUserCacheFilename(), 'wb').write(s)\n         self.userMapFromPerforceServer = True\n \n     def loadUserMapFromCache(self):\n         self.users = {}\n         self.userMapFromPerforceServer = False\n         try:\n-            cache = open(self.getUserCacheFilename(), 'r')\n+            cache = open(self.getUserCacheFilename(), 'rb')\n             lines = cache.readlines()\n             cache.close()\n             for line in lines:\n-                entry = line.strip().split(\"\\t\")\n+                entry = line.strip().split(b\"\\t\")\n                 self.users[entry[0]] = entry[1]\n         except IOError:\n             self.getUserMapFromPerforceServer()\n@@ -1780,7 +1782,7 @@ def p4UserForCommit(self,id):\n         # Return the tuple (perforce user,git email) for a given git commit id\n         self.getUserMapFromPerforceServer()\n         gitEmail = read_pipe([\"git\", \"log\", \"--max-count=1\",\n-                              \"--format=%ae\", id])\n+                              \"--format=%ae\", id], raw=True)\n         gitEmail = gitEmail.strip()\n         if gitEmail not in self.emails:\n             return (None,gitEmail)\n@@ -1911,7 +1913,7 @@ def prepareSubmitTemplate(self, changelist=None):\n             template += key + ':'\n             if key == 'Description':\n                 template += '\\n'\n-            for field_line in change_entry[key].splitlines():\n+            for field_line in decode_text_stream(change_entry[key]).splitlines():\n                 template += '\\t'+field_line+'\\n'\n         if len(files_list) > 0:\n             template += '\\n'\n@@ -2163,7 +2165,7 @@ def applyCommit(self, id):\n            submitTemplate += \"\\n######## Actual user %s, modified after commit\\n\" % p4User\n \n         if self.checkAuthorship and not self.p4UserIsMe(p4User):\n-            submitTemplate += \"######## git author %s does not match your p4 account.\\n\" % gitEmail\n+            submitTemplate += \"######## git author %s does not match your p4 account.\\n\" % decode_text_stream(gitEmail)\n             submitTemplate += \"######## Use option --preserve-user to modify authorship.\\n\"\n             submitTemplate += \"######## Variable git-p4.skipUserNameCheck hides this message.\\n\"\n \n@@ -2802,7 +2804,7 @@ def __init__(self):\n         self.knownBranches = {}\n         self.initialParents = {}\n \n-        self.tz = \"%+03d%02d\" % (- time.timezone / 3600, ((- time.timezone % 3600) / 60))\n+        self.tz = b\"%+03d%02d\" % (- time.timezone / 3600, ((- time.timezone % 3600) / 60))\n         self.labels = {}\n \n     # Force a checkpoint in fast-import and wait for it to finish\n@@ -3161,7 +3163,7 @@ def make_email(self, userid):\n         if userid in self.users:\n             return self.users[userid]\n         else:\n-            return \"%s <a@b>\" % userid\n+            return b\"%s <a@b>\" % userid\n \n     def streamTag(self, gitStream, labelName, labelDetails, commit, epoch):\n         \"\"\" Stream a p4 tag.\n@@ -3184,9 +3186,9 @@ def streamTag(self, gitStream, labelName, labelDetails, commit, epoch):\n             email = self.make_email(owner)\n         else:\n             email = self.make_email(self.p4UserId())\n-        tagger = \"%s %s %s\" % (email, epoch, self.tz)\n+        tagger = b\"%s %s %s\" % (email, epoch, self.tz)\n \n-        gitStream.write(\"tagger %s\\n\" % tagger)\n+        gitStream.write(b\"tagger %s\\n\" % tagger)\n \n         print(\"labelDetails=\",labelDetails)\n         if 'Description' in labelDetails:\n@@ -3279,12 +3281,11 @@ def commit(self, details, files, branch, parent = \"\", allow_empty=False):\n         self.gitStream.write(\"commit %s\\n\" % branch)\n         self.gitStream.write(\"mark :%s\\n\" % details[\"change\"])\n         self.committedChanges.add(int(details[\"change\"]))\n-        committer = \"\"\n         if author not in self.users:\n             self.getUserMapFromPerforceServer()\n-        committer = \"%s %s %s\" % (self.make_email(author), epoch, self.tz)\n+        committer = b\"%s %s %s\" % (self.make_email(author), epoch, self.tz)\n \n-        self.gitStream.write(\"committer %s\\n\" % committer)\n+        self.gitStream.write(b\"committer %s\\n\" % committer)\n \n         self.gitStream.write(\"data <<EOT\\n\")\n         self.gitStream.write(details[\"desc\"])\n@@ -3422,7 +3423,7 @@ def importP4Labels(self, stream, p4Labels):\n                         print(\"Could not convert label time %s\" % labelDetails['Update'])\n                         tmwhen = 1\n \n-                    when = int(time.mktime(tmwhen))\n+                    when = b\"%i\" % int(time.mktime(tmwhen))\n                     self.streamTag(stream, name, labelDetails, gitCommit, when)\n                     if verbose:\n                         print(\"p4 label %s mapped to git commit %s\" % (name, gitCommit))\n@@ -3708,7 +3709,7 @@ def importHeadRevision(self, revision):\n         print(\"Doing initial import of %s from revision %s into %s\" % (' '.join(self.depotPaths), revision, self.branch))\n \n         details = {}\n-        details[\"user\"] = \"git perforce import user\"\n+        details[\"user\"] = b\"git perforce import user\"\n         details[\"desc\"] = (\"Initial import of %s from the state at revision %s\\n\"\n                            % (' '.join(self.depotPaths), revision))\n         details[\"change\"] = revision\ndiff --git a/t/t9835-git-p4-message-encoding.sh b/t/t9835-git-p4-message-encoding.sh\nnew file mode 100755\nindex 0000000000..93f24fe295\n--- /dev/null\n+++ b/t/t9835-git-p4-message-encoding.sh\n@@ -0,0 +1,48 @@\n+#!/bin/sh\n+\n+test_description='Clone repositories with non ASCII commit messages'\n+\n+. ./lib-git-p4.sh\n+\n+UTF8=\"$(printf \"a-\\303\\244_o-\\303\\266_u-\\303\\274\")\"\n+ISO8859=\"$(printf \"a-\\344_o-\\366_u-\\374\")\"\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'create commits in perforce' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\tp4_add_user \"${UTF8}\" &&\n+\t\tp4_add_user \"${ISO8859}\" &&\n+\n+\t\t>dummy-file1 &&\n+\t\tP4USER=\"${UTF8}\" p4 add dummy-file1 &&\n+\t\tP4USER=\"${UTF8}\" p4 submit -d \"message ${UTF8}\" &&\n+\n+\t\t>dummy-file2 &&\n+\t\tP4USER=\"${ISO8859}\" p4 add dummy-file2 &&\n+\t\tP4USER=\"${ISO8859}\" p4 submit -d \"message ${ISO8859}\"\n+\t)\n+'\n+\n+test_expect_success 'check UTF-8 commit' '\n+\t(\n+\t\tgit p4 clone --destination=\"$git/1\" //depot@1,1 &&\n+\t\tgit -C \"$git/1\" cat-file commit HEAD | grep -q \"^message ${UTF8}$\" &&\n+\t\tgit -C \"$git/1\" cat-file commit HEAD | grep -q \"^author Dr. ${UTF8} <${UTF8}@example.com>\"\n+\t)\n+'\n+\n+test_expect_success 'check ISO-8859 commit' '\n+\t(\n+\t\tgit p4 clone --destination=\"$git/2\" //depot@2,2 &&\n+\t\tgit -C \"$git/2\" cat-file commit HEAD > /tmp/dump.txt &&\n+\t\tgit -C \"$git/2\" cat-file commit HEAD | grep -q \"^message ${ISO8859}$\" &&\n+\t\tgit -C \"$git/2\" cat-file commit HEAD | grep -q \"^author Dr. ${ISO8859} <${ISO8859}@example.com>\"\n+\t)\n+'\n+\n+test_done\n-- \n2.26.3\n\n"},{"id":"423258","messageId":"CAKu1iLXRrsB4mRsDfhBH5aahWzDjpfqLuWP9t47RMB=RdpL1iA@mail.gmail.com","threadId":"55479","inReplyTo":"20210412085251.51475-3-andrew@adoakley.name","subject":"Re: [PATCH 2/2] git-p4: do not decode data from perforce by default","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-29T10:00:06Z","receivedAt":"2021-04-29T10:00:21Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"I checked out \"seen\" and ran the test script from this patch\n(t9835-git-p4-message-encoding.sh) on my Windows machine and it fails.\n\nI don't think the solution in this patch will solve the issue of non\nUTF-8 descriptions on Windows. The interaction between git-p4.py and\np4 around non-ASCII descriptions is different on Linux and Windows (at\nleast with the default code page settings).  Unfortunately the CI on\ngitlab does not include any Windows test environments that have p4\ninstalled.\n\nAs far as I can tell, non-ASCII strings passed to \"p4 submit -d\" pass\nunchanged to the Perforce database on Linux.  As well, such data also\npasses unchanged in the other direction, when \"p4\" output is consumed\nby git-p4.py.  Since this patch avoids decoding descriptions, and the\ntest script uses binary data for descriptions, the tests pass on\nLinux.\n\nHowever, on Windows, UTF-8 strings passed to \"p4 submit -d\" are\nsomehow converted to the default Windows code page by the time they\nare stored in the Perforce database, probably as part of the process\nof passing the command line arguments to the Windows p4 executable.\nHowever, the \"code page\" data is *not* converted to UTF-8 on the way\nback from p4 to git-p4.py.  The only way to get it into UTF-8 is to\ncall string.decode().  As a result, this patch, which takes out the\ncall to string.decode() will not work on Windows.\n"},{"id":"423324","messageId":"20210430095342.58134e4e@ado-tr","threadId":"55479","inReplyTo":"CAKu1iLXRrsB4mRsDfhBH5aahWzDjpfqLuWP9t47RMB=RdpL1iA@mail.gmail.com","subject":"Re: [PATCH 2/2] git-p4: do not decode data from perforce by default","fromName":"Andrew Oakley","fromEmail":"andrew@adoakley.name","sentAt":"2021-04-30T08:53:42Z","receivedAt":"2021-04-30T08:53:52Z","isPatch":true,"sender":{"key":"andrew@adoakley.name","avatar":"https://avatars.githubusercontent.com/u/1107440?v=4"},"body":"On Thu, 29 Apr 2021 03:00:06 -0700\nTzadik Vanderhoof <tzadik.vanderhoof@gmail.com> wrote:\n> However, on Windows, UTF-8 strings passed to \"p4 submit -d\" are\n> somehow converted to the default Windows code page by the time they\n> are stored in the Perforce database, probably as part of the process\n> of passing the command line arguments to the Windows p4 executable.\n> However, the \"code page\" data is *not* converted to UTF-8 on the way\n> back from p4 to git-p4.py.  The only way to get it into UTF-8 is to\n> call string.decode().  As a result, this patch, which takes out the\n> call to string.decode() will not work on Windows.\n\nThanks for that explanation, the reencoding of the data on Windows is\nnot something I was expecting.  Given the behaviour you've described, I\nsuspect that there might be two different problems that we are trying\nto solve.\n\nThe perforce depot I'm working with has a mixture of encodings, and\ncommits are created from a variety of different environments. The\nmajority of commits are ASCII or UTF-8, there are a small number that\nare in some other encoding.  Any attempt to reencode the data is likely\nto make the problem worse in at least some cases.\n\nI suspect that other perforce depots are used primarily from Windows\nmachines, and have data that is encoded in a mostly consistent way but\nthe encoding is not UTF-8.  Re-encoding the data for git makes sense in\nthat case.  Is this the kind of repository you have?\n\nIf there are these two different cases then we probably need to come up\nwith a patch that solves both issues.\n\nFor my cases where we've got a repository containing all sorts of junk,\nit sounds like it might be awkward to create a test case that works on\nWindows.\n"},{"id":"423348","messageId":"021c0caf-8e6f-4fbb-6ff7-40bacbe5de38@diamand.org","threadId":"55479","inReplyTo":"20210430095342.58134e4e@ado-tr","subject":"Re: [PATCH 2/2] git-p4: do not decode data from perforce by default","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2021-04-30T15:33:11Z","receivedAt":"2021-04-30T15:33:10Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"\n\nOn 30/04/2021 08:53, Andrew Oakley wrote:\n> On Thu, 29 Apr 2021 03:00:06 -0700\n> Tzadik Vanderhoof <tzadik.vanderhoof@gmail.com> wrote:\n>> However, on Windows, UTF-8 strings passed to \"p4 submit -d\" are\n>> somehow converted to the default Windows code page by the time they\n>> are stored in the Perforce database, probably as part of the process\n>> of passing the command line arguments to the Windows p4 executable.\n>> However, the \"code page\" data is *not* converted to UTF-8 on the way\n>> back from p4 to git-p4.py.  The only way to get it into UTF-8 is to\n>> call string.decode().  As a result, this patch, which takes out the\n>> call to string.decode() will not work on Windows.\n> \n> Thanks for that explanation, the reencoding of the data on Windows is\n> not something I was expecting.  Given the behaviour you've described, I\n> suspect that there might be two different problems that we are trying\n> to solve.\n> \n> The perforce depot I'm working with has a mixture of encodings, and\n> commits are created from a variety of different environments. The\n> majority of commits are ASCII or UTF-8, there are a small number that\n> are in some other encoding.  Any attempt to reencode the data is likely\n> to make the problem worse in at least some cases.\n> \n> I suspect that other perforce depots are used primarily from Windows\n> machines, and have data that is encoded in a mostly consistent way but\n> the encoding is not UTF-8.  Re-encoding the data for git makes sense in\n> that case.  Is this the kind of repository you have?\n> \n> If there are these two different cases then we probably need to come up\n> with a patch that solves both issues.\n> \n> For my cases where we've got a repository containing all sorts of junk,\n> it sounds like it might be awkward to create a test case that works on\n> Windows.\n> \n\n\nhttps://www.perforce.com/perforce/doc.current/user/i18nnotes.txt\n\nTzadik - is your server unicode enabled or not? That would be \ninteresting to know:\n\n     p4 counters | grep -i unicode\n\nI suspect it is not. It's only if unicode is enabled that the server \nwill convert to/from utf8 (at least that's my understanding). Without \nthis setting, p4d and p4 are (probably) not doing any conversions.\n\nI think it might be useful to clarify exactly what conversions are \nactually happening.\n\nI wonder what encoding Perforce thinks you've got in place.\n\n\n\n\n\n"},{"id":"423364","messageId":"CAKu1iLWbmPrVjAcgLKP1yisjmVxJr+kKQWJLiqkRzh=aAzETwA@mail.gmail.com","threadId":"55479","inReplyTo":"021c0caf-8e6f-4fbb-6ff7-40bacbe5de38@diamand.org","subject":"Re: [PATCH 2/2] git-p4: do not decode data from perforce by default","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-30T18:08:57Z","receivedAt":"2021-04-30T18:09:15Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"On Fri, Apr 30, 2021 at 8:33 AM Luke Diamand <luke@diamand.org> wrote:\n>\n> Tzadik - is your server unicode enabled or not? That would be\n> interesting to know:\n>\n>      p4 counters | grep -i unicode\n>\n> I suspect it is not. It's only if unicode is enabled that the server\n> will convert to/from utf8 (at least that's my understanding). Without\n> this setting, p4d and p4 are (probably) not doing any conversions.\n\nMy server is not unicode.\n\nThese conversions are happening even with a non-Unicode perforce db.\nI don't think it's the p4d code per se that is doing the conversion, but\nrather an interaction between the OS and the code, which is different\nunder Linux vs Windows.  If you create a trivial C program that dumps\nthe hex values of the bytes it receives in argv, you can see this\ndifferent behavior:\n\n#include <stdio.h>\n\nvoid main(int argc, char *argv[]) {\n    int i, j;\n    char *s;\n    for (i = 1; i < argc; ++i) {\n        s = argv[i];\n        for (j = 0; s[j] != '\\0'; ++j)\n            printf(\" %X\", (unsigned char)s[j]);\n        printf(\"\\n\");\n        printf(\"[%s]\\n\\n\", s);\n    }\n}\n\nWhen built with Visual Studio and called from Cygwin, if you pass in\nargs with UTF-8 encoded characters, the program will spit them out in\ncp1252. If you compile it on a Linux system using gcc, it will spit them out\nin UTF-8 (unchanged).  I suspect that's what's happening with p4d on\nWindows vs Linux.\n\nIn any event, if you look at my patch (v6 is the latest...\nhttps://lore.kernel.org/git/20210429073905.837-1-tzadik.vanderhoof@gmail.com/ ),\nyou will see I have written tests that pass under both Linux and Windows.\n(If you want to run them yourself, you need to base my patch off of \"master\",\nnot \"seen\").  The tests make clear what the different behavior is and\nalso show that p4d is not set to Unicode (since the tests do not change the\ndefault setting).\n"},{"id":"423611","messageId":"20210504220153.1d9f0cb2@ado-tr","threadId":"55479","inReplyTo":"CAKu1iLWbmPrVjAcgLKP1yisjmVxJr+kKQWJLiqkRzh=aAzETwA@mail.gmail.com","subject":"Re: [PATCH 2/2] git-p4: do not decode data from perforce by default","fromName":"Andrew Oakley","fromEmail":"andrew@adoakley.name","sentAt":"2021-05-04T21:01:53Z","receivedAt":"2021-05-04T21:02:03Z","isPatch":true,"sender":{"key":"andrew@adoakley.name","avatar":"https://avatars.githubusercontent.com/u/1107440?v=4"},"body":"On Fri, 30 Apr 2021 11:08:57 -0700\nTzadik Vanderhoof <tzadik.vanderhoof@gmail.com> wrote:\n> My server is not unicode.\n> \n> These conversions are happening even with a non-Unicode perforce db.\n> I don't think it's the p4d code per se that is doing the conversion,\n> but rather an interaction between the OS and the code, which is\n> different under Linux vs Windows.\n\nIt's not particularly obvious exactly what is happening here.  The\nperforce command line client is written in a rather odd way - it uses\nthe unicode (UTF-16) wWinMainCRTStartup entry point but then calls an\nundocumented API to get the \"narrow\" version of the command line.  The\ncode is here:\n\nhttps://swarm.workshop.perforce.com/projects/perforce_software-p4/files/2016-1/client/clientmain.cc\n\nI think that perforce will end up with the data in a code page that\ndepends on the configuration of the machine.  I don't think the exact\ndetails matter here - just that it's some semi-arbitrary encoding that\nisn't recorded in the commit.\n\nThe key thing that I'm trying to point out here is that the encoding is\nnot necessarily consistent between different commits.  The changes that\nyou have proposed force you to pick one encoding that will be used for\nevery commit.  If it's wrong then data will be corrupted, and there is\nno option provided to avoid that.  The only way I can see to avoid this\nissue is to not attempt to re-encode the data - just pass it directly\nto git.\n\nI think another way to solve the issue you have is the encoding header\non git commits.  We can pass the bytes through git-p4 unmodified, but\nmark the commit message as being encoded using something that isn't\nUTF-8.  That avoids any potentially lossy conversions when cloning the\nrepository, but should allow the data to be displayed correctly in git.\n\n> In any event, if you look at my patch (v6 is the latest...\n> https://lore.kernel.org/git/20210429073905.837-1-tzadik.vanderhoof@gmail.com/\n> ), you will see I have written tests that pass under both Linux and\n> Windows. (If you want to run them yourself, you need to base my patch\n> off of \"master\", not \"seen\").  The tests make clear what the\n> different behavior is and also show that p4d is not set to Unicode\n> (since the tests do not change the default setting).\n\nI don't think the tests are doing anything interesting on Linux - you\nstick valid UTF-8 in, and valid UTF-8 data comes out.   I suspect the\ntests will fail on Windows if the relevant code page is set to a value\nthat you're not expecting.\n\nFor the purposes of writing tests that work the same everywhere we can\nuse `p4 submit -i`.  The data written on stdin isn't reencoded, even on\nWindows.\n\nI can rework my test to use `p4 submit -i` on windows.  It should be\nfairly simple to write another change which allows the encoding to be\nset on commits created by git-p4.\n\nDoes that seem like a reasonable way forward?  I think it gets us:\n- sensible handling for repositories with mixed encodings\n- sensible handling for repositories with known encodings\n- tests that work the same on Linux and Windows\n"},{"id":"423619","messageId":"CAKu1iLXOFiUGmQUeoW-YkiiJ8P2+LznXWz4YabEiGktv=nUYjA@mail.gmail.com","threadId":"55479","inReplyTo":"20210504220153.1d9f0cb2@ado-tr","subject":"Re: [PATCH 2/2] git-p4: do not decode data from perforce by default","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-05-04T21:46:40Z","receivedAt":"2021-05-04T21:46:55Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"On Tue, May 4, 2021 at 2:01 PM Andrew Oakley <andrew@adoakley.name> wrote:\n> The key thing that I'm trying to point out here is that the encoding is\n> not necessarily consistent between different commits.  The changes that\n> you have proposed force you to pick one encoding that will be used for\n> every commit.  If it's wrong then data will be corrupted, and there is\n> no option provided to avoid that.  The only way I can see to avoid this\n> issue is to not attempt to re-encode the data - just pass it directly\n> to git.\n\nNo, my \"fallbackEndocing\" setting is just that... a fallback.  My proposal\n*always* tries to decode in UTF-8 first!  Only if that throws an exception\nwill my \"fallbackEncoding\" come into play, and it only comes into play\nfor the single changeset description that was invalid UTF-8.  After that,\nsubsequent descriptions will again be tried in UTF-8 first.\n\nThe design of the UTF-8 format makes it very unlikely that non UTF-8 text\nwill pass undetected through a UTF-8 decoder, so by attempting to decode\nin UTF-8 first, there is very little risk of a lossy conversion.\n\nAs for passing data through \"raw\", that will *guarantee* bad encoding on\nany descriptions that are not UTF-8, because git will interpret the data\nas UTF-8 once it has been put into the commit (unless the encoding header\nis used, as you mentioned) .  If that header is not used, and it was not in\nUTF-8 in Perforce, it has zero chance of being correct in git unless\nit is decoded.\nAt least \"fallbackEncoding\" gives it SOME chance of decoding it correctly.\n\n> I think another way to solve the issue you have is the encoding header\n> on git commits.  We can pass the bytes through git-p4 unmodified, but\n> mark the commit message as being encoded using something that isn't\n> UTF-8.  That avoids any potentially lossy conversions when cloning the\n> repository, but should allow the data to be displayed correctly in git.\n\nYes, that could be a solution.  I will try that out.\n\n> > In any event, if you look at my patch (v6 is the latest...\n> > https://lore.kernel.org/git/20210429073905.837-1-tzadik.vanderhoof@gmail.com/\n> > ), you will see I have written tests that pass under both Linux and\n> > Windows. (If you want to run them yourself, you need to base my patch\n> > off of \"master\", not \"seen\").  The tests make clear what the\n> > different behavior is and also show that p4d is not set to Unicode\n> > (since the tests do not change the default setting).\n>\n> I don't think the tests are doing anything interesting on Linux - you\n> stick valid UTF-8 in, and valid UTF-8 data comes out.\n\nTotally agree.... I only did that to get them to pass the Gitlab CI.\nI submitted an earlier\npatch that simply skipped the test file on Linux, but I got pushback\non that, so I\nmade them pass on Linux, even though they aren't useful.\n\n> I suspect the tests will fail on Windows if the relevant code page is set to a value\n> that you're not expecting.\n\nIt depends.  If the code page is set to UTF-8 (65001) I think the\ntests would still work,\nbecause as I said above, my code always tries to decode with UTF-8\nfirst, no matter what\nthe \"fallbackEncoding\" setting is.\n\nIf the code page is set to something other than UTF-8 or the default,\nthen one of my tests would\nfail, because it uses a hard-coded \"fallbackEncoding\" of \"cp1252\".\n\nBut the code would work for the user!  All they would need to do is\nset \"fallbackEncoding\" to\nthe code page they're actually using, instead of \"cp1252\".\n\n(More sophisticated tests could be developed that explicitly set the\ncode page and use the\ncorresponding \"fallbackEncoding\" setting.)\n\n> For the purposes of writing tests that work the same everywhere we can\n> use `p4 submit -i`.  The data written on stdin isn't reencoded, even on\n> Windows.\n\nI already have gone down the `p4 submit -i` road.  It behaves exactly\nthe same as\npassing the description on the command line.\n\n(One of several dead-ends I went down that I haven't mentioned in my emails)\n"},{"id":"423630","messageId":"xmqqczu656iv.fsf@gitster.g","threadId":"55479","inReplyTo":"CAKu1iLXOFiUGmQUeoW-YkiiJ8P2+LznXWz4YabEiGktv=nUYjA@mail.gmail.com","subject":"Re: [PATCH 2/2] git-p4: do not decode data from perforce by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-05T01:11:20Z","receivedAt":"2021-05-05T01:11:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tzadik Vanderhoof <tzadik.vanderhoof@gmail.com> writes:\n\n> On Tue, May 4, 2021 at 2:01 PM Andrew Oakley <andrew@adoakley.name> wrote:\n>> The key thing that I'm trying to point out here is that the encoding is\n>> not necessarily consistent between different commits.  The changes that\n>> you have proposed force you to pick one encoding that will be used for\n>> every commit.  If it's wrong then data will be corrupted, and there is\n>> no option provided to avoid that.  The only way I can see to avoid this\n>> issue is to not attempt to re-encode the data - just pass it directly\n>> to git.\n>\n> No, my \"fallbackEndocing\" setting is just that... a fallback.  My proposal\n> *always* tries to decode in UTF-8 first!  Only if that throws an exception\n> will my \"fallbackEncoding\" come into play, and it only comes into play\n> for the single changeset description that was invalid UTF-8.  After that,\n> subsequent descriptions will again be tried in UTF-8 first.\n\nHmph, I do not quite see the need for \"No\" at the beginning of what\nyou said.  The fallbackEncoding can specify only one non UTF-8\nencoding, so even if majority of commits were in UTF-8 but when you\nneed to import two commits with non UTF-8 encoding, there is no\nsuitable value to give to the fallbackEncoding setting.  One of\nthese two commits will fail to decode first in UTF-8 and then fail\nto decode again with the fallback, and after that a corrupted\nmessage remains.\n\n>> I think another way to solve the issue you have is the encoding header\n>> on git commits.  We can pass the bytes through git-p4 unmodified, but\n>> mark the commit message as being encoded using something that isn't\n>> UTF-8.  That avoids any potentially lossy conversions when cloning the\n>> repository, but should allow the data to be displayed correctly in git.\n>\n> Yes, that could be a solution.  I will try that out.\n\nIf we can determine in what encoding the thing that came out of\nPerforce is written in, we can put it on the encoding header of the\nresulting commit.  But if that is possible to begin with, perhaps we\ndo not even need to do so---if you can determine what the original\nencoding is, you can reencode with that encoding into UTF-8 inside\ngit-p4 while creating the commit, no?\n\nAnd if the raw data that came from Perforce cannot be reencoded to\nUTF-8 (i.e. iconv fails to process for some reason), then whether\nthe translation is done at the import time (i.e. where you would\nhave used the fallbackEncoding to reencode into UTF-8) or at the\ndisplay time (i.e. \"git show\" would notice the encoding header and\ntry to reencode the raw data from that encoding into UTF-8), it\nwould fail in the same way, so I do not see much advantage in\nwriting the encoding header into the resulting object (other than\nshifting the blame to downstream and keeping the original data\nintact, which is a good design principle).\n"},{"id":"423641","messageId":"CAKu1iLUaLuAZWqjNK4tfhhR=YaSt4MdQ+90ZY-JcEh_SeHyYCw@mail.gmail.com","threadId":"55479","inReplyTo":"xmqqczu656iv.fsf@gitster.g","subject":"Re: [PATCH 2/2] git-p4: do not decode data from perforce by default","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-05-05T04:02:54Z","receivedAt":"2021-05-05T04:03:10Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"On Tue, May 4, 2021 at 6:11 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Tzadik Vanderhoof <tzadik.vanderhoof@gmail.com> writes:\n>\n> > On Tue, May 4, 2021 at 2:01 PM Andrew Oakley <andrew@adoakley.name> wrote:\n> >> The key thing that I'm trying to point out here is that the encoding is\n> >> not necessarily consistent between different commits.  The changes that\n> >> you have proposed force you to pick one encoding that will be used for\n> >> every commit.  If it's wrong then data will be corrupted, and there is\n> >> no option provided to avoid that.  The only way I can see to avoid this\n> >> issue is to not attempt to re-encode the data - just pass it directly\n> >> to git.\n> >\n> > No, my \"fallbackEndocing\" setting is just that... a fallback.  My proposal\n> > *always* tries to decode in UTF-8 first!  Only if that throws an exception\n> > will my \"fallbackEncoding\" come into play, and it only comes into play\n> > for the single changeset description that was invalid UTF-8.  After that,\n> > subsequent descriptions will again be tried in UTF-8 first.\n>\n>  The fallbackEncoding can specify only one non UTF-8\n> encoding, so even if majority of commits were in UTF-8 but when you\n> need to import two commits with non UTF-8 encoding, there is no\n> suitable value to give to the fallbackEncoding setting.  One of\n> these two commits will fail to decode first in UTF-8 and then fail\n> to decode again with the fallback, and after that a corrupted\n> message remains.\n\nI'm not sure I understand your scenario.  If the majority of commits\nare in UTF-8, but there are 2 with the same UTF-8 encoding (say\n\"cp1252\"), then just set \"fallbackEndocing\" to \"cp1252\" and all\nthe commits will display fine.\n\nAre you talking about a scenario where most of the commits are UTF-8,\none is \"cp1252\" and another one is \"cp1251\", so a total of 3 encodings\nare used in the Perforce depot?  I don't think that is a common scenario.\n\nBut you have a point that my patch does not address that scenario.\n\n> If we can determine in what encoding the thing that came out of\n> Perforce is written in, we can put it on the encoding header of the\n> resulting commit.  But if that is possible to begin with, perhaps we\n> do not even need to do so---if you can determine what the original\n> encoding is, you can reencode with that encoding into UTF-8 inside\n> git-p4 while creating the commit, no?\n>\n> And if the raw data that came from Perforce cannot be reencoded to\n> UTF-8 (i.e. iconv fails to process for some reason), then whether\n> the translation is done at the import time (i.e. where you would\n> have used the fallbackEncoding to reencode into UTF-8) or at the\n> display time (i.e. \"git show\" would notice the encoding header and\n> try to reencode the raw data from that encoding into UTF-8), it\n> would fail in the same way, so I do not see much advantage in\n> writing the encoding header into the resulting object (other than\n> shifting the blame to downstream and keeping the original data\n> intact, which is a good design principle).\n\nI agree with the idea that if you know what the encoding is, then\nwhy not just use that knowledge to convert that to UTF-8, rather\nthan use the encoding header.\n\nI'm not sure about how complete the support in \"git\" is for encoding\nheaders. Does *everything* that reads commit messages respect the\nencoding header and handle it properly?  The documentation seems to\ndiscourage their use altogether and in any event, the main use case\nseems to be a project where everyone has settled on the same legacy\nencoding to use everywhere, which is not really the case for our situation.\n\nIf we want to abandon the \"fallbackEncoding\" direction, then the only other\noption I see is:\n\nStep 1) try to decode in UTF-8.  If that succeeds then it is almost certain\nthat it really was in UTF-8 (due to the design of UTF-8).  Make the commit\nin UTF-8.\n\nStep 2) If it failed to decode in UTF-8, either use heuristics to detect the\nencoding, or query the OS for the current code page and assume it's that.\nThen use the selected encoding to convert to UTF-8.\n\nOnly the heuristics option will satisfy the case of more than 1 encoding\n(in addition to UTF-8) being present in the Perforce depot, and in any event\nthe current code page may not help at all.\n\nI'm not really familiar with what encoding-detection heuristics are available\nfor Python and how reliable, stable or performant they are.  It would also\ninvolve taking on another library dependency.\n\nI will take a look at the encoding-detection options out there.\n"},{"id":"423642","messageId":"CAKu1iLWHBgG-9RkAeDRUUQwdjTeY00gxtDC_K+e1bGvRh4ZktA@mail.gmail.com","threadId":"55479","inReplyTo":"CAKu1iLUaLuAZWqjNK4tfhhR=YaSt4MdQ+90ZY-JcEh_SeHyYCw@mail.gmail.com","subject":"Re: [PATCH 2/2] git-p4: do not decode data from perforce by default","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-05-05T04:06:59Z","receivedAt":"2021-05-05T04:07:13Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"Oops, noticed a typo.... the paragraph should read:\n\nI'm not sure I understand your scenario.  If the majority of commits\nare in UTF-8, but there are 2 with the same *non* UTF-8 encoding (say\n\"cp1252\"), then just set \"fallbackEndocing\" to \"cp1252\" and all\nthe commits will display fine.\n"},{"id":"423646","messageId":"xmqq1ral4x47.fsf@gitster.g","threadId":"55479","inReplyTo":"CAKu1iLUaLuAZWqjNK4tfhhR=YaSt4MdQ+90ZY-JcEh_SeHyYCw@mail.gmail.com","subject":"Re: [PATCH 2/2] git-p4: do not decode data from perforce by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-05T04:34:32Z","receivedAt":"2021-05-05T04:34:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tzadik Vanderhoof <tzadik.vanderhoof@gmail.com> writes:\n\n> On Tue, May 4, 2021 at 6:11 PM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Tzadik Vanderhoof <tzadik.vanderhoof@gmail.com> writes:\n>>\n>> > On Tue, May 4, 2021 at 2:01 PM Andrew Oakley <andrew@adoakley.name> wrote:\n>> >> The key thing that I'm trying to point out here is that the encoding is\n>> >> not necessarily consistent between different commits.  The changes that\n>> >> you have proposed force you to pick one encoding that will be used for\n>> >> every commit.  If it's wrong then data will be corrupted, and there is\n>> >> no option provided to avoid that.  The only way I can see to avoid this\n>> >> issue is to not attempt to re-encode the data - just pass it directly\n>> >> to git.\n>> > ...\n> Are you talking about a scenario where most of the commits are UTF-8,\n> one is \"cp1252\" and another one is \"cp1251\", so a total of 3 encodings\n> are used in the Perforce depot?  I don't think that is a common scenario.\n\nYes.  I think that is where \"not necessarily consistent between\ndifferent commits\" leads us to---not limited only to two encodings.\n\n> I agree with the idea that if you know what the encoding is, then\n> why not just use that knowledge to convert that to UTF-8, rather\n> than use the encoding header.\n"}]}