{"thread":{"id":"58122","subject":"[PATCH] git-p4: fix bug with encoding of p4 client name","startedAt":"2022-07-08T08:01:09Z","lastAt":"2022-07-21T16:46:32Z","messageCount":9,"participants":["Kilian Kilger via GitGitGadget","Tao Klerks","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"458626","messageId":"pull.1285.git.git.1657267260405.gitgitgadget@gmail.com","threadId":"58122","inReplyTo":null,"subject":"[PATCH] git-p4: fix bug with encoding of p4 client name","fromName":"Kilian Kilger via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-08T08:01:00Z","receivedAt":"2022-07-08T08:01:09Z","isPatch":true,"sender":{"key":"name:Kilian Kilger","avatar":null},"body":"From: Kilian Kilger \n\nThe Perforce client name can contain arbitrary characters\nwhich do not decode to UTF-8. Use the fallback strategy\nimplemented in metadata_stream_to_writable_bytes() also\nfor the client name.\n\nSigned-off-by: Kilian Kilger \n---\n git-p4: Fix bug with encoding of P4 client name\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1285%2Fcohomology%2Fmaint-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1285/cohomology/maint-v1\nPull-Request: https://github.com/git/git/pull/1285\n\n git-p4.py | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8fbf6eb1fe3..e65d6a2b0e1 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -854,12 +854,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- # - `desc` or `FullName` which may contain non-UTF8 encoded text handled below, eagerly converted to bytes\n+ # - `desc` or `client` 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', 'desc', 'FullName', 'path', 'clientFile') or key.startswith('depotFile')):\n+ if isinstance(value, bytes) and not (key in ('data', 'desc', 'FullName', 'path', 'clientFile', 'client') or key.startswith('depotFile')):\n value = value.decode()\n decoded_entry[key] = value\n # Parse out data if it's an error response\n@@ -871,6 +871,8 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n continue\n if 'desc' in entry:\n entry['desc'] = metadata_stream_to_writable_bytes(entry['desc'])\n+ if 'client' in entry:\n+ entry['client'] = metadata_stream_to_writable_bytes(entry['client'])\n if 'FullName' in entry:\n entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName'])\n if cb is not None:\n\nbase-commit: e4a4b31577c7419497ac30cebe30d755b97752c5\n-- \ngitgitgadget\n"},{"id":"458630","messageId":"CAPMMpohwP1U4pr-G=fDDu_AHTY-2mn8hn8zBf-pAsmm=w0mSzw@mail.gmail.com","threadId":"58122","inReplyTo":"pull.1285.git.git.1657267260405.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-p4: fix bug with encoding of p4 client name","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-07-08T11:28:51Z","receivedAt":"2022-07-08T11:29:11Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"This makes sense to me, and I don't see anything wrong with the \"form\"\n(and nor does GitGitGadget).\n\nNot sure whether formal review sign-off is used on this list, I don't\ntend to see it, but do I see \"Reviewed-by\" on patches, so FWIW:\n\nReviewed-by: Tao Klerks \n\n\nOn Fri, Jul 8, 2022 at 10:01 AM Kilian Kilger via GitGitGadget\n wrote:\n>\n> From: Kilian Kilger \n>\n> The Perforce client name can contain arbitrary characters\n> which do not decode to UTF-8. Use the fallback strategy\n> implemented in metadata_stream_to_writable_bytes() also\n> for the client name.\n>\n> Signed-off-by: Kilian Kilger \n> ---\n> git-p4: Fix bug with encoding of P4 client name\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1285%2Fcohomology%2Fmaint-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1285/cohomology/maint-v1\n> Pull-Request: https://github.com/git/git/pull/1285\n>\n> git-p4.py | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 8fbf6eb1fe3..e65d6a2b0e1 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -854,12 +854,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> - # - `desc` or `FullName` which may contain non-UTF8 encoded text handled below, eagerly converted to bytes\n> + # - `desc` or `client` 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', 'desc', 'FullName', 'path', 'clientFile') or key.startswith('depotFile')):\n> + if isinstance(value, bytes) and not (key in ('data', 'desc', 'FullName', 'path', 'clientFile', 'client') or key.startswith('depotFile')):\n> value = value.decode()\n> decoded_entry[key] = value\n> # Parse out data if it's an error response\n> @@ -871,6 +871,8 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n> continue\n> if 'desc' in entry:\n> entry['desc'] = metadata_stream_to_writable_bytes(entry['desc'])\n> + if 'client' in entry:\n> + entry['client'] = metadata_stream_to_writable_bytes(entry['client'])\n> if 'FullName' in entry:\n> entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName'])\n> if cb is not None:\n>\n> base-commit: e4a4b31577c7419497ac30cebe30d755b97752c5\n> --\n> gitgitgadget\n"},{"id":"458641","messageId":"xmqqmtdjwspm.fsf@gitster.g","threadId":"58122","inReplyTo":"CAPMMpohwP1U4pr-G=fDDu_AHTY-2mn8hn8zBf-pAsmm=w0mSzw@mail.gmail.com","subject":"Re: [PATCH] git-p4: fix bug with encoding of p4 client name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-08T15:05:09Z","receivedAt":"2022-07-08T15:05:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks writes:\n\n>\n>\n> On Fri, Jul 8, 2022 at 10:01 AM Kilian Kilger via GitGitGadget\n> wrote:\n>>\n>> From: Kilian Kilger \n>>\n>> The Perforce client name can contain arbitrary characters\n>> which do not decode to UTF-8. Use the fallback strategy\n>> implemented in metadata_stream_to_writable_bytes() also\n>> for the client name.\n>>\n>> Signed-off-by: Kilian Kilger \n>> ---\n>> ...\n>>\n>> @@ -871,6 +871,8 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n>> continue\n>> if 'desc' in entry:\n>> entry['desc'] = metadata_stream_to_writable_bytes(entry['desc'])\n>> + if 'client' in entry:\n>> + entry['client'] = metadata_stream_to_writable_bytes(entry['client'])\n>> if 'FullName' in entry:\n>> entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName']\n\nWe had two repetitions and now we have three, which is a good time\nto see if it makes sense to reduce the temptation for future\ndevelopers to add the fourth repetition in the next round, e.g.\n\n\tfor e in [\"client\", \"desc\", \"FullName\"]:\n\t\tif e in entry:\n\t\t\tentry[e] = metadata_stream_to_writable_bytes(entry[e])\n\nor something like that?\n\n> This makes sense to me, and I don't see anything wrong with the \"form\"\n> (and nor does GitGitGadget).\n\nOne thing that is a bit problematic is that in-body From does not\nmatch the sign-off. Kilian, which identity do you want to use in\nyour contribution to this project? \n\n> Not sure whether formal review sign-off is used on this list, I don't\n> tend to see it, but do I see \"Reviewed-by\" on patches, so FWIW:\n>\n> Reviewed-by: Tao Klerks \n\nThanks.\n\n"},{"id":"459244","messageId":"pull.1285.v2.git.git.1658134679233.gitgitgadget@gmail.com","threadId":"58122","inReplyTo":"pull.1285.git.git.1657267260405.gitgitgadget@gmail.com","subject":"[PATCH v2] git-p4: fix bug with encoding of p4 client name","fromName":"Kilian Kilger via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-18T08:57:59Z","receivedAt":"2022-07-18T08:58:09Z","isPatch":true,"sender":{"key":"name:Kilian Kilger","avatar":null},"body":"From: Kilian Kilger \n\nThe Perforce client name can contain arbitrary characters\nwhich do not decode to UTF-8. Use the fallback strategy\nimplemented in metadata_stream_to_writable_bytes() also\nfor the client name.\n\nSigned-off-by: Kilian Kilger \n---\n git-p4: Fix bug with encoding of P4 client name\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1285%2Fcohomology%2Fmaint-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1285/cohomology/maint-v2\nPull-Request: https://github.com/git/git/pull/1285\n\nRange-diff vs v1:\n\n 1: 7393b59c642 ! 1: 3280a9579bc git-p4: fix bug with encoding of p4 client name\n @@\n ## Metadata ##\n -Author: Kilian Kilger \n +Author: Kilian Kilger \n \n ## Commit message ##\n git-p4: fix bug with encoding of p4 client name\n @@ Commit message\n Signed-off-by: Kilian Kilger \n \n ## git-p4.py ##\n +@@ git-p4.py: def isModeExecChanged(src_mode, dst_mode):\n + return isModeExec(src_mode) != isModeExec(dst_mode)\n + \n + \n ++def p4KeysContainingNonUtf8Chars():\n ++ \"\"\"Returns all keys which may contain non UTF-8 encoded strings\n ++ for which a fallback strategy has to be applied.\n ++ \"\"\"\n ++ return ['desc', 'client', 'FullName']\n ++\n ++\n ++def p4KeysContainingBinaryData():\n ++ \"\"\"Returns all keys which may contain arbitrary binary data\n ++ \"\"\"\n ++ return ['data']\n ++\n ++\n ++def p4KeyContainsFilePaths(key):\n ++ \"\"\"Returns True if the key contains file paths. These are handled by decode_path().\n ++ Otherwise False.\n ++ \"\"\"\n ++ return key.startswith('depotFile') or key in ['path', 'clientFile']\n ++\n ++\n ++def p4KeyWhichCanBeDirectlyDecoded(key):\n ++ \"\"\"Returns True if the key can be directly decoded as UTF-8 string\n ++ Otherwise False.\n ++\n ++ Keys which can not be encoded directly:\n ++ - `data` which may contain arbitrary binary data\n ++ - `desc` or `client` or `FullName` which may contain non-UTF8 encoded text\n ++ - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text, handled by decode_path()\n ++ \"\"\"\n ++ if key in p4KeysContainingNonUtf8Chars() or \\\n ++ key in p4KeysContainingBinaryData() or \\\n ++ p4KeyContainsFilePaths(key):\n ++ return False\n ++ return True\n ++\n ++\n + def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n + errors_as_exceptions=False, *k, **kw):\n + \n @@ git-p4.py: def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n + try:\n + while True:\n + entry = marshal.load(p4.stdout)\n ++\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 +- # Decode unmarshalled dict to use str keys and values, except for:\n +- # - `data` which may contain arbitrary binary data\n - # - `desc` or `FullName` which may contain non-UTF8 encoded text handled below, eagerly converted to bytes\n -+ # - `desc` or `client` 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 +- # - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text, handled by decode_path()\n ++ # Decode unmarshalled dict to use str keys and values. Special cases are handled below.\n decoded_entry = {}\n for key, value in entry.items():\n key = key.decode()\n - if isinstance(value, bytes) and not (key in ('data', 'desc', 'FullName', 'path', 'clientFile') or key.startswith('depotFile')):\n -+ if isinstance(value, bytes) and not (key in ('data', 'desc', 'FullName', 'path', 'clientFile', 'client') or key.startswith('depotFile')):\n ++ if isinstance(value, bytes) and p4KeyWhichCanBeDirectlyDecoded(key):\n value = value.decode()\n decoded_entry[key] = value\n # Parse out data if it's an error response\n @@ git-p4.py: 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 'client' in entry:\n -+ entry['client'] = metadata_stream_to_writable_bytes(entry['client'])\n - if 'FullName' in entry:\n - entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName'])\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 ++ for key in p4KeysContainingNonUtf8Chars():\n ++ if key in entry:\n ++ entry[key] = metadata_stream_to_writable_bytes(entry[key])\n if cb is not None:\n + cb(entry)\n + else:\n\n\n git-p4.py | 51 ++++++++++++++++++++++++++++++++++++++++++---------\n 1 file changed, 42 insertions(+), 9 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8fbf6eb1fe3..9323b943c68 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -822,6 +822,42 @@ def isModeExecChanged(src_mode, dst_mode):\n return isModeExec(src_mode) != isModeExec(dst_mode)\n \n \n+def p4KeysContainingNonUtf8Chars():\n+ \"\"\"Returns all keys which may contain non UTF-8 encoded strings\n+ for which a fallback strategy has to be applied.\n+ \"\"\"\n+ return ['desc', 'client', 'FullName']\n+\n+\n+def p4KeysContainingBinaryData():\n+ \"\"\"Returns all keys which may contain arbitrary binary data\n+ \"\"\"\n+ return ['data']\n+\n+\n+def p4KeyContainsFilePaths(key):\n+ \"\"\"Returns True if the key contains file paths. These are handled by decode_path().\n+ Otherwise False.\n+ \"\"\"\n+ return key.startswith('depotFile') or key in ['path', 'clientFile']\n+\n+\n+def p4KeyWhichCanBeDirectlyDecoded(key):\n+ \"\"\"Returns True if the key can be directly decoded as UTF-8 string\n+ Otherwise False.\n+\n+ Keys which can not be encoded directly:\n+ - `data` which may contain arbitrary binary data\n+ - `desc` or `client` or `FullName` which may contain non-UTF8 encoded text\n+ - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text, handled by decode_path()\n+ \"\"\"\n+ if key in p4KeysContainingNonUtf8Chars() or \\\n+ key in p4KeysContainingBinaryData() or \\\n+ p4KeyContainsFilePaths(key):\n+ return False\n+ return True\n+\n+\n def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n errors_as_exceptions=False, *k, **kw):\n \n@@ -851,15 +887,13 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n try:\n while True:\n entry = marshal.load(p4.stdout)\n+\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- # - `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+ # Decode unmarshalled dict to use str keys and values. Special cases are handled below.\n decoded_entry = {}\n for key, value in entry.items():\n key = key.decode()\n- if isinstance(value, bytes) and not (key in ('data', 'desc', 'FullName', 'path', 'clientFile') or key.startswith('depotFile')):\n+ if isinstance(value, bytes) and p4KeyWhichCanBeDirectlyDecoded(key):\n value = value.decode()\n decoded_entry[key] = value\n # Parse out data if it's an error response\n@@ -869,10 +903,9 @@ 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+ for key in p4KeysContainingNonUtf8Chars():\n+ if key in entry:\n+ entry[key] = metadata_stream_to_writable_bytes(entry[key])\n if cb is not None:\n cb(entry)\n else:\n\nbase-commit: e4a4b31577c7419497ac30cebe30d755b97752c5\n-- \ngitgitgadget\n"},{"id":"459287","messageId":"xmqqcze2s7h0.fsf@gitster.g","threadId":"58122","inReplyTo":"pull.1285.v2.git.git.1658134679233.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] git-p4: fix bug with encoding of p4 client name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-18T16:36:59Z","receivedAt":"2022-07-18T16:38:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kilian Kilger via GitGitGadget\" writes:\n\n> From: Kilian Kilger \n>\n> The Perforce client name can contain arbitrary characters\n> which do not decode to UTF-8. Use the fallback strategy\n> implemented in metadata_stream_to_writable_bytes() also\n> for the client name.\n>\n> Signed-off-by: Kilian Kilger \n> ---\n> git-p4: Fix bug with encoding of P4 client name\n\nSorry, is this to improve the topic that has already last Monday?\n\nIf so, it is way too late to update with a replacement patch, like\nthis. Instead, please send in a follow-up patch on top of what has\nalready been merged.\n\nThe description above seems to be identical to what was in the\nprevious round, but the patch seems to use a lot more code, so\nsuch a follow-up patch would have plenty to explain how the new\nstrategy improves over the previous one.\n\nThanks.\n\n"},{"id":"459602","messageId":"pull.1285.v3.git.git.1658394440.gitgitgadget@gmail.com","threadId":"58122","inReplyTo":"pull.1285.v2.git.git.1658134679233.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] git-p4: Fix bug with encoding of P4 client name","fromName":"Kilian Kilger via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-21T09:07:18Z","receivedAt":"2022-07-21T09:07:29Z","isPatch":true,"sender":{"key":"name:Kilian Kilger","avatar":null},"body":"Kilian Kilger (2):\n git-p4: fix bug with encoding of p4 client name\n git-p4: refactoring of p4CmdList()\n\n git-p4.py | 51 ++++++++++++++++++++++++++++++++++++++++++---------\n 1 file changed, 42 insertions(+), 9 deletions(-)\n\n\nbase-commit: e4a4b31577c7419497ac30cebe30d755b97752c5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1285%2Fcohomology%2Fmaint-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1285/cohomology/maint-v3\nPull-Request: https://github.com/git/git/pull/1285\n\nRange-diff vs v2:\n\n -: ----------- > 1: 87e7809b75a git-p4: fix bug with encoding of p4 client name\n 1: 3280a9579bc ! 2: 4a81423f0e8 git-p4: fix bug with encoding of p4 client name\n @@ Metadata\n Author: Kilian Kilger \n \n ## Commit message ##\n - git-p4: fix bug with encoding of p4 client name\n + git-p4: refactoring of p4CmdList()\n \n - The Perforce client name can contain arbitrary characters\n - which do not decode to UTF-8. Use the fallback strategy\n - implemented in metadata_stream_to_writable_bytes() also\n - for the client name.\n + The function p4CmdList executes a Perforce command and\n + decodes the marshalled python dictionary. Special care has to be\n + taken for certain dictionary values which contain non-unicode characters.\n + The old handling contained separate hacks for each of the corresponding\n + dictionary keys. This commit tries to refactor the coding to handle the\n + special cases uniformely.\n \n Signed-off-by: Kilian Kilger \n \n @@ git-p4.py: def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=F\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 -- # - `desc` or `FullName` which may contain non-UTF8 encoded text handled below, eagerly converted to bytes\n +- # - `desc` or `client` 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 + # Decode unmarshalled dict to use str keys and values. Special cases are handled below.\n decoded_entry = {}\n for key, value in entry.items():\n key = key.decode()\n -- if isinstance(value, bytes) and not (key in ('data', 'desc', 'FullName', 'path', 'clientFile') or key.startswith('depotFile')):\n +- if isinstance(value, bytes) and not (key in ('data', 'desc', 'FullName', 'path', 'clientFile', 'client') or key.startswith('depotFile')):\n + if isinstance(value, bytes) and p4KeyWhichCanBeDirectlyDecoded(key):\n value = value.decode()\n decoded_entry[key] = value\n @@ git-p4.py: def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=F\n continue\n - if 'desc' in entry:\n - entry['desc'] = metadata_stream_to_writable_bytes(entry['desc'])\n +- if 'client' in entry:\n +- entry['client'] = metadata_stream_to_writable_bytes(entry['client'])\n - if 'FullName' in entry:\n - entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName'])\n + for key in p4KeysContainingNonUtf8Chars():\n\n-- \ngitgitgadget\n"},{"id":"459603","messageId":"87e7809b75acedc5f099bf79c6d3544e06fe2c61.1658394440.git.gitgitgadget@gmail.com","threadId":"58122","inReplyTo":"pull.1285.v3.git.git.1658394440.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] git-p4: fix bug with encoding of p4 client name","fromName":"Kilian Kilger via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-21T09:07:19Z","receivedAt":"2022-07-21T09:07:31Z","isPatch":true,"sender":{"key":"name:Kilian Kilger","avatar":null},"body":"From: Kilian Kilger \n\nThe Perforce client name can contain arbitrary characters\nwhich do not decode to UTF-8. Use the fallback strategy\nimplemented in metadata_stream_to_writable_bytes() also\nfor the client name.\n\nSigned-off-by: Kilian Kilger \nReviewed-by: Tao Klerks \nSigned-off-by: Junio C Hamano \n---\n git-p4.py | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8fbf6eb1fe3..e65d6a2b0e1 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -854,12 +854,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- # - `desc` or `FullName` which may contain non-UTF8 encoded text handled below, eagerly converted to bytes\n+ # - `desc` or `client` 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', 'desc', 'FullName', 'path', 'clientFile') or key.startswith('depotFile')):\n+ if isinstance(value, bytes) and not (key in ('data', 'desc', 'FullName', 'path', 'clientFile', 'client') or key.startswith('depotFile')):\n value = value.decode()\n decoded_entry[key] = value\n # Parse out data if it's an error response\n@@ -871,6 +871,8 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n continue\n if 'desc' in entry:\n entry['desc'] = metadata_stream_to_writable_bytes(entry['desc'])\n+ if 'client' in entry:\n+ entry['client'] = metadata_stream_to_writable_bytes(entry['client'])\n if 'FullName' in entry:\n entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName'])\n if cb is not None:\n-- \ngitgitgadget\n\n"},{"id":"459604","messageId":"4a81423f0e86764ee3c90cfff98f60a07b40a82c.1658394440.git.gitgitgadget@gmail.com","threadId":"58122","inReplyTo":"pull.1285.v3.git.git.1658394440.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] git-p4: refactoring of p4CmdList()","fromName":"Kilian Kilger via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-21T09:07:20Z","receivedAt":"2022-07-21T09:07:32Z","isPatch":true,"sender":{"key":"name:Kilian Kilger","avatar":null},"body":"From: Kilian Kilger \n\nThe function p4CmdList executes a Perforce command and\ndecodes the marshalled python dictionary. Special care has to be\ntaken for certain dictionary values which contain non-unicode characters.\nThe old handling contained separate hacks for each of the corresponding\ndictionary keys. This commit tries to refactor the coding to handle the\nspecial cases uniformely.\n\nSigned-off-by: Kilian Kilger \n---\n git-p4.py | 53 ++++++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 42 insertions(+), 11 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex e65d6a2b0e1..9323b943c68 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -822,6 +822,42 @@ def isModeExecChanged(src_mode, dst_mode):\n return isModeExec(src_mode) != isModeExec(dst_mode)\n \n \n+def p4KeysContainingNonUtf8Chars():\n+ \"\"\"Returns all keys which may contain non UTF-8 encoded strings\n+ for which a fallback strategy has to be applied.\n+ \"\"\"\n+ return ['desc', 'client', 'FullName']\n+\n+\n+def p4KeysContainingBinaryData():\n+ \"\"\"Returns all keys which may contain arbitrary binary data\n+ \"\"\"\n+ return ['data']\n+\n+\n+def p4KeyContainsFilePaths(key):\n+ \"\"\"Returns True if the key contains file paths. These are handled by decode_path().\n+ Otherwise False.\n+ \"\"\"\n+ return key.startswith('depotFile') or key in ['path', 'clientFile']\n+\n+\n+def p4KeyWhichCanBeDirectlyDecoded(key):\n+ \"\"\"Returns True if the key can be directly decoded as UTF-8 string\n+ Otherwise False.\n+\n+ Keys which can not be encoded directly:\n+ - `data` which may contain arbitrary binary data\n+ - `desc` or `client` or `FullName` which may contain non-UTF8 encoded text\n+ - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text, handled by decode_path()\n+ \"\"\"\n+ if key in p4KeysContainingNonUtf8Chars() or \\\n+ key in p4KeysContainingBinaryData() or \\\n+ p4KeyContainsFilePaths(key):\n+ return False\n+ return True\n+\n+\n def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n errors_as_exceptions=False, *k, **kw):\n \n@@ -851,15 +887,13 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n try:\n while True:\n entry = marshal.load(p4.stdout)\n+\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- # - `desc` or `client` 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+ # Decode unmarshalled dict to use str keys and values. Special cases are handled below.\n decoded_entry = {}\n for key, value in entry.items():\n key = key.decode()\n- if isinstance(value, bytes) and not (key in ('data', 'desc', 'FullName', 'path', 'clientFile', 'client') or key.startswith('depotFile')):\n+ if isinstance(value, bytes) and p4KeyWhichCanBeDirectlyDecoded(key):\n value = value.decode()\n decoded_entry[key] = value\n # Parse out data if it's an error response\n@@ -869,12 +903,9 @@ 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 'client' in entry:\n- entry['client'] = metadata_stream_to_writable_bytes(entry['client'])\n- if 'FullName' in entry:\n- entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName'])\n+ for key in p4KeysContainingNonUtf8Chars():\n+ if key in entry:\n+ entry[key] = metadata_stream_to_writable_bytes(entry[key])\n if cb is not None:\n cb(entry)\n else:\n-- \ngitgitgadget\n"},{"id":"459659","messageId":"xmqqr12ejtwe.fsf@gitster.g","threadId":"58122","inReplyTo":"pull.1285.v3.git.git.1658394440.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/2] git-p4: Fix bug with encoding of P4 client name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-21T16:46:25Z","receivedAt":"2022-07-21T16:46:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kilian Kilger via GitGitGadget\" writes:\n\n> Kilian Kilger (2):\n> git-p4: fix bug with encoding of p4 client name\n> git-p4: refactoring of p4CmdList()\n\n[PATCH v3 1/2] seems to be identical to 34f67c96 (git-p4: fix bug\nwith encoding of p4 client name, 2022-07-08) that was already merged\nto 'next' yesterday, so I can safely ignore it and take [PATCH v3\n2/2] as if it were a single follow-up patch on the same topic and\nqueue it on top.\n\nThanks.\n"}]}