{"thread":{"id":"57709","subject":"[PATCH] [RFC] git-p4: improve encoding handling to support inconsistent encodings","startedAt":"2022-04-11T09:43:04Z","lastAt":"2022-04-30T19:27:04Z","messageCount":13,"participants":["Tao Klerks via GitGitGadget","Ævar Arnfjörð Bjarmason","Tao Klerks","Andrew Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"453392","messageId":"pull.1206.git.1649670174972.gitgitgadget@gmail.com","threadId":"57709","inReplyTo":null,"subject":"[PATCH] [RFC] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-11T09:42:54Z","receivedAt":"2022-04-11T09:43:04Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\ngit-p4 is designed to run correctly under python2.7 and python3, but\nits functional behavior wrt importing user-entered text differs across\nthese environments:\n\nUnder python2, git-p4 \"naively\" writes the Perforce bytestream into git\nmetadata (and does not set an \"encoding\" header on the commits); this\nmeans that any non-utf-8 byte sequences end up creating invalidly-encoded\ndata in git.\n\nUnder python3, git-p4 attempts to decode the Perforce bytestream as utf-8\ndata, and fails badly (with an unhelpful error) when non-utf-8 data is\nencountered.\n\nPerforce clients (esp. p4v) encourage user entry of changelist\ndescriptions (and user full names) in OS-local encoding, and store the\nresulting bytestream to the server unmodified - such that different\nclients can end up creating mutually-unintelligible messages. The most\ncommon inconsistency, in many Perforce environments, is likely to be utf-8\n(typical in linux) vs cp-1252 (typical in windows).\n\nMake the changelist-description- and user-fullname-handling code\npython-runtime-agnostic, introducing three \"strategies\" selectable via\nconfig:\n- 'legacy', behaving as previously under python2,\n- 'strict', behaving as previously under python3, and\n- 'fallback', favoring utf-8 but supporting a secondary encoding when\nutf-8 decoding fails.\n\nKeep the python2 default behavior as-is ('legacy' strategy), but switch\nthe python3 default strategy to 'fallback' with fallback encoding\n'cp1252'.\n\nAlso include tests exercising these encoding strategies, documentation for\nthe new config, and improve the user-facing error messages when decoding\ndoes fail.\n\nSigned-off-by: Tao Klerks \n---\n RFC: Git p4 encoding strategy\n \n git-p4 is designed to run correctly under python2.7 and python3, but its\n functional behavior wrt importing user-entered text differs across these\n environments:\n \n Under python2, git-p4 \"naively\" writes the Perforce bytestream into git\n metadata (and does not set an \"encoding\" header on the commits); this\n means that any non-utf-8 byte sequences end up creating\n invalidly-encoded data in git.\n \n Under python3, git-p4 attempts to decode the Perforce bytestream as\n utf-8 data, and fails badly (with an unhelpful error) when non-utf-8\n data is encountered.\n \n Perforce clients (esp. p4v) encourage user entry of changelist\n descriptions (and user full names) in OS-local encoding, and store the\n resulting bytestream to the server unmodified - such that different\n clients can end up creating mutually-unintelligible messages. The most\n common inconsistency, in many Perforce environments, is likely to be\n utf-8 (typical in linux) vs cp-1252 (typical in windows).\n \n Make the changelist-description- and user-fullname-handling code\n python-runtime-agnostic, introducing three \"strategies\" selectable via\n config: 'legacy', behaving as previously under python2, 'strict',\n behaving as previously under python3, and 'fallback', favoring utf-8 but\n supporting a secondary encoding when utf-8 decoding fails.\n \n Keep the python2 default behavior as-is ('legacy' strategy), but switch\n the python3 default strategy to 'fallback' with fallback encoding\n 'cp1252'.\n \n Also include tests exercising these encoding strategies, documentation\n for the new config, and improve the user-facing error messages when\n decoding does fail.\n \n OPEN QUESTIONS:\n \n * Does it make sense to make \"fallback\" the default decoding strategy\n in python3? This is definitely a change in behavior, but I believe\n for the better; failing with \"we defaulted to strict, but you can run\n again with this other option if you want it to work\" seems unkind,\n only making sense if we thought fallback to cp1252 would be wrong in\n a substantial proportion of cases...\n * Is it OK to duplicate the bulk of the testing code across\n t9835-git-p4-metadata-encoding-python2.sh and\n t9836-git-p4-metadata-encoding-python3.sh?\n * Is it OK to explicitly call \"git-p4.py\" in tests, rather than the\n build output \"git-p4\", in order to be able to select the python\n runtime on a per-test basis?\n * Is it OK to look for python2 and python3 in /usr/bin/ (in testing),\n or would it be better to find them with \"which\"?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1206%2FTaoK%2Fgit-p4-encoding-strategy-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1206/TaoK/git-p4-encoding-strategy-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1206\n\n Documentation/git-p4.txt | 36 +++-\n git-p4.py | 104 +++++++++--\n t/lib-git-p4.sh | 3 +-\n t/t9835-git-p4-metadata-encoding-python2.sh | 185 +++++++++++++++++++\n t/t9836-git-p4-metadata-encoding-python3.sh | 186 ++++++++++++++++++++\n 5 files changed, 496 insertions(+), 18 deletions(-)\n create mode 100755 t/t9835-git-p4-metadata-encoding-python2.sh\n create mode 100755 t/t9836-git-p4-metadata-encoding-python3.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex e21fcd8f712..43b6f54a7d7 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -636,7 +636,41 @@ git-p4.pathEncoding::\n \tGit expects paths encoded as UTF-8. Use this config to tell git-p4\n \twhat encoding Perforce had used for the paths. This encoding is used\n \tto transcode the paths to UTF-8. As an example, Perforce on Windows\n-\toften uses \"cp1252\" to encode path names.\n+\toften uses \"cp1252\" to encode path names. If this option is passed\n+\tinto a p4 clone request, it is persisted in the resulting new git\n+\trepo.\n+\n+git-p4.metadataDecodingStrategy::\n+\tPerforce keeps the encoding of a changelist descriptions and user\n+\tfull names as stored by the client on a given OS. The p4v client\n+\tuses the OS-local encosing, and so different users can end up storing\n+\tdifferent changelist descriptions or user full names in different\n+\tencodings, in the same depot.\n+\tGit tolerates inconsistent/incorrect encodings in commit messages\n+\tand author names, but expects them to be specified in utf-8.\n+\tgit-p4 can use three different decoding strategies in handling the\n+\tencoding uncertainty in Perforce: 'legacy' simply passes the original\n+\tbytes through from Perforce to git, creating usable but\n+\tincorrectly-encoded data when the Perforce data is encoded as\n+\tanything other than utf-8. 'strict' expects the Perforce data to be\n+\tencoded as utf-8, and fails to import when this is not true.\n+\t'fallback' attempts to interpret the data as utf-8, and otherwise\n+\tfalls back to using a secondary encoding - by default the common\n+\twindows encoding 'cp-1252'.\n+\tUnder python2 the default strategy is 'legacy' for historical\n+\treasons, and under python3 the default is 'fallback'.\n+\tWhen 'strict' is selected and decoding fails, the error message will\n+\tpropose changing this config parameter as a workaround. If this\n+\toption is passed into a p4 clone request, it is persisted into the\n+\tresulting new git repo.\n+\n+git-p4.metadataFallbackEncoding::\n+\tSpecify the fallback encoding to use when decoding Perforce author\n+\tnames and changelists descriptions using the 'fallback' strategy\n+\t(see git-p4.metadataDecodingStrategy). The fallback encoding will\n+\tonly be used when decoding as utf-8 fails. This option defaults to\n+\tcp1252, a common windows encoding. If this option is passed into a\n+\tp4 clone request, it is persisted into the resulting new git repo.\n \n git-p4.largeFileSystem::\n \tSpecify the system that is used for large (binary) files. Please note\ndiff --git a/git-p4.py b/git-p4.py\nindex a9b1f904410..a2149dd38ae 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -54,6 +54,9 @@ defaultLabelRegexp = r'[a-zA-Z0-9_\\-.]+$'\n # The block size is reduced automatically if required\n defaultBlockSize = 1<<20\n \n+defaultMetadataDecodingStrategy = 'legacy' if sys.version_info.major == 2 else 'fallback'\n+defaultFallbackMetadataEncoding = 'cp1252'\n+\n p4_access_checked = False\n \n re_ko_keywords = re.compile(br'\\$(Id|Header)(:[^$\\n]+)?\\$')\n@@ -203,6 +206,52 @@ else:\n def encode_text_stream(s):\n return s.encode('utf_8') if isinstance(s, unicode) else s\n \n+class MetadataDecodingException(Exception):\n+ def __init__(self, input_string, fallback_encoding, fallback_error):\n+ self.input_string = input_string\n+ self.fallback_encoding = fallback_encoding\n+ self.fallback_error = fallback_error\n+\n+ def __str__(self):\n+ error_message = \"\"\"Decoding returned data failed!\n+The failing string was:\n+---\n+{}\n+---\"\"\".format(self.input_string)\n+\n+ if not self.fallback_error:\n+ error_message += \"\"\"\n+Consider setting the git-p4.metadataDecodingStrategy config option to\n+'fallback', to allow metadata to be decoded using a fallback encoding,\n+defaulting to cp1252.\"\"\"\n+ else:\n+ error_message += \"\"\"\n+The conversion failed while using the fallback encoding '{}';\n+consider using a more forgiving one. Conversion error text:\n+{}\n+\"\"\".format(self.fallback_encoding, self.fallback_error)\n+\n+ return error_message\n+\n+def metadata_stream_to_writable_bytes(s):\n+ encodingStrategy = gitConfig('git-p4.metadataDecodingStrategy') or defaultMetadataDecodingStrategy\n+ fallbackEncoding = gitConfig('git-p4.metadataFallbackEncoding') or defaultFallbackMetadataEncoding\n+ if not isinstance(s, bytes):\n+ return s.encode('utf_8')\n+ if encodingStrategy == 'legacy':\n+ return s\n+ try:\n+ s.decode('utf_8')\n+ return s\n+ except UnicodeDecodeError:\n+ fallback_error = None\n+ if encodingStrategy == 'fallback' and fallbackEncoding:\n+ try:\n+ return s.decode(fallbackEncoding).encode('utf_8')\n+ except Exception as e:\n+ fallback_error = e\n+ raise MetadataDecodingException(s, fallbackEncoding, fallback_error)\n+\n def decode_path(path):\n \"\"\"Decode a given string (bytes or otherwise) using configured path encoding options\n \"\"\"\n@@ -702,11 +751,12 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\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+ # - `desc` or `FullName` which may contain non-UTF8 encoded text handled below, eagerly converted to bytes\n+ # - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text, handled by decode_path()\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 ('data', 'desc', 'FullName', 'path', 'clientFile') or key.startswith('depotFile')):\n value = value.decode()\n decoded_entry[key] = value\n # Parse out data if it's an error response\n@@ -716,6 +766,10 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n if skip_info:\n if 'code' in entry and entry['code'] == 'info':\n continue\n+ if 'desc' in entry:\n+ entry['desc'] = metadata_stream_to_writable_bytes(entry['desc'])\n+ if 'FullName' in entry:\n+ entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName'])\n if cb is not None:\n cb(entry)\n else:\n@@ -1435,7 +1489,13 @@ class P4UserMap:\n for output in p4CmdList([\"users\"]):\n if \"User\" not in output:\n continue\n- self.users[output[\"User\"]] = output[\"FullName\"] + \" <\" + output[\"Email\"] + \">\"\n+ # \"FullName\" is bytes. \"Email\" on the other hand might be bytes\n+ # or unicode string depending on whether we are running under\n+ # python2 or python3. To support\n+ # git-p4.metadataDecodingStrategy=legacy, self.users dict values\n+ # are always bytes, ready to be written to git.\n+ emailbytes = metadata_stream_to_writable_bytes(output[\"Email\"])\n+ self.users[output[\"User\"]] = output[\"FullName\"] + b\" <\" + emailbytes + b\">\"\n self.emails[output[\"Email\"]] = output[\"User\"]\n \n mapUserConfigRegex = re.compile(r\"^\\s*(\\S+)\\s*=\\s*(.+)\\s*<(\\S+)>\\s*$\", re.VERBOSE)\n@@ -1445,26 +1505,28 @@ class P4UserMap:\n user = mapUser[0][0]\n fullname = mapUser[0][1]\n email = mapUser[0][2]\n- self.users[user] = fullname + \" <\" + email + \">\"\n+ fulluser = fullname + \" <\" + email + \">\"\n+ self.users[user] = metadata_stream_to_writable_bytes(fulluser)\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+ keybytes = metadata_stream_to_writable_bytes(key)\n+ s += b\"%s\\t%s\\n\" % (keybytes.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- self.users[entry[0]] = entry[1]\n+ entry = line.strip().split(b\"\\t\")\n+ self.users[entry[0].decode('utf_8')] = entry[1]\n except IOError:\n self.getUserMapFromPerforceServer()\n \n@@ -3020,7 +3082,8 @@ class P4Sync(Command, P4UserMap):\n if userid in self.users:\n return self.users[userid]\n else:\n- return \"%s \" % userid\n+ userid_bytes = metadata_stream_to_writable_bytes(userid)\n+ return b\"%s \" % userid_bytes\n \n def streamTag(self, gitStream, labelName, labelDetails, commit, epoch):\n \"\"\" Stream a p4 tag.\n@@ -3043,9 +3106,10 @@ class P4Sync(Command, P4UserMap):\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 \n- gitStream.write(\"tagger %s\\n\" % tagger)\n+ gitStream.write(\"tagger \")\n+ gitStream.write(email)\n+ gitStream.write(\" %s %s\\n\" % (epoch, self.tz))\n \n print(\"labelDetails=\",labelDetails)\n if 'Description' in labelDetails:\n@@ -3138,12 +3202,12 @@ class P4Sync(Command, P4UserMap):\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 \n- self.gitStream.write(\"committer %s\\n\" % committer)\n+ self.gitStream.write(\"committer \")\n+ self.gitStream.write(self.make_email(author))\n+ self.gitStream.write(\" %s %s\\n\" % (epoch, self.tz))\n \n self.gitStream.write(\"data <&1 | cut -c 8)\n+python_target_exists=$(/usr/bin/python$python_target_version -V 2>&1)\n+if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_exists\"\n+then\n+\tmkdir temp_python\n+\tPATH=\"$(pwd)/temp_python:$PATH\" && export PATH\n+\tln -s /usr/bin/python$python_target_version temp_python/python\n+fi\n+\n+python_major_version=$(python -V 2>&1 | cut -c 8)\n+if ! test \"$python_major_version\" = \"$python_target_version\"\n+then\n+\tskip_all=\"skipping python$python_target_version-specific git p4 tests; python$python_target_version not available\"\n+\ttest_done\n+fi\n+\n+remove_user_cache () {\n+\trm \"$HOME/.gitp4-usercache.txt\" || true\n+}\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'init depot' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\tp4_add_user \"utf8_author\" \"ǣuthor\" &&\n+\t\tP4USER=utf8_author &&\n+\t\ttouch file1 &&\n+\t\tp4 add file1 &&\n+\t\tp4 submit -d \"first CL has some utf-8 tǣxt\" &&\n+\n+\t\tp4_add_user \"latin1_author\" \"$(echo æuthor |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\t\tP4USER=latin1_author &&\n+\t\ttouch file2 &&\n+\t\tp4 add file2 &&\n+\t\tp4 submit -d \"$(echo second CL has some latin-1 tæxt |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\n+\t\tp4_add_user \"cp1252_author\" \"$(echo æuthœr |\n+\t\t\ticonv -f utf8 -t cp1252)\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file3 &&\n+\t\tp4 add file3 &&\n+\t\tp4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n+\t\t iconv -f utf8 -t cp1252)\"\n+\t)\n+'\n+\n+test_expect_success 'clone non-utf8 repo with strict encoding' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\ttest_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest=\"$git\" //depot@all 2>err &&\n+\tgrep \"Decoding returned data failed!\" err\n+'\n+\n+test_expect_success 'check utf-8 contents with legacy strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents corrupted in git with legacy strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tbadly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n+\t\tgrep \"$badly_encoded_in_git\" actual &&\n+\t\tbad_author_in_git=\"$(echo æuthor | iconv -f utf8 -t latin1)\" &&\n+\t\tgrep \"$bad_author_in_git\" actual\n+\t)\n+'\n+\n+test_expect_success 'check utf-8 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some latin-1 tæxt\" actual &&\n+\t\tgrep \"æuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"sœme cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file4 &&\n+\t\tp4 add file4 &&\n+\t\tp4 submit -d \"$(echo fourth CL has sœme more cp-1252 tæxt |\n+\t\t\ticonv -f utf8 -t cp1252)\"\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\n+\t\tgit p4.py sync --branch=master &&\n+\n+\t\tgit log p4/master >actual &&\n+\t\tcat actual &&\n+\t\tgrep \"sœme more cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+############################\n+## / END REPEATED SECTION ##\n+############################\n+\n+test_expect_success 'legacy (latin-1 contents corrupted in git) is the default with python2' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tbadly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n+\t\tgrep \"$badly_encoded_in_git\" actual\n+\t)\n+'\n+\n+test_done\ndiff --git a/t/t9836-git-p4-metadata-encoding-python3.sh b/t/t9836-git-p4-metadata-encoding-python3.sh\nnew file mode 100755\nindex 00000000000..ee2f707218b\n--- /dev/null\n+++ b/t/t9836-git-p4-metadata-encoding-python3.sh\n@@ -0,0 +1,186 @@\n+#!/bin/sh\n+\n+test_description='git p4 metadata encoding\n+\n+This test checks that the import process handles inconsistent text\n+encoding in p4 metadata (author names, commit messages, etc) without\n+failing, and produces maximally sane output in git.'\n+\n+. ./lib-git-p4.sh\n+\n+python_target_version='3'\n+\n+###############################\n+## SECTION REPEATED IN t9835 ##\n+###############################\n+\n+# HORRIBLE HACK TO ENSURE PYTHON VERSION!\n+# Weirdnesses:\n+# - Looking for python2 and python3 in a very specific path (/usr/bin/)\n+# - Code is inelegant\n+# - Code is duplicated (like most of this test script)\n+# - Test calls \"git-p4.py\" rather than \"git-p4\", because the latter references a specific path\n+\n+python_major_version=$(python -V 2>&1 | cut -c 8)\n+python_target_exists=$(/usr/bin/python$python_target_version -V 2>&1)\n+if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_exists\"\n+then\n+\tmkdir temp_python\n+\tPATH=\"$(pwd)/temp_python:$PATH\" && export PATH\n+\tln -s /usr/bin/python$python_target_version temp_python/python\n+fi\n+\n+python_major_version=$(python -V 2>&1 | cut -c 8)\n+if ! test \"$python_major_version\" = \"$python_target_version\"\n+then\n+\tskip_all=\"skipping python$python_target_version-specific git p4 tests; python$python_target_version not available\"\n+\ttest_done\n+fi\n+\n+remove_user_cache () {\n+\trm \"$HOME/.gitp4-usercache.txt\" || true\n+}\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'init depot' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\tp4_add_user \"utf8_author\" \"ǣuthor\" &&\n+\t\tP4USER=utf8_author &&\n+\t\ttouch file1 &&\n+\t\tp4 add file1 &&\n+\t\tp4 submit -d \"first CL has some utf-8 tǣxt\" &&\n+\n+\t\tp4_add_user \"latin1_author\" \"$(echo æuthor |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\t\tP4USER=latin1_author &&\n+\t\ttouch file2 &&\n+\t\tp4 add file2 &&\n+\t\tp4 submit -d \"$(echo second CL has some latin-1 tæxt |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\n+\t\tp4_add_user \"cp1252_author\" \"$(echo æuthœr |\n+\t\t\ticonv -f utf8 -t cp1252)\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file3 &&\n+\t\tp4 add file3 &&\n+\t\tp4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n+\t\t iconv -f utf8 -t cp1252)\"\n+\t)\n+'\n+\n+test_expect_success 'clone non-utf8 repo with strict encoding' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\ttest_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest=\"$git\" //depot@all 2>err &&\n+\tgrep \"Decoding returned data failed!\" err\n+'\n+\n+test_expect_success 'check utf-8 contents with legacy strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents corrupted in git with legacy strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tbadly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n+\t\tgrep \"$badly_encoded_in_git\" actual &&\n+\t\tbad_author_in_git=\"$(echo æuthor | iconv -f utf8 -t latin1)\" &&\n+\t\tgrep \"$bad_author_in_git\" actual\n+\t)\n+'\n+\n+test_expect_success 'check utf-8 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some latin-1 tæxt\" actual &&\n+\t\tgrep \"æuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"sœme cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file4 &&\n+\t\tp4 add file4 &&\n+\t\tp4 submit -d \"$(echo fourth CL has sœme more cp-1252 tæxt |\n+\t\t\ticonv -f utf8 -t cp1252)\"\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\n+\t\tgit p4.py sync --branch=master &&\n+\n+\t\tgit log p4/master >actual &&\n+\t\tcat actual &&\n+\t\tgrep \"sœme more cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+############################\n+## / END REPEATED SECTION ##\n+############################\n+\n+\n+test_expect_success 'fallback (both utf-8 and cp-1252 contents handled) is the default with python3' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"sœme cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+test_done\n\nbase-commit: 07330a41d66a2c9589b585a3a24ecdcf19994f19\n-- \ngitgitgadget\n"},{"id":"453473","messageId":"pull.1206.v2.git.1649831069578.gitgitgadget@gmail.com","threadId":"57709","inReplyTo":"pull.1206.git.1649670174972.gitgitgadget@gmail.com","subject":"[PATCH v2] [RFC] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-13T06:24:29Z","receivedAt":"2022-04-13T06:24:42Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\ngit-p4 is designed to run correctly under python2.7 and python3, but\nits functional behavior wrt importing user-entered text differs across\nthese environments:\n\nUnder python2, git-p4 \"naively\" writes the Perforce bytestream into git\nmetadata (and does not set an \"encoding\" header on the commits); this\nmeans that any non-utf-8 byte sequences end up creating invalidly-encoded\ncommit metadata in git.\n\nUnder python3, git-p4 attempts to decode the Perforce bytestream as utf-8\ndata, and fails badly (with an unhelpful error) when non-utf-8 data is\nencountered.\n\nPerforce clients (especially p4v) encourage user entry of changelist\ndescriptions (and user full names) in OS-local encoding, and store the\nresulting bytestream to the server unmodified - such that different\nclients can end up creating mutually-unintelligible messages. The most\ncommon inconsistency, in many Perforce environments, is likely to be utf-8\n(typical in linux) vs cp-1252 (typical in windows).\n\nMake the changelist-description- and user-fullname-handling code\npython-runtime-agnostic, introducing three \"strategies\" selectable via\nconfig:\n- 'legacy', behaving as previously under python2,\n- 'strict', behaving as previously under python3, and\n- 'fallback', favoring utf-8 but supporting a secondary encoding when\nutf-8 decoding fails, and finally replacing remaining unmappable bytes.\n\nKeep the python2 default behavior as-is ('legacy' strategy), but switch\nthe python3 default strategy to 'fallback' with fallback encoding\n'cp1252'.\n\nAlso include tests exercising these encoding strategies, documentation for\nthe new config, and improve the user-facing error messages when decoding\ndoes fail.\n\nSigned-off-by: Tao Klerks \n---\n RFC: Git p4 encoding strategy\n \n OPEN QUESTIONS:\n \n * Does it make sense to make \"fallback\" the default decoding strategy\n in python3? This is definitely a change in behavior, but I believe\n for the better; failing with \"we defaulted to strict, but you can run\n again with this other option if you want it to work\" seems unkind,\n only making sense if we thought fallback to cp1252 would be wrong in\n a substantial proportion of cases...\n * Is it OK to duplicate the bulk of the testing code across\n t9835-git-p4-metadata-encoding-python2.sh and\n t9836-git-p4-metadata-encoding-python3.sh?\n * Is it OK to explicitly call \"git-p4.py\" in tests, rather than the\n build output \"git-p4\", in order to be able to select the python\n runtime on a per-test basis? Is there a better approach?\n * Is the naming of the strategies appropriate? Should the default\n python2 strategy be called something less opinionated, like\n \"passthrough\"?\n \n Changes wrt v1:\n \n * Added \"and replace any remaining unmappable bytes\" behavior to the\n \"fallback\" strategy; common reasonable encodings like cp1252 still\n contain unmapped codepoints, and if those are found, there is really\n nothing that can be done about it other than ignoring the crazy\n bytes; this approach is consistent with the longstanding\n path-encoding-handling strategy.\n * Simplified error-handling accordingly\n * Cleaned up tests & commit messages slightly\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1206%2FTaoK%2Fgit-p4-encoding-strategy-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1206/TaoK/git-p4-encoding-strategy-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1206\n\nRange-diff vs v1:\n\n 1: 9d33aa125b0 ! 1: 6d227ad57ea [RFC] git-p4: improve encoding handling to support inconsistent encodings\n @@ Commit message\n Under python2, git-p4 \"naively\" writes the Perforce bytestream into git\n metadata (and does not set an \"encoding\" header on the commits); this\n means that any non-utf-8 byte sequences end up creating invalidly-encoded\n - data in git.\n + commit metadata in git.\n \n Under python3, git-p4 attempts to decode the Perforce bytestream as utf-8\n data, and fails badly (with an unhelpful error) when non-utf-8 data is\n encountered.\n \n - Perforce clients (esp. p4v) encourage user entry of changelist\n + Perforce clients (especially p4v) encourage user entry of changelist\n descriptions (and user full names) in OS-local encoding, and store the\n resulting bytestream to the server unmodified - such that different\n clients can end up creating mutually-unintelligible messages. The most\n @@ Commit message\n - 'legacy', behaving as previously under python2,\n - 'strict', behaving as previously under python3, and\n - 'fallback', favoring utf-8 but supporting a secondary encoding when\n - utf-8 decoding fails.\n + utf-8 decoding fails, and finally replacing remaining unmappable bytes.\n \n Keep the python2 default behavior as-is ('legacy' strategy), but switch\n the python3 default strategy to 'fallback' with fallback encoding\n @@ Documentation/git-p4.txt: git-p4.pathEncoding::\n +\tencoded as utf-8, and fails to import when this is not true.\n +\t'fallback' attempts to interpret the data as utf-8, and otherwise\n +\tfalls back to using a secondary encoding - by default the common\n -+\twindows encoding 'cp-1252'.\n ++\twindows encoding 'cp-1252' - with any remaining unparseable bytes\n ++\treplaced with a placeholder character.\n +\tUnder python2 the default strategy is 'legacy' for historical\n +\treasons, and under python3 the default is 'fallback'.\n +\tWhen 'strict' is selected and decoding fails, the error message will\n @@ git-p4.py: else:\n def encode_text_stream(s):\n return s.encode('utf_8') if isinstance(s, unicode) else s\n \n ++\n +class MetadataDecodingException(Exception):\n -+ def __init__(self, input_string, fallback_encoding, fallback_error):\n ++ def __init__(self, input_string):\n + self.input_string = input_string\n -+ self.fallback_encoding = fallback_encoding\n -+ self.fallback_error = fallback_error\n +\n + def __str__(self):\n -+ error_message = \"\"\"Decoding returned data failed!\n ++ return \"\"\"Decoding perforce metadata failed!\n +The failing string was:\n +---\n +{}\n -+---\"\"\".format(self.input_string)\n -+\n -+ if not self.fallback_error:\n -+ error_message += \"\"\"\n ++---\n +Consider setting the git-p4.metadataDecodingStrategy config option to\n +'fallback', to allow metadata to be decoded using a fallback encoding,\n -+defaulting to cp1252.\"\"\"\n -+ else:\n -+ error_message += \"\"\"\n -+The conversion failed while using the fallback encoding '{}';\n -+consider using a more forgiving one. Conversion error text:\n -+{}\n -+\"\"\".format(self.fallback_encoding, self.fallback_error)\n ++defaulting to cp1252.\"\"\".format(self.input_string)\n +\n -+ return error_message\n +\n +def metadata_stream_to_writable_bytes(s):\n + encodingStrategy = gitConfig('git-p4.metadataDecodingStrategy') or defaultMetadataDecodingStrategy\n @@ git-p4.py: else:\n + s.decode('utf_8')\n + return s\n + except UnicodeDecodeError:\n -+ fallback_error = None\n + if encodingStrategy == 'fallback' and fallbackEncoding:\n -+ try:\n -+ return s.decode(fallbackEncoding).encode('utf_8')\n -+ except Exception as e:\n -+ fallback_error = e\n -+ raise MetadataDecodingException(s, fallbackEncoding, fallback_error)\n ++ return s.decode(fallbackEncoding, errors='replace').encode('utf_8')\n ++ raise MetadataDecodingException(s)\n +\n def decode_path(path):\n \"\"\"Decode a given string (bytes or otherwise) using configured path encoding options\n @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n +## SECTION REPEATED IN t9836 ##\n +###############################\n +\n -+# HORRIBLE HACK TO ENSURE PYTHON VERSION!\n -+# Weirdnesses:\n -+# - Looking for python2 and python3 in a very specific path (/usr/bin/)\n -+# - Code is inelegant\n -+# - Code is duplicated (like most of this test script)\n -+# - Test calls \"git-p4.py\" rather than \"git-p4\", because the latter references a specific path\n ++# Please note: this test calls \"git-p4.py\" rather than \"git-p4\", because the\n ++# latter references a specific path so we can't easily force it to run under\n ++# the python version we need to.\n +\n +python_major_version=$(python -V 2>&1 | cut -c 8)\n -+python_target_exists=$(/usr/bin/python$python_target_version -V 2>&1)\n -+if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_exists\"\n ++python_target_binary=$(which python$python_target_version)\n ++if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_binary\"\n +then\n +\tmkdir temp_python\n +\tPATH=\"$(pwd)/temp_python:$PATH\" && export PATH\n -+\tln -s /usr/bin/python$python_target_version temp_python/python\n ++\tln -s $python_target_binary temp_python/python\n +fi\n +\n +python_major_version=$(python -V 2>&1 | cut -c 8)\n @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n +\ttest_when_finished cleanup_git &&\n +\ttest_when_finished remove_user_cache &&\n +\ttest_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest=\"$git\" //depot@all 2>err &&\n -+\tgrep \"Decoding returned data failed!\" err\n ++\tgrep \"Decoding perforce metadata failed!\" err\n +'\n +\n +test_expect_success 'check utf-8 contents with legacy strategy' '\n @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n +## SECTION REPEATED IN t9835 ##\n +###############################\n +\n -+# HORRIBLE HACK TO ENSURE PYTHON VERSION!\n -+# Weirdnesses:\n -+# - Looking for python2 and python3 in a very specific path (/usr/bin/)\n -+# - Code is inelegant\n -+# - Code is duplicated (like most of this test script)\n -+# - Test calls \"git-p4.py\" rather than \"git-p4\", because the latter references a specific path\n ++# Please note: this test calls \"git-p4.py\" rather than \"git-p4\", because the\n ++# latter references a specific path so we can't easily force it to run under\n ++# the python version we need to.\n +\n +python_major_version=$(python -V 2>&1 | cut -c 8)\n -+python_target_exists=$(/usr/bin/python$python_target_version -V 2>&1)\n -+if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_exists\"\n ++python_target_binary=$(which python$python_target_version)\n ++if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_binary\"\n +then\n +\tmkdir temp_python\n +\tPATH=\"$(pwd)/temp_python:$PATH\" && export PATH\n -+\tln -s /usr/bin/python$python_target_version temp_python/python\n ++\tln -s $python_target_binary temp_python/python\n +fi\n +\n +python_major_version=$(python -V 2>&1 | cut -c 8)\n @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n +\ttest_when_finished cleanup_git &&\n +\ttest_when_finished remove_user_cache &&\n +\ttest_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest=\"$git\" //depot@all 2>err &&\n -+\tgrep \"Decoding returned data failed!\" err\n ++\tgrep \"Decoding perforce metadata failed!\" err\n +'\n +\n +test_expect_success 'check utf-8 contents with legacy strategy' '\n\n\n Documentation/git-p4.txt | 37 +++-\n git-p4.py | 89 ++++++++--\n t/lib-git-p4.sh | 3 +-\n t/t9835-git-p4-metadata-encoding-python2.sh | 182 +++++++++++++++++++\n t/t9836-git-p4-metadata-encoding-python3.sh | 183 ++++++++++++++++++++\n 5 files changed, 476 insertions(+), 18 deletions(-)\n create mode 100755 t/t9835-git-p4-metadata-encoding-python2.sh\n create mode 100755 t/t9836-git-p4-metadata-encoding-python3.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex e21fcd8f712..e21bc6a5e37 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -636,7 +636,42 @@ git-p4.pathEncoding::\n \tGit expects paths encoded as UTF-8. Use this config to tell git-p4\n \twhat encoding Perforce had used for the paths. This encoding is used\n \tto transcode the paths to UTF-8. As an example, Perforce on Windows\n-\toften uses \"cp1252\" to encode path names.\n+\toften uses \"cp1252\" to encode path names. If this option is passed\n+\tinto a p4 clone request, it is persisted in the resulting new git\n+\trepo.\n+\n+git-p4.metadataDecodingStrategy::\n+\tPerforce keeps the encoding of a changelist descriptions and user\n+\tfull names as stored by the client on a given OS. The p4v client\n+\tuses the OS-local encosing, and so different users can end up storing\n+\tdifferent changelist descriptions or user full names in different\n+\tencodings, in the same depot.\n+\tGit tolerates inconsistent/incorrect encodings in commit messages\n+\tand author names, but expects them to be specified in utf-8.\n+\tgit-p4 can use three different decoding strategies in handling the\n+\tencoding uncertainty in Perforce: 'legacy' simply passes the original\n+\tbytes through from Perforce to git, creating usable but\n+\tincorrectly-encoded data when the Perforce data is encoded as\n+\tanything other than utf-8. 'strict' expects the Perforce data to be\n+\tencoded as utf-8, and fails to import when this is not true.\n+\t'fallback' attempts to interpret the data as utf-8, and otherwise\n+\tfalls back to using a secondary encoding - by default the common\n+\twindows encoding 'cp-1252' - with any remaining unparseable bytes\n+\treplaced with a placeholder character.\n+\tUnder python2 the default strategy is 'legacy' for historical\n+\treasons, and under python3 the default is 'fallback'.\n+\tWhen 'strict' is selected and decoding fails, the error message will\n+\tpropose changing this config parameter as a workaround. If this\n+\toption is passed into a p4 clone request, it is persisted into the\n+\tresulting new git repo.\n+\n+git-p4.metadataFallbackEncoding::\n+\tSpecify the fallback encoding to use when decoding Perforce author\n+\tnames and changelists descriptions using the 'fallback' strategy\n+\t(see git-p4.metadataDecodingStrategy). The fallback encoding will\n+\tonly be used when decoding as utf-8 fails. This option defaults to\n+\tcp1252, a common windows encoding. If this option is passed into a\n+\tp4 clone request, it is persisted into the resulting new git repo.\n \n git-p4.largeFileSystem::\n \tSpecify the system that is used for large (binary) files. Please note\ndiff --git a/git-p4.py b/git-p4.py\nindex a9b1f904410..c5e74aaa515 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -54,6 +54,9 @@ defaultLabelRegexp = r'[a-zA-Z0-9_\\-.]+$'\n # The block size is reduced automatically if required\n defaultBlockSize = 1<<20\n \n+defaultMetadataDecodingStrategy = 'legacy' if sys.version_info.major == 2 else 'fallback'\n+defaultFallbackMetadataEncoding = 'cp1252'\n+\n p4_access_checked = False\n \n re_ko_keywords = re.compile(br'\\$(Id|Header)(:[^$\\n]+)?\\$')\n@@ -203,6 +206,37 @@ else:\n def encode_text_stream(s):\n return s.encode('utf_8') if isinstance(s, unicode) else s\n \n+\n+class MetadataDecodingException(Exception):\n+ def __init__(self, input_string):\n+ self.input_string = input_string\n+\n+ def __str__(self):\n+ return \"\"\"Decoding perforce metadata failed!\n+The failing string was:\n+---\n+{}\n+---\n+Consider setting the git-p4.metadataDecodingStrategy config option to\n+'fallback', to allow metadata to be decoded using a fallback encoding,\n+defaulting to cp1252.\"\"\".format(self.input_string)\n+\n+\n+def metadata_stream_to_writable_bytes(s):\n+ encodingStrategy = gitConfig('git-p4.metadataDecodingStrategy') or defaultMetadataDecodingStrategy\n+ fallbackEncoding = gitConfig('git-p4.metadataFallbackEncoding') or defaultFallbackMetadataEncoding\n+ if not isinstance(s, bytes):\n+ return s.encode('utf_8')\n+ if encodingStrategy == 'legacy':\n+ return s\n+ try:\n+ s.decode('utf_8')\n+ return s\n+ except UnicodeDecodeError:\n+ if encodingStrategy == 'fallback' and fallbackEncoding:\n+ return s.decode(fallbackEncoding, errors='replace').encode('utf_8')\n+ raise MetadataDecodingException(s)\n+\n def decode_path(path):\n \"\"\"Decode a given string (bytes or otherwise) using configured path encoding options\n \"\"\"\n@@ -702,11 +736,12 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\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+ # - `desc` or `FullName` which may contain non-UTF8 encoded text handled below, eagerly converted to bytes\n+ # - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text, handled by decode_path()\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 ('data', 'desc', 'FullName', 'path', 'clientFile') or key.startswith('depotFile')):\n value = value.decode()\n decoded_entry[key] = value\n # Parse out data if it's an error response\n@@ -716,6 +751,10 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n if skip_info:\n if 'code' in entry and entry['code'] == 'info':\n continue\n+ if 'desc' in entry:\n+ entry['desc'] = metadata_stream_to_writable_bytes(entry['desc'])\n+ if 'FullName' in entry:\n+ entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName'])\n if cb is not None:\n cb(entry)\n else:\n@@ -1435,7 +1474,13 @@ class P4UserMap:\n for output in p4CmdList([\"users\"]):\n if \"User\" not in output:\n continue\n- self.users[output[\"User\"]] = output[\"FullName\"] + \" <\" + output[\"Email\"] + \">\"\n+ # \"FullName\" is bytes. \"Email\" on the other hand might be bytes\n+ # or unicode string depending on whether we are running under\n+ # python2 or python3. To support\n+ # git-p4.metadataDecodingStrategy=legacy, self.users dict values\n+ # are always bytes, ready to be written to git.\n+ emailbytes = metadata_stream_to_writable_bytes(output[\"Email\"])\n+ self.users[output[\"User\"]] = output[\"FullName\"] + b\" <\" + emailbytes + b\">\"\n self.emails[output[\"Email\"]] = output[\"User\"]\n \n mapUserConfigRegex = re.compile(r\"^\\s*(\\S+)\\s*=\\s*(.+)\\s*<(\\S+)>\\s*$\", re.VERBOSE)\n@@ -1445,26 +1490,28 @@ class P4UserMap:\n user = mapUser[0][0]\n fullname = mapUser[0][1]\n email = mapUser[0][2]\n- self.users[user] = fullname + \" <\" + email + \">\"\n+ fulluser = fullname + \" <\" + email + \">\"\n+ self.users[user] = metadata_stream_to_writable_bytes(fulluser)\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+ keybytes = metadata_stream_to_writable_bytes(key)\n+ s += b\"%s\\t%s\\n\" % (keybytes.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- self.users[entry[0]] = entry[1]\n+ entry = line.strip().split(b\"\\t\")\n+ self.users[entry[0].decode('utf_8')] = entry[1]\n except IOError:\n self.getUserMapFromPerforceServer()\n \n@@ -3020,7 +3067,8 @@ class P4Sync(Command, P4UserMap):\n if userid in self.users:\n return self.users[userid]\n else:\n- return \"%s \" % userid\n+ userid_bytes = metadata_stream_to_writable_bytes(userid)\n+ return b\"%s \" % userid_bytes\n \n def streamTag(self, gitStream, labelName, labelDetails, commit, epoch):\n \"\"\" Stream a p4 tag.\n@@ -3043,9 +3091,10 @@ class P4Sync(Command, P4UserMap):\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 \n- gitStream.write(\"tagger %s\\n\" % tagger)\n+ gitStream.write(\"tagger \")\n+ gitStream.write(email)\n+ gitStream.write(\" %s %s\\n\" % (epoch, self.tz))\n \n print(\"labelDetails=\",labelDetails)\n if 'Description' in labelDetails:\n@@ -3138,12 +3187,12 @@ class P4Sync(Command, P4UserMap):\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 \n- self.gitStream.write(\"committer %s\\n\" % committer)\n+ self.gitStream.write(\"committer \")\n+ self.gitStream.write(self.make_email(author))\n+ self.gitStream.write(\" %s %s\\n\" % (epoch, self.tz))\n \n self.gitStream.write(\"data <&1 | cut -c 8)\n+python_target_binary=$(which python$python_target_version)\n+if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_binary\"\n+then\n+\tmkdir temp_python\n+\tPATH=\"$(pwd)/temp_python:$PATH\" && export PATH\n+\tln -s $python_target_binary temp_python/python\n+fi\n+\n+python_major_version=$(python -V 2>&1 | cut -c 8)\n+if ! test \"$python_major_version\" = \"$python_target_version\"\n+then\n+\tskip_all=\"skipping python$python_target_version-specific git p4 tests; python$python_target_version not available\"\n+\ttest_done\n+fi\n+\n+remove_user_cache () {\n+\trm \"$HOME/.gitp4-usercache.txt\" || true\n+}\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'init depot' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\tp4_add_user \"utf8_author\" \"ǣuthor\" &&\n+\t\tP4USER=utf8_author &&\n+\t\ttouch file1 &&\n+\t\tp4 add file1 &&\n+\t\tp4 submit -d \"first CL has some utf-8 tǣxt\" &&\n+\n+\t\tp4_add_user \"latin1_author\" \"$(echo æuthor |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\t\tP4USER=latin1_author &&\n+\t\ttouch file2 &&\n+\t\tp4 add file2 &&\n+\t\tp4 submit -d \"$(echo second CL has some latin-1 tæxt |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\n+\t\tp4_add_user \"cp1252_author\" \"$(echo æuthœr |\n+\t\t\ticonv -f utf8 -t cp1252)\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file3 &&\n+\t\tp4 add file3 &&\n+\t\tp4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n+\t\t iconv -f utf8 -t cp1252)\"\n+\t)\n+'\n+\n+test_expect_success 'clone non-utf8 repo with strict encoding' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\ttest_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest=\"$git\" //depot@all 2>err &&\n+\tgrep \"Decoding perforce metadata failed!\" err\n+'\n+\n+test_expect_success 'check utf-8 contents with legacy strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents corrupted in git with legacy strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tbadly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n+\t\tgrep \"$badly_encoded_in_git\" actual &&\n+\t\tbad_author_in_git=\"$(echo æuthor | iconv -f utf8 -t latin1)\" &&\n+\t\tgrep \"$bad_author_in_git\" actual\n+\t)\n+'\n+\n+test_expect_success 'check utf-8 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some latin-1 tæxt\" actual &&\n+\t\tgrep \"æuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"sœme cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file4 &&\n+\t\tp4 add file4 &&\n+\t\tp4 submit -d \"$(echo fourth CL has sœme more cp-1252 tæxt |\n+\t\t\ticonv -f utf8 -t cp1252)\"\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\n+\t\tgit p4.py sync --branch=master &&\n+\n+\t\tgit log p4/master >actual &&\n+\t\tcat actual &&\n+\t\tgrep \"sœme more cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+############################\n+## / END REPEATED SECTION ##\n+############################\n+\n+test_expect_success 'legacy (latin-1 contents corrupted in git) is the default with python2' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tbadly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n+\t\tgrep \"$badly_encoded_in_git\" actual\n+\t)\n+'\n+\n+test_done\ndiff --git a/t/t9836-git-p4-metadata-encoding-python3.sh b/t/t9836-git-p4-metadata-encoding-python3.sh\nnew file mode 100755\nindex 00000000000..6c8e8cce5f1\n--- /dev/null\n+++ b/t/t9836-git-p4-metadata-encoding-python3.sh\n@@ -0,0 +1,183 @@\n+#!/bin/sh\n+\n+test_description='git p4 metadata encoding\n+\n+This test checks that the import process handles inconsistent text\n+encoding in p4 metadata (author names, commit messages, etc) without\n+failing, and produces maximally sane output in git.'\n+\n+. ./lib-git-p4.sh\n+\n+python_target_version='3'\n+\n+###############################\n+## SECTION REPEATED IN t9835 ##\n+###############################\n+\n+# Please note: this test calls \"git-p4.py\" rather than \"git-p4\", because the\n+# latter references a specific path so we can't easily force it to run under\n+# the python version we need to.\n+\n+python_major_version=$(python -V 2>&1 | cut -c 8)\n+python_target_binary=$(which python$python_target_version)\n+if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_binary\"\n+then\n+\tmkdir temp_python\n+\tPATH=\"$(pwd)/temp_python:$PATH\" && export PATH\n+\tln -s $python_target_binary temp_python/python\n+fi\n+\n+python_major_version=$(python -V 2>&1 | cut -c 8)\n+if ! test \"$python_major_version\" = \"$python_target_version\"\n+then\n+\tskip_all=\"skipping python$python_target_version-specific git p4 tests; python$python_target_version not available\"\n+\ttest_done\n+fi\n+\n+remove_user_cache () {\n+\trm \"$HOME/.gitp4-usercache.txt\" || true\n+}\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'init depot' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\tp4_add_user \"utf8_author\" \"ǣuthor\" &&\n+\t\tP4USER=utf8_author &&\n+\t\ttouch file1 &&\n+\t\tp4 add file1 &&\n+\t\tp4 submit -d \"first CL has some utf-8 tǣxt\" &&\n+\n+\t\tp4_add_user \"latin1_author\" \"$(echo æuthor |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\t\tP4USER=latin1_author &&\n+\t\ttouch file2 &&\n+\t\tp4 add file2 &&\n+\t\tp4 submit -d \"$(echo second CL has some latin-1 tæxt |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\n+\t\tp4_add_user \"cp1252_author\" \"$(echo æuthœr |\n+\t\t\ticonv -f utf8 -t cp1252)\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file3 &&\n+\t\tp4 add file3 &&\n+\t\tp4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n+\t\t iconv -f utf8 -t cp1252)\"\n+\t)\n+'\n+\n+test_expect_success 'clone non-utf8 repo with strict encoding' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\ttest_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest=\"$git\" //depot@all 2>err &&\n+\tgrep \"Decoding perforce metadata failed!\" err\n+'\n+\n+test_expect_success 'check utf-8 contents with legacy strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents corrupted in git with legacy strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tbadly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n+\t\tgrep \"$badly_encoded_in_git\" actual &&\n+\t\tbad_author_in_git=\"$(echo æuthor | iconv -f utf8 -t latin1)\" &&\n+\t\tgrep \"$bad_author_in_git\" actual\n+\t)\n+'\n+\n+test_expect_success 'check utf-8 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some latin-1 tæxt\" actual &&\n+\t\tgrep \"æuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"sœme cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file4 &&\n+\t\tp4 add file4 &&\n+\t\tp4 submit -d \"$(echo fourth CL has sœme more cp-1252 tæxt |\n+\t\t\ticonv -f utf8 -t cp1252)\"\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\n+\t\tgit p4.py sync --branch=master &&\n+\n+\t\tgit log p4/master >actual &&\n+\t\tcat actual &&\n+\t\tgrep \"sœme more cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+############################\n+## / END REPEATED SECTION ##\n+############################\n+\n+\n+test_expect_success 'fallback (both utf-8 and cp-1252 contents handled) is the default with python3' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"sœme cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+test_done\n\nbase-commit: 11cfe552610386954886543f5de87dcc49ad5735\n-- \ngitgitgadget\n"},{"id":"453479","messageId":"220413.86zgkpf5g7.gmgdl@evledraar.gmail.com","threadId":"57709","inReplyTo":"pull.1206.v2.git.1649831069578.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] [RFC] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-13T13:59:45Z","receivedAt":"2022-04-13T14:01:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 13 2022, Tao Klerks via GitGitGadget wrote:\n\n> Under python2, git-p4 \"naively\" writes the Perforce bytestream into git\n> metadata (and does not set an \"encoding\" header on the commits); this\n> means that any non-utf-8 byte sequences end up creating invalidly-encoded\n> commit metadata in git.\n\nIf it doesn't have an \"encoding\" header isn't any sequence of bytes OK\nwith git, so how does it create invalid metadata in git?\n\nDo you mean that something on the Python side gets confused and doesn't\ncorrectly encode it in that case, or that it's e.g. valid UTF-8 but\nwe're lacking the metadata?\n"},{"id":"453483","messageId":"CAPMMpoj3xZfKnH456AbiHatbBx98yXuj=yWBA8tdHhHdqn_H3Q@mail.gmail.com","threadId":"57709","inReplyTo":"220413.86zgkpf5g7.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] [RFC] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-04-13T15:18:51Z","receivedAt":"2022-04-13T15:19:13Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Wed, Apr 13, 2022 at 4:01 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Wed, Apr 13 2022, Tao Klerks via GitGitGadget wrote:\n>\n> > Under python2, git-p4 \"naively\" writes the Perforce bytestream into git\n> > metadata (and does not set an \"encoding\" header on the commits); this\n> > means that any non-utf-8 byte sequences end up creating invalidly-encoded\n> > commit metadata in git.\n>\n> If it doesn't have an \"encoding\" header isn't any sequence of bytes OK\n> with git, so how does it create invalid metadata in git?\n\nJust because git allows you to shove any sequence of bytes into a\ncommit header, doesn't mean the resulting text is \"valid\" metadata\ntext for all or most purposes. The correct way to encode text in git\ncommit metadata is utf-8 (OR tell any readers of this data that it's\nsomething other than utf-8 via the encoding header) - it's just that\ngit itself, the official client, is tolerant of bad byte sequences.\nOther clients are less tolerant. \"Sublime Merge\", for example, will\nfail to display the commit text at all in some contexts if the bytes\nare not valid utf-8 (or noted as being something else).\n\n>\n> Do you mean that something on the Python side gets confused and doesn't\n> correctly encode it in that case, or that it's e.g. valid UTF-8 but\n> we're lacking the metadata?\n\nIn git-p4 under python2, the bytes are simply copied from the perforce\ncommit metadata into the git commit metadata verbatim; if those bytes\nhappen to be valid utf-8, then they will be interpreted as such in git\nand everything is great. If that is *not* the case, eg the bytes are\nactually windows cp1252 (with bytes/characters in the x8a+ range),\nthen \"git log\" for example will output the raw bytes, and anything\nlooking at those bytes (a terminal, or a process that called git) will\nget those unexpected bytes, and need to deal accordingly. A terminal\nwill probably display \"unprintable character\" glyphs, python3 will\nblow up by default, python 2 will be perfectly happy by default, etc.\n\nI summarize this \"non-utf-8 bytes in a git commit message without a\nqualifying 'encoding' header\" situation as \"invalidly-encoded commit\nmetadata in git\", due to the impact on downstream consumers of git\nmetadata. Is there a better characterization?\n\nThanks,\nTao\n"},{"id":"453518","messageId":"220413.86sfqgerf7.gmgdl@evledraar.gmail.com","threadId":"57709","inReplyTo":"CAPMMpoj3xZfKnH456AbiHatbBx98yXuj=yWBA8tdHhHdqn_H3Q@mail.gmail.com","subject":"Re: [PATCH v2] [RFC] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-13T18:52:34Z","receivedAt":"2022-04-13T19:04:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 13 2022, Tao Klerks wrote:\n\n> On Wed, Apr 13, 2022 at 4:01 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>>\n>> On Wed, Apr 13 2022, Tao Klerks via GitGitGadget wrote:\n>>\n>> > Under python2, git-p4 \"naively\" writes the Perforce bytestream into git\n>> > metadata (and does not set an \"encoding\" header on the commits); this\n>> > means that any non-utf-8 byte sequences end up creating invalidly-encoded\n>> > commit metadata in git.\n>>\n>> If it doesn't have an \"encoding\" header isn't any sequence of bytes OK\n>> with git, so how does it create invalid metadata in git?\n>\n> Just because git allows you to shove any sequence of bytes into a\n> commit header, doesn't mean the resulting text is \"valid\" metadata\n> text for all or most purposes. The correct way to encode text in git\n> commit metadata is utf-8 (OR tell any readers of this data that it's\n> something other than utf-8 via the encoding header) - it's just that\n> git itself, the official client, is tolerant of bad byte sequences.\n> Other clients are less tolerant. \"Sublime Merge\", for example, will\n> fail to display the commit text at all in some contexts if the bytes\n> are not valid utf-8 (or noted as being something else).\n>\n>>\n>> Do you mean that something on the Python side gets confused and doesn't\n>> correctly encode it in that case, or that it's e.g. valid UTF-8 but\n>> we're lacking the metadata?\n>\n> In git-p4 under python2, the bytes are simply copied from the perforce\n> commit metadata into the git commit metadata verbatim; if those bytes\n> happen to be valid utf-8, then they will be interpreted as such in git\n> and everything is great. If that is *not* the case, eg the bytes are\n> actually windows cp1252 (with bytes/characters in the x8a+ range),\n> then \"git log\" for example will output the raw bytes, and anything\n> looking at those bytes (a terminal, or a process that called git) will\n> get those unexpected bytes, and need to deal accordingly. A terminal\n> will probably display \"unprintable character\" glyphs, python3 will\n> blow up by default, python 2 will be perfectly happy by default, etc.\n>\n> I summarize this \"non-utf-8 bytes in a git commit message without a\n> qualifying 'encoding' header\" situation as \"invalidly-encoded commit\n> metadata in git\", due to the impact on downstream consumers of git\n> metadata. Is there a better characterization?\n\nI must admit that all this time I'd been missing the \"Lack of this\nheader implies that the commit log message is encoded in UTF-8.\" part of\nthe docs added in 5dc7bcc2453 (Documentation: i18n commit log message\nnotes., 2006-12-30). I.e. I thought that \"encoding\"-header-less just\nmeant/implied \"whatever\".\n\nBut then again there wouldn't be much point in the encoding header if\nits value is \"utf8\", so surely we'd want to use the lack of a header to\ndisambiguate utf8 v.s. non-utf8.\n\nAFAICT we only allow selecting between encodings, not \"no idea what this\nis, but here's some raw sequence of bytes\", except by omitting the\nheader.\n\nIt seems to me that between legacy/strict/fallback there's a 4th setting\nmissing here. I.e. a \"try-encoding\". One where if your data is valid\nutf8 (or cp1252 if we want to get fancy and combine it with \"fallback\")\nyou get an encoding header, but having tried that we'll just write\nwhatever raw data we found, but *not* munge it.\n\nI haven't worked with p4, but having done some legacy SCM imports it was\nnice to be able to map data 1=1 to git in those \"odd encoding\" cases, or\neven cases where there was raw binary in a commit message or whatever...\n\nAnyway, all of this is fine with me as-is, I just had a drive-by\nquestion after some admittedly not very careful reading, sorry.\n"},{"id":"453598","messageId":"20220413214109.48097ac1@ado-tr.dyn.home.arpa","threadId":"57709","inReplyTo":"pull.1206.v2.git.1649831069578.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] [RFC] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Andrew Oakley","fromEmail":"andrew@adoakley.name","sentAt":"2022-04-13T20:41:09Z","receivedAt":"2022-04-13T21:02:37Z","isPatch":true,"sender":{"key":"andrew@adoakley.name","avatar":"https://avatars.githubusercontent.com/u/1107440?v=4"},"body":"Thanks for doing this. I've been meaning to write some similar code\nfor years and never quite got around to it. So maybe my opinion\nshouldn't be worth much :/.\n\nOn Wed, 13 Apr 2022 06:24:29 +0000\n\"Tao Klerks via GitGitGadget\" wrote:\n> Make the changelist-description- and user-fullname-handling code\n> python-runtime-agnostic, introducing three \"strategies\" selectable via\n> config:\n> - 'legacy', behaving as previously under python2,\n> - 'strict', behaving as previously under python3, and\n> - 'fallback', favoring utf-8 but supporting a secondary encoding when\n> utf-8 decoding fails, and finally replacing remaining unmappable\n> bytes.\n\nI was thinking about making the config option be a list of encodings to\ntry. So the options you've given map something like this:\n- \"legacy\" -> \"raw\"\n- \"strict\" -> \"utf8\"\n- \"fallback\" -> \"utf8 cp1252\" (or whatever is configured)\n\nThis doesn't handle the case of using a replacement character, but in\nreality I suspect that fallback encoding will always be a legacy 8bit\ncodec anyway.\n\nI think what you've proposed is fine too, I'm not sure what would end\nup being easier to understand.\n\n> * Does it make sense to make \"fallback\" the default decoding\n> strategy in python3? This is definitely a change in behavior, but I\n> believe for the better; failing with \"we defaulted to strict, but you\n> can run again with this other option if you want it to work\" seems\n> unkind, only making sense if we thought fallback to cp1252 would be\n> wrong in a substantial proportion of cases...\n\nThe only issue I can see with changing the default is that it might\nlead to a surprising loss of data for someone migrating to git. Maybe\nprint a warning the first time git-p4 encounters something that can't be\ndecoded as UTF-8, but then continue with the fallback to cp1252?\n\n> * Is it OK to duplicate the bulk of the testing code across\n> t9835-git-p4-metadata-encoding-python2.sh and\n> t9836-git-p4-metadata-encoding-python3.sh?\n> * Is it OK to explicitly call \"git-p4.py\" in tests, rather than\n> the build output \"git-p4\", in order to be able to select the python\n> runtime on a per-test basis? Is there a better approach?\n\nI tried to find a nicer way to do this and failed.\n\n> * Is the naming of the strategies appropriate? Should the default\n> python2 strategy be called something less opinionated, like\n> \"passthrough\"?\n\nI think that \"passthrough\" or \"raw\" would be more descriptive names.\n\nThe changes to git-p4 itself look good to me. I think that dealing\nwith bytes more and strings less will be good going forward.\n"},{"id":"453646","messageId":"CAPMMpojSp0kdAC7JD10kv+rODKV3eYdt0W1cNva3tzF3sLru+A@mail.gmail.com","threadId":"57709","inReplyTo":"220413.86sfqgerf7.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] [RFC] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-04-14T09:38:42Z","receivedAt":"2022-04-14T09:38:59Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Wed, Apr 13, 2022 at 9:04 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> AFAICT we only allow selecting between encodings, not \"no idea what this\n> is, but here's some raw sequence of bytes\", except by omitting the\n> header.\n>\n\nI don't understand what you mean here. My reading of the docs is that\nomitting the header *does not* imply \"no idea what this is\" - it\nimplies \"this is utf-8\".\n\nGit will do the best it can (convert to utf-8 at output time if the\nbytes are parseable in the specified or implied encoding, and return\nthe raw bytes otherwise), *regardless* of whether an encoding is\nexplicitly specified or utf-8 is implied.\n\n> It seems to me that between legacy/strict/fallback there's a 4th setting\n> missing here. I.e. a \"try-encoding\". One where if your data is valid\n> utf8 (or cp1252 if we want to get fancy and combine it with \"fallback\")\n> you get an encoding header, but having tried that we'll just write\n> whatever raw data we found, but *not* munge it.\n\nFor utf-8 specifically, the \"legacy\" strategy achieves this effect\nwith less overhead: It copies the bytes over raw, and in git the\nimplied encoding is utf-8. So if the bytes were utf-8 in the first\nplace, then they're good (we didn't need to munge them in any way),\nand if they weren't utf-8 we copied them over anyway, which is the\n\"tried and failed\" behavior you propose also.\n\nFor cp1252 or another encoding, the behavior you propose is an\nenhancement/modification of the current \"fallback\" behavior, I guess:\nCurrently if the fallback encoding doesn't wash, any remaining \"bad\nbytes\" get replaced out of existence. This leads to data loss of those\nindividual bytes, and it also means that if the data really wasn't\ncp1252 in the first place, you might just have garbled up the data\nsomething awful. Of course, you might have done that without any\nerrors anyway - cp1252 only leaves 4 unmapped bytes, so most arbitrary\ntext data will be successfully \"interpreted\", no matter how\nerroneously.\n\nThe advantage of the current \"replace\" behavior is that you *always*\nend up with valid utf-8 data in your commit text. The disadvantage is\nthat you can suffer (minor) unrecoverable data loss.\n\nI think your proposal is that (optionally?) the fallback-or-raw\nbehavior would simply spit out / leave the original bytes in this\nsituation, not making any attempt to interpret them or convert them to\nutf-8, but simply bring them to git as-is. This would make that\nparticular commit message \"invalidly encoded\", in the sense that any\ngit client or git-caller would need to \"do what it can\" with that\nsequence of bytes.\n\nThis tradeoff between avoidance of data loss, and type/encoding\ncoherence, is not one I'm comfortable deciding on. I would ideally\nprefer a third route, where the data *is* interpreted and converted,\nbut in a fully reversible way.\n\nWhat would you think of a scheme where, *if* the fallback encoding\nfails to decode successfully, we simply take all x80+ bytes and escape\nthem to a form like \"\\x8c\", so a commit message might end up like\n\"solve the problem with the japanese character: \\8f\\c4.\"? Would this\nway of preserving the bytes, without breaking out of having a known\n(utf-8) encoding in the git commits, make sense to you? We could even\nadd a suffix to the message like \"[original data contained bytes that\ncould not be mapped in targeted encoding cp1252; bytes at or over x80\nwere escaped as \\xNN, and backslashes were escaped as \\\\ to avoid\nambiguity]\", or something less horrendously verbose :)\n\n>\n> I haven't worked with p4, but having done some legacy SCM imports it was\n> nice to be able to map data 1=1 to git in those \"odd encoding\" cases, or\n> even cases where there was raw binary in a commit message or whatever...\n>\n\nMy problem here, again, is that these \"badly encoded commit messages\"\nendure forever in your commit history: any tool that wants to parse\nyour history will have to deal with them, skirt around them, etc. That\njust seems like bad discipline. If the intent is to ensure that you\n*can* reconstruct those bytes if/when you need to, we should find a\nway to store them safely, in a way that won't randomly trip others up.\n\n> Anyway, all of this is fine with me as-is, I just had a drive-by\n> question after some admittedly not very careful reading, sorry.\n\nThanks for looking into it!\n"},{"id":"453647","messageId":"CAPMMpoiXNKNnARhJ2n+nzOj==-27YA68OvMmUyYnSaoLbfE4xw@mail.gmail.com","threadId":"57709","inReplyTo":"20220413214109.48097ac1@ado-tr.dyn.home.arpa","subject":"Re: [PATCH v2] [RFC] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-04-14T09:57:29Z","receivedAt":"2022-04-14T09:57:44Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Wed, Apr 13, 2022 at 10:41 PM Andrew Oakley wrote:\n>\n> On Wed, 13 Apr 2022 06:24:29 +0000\n> \"Tao Klerks via GitGitGadget\" wrote:\n> > Make the changelist-description- and user-fullname-handling code\n> > python-runtime-agnostic, introducing three \"strategies\" selectable via\n> > config:\n> > - 'legacy', behaving as previously under python2,\n> > - 'strict', behaving as previously under python3, and\n> > - 'fallback', favoring utf-8 but supporting a secondary encoding when\n> > utf-8 decoding fails, and finally replacing remaining unmappable\n> > bytes.\n>\n> I was thinking about making the config option be a list of encodings to\n> try. So the options you've given map something like this:\n> - \"legacy\" -> \"raw\"\n> - \"strict\" -> \"utf8\"\n> - \"fallback\" -> \"utf8 cp1252\" (or whatever is configured)\n>\n> This doesn't handle the case of using a replacement character, but in\n> reality I suspect that fallback encoding will always be a legacy 8bit\n> codec anyway.\n>\n> I think what you've proposed is fine too, I'm not sure what would end\n> up being easier to understand.\n\nI'm not sure I understand the proposal... Specifically, I don't\nunderstand how \"raw\" would work in that scheme.\n\nIn \"my\" current scheme, there is a big difference between \"legacy\" and\nthe other two strategies: the legacy strategy reads \"raw\", but also\n*writes* \"raw\".\n\nThe other schemes read whatever encoding, and then write utf-8. In the\ncase of strict, that actually works out the same as \"raw\", as long as\nthe input bytes were valid utf-8 (and otherwise nothing happens\nanyway). In the case of fallback, that's a completely different\nbehavior to legacy's read-raw write-raw.\n\nIs your proposal to independently specify the read encodings *and* the\nwrite encoding, as separate parameters? That was actually my original\napproach, but it turned out to, in my opinion, be harder to understand\n(and implement :) )\n\n>\n> > * Does it make sense to make \"fallback\" the default decoding\n> > strategy in python3? This is definitely a change in behavior, but I\n> > believe for the better; failing with \"we defaulted to strict, but you\n> > can run again with this other option if you want it to work\" seems\n> > unkind, only making sense if we thought fallback to cp1252 would be\n> > wrong in a substantial proportion of cases...\n>\n> The only issue I can see with changing the default is that it might\n> lead to a surprising loss of data for someone migrating to git. Maybe\n> print a warning the first time git-p4 encounters something that can't be\n> decoded as UTF-8, but then continue with the fallback to cp1252?\n\nHonestly, I'm not sure how much a warning does. In my perforce repo,\nfor example, any new warnings during the import would get drowned out\nby the mac metadata ignoring warnings.\n\nI understand and share the data loss concern.\n\nAs I just answered Ævar, I *think* I'd like to address the data loss\nconcern by escaping all x80+ bytes if something cannot be interpreted\neven using the fallback encoding. In a commit message there could also\nbe a suffix explaining what happened, although I suspect that's\npointless complexity. The advantage of this approach is that it makes\nit *possible* to reconstruct the original bytestream precisely, but\nwithout creating badly-encoded git commit messages that need to be\nskirted around.\n\n>\n> > * Is it OK to duplicate the bulk of the testing code across\n> > t9835-git-p4-metadata-encoding-python2.sh and\n> > t9836-git-p4-metadata-encoding-python3.sh?\n> > * Is it OK to explicitly call \"git-p4.py\" in tests, rather than\n> > the build output \"git-p4\", in order to be able to select the python\n> > runtime on a per-test basis? Is there a better approach?\n>\n> I tried to find a nicer way to do this and failed.\n\nOK thx.\n\n>\n> > * Is the naming of the strategies appropriate? Should the default\n> > python2 strategy be called something less opinionated, like\n> > \"passthrough\"?\n>\n> I think that \"passthrough\" or \"raw\" would be more descriptive names.\n>\n\nOK thx, I'll take \"passthrough\" as it feels slightly less positive\nthan \"raw\", for some reason that I can't put my finger on :)\n\n> The changes to git-p4 itself look good to me. I think that dealing\n> with bytes more and strings less will be good going forward.\n\nThx for your feedback!\n"},{"id":"453798","messageId":"80e83d8e-1f68-16be-6d68-fbc4aadfc78d@adoakley.name","threadId":"57709","inReplyTo":"CAPMMpoiXNKNnARhJ2n+nzOj==-27YA68OvMmUyYnSaoLbfE4xw@mail.gmail.com","subject":"Re: [PATCH v2] [RFC] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Andrew Oakley","fromEmail":"andrew@adoakley.name","sentAt":"2022-04-17T18:11:44Z","receivedAt":"2022-04-17T18:17:20Z","isPatch":true,"sender":{"key":"andrew@adoakley.name","avatar":"https://avatars.githubusercontent.com/u/1107440?v=4"},"body":"On 14/04/2022 10:57, Tao Klerks wrote:\n> On Wed, Apr 13, 2022 at 10:41 PM Andrew Oakley wrote:\n>>\n>> On Wed, 13 Apr 2022 06:24:29 +0000\n>> \"Tao Klerks via GitGitGadget\" wrote:\n>>> Make the changelist-description- and user-fullname-handling code\n>>> python-runtime-agnostic, introducing three \"strategies\" selectable via\n>>> config:\n>>> - 'legacy', behaving as previously under python2,\n>>> - 'strict', behaving as previously under python3, and\n>>> - 'fallback', favoring utf-8 but supporting a secondary encoding when\n>>> utf-8 decoding fails, and finally replacing remaining unmappable\n>>> bytes.\n>>\n>> I was thinking about making the config option be a list of encodings to\n>> try. So the options you've given map something like this:\n>> - \"legacy\" -> \"raw\"\n>> - \"strict\" -> \"utf8\"\n>> - \"fallback\" -> \"utf8 cp1252\" (or whatever is configured)\n>>\n>> This doesn't handle the case of using a replacement character, but in\n>> reality I suspect that fallback encoding will always be a legacy 8bit\n>> codec anyway.\n>>\n>> I think what you've proposed is fine too, I'm not sure what would end\n>> up being easier to understand.\n> \n> I'm not sure I understand the proposal... Specifically, I don't\n> understand how \"raw\" would work in that scheme.\n> \n> In \"my\" current scheme, there is a big difference between \"legacy\" and\n> the other two strategies: the legacy strategy reads \"raw\", but also\n> *writes* \"raw\".\n> \n> The other schemes read whatever encoding, and then write utf-8. In the\n> case of strict, that actually works out the same as \"raw\", as long as\n> the input bytes were valid utf-8 (and otherwise nothing happens\n> anyway). In the case of fallback, that's a completely different\n> behavior to legacy's read-raw write-raw.\n\nThe way I look at it is that you both read and write bytes, and you may \nattempt to decode and re-encode text on the way. Both the decoding and \nthe encoding are done in metadata_stream_to_writable_bytes, so nothing \nelse needs to know about the raw option being different.\n\nPerhaps it's easier to explain with a bit of (untested) code. I was \nthinking of something like this:\n\ndef metadata_stream_to_writable_bytes(s):\n if not isinstance(s, bytes):\n return s.encode('utf-8')\n\n for encoding in gitConfigList('git-p4.metadataEncoding') or \n['utf-8', 'cp1252']:\n if encoding == 'passthrough':\n return s # do not try to correct text encoding\n else:\n try:\n return s.decode(encoding).encode('utf-8')\n except UnicodeDecodeError:\n pass # try the next configured encoding\n\n raise MetadataDecodingException(s)\n> Is your proposal to independently specify the read encodings *and* the\n> write encoding, as separate parameters? That was actually my original\n> approach, but it turned out to, in my opinion, be harder to understand\n> (and implement :) )\n\nI don't think it's important to be able specify the encoding to be used \nin git. I've not spotted anyone asking for that feature. I think it \ncould be added later if someone needs it.\n\n>>> * Does it make sense to make \"fallback\" the default decoding\n>>> strategy in python3? This is definitely a change in behavior, but I\n>>> believe for the better; failing with \"we defaulted to strict, but you\n>>> can run again with this other option if you want it to work\" seems\n>>> unkind, only making sense if we thought fallback to cp1252 would be\n>>> wrong in a substantial proportion of cases...\n>>\n>> The only issue I can see with changing the default is that it might\n>> lead to a surprising loss of data for someone migrating to git. Maybe\n>> print a warning the first time git-p4 encounters something that can't be\n>> decoded as UTF-8, but then continue with the fallback to cp1252?\n> \n> Honestly, I'm not sure how much a warning does. In my perforce repo,\n> for example, any new warnings during the import would get drowned out\n> by the mac metadata ignoring warnings.\n\nFWIW in the perforce repository I work with this doesn't happen much and \nI would notice the additional warning about text encodings. I suspect \nthis will be another thing which varies a lot between repositories.\n\n> I understand and share the data loss concern.\n> \n> As I just answered Ævar, I *think* I'd like to address the data loss\n> concern by escaping all x80+ bytes if something cannot be interpreted\n> even using the fallback encoding. In a commit message there could also\n> be a suffix explaining what happened, although I suspect that's\n> pointless complexity. The advantage of this approach is that it makes\n> it *possible* to reconstruct the original bytestream precisely, but\n> without creating badly-encoded git commit messages that need to be\n> skirted around.\n\nI think this gets pretty messy though. In my opinion it's not any nicer \nthan putting the raw bytes in the commit message.\n\nGit does not make any attempt enforce the commit metadata encoding, so I \nthink that tools really should make an attempt to handle invalid data in \na somewhat sensible fashion.\n\nI don't think there is really a \"right\" answer, anything reasonable \nwould be better than what we've got now.\n"},{"id":"453939","messageId":"pull.1206.v3.git.1650399590844.gitgitgadget@gmail.com","threadId":"57709","inReplyTo":"pull.1206.v2.git.1649831069578.gitgitgadget@gmail.com","subject":"[PATCH v3] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-19T20:19:50Z","receivedAt":"2022-04-19T20:20:22Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\ngit-p4 is designed to run correctly under python2.7 and python3, but\nits functional behavior wrt importing user-entered text differs across\nthese environments:\n\nUnder python2, git-p4 \"naively\" writes the Perforce bytestream into git\nmetadata (and does not set an \"encoding\" header on the commits); this\nmeans that any non-utf-8 byte sequences end up creating invalidly-encoded\ncommit metadata in git.\n\nUnder python3, git-p4 attempts to decode the Perforce bytestream as utf-8\ndata, and fails badly (with an unhelpful error) when non-utf-8 data is\nencountered.\n\nPerforce clients (especially p4v) encourage user entry of changelist\ndescriptions (and user full names) in OS-local encoding, and store the\nresulting bytestream to the server unmodified - such that different\nclients can end up creating mutually-unintelligible messages. The most\ncommon inconsistency, in many Perforce environments, is likely to be utf-8\n(typical in linux) vs cp-1252 (typical in windows).\n\nMake the changelist-description- and user-fullname-handling code\npython-runtime-agnostic, introducing three \"strategies\" selectable via\nconfig:\n- 'passthrough', behaving as previously under python2,\n- 'strict', behaving as previously under python3, and\n- 'fallback', favoring utf-8 but supporting a secondary encoding when\nutf-8 decoding fails, and finally escaping high-range bytes if the\ndecoding with the secondary encoding also fails.\n\nKeep the python2 default behavior as-is ('legacy' strategy), but switch\nthe python3 default strategy to 'fallback' with default fallback encoding\n'cp1252'.\n\nAlso include tests exercising these encoding strategies, documentation for\nthe new config, and improve the user-facing error messages when decoding\ndoes fail.\n\nSigned-off-by: Tao Klerks \n---\n RFC: Git p4 encoding strategy\n \n OPEN QUESTIONS:\n \n * Does it make sense to make \"fallback\" the default decoding strategy\n in python3? This is definitely a change in behavior, but I believe\n for the better; failing with \"we defaulted to strict, but you can run\n again with this other option if you want it to work\" seems unkind,\n only making sense if we thought fallback to cp1252 would be wrong in\n a substantial proportion of cases...\n * Is it OK to duplicate the bulk of the testing code across\n t9835-git-p4-metadata-encoding-python2.sh and\n t9836-git-p4-metadata-encoding-python3.sh?\n * Is it OK to explicitly call \"git-p4.py\" in tests, rather than the\n build output \"git-p4\", in order to be able to select the python\n runtime on a per-test basis? Is there a better approach?\n * Is the naming of the strategies appropriate? Should the default\n python2 strategy be called something less opinionated, like\n \"passthrough\"?\n \n Changes wrt v1:\n \n * Added \"and replace any remaining unmappable bytes\" behavior to the\n \"fallback\" strategy; common reasonable encodings like cp1252 still\n contain unmapped codepoints, and if those are found, there is really\n nothing that can be done about it other than ignoring the crazy\n bytes; this approach is consistent with the longstanding\n path-encoding-handling strategy.\n * Simplified error-handling accordingly\n * Cleaned up tests & commit messages slightly\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1206%2FTaoK%2Fgit-p4-encoding-strategy-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1206/TaoK/git-p4-encoding-strategy-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1206\n\nRange-diff vs v2:\n\n 1: 6d227ad57ea ! 1: 1d83b6d7865 [RFC] git-p4: improve encoding handling to support inconsistent encodings\n @@ Metadata\n Author: Tao Klerks \n \n ## Commit message ##\n - [RFC] git-p4: improve encoding handling to support inconsistent encodings\n + git-p4: improve encoding handling to support inconsistent encodings\n \n git-p4 is designed to run correctly under python2.7 and python3, but\n its functional behavior wrt importing user-entered text differs across\n @@ Commit message\n Make the changelist-description- and user-fullname-handling code\n python-runtime-agnostic, introducing three \"strategies\" selectable via\n config:\n - - 'legacy', behaving as previously under python2,\n + - 'passthrough', behaving as previously under python2,\n - 'strict', behaving as previously under python3, and\n - 'fallback', favoring utf-8 but supporting a secondary encoding when\n - utf-8 decoding fails, and finally replacing remaining unmappable bytes.\n + utf-8 decoding fails, and finally escaping high-range bytes if the\n + decoding with the secondary encoding also fails.\n \n Keep the python2 default behavior as-is ('legacy' strategy), but switch\n - the python3 default strategy to 'fallback' with fallback encoding\n + the python3 default strategy to 'fallback' with default fallback encoding\n 'cp1252'.\n \n Also include tests exercising these encoding strategies, documentation for\n @@ Documentation/git-p4.txt: git-p4.pathEncoding::\n +git-p4.metadataDecodingStrategy::\n +\tPerforce keeps the encoding of a changelist descriptions and user\n +\tfull names as stored by the client on a given OS. The p4v client\n -+\tuses the OS-local encosing, and so different users can end up storing\n ++\tuses the OS-local encoding, and so different users can end up storing\n +\tdifferent changelist descriptions or user full names in different\n +\tencodings, in the same depot.\n +\tGit tolerates inconsistent/incorrect encodings in commit messages\n +\tand author names, but expects them to be specified in utf-8.\n +\tgit-p4 can use three different decoding strategies in handling the\n -+\tencoding uncertainty in Perforce: 'legacy' simply passes the original\n -+\tbytes through from Perforce to git, creating usable but\n ++\tencoding uncertainty in Perforce: 'passthrough' simply passes the\n ++\toriginal bytes through from Perforce to git, creating usable but\n +\tincorrectly-encoded data when the Perforce data is encoded as\n +\tanything other than utf-8. 'strict' expects the Perforce data to be\n +\tencoded as utf-8, and fails to import when this is not true.\n +\t'fallback' attempts to interpret the data as utf-8, and otherwise\n +\tfalls back to using a secondary encoding - by default the common\n -+\twindows encoding 'cp-1252' - with any remaining unparseable bytes\n -+\treplaced with a placeholder character.\n -+\tUnder python2 the default strategy is 'legacy' for historical\n ++\twindows encoding 'cp-1252' - with upper-range bytes escaped if\n ++\tdecoding with the fallback encoding also fails.\n ++\tUnder python2 the default strategy is 'passthrough' for historical\n +\treasons, and under python3 the default is 'fallback'.\n +\tWhen 'strict' is selected and decoding fails, the error message will\n +\tpropose changing this config parameter as a workaround. If this\n @@ Documentation/git-p4.txt: git-p4.pathEncoding::\n \tSpecify the system that is used for large (binary) files. Please note\n \n ## git-p4.py ##\n +@@\n + # pylint: disable=too-many-statements,too-many-instance-attributes\n + # pylint: disable=too-many-branches,too-many-nested-blocks\n + #\n ++import struct\n + import sys\n + if sys.version_info.major < 3 and sys.version_info.minor < 7:\n + sys.stderr.write(\"git-p4: requires Python 2.7 or later.\\n\")\n @@ git-p4.py: defaultLabelRegexp = r'[a-zA-Z0-9_\\-.]+$'\n # The block size is reduced automatically if required\n defaultBlockSize = 1<<20\n \n -+defaultMetadataDecodingStrategy = 'legacy' if sys.version_info.major == 2 else 'fallback'\n ++defaultMetadataDecodingStrategy = 'passthrough' if sys.version_info.major == 2 else 'fallback'\n +defaultFallbackMetadataEncoding = 'cp1252'\n +\n p4_access_checked = False\n @@ git-p4.py: else:\n +defaulting to cp1252.\"\"\".format(self.input_string)\n +\n +\n ++encoding_fallback_warning_issued = False\n ++encoding_escape_warning_issued = False\n +def metadata_stream_to_writable_bytes(s):\n + encodingStrategy = gitConfig('git-p4.metadataDecodingStrategy') or defaultMetadataDecodingStrategy\n + fallbackEncoding = gitConfig('git-p4.metadataFallbackEncoding') or defaultFallbackMetadataEncoding\n + if not isinstance(s, bytes):\n + return s.encode('utf_8')\n -+ if encodingStrategy == 'legacy':\n ++ if encodingStrategy == 'passthrough':\n + return s\n + try:\n + s.decode('utf_8')\n + return s\n + except UnicodeDecodeError:\n + if encodingStrategy == 'fallback' and fallbackEncoding:\n -+ return s.decode(fallbackEncoding, errors='replace').encode('utf_8')\n ++ global encoding_fallback_warning_issued\n ++ global encoding_escape_warning_issued\n ++ try:\n ++ if not encoding_fallback_warning_issued:\n ++ print(\"\\nCould not decode value as utf-8; using configured fallback encoding %s: %s\" % (fallbackEncoding, s))\n ++ print(\"\\n(this warning is only displayed once during an import)\")\n ++ encoding_fallback_warning_issued = True\n ++ return s.decode(fallbackEncoding).encode('utf_8')\n ++ except Exception as exc:\n ++ if not encoding_escape_warning_issued:\n ++ print(\"\\nCould not decode value with configured fallback encoding %s; escaping bytes over 127: %s\" % (fallbackEncoding, s))\n ++ print(\"\\n(this warning is only displayed once during an import)\")\n ++ encoding_escape_warning_issued = True\n ++ escaped_bytes = b''\n ++ # bytes and strings work very differently in python2 vs python3...\n ++ if str is bytes:\n ++ for byte in s:\n ++ byte_number = struct.unpack('>B', byte)[0]\n ++ if byte_number > 127:\n ++ escaped_bytes += b'%'\n ++ escaped_bytes += hex(byte_number)[2:].upper()\n ++ else:\n ++ escaped_bytes += byte\n ++ else:\n ++ for byte_number in s:\n ++ if byte_number > 127:\n ++ escaped_bytes += b'%'\n ++ escaped_bytes += hex(byte_number).upper().encode()[2:]\n ++ else:\n ++ escaped_bytes += bytes([byte_number])\n ++ return escaped_bytes\n ++\n + raise MetadataDecodingException(s)\n +\n def decode_path(path):\n @@ git-p4.py: class P4UserMap:\n + # \"FullName\" is bytes. \"Email\" on the other hand might be bytes\n + # or unicode string depending on whether we are running under\n + # python2 or python3. To support\n -+ # git-p4.metadataDecodingStrategy=legacy, self.users dict values\n ++ # git-p4.metadataDecodingStrategy=fallback, self.users dict values\n + # are always bytes, ready to be written to git.\n + emailbytes = metadata_stream_to_writable_bytes(output[\"Email\"])\n + self.users[output[\"User\"]] = output[\"FullName\"] + b\" <\" + emailbytes + b\">\"\n @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n +\t\ttouch file3 &&\n +\t\tp4 add file3 &&\n +\t\tp4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n -+\t\t iconv -f utf8 -t cp1252)\"\n ++\t\t iconv -f utf8 -t cp1252)\" &&\n ++\n ++\t\tp4_add_user \"cp850_author\" \"$(echo Åuthor |\n ++\t\t\ticonv -f utf8 -t cp850)\" &&\n ++\t\tP4USER=cp850_author &&\n ++\t\ttouch file4 &&\n ++\t\tp4 add file4 &&\n ++\t\tp4 submit -d \"$(echo fourth CL hÅs some cp850 text |\n ++\t\t\ticonv -f utf8 -t cp850)\"\n +\t)\n +'\n +\n @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n +\tgrep \"Decoding perforce metadata failed!\" err\n +'\n +\n -+test_expect_success 'check utf-8 contents with legacy strategy' '\n ++test_expect_success 'check utf-8 contents with passthrough strategy' '\n +\ttest_when_finished cleanup_git &&\n +\ttest_when_finished remove_user_cache &&\n -+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n ++\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n +\t(\n +\t\tcd \"$git\" &&\n +\t\tgit log >actual &&\n @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n +\t)\n +'\n +\n -+test_expect_success 'check latin-1 contents corrupted in git with legacy strategy' '\n ++test_expect_success 'check latin-1 contents corrupted in git with passthrough strategy' '\n +\ttest_when_finished cleanup_git &&\n +\ttest_when_finished remove_user_cache &&\n -+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n ++\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n +\t(\n +\t\tcd \"$git\" &&\n +\t\tgit log >actual &&\n @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n +\t)\n +'\n +\n ++test_expect_success 'check cp850 contents parsed with correct fallback' '\n ++\ttest_when_finished cleanup_git &&\n ++\ttest_when_finished remove_user_cache &&\n ++\tgit -c git-p4.metadataDecodingStrategy=fallback -c git-p4.metadataFallbackEncoding=cp850 p4.py clone --dest=\"$git\" //depot@all &&\n ++\t(\n ++\t\tcd \"$git\" &&\n ++\t\tgit log >actual &&\n ++\t\tgrep \"hÅs some cp850 text\" actual &&\n ++\t\tgrep \"Åuthor\" actual\n ++\t)\n ++'\n ++\n ++test_expect_success 'check cp850-only contents escaped when cp1252 is fallback' '\n ++\ttest_when_finished cleanup_git &&\n ++\ttest_when_finished remove_user_cache &&\n ++\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n ++\t(\n ++\t\tcd \"$git\" &&\n ++\t\tgit log >actual &&\n ++\t\tgrep \"h%8Fs some cp850 text\" actual &&\n ++\t\tgrep \"%8Futhor\" actual\n ++\t)\n ++'\n ++\n +test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n +\ttest_when_finished cleanup_git &&\n +\ttest_when_finished remove_user_cache &&\n @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n +\t(\n +\t\tcd \"$cli\" &&\n +\t\tP4USER=cp1252_author &&\n -+\t\ttouch file4 &&\n -+\t\tp4 add file4 &&\n -+\t\tp4 submit -d \"$(echo fourth CL has sœme more cp-1252 tæxt |\n ++\t\ttouch file10 &&\n ++\t\tp4 add file10 &&\n ++\t\tp4 submit -d \"$(echo later CL has sœme more cp-1252 tæxt |\n +\t\t\ticonv -f utf8 -t cp1252)\"\n +\t) &&\n +\t(\n @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n +\t\tgit p4.py sync --branch=master &&\n +\n +\t\tgit log p4/master >actual &&\n -+\t\tcat actual &&\n +\t\tgrep \"sœme more cp-1252 tæxt\" actual &&\n +\t\tgrep \"æuthœr\" actual\n +\t)\n @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n +## / END REPEATED SECTION ##\n +############################\n +\n -+test_expect_success 'legacy (latin-1 contents corrupted in git) is the default with python2' '\n ++test_expect_success 'passthrough (latin-1 contents corrupted in git) is the default with python2' '\n +\ttest_when_finished cleanup_git &&\n +\ttest_when_finished remove_user_cache &&\n -+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n ++\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n +\t(\n +\t\tcd \"$git\" &&\n +\t\tgit log >actual &&\n @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n +\t\ttouch file3 &&\n +\t\tp4 add file3 &&\n +\t\tp4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n -+\t\t iconv -f utf8 -t cp1252)\"\n ++\t\t iconv -f utf8 -t cp1252)\" &&\n ++\n ++\t\tp4_add_user \"cp850_author\" \"$(echo Åuthor |\n ++\t\t\ticonv -f utf8 -t cp850)\" &&\n ++\t\tP4USER=cp850_author &&\n ++\t\ttouch file4 &&\n ++\t\tp4 add file4 &&\n ++\t\tp4 submit -d \"$(echo fourth CL hÅs some cp850 text |\n ++\t\t\ticonv -f utf8 -t cp850)\"\n +\t)\n +'\n +\n @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n +\tgrep \"Decoding perforce metadata failed!\" err\n +'\n +\n -+test_expect_success 'check utf-8 contents with legacy strategy' '\n ++test_expect_success 'check utf-8 contents with passthrough strategy' '\n +\ttest_when_finished cleanup_git &&\n +\ttest_when_finished remove_user_cache &&\n -+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n ++\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n +\t(\n +\t\tcd \"$git\" &&\n +\t\tgit log >actual &&\n @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n +\t)\n +'\n +\n -+test_expect_success 'check latin-1 contents corrupted in git with legacy strategy' '\n ++test_expect_success 'check latin-1 contents corrupted in git with passthrough strategy' '\n +\ttest_when_finished cleanup_git &&\n +\ttest_when_finished remove_user_cache &&\n -+\tgit -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n ++\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n +\t(\n +\t\tcd \"$git\" &&\n +\t\tgit log >actual &&\n @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n +\t)\n +'\n +\n ++test_expect_success 'check cp850 contents parsed with correct fallback' '\n ++\ttest_when_finished cleanup_git &&\n ++\ttest_when_finished remove_user_cache &&\n ++\tgit -c git-p4.metadataDecodingStrategy=fallback -c git-p4.metadataFallbackEncoding=cp850 p4.py clone --dest=\"$git\" //depot@all &&\n ++\t(\n ++\t\tcd \"$git\" &&\n ++\t\tgit log >actual &&\n ++\t\tgrep \"hÅs some cp850 text\" actual &&\n ++\t\tgrep \"Åuthor\" actual\n ++\t)\n ++'\n ++\n ++test_expect_success 'check cp850-only contents escaped when cp1252 is fallback' '\n ++\ttest_when_finished cleanup_git &&\n ++\ttest_when_finished remove_user_cache &&\n ++\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n ++\t(\n ++\t\tcd \"$git\" &&\n ++\t\tgit log >actual &&\n ++\t\tgrep \"h%8Fs some cp850 text\" actual &&\n ++\t\tgrep \"%8Futhor\" actual\n ++\t)\n ++'\n ++\n +test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n +\ttest_when_finished cleanup_git &&\n +\ttest_when_finished remove_user_cache &&\n @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n +\t(\n +\t\tcd \"$cli\" &&\n +\t\tP4USER=cp1252_author &&\n -+\t\ttouch file4 &&\n -+\t\tp4 add file4 &&\n -+\t\tp4 submit -d \"$(echo fourth CL has sœme more cp-1252 tæxt |\n ++\t\ttouch file10 &&\n ++\t\tp4 add file10 &&\n ++\t\tp4 submit -d \"$(echo later CL has sœme more cp-1252 tæxt |\n +\t\t\ticonv -f utf8 -t cp1252)\"\n +\t) &&\n +\t(\n @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n +\t\tgit p4.py sync --branch=master &&\n +\n +\t\tgit log p4/master >actual &&\n -+\t\tcat actual &&\n +\t\tgrep \"sœme more cp-1252 tæxt\" actual &&\n +\t\tgrep \"æuthœr\" actual\n +\t)\n\n\n Documentation/git-p4.txt | 37 +++-\n git-p4.py | 123 +++++++++--\n t/lib-git-p4.sh | 3 +-\n t/t9835-git-p4-metadata-encoding-python2.sh | 213 +++++++++++++++++++\n t/t9836-git-p4-metadata-encoding-python3.sh | 214 ++++++++++++++++++++\n 5 files changed, 572 insertions(+), 18 deletions(-)\n create mode 100755 t/t9835-git-p4-metadata-encoding-python2.sh\n create mode 100755 t/t9836-git-p4-metadata-encoding-python3.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex e21fcd8f712..de5ee6748e3 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -636,7 +636,42 @@ git-p4.pathEncoding::\n \tGit expects paths encoded as UTF-8. Use this config to tell git-p4\n \twhat encoding Perforce had used for the paths. This encoding is used\n \tto transcode the paths to UTF-8. As an example, Perforce on Windows\n-\toften uses \"cp1252\" to encode path names.\n+\toften uses \"cp1252\" to encode path names. If this option is passed\n+\tinto a p4 clone request, it is persisted in the resulting new git\n+\trepo.\n+\n+git-p4.metadataDecodingStrategy::\n+\tPerforce keeps the encoding of a changelist descriptions and user\n+\tfull names as stored by the client on a given OS. The p4v client\n+\tuses the OS-local encoding, and so different users can end up storing\n+\tdifferent changelist descriptions or user full names in different\n+\tencodings, in the same depot.\n+\tGit tolerates inconsistent/incorrect encodings in commit messages\n+\tand author names, but expects them to be specified in utf-8.\n+\tgit-p4 can use three different decoding strategies in handling the\n+\tencoding uncertainty in Perforce: 'passthrough' simply passes the\n+\toriginal bytes through from Perforce to git, creating usable but\n+\tincorrectly-encoded data when the Perforce data is encoded as\n+\tanything other than utf-8. 'strict' expects the Perforce data to be\n+\tencoded as utf-8, and fails to import when this is not true.\n+\t'fallback' attempts to interpret the data as utf-8, and otherwise\n+\tfalls back to using a secondary encoding - by default the common\n+\twindows encoding 'cp-1252' - with upper-range bytes escaped if\n+\tdecoding with the fallback encoding also fails.\n+\tUnder python2 the default strategy is 'passthrough' for historical\n+\treasons, and under python3 the default is 'fallback'.\n+\tWhen 'strict' is selected and decoding fails, the error message will\n+\tpropose changing this config parameter as a workaround. If this\n+\toption is passed into a p4 clone request, it is persisted into the\n+\tresulting new git repo.\n+\n+git-p4.metadataFallbackEncoding::\n+\tSpecify the fallback encoding to use when decoding Perforce author\n+\tnames and changelists descriptions using the 'fallback' strategy\n+\t(see git-p4.metadataDecodingStrategy). The fallback encoding will\n+\tonly be used when decoding as utf-8 fails. This option defaults to\n+\tcp1252, a common windows encoding. If this option is passed into a\n+\tp4 clone request, it is persisted into the resulting new git repo.\n \n git-p4.largeFileSystem::\n \tSpecify the system that is used for large (binary) files. Please note\ndiff --git a/git-p4.py b/git-p4.py\nindex a9b1f904410..d24c3535f8a 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -15,6 +15,7 @@\n # pylint: disable=too-many-statements,too-many-instance-attributes\n # pylint: disable=too-many-branches,too-many-nested-blocks\n #\n+import struct\n import sys\n if sys.version_info.major < 3 and sys.version_info.minor < 7:\n sys.stderr.write(\"git-p4: requires Python 2.7 or later.\\n\")\n@@ -54,6 +55,9 @@ defaultLabelRegexp = r'[a-zA-Z0-9_\\-.]+$'\n # The block size is reduced automatically if required\n defaultBlockSize = 1<<20\n \n+defaultMetadataDecodingStrategy = 'passthrough' if sys.version_info.major == 2 else 'fallback'\n+defaultFallbackMetadataEncoding = 'cp1252'\n+\n p4_access_checked = False\n \n re_ko_keywords = re.compile(br'\\$(Id|Header)(:[^$\\n]+)?\\$')\n@@ -203,6 +207,70 @@ else:\n def encode_text_stream(s):\n return s.encode('utf_8') if isinstance(s, unicode) else s\n \n+\n+class MetadataDecodingException(Exception):\n+ def __init__(self, input_string):\n+ self.input_string = input_string\n+\n+ def __str__(self):\n+ return \"\"\"Decoding perforce metadata failed!\n+The failing string was:\n+---\n+{}\n+---\n+Consider setting the git-p4.metadataDecodingStrategy config option to\n+'fallback', to allow metadata to be decoded using a fallback encoding,\n+defaulting to cp1252.\"\"\".format(self.input_string)\n+\n+\n+encoding_fallback_warning_issued = False\n+encoding_escape_warning_issued = False\n+def metadata_stream_to_writable_bytes(s):\n+ encodingStrategy = gitConfig('git-p4.metadataDecodingStrategy') or defaultMetadataDecodingStrategy\n+ fallbackEncoding = gitConfig('git-p4.metadataFallbackEncoding') or defaultFallbackMetadataEncoding\n+ if not isinstance(s, bytes):\n+ return s.encode('utf_8')\n+ if encodingStrategy == 'passthrough':\n+ return s\n+ try:\n+ s.decode('utf_8')\n+ return s\n+ except UnicodeDecodeError:\n+ if encodingStrategy == 'fallback' and fallbackEncoding:\n+ global encoding_fallback_warning_issued\n+ global encoding_escape_warning_issued\n+ try:\n+ if not encoding_fallback_warning_issued:\n+ print(\"\\nCould not decode value as utf-8; using configured fallback encoding %s: %s\" % (fallbackEncoding, s))\n+ print(\"\\n(this warning is only displayed once during an import)\")\n+ encoding_fallback_warning_issued = True\n+ return s.decode(fallbackEncoding).encode('utf_8')\n+ except Exception as exc:\n+ if not encoding_escape_warning_issued:\n+ print(\"\\nCould not decode value with configured fallback encoding %s; escaping bytes over 127: %s\" % (fallbackEncoding, s))\n+ print(\"\\n(this warning is only displayed once during an import)\")\n+ encoding_escape_warning_issued = True\n+ escaped_bytes = b''\n+ # bytes and strings work very differently in python2 vs python3...\n+ if str is bytes:\n+ for byte in s:\n+ byte_number = struct.unpack('>B', byte)[0]\n+ if byte_number > 127:\n+ escaped_bytes += b'%'\n+ escaped_bytes += hex(byte_number)[2:].upper()\n+ else:\n+ escaped_bytes += byte\n+ else:\n+ for byte_number in s:\n+ if byte_number > 127:\n+ escaped_bytes += b'%'\n+ escaped_bytes += hex(byte_number).upper().encode()[2:]\n+ else:\n+ escaped_bytes += bytes([byte_number])\n+ return escaped_bytes\n+\n+ raise MetadataDecodingException(s)\n+\n def decode_path(path):\n \"\"\"Decode a given string (bytes or otherwise) using configured path encoding options\n \"\"\"\n@@ -702,11 +770,12 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\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+ # - `desc` or `FullName` which may contain non-UTF8 encoded text handled below, eagerly converted to bytes\n+ # - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text, handled by decode_path()\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 ('data', 'desc', 'FullName', 'path', 'clientFile') or key.startswith('depotFile')):\n value = value.decode()\n decoded_entry[key] = value\n # Parse out data if it's an error response\n@@ -716,6 +785,10 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n if skip_info:\n if 'code' in entry and entry['code'] == 'info':\n continue\n+ if 'desc' in entry:\n+ entry['desc'] = metadata_stream_to_writable_bytes(entry['desc'])\n+ if 'FullName' in entry:\n+ entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName'])\n if cb is not None:\n cb(entry)\n else:\n@@ -1435,7 +1508,13 @@ class P4UserMap:\n for output in p4CmdList([\"users\"]):\n if \"User\" not in output:\n continue\n- self.users[output[\"User\"]] = output[\"FullName\"] + \" <\" + output[\"Email\"] + \">\"\n+ # \"FullName\" is bytes. \"Email\" on the other hand might be bytes\n+ # or unicode string depending on whether we are running under\n+ # python2 or python3. To support\n+ # git-p4.metadataDecodingStrategy=fallback, self.users dict values\n+ # are always bytes, ready to be written to git.\n+ emailbytes = metadata_stream_to_writable_bytes(output[\"Email\"])\n+ self.users[output[\"User\"]] = output[\"FullName\"] + b\" <\" + emailbytes + b\">\"\n self.emails[output[\"Email\"]] = output[\"User\"]\n \n mapUserConfigRegex = re.compile(r\"^\\s*(\\S+)\\s*=\\s*(.+)\\s*<(\\S+)>\\s*$\", re.VERBOSE)\n@@ -1445,26 +1524,28 @@ class P4UserMap:\n user = mapUser[0][0]\n fullname = mapUser[0][1]\n email = mapUser[0][2]\n- self.users[user] = fullname + \" <\" + email + \">\"\n+ fulluser = fullname + \" <\" + email + \">\"\n+ self.users[user] = metadata_stream_to_writable_bytes(fulluser)\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+ keybytes = metadata_stream_to_writable_bytes(key)\n+ s += b\"%s\\t%s\\n\" % (keybytes.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- self.users[entry[0]] = entry[1]\n+ entry = line.strip().split(b\"\\t\")\n+ self.users[entry[0].decode('utf_8')] = entry[1]\n except IOError:\n self.getUserMapFromPerforceServer()\n \n@@ -3020,7 +3101,8 @@ class P4Sync(Command, P4UserMap):\n if userid in self.users:\n return self.users[userid]\n else:\n- return \"%s \" % userid\n+ userid_bytes = metadata_stream_to_writable_bytes(userid)\n+ return b\"%s \" % userid_bytes\n \n def streamTag(self, gitStream, labelName, labelDetails, commit, epoch):\n \"\"\" Stream a p4 tag.\n@@ -3043,9 +3125,10 @@ class P4Sync(Command, P4UserMap):\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 \n- gitStream.write(\"tagger %s\\n\" % tagger)\n+ gitStream.write(\"tagger \")\n+ gitStream.write(email)\n+ gitStream.write(\" %s %s\\n\" % (epoch, self.tz))\n \n print(\"labelDetails=\",labelDetails)\n if 'Description' in labelDetails:\n@@ -3138,12 +3221,12 @@ class P4Sync(Command, P4UserMap):\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 \n- self.gitStream.write(\"committer %s\\n\" % committer)\n+ self.gitStream.write(\"committer \")\n+ self.gitStream.write(self.make_email(author))\n+ self.gitStream.write(\" %s %s\\n\" % (epoch, self.tz))\n \n self.gitStream.write(\"data <&1 | cut -c 8)\n+python_target_binary=$(which python$python_target_version)\n+if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_binary\"\n+then\n+\tmkdir temp_python\n+\tPATH=\"$(pwd)/temp_python:$PATH\" && export PATH\n+\tln -s $python_target_binary temp_python/python\n+fi\n+\n+python_major_version=$(python -V 2>&1 | cut -c 8)\n+if ! test \"$python_major_version\" = \"$python_target_version\"\n+then\n+\tskip_all=\"skipping python$python_target_version-specific git p4 tests; python$python_target_version not available\"\n+\ttest_done\n+fi\n+\n+remove_user_cache () {\n+\trm \"$HOME/.gitp4-usercache.txt\" || true\n+}\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'init depot' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\tp4_add_user \"utf8_author\" \"ǣuthor\" &&\n+\t\tP4USER=utf8_author &&\n+\t\ttouch file1 &&\n+\t\tp4 add file1 &&\n+\t\tp4 submit -d \"first CL has some utf-8 tǣxt\" &&\n+\n+\t\tp4_add_user \"latin1_author\" \"$(echo æuthor |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\t\tP4USER=latin1_author &&\n+\t\ttouch file2 &&\n+\t\tp4 add file2 &&\n+\t\tp4 submit -d \"$(echo second CL has some latin-1 tæxt |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\n+\t\tp4_add_user \"cp1252_author\" \"$(echo æuthœr |\n+\t\t\ticonv -f utf8 -t cp1252)\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file3 &&\n+\t\tp4 add file3 &&\n+\t\tp4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n+\t\t iconv -f utf8 -t cp1252)\" &&\n+\n+\t\tp4_add_user \"cp850_author\" \"$(echo Åuthor |\n+\t\t\ticonv -f utf8 -t cp850)\" &&\n+\t\tP4USER=cp850_author &&\n+\t\ttouch file4 &&\n+\t\tp4 add file4 &&\n+\t\tp4 submit -d \"$(echo fourth CL hÅs some cp850 text |\n+\t\t\ticonv -f utf8 -t cp850)\"\n+\t)\n+'\n+\n+test_expect_success 'clone non-utf8 repo with strict encoding' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\ttest_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest=\"$git\" //depot@all 2>err &&\n+\tgrep \"Decoding perforce metadata failed!\" err\n+'\n+\n+test_expect_success 'check utf-8 contents with passthrough strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents corrupted in git with passthrough strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tbadly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n+\t\tgrep \"$badly_encoded_in_git\" actual &&\n+\t\tbad_author_in_git=\"$(echo æuthor | iconv -f utf8 -t latin1)\" &&\n+\t\tgrep \"$bad_author_in_git\" actual\n+\t)\n+'\n+\n+test_expect_success 'check utf-8 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some latin-1 tæxt\" actual &&\n+\t\tgrep \"æuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"sœme cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp850 contents parsed with correct fallback' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback -c git-p4.metadataFallbackEncoding=cp850 p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"hÅs some cp850 text\" actual &&\n+\t\tgrep \"Åuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp850-only contents escaped when cp1252 is fallback' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"h%8Fs some cp850 text\" actual &&\n+\t\tgrep \"%8Futhor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file10 &&\n+\t\tp4 add file10 &&\n+\t\tp4 submit -d \"$(echo later CL has sœme more cp-1252 tæxt |\n+\t\t\ticonv -f utf8 -t cp1252)\"\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\n+\t\tgit p4.py sync --branch=master &&\n+\n+\t\tgit log p4/master >actual &&\n+\t\tgrep \"sœme more cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+############################\n+## / END REPEATED SECTION ##\n+############################\n+\n+test_expect_success 'passthrough (latin-1 contents corrupted in git) is the default with python2' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tbadly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n+\t\tgrep \"$badly_encoded_in_git\" actual\n+\t)\n+'\n+\n+test_done\ndiff --git a/t/t9836-git-p4-metadata-encoding-python3.sh b/t/t9836-git-p4-metadata-encoding-python3.sh\nnew file mode 100755\nindex 00000000000..63350dc4b5c\n--- /dev/null\n+++ b/t/t9836-git-p4-metadata-encoding-python3.sh\n@@ -0,0 +1,214 @@\n+#!/bin/sh\n+\n+test_description='git p4 metadata encoding\n+\n+This test checks that the import process handles inconsistent text\n+encoding in p4 metadata (author names, commit messages, etc) without\n+failing, and produces maximally sane output in git.'\n+\n+. ./lib-git-p4.sh\n+\n+python_target_version='3'\n+\n+###############################\n+## SECTION REPEATED IN t9835 ##\n+###############################\n+\n+# Please note: this test calls \"git-p4.py\" rather than \"git-p4\", because the\n+# latter references a specific path so we can't easily force it to run under\n+# the python version we need to.\n+\n+python_major_version=$(python -V 2>&1 | cut -c 8)\n+python_target_binary=$(which python$python_target_version)\n+if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_binary\"\n+then\n+\tmkdir temp_python\n+\tPATH=\"$(pwd)/temp_python:$PATH\" && export PATH\n+\tln -s $python_target_binary temp_python/python\n+fi\n+\n+python_major_version=$(python -V 2>&1 | cut -c 8)\n+if ! test \"$python_major_version\" = \"$python_target_version\"\n+then\n+\tskip_all=\"skipping python$python_target_version-specific git p4 tests; python$python_target_version not available\"\n+\ttest_done\n+fi\n+\n+remove_user_cache () {\n+\trm \"$HOME/.gitp4-usercache.txt\" || true\n+}\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'init depot' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\tp4_add_user \"utf8_author\" \"ǣuthor\" &&\n+\t\tP4USER=utf8_author &&\n+\t\ttouch file1 &&\n+\t\tp4 add file1 &&\n+\t\tp4 submit -d \"first CL has some utf-8 tǣxt\" &&\n+\n+\t\tp4_add_user \"latin1_author\" \"$(echo æuthor |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\t\tP4USER=latin1_author &&\n+\t\ttouch file2 &&\n+\t\tp4 add file2 &&\n+\t\tp4 submit -d \"$(echo second CL has some latin-1 tæxt |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\n+\t\tp4_add_user \"cp1252_author\" \"$(echo æuthœr |\n+\t\t\ticonv -f utf8 -t cp1252)\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file3 &&\n+\t\tp4 add file3 &&\n+\t\tp4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n+\t\t iconv -f utf8 -t cp1252)\" &&\n+\n+\t\tp4_add_user \"cp850_author\" \"$(echo Åuthor |\n+\t\t\ticonv -f utf8 -t cp850)\" &&\n+\t\tP4USER=cp850_author &&\n+\t\ttouch file4 &&\n+\t\tp4 add file4 &&\n+\t\tp4 submit -d \"$(echo fourth CL hÅs some cp850 text |\n+\t\t\ticonv -f utf8 -t cp850)\"\n+\t)\n+'\n+\n+test_expect_success 'clone non-utf8 repo with strict encoding' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\ttest_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest=\"$git\" //depot@all 2>err &&\n+\tgrep \"Decoding perforce metadata failed!\" err\n+'\n+\n+test_expect_success 'check utf-8 contents with passthrough strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents corrupted in git with passthrough strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tbadly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n+\t\tgrep \"$badly_encoded_in_git\" actual &&\n+\t\tbad_author_in_git=\"$(echo æuthor | iconv -f utf8 -t latin1)\" &&\n+\t\tgrep \"$bad_author_in_git\" actual\n+\t)\n+'\n+\n+test_expect_success 'check utf-8 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some latin-1 tæxt\" actual &&\n+\t\tgrep \"æuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"sœme cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp850 contents parsed with correct fallback' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback -c git-p4.metadataFallbackEncoding=cp850 p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"hÅs some cp850 text\" actual &&\n+\t\tgrep \"Åuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp850-only contents escaped when cp1252 is fallback' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"h%8Fs some cp850 text\" actual &&\n+\t\tgrep \"%8Futhor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file10 &&\n+\t\tp4 add file10 &&\n+\t\tp4 submit -d \"$(echo later CL has sœme more cp-1252 tæxt |\n+\t\t\ticonv -f utf8 -t cp1252)\"\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\n+\t\tgit p4.py sync --branch=master &&\n+\n+\t\tgit log p4/master >actual &&\n+\t\tgrep \"sœme more cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+############################\n+## / END REPEATED SECTION ##\n+############################\n+\n+\n+test_expect_success 'fallback (both utf-8 and cp-1252 contents handled) is the default with python3' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"sœme cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+test_done\n\nbase-commit: 11cfe552610386954886543f5de87dcc49ad5735\n-- \ngitgitgadget\n"},{"id":"453940","messageId":"CAPMMpojwo0BG45BNN6urAgd-yt1BPXKPmH6q8zza+v9BDaXKng@mail.gmail.com","threadId":"57709","inReplyTo":"80e83d8e-1f68-16be-6d68-fbc4aadfc78d@adoakley.name","subject":"Re: [PATCH v2] [RFC] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-04-19T20:30:10Z","receivedAt":"2022-04-19T20:30:26Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Sun, Apr 17, 2022 at 8:17 PM Andrew Oakley wrote:\n>\n>\n> The way I look at it is that you both read and write bytes, and you may\n> attempt to decode and re-encode text on the way. Both the decoding and\n> the encoding are done in metadata_stream_to_writable_bytes, so nothing\n> else needs to know about the raw option being different.\n>\n\nRight - personally I just believe making the distinction explicit as\n\"strategies\" makes for a less magical explanation than a special\nencoding value that's not just a different encoding but also a\ndifferent behavior.\n\nIn other aspects, the behavior you're proposing (except for the final\nfallback-decoding-failure) seems to be equivalent to what I've\nimplemented in the latest version.\n\n>\n> > I understand and share the data loss concern.\n> >\n> > As I just answered Ævar, I *think* I'd like to address the data loss\n> > concern by escaping all x80+ bytes if something cannot be interpreted\n> > even using the fallback encoding. In a commit message there could also\n> > be a suffix explaining what happened, although I suspect that's\n> > pointless complexity. The advantage of this approach is that it makes\n> > it *possible* to reconstruct the original bytestream precisely, but\n> > without creating badly-encoded git commit messages that need to be\n> > skirted around.\n>\n> I think this gets pretty messy though. In my opinion it's not any nicer\n> than putting the raw bytes in the commit message.\n>\n> Git does not make any attempt enforce the commit metadata encoding, so I\n> think that tools really should make an attempt to handle invalid data in\n> a somewhat sensible fashion.\n>\n> I don't think there is really a \"right\" answer, anything reasonable\n> would be better than what we've got now.\n\nAlright - I went ahead with the \"escape if you can't do it right\"\nbehavior anyway, because it makes me feel better about being able to\nsay \"no information loss\" :)\n"},{"id":"453941","messageId":"CAPMMpogSt_Soih=DvMgb71nPxo-jkiVS5XF3iw40vkuo2W+8Sg@mail.gmail.com","threadId":"57709","inReplyTo":"pull.1206.v3.git.1650399590844.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-04-19T20:33:27Z","receivedAt":"2022-04-19T20:33:46Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"My apologies, I messed up the cover letter, *and* the subject line,\nand now GitGitGadget won't let me submit the same commit again...\n\nThe subject line *should* have read: Git p4 encoding strategy\n\n\nThe cover letter (after \"---\") should have read:\n---\nThis is no longer RFC, it's now request-for-Review!\n\nChanges wrt v2:\n * Renamed \"legacy\" strategy to \"passthrough\", reflecting the possible\nvalue of maintaining it long-term\n * Changed \"fallback decoding failure\" behavior to escape over-127\nbytes, instead of omitting them. There should now be no information\nloss under any scenario, although recovering the original bytes might\nbe non-trivial\n\nOn Tue, Apr 19, 2022 at 10:19 PM Tao Klerks via GitGitGadget\n wrote:\n>\n> From: Tao Klerks \n>\n> git-p4 is designed to run correctly under python2.7 and python3, but\n> its functional behavior wrt importing user-entered text differs across\n> these environments:\n>\n> Under python2, git-p4 \"naively\" writes the Perforce bytestream into git\n> metadata (and does not set an \"encoding\" header on the commits); this\n> means that any non-utf-8 byte sequences end up creating invalidly-encoded\n> commit metadata in git.\n>\n> Under python3, git-p4 attempts to decode the Perforce bytestream as utf-8\n> data, and fails badly (with an unhelpful error) when non-utf-8 data is\n> encountered.\n>\n> Perforce clients (especially p4v) encourage user entry of changelist\n> descriptions (and user full names) in OS-local encoding, and store the\n> resulting bytestream to the server unmodified - such that different\n> clients can end up creating mutually-unintelligible messages. The most\n> common inconsistency, in many Perforce environments, is likely to be utf-8\n> (typical in linux) vs cp-1252 (typical in windows).\n>\n> Make the changelist-description- and user-fullname-handling code\n> python-runtime-agnostic, introducing three \"strategies\" selectable via\n> config:\n> - 'passthrough', behaving as previously under python2,\n> - 'strict', behaving as previously under python3, and\n> - 'fallback', favoring utf-8 but supporting a secondary encoding when\n> utf-8 decoding fails, and finally escaping high-range bytes if the\n> decoding with the secondary encoding also fails.\n>\n> Keep the python2 default behavior as-is ('legacy' strategy), but switch\n> the python3 default strategy to 'fallback' with default fallback encoding\n> 'cp1252'.\n>\n> Also include tests exercising these encoding strategies, documentation for\n> the new config, and improve the user-facing error messages when decoding\n> does fail.\n>\n> Signed-off-by: Tao Klerks \n> ---\n> RFC: Git p4 encoding strategy\n>\n> OPEN QUESTIONS:\n>\n> * Does it make sense to make \"fallback\" the default decoding strategy\n> in python3? This is definitely a change in behavior, but I believe\n> for the better; failing with \"we defaulted to strict, but you can run\n> again with this other option if you want it to work\" seems unkind,\n> only making sense if we thought fallback to cp1252 would be wrong in\n> a substantial proportion of cases...\n> * Is it OK to duplicate the bulk of the testing code across\n> t9835-git-p4-metadata-encoding-python2.sh and\n> t9836-git-p4-metadata-encoding-python3.sh?\n> * Is it OK to explicitly call \"git-p4.py\" in tests, rather than the\n> build output \"git-p4\", in order to be able to select the python\n> runtime on a per-test basis? Is there a better approach?\n> * Is the naming of the strategies appropriate? Should the default\n> python2 strategy be called something less opinionated, like\n> \"passthrough\"?\n>\n> Changes wrt v1:\n>\n> * Added \"and replace any remaining unmappable bytes\" behavior to the\n> \"fallback\" strategy; common reasonable encodings like cp1252 still\n> contain unmapped codepoints, and if those are found, there is really\n> nothing that can be done about it other than ignoring the crazy\n> bytes; this approach is consistent with the longstanding\n> path-encoding-handling strategy.\n> * Simplified error-handling accordingly\n> * Cleaned up tests & commit messages slightly\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1206%2FTaoK%2Fgit-p4-encoding-strategy-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1206/TaoK/git-p4-encoding-strategy-v3\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1206\n>\n> Range-diff vs v2:\n>\n> 1: 6d227ad57ea ! 1: 1d83b6d7865 [RFC] git-p4: improve encoding handling to support inconsistent encodings\n> @@ Metadata\n> Author: Tao Klerks \n>\n> ## Commit message ##\n> - [RFC] git-p4: improve encoding handling to support inconsistent encodings\n> + git-p4: improve encoding handling to support inconsistent encodings\n>\n> git-p4 is designed to run correctly under python2.7 and python3, but\n> its functional behavior wrt importing user-entered text differs across\n> @@ Commit message\n> Make the changelist-description- and user-fullname-handling code\n> python-runtime-agnostic, introducing three \"strategies\" selectable via\n> config:\n> - - 'legacy', behaving as previously under python2,\n> + - 'passthrough', behaving as previously under python2,\n> - 'strict', behaving as previously under python3, and\n> - 'fallback', favoring utf-8 but supporting a secondary encoding when\n> - utf-8 decoding fails, and finally replacing remaining unmappable bytes.\n> + utf-8 decoding fails, and finally escaping high-range bytes if the\n> + decoding with the secondary encoding also fails.\n>\n> Keep the python2 default behavior as-is ('legacy' strategy), but switch\n> - the python3 default strategy to 'fallback' with fallback encoding\n> + the python3 default strategy to 'fallback' with default fallback encoding\n> 'cp1252'.\n>\n> Also include tests exercising these encoding strategies, documentation for\n> @@ Documentation/git-p4.txt: git-p4.pathEncoding::\n> +git-p4.metadataDecodingStrategy::\n> + Perforce keeps the encoding of a changelist descriptions and user\n> + full names as stored by the client on a given OS. The p4v client\n> -+ uses the OS-local encosing, and so different users can end up storing\n> ++ uses the OS-local encoding, and so different users can end up storing\n> + different changelist descriptions or user full names in different\n> + encodings, in the same depot.\n> + Git tolerates inconsistent/incorrect encodings in commit messages\n> + and author names, but expects them to be specified in utf-8.\n> + git-p4 can use three different decoding strategies in handling the\n> -+ encoding uncertainty in Perforce: 'legacy' simply passes the original\n> -+ bytes through from Perforce to git, creating usable but\n> ++ encoding uncertainty in Perforce: 'passthrough' simply passes the\n> ++ original bytes through from Perforce to git, creating usable but\n> + incorrectly-encoded data when the Perforce data is encoded as\n> + anything other than utf-8. 'strict' expects the Perforce data to be\n> + encoded as utf-8, and fails to import when this is not true.\n> + 'fallback' attempts to interpret the data as utf-8, and otherwise\n> + falls back to using a secondary encoding - by default the common\n> -+ windows encoding 'cp-1252' - with any remaining unparseable bytes\n> -+ replaced with a placeholder character.\n> -+ Under python2 the default strategy is 'legacy' for historical\n> ++ windows encoding 'cp-1252' - with upper-range bytes escaped if\n> ++ decoding with the fallback encoding also fails.\n> ++ Under python2 the default strategy is 'passthrough' for historical\n> + reasons, and under python3 the default is 'fallback'.\n> + When 'strict' is selected and decoding fails, the error message will\n> + propose changing this config parameter as a workaround. If this\n> @@ Documentation/git-p4.txt: git-p4.pathEncoding::\n> Specify the system that is used for large (binary) files. Please note\n>\n> ## git-p4.py ##\n> +@@\n> + # pylint: disable=too-many-statements,too-many-instance-attributes\n> + # pylint: disable=too-many-branches,too-many-nested-blocks\n> + #\n> ++import struct\n> + import sys\n> + if sys.version_info.major < 3 and sys.version_info.minor < 7:\n> + sys.stderr.write(\"git-p4: requires Python 2.7 or later.\\n\")\n> @@ git-p4.py: defaultLabelRegexp = r'[a-zA-Z0-9_\\-.]+$'\n> # The block size is reduced automatically if required\n> defaultBlockSize = 1<<20\n>\n> -+defaultMetadataDecodingStrategy = 'legacy' if sys.version_info.major == 2 else 'fallback'\n> ++defaultMetadataDecodingStrategy = 'passthrough' if sys.version_info.major == 2 else 'fallback'\n> +defaultFallbackMetadataEncoding = 'cp1252'\n> +\n> p4_access_checked = False\n> @@ git-p4.py: else:\n> +defaulting to cp1252.\"\"\".format(self.input_string)\n> +\n> +\n> ++encoding_fallback_warning_issued = False\n> ++encoding_escape_warning_issued = False\n> +def metadata_stream_to_writable_bytes(s):\n> + encodingStrategy = gitConfig('git-p4.metadataDecodingStrategy') or defaultMetadataDecodingStrategy\n> + fallbackEncoding = gitConfig('git-p4.metadataFallbackEncoding') or defaultFallbackMetadataEncoding\n> + if not isinstance(s, bytes):\n> + return s.encode('utf_8')\n> -+ if encodingStrategy == 'legacy':\n> ++ if encodingStrategy == 'passthrough':\n> + return s\n> + try:\n> + s.decode('utf_8')\n> + return s\n> + except UnicodeDecodeError:\n> + if encodingStrategy == 'fallback' and fallbackEncoding:\n> -+ return s.decode(fallbackEncoding, errors='replace').encode('utf_8')\n> ++ global encoding_fallback_warning_issued\n> ++ global encoding_escape_warning_issued\n> ++ try:\n> ++ if not encoding_fallback_warning_issued:\n> ++ print(\"\\nCould not decode value as utf-8; using configured fallback encoding %s: %s\" % (fallbackEncoding, s))\n> ++ print(\"\\n(this warning is only displayed once during an import)\")\n> ++ encoding_fallback_warning_issued = True\n> ++ return s.decode(fallbackEncoding).encode('utf_8')\n> ++ except Exception as exc:\n> ++ if not encoding_escape_warning_issued:\n> ++ print(\"\\nCould not decode value with configured fallback encoding %s; escaping bytes over 127: %s\" % (fallbackEncoding, s))\n> ++ print(\"\\n(this warning is only displayed once during an import)\")\n> ++ encoding_escape_warning_issued = True\n> ++ escaped_bytes = b''\n> ++ # bytes and strings work very differently in python2 vs python3...\n> ++ if str is bytes:\n> ++ for byte in s:\n> ++ byte_number = struct.unpack('>B', byte)[0]\n> ++ if byte_number > 127:\n> ++ escaped_bytes += b'%'\n> ++ escaped_bytes += hex(byte_number)[2:].upper()\n> ++ else:\n> ++ escaped_bytes += byte\n> ++ else:\n> ++ for byte_number in s:\n> ++ if byte_number > 127:\n> ++ escaped_bytes += b'%'\n> ++ escaped_bytes += hex(byte_number).upper().encode()[2:]\n> ++ else:\n> ++ escaped_bytes += bytes([byte_number])\n> ++ return escaped_bytes\n> ++\n> + raise MetadataDecodingException(s)\n> +\n> def decode_path(path):\n> @@ git-p4.py: class P4UserMap:\n> + # \"FullName\" is bytes. \"Email\" on the other hand might be bytes\n> + # or unicode string depending on whether we are running under\n> + # python2 or python3. To support\n> -+ # git-p4.metadataDecodingStrategy=legacy, self.users dict values\n> ++ # git-p4.metadataDecodingStrategy=fallback, self.users dict values\n> + # are always bytes, ready to be written to git.\n> + emailbytes = metadata_stream_to_writable_bytes(output[\"Email\"])\n> + self.users[output[\"User\"]] = output[\"FullName\"] + b\" <\" + emailbytes + b\">\"\n> @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n> + touch file3 &&\n> + p4 add file3 &&\n> + p4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n> -+ iconv -f utf8 -t cp1252)\"\n> ++ iconv -f utf8 -t cp1252)\" &&\n> ++\n> ++ p4_add_user \"cp850_author\" \"$(echo Åuthor |\n> ++ iconv -f utf8 -t cp850)\" &&\n> ++ P4USER=cp850_author &&\n> ++ touch file4 &&\n> ++ p4 add file4 &&\n> ++ p4 submit -d \"$(echo fourth CL hÅs some cp850 text |\n> ++ iconv -f utf8 -t cp850)\"\n> + )\n> +'\n> +\n> @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n> + grep \"Decoding perforce metadata failed!\" err\n> +'\n> +\n> -+test_expect_success 'check utf-8 contents with legacy strategy' '\n> ++test_expect_success 'check utf-8 contents with passthrough strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> -+ git -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n> ++ git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n> + )\n> +'\n> +\n> -+test_expect_success 'check latin-1 contents corrupted in git with legacy strategy' '\n> ++test_expect_success 'check latin-1 contents corrupted in git with passthrough strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> -+ git -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n> ++ git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n> + )\n> +'\n> +\n> ++test_expect_success 'check cp850 contents parsed with correct fallback' '\n> ++ test_when_finished cleanup_git &&\n> ++ test_when_finished remove_user_cache &&\n> ++ git -c git-p4.metadataDecodingStrategy=fallback -c git-p4.metadataFallbackEncoding=cp850 p4.py clone --dest=\"$git\" //depot@all &&\n> ++ (\n> ++ cd \"$git\" &&\n> ++ git log >actual &&\n> ++ grep \"hÅs some cp850 text\" actual &&\n> ++ grep \"Åuthor\" actual\n> ++ )\n> ++'\n> ++\n> ++test_expect_success 'check cp850-only contents escaped when cp1252 is fallback' '\n> ++ test_when_finished cleanup_git &&\n> ++ test_when_finished remove_user_cache &&\n> ++ git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n> ++ (\n> ++ cd \"$git\" &&\n> ++ git log >actual &&\n> ++ grep \"h%8Fs some cp850 text\" actual &&\n> ++ grep \"%8Futhor\" actual\n> ++ )\n> ++'\n> ++\n> +test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n> + (\n> + cd \"$cli\" &&\n> + P4USER=cp1252_author &&\n> -+ touch file4 &&\n> -+ p4 add file4 &&\n> -+ p4 submit -d \"$(echo fourth CL has sœme more cp-1252 tæxt |\n> ++ touch file10 &&\n> ++ p4 add file10 &&\n> ++ p4 submit -d \"$(echo later CL has sœme more cp-1252 tæxt |\n> + iconv -f utf8 -t cp1252)\"\n> + ) &&\n> + (\n> @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n> + git p4.py sync --branch=master &&\n> +\n> + git log p4/master >actual &&\n> -+ cat actual &&\n> + grep \"sœme more cp-1252 tæxt\" actual &&\n> + grep \"æuthœr\" actual\n> + )\n> @@ t/t9835-git-p4-metadata-encoding-python2.sh (new)\n> +## / END REPEATED SECTION ##\n> +############################\n> +\n> -+test_expect_success 'legacy (latin-1 contents corrupted in git) is the default with python2' '\n> ++test_expect_success 'passthrough (latin-1 contents corrupted in git) is the default with python2' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> -+ git -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n> ++ git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n> + touch file3 &&\n> + p4 add file3 &&\n> + p4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n> -+ iconv -f utf8 -t cp1252)\"\n> ++ iconv -f utf8 -t cp1252)\" &&\n> ++\n> ++ p4_add_user \"cp850_author\" \"$(echo Åuthor |\n> ++ iconv -f utf8 -t cp850)\" &&\n> ++ P4USER=cp850_author &&\n> ++ touch file4 &&\n> ++ p4 add file4 &&\n> ++ p4 submit -d \"$(echo fourth CL hÅs some cp850 text |\n> ++ iconv -f utf8 -t cp850)\"\n> + )\n> +'\n> +\n> @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n> + grep \"Decoding perforce metadata failed!\" err\n> +'\n> +\n> -+test_expect_success 'check utf-8 contents with legacy strategy' '\n> ++test_expect_success 'check utf-8 contents with passthrough strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> -+ git -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n> ++ git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n> + )\n> +'\n> +\n> -+test_expect_success 'check latin-1 contents corrupted in git with legacy strategy' '\n> ++test_expect_success 'check latin-1 contents corrupted in git with passthrough strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> -+ git -c git-p4.metadataDecodingStrategy=legacy p4.py clone --dest=\"$git\" //depot@all &&\n> ++ git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n> + )\n> +'\n> +\n> ++test_expect_success 'check cp850 contents parsed with correct fallback' '\n> ++ test_when_finished cleanup_git &&\n> ++ test_when_finished remove_user_cache &&\n> ++ git -c git-p4.metadataDecodingStrategy=fallback -c git-p4.metadataFallbackEncoding=cp850 p4.py clone --dest=\"$git\" //depot@all &&\n> ++ (\n> ++ cd \"$git\" &&\n> ++ git log >actual &&\n> ++ grep \"hÅs some cp850 text\" actual &&\n> ++ grep \"Åuthor\" actual\n> ++ )\n> ++'\n> ++\n> ++test_expect_success 'check cp850-only contents escaped when cp1252 is fallback' '\n> ++ test_when_finished cleanup_git &&\n> ++ test_when_finished remove_user_cache &&\n> ++ git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n> ++ (\n> ++ cd \"$git\" &&\n> ++ git log >actual &&\n> ++ grep \"h%8Fs some cp850 text\" actual &&\n> ++ grep \"%8Futhor\" actual\n> ++ )\n> ++'\n> ++\n> +test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n> + (\n> + cd \"$cli\" &&\n> + P4USER=cp1252_author &&\n> -+ touch file4 &&\n> -+ p4 add file4 &&\n> -+ p4 submit -d \"$(echo fourth CL has sœme more cp-1252 tæxt |\n> ++ touch file10 &&\n> ++ p4 add file10 &&\n> ++ p4 submit -d \"$(echo later CL has sœme more cp-1252 tæxt |\n> + iconv -f utf8 -t cp1252)\"\n> + ) &&\n> + (\n> @@ t/t9836-git-p4-metadata-encoding-python3.sh (new)\n> + git p4.py sync --branch=master &&\n> +\n> + git log p4/master >actual &&\n> -+ cat actual &&\n> + grep \"sœme more cp-1252 tæxt\" actual &&\n> + grep \"æuthœr\" actual\n> + )\n>\n>\n> Documentation/git-p4.txt | 37 +++-\n> git-p4.py | 123 +++++++++--\n> t/lib-git-p4.sh | 3 +-\n> t/t9835-git-p4-metadata-encoding-python2.sh | 213 +++++++++++++++++++\n> t/t9836-git-p4-metadata-encoding-python3.sh | 214 ++++++++++++++++++++\n> 5 files changed, 572 insertions(+), 18 deletions(-)\n> create mode 100755 t/t9835-git-p4-metadata-encoding-python2.sh\n> create mode 100755 t/t9836-git-p4-metadata-encoding-python3.sh\n>\n> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n> index e21fcd8f712..de5ee6748e3 100644\n> --- a/Documentation/git-p4.txt\n> +++ b/Documentation/git-p4.txt\n> @@ -636,7 +636,42 @@ git-p4.pathEncoding::\n> Git expects paths encoded as UTF-8. Use this config to tell git-p4\n> what encoding Perforce had used for the paths. This encoding is used\n> to transcode the paths to UTF-8. As an example, Perforce on Windows\n> - often uses \"cp1252\" to encode path names.\n> + often uses \"cp1252\" to encode path names. If this option is passed\n> + into a p4 clone request, it is persisted in the resulting new git\n> + repo.\n> +\n> +git-p4.metadataDecodingStrategy::\n> + Perforce keeps the encoding of a changelist descriptions and user\n> + full names as stored by the client on a given OS. The p4v client\n> + uses the OS-local encoding, and so different users can end up storing\n> + different changelist descriptions or user full names in different\n> + encodings, in the same depot.\n> + Git tolerates inconsistent/incorrect encodings in commit messages\n> + and author names, but expects them to be specified in utf-8.\n> + git-p4 can use three different decoding strategies in handling the\n> + encoding uncertainty in Perforce: 'passthrough' simply passes the\n> + original bytes through from Perforce to git, creating usable but\n> + incorrectly-encoded data when the Perforce data is encoded as\n> + anything other than utf-8. 'strict' expects the Perforce data to be\n> + encoded as utf-8, and fails to import when this is not true.\n> + 'fallback' attempts to interpret the data as utf-8, and otherwise\n> + falls back to using a secondary encoding - by default the common\n> + windows encoding 'cp-1252' - with upper-range bytes escaped if\n> + decoding with the fallback encoding also fails.\n> + Under python2 the default strategy is 'passthrough' for historical\n> + reasons, and under python3 the default is 'fallback'.\n> + When 'strict' is selected and decoding fails, the error message will\n> + propose changing this config parameter as a workaround. If this\n> + option is passed into a p4 clone request, it is persisted into the\n> + resulting new git repo.\n> +\n> +git-p4.metadataFallbackEncoding::\n> + Specify the fallback encoding to use when decoding Perforce author\n> + names and changelists descriptions using the 'fallback' strategy\n> + (see git-p4.metadataDecodingStrategy). The fallback encoding will\n> + only be used when decoding as utf-8 fails. This option defaults to\n> + cp1252, a common windows encoding. If this option is passed into a\n> + p4 clone request, it is persisted into the resulting new git repo.\n>\n> git-p4.largeFileSystem::\n> Specify the system that is used for large (binary) files. Please note\n> diff --git a/git-p4.py b/git-p4.py\n> index a9b1f904410..d24c3535f8a 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -15,6 +15,7 @@\n> # pylint: disable=too-many-statements,too-many-instance-attributes\n> # pylint: disable=too-many-branches,too-many-nested-blocks\n> #\n> +import struct\n> import sys\n> if sys.version_info.major < 3 and sys.version_info.minor < 7:\n> sys.stderr.write(\"git-p4: requires Python 2.7 or later.\\n\")\n> @@ -54,6 +55,9 @@ defaultLabelRegexp = r'[a-zA-Z0-9_\\-.]+$'\n> # The block size is reduced automatically if required\n> defaultBlockSize = 1<<20\n>\n> +defaultMetadataDecodingStrategy = 'passthrough' if sys.version_info.major == 2 else 'fallback'\n> +defaultFallbackMetadataEncoding = 'cp1252'\n> +\n> p4_access_checked = False\n>\n> re_ko_keywords = re.compile(br'\\$(Id|Header)(:[^$\\n]+)?\\$')\n> @@ -203,6 +207,70 @@ else:\n> def encode_text_stream(s):\n> return s.encode('utf_8') if isinstance(s, unicode) else s\n>\n> +\n> +class MetadataDecodingException(Exception):\n> + def __init__(self, input_string):\n> + self.input_string = input_string\n> +\n> + def __str__(self):\n> + return \"\"\"Decoding perforce metadata failed!\n> +The failing string was:\n> +---\n> +{}\n> +---\n> +Consider setting the git-p4.metadataDecodingStrategy config option to\n> +'fallback', to allow metadata to be decoded using a fallback encoding,\n> +defaulting to cp1252.\"\"\".format(self.input_string)\n> +\n> +\n> +encoding_fallback_warning_issued = False\n> +encoding_escape_warning_issued = False\n> +def metadata_stream_to_writable_bytes(s):\n> + encodingStrategy = gitConfig('git-p4.metadataDecodingStrategy') or defaultMetadataDecodingStrategy\n> + fallbackEncoding = gitConfig('git-p4.metadataFallbackEncoding') or defaultFallbackMetadataEncoding\n> + if not isinstance(s, bytes):\n> + return s.encode('utf_8')\n> + if encodingStrategy == 'passthrough':\n> + return s\n> + try:\n> + s.decode('utf_8')\n> + return s\n> + except UnicodeDecodeError:\n> + if encodingStrategy == 'fallback' and fallbackEncoding:\n> + global encoding_fallback_warning_issued\n> + global encoding_escape_warning_issued\n> + try:\n> + if not encoding_fallback_warning_issued:\n> + print(\"\\nCould not decode value as utf-8; using configured fallback encoding %s: %s\" % (fallbackEncoding, s))\n> + print(\"\\n(this warning is only displayed once during an import)\")\n> + encoding_fallback_warning_issued = True\n> + return s.decode(fallbackEncoding).encode('utf_8')\n> + except Exception as exc:\n> + if not encoding_escape_warning_issued:\n> + print(\"\\nCould not decode value with configured fallback encoding %s; escaping bytes over 127: %s\" % (fallbackEncoding, s))\n> + print(\"\\n(this warning is only displayed once during an import)\")\n> + encoding_escape_warning_issued = True\n> + escaped_bytes = b''\n> + # bytes and strings work very differently in python2 vs python3...\n> + if str is bytes:\n> + for byte in s:\n> + byte_number = struct.unpack('>B', byte)[0]\n> + if byte_number > 127:\n> + escaped_bytes += b'%'\n> + escaped_bytes += hex(byte_number)[2:].upper()\n> + else:\n> + escaped_bytes += byte\n> + else:\n> + for byte_number in s:\n> + if byte_number > 127:\n> + escaped_bytes += b'%'\n> + escaped_bytes += hex(byte_number).upper().encode()[2:]\n> + else:\n> + escaped_bytes += bytes([byte_number])\n> + return escaped_bytes\n> +\n> + raise MetadataDecodingException(s)\n> +\n> def decode_path(path):\n> \"\"\"Decode a given string (bytes or otherwise) using configured path encoding options\n> \"\"\"\n> @@ -702,11 +770,12 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\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> + # - `desc` or `FullName` which may contain non-UTF8 encoded text handled below, eagerly converted to bytes\n> + # - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text, handled by decode_path()\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 ('data', 'desc', 'FullName', 'path', 'clientFile') or key.startswith('depotFile')):\n> value = value.decode()\n> decoded_entry[key] = value\n> # Parse out data if it's an error response\n> @@ -716,6 +785,10 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n> if skip_info:\n> if 'code' in entry and entry['code'] == 'info':\n> continue\n> + if 'desc' in entry:\n> + entry['desc'] = metadata_stream_to_writable_bytes(entry['desc'])\n> + if 'FullName' in entry:\n> + entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName'])\n> if cb is not None:\n> cb(entry)\n> else:\n> @@ -1435,7 +1508,13 @@ class P4UserMap:\n> for output in p4CmdList([\"users\"]):\n> if \"User\" not in output:\n> continue\n> - self.users[output[\"User\"]] = output[\"FullName\"] + \" <\" + output[\"Email\"] + \">\"\n> + # \"FullName\" is bytes. \"Email\" on the other hand might be bytes\n> + # or unicode string depending on whether we are running under\n> + # python2 or python3. To support\n> + # git-p4.metadataDecodingStrategy=fallback, self.users dict values\n> + # are always bytes, ready to be written to git.\n> + emailbytes = metadata_stream_to_writable_bytes(output[\"Email\"])\n> + self.users[output[\"User\"]] = output[\"FullName\"] + b\" <\" + emailbytes + b\">\"\n> self.emails[output[\"Email\"]] = output[\"User\"]\n>\n> mapUserConfigRegex = re.compile(r\"^\\s*(\\S+)\\s*=\\s*(.+)\\s*<(\\S+)>\\s*$\", re.VERBOSE)\n> @@ -1445,26 +1524,28 @@ class P4UserMap:\n> user = mapUser[0][0]\n> fullname = mapUser[0][1]\n> email = mapUser[0][2]\n> - self.users[user] = fullname + \" <\" + email + \">\"\n> + fulluser = fullname + \" <\" + email + \">\"\n> + self.users[user] = metadata_stream_to_writable_bytes(fulluser)\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> + keybytes = metadata_stream_to_writable_bytes(key)\n> + s += b\"%s\\t%s\\n\" % (keybytes.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> - self.users[entry[0]] = entry[1]\n> + entry = line.strip().split(b\"\\t\")\n> + self.users[entry[0].decode('utf_8')] = entry[1]\n> except IOError:\n> self.getUserMapFromPerforceServer()\n>\n> @@ -3020,7 +3101,8 @@ class P4Sync(Command, P4UserMap):\n> if userid in self.users:\n> return self.users[userid]\n> else:\n> - return \"%s \" % userid\n> + userid_bytes = metadata_stream_to_writable_bytes(userid)\n> + return b\"%s \" % userid_bytes\n>\n> def streamTag(self, gitStream, labelName, labelDetails, commit, epoch):\n> \"\"\" Stream a p4 tag.\n> @@ -3043,9 +3125,10 @@ class P4Sync(Command, P4UserMap):\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>\n> - gitStream.write(\"tagger %s\\n\" % tagger)\n> + gitStream.write(\"tagger \")\n> + gitStream.write(email)\n> + gitStream.write(\" %s %s\\n\" % (epoch, self.tz))\n>\n> print(\"labelDetails=\",labelDetails)\n> if 'Description' in labelDetails:\n> @@ -3138,12 +3221,12 @@ class P4Sync(Command, P4UserMap):\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>\n> - self.gitStream.write(\"committer %s\\n\" % committer)\n> + self.gitStream.write(\"committer \")\n> + self.gitStream.write(self.make_email(author))\n> + self.gitStream.write(\" %s %s\\n\" % (epoch, self.tz))\n>\n> self.gitStream.write(\"data < self.gitStream.write(details[\"desc\"])\n> @@ -4055,6 +4138,14 @@ class P4Clone(P4Sync):\n> if self.useClientSpec_from_options:\n> system([\"git\", \"config\", \"--bool\", \"git-p4.useclientspec\", \"true\"])\n>\n> + # persist any git-p4 encoding-handling config options passed in for clone:\n> + if gitConfig('git-p4.metadataDecodingStrategy'):\n> + system([\"git\", \"config\", \"git-p4.metadataDecodingStrategy\", gitConfig('git-p4.metadataDecodingStrategy')])\n> + if gitConfig('git-p4.metadataFallbackEncoding'):\n> + system([\"git\", \"config\", \"git-p4.metadataFallbackEncoding\", gitConfig('git-p4.metadataFallbackEncoding')])\n> + if gitConfig('git-p4.pathEncoding'):\n> + system([\"git\", \"config\", \"git-p4.pathEncoding\", gitConfig('git-p4.pathEncoding')])\n> +\n> return True\n>\n> class P4Unshelve(Command):\n> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\n> index 5aff2abe8b5..2a5b8738ea3 100644\n> --- a/t/lib-git-p4.sh\n> +++ b/t/lib-git-p4.sh\n> @@ -142,10 +142,11 @@ start_p4d () {\n>\n> p4_add_user () {\n> name=$1 &&\n> + fullname=\"${2:-Dr. $1}\"\n> p4 user -f -i <<-EOF\n> User: $name\n> Email: $name@example.com\n> - FullName: Dr. $name\n> + FullName: $fullname\n> EOF\n> }\n>\n> diff --git a/t/t9835-git-p4-metadata-encoding-python2.sh b/t/t9835-git-p4-metadata-encoding-python2.sh\n> new file mode 100755\n> index 00000000000..036bf79c667\n> --- /dev/null\n> +++ b/t/t9835-git-p4-metadata-encoding-python2.sh\n> @@ -0,0 +1,213 @@\n> +#!/bin/sh\n> +\n> +test_description='git p4 metadata encoding\n> +\n> +This test checks that the import process handles inconsistent text\n> +encoding in p4 metadata (author names, commit messages, etc) without\n> +failing, and produces maximally sane output in git.'\n> +\n> +. ./lib-git-p4.sh\n> +\n> +python_target_version='2'\n> +\n> +###############################\n> +## SECTION REPEATED IN t9836 ##\n> +###############################\n> +\n> +# Please note: this test calls \"git-p4.py\" rather than \"git-p4\", because the\n> +# latter references a specific path so we can't easily force it to run under\n> +# the python version we need to.\n> +\n> +python_major_version=$(python -V 2>&1 | cut -c 8)\n> +python_target_binary=$(which python$python_target_version)\n> +if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_binary\"\n> +then\n> + mkdir temp_python\n> + PATH=\"$(pwd)/temp_python:$PATH\" && export PATH\n> + ln -s $python_target_binary temp_python/python\n> +fi\n> +\n> +python_major_version=$(python -V 2>&1 | cut -c 8)\n> +if ! test \"$python_major_version\" = \"$python_target_version\"\n> +then\n> + skip_all=\"skipping python$python_target_version-specific git p4 tests; python$python_target_version not available\"\n> + test_done\n> +fi\n> +\n> +remove_user_cache () {\n> + rm \"$HOME/.gitp4-usercache.txt\" || true\n> +}\n> +\n> +test_expect_success 'start p4d' '\n> + start_p4d\n> +'\n> +\n> +test_expect_success 'init depot' '\n> + (\n> + cd \"$cli\" &&\n> +\n> + p4_add_user \"utf8_author\" \"ǣuthor\" &&\n> + P4USER=utf8_author &&\n> + touch file1 &&\n> + p4 add file1 &&\n> + p4 submit -d \"first CL has some utf-8 tǣxt\" &&\n> +\n> + p4_add_user \"latin1_author\" \"$(echo æuthor |\n> + iconv -f utf8 -t latin1)\" &&\n> + P4USER=latin1_author &&\n> + touch file2 &&\n> + p4 add file2 &&\n> + p4 submit -d \"$(echo second CL has some latin-1 tæxt |\n> + iconv -f utf8 -t latin1)\" &&\n> +\n> + p4_add_user \"cp1252_author\" \"$(echo æuthœr |\n> + iconv -f utf8 -t cp1252)\" &&\n> + P4USER=cp1252_author &&\n> + touch file3 &&\n> + p4 add file3 &&\n> + p4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n> + iconv -f utf8 -t cp1252)\" &&\n> +\n> + p4_add_user \"cp850_author\" \"$(echo Åuthor |\n> + iconv -f utf8 -t cp850)\" &&\n> + P4USER=cp850_author &&\n> + touch file4 &&\n> + p4 add file4 &&\n> + p4 submit -d \"$(echo fourth CL hÅs some cp850 text |\n> + iconv -f utf8 -t cp850)\"\n> + )\n> +'\n> +\n> +test_expect_success 'clone non-utf8 repo with strict encoding' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + test_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest=\"$git\" //depot@all 2>err &&\n> + grep \"Decoding perforce metadata failed!\" err\n> +'\n> +\n> +test_expect_success 'check utf-8 contents with passthrough strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"some utf-8 tǣxt\" actual &&\n> + grep \"ǣuthor\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check latin-1 contents corrupted in git with passthrough strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + badly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n> + grep \"$badly_encoded_in_git\" actual &&\n> + bad_author_in_git=\"$(echo æuthor | iconv -f utf8 -t latin1)\" &&\n> + grep \"$bad_author_in_git\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check utf-8 contents with fallback strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"some utf-8 tǣxt\" actual &&\n> + grep \"ǣuthor\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check latin-1 contents with fallback strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"some latin-1 tæxt\" actual &&\n> + grep \"æuthor\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check cp-1252 contents with fallback strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"sœme cp-1252 tæxt\" actual &&\n> + grep \"æuthœr\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check cp850 contents parsed with correct fallback' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=fallback -c git-p4.metadataFallbackEncoding=cp850 p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"hÅs some cp850 text\" actual &&\n> + grep \"Åuthor\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check cp850-only contents escaped when cp1252 is fallback' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"h%8Fs some cp850 text\" actual &&\n> + grep \"%8Futhor\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$cli\" &&\n> + P4USER=cp1252_author &&\n> + touch file10 &&\n> + p4 add file10 &&\n> + p4 submit -d \"$(echo later CL has sœme more cp-1252 tæxt |\n> + iconv -f utf8 -t cp1252)\"\n> + ) &&\n> + (\n> + cd \"$git\" &&\n> +\n> + git p4.py sync --branch=master &&\n> +\n> + git log p4/master >actual &&\n> + grep \"sœme more cp-1252 tæxt\" actual &&\n> + grep \"æuthœr\" actual\n> + )\n> +'\n> +\n> +############################\n> +## / END REPEATED SECTION ##\n> +############################\n> +\n> +test_expect_success 'passthrough (latin-1 contents corrupted in git) is the default with python2' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + badly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n> + grep \"$badly_encoded_in_git\" actual\n> + )\n> +'\n> +\n> +test_done\n> diff --git a/t/t9836-git-p4-metadata-encoding-python3.sh b/t/t9836-git-p4-metadata-encoding-python3.sh\n> new file mode 100755\n> index 00000000000..63350dc4b5c\n> --- /dev/null\n> +++ b/t/t9836-git-p4-metadata-encoding-python3.sh\n> @@ -0,0 +1,214 @@\n> +#!/bin/sh\n> +\n> +test_description='git p4 metadata encoding\n> +\n> +This test checks that the import process handles inconsistent text\n> +encoding in p4 metadata (author names, commit messages, etc) without\n> +failing, and produces maximally sane output in git.'\n> +\n> +. ./lib-git-p4.sh\n> +\n> +python_target_version='3'\n> +\n> +###############################\n> +## SECTION REPEATED IN t9835 ##\n> +###############################\n> +\n> +# Please note: this test calls \"git-p4.py\" rather than \"git-p4\", because the\n> +# latter references a specific path so we can't easily force it to run under\n> +# the python version we need to.\n> +\n> +python_major_version=$(python -V 2>&1 | cut -c 8)\n> +python_target_binary=$(which python$python_target_version)\n> +if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_binary\"\n> +then\n> + mkdir temp_python\n> + PATH=\"$(pwd)/temp_python:$PATH\" && export PATH\n> + ln -s $python_target_binary temp_python/python\n> +fi\n> +\n> +python_major_version=$(python -V 2>&1 | cut -c 8)\n> +if ! test \"$python_major_version\" = \"$python_target_version\"\n> +then\n> + skip_all=\"skipping python$python_target_version-specific git p4 tests; python$python_target_version not available\"\n> + test_done\n> +fi\n> +\n> +remove_user_cache () {\n> + rm \"$HOME/.gitp4-usercache.txt\" || true\n> +}\n> +\n> +test_expect_success 'start p4d' '\n> + start_p4d\n> +'\n> +\n> +test_expect_success 'init depot' '\n> + (\n> + cd \"$cli\" &&\n> +\n> + p4_add_user \"utf8_author\" \"ǣuthor\" &&\n> + P4USER=utf8_author &&\n> + touch file1 &&\n> + p4 add file1 &&\n> + p4 submit -d \"first CL has some utf-8 tǣxt\" &&\n> +\n> + p4_add_user \"latin1_author\" \"$(echo æuthor |\n> + iconv -f utf8 -t latin1)\" &&\n> + P4USER=latin1_author &&\n> + touch file2 &&\n> + p4 add file2 &&\n> + p4 submit -d \"$(echo second CL has some latin-1 tæxt |\n> + iconv -f utf8 -t latin1)\" &&\n> +\n> + p4_add_user \"cp1252_author\" \"$(echo æuthœr |\n> + iconv -f utf8 -t cp1252)\" &&\n> + P4USER=cp1252_author &&\n> + touch file3 &&\n> + p4 add file3 &&\n> + p4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n> + iconv -f utf8 -t cp1252)\" &&\n> +\n> + p4_add_user \"cp850_author\" \"$(echo Åuthor |\n> + iconv -f utf8 -t cp850)\" &&\n> + P4USER=cp850_author &&\n> + touch file4 &&\n> + p4 add file4 &&\n> + p4 submit -d \"$(echo fourth CL hÅs some cp850 text |\n> + iconv -f utf8 -t cp850)\"\n> + )\n> +'\n> +\n> +test_expect_success 'clone non-utf8 repo with strict encoding' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + test_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest=\"$git\" //depot@all 2>err &&\n> + grep \"Decoding perforce metadata failed!\" err\n> +'\n> +\n> +test_expect_success 'check utf-8 contents with passthrough strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"some utf-8 tǣxt\" actual &&\n> + grep \"ǣuthor\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check latin-1 contents corrupted in git with passthrough strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + badly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n> + grep \"$badly_encoded_in_git\" actual &&\n> + bad_author_in_git=\"$(echo æuthor | iconv -f utf8 -t latin1)\" &&\n> + grep \"$bad_author_in_git\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check utf-8 contents with fallback strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"some utf-8 tǣxt\" actual &&\n> + grep \"ǣuthor\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check latin-1 contents with fallback strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"some latin-1 tæxt\" actual &&\n> + grep \"æuthor\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check cp-1252 contents with fallback strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"sœme cp-1252 tæxt\" actual &&\n> + grep \"æuthœr\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check cp850 contents parsed with correct fallback' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=fallback -c git-p4.metadataFallbackEncoding=cp850 p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"hÅs some cp850 text\" actual &&\n> + grep \"Åuthor\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check cp850-only contents escaped when cp1252 is fallback' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"h%8Fs some cp850 text\" actual &&\n> + grep \"%8Futhor\" actual\n> + )\n> +'\n> +\n> +test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$cli\" &&\n> + P4USER=cp1252_author &&\n> + touch file10 &&\n> + p4 add file10 &&\n> + p4 submit -d \"$(echo later CL has sœme more cp-1252 tæxt |\n> + iconv -f utf8 -t cp1252)\"\n> + ) &&\n> + (\n> + cd \"$git\" &&\n> +\n> + git p4.py sync --branch=master &&\n> +\n> + git log p4/master >actual &&\n> + grep \"sœme more cp-1252 tæxt\" actual &&\n> + grep \"æuthœr\" actual\n> + )\n> +'\n> +\n> +############################\n> +## / END REPEATED SECTION ##\n> +############################\n> +\n> +\n> +test_expect_success 'fallback (both utf-8 and cp-1252 contents handled) is the default with python3' '\n> + test_when_finished cleanup_git &&\n> + test_when_finished remove_user_cache &&\n> + git p4.py clone --dest=\"$git\" //depot@all &&\n> + (\n> + cd \"$git\" &&\n> + git log >actual &&\n> + grep \"sœme cp-1252 tæxt\" actual &&\n> + grep \"æuthœr\" actual\n> + )\n> +'\n> +\n> +test_done\n>\n> base-commit: 11cfe552610386954886543f5de87dcc49ad5735\n> --\n> gitgitgadget\n"},{"id":"454686","messageId":"pull.1206.v4.git.1651346812586.gitgitgadget@gmail.com","threadId":"57709","inReplyTo":"pull.1206.v3.git.1650399590844.gitgitgadget@gmail.com","subject":"[PATCH v4] git-p4: improve encoding handling to support inconsistent encodings","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-30T19:26:52Z","receivedAt":"2022-04-30T19:27:04Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\ngit-p4 is designed to run correctly under python2.7 and python3, but\nits functional behavior wrt importing user-entered text differs across\nthese environments:\n\nUnder python2, git-p4 \"naively\" writes the Perforce bytestream into git\nmetadata (and does not set an \"encoding\" header on the commits); this\nmeans that any non-utf-8 byte sequences end up creating invalidly-encoded\ncommit metadata in git.\n\nUnder python3, git-p4 attempts to decode the Perforce bytestream as utf-8\ndata, and fails badly (with an unhelpful error) when non-utf-8 data is\nencountered.\n\nPerforce clients (especially p4v) encourage user entry of changelist\ndescriptions (and user full names) in OS-local encoding, and store the\nresulting bytestream to the server unmodified - such that different\nclients can end up creating mutually-unintelligible messages. The most\ncommon inconsistency, in many Perforce environments, is likely to be utf-8\n(typical in linux) vs cp-1252 (typical in windows).\n\nMake the changelist-description- and user-fullname-handling code\npython-runtime-agnostic, introducing three \"strategies\" selectable via\nconfig:\n- 'passthrough', behaving as previously under python2,\n- 'strict', behaving as previously under python3, and\n- 'fallback', favoring utf-8 but supporting a secondary encoding when\nutf-8 decoding fails, and finally escaping high-range bytes if the\ndecoding with the secondary encoding also fails.\n\nKeep the python2 default behavior as-is ('legacy' strategy), but switch\nthe python3 default strategy to 'fallback' with default fallback encoding\n'cp1252'.\n\nAlso include tests exercising these encoding strategies, documentation for\nthe new config, and improve the user-facing error messages when decoding\ndoes fail.\n\nSigned-off-by: Tao Klerks \n---\n Git p4 encoding strategy\n \n This is no longer RFC, it's now request-for-Review!\n \n Changes wrt v2:\n \n * Renamed \"legacy\" strategy to \"passthrough\", reflecting the possible\n value of maintaining it long-term\n * Changed \"fallback decoding failure\" behavior to escape over-127\n bytes, instead of omitting them. There should now be no information\n loss under any scenario, although recovering the original bytes might\n be non-trivial\n \n Changes wrt v3:\n \n * I had accidentally sent with the old title and cover letter.\n \n Changes wrt v4:\n \n * Rebased onto recent master\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1206%2FTaoK%2Fgit-p4-encoding-strategy-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1206/TaoK/git-p4-encoding-strategy-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1206\n\nRange-diff vs v3:\n\n 1: 1d83b6d7865 = 1: 8392e2a3f75 git-p4: improve encoding handling to support inconsistent encodings\n\n\n Documentation/git-p4.txt | 37 +++-\n git-p4.py | 123 +++++++++--\n t/lib-git-p4.sh | 3 +-\n t/t9835-git-p4-metadata-encoding-python2.sh | 213 +++++++++++++++++++\n t/t9836-git-p4-metadata-encoding-python3.sh | 214 ++++++++++++++++++++\n 5 files changed, 572 insertions(+), 18 deletions(-)\n create mode 100755 t/t9835-git-p4-metadata-encoding-python2.sh\n create mode 100755 t/t9836-git-p4-metadata-encoding-python3.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex e21fcd8f712..de5ee6748e3 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -636,7 +636,42 @@ git-p4.pathEncoding::\n \tGit expects paths encoded as UTF-8. Use this config to tell git-p4\n \twhat encoding Perforce had used for the paths. This encoding is used\n \tto transcode the paths to UTF-8. As an example, Perforce on Windows\n-\toften uses \"cp1252\" to encode path names.\n+\toften uses \"cp1252\" to encode path names. If this option is passed\n+\tinto a p4 clone request, it is persisted in the resulting new git\n+\trepo.\n+\n+git-p4.metadataDecodingStrategy::\n+\tPerforce keeps the encoding of a changelist descriptions and user\n+\tfull names as stored by the client on a given OS. The p4v client\n+\tuses the OS-local encoding, and so different users can end up storing\n+\tdifferent changelist descriptions or user full names in different\n+\tencodings, in the same depot.\n+\tGit tolerates inconsistent/incorrect encodings in commit messages\n+\tand author names, but expects them to be specified in utf-8.\n+\tgit-p4 can use three different decoding strategies in handling the\n+\tencoding uncertainty in Perforce: 'passthrough' simply passes the\n+\toriginal bytes through from Perforce to git, creating usable but\n+\tincorrectly-encoded data when the Perforce data is encoded as\n+\tanything other than utf-8. 'strict' expects the Perforce data to be\n+\tencoded as utf-8, and fails to import when this is not true.\n+\t'fallback' attempts to interpret the data as utf-8, and otherwise\n+\tfalls back to using a secondary encoding - by default the common\n+\twindows encoding 'cp-1252' - with upper-range bytes escaped if\n+\tdecoding with the fallback encoding also fails.\n+\tUnder python2 the default strategy is 'passthrough' for historical\n+\treasons, and under python3 the default is 'fallback'.\n+\tWhen 'strict' is selected and decoding fails, the error message will\n+\tpropose changing this config parameter as a workaround. If this\n+\toption is passed into a p4 clone request, it is persisted into the\n+\tresulting new git repo.\n+\n+git-p4.metadataFallbackEncoding::\n+\tSpecify the fallback encoding to use when decoding Perforce author\n+\tnames and changelists descriptions using the 'fallback' strategy\n+\t(see git-p4.metadataDecodingStrategy). The fallback encoding will\n+\tonly be used when decoding as utf-8 fails. This option defaults to\n+\tcp1252, a common windows encoding. If this option is passed into a\n+\tp4 clone request, it is persisted into the resulting new git repo.\n \n git-p4.largeFileSystem::\n \tSpecify the system that is used for large (binary) files. Please note\ndiff --git a/git-p4.py b/git-p4.py\nindex a9b1f904410..d24c3535f8a 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -15,6 +15,7 @@\n # pylint: disable=too-many-statements,too-many-instance-attributes\n # pylint: disable=too-many-branches,too-many-nested-blocks\n #\n+import struct\n import sys\n if sys.version_info.major < 3 and sys.version_info.minor < 7:\n sys.stderr.write(\"git-p4: requires Python 2.7 or later.\\n\")\n@@ -54,6 +55,9 @@ defaultLabelRegexp = r'[a-zA-Z0-9_\\-.]+$'\n # The block size is reduced automatically if required\n defaultBlockSize = 1<<20\n \n+defaultMetadataDecodingStrategy = 'passthrough' if sys.version_info.major == 2 else 'fallback'\n+defaultFallbackMetadataEncoding = 'cp1252'\n+\n p4_access_checked = False\n \n re_ko_keywords = re.compile(br'\\$(Id|Header)(:[^$\\n]+)?\\$')\n@@ -203,6 +207,70 @@ else:\n def encode_text_stream(s):\n return s.encode('utf_8') if isinstance(s, unicode) else s\n \n+\n+class MetadataDecodingException(Exception):\n+ def __init__(self, input_string):\n+ self.input_string = input_string\n+\n+ def __str__(self):\n+ return \"\"\"Decoding perforce metadata failed!\n+The failing string was:\n+---\n+{}\n+---\n+Consider setting the git-p4.metadataDecodingStrategy config option to\n+'fallback', to allow metadata to be decoded using a fallback encoding,\n+defaulting to cp1252.\"\"\".format(self.input_string)\n+\n+\n+encoding_fallback_warning_issued = False\n+encoding_escape_warning_issued = False\n+def metadata_stream_to_writable_bytes(s):\n+ encodingStrategy = gitConfig('git-p4.metadataDecodingStrategy') or defaultMetadataDecodingStrategy\n+ fallbackEncoding = gitConfig('git-p4.metadataFallbackEncoding') or defaultFallbackMetadataEncoding\n+ if not isinstance(s, bytes):\n+ return s.encode('utf_8')\n+ if encodingStrategy == 'passthrough':\n+ return s\n+ try:\n+ s.decode('utf_8')\n+ return s\n+ except UnicodeDecodeError:\n+ if encodingStrategy == 'fallback' and fallbackEncoding:\n+ global encoding_fallback_warning_issued\n+ global encoding_escape_warning_issued\n+ try:\n+ if not encoding_fallback_warning_issued:\n+ print(\"\\nCould not decode value as utf-8; using configured fallback encoding %s: %s\" % (fallbackEncoding, s))\n+ print(\"\\n(this warning is only displayed once during an import)\")\n+ encoding_fallback_warning_issued = True\n+ return s.decode(fallbackEncoding).encode('utf_8')\n+ except Exception as exc:\n+ if not encoding_escape_warning_issued:\n+ print(\"\\nCould not decode value with configured fallback encoding %s; escaping bytes over 127: %s\" % (fallbackEncoding, s))\n+ print(\"\\n(this warning is only displayed once during an import)\")\n+ encoding_escape_warning_issued = True\n+ escaped_bytes = b''\n+ # bytes and strings work very differently in python2 vs python3...\n+ if str is bytes:\n+ for byte in s:\n+ byte_number = struct.unpack('>B', byte)[0]\n+ if byte_number > 127:\n+ escaped_bytes += b'%'\n+ escaped_bytes += hex(byte_number)[2:].upper()\n+ else:\n+ escaped_bytes += byte\n+ else:\n+ for byte_number in s:\n+ if byte_number > 127:\n+ escaped_bytes += b'%'\n+ escaped_bytes += hex(byte_number).upper().encode()[2:]\n+ else:\n+ escaped_bytes += bytes([byte_number])\n+ return escaped_bytes\n+\n+ raise MetadataDecodingException(s)\n+\n def decode_path(path):\n \"\"\"Decode a given string (bytes or otherwise) using configured path encoding options\n \"\"\"\n@@ -702,11 +770,12 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\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+ # - `desc` or `FullName` which may contain non-UTF8 encoded text handled below, eagerly converted to bytes\n+ # - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text, handled by decode_path()\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 ('data', 'desc', 'FullName', 'path', 'clientFile') or key.startswith('depotFile')):\n value = value.decode()\n decoded_entry[key] = value\n # Parse out data if it's an error response\n@@ -716,6 +785,10 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n if skip_info:\n if 'code' in entry and entry['code'] == 'info':\n continue\n+ if 'desc' in entry:\n+ entry['desc'] = metadata_stream_to_writable_bytes(entry['desc'])\n+ if 'FullName' in entry:\n+ entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName'])\n if cb is not None:\n cb(entry)\n else:\n@@ -1435,7 +1508,13 @@ class P4UserMap:\n for output in p4CmdList([\"users\"]):\n if \"User\" not in output:\n continue\n- self.users[output[\"User\"]] = output[\"FullName\"] + \" <\" + output[\"Email\"] + \">\"\n+ # \"FullName\" is bytes. \"Email\" on the other hand might be bytes\n+ # or unicode string depending on whether we are running under\n+ # python2 or python3. To support\n+ # git-p4.metadataDecodingStrategy=fallback, self.users dict values\n+ # are always bytes, ready to be written to git.\n+ emailbytes = metadata_stream_to_writable_bytes(output[\"Email\"])\n+ self.users[output[\"User\"]] = output[\"FullName\"] + b\" <\" + emailbytes + b\">\"\n self.emails[output[\"Email\"]] = output[\"User\"]\n \n mapUserConfigRegex = re.compile(r\"^\\s*(\\S+)\\s*=\\s*(.+)\\s*<(\\S+)>\\s*$\", re.VERBOSE)\n@@ -1445,26 +1524,28 @@ class P4UserMap:\n user = mapUser[0][0]\n fullname = mapUser[0][1]\n email = mapUser[0][2]\n- self.users[user] = fullname + \" <\" + email + \">\"\n+ fulluser = fullname + \" <\" + email + \">\"\n+ self.users[user] = metadata_stream_to_writable_bytes(fulluser)\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+ keybytes = metadata_stream_to_writable_bytes(key)\n+ s += b\"%s\\t%s\\n\" % (keybytes.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- self.users[entry[0]] = entry[1]\n+ entry = line.strip().split(b\"\\t\")\n+ self.users[entry[0].decode('utf_8')] = entry[1]\n except IOError:\n self.getUserMapFromPerforceServer()\n \n@@ -3020,7 +3101,8 @@ class P4Sync(Command, P4UserMap):\n if userid in self.users:\n return self.users[userid]\n else:\n- return \"%s \" % userid\n+ userid_bytes = metadata_stream_to_writable_bytes(userid)\n+ return b\"%s \" % userid_bytes\n \n def streamTag(self, gitStream, labelName, labelDetails, commit, epoch):\n \"\"\" Stream a p4 tag.\n@@ -3043,9 +3125,10 @@ class P4Sync(Command, P4UserMap):\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 \n- gitStream.write(\"tagger %s\\n\" % tagger)\n+ gitStream.write(\"tagger \")\n+ gitStream.write(email)\n+ gitStream.write(\" %s %s\\n\" % (epoch, self.tz))\n \n print(\"labelDetails=\",labelDetails)\n if 'Description' in labelDetails:\n@@ -3138,12 +3221,12 @@ class P4Sync(Command, P4UserMap):\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 \n- self.gitStream.write(\"committer %s\\n\" % committer)\n+ self.gitStream.write(\"committer \")\n+ self.gitStream.write(self.make_email(author))\n+ self.gitStream.write(\" %s %s\\n\" % (epoch, self.tz))\n \n self.gitStream.write(\"data <&1 | cut -c 8)\n+python_target_binary=$(which python$python_target_version)\n+if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_binary\"\n+then\n+\tmkdir temp_python\n+\tPATH=\"$(pwd)/temp_python:$PATH\" && export PATH\n+\tln -s $python_target_binary temp_python/python\n+fi\n+\n+python_major_version=$(python -V 2>&1 | cut -c 8)\n+if ! test \"$python_major_version\" = \"$python_target_version\"\n+then\n+\tskip_all=\"skipping python$python_target_version-specific git p4 tests; python$python_target_version not available\"\n+\ttest_done\n+fi\n+\n+remove_user_cache () {\n+\trm \"$HOME/.gitp4-usercache.txt\" || true\n+}\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'init depot' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\tp4_add_user \"utf8_author\" \"ǣuthor\" &&\n+\t\tP4USER=utf8_author &&\n+\t\ttouch file1 &&\n+\t\tp4 add file1 &&\n+\t\tp4 submit -d \"first CL has some utf-8 tǣxt\" &&\n+\n+\t\tp4_add_user \"latin1_author\" \"$(echo æuthor |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\t\tP4USER=latin1_author &&\n+\t\ttouch file2 &&\n+\t\tp4 add file2 &&\n+\t\tp4 submit -d \"$(echo second CL has some latin-1 tæxt |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\n+\t\tp4_add_user \"cp1252_author\" \"$(echo æuthœr |\n+\t\t\ticonv -f utf8 -t cp1252)\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file3 &&\n+\t\tp4 add file3 &&\n+\t\tp4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n+\t\t iconv -f utf8 -t cp1252)\" &&\n+\n+\t\tp4_add_user \"cp850_author\" \"$(echo Åuthor |\n+\t\t\ticonv -f utf8 -t cp850)\" &&\n+\t\tP4USER=cp850_author &&\n+\t\ttouch file4 &&\n+\t\tp4 add file4 &&\n+\t\tp4 submit -d \"$(echo fourth CL hÅs some cp850 text |\n+\t\t\ticonv -f utf8 -t cp850)\"\n+\t)\n+'\n+\n+test_expect_success 'clone non-utf8 repo with strict encoding' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\ttest_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest=\"$git\" //depot@all 2>err &&\n+\tgrep \"Decoding perforce metadata failed!\" err\n+'\n+\n+test_expect_success 'check utf-8 contents with passthrough strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents corrupted in git with passthrough strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tbadly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n+\t\tgrep \"$badly_encoded_in_git\" actual &&\n+\t\tbad_author_in_git=\"$(echo æuthor | iconv -f utf8 -t latin1)\" &&\n+\t\tgrep \"$bad_author_in_git\" actual\n+\t)\n+'\n+\n+test_expect_success 'check utf-8 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some latin-1 tæxt\" actual &&\n+\t\tgrep \"æuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"sœme cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp850 contents parsed with correct fallback' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback -c git-p4.metadataFallbackEncoding=cp850 p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"hÅs some cp850 text\" actual &&\n+\t\tgrep \"Åuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp850-only contents escaped when cp1252 is fallback' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"h%8Fs some cp850 text\" actual &&\n+\t\tgrep \"%8Futhor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file10 &&\n+\t\tp4 add file10 &&\n+\t\tp4 submit -d \"$(echo later CL has sœme more cp-1252 tæxt |\n+\t\t\ticonv -f utf8 -t cp1252)\"\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\n+\t\tgit p4.py sync --branch=master &&\n+\n+\t\tgit log p4/master >actual &&\n+\t\tgrep \"sœme more cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+############################\n+## / END REPEATED SECTION ##\n+############################\n+\n+test_expect_success 'passthrough (latin-1 contents corrupted in git) is the default with python2' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tbadly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n+\t\tgrep \"$badly_encoded_in_git\" actual\n+\t)\n+'\n+\n+test_done\ndiff --git a/t/t9836-git-p4-metadata-encoding-python3.sh b/t/t9836-git-p4-metadata-encoding-python3.sh\nnew file mode 100755\nindex 00000000000..63350dc4b5c\n--- /dev/null\n+++ b/t/t9836-git-p4-metadata-encoding-python3.sh\n@@ -0,0 +1,214 @@\n+#!/bin/sh\n+\n+test_description='git p4 metadata encoding\n+\n+This test checks that the import process handles inconsistent text\n+encoding in p4 metadata (author names, commit messages, etc) without\n+failing, and produces maximally sane output in git.'\n+\n+. ./lib-git-p4.sh\n+\n+python_target_version='3'\n+\n+###############################\n+## SECTION REPEATED IN t9835 ##\n+###############################\n+\n+# Please note: this test calls \"git-p4.py\" rather than \"git-p4\", because the\n+# latter references a specific path so we can't easily force it to run under\n+# the python version we need to.\n+\n+python_major_version=$(python -V 2>&1 | cut -c 8)\n+python_target_binary=$(which python$python_target_version)\n+if ! test \"$python_major_version\" = \"$python_target_version\" && test \"$python_target_binary\"\n+then\n+\tmkdir temp_python\n+\tPATH=\"$(pwd)/temp_python:$PATH\" && export PATH\n+\tln -s $python_target_binary temp_python/python\n+fi\n+\n+python_major_version=$(python -V 2>&1 | cut -c 8)\n+if ! test \"$python_major_version\" = \"$python_target_version\"\n+then\n+\tskip_all=\"skipping python$python_target_version-specific git p4 tests; python$python_target_version not available\"\n+\ttest_done\n+fi\n+\n+remove_user_cache () {\n+\trm \"$HOME/.gitp4-usercache.txt\" || true\n+}\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'init depot' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\tp4_add_user \"utf8_author\" \"ǣuthor\" &&\n+\t\tP4USER=utf8_author &&\n+\t\ttouch file1 &&\n+\t\tp4 add file1 &&\n+\t\tp4 submit -d \"first CL has some utf-8 tǣxt\" &&\n+\n+\t\tp4_add_user \"latin1_author\" \"$(echo æuthor |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\t\tP4USER=latin1_author &&\n+\t\ttouch file2 &&\n+\t\tp4 add file2 &&\n+\t\tp4 submit -d \"$(echo second CL has some latin-1 tæxt |\n+\t\t\ticonv -f utf8 -t latin1)\" &&\n+\n+\t\tp4_add_user \"cp1252_author\" \"$(echo æuthœr |\n+\t\t\ticonv -f utf8 -t cp1252)\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file3 &&\n+\t\tp4 add file3 &&\n+\t\tp4 submit -d \"$(echo third CL has sœme cp-1252 tæxt |\n+\t\t iconv -f utf8 -t cp1252)\" &&\n+\n+\t\tp4_add_user \"cp850_author\" \"$(echo Åuthor |\n+\t\t\ticonv -f utf8 -t cp850)\" &&\n+\t\tP4USER=cp850_author &&\n+\t\ttouch file4 &&\n+\t\tp4 add file4 &&\n+\t\tp4 submit -d \"$(echo fourth CL hÅs some cp850 text |\n+\t\t\ticonv -f utf8 -t cp850)\"\n+\t)\n+'\n+\n+test_expect_success 'clone non-utf8 repo with strict encoding' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\ttest_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest=\"$git\" //depot@all 2>err &&\n+\tgrep \"Decoding perforce metadata failed!\" err\n+'\n+\n+test_expect_success 'check utf-8 contents with passthrough strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents corrupted in git with passthrough strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tbadly_encoded_in_git=$(echo \"some latin-1 tæxt\" | iconv -f utf8 -t latin1) &&\n+\t\tgrep \"$badly_encoded_in_git\" actual &&\n+\t\tbad_author_in_git=\"$(echo æuthor | iconv -f utf8 -t latin1)\" &&\n+\t\tgrep \"$bad_author_in_git\" actual\n+\t)\n+'\n+\n+test_expect_success 'check utf-8 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some utf-8 tǣxt\" actual &&\n+\t\tgrep \"ǣuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check latin-1 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"some latin-1 tæxt\" actual &&\n+\t\tgrep \"æuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"sœme cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp850 contents parsed with correct fallback' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback -c git-p4.metadataFallbackEncoding=cp850 p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"hÅs some cp850 text\" actual &&\n+\t\tgrep \"Åuthor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp850-only contents escaped when cp1252 is fallback' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"h%8Fs some cp850 text\" actual &&\n+\t\tgrep \"%8Futhor\" actual\n+\t)\n+'\n+\n+test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tP4USER=cp1252_author &&\n+\t\ttouch file10 &&\n+\t\tp4 add file10 &&\n+\t\tp4 submit -d \"$(echo later CL has sœme more cp-1252 tæxt |\n+\t\t\ticonv -f utf8 -t cp1252)\"\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\n+\t\tgit p4.py sync --branch=master &&\n+\n+\t\tgit log p4/master >actual &&\n+\t\tgrep \"sœme more cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+############################\n+## / END REPEATED SECTION ##\n+############################\n+\n+\n+test_expect_success 'fallback (both utf-8 and cp-1252 contents handled) is the default with python3' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_when_finished remove_user_cache &&\n+\tgit p4.py clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit log >actual &&\n+\t\tgrep \"sœme cp-1252 tæxt\" actual &&\n+\t\tgrep \"æuthœr\" actual\n+\t)\n+'\n+\n+test_done\n\nbase-commit: 0f828332d5ac36fc63b7d8202652efa152809856\n-- \ngitgitgadget\n"}]}