{"thread":{"id":"55456","subject":"git-p4 crashes on non UTF-8 output from p4","startedAt":"2021-04-08T19:28:41Z","lastAt":"2021-04-29T17:29:54Z","messageCount":24,"participants":["Tzadik Vanderhoof","Torsten Bögershausen","Eric Sunshine","Junio C Hamano","Luke Diamand"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"421256","messageId":"CAKu1iLXtwuCQTS0s7_LEm0OJF-4s0UhPhDW1r5Zb7=GsSPfpdQ@mail.gmail.com","threadId":"55456","inReplyTo":null,"subject":"git-p4 crashes on non UTF-8 output from p4","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-08T19:28:25Z","receivedAt":"2021-04-08T19:28:41Z","isPatch":false,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"When git-p4 reads the output from a p4 command, it assumes it will be\n100% UTF-8. If even one character in the output of one p4 command is\nnot UTF-8, git-p4 crashes with:\n\nFile \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\ndecode byte Ox93 in position 42: invalid start byte\n\nI'd like to make a pull request to have it try another encoding (eg\ncp1252) and/or use the Unicode replacement character, to prevent the\nwhole program from crashing on such a minor problem.\n\nThis is especially a problem on the \"git p4 clone\" command with @all,\nwhere git-p4 needs to read thousands of changeset descriptions, one of\nwhich may have a stray smart quote, causing the whole clone operation\nto fail.\n\nSound ok?\n"},{"id":"421409","messageId":"20210409153815.7joohvmlnh6itczc@tb-raspi4","threadId":"55456","inReplyTo":"CAKu1iLXtwuCQTS0s7_LEm0OJF-4s0UhPhDW1r5Zb7=GsSPfpdQ@mail.gmail.com","subject":"Re: git-p4 crashes on non UTF-8 output from p4","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-04-09T15:38:16Z","receivedAt":"2021-04-09T15:38:22Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Thu, Apr 08, 2021 at 12:28:25PM -0700, Tzadik Vanderhoof wrote:\n> When git-p4 reads the output from a p4 command, it assumes it will be\n> 100% UTF-8. If even one character in the output of one p4 command is\n> not UTF-8, git-p4 crashes with:\n>\n> File \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n> value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\n> decode byte Ox93 in position 42: invalid start byte\n>\n> I'd like to make a pull request to have it try another encoding (eg\n> cp1252) and/or use the Unicode replacement character, to prevent the\n> whole program from crashing on such a minor problem.\n>\n> This is especially a problem on the \"git p4 clone\" command with @all,\n> where git-p4 needs to read thousands of changeset descriptions, one of\n> which may have a stray smart quote, causing the whole clone operation\n> to fail.\n>\n> Sound ok?\n\nWelcome to the Git community.\nTo start with: I am not a git-p4 expert as such, but seeing that a program is crashing\nis never a good thing.\nAll efforts to prevent the crash are a step forward.\n\nAs you mention cp1252 (which is more used under Windows), there are probably lots of\nsystem out there which use ISO-8859-15 (or ISO-8859-1) we may have the first whish:\n\nMake the encoding/fallback configurable.\nLet people choose if they want a crash (if things are broken),\nfallback to cp1252 or one of the other ISO-ISO-8859-x encodings.\n\nIn that sense: we look forward to a pull-request.\n"},{"id":"421569","messageId":"CAKu1iLX1AyTCSGxDVgiR1cr4=4ODD-gn8jHAinhp7OhDChAf1A@mail.gmail.com","threadId":"55456","inReplyTo":"20210409153815.7joohvmlnh6itczc@tb-raspi4","subject":"Re: git-p4 crashes on non UTF-8 output from p4","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-11T07:16:25Z","receivedAt":"2021-04-11T07:16:41Z","isPatch":false,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"Here is the pull request:\n\nFrom 8d234af842223dceae76ce0affd3bbb3f17bb6d9 Mon Sep 17 00:00:00 2001\nFrom: Tzadik Vanderhoof \nDate: Sat, 10 Apr 2021 22:41:39 -0700\nSubject: [PATCH] add git-p4.fallbackEncoding config variable, to prevent\n git-p4 from crashing on non UTF-8 changeset descriptions\n\n---\n Documentation/git-p4.txt | 10 ++++++++++\n git-p4.py | 11 ++++++++++-\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex f89e68b..71f3487 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -638,6 +638,16 @@ git-p4.pathEncoding::\n to transcode the paths to UTF-8. As an example, Perforce on Windows\n often uses \"cp1252\" to encode path names.\n\n+git-p4.fallbackEncoding::\n+ Perforce changeset descriptions can be in a mixture of encodings. Git-p4\n+ first tries to interpret each description as UTF-8. If that fails, this\n+ config allows another encoding to be tried. The default is \"cp1252\". You\n+ can set it to another encoding, for example, \"iso-8859-5\". If instead of\n+ an encoding, you specify \"replace\", UTF-8 will be used, with invalid UTF-8\n+ characters replaced by the Unicode replacement character. If you specify\n+ \"none\", there is no fallback, and any non UTF-8 character will cause\n+ git-p4 to immediately fail.\n+\n git-p4.largeFileSystem::\n Specify the system that is used for large (binary) files. Please note\n that large file systems do not support the 'git p4 submit' command.\ndiff --git a/git-p4.py b/git-p4.py\nindex 09c9e93..18d02b4 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -771,7 +771,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b',\ncb=None, skip_info=False,\n for key, value in entry.items():\n key = key.decode()\n if isinstance(value, bytes) and not (key in\n('data', 'path', 'clientFile') or key.startswith('depotFile')):\n- value = value.decode()\n+ try:\n+ value = value.decode()\n+ except:\n+ fallbackEncoding =\ngitConfig(\"git-p4.fallbackEncoding\").lower() or 'cp1252'\n+ if fallbackEncoding == 'none':\n+ raise\n+ elif fallbackEncoding == 'replace':\n+ value = value.decode(errors='replace')\n+ else:\n+ value = value.decode(encoding=fallbackEncoding)\n decoded_entry[key] = value\n # Parse out data if it's an error response\n if decoded_entry.get('code') == 'error' and 'data' in\ndecoded_entry:\n-- \n2.31.1.windows.1\n\nOn Fri, Apr 9, 2021 at 8:38 AM Torsten Bögershausen wrote:\n>\n> On Thu, Apr 08, 2021 at 12:28:25PM -0700, Tzadik Vanderhoof wrote:\n> > When git-p4 reads the output from a p4 command, it assumes it will be\n> > 100% UTF-8. If even one character in the output of one p4 command is\n> > not UTF-8, git-p4 crashes with:\n> >\n> > File \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n> > value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\n> > decode byte Ox93 in position 42: invalid start byte\n> >\n> > I'd like to make a pull request to have it try another encoding (eg\n> > cp1252) and/or use the Unicode replacement character, to prevent the\n> > whole program from crashing on such a minor problem.\n> >\n> > This is especially a problem on the \"git p4 clone\" command with @all,\n> > where git-p4 needs to read thousands of changeset descriptions, one of\n> > which may have a stray smart quote, causing the whole clone operation\n> > to fail.\n> >\n> > Sound ok?\n>\n> Welcome to the Git community.\n> To start with: I am not a git-p4 expert as such, but seeing that a program is crashing\n> is never a good thing.\n> All efforts to prevent the crash are a step forward.\n>\n> As you mention cp1252 (which is more used under Windows), there are probably lots of\n> system out there which use ISO-8859-15 (or ISO-8859-1) we may have the first whish:\n>\n> Make the encoding/fallback configurable.\n> Let people choose if they want a crash (if things are broken),\n> fallback to cp1252 or one of the other ISO-ISO-8859-x encodings.\n>\n> In that sense: we look forward to a pull-request.\n\n\n\n-- \nTzadik\n"},{"id":"421575","messageId":"20210411093746.ymqofe2uawclwu5i@tb-raspi4","threadId":"55456","inReplyTo":"CAKu1iLX1AyTCSGxDVgiR1cr4=4ODD-gn8jHAinhp7OhDChAf1A@mail.gmail.com","subject":"Re: git-p4 crashes on non UTF-8 output from p4","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-04-11T09:37:46Z","receivedAt":"2021-04-11T09:37:51Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sun, Apr 11, 2021 at 12:16:25AM -0700, Tzadik Vanderhoof wrote:\n> Here is the pull request:\n\nThanks for the work. Some comments inline.\n\n>\n> From 8d234af842223dceae76ce0affd3bbb3f17bb6d9 Mon Sep 17 00:00:00 2001\n> From: Tzadik Vanderhoof \n> Date: Sat, 10 Apr 2021 22:41:39 -0700\n\n\nThe subject should be one short line, highlighting what this is all about,\nfollowed by a blank line and a longer description about the problem and\nthe solution. The original description was good, see below.\n\n> Subject: [PATCH] add git-p4.fallbackEncoding config variable, to prevent\n> git-p4 from crashing on non UTF-8 changeset descriptions\n\nIn that sense I make a first trial here, subject for improvements:\n\n\nSubject: [PATCH] Add git-p4.fallbackEncoding config variable\n\nWhen git-p4 reads the output from a p4 command, it assumes it will be\n100% UTF-8. If even one character in the output of one p4 command is\nnot UTF-8, git-p4 crashes e.g. with:\n\nFile \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\ndecode byte Ox93 in position 42: invalid start byte\n\nAllow to try another encoding (eg cp1252) and/or use the\nUnicode replacement character to prevent the whole program from crashing\non such a \"minor\" problem.\n\nThis is especially a problem on the \"git p4 clone\" command with @all,\nwhere git-p4 needs to read thousands of changeset descriptions, one of\nwhich may have a stray smart quote, causing the whole clone operation\nto fail.\n\nIntroduce \"git-p4.fallbackEncoding\" to handle non UTF-8 encodings, if needed.\n\n>\n> ---\n> Documentation/git-p4.txt | 10 ++++++++++\n> git-p4.py | 11 ++++++++++-\n> 2 files changed, 20 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n> index f89e68b..71f3487 100644\n> --- a/Documentation/git-p4.txt\n> +++ b/Documentation/git-p4.txt\n> @@ -638,6 +638,16 @@ git-p4.pathEncoding::\n> to transcode the paths to UTF-8. As an example, Perforce on Windows\n> often uses \"cp1252\" to encode path names.\n>\n> +git-p4.fallbackEncoding::\n> + Perforce changeset descriptions can be in a mixture of encodings. Git-p4\n> + first tries to interpret each description as UTF-8. If that fails, this\n> + config allows another encoding to be tried. The default is \"cp1252\". You\n\nI know that cp1252 is attractive to be used, especially for Windows installations that\nuse Latin-based \"characters\".\nBut: If we introduce a new config-variable into Git, the default tends to be\n\"if not set to anything, behave as the old Git\".\n\n> + can set it to another encoding, for example, \"iso-8859-5\". If instead of\nISO-8859-5 may be more portable on the different i18 implementations\nthan the lower-case spelling.\n\n> + an encoding, you specify \"replace\", UTF-8 will be used, with invalid UTF-8\n> + characters replaced by the Unicode replacement character. If you specify\n> + \"none\", there is no fallback, and any non UTF-8 character will cause\n> + git-p4 to immediately fail.\n\nAs said, before, many people may expect Git to fail, so that the default should be\nnone to avoid surprises.\nWhen a \"non-UTF-8-clean\" repo is handled, they want to know it.\n\n> +\n> git-p4.largeFileSystem::\n> Specify the system that is used for large (binary) files. Please note\n> that large file systems do not support the 'git p4 submit' command.\n> diff --git a/git-p4.py b/git-p4.py\n> index 09c9e93..18d02b4 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -771,7 +771,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b',\n> cb=None, skip_info=False,\n> for key, value in entry.items():\n> key = key.decode()\n> if isinstance(value, bytes) and not (key in\n> ('data', 'path', 'clientFile') or key.startswith('depotFile')):\n> - value = value.decode()\n> + try:\n> + value = value.decode()\n> + except:\n> + fallbackEncoding =\n> gitConfig(\"git-p4.fallbackEncoding\").lower() or 'cp1252'\n> + if fallbackEncoding == 'none':\n> + raise\n\nWould it make sense to tell the user about the new config value here?\n raise Exception(\"Non UTF-8 detected. See git-p4.fallbackEncoding\"\nOr somewhat in that style ?\n\n> + elif fallbackEncoding == 'replace':\n> + value = value.decode(errors='replace')\n> + else:\n> + value = value.decode(encoding=fallbackEncoding)\n> decoded_entry[key] = value\n> # Parse out data if it's an error response\n> if decoded_entry.get('code') == 'error' and 'data' in\n> decoded_entry:\n\n\nDid I miss the Signed-off-by here?\n\nPlease have a look here:\nhttps://git-scm.com/docs/SubmittingPatches\n\n(or look at Documentation/SubmittingPatches in your git source code)\n\nAnd finally: Thanks for the contribution.\nIs there any chance to add test-cases, to make sure that this feature\nis well-tested now and in the future ?\n\n\n> --\n> 2.31.1.windows.1\n>\n> On Fri, Apr 9, 2021 at 8:38 AM Torsten Bögershausen wrote:\n> >\n> > On Thu, Apr 08, 2021 at 12:28:25PM -0700, Tzadik Vanderhoof wrote:\n> > > When git-p4 reads the output from a p4 command, it assumes it will be\n> > > 100% UTF-8. If even one character in the output of one p4 command is\n> > > not UTF-8, git-p4 crashes with:\n> > >\n> > > File \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n> > > value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\n> > > decode byte Ox93 in position 42: invalid start byte\n> > >\n> > > I'd like to make a pull request to have it try another encoding (eg\n> > > cp1252) and/or use the Unicode replacement character, to prevent the\n> > > whole program from crashing on such a minor problem.\n> > >\n> > > This is especially a problem on the \"git p4 clone\" command with @all,\n> > > where git-p4 needs to read thousands of changeset descriptions, one of\n> > > which may have a stray smart quote, causing the whole clone operation\n> > > to fail.\n> > >\n> > > Sound ok?\n> >\n> > Welcome to the Git community.\n> > To start with: I am not a git-p4 expert as such, but seeing that a program is crashing\n> > is never a good thing.\n> > All efforts to prevent the crash are a step forward.\n> >\n> > As you mention cp1252 (which is more used under Windows), there are probably lots of\n> > system out there which use ISO-8859-15 (or ISO-8859-1) we may have the first whish:\n> >\n> > Make the encoding/fallback configurable.\n> > Let people choose if they want a crash (if things are broken),\n> > fallback to cp1252 or one of the other ISO-ISO-8859-x encodings.\n> >\n> > In that sense: we look forward to a pull-request.\n>\n>\n>\n> --\n> Tzadik\n"},{"id":"421617","messageId":"CAKu1iLUNooP+FDMJKekH4b2Cq5BhFZwAb=d28iPv55C5+cQbCg@mail.gmail.com","threadId":"55456","inReplyTo":"20210411093746.ymqofe2uawclwu5i@tb-raspi4","subject":"Re: git-p4 crashes on non UTF-8 output from p4","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-11T20:21:47Z","receivedAt":"2021-04-11T20:22:01Z","isPatch":false,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"Thank you for your excellent and friendly feedback!\n\nI understand everything you said, but I have a question about the unit\ntest you requested. The git-p4.py script currently does not have\ntests and is not written in a way that would be testable. (The Python\nfunction I modified calls into the shell and requires a valid Perforce\ninstallation.)\n\nWould you prefer I a) refactor the code to be testable and then write\ntests or b) skip the unit testing (not sure if there are any further\noptions)?\n\n(For option a) I would break out the part of the function I modified\ninto another function and then call my new function for my testing.\n(I guess it would be better to break the refactoring and my changes\ninto 2 separate commits.)\n\nOn Sun, Apr 11, 2021 at 2:37 AM Torsten Bögershausen wrote:\n>\n> On Sun, Apr 11, 2021 at 12:16:25AM -0700, Tzadik Vanderhoof wrote:\n> > Here is the pull request:\n>\n> Thanks for the work. Some comments inline.\n>\n> >\n> > From 8d234af842223dceae76ce0affd3bbb3f17bb6d9 Mon Sep 17 00:00:00 2001\n> > From: Tzadik Vanderhoof \n> > Date: Sat, 10 Apr 2021 22:41:39 -0700\n>\n>\n> The subject should be one short line, highlighting what this is all about,\n> followed by a blank line and a longer description about the problem and\n> the solution. The original description was good, see below.\n>\n> > Subject: [PATCH] add git-p4.fallbackEncoding config variable, to prevent\n> > git-p4 from crashing on non UTF-8 changeset descriptions\n>\n> In that sense I make a first trial here, subject for improvements:\n>\n>\n> Subject: [PATCH] Add git-p4.fallbackEncoding config variable\n>\n> When git-p4 reads the output from a p4 command, it assumes it will be\n> 100% UTF-8. If even one character in the output of one p4 command is\n> not UTF-8, git-p4 crashes e.g. with:\n>\n> File \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n> value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\n> decode byte Ox93 in position 42: invalid start byte\n>\n> Allow to try another encoding (eg cp1252) and/or use the\n> Unicode replacement character to prevent the whole program from crashing\n> on such a \"minor\" problem.\n>\n> This is especially a problem on the \"git p4 clone\" command with @all,\n> where git-p4 needs to read thousands of changeset descriptions, one of\n> which may have a stray smart quote, causing the whole clone operation\n> to fail.\n>\n> Introduce \"git-p4.fallbackEncoding\" to handle non UTF-8 encodings, if needed.\n>\n> >\n> > ---\n> > Documentation/git-p4.txt | 10 ++++++++++\n> > git-p4.py | 11 ++++++++++-\n> > 2 files changed, 20 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n> > index f89e68b..71f3487 100644\n> > --- a/Documentation/git-p4.txt\n> > +++ b/Documentation/git-p4.txt\n> > @@ -638,6 +638,16 @@ git-p4.pathEncoding::\n> > to transcode the paths to UTF-8. As an example, Perforce on Windows\n> > often uses \"cp1252\" to encode path names.\n> >\n> > +git-p4.fallbackEncoding::\n> > + Perforce changeset descriptions can be in a mixture of encodings. Git-p4\n> > + first tries to interpret each description as UTF-8. If that fails, this\n> > + config allows another encoding to be tried. The default is \"cp1252\". You\n>\n> I know that cp1252 is attractive to be used, especially for Windows installations that\n> use Latin-based \"characters\".\n> But: If we introduce a new config-variable into Git, the default tends to be\n> \"if not set to anything, behave as the old Git\".\n>\n> > + can set it to another encoding, for example, \"iso-8859-5\". If instead of\n> ISO-8859-5 may be more portable on the different i18 implementations\n> than the lower-case spelling.\n>\n> > + an encoding, you specify \"replace\", UTF-8 will be used, with invalid UTF-8\n> > + characters replaced by the Unicode replacement character. If you specify\n> > + \"none\", there is no fallback, and any non UTF-8 character will cause\n> > + git-p4 to immediately fail.\n>\n> As said, before, many people may expect Git to fail, so that the default should be\n> none to avoid surprises.\n> When a \"non-UTF-8-clean\" repo is handled, they want to know it.\n>\n> > +\n> > git-p4.largeFileSystem::\n> > Specify the system that is used for large (binary) files. Please note\n> > that large file systems do not support the 'git p4 submit' command.\n> > diff --git a/git-p4.py b/git-p4.py\n> > index 09c9e93..18d02b4 100755\n> > --- a/git-p4.py\n> > +++ b/git-p4.py\n> > @@ -771,7 +771,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b',\n> > cb=None, skip_info=False,\n> > for key, value in entry.items():\n> > key = key.decode()\n> > if isinstance(value, bytes) and not (key in\n> > ('data', 'path', 'clientFile') or key.startswith('depotFile')):\n> > - value = value.decode()\n> > + try:\n> > + value = value.decode()\n> > + except:\n> > + fallbackEncoding =\n> > gitConfig(\"git-p4.fallbackEncoding\").lower() or 'cp1252'\n> > + if fallbackEncoding == 'none':\n> > + raise\n>\n> Would it make sense to tell the user about the new config value here?\n> raise Exception(\"Non UTF-8 detected. See git-p4.fallbackEncoding\"\n> Or somewhat in that style ?\n>\n> > + elif fallbackEncoding == 'replace':\n> > + value = value.decode(errors='replace')\n> > + else:\n> > + value = value.decode(encoding=fallbackEncoding)\n> > decoded_entry[key] = value\n> > # Parse out data if it's an error response\n> > if decoded_entry.get('code') == 'error' and 'data' in\n> > decoded_entry:\n>\n>\n> Did I miss the Signed-off-by here?\n>\n> Please have a look here:\n> https://git-scm.com/docs/SubmittingPatches\n>\n> (or look at Documentation/SubmittingPatches in your git source code)\n>\n> And finally: Thanks for the contribution.\n> Is there any chance to add test-cases, to make sure that this feature\n> is well-tested now and in the future ?\n>\n>\n> > --\n> > 2.31.1.windows.1\n> >\n> > On Fri, Apr 9, 2021 at 8:38 AM Torsten Bögershausen wrote:\n> > >\n> > > On Thu, Apr 08, 2021 at 12:28:25PM -0700, Tzadik Vanderhoof wrote:\n> > > > When git-p4 reads the output from a p4 command, it assumes it will be\n> > > > 100% UTF-8. If even one character in the output of one p4 command is\n> > > > not UTF-8, git-p4 crashes with:\n> > > >\n> > > > File \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n> > > > value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\n> > > > decode byte Ox93 in position 42: invalid start byte\n> > > >\n> > > > I'd like to make a pull request to have it try another encoding (eg\n> > > > cp1252) and/or use the Unicode replacement character, to prevent the\n> > > > whole program from crashing on such a minor problem.\n> > > >\n> > > > This is especially a problem on the \"git p4 clone\" command with @all,\n> > > > where git-p4 needs to read thousands of changeset descriptions, one of\n> > > > which may have a stray smart quote, causing the whole clone operation\n> > > > to fail.\n> > > >\n> > > > Sound ok?\n> > >\n> > > Welcome to the Git community.\n> > > To start with: I am not a git-p4 expert as such, but seeing that a program is crashing\n> > > is never a good thing.\n> > > All efforts to prevent the crash are a step forward.\n> > >\n> > > As you mention cp1252 (which is more used under Windows), there are probably lots of\n> > > system out there which use ISO-8859-15 (or ISO-8859-1) we may have the first whish:\n> > >\n> > > Make the encoding/fallback configurable.\n> > > Let people choose if they want a crash (if things are broken),\n> > > fallback to cp1252 or one of the other ISO-ISO-8859-x encodings.\n> > >\n> > > In that sense: we look forward to a pull-request.\n> >\n> >\n> >\n> > --\n> > Tzadik\n\n\n\n-- \nTzadik\n"},{"id":"421636","messageId":"20210412040614.gqiot5qcsfpiae3a@tb-raspi4","threadId":"55456","inReplyTo":"CAKu1iLUNooP+FDMJKekH4b2Cq5BhFZwAb=d28iPv55C5+cQbCg@mail.gmail.com","subject":"Re: git-p4 crashes on non UTF-8 output from p4","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-04-12T04:06:14Z","receivedAt":"2021-04-12T04:06:22Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\nHej Tzadik,\n\nLet's start with a side note:\nThis mailing list doesn't use top-posting, everything new is at the end,\nso I moved everything there.\n\nAnd removed some of the old stuff.\n\n> On Sun, Apr 11, 2021 at 2:37 AM Torsten Bögershausen wrote:\n> >\n> > On Sun, Apr 11, 2021 at 12:16:25AM -0700, Tzadik Vanderhoof wrote:\n> > > Here is the pull request:\n> >\n> > Thanks for the work. Some comments inline.\n> >\n> > >\n> > > From 8d234af842223dceae76ce0affd3bbb3f17bb6d9 Mon Sep 17 00:00:00 2001\n> > > From: Tzadik Vanderhoof \n> > > Date: Sat, 10 Apr 2021 22:41:39 -0700\n> >\n> >\n> > The subject should be one short line, highlighting what this is all about,\n> > followed by a blank line and a longer description about the problem and\n> > the solution. The original description was good, see below.\n> >\n> > > Subject: [PATCH] add git-p4.fallbackEncoding config variable, to prevent\n> > > git-p4 from crashing on non UTF-8 changeset descriptions\n> >\n> > In that sense I make a first trial here, subject for improvements:\n> >\n> >\n> > Subject: [PATCH] Add git-p4.fallbackEncoding config variable\n> >\n> > When git-p4 reads the output from a p4 command, it assumes it will be\n> > 100% UTF-8. If even one character in the output of one p4 command is\n> > not UTF-8, git-p4 crashes e.g. with:\n> >\n> > File \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n> > value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\n> > decode byte Ox93 in position 42: invalid start byte\n> >\n> > Allow to try another encoding (eg cp1252) and/or use the\n> > Unicode replacement character to prevent the whole program from crashing\n> > on such a \"minor\" problem.\n> >\n> > This is especially a problem on the \"git p4 clone\" command with @all,\n> > where git-p4 needs to read thousands of changeset descriptions, one of\n> > which may have a stray smart quote, causing the whole clone operation\n> > to fail.\n> >\n> > Introduce \"git-p4.fallbackEncoding\" to handle non UTF-8 encodings, if needed.\n> >\n> > >\n> > > ---\n> > > Documentation/git-p4.txt | 10 ++++++++++\n> > > git-p4.py | 11 ++++++++++-\n> > > 2 files changed, 20 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n> > > index f89e68b..71f3487 100644\n> > > --- a/Documentation/git-p4.txt\n> > > +++ b/Documentation/git-p4.txt\n> > > @@ -638,6 +638,16 @@ git-p4.pathEncoding::\n> > > to transcode the paths to UTF-8. As an example, Perforce on Windows\n> > > often uses \"cp1252\" to encode path names.\n> > >\n> > > +git-p4.fallbackEncoding::\n> > > + Perforce changeset descriptions can be in a mixture of encodings. Git-p4\n> > > + first tries to interpret each description as UTF-8. If that fails, this\n> > > + config allows another encoding to be tried. The default is \"cp1252\". You\n> >\n> > I know that cp1252 is attractive to be used, especially for Windows installations that\n> > use Latin-based \"characters\".\n> > But: If we introduce a new config-variable into Git, the default tends to be\n> > \"if not set to anything, behave as the old Git\".\n> >\n> > > + can set it to another encoding, for example, \"iso-8859-5\". If instead of\n> > ISO-8859-5 may be more portable on the different i18 implementations\n> > than the lower-case spelling.\n> >\n> > > + an encoding, you specify \"replace\", UTF-8 will be used, with invalid UTF-8\n> > > + characters replaced by the Unicode replacement character. If you specify\n> > > + \"none\", there is no fallback, and any non UTF-8 character will cause\n> > > + git-p4 to immediately fail.\n> >\n> > As said, before, many people may expect Git to fail, so that the default should be\n> > none to avoid surprises.\n> > When a \"non-UTF-8-clean\" repo is handled, they want to know it.\n> >\n> > > +\n> > > git-p4.largeFileSystem::\n> > > Specify the system that is used for large (binary) files. Please note\n> > > that large file systems do not support the 'git p4 submit' command.\n> > > diff --git a/git-p4.py b/git-p4.py\n> > > index 09c9e93..18d02b4 100755\n> > > --- a/git-p4.py\n> > > +++ b/git-p4.py\n> > > @@ -771,7 +771,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b',\n> > > cb=None, skip_info=False,\n> > > for key, value in entry.items():\n> > > key = key.decode()\n> > > if isinstance(value, bytes) and not (key in\n> > > ('data', 'path', 'clientFile') or key.startswith('depotFile')):\n> > > - value = value.decode()\n> > > + try:\n> > > + value = value.decode()\n> > > + except:\n> > > + fallbackEncoding =\n> > > gitConfig(\"git-p4.fallbackEncoding\").lower() or 'cp1252'\n> > > + if fallbackEncoding == 'none':\n> > > + raise\n> >\n> > Would it make sense to tell the user about the new config value here?\n> > raise Exception(\"Non UTF-8 detected. See git-p4.fallbackEncoding\"\n> > Or somewhat in that style ?\n> >\n> > > + elif fallbackEncoding == 'replace':\n> > > + value = value.decode(errors='replace')\n> > > + else:\n> > > + value = value.decode(encoding=fallbackEncoding)\n> > > decoded_entry[key] = value\n> > > # Parse out data if it's an error response\n> > > if decoded_entry.get('code') == 'error' and 'data' in\n> > > decoded_entry:\n> >\n> >\n> > Did I miss the Signed-off-by here?\n> >\n> > Please have a look here:\n> > https://git-scm.com/docs/SubmittingPatches\n> >\n> > (or look at Documentation/SubmittingPatches in your git source code)\n> >\n> > And finally: Thanks for the contribution.\n> > Is there any chance to add test-cases, to make sure that this feature\n> > is well-tested now and in the future ?\n> >\n[snip]\n\n\nOn Sun, Apr 11, 2021 at 01:21:47PM -0700, Tzadik Vanderhoof wrote:\n> Thank you for your excellent and friendly feedback!\n>\n> I understand everything you said, but I have a question about the unit\n> test you requested. The git-p4.py script currently does not have\n> tests and is not written in a way that would be testable. (The Python\n> function I modified calls into the shell and requires a valid Perforce\n> installation.)\n\nI am not really sure about that (there are no test cases).\nA valid Perforce installation is needed, yes. Otherwise the p4 tests are skipped.\nThere are a lot of p4 tests under t/t98..p4....sh\n\ngit show a8b05162e894b88aeb7d5064dab\n\nTells me e.g. that both git-p4.py and, in this very commit,\nt/t9822-git-p4-path-encoding.sh have been changed.\n\nAll the test are t98xx-git-p4-something.sh (fiund under t),\nand you new testcase may be named something like\n\nt9835-git-p4-fallbackEncoding.sh\n\n>\n> Would you prefer I a) refactor the code to be testable and then write\n> tests or b) skip the unit testing (not sure if there are any further\n> options)?\n>\n> (For option a) I would break out the part of the function I modified\n> into another function and then call my new function for my testing.\n> (I guess it would be better to break the refactoring and my changes\n> into 2 separate commits.)\n>\n\nProbably not. Typically we can construct a test case first,\nand see that ot fails (in you local test-running).\nAfter updating git-p4.py with your improvments the new test should pass.\nAnd of course all the other p4 tests.\n\nThere is a whole bunch of \"CI tests\", run on Linux, MacOs, Windows.\nOne example of a test run is here, and the \"regular (linux-clang,...)\nis running the p4 tests:\n\nhttps://github.com/git/git/runs/2309995044?check_suite_focus=true\n"},{"id":"422548","messageId":"20210421084604.3095-1-tzadik.vanderhoof@gmail.com","threadId":"55456","inReplyTo":"20210412040614.gqiot5qcsfpiae3a@tb-raspi4","subject":"[PATCH] add git-p4.fallbackEncoding config variable, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-21T08:46:04Z","receivedAt":"2021-04-21T08:46:46Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"---\n Documentation/git-p4.txt | 10 ++++\n git-p4.py | 11 +++-\n t/t9835-git-p4-config-fallback-encoding.sh | 65 ++++++++++++++++++++++\n 3 files changed, 85 insertions(+), 1 deletion(-)\n create mode 100755 t/t9835-git-p4-config-fallback-encoding.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex f89e68b..e0131a9 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -638,6 +638,16 @@ git-p4.pathEncoding::\n \tto transcode the paths to UTF-8. As an example, Perforce on Windows\n \toften uses \"cp1252\" to encode path names.\n \n+git-p4.fallbackEncoding::\n+\tPerforce changeset descriptions can be in a mixture of encodings.\n+\tGit-p4 first tries to interpret each description as UTF-8. If that\n+\tfails, this config allows another encoding to be tried. You\n+\tcan specify, for example, \"cp1252\". If instead of an encoding,\n+\tyou specify \"replace\", UTF-8 will be used, with invalid UTF-8\n+\tcharacters replaced by the Unicode replacement character. If you\n+\tspecify \"none\" (the default), there is no fallback, and any non\n+\tUTF-8 character will cause git-p4 to immediately fail.\n+\n git-p4.largeFileSystem::\n \tSpecify the system that is used for large (binary) files. Please note\n \tthat large file systems do not support the 'git p4 submit' command.\ndiff --git a/git-p4.py b/git-p4.py\nindex 09c9e93..173f78a 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -771,7 +771,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\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- value = value.decode()\n+ try:\n+ value = value.decode()\n+ except UnicodeDecodeError as ex:\n+ fallbackEncoding = gitConfig(\"git-p4.fallbackEncoding\").lower() or 'none'\n+ if fallbackEncoding == 'none':\n+ raise Exception(\"UTF8 decoding failed. Consider using git config git-p4.fallbackEncoding\") from ex\n+ elif fallbackEncoding == 'replace':\n+ value = value.decode(errors='replace')\n+ else:\n+ value = value.decode(encoding=fallbackEncoding)\n decoded_entry[key] = value\n # Parse out data if it's an error response\n if decoded_entry.get('code') == 'error' and 'data' in decoded_entry:\ndiff --git a/t/t9835-git-p4-config-fallback-encoding.sh b/t/t9835-git-p4-config-fallback-encoding.sh\nnew file mode 100755\nindex 0000000..56a245e\n--- /dev/null\n+++ b/t/t9835-git-p4-config-fallback-encoding.sh\n@@ -0,0 +1,65 @@\n+#!/bin/sh\n+\n+test_description='test git-p4.fallbackEncoding config'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./lib-git-p4.sh\n+\n+if test_have_prereq !MINGW,!CYGWIN; then\n+\tskip_all='This system is not subject to encoding failures in \"git p4 clone\"'\n+\ttest_done\n+fi\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'add cp1252 description' '\n+\tcd \"$cli\" &&\n+\techo file1 >file1 &&\n+\tp4 add file1 &&\n+\tp4 submit -d documentación\n+'\n+\n+test_expect_success 'clone fails with git-p4.fallbackEncoding unset' '\n+\ttest_might_fail git config --global --unset git-p4.fallbackEncoding &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\ttest_must_fail git p4 clone --dest=\"$git\" //depot@all 2>> actual &&\n+\t\tgrep \"UTF8 decoding failed. Consider using git config git-p4.fallbackEncoding\" actual\n+\t)\n+'\n+test_expect_success 'clone fails with git-p4.fallbackEncoding set to \"none\"' '\n+\tgit config --global git-p4.fallbackEncoding none &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\ttest_must_fail git p4 clone --dest=\"$git\" //depot@all 2>> actual &&\n+\t\tgrep \"UTF8 decoding failed. Consider using git config git-p4.fallbackEncoding\" actual\n+\t)\n+'\n+\n+test_expect_success 'clone succeeds with git-p4.fallbackEncoding set to \"cp1252\"' '\n+\tgit config --global git-p4.fallbackEncoding cp1252 &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t\tcd \"$git\" &&\n+\t\tgit log --oneline >log &&\n+\t\tdesc=$(head -1 log | awk '\\''{print $2}'\\'') &&\t[ \"$desc\" = \"documentación\" ]\n+\t)\n+'\n+\n+test_expect_success 'clone succeeds with git-p4.fallbackEncoding set to \"replace\"' '\n+\tgit config --global git-p4.fallbackEncoding replace &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t\tcd \"$git\" &&\n+\t\tgit log --oneline >log &&\n+\t\tdesc=$(head -1 log | awk '\\''{print $2}'\\'') &&\t[ \"$desc\" = \"documentaci�n\" ]\n+\t)\n+'\n+\n+test_done\n-- \n2.31.1.windows.1\n\n"},{"id":"422549","messageId":"CAKu1iLWfaAaKH4Uui4wfa0STFEaXqqtc304b5V0ZNtmBg78J+w@mail.gmail.com","threadId":"55456","inReplyTo":"20210421084604.3095-1-tzadik.vanderhoof@gmail.com","subject":"Re: [PATCH] add git-p4.fallbackEncoding config variable, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-21T08:55:57Z","receivedAt":"2021-04-21T08:56:16Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"Signed-off-by: Tzadik Vanderhoof \n"},{"id":"422664","messageId":"20210422050504.441-1-tzadik.vanderhoof@gmail.com","threadId":"55456","inReplyTo":"CAKu1iLWfaAaKH4Uui4wfa0STFEaXqqtc304b5V0ZNtmBg78J+w@mail.gmail.com","subject":"[PATCH v3] add git-p4.fallbackEncoding config setting, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-22T05:05:04Z","receivedAt":"2021-04-22T05:09:11Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"When git-p4 reads the output from a p4 command, it assumes it will be\n100% UTF-8. If even one character in the output of one p4 command is\nnot UTF-8, git-p4 crashes with:\n\nFile \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\ndecode byte Ox93 in position 42: invalid start byte\n\nThis is especially a problem on the \"git p4 clone ... @all\" command,\nwhere git-p4 needs to read thousands of changeset descriptions, one of\nwhich may have a stray smart quote, causing the whole clone operation\nto fail.\n\nThis pull request adds a new config setting, allowing git-p4 to try\nanother encoding (for example, \"cp1252\") and/or use the Unicode replacement\ncharacter, to prevent the whole program from crashing on such a minor problem.\n\nSee the documentation included in the patch for more details of how\nthe new config setting works.\n\nSigned-off-by: Tzadik Vanderhoof \n---\n Documentation/git-p4.txt | 10 ++++\n git-p4.py | 11 +++-\n t/t9835-git-p4-config-fallback-encoding.sh | 65 ++++++++++++++++++++++\n 3 files changed, 85 insertions(+), 1 deletion(-)\n create mode 100755 t/t9835-git-p4-config-fallback-encoding.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex f89e68b..e0131a9 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -638,6 +638,16 @@ git-p4.pathEncoding::\n \tto transcode the paths to UTF-8. As an example, Perforce on Windows\n \toften uses \"cp1252\" to encode path names.\n \n+git-p4.fallbackEncoding::\n+\tPerforce changeset descriptions can be in a mixture of encodings.\n+\tGit-p4 first tries to interpret each description as UTF-8. If that\n+\tfails, this config allows another encoding to be tried. You\n+\tcan specify, for example, \"cp1252\". If instead of an encoding,\n+\tyou specify \"replace\", UTF-8 will be used, with invalid UTF-8\n+\tcharacters replaced by the Unicode replacement character. If you\n+\tspecify \"none\" (the default), there is no fallback, and any non\n+\tUTF-8 character will cause git-p4 to immediately fail.\n+\n git-p4.largeFileSystem::\n \tSpecify the system that is used for large (binary) files. Please note\n \tthat large file systems do not support the 'git p4 submit' command.\ndiff --git a/git-p4.py b/git-p4.py\nindex 09c9e93..3364287 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -771,7 +771,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\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- value = value.decode()\n+ try:\n+ value = value.decode()\n+ except UnicodeDecodeError as ex:\n+ fallbackEncoding = gitConfig(\"git-p4.fallbackEncoding\").lower() or 'none'\n+ if fallbackEncoding == 'none':\n+ raise Exception(\"UTF8 decoding failed. Consider using git config git-p4.fallbackEncoding\")\n+ elif fallbackEncoding == 'replace':\n+ value = value.decode(errors='replace')\n+ else:\n+ value = value.decode(encoding=fallbackEncoding)\n decoded_entry[key] = value\n # Parse out data if it's an error response\n if decoded_entry.get('code') == 'error' and 'data' in decoded_entry:\ndiff --git a/t/t9835-git-p4-config-fallback-encoding.sh b/t/t9835-git-p4-config-fallback-encoding.sh\nnew file mode 100755\nindex 0000000..56a245e\n--- /dev/null\n+++ b/t/t9835-git-p4-config-fallback-encoding.sh\n@@ -0,0 +1,65 @@\n+#!/bin/sh\n+\n+test_description='test git-p4.fallbackEncoding config'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./lib-git-p4.sh\n+\n+if test_have_prereq !MINGW,!CYGWIN; then\n+\tskip_all='This system is not subject to encoding failures in \"git p4 clone\"'\n+\ttest_done\n+fi\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'add cp1252 description' '\n+\tcd \"$cli\" &&\n+\techo file1 >file1 &&\n+\tp4 add file1 &&\n+\tp4 submit -d documentación\n+'\n+\n+test_expect_success 'clone fails with git-p4.fallbackEncoding unset' '\n+\ttest_might_fail git config --global --unset git-p4.fallbackEncoding &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\ttest_must_fail git p4 clone --dest=\"$git\" //depot@all 2>> actual &&\n+\t\tgrep \"UTF8 decoding failed. Consider using git config git-p4.fallbackEncoding\" actual\n+\t)\n+'\n+test_expect_success 'clone fails with git-p4.fallbackEncoding set to \"none\"' '\n+\tgit config --global git-p4.fallbackEncoding none &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\ttest_must_fail git p4 clone --dest=\"$git\" //depot@all 2>> actual &&\n+\t\tgrep \"UTF8 decoding failed. Consider using git config git-p4.fallbackEncoding\" actual\n+\t)\n+'\n+\n+test_expect_success 'clone succeeds with git-p4.fallbackEncoding set to \"cp1252\"' '\n+\tgit config --global git-p4.fallbackEncoding cp1252 &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t\tcd \"$git\" &&\n+\t\tgit log --oneline >log &&\n+\t\tdesc=$(head -1 log | awk '\\''{print $2}'\\'') &&\t[ \"$desc\" = \"documentación\" ]\n+\t)\n+'\n+\n+test_expect_success 'clone succeeds with git-p4.fallbackEncoding set to \"replace\"' '\n+\tgit config --global git-p4.fallbackEncoding replace &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t\tcd \"$git\" &&\n+\t\tgit log --oneline >log &&\n+\t\tdesc=$(head -1 log | awk '\\''{print $2}'\\'') &&\t[ \"$desc\" = \"documentaci�n\" ]\n+\t)\n+'\n+\n+test_done\n-- \n2.31.1.windows.1\n\n"},{"id":"422693","messageId":"20210422155047.3unltvv3mh5uq7wp@tb-raspi4","threadId":"55456","inReplyTo":"20210422050504.441-1-tzadik.vanderhoof@gmail.com","subject":"Re: [PATCH v3] add git-p4.fallbackEncoding config setting, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-04-22T15:50:47Z","receivedAt":"2021-04-22T15:50:51Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Wed, Apr 21, 2021 at 10:05:04PM -0700, Tzadik Vanderhoof wrote:\n\nThanks for V3, please see some comments inline.\n\n> When git-p4 reads the output from a p4 command, it assumes it will be\n> 100% UTF-8. If even one character in the output of one p4 command is\n> not UTF-8, git-p4 crashes with:\n>\n> File \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n> value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\n> decode byte Ox93 in position 42: invalid start byte\n>\n> This is especially a problem on the \"git p4 clone ... @all\" command,\n> where git-p4 needs to read thousands of changeset descriptions, one of\n> which may have a stray smart quote, causing the whole clone operation\n> to fail.\n>\n\n> This pull request adds a new config setting, allowing git-p4 to try\n> another encoding (for example, \"cp1252\") and/or use the Unicode replacement\n> character, to prevent the whole program from crashing on such a minor problem.\n\n\"This pull request\" is somewhat superflous wording.\nHow about:\n\nAdd a new config setting, allowing git-p4 to try a fallback encoding\n(for example, \"cp1252\") and/or use the Unicode replacement character,\nto prevent the whole program from crashing on such a minor problem.\n\nDocumentation is good (and needed, and neccessary).\nProbably this is then not needed:\n> See the documentation included in the patch for more details of how\n> the new config setting works.\n\n>\n> Signed-off-by: Tzadik Vanderhoof \n> ---\n> Documentation/git-p4.txt | 10 ++++\n> git-p4.py | 11 +++-\n> t/t9835-git-p4-config-fallback-encoding.sh | 65 ++++++++++++++++++++++\n> 3 files changed, 85 insertions(+), 1 deletion(-)\n> create mode 100755 t/t9835-git-p4-config-fallback-encoding.sh\n>\n> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n> index f89e68b..e0131a9 100644\n> --- a/Documentation/git-p4.txt\n> +++ b/Documentation/git-p4.txt\n> @@ -638,6 +638,16 @@ git-p4.pathEncoding::\n> \tto transcode the paths to UTF-8. As an example, Perforce on Windows\n> \toften uses \"cp1252\" to encode path names.\n>\n> +git-p4.fallbackEncoding::\n> +\tPerforce changeset descriptions can be in a mixture of encodings.\n> +\tGit-p4 first tries to interpret each description as UTF-8. If that\n> +\tfails, this config allows another encoding to be tried. You\n> +\tcan specify, for example, \"cp1252\".\n\nThat looks OK according to\nhttps://docs.python.org/3/library/codecs.html#standard-encodings\n\n> + If instead of an encoding,\n> +\tyou specify \"replace\", UTF-8 will be used, with invalid UTF-8\n> +\tcharacters replaced by the Unicode replacement character. If you\n> +\tspecify \"none\" (the default), there is no fallback, and any non\n> +\tUTF-8 character will cause git-p4 to immediately fail.\n> +\n\nMay be, that is a matter of taste:\n\n> + If git-p4.fallbackEncoding is \"replace\" \", UTF-8 will be used, with invalid UTF-8\n> +\tcharacters replaced by the Unicode replacement character.\n> +\tThe default is \"none\": there is no fallback, and any non\n> +\tUTF-8 character will cause git-p4 to immediately fail.\n> +\n\n\n\n> git-p4.largeFileSystem::\n> \tSpecify the system that is used for large (binary) files. Please note\n> \tthat large file systems do not support the 'git p4 submit' command.\n> diff --git a/git-p4.py b/git-p4.py\n> index 09c9e93..3364287 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -771,7 +771,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\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> - value = value.decode()\n> + try:\n> + value = value.decode()\n> + except UnicodeDecodeError as ex:\n> + fallbackEncoding = gitConfig(\"git-p4.fallbackEncoding\").lower() or 'none'\n> + if fallbackEncoding == 'none':\n> + raise Exception(\"UTF8 decoding failed. Consider using git config git-p4.fallbackEncoding\")\n> + elif fallbackEncoding == 'replace':\n> + value = value.decode(errors='replace')\n> + else:\n> + value = value.decode(encoding=fallbackEncoding)\n> decoded_entry[key] = value\n> # Parse out data if it's an error response\n> if decoded_entry.get('code') == 'error' and 'data' in decoded_entry:\n> diff --git a/t/t9835-git-p4-config-fallback-encoding.sh b/t/t9835-git-p4-config-fallback-encoding.sh\n> new file mode 100755\n> index 0000000..56a245e\n> --- /dev/null\n> +++ b/t/t9835-git-p4-config-fallback-encoding.sh\n> @@ -0,0 +1,65 @@\n> +#!/bin/sh\n> +\n> +test_description='test git-p4.fallbackEncoding config'\n> +\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n> +. ./lib-git-p4.sh\n> +\n> +if test_have_prereq !MINGW,!CYGWIN; then\n> +\tskip_all='This system is not subject to encoding failures in \"git p4 clone\"'\n> +\ttest_done\n> +fi\n\nOut of curiosity: Why are Windows versions (MINGW, CYGWIN) excluded ?\n\n> +\n> +test_expect_success 'start p4d' '\n> +\tstart_p4d\n> +'\n> +\n> +test_expect_success 'add cp1252 description' '\n> +\tcd \"$cli\" &&\n> +\techo file1 >file1 &&\n> +\tp4 add file1 &&\n> +\tp4 submit -d documentación\n> +'\n> +\n> +test_expect_success 'clone fails with git-p4.fallbackEncoding unset' '\n> +\ttest_might_fail git config --global --unset git-p4.fallbackEncoding &&\n> +\ttest_when_finished cleanup_git &&\n> +\t(\n> +\t\ttest_must_fail git p4 clone --dest=\"$git\" //depot@all 2>> actual &&\nShould this be >actual ?\nAnd please no ' ' between the '>' and the filename.\n\n> +\t\tgrep \"UTF8 decoding failed. Consider using git config git-p4.fallbackEncoding\" actual\n> +\t)\n> +'\n> +test_expect_success 'clone fails with git-p4.fallbackEncoding set to \"none\"' '\n> +\tgit config --global git-p4.fallbackEncoding none &&\n> +\ttest_when_finished cleanup_git &&\n> +\t(\n> +\t\ttest_must_fail git p4 clone --dest=\"$git\" //depot@all 2>> actual &&\nSame here\n2 >actual\n\n> +\t\tgrep \"UTF8 decoding failed. Consider using git config git-p4.fallbackEncoding\" actual\n> +\t)\n> +'\n> +\n> +test_expect_success 'clone succeeds with git-p4.fallbackEncoding set to \"cp1252\"' '\n> +\tgit config --global git-p4.fallbackEncoding cp1252 &&\n> +\ttest_when_finished cleanup_git &&\n> +\t(\n> +\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n> +\t\tcd \"$git\" &&\n> +\t\tgit log --oneline >log &&\n> +\t\tdesc=$(head -1 log | awk '\\''{print $2}'\\'') &&\t[ \"$desc\" = \"documentación\" ]\n\nStyle nit:\nSee Documentation/CodingGuidelines: - We prefer \"test\" over \"[ ... ]\".\n\ndesc=$(head -1 log | awk '\\''{print $2}'\\'') &&\ttest \"$desc\" = \"documentación\"\n\n> +\t)\n> +'\n> +\n> +test_expect_success 'clone succeeds with git-p4.fallbackEncoding set to \"replace\"' '\n> +\tgit config --global git-p4.fallbackEncoding replace &&\n> +\ttest_when_finished cleanup_git &&\n> +\t(\n> +\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n> +\t\tcd \"$git\" &&\n> +\t\tgit log --oneline >log &&\n> +\t\tdesc=$(head -1 log | awk '\\''{print $2}'\\'') &&\t[ \"$desc\" = \"documentaci�n\" ]\n> +\t)\n> +'\n> +\n> +test_done\n> --\n> 2.31.1.windows.1\n>\n\nAre there any more comments from the p4 experts ?\n"},{"id":"422694","messageId":"CAPig+cQE0oHHY89D6fLyymduY3=zSe8y246cz1P2MjTZhrMHNQ@mail.gmail.com","threadId":"55456","inReplyTo":"20210422155047.3unltvv3mh5uq7wp@tb-raspi4","subject":"Re: [PATCH v3] add git-p4.fallbackEncoding config setting, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-04-22T16:17:04Z","receivedAt":"2021-04-22T16:17:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Apr 22, 2021 at 11:51 AM Torsten Bögershausen wrote:\n> On Wed, Apr 21, 2021 at 10:05:04PM -0700, Tzadik Vanderhoof wrote:\n> > +if test_have_prereq !MINGW,!CYGWIN; then\n> > + skip_all='This system is not subject to encoding failures in \"git p4 clone\"'\n> > + test_done\n> > +fi\n>\n> Out of curiosity: Why are Windows versions (MINGW, CYGWIN) excluded ?\n\nThe answer to this question is probably worthy of recording as an\nin-code comment just above this conditional so that people coming upon\nthis test script in the future don't have to ask the same question\n(which is especially important if the author is no longer reachable).\nIf an in-code comment is overkill, then it would probably be a good\nidea for the commit message to explain the reason.\n\n> > +test_expect_success 'clone succeeds with git-p4.fallbackEncoding set to \"cp1252\"' '\n> > + git config --global git-p4.fallbackEncoding cp1252 &&\n> > + test_when_finished cleanup_git &&\n> > + (\n> > + git p4 clone --dest=\"$git\" //depot@all &&\n> > + cd \"$git\" &&\n> > + git log --oneline >log &&\n> > + desc=$(head -1 log | awk '\\''{print $2}'\\'') && [ \"$desc\" = \"documentación\" ]\n>\n> Style nit:\n> See Documentation/CodingGuidelines: - We prefer \"test\" over \"[ ... ]\".\n>\n> desc=$(head -1 log | awk '\\''{print $2}'\\'') && test \"$desc\" = \"documentación\"\n\nStyle also suggests splitting the line after the &&.\n\nWe normally want to avoid using bare single-quotes inside the body of\nthe test since the body itself is a single-quoted string. These\nsingle-quotes make it harder for a reader to reason about what is\ngoing on; especially with the $2 in there, one has to spend extra\ncycles wondering if $2 is correctly expanded when the test runs or\nwhen it is first defined. So, an easier-to-understand rewrite might\nbe:\n\n desc=$(head -1 log | awk ''{print \\$2}'') &&\n test \"$desc\" = \"documentación\"\n\nMany existing tests in this project use `cut` for word-plucking, so an\nalternative would be:\n\n desc=$(head -1 log | cut -d\" \" -f2) &&\n"},{"id":"422701","messageId":"CAPig+cQUaJq4Bu1NDSBnsQoR2HXhQ+s+4aQHeVP82DM_BuEL8Q@mail.gmail.com","threadId":"55456","inReplyTo":"CAPig+cQE0oHHY89D6fLyymduY3=zSe8y246cz1P2MjTZhrMHNQ@mail.gmail.com","subject":"Re: [PATCH v3] add git-p4.fallbackEncoding config setting, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-04-22T22:33:43Z","receivedAt":"2021-04-22T22:33:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Apr 22, 2021 at 12:17 PM Eric Sunshine wrote:\n> We normally want to avoid using bare single-quotes inside the body of\n> the test since the body itself is a single-quoted string. These\n> single-quotes make it harder for a reader to reason about what is\n> going on; especially with the $2 in there, one has to spend extra\n> cycles wondering if $2 is correctly expanded when the test runs or\n> when it is first defined. So, an easier-to-understand rewrite might\n> be:\n>\n> desc=$(head -1 log | awk ''{print \\$2}'') &&\n\nOf course, the quotes surrounding the {print...} should be\ndouble-quotes, not pairs of single-quotes:\n\n desc=$(head -1 log | awk \"{print \\$2}\") &&\n\n(I didn't notice the problem when originally composing the email since\nthe compose window wasn't using a fixed-width font, and only noticed\nit later when re-reading it in a mail reader which does use\nfixed-width. Sorry for any potential confusion.)\n\n> Many existing tests in this project use `cut` for word-plucking, so an\n> alternative would be:\n>\n> desc=$(head -1 log | cut -d\" \" -f2) &&\n\nAt any rate, using `cut` would be a good option since there's plenty\nof precedent in existing test scripts.\n"},{"id":"422711","messageId":"20210423063632.1973-1-tzadik.vanderhoof@gmail.com","threadId":"55456","inReplyTo":"CAPig+cQUaJq4Bu1NDSBnsQoR2HXhQ+s+4aQHeVP82DM_BuEL8Q@mail.gmail.com","subject":"[PATCH] add git-p4.fallbackEncoding config variable, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-23T06:36:32Z","receivedAt":"2021-04-23T06:38:15Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"When git-p4 reads the output from a p4 command, it assumes it will be\n100% UTF-8. If even one character in the output of one p4 command is\nnot UTF-8, git-p4 crashes with:\n\n File \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\n decode byte Ox93 in position 42: invalid start byte\n\nThis is especially a problem for the \"git p4 clone ... @all\" command,\nwhere git-p4 needs to read thousands of changeset descriptions, one of\nwhich may have a stray smart quote, causing the whole clone operation\nto fail.\n\nAdd a new config setting, allowing git-p4 to try a fallback encoding\n(for example, \"cp1252\") and/or use the Unicode replacement character,\nto prevent the whole program from crashing on such a minor problem.\n\nSigned-off-by: Tzadik Vanderhoof \n---\n Documentation/git-p4.txt | 9 +++\n git-p4.py | 11 +++-\n t/t9835-git-p4-config-fallback-encoding.sh | 76 ++++++++++++++++++++++\n 3 files changed, 95 insertions(+), 1 deletion(-)\n create mode 100755 t/t9835-git-p4-config-fallback-encoding.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex f89e68b424..86d3ffa644 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -638,6 +638,15 @@ git-p4.pathEncoding::\n \tto transcode the paths to UTF-8. As an example, Perforce on Windows\n \toften uses \"cp1252\" to encode path names.\n \n+git-p4.fallbackEncoding::\n+\tPerforce changeset descriptions can be stored in any encoding.\n+\tGit-p4 first tries to interpret each description as UTF-8. If that\n+\tfails, this config allows another encoding to be tried. You can specify,\n+\tfor example, \"cp1252\". If git-p4.fallbackEncoding is \"replace\", UTF-8 will\n+\tbe used, with invalid UTF-8 characters replaced by the Unicode replacement\n+\tcharacter. The default is \"none\": there is no fallback, and any non UTF-8\n+\tcharacter will cause git-p4 to immediately fail.\n+\n git-p4.largeFileSystem::\n \tSpecify the system that is used for large (binary) files. Please note\n \tthat large file systems do not support the 'git p4 submit' command.\ndiff --git a/git-p4.py b/git-p4.py\nindex 09c9e93ac4..202fb01bdf 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -771,7 +771,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\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- value = value.decode()\n+ try:\n+ value = value.decode()\n+ except UnicodeDecodeError:\n+ fallbackEncoding = gitConfig(\"git-p4.fallbackEncoding\").lower() or 'none'\n+ if fallbackEncoding == 'none':\n+ raise Exception(\"UTF-8 decoding failed. Consider using git config git-p4.fallbackEncoding\")\n+ elif fallbackEncoding == 'replace':\n+ value = value.decode(errors='replace')\n+ else:\n+ value = value.decode(encoding=fallbackEncoding)\n decoded_entry[key] = value\n # Parse out data if it's an error response\n if decoded_entry.get('code') == 'error' and 'data' in decoded_entry:\ndiff --git a/t/t9835-git-p4-config-fallback-encoding.sh b/t/t9835-git-p4-config-fallback-encoding.sh\nnew file mode 100755\nindex 0000000000..ce352c826b\n--- /dev/null\n+++ b/t/t9835-git-p4-config-fallback-encoding.sh\n@@ -0,0 +1,76 @@\n+#!/bin/sh\n+\n+test_description='test git-p4.fallbackEncoding config'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./lib-git-p4.sh\n+\n+# The Windows build of p4 encodes its command-line arguments according to the\n+# active code page (which defaults to \"cp1252\"). As a result, \"p4 submit -d\" causes\n+# Unicode changeset descriptions to be stored in the Perforce database as cp1252,\n+# and a subsequent \"git p4 clone\" attempting to decode these descriptions as UTF-8\n+# will raise a UnicodeDecodeError, necessitating the use of the git-p4.fallbackEncoding config.\n+#\n+# The Linux build of p4 encodes its command-line arguments as UTF-8, so changeset descriptions\n+# are stored as UTF-8, and UnicodeDecodeError is never raised by \"git p4 clone\".\n+\n+if test_have_prereq !MINGW,!CYGWIN; then\n+\tskip_all='This system is not subject to encoding failures in \"git p4 clone\"'\n+\ttest_done\n+fi\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'add cp1252 description' '\n+\tcd \"$cli\" &&\n+\techo file1 >file1 &&\n+\tp4 add file1 &&\n+\tp4 submit -d documentación\n+'\n+\n+test_expect_success 'clone fails with git-p4.fallbackEncoding unset' '\n+\ttest_might_fail git config --global --unset git-p4.fallbackEncoding &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\ttest_must_fail git p4 clone --dest=\"$git\" //depot@all 2>error &&\n+\t\tgrep \"UTF-8 decoding failed. Consider using git config git-p4.fallbackEncoding\" error\n+\t)\n+'\n+test_expect_success 'clone fails with git-p4.fallbackEncoding set to \"none\"' '\n+\tgit config --global git-p4.fallbackEncoding none &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\ttest_must_fail git p4 clone --dest=\"$git\" //depot@all 2>error &&\n+\t\tgrep \"UTF-8 decoding failed. Consider using git config git-p4.fallbackEncoding\" error\n+\t)\n+'\n+\n+test_expect_success 'clone succeeds with git-p4.fallbackEncoding set to \"cp1252\"' '\n+\tgit config --global git-p4.fallbackEncoding cp1252 &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t\tcd \"$git\" &&\n+\t\tgit log --oneline >log &&\n+\t\tdesc=$(head -1 log | cut -d\" \" -f2) &&\n+\t\ttest \"$desc\" = \"documentación\"\n+\t)\n+'\n+\n+test_expect_success 'clone succeeds with git-p4.fallbackEncoding set to \"replace\"' '\n+\tgit config --global git-p4.fallbackEncoding replace &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t\tcd \"$git\" &&\n+\t\tgit log --oneline >log &&\n+\t\tdesc=$(head -1 log | cut -d\" \" -f2) &&\n+\t\ttest \"$desc\" = \"documentaci�n\"\n+\t)\n+'\n+\n+test_done\n-- \n2.31.1\n\n"},{"id":"422712","messageId":"CAKu1iLVwfQ7Y-bOSO1tyxyFaNWum8sKW4b00i1nJCef98_2=UQ@mail.gmail.com","threadId":"55456","inReplyTo":"20210423063632.1973-1-tzadik.vanderhoof@gmail.com","subject":"Re: [PATCH] add git-p4.fallbackEncoding config variable, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-23T06:44:51Z","receivedAt":"2021-04-23T06:45:06Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"(last patch should be labeled as v4... sorry)\n"},{"id":"422772","messageId":"CAKu1iLXPi4zc-5-RtZo3UBwTQ1GqshXjLEZKT=WvtvB0aiuUJA@mail.gmail.com","threadId":"55456","inReplyTo":"CAKu1iLVwfQ7Y-bOSO1tyxyFaNWum8sKW4b00i1nJCef98_2=UQ@mail.gmail.com","subject":"Re: [PATCH] add git-p4.fallbackEncoding config variable, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-23T19:08:17Z","receivedAt":"2021-04-23T19:08:38Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"To clarify....\n\nThe new config variable I am introducing addresses an issue that only\noccurs on Windows. This is because the behavior of the \"p4\" command\ndiffers on Windows vs Linux around Unicode in changeset descriptions.\n\nI don't have the source code for \"p4\", but I'm guessing it's written\nin C, and that this difference in behavior is simply a result of the\nfact that there is no defined standard of how \"char *argv[]\" in \"main\"\nshould deal with non-ASCII characters being passed in from the command\nline.\n\nAs a result, \"git p4 clone\" on Linux is not affected by this \"p4\"\nbehavior. Since my tests assume the Windows behavior, they fail when\nrun on Linux. For this reason, I added code to my tests to skip them\non Linux.\n\nOn a related note, I don't think there are any CI environments on\ngithub for git that are (a) on Windows, and (b) have Python and (c)\nhave Perforce, so I don't think my tests are actually running on\ngithub CI. I'm not sure how that can be addressed.\n"},{"id":"422852","messageId":"20210424081447.uxabqbxc54k6yxrg@tb-raspi4","threadId":"55456","inReplyTo":"CAKu1iLXPi4zc-5-RtZo3UBwTQ1GqshXjLEZKT=WvtvB0aiuUJA@mail.gmail.com","subject":"Re: [PATCH] add git-p4.fallbackEncoding config variable, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-04-24T08:14:47Z","receivedAt":"2021-04-24T08:15:00Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"(Adding some of the p4 and Windows experts in cc)\n\nOn Fri, Apr 23, 2021 at 12:08:17PM -0700, Tzadik Vanderhoof wrote:\n> To clarify....\n\nGood. This is good information, and the important stuff could go\ninto the commit message. And because the commit as such should be\nself-contained (as much as possible).\nGiving an overview about the problem.\n\n>\n> The new config variable I am introducing addresses an issue that only\n> occurs on Windows. This is because the behavior of the \"p4\" command\n> differs on Windows vs Linux around Unicode in changeset descriptions.\n\nWhat does Windows mean in this context ?\nIs p4 a \"console application\" ?\nIn this case it may be possible to use CHCP to change to a different code page ?\n\n>\n> I don't have the source code for \"p4\", but I'm guessing it's written\n> in C, and that this difference in behavior is simply a result of the\n> fact that there is no defined standard of how \"char *argv[]\" in \"main\"\n> should deal with non-ASCII characters being passed in from the command\n> line.\n>\n> As a result, \"git p4 clone\" on Linux is not affected by this \"p4\"\n> behavior.\n\nIs it ?\nWhat happens if yoy have a p4 depot that was feed from Windows in CP-1252 and is now\naccessed from a Linux machine ?\nDoesthe Linux box face the same problems ?\n\n> Since my tests assume the Windows behavior, they fail when\n> run on Linux. For this reason, I added code to my tests to skip them\n> on Linux.\n\nThat makes sense, but what is the \"Windows behavior\" more in detail ?\nMy understanding is that when you press e.g. the key 'Ä' on the keybaord,\nit will give a different byte sequence once that 'Ä' is transferred\nacross the wire (to the p4 server).\nThis is dependent on what Linux calls a locale, and all major Linux installations\nuse UTF-8 these days by default.\nBut that was not always the case, since in old days they used ISO-8851-1 or something\nelse, usable for your contry/region.\n\nSo most Windows \"console applications\" are not run under UTF-8, but it\nmay be possible that \"CHCP 65000\" (or so) works - more testing needed.\n>\n> On a related note, I don't think there are any CI environments on\n> github for git that are (a) on Windows, and (b) have Python and (c)\n> have Perforce, so I don't think my tests are actually running on\n> github CI. I'm not sure how that can be addressed.\n\nThat are 3 different questions -\n(a) Yes, git is compiled under Windows, both gcc and MSVC (correct me if that is wrong)\n(b) Yes, we have python on the different CI. Github actions has python, I use it every day.\n(c) There are tests run for p4, but it seems if they are only run under Linux.\n\nIt would be nice if your test can pass under Linux, why are they failing ?\n\nIf I dig here:\n\n\nWe can see that the t98 test are run, and are passing. Just to pick one:\n[15:28:22] t9804-git-p4-label.sh .............................. ok 3969\n\nThanks for working on this.\nIt would be good to have a v5 version of the patch with some more informations,\nlike above, and may be :how is the p4 server configured ?\n(Unicode or not ?)\n\n"},{"id":"423054","messageId":"20210427053916.1977-1-tzadik.vanderhoof@gmail.com","threadId":"55456","inReplyTo":"20210424081447.uxabqbxc54k6yxrg@tb-raspi4","subject":"[PATCH v5] add git-p4.fallbackEncoding config variable, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-27T05:39:16Z","receivedAt":"2021-04-27T05:39:50Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"When git-p4 reads the output from a p4 command, it assumes it will be\n100% UTF-8. If even one character in the output of one p4 command is\nnot UTF-8, git-p4 crashes with:\n\n File \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\n decode byte Ox93 in position 42: invalid start byte\n\nThis is especially a problem for the \"git p4 clone ... @all\" command,\nwhere git-p4 needs to read thousands of changeset descriptions, one of\nwhich may have a stray smart quote, causing the whole clone operation\nto fail.\n\nAdd a new config setting, allowing git-p4 to try a fallback encoding\n(for example, \"cp1252\") and/or use the Unicode replacement character,\nto prevent the whole program from crashing on such a minor problem.\n\nSigned-off-by: Tzadik Vanderhoof \n---\n Documentation/git-p4.txt | 9 +++\n git-p4.py | 11 ++-\n t/t9835-git-p4-config-fallback-encoding.sh | 87 ++++++++++++++++++++++\n 3 files changed, 106 insertions(+), 1 deletion(-)\n create mode 100755 t/t9835-git-p4-config-fallback-encoding.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex f89e68b424..86d3ffa644 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -638,6 +638,15 @@ git-p4.pathEncoding::\n \tto transcode the paths to UTF-8. As an example, Perforce on Windows\n \toften uses \"cp1252\" to encode path names.\n \n+git-p4.fallbackEncoding::\n+\tPerforce changeset descriptions can be stored in any encoding.\n+\tGit-p4 first tries to interpret each description as UTF-8. If that\n+\tfails, this config allows another encoding to be tried. You can specify,\n+\tfor example, \"cp1252\". If git-p4.fallbackEncoding is \"replace\", UTF-8 will\n+\tbe used, with invalid UTF-8 characters replaced by the Unicode replacement\n+\tcharacter. The default is \"none\": there is no fallback, and any non UTF-8\n+\tcharacter will cause git-p4 to immediately fail.\n+\n git-p4.largeFileSystem::\n \tSpecify the system that is used for large (binary) files. Please note\n \tthat large file systems do not support the 'git p4 submit' command.\ndiff --git a/git-p4.py b/git-p4.py\nindex 09c9e93ac4..202fb01bdf 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -771,7 +771,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\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- value = value.decode()\n+ try:\n+ value = value.decode()\n+ except UnicodeDecodeError:\n+ fallbackEncoding = gitConfig(\"git-p4.fallbackEncoding\").lower() or 'none'\n+ if fallbackEncoding == 'none':\n+ raise Exception(\"UTF-8 decoding failed. Consider using git config git-p4.fallbackEncoding\")\n+ elif fallbackEncoding == 'replace':\n+ value = value.decode(errors='replace')\n+ else:\n+ value = value.decode(encoding=fallbackEncoding)\n decoded_entry[key] = value\n # Parse out data if it's an error response\n if decoded_entry.get('code') == 'error' and 'data' in decoded_entry:\ndiff --git a/t/t9835-git-p4-config-fallback-encoding.sh b/t/t9835-git-p4-config-fallback-encoding.sh\nnew file mode 100755\nindex 0000000000..383507803e\n--- /dev/null\n+++ b/t/t9835-git-p4-config-fallback-encoding.sh\n@@ -0,0 +1,87 @@\n+#!/bin/sh\n+\n+test_description='test git-p4.fallbackEncoding config'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./lib-git-p4.sh\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'add Unicode description' '\n+\tcd \"$cli\" &&\n+\techo file1 >file1 &&\n+\tp4 add file1 &&\n+\tp4 submit -d documentación\n+'\n+\n+# Unicode descriptions cause clone to throw in some environments. This test\n+# determines if that is the case in our environment. If so we create a file called \"clone_fails\".\n+# We check that file to in subsequent tests to determine what behavior to expect.\n+\n+clone_fails=\"$TRASH_DIRECTORY/clone_fails\"\n+\n+test_expect_success 'clone with git-p4.fallbackEncoding unset' '\n+\ttest_might_fail git config --global --unset git-p4.fallbackEncoding &&\n+\ttest_when_finished cleanup_git && {\n+\t\tgit p4 clone --dest=\"$git\" //depot@all 2>error || (\n+\t\t\tcp /dev/null \"$clone_fails\" &&\n+\t\t\tgrep \"UTF-8 decoding failed. Consider using git config git-p4.fallbackEncoding\" error\n+\t\t)\n+\t}\n+'\n+\n+test_expect_success 'clone with git-p4.fallbackEncoding set to \"none\"' '\n+\tgit config --global git-p4.fallbackEncoding none &&\n+\ttest_when_finished cleanup_git && {\n+\t\t(\n+\t\t\ttest -f \"$clone_fails\" &&\n+\t\t\ttest_must_fail git p4 clone --dest=\"$git\" //depot@all 2>error &&\n+\t\t\tgrep \"UTF-8 decoding failed. Consider using git config git-p4.fallbackEncoding\" error\n+\t\t) ||\n+\t\t(\n+\t\t\t! test -f \"$clone_fails\" &&\n+\t\t\tgit p4 clone --dest=\"$git\" //depot@all 2>error\n+\t\t)\n+\t}\n+'\n+\n+test_expect_success 'clone with git-p4.fallbackEncoding set to \"cp1252\"' '\n+\tgit config --global git-p4.fallbackEncoding cp1252 &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t\tcd \"$git\" &&\n+\t\tgit log --oneline >log &&\n+\t\tdesc=$(head -1 log | cut -d\" \" -f2) &&\n+\t\ttest \"$desc\" = \"documentación\"\n+\t)\n+'\n+\n+test_expect_success 'clone with git-p4.fallbackEncoding set to \"replace\"' '\n+\tgit config --global git-p4.fallbackEncoding replace &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t\tcd \"$git\" &&\n+\t\tgit log --oneline >log &&\n+\t\tdesc=$(head -1 log | cut -d\" \" -f2) &&\n+\t\t{\n+\t\t\t(test -f \"$clone_fails\" &&\n+\t\t\t\ttest \"$desc\" = \"documentaci�n\"\n+\t\t\t) ||\n+\t\t\t(! test -f \"$clone_fails\" &&\n+\t\t\t\ttest \"$desc\" = \"documentación\"\n+\t\t\t)\n+\t\t}\n+\t)\n+'\n+\n+test_expect_success 'unset git-p4.fallbackEncoding' '\n+\tgit config --global --unset git-p4.fallbackEncoding\n+'\n+\n+test_done\n-- \n2.31.1\n\n"},{"id":"423055","messageId":"CAKu1iLUYZRFV4QX2N3o9G89n0efE+2mBC7piV6Ks2+v+xeYvmw@mail.gmail.com","threadId":"55456","inReplyTo":"20210427053916.1977-1-tzadik.vanderhoof@gmail.com","subject":"Re: [PATCH v5] add git-p4.fallbackEncoding config variable, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-27T05:45:10Z","receivedAt":"2021-04-27T05:45:25Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"I modified the test to work on both Linux and Windows. See the\ncomments in the test.\n"},{"id":"423174","messageId":"xmqqr1ivauph.fsf@gitster.g","threadId":"55456","inReplyTo":"20210427053916.1977-1-tzadik.vanderhoof@gmail.com","subject":"Re: [PATCH v5] add git-p4.fallbackEncoding config variable, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-28T04:39:38Z","receivedAt":"2021-04-28T04:39:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tzadik Vanderhoof writes:\n\n> t/t9835-git-p4-config-fallback-encoding.sh | 87 ++++++++++++++++++++++\n> 3 files changed, 106 insertions(+), 1 deletion(-)\n> create mode 100755 t/t9835-git-p4-config-fallback-encoding.sh\n\n9835 is already taken (see 'seen').\n"},{"id":"423196","messageId":"20210428145824.43c4t7hkjfqjyspb@tb-raspi4","threadId":"55456","inReplyTo":"xmqqr1ivauph.fsf@gitster.g","subject":"Re: [PATCH v5] add git-p4.fallbackEncoding config variable, to prevent git-p4 from crashing on non UTF-8 changeset descriptions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-04-28T14:58:24Z","receivedAt":"2021-04-28T15:00:49Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Wed, Apr 28, 2021 at 01:39:38PM +0900, Junio C Hamano wrote:\n> Tzadik Vanderhoof writes:\n>\n> > t/t9835-git-p4-config-fallback-encoding.sh | 87 ++++++++++++++++++++++\n> > 3 files changed, 106 insertions(+), 1 deletion(-)\n> > create mode 100755 t/t9835-git-p4-config-fallback-encoding.sh\n>\n> 9835 is already taken (see 'seen').\n\nIn general, this looks good to me.\nThere are two minor nitpicks to make the patch more the git-way:\n\n> Subject: [PATCH v5] add git-p4.fallbackEncoding config variable, to prevent git-p4 from crashing on non UTF-8 changeset descriptionsw\n\nThe head line is somewhat too long.\nIt should be much shorter, like 50-55 characters, if I recall it rigth.\nThe first line of the commit message is what we see under PATCH in the email,\nfollowed by a blank line (that's what we have) and a detailed description\n(Which we have)\n\nHow abut this ?\n\ngit-p4: Add git-p4.fallbackEncoding\n\nAdd git-p4.fallbackEncoding config variable,\nto prevent git-p4 from crashing on non UTF-8 changeset descriptions.\n\nWhen git-p4 reads the output from a p4 command, it assumes it will be\n100% UTF-8. If even one character in the output of one p4 command is\nnot UTF-8, git-p4 crashes with:\n\n File \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\n decode byte Ox93 in position 42: invalid start byte\n\nThis is especially a problem for the \"git p4 clone ... @all\" command,\nwhere git-p4 needs to read thousands of changeset descriptions, one of\nwhich may have a stray smart quote, causing the whole clone operation\nto fail.\n\nAdd a new config setting, allowing git-p4 to try a fallback encoding\n(for example, \"cp1252\") and/or use the Unicode replacement character,\nto prevent the whole program from crashing on such a minor problem.\n\n\n[]\n\nAnd then, somewhere in the test:\n\n\t\t\tcp /dev/null \"$clone_fails\" &&\n\nThis should create an empty file, right ?\nThen we can use a simple output-redirection:\n\n\t\t\t>\"$clone_fails\" &&\n\n"},{"id":"423246","messageId":"20210429073905.837-1-tzadik.vanderhoof@gmail.com","threadId":"55456","inReplyTo":"20210428145824.43c4t7hkjfqjyspb@tb-raspi4","subject":"[PATCH v6] Add git-p4.fallbackEncoding","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-29T07:39:05Z","receivedAt":"2021-04-29T07:42:33Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"Add git-p4.fallbackEncoding config variable, to prevent git-p4 from\ncrashing on non UTF-8 changeset descriptions.\n\nWhen git-p4 reads the output from a p4 command, it assumes it will\nbe 100% UTF-8. If even one character in the output of one p4 command is\nnot UTF-8, git-p4 crashes with:\n\n File \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\n decode byte Ox93 in position 42: invalid start byte\n\nThis is especially a problem for the \"git p4 clone ... @all\" command,\nwhere git-p4 needs to read thousands of changeset descriptions, one of\nwhich may have a stray smart quote, causing the whole clone operation\nto fail.\n\nAdd a new config setting, allowing git-p4 to try a fallback encoding\n(for example, \"cp1252\") and/or use the Unicode replacement character,\nto prevent the whole program from crashing on such a minor problem.\n\nSigned-off-by: Tzadik Vanderhoof \n---\n Documentation/git-p4.txt | 9 ++\n git-p4.py | 11 ++-\n t/t9836-git-p4-config-fallback-encoding.sh | 98 ++++++++++++++++++++++\n 3 files changed, 117 insertions(+), 1 deletion(-)\n create mode 100755 t/t9836-git-p4-config-fallback-encoding.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex f89e68b424..86d3ffa644 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -638,6 +638,15 @@ git-p4.pathEncoding::\n \tto transcode the paths to UTF-8. As an example, Perforce on Windows\n \toften uses \"cp1252\" to encode path names.\n \n+git-p4.fallbackEncoding::\n+\tPerforce changeset descriptions can be stored in any encoding.\n+\tGit-p4 first tries to interpret each description as UTF-8. If that\n+\tfails, this config allows another encoding to be tried. You can specify,\n+\tfor example, \"cp1252\". If git-p4.fallbackEncoding is \"replace\", UTF-8 will\n+\tbe used, with invalid UTF-8 characters replaced by the Unicode replacement\n+\tcharacter. The default is \"none\": there is no fallback, and any non UTF-8\n+\tcharacter will cause git-p4 to immediately fail.\n+\n git-p4.largeFileSystem::\n \tSpecify the system that is used for large (binary) files. Please note\n \tthat large file systems do not support the 'git p4 submit' command.\ndiff --git a/git-p4.py b/git-p4.py\nindex 09c9e93ac4..202fb01bdf 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -771,7 +771,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\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- value = value.decode()\n+ try:\n+ value = value.decode()\n+ except UnicodeDecodeError:\n+ fallbackEncoding = gitConfig(\"git-p4.fallbackEncoding\").lower() or 'none'\n+ if fallbackEncoding == 'none':\n+ raise Exception(\"UTF-8 decoding failed. Consider using git config git-p4.fallbackEncoding\")\n+ elif fallbackEncoding == 'replace':\n+ value = value.decode(errors='replace')\n+ else:\n+ value = value.decode(encoding=fallbackEncoding)\n decoded_entry[key] = value\n # Parse out data if it's an error response\n if decoded_entry.get('code') == 'error' and 'data' in decoded_entry:\ndiff --git a/t/t9836-git-p4-config-fallback-encoding.sh b/t/t9836-git-p4-config-fallback-encoding.sh\nnew file mode 100755\nindex 0000000000..901bb3759d\n--- /dev/null\n+++ b/t/t9836-git-p4-config-fallback-encoding.sh\n@@ -0,0 +1,98 @@\n+#!/bin/sh\n+\n+test_description='test git-p4.fallbackEncoding config'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./lib-git-p4.sh\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'add Unicode description' '\n+\tcd \"$cli\" &&\n+\techo file1 >file1 &&\n+\tp4 add file1 &&\n+\tp4 submit -d documentación\n+'\n+\n+# Unicode descriptions cause \"git p4 clone\" to crash with a UnicodeDecodeError in some\n+# environments. This test determines if that is the case in our environment. If so,\n+# we create a file called \"clone_fails\". In subsequent tests, we check whether that\n+# file exists to determine what behavior to expect.\n+\n+clone_fails=\"$TRASH_DIRECTORY/clone_fails\"\n+\n+# If clone fails with git-p4.fallbackEncoding set to \"none\", create the \"clone_fails\" file,\n+# and make sure the error message is correct\n+\n+test_expect_success 'clone with git-p4.fallbackEncoding set to \"none\"' '\n+\tgit config --global git-p4.fallbackEncoding none &&\n+\ttest_when_finished cleanup_git && {\n+\t\tgit p4 clone --dest=\"$git\" //depot@all 2>error || (\n+\t\t\t>\"$clone_fails\" &&\n+\t\t\tgrep \"UTF-8 decoding failed. Consider using git config git-p4.fallbackEncoding\" error\n+\t\t)\n+\t}\n+'\n+\n+# If clone fails with git-p4.fallbackEncoding set to \"none\", it should also fail when it's unset,\n+# also with the correct error message. Otherwise the clone should succeed.\n+\n+test_expect_success 'clone with git-p4.fallbackEncoding unset' '\n+\tgit config --global --unset git-p4.fallbackEncoding &&\n+\ttest_when_finished cleanup_git && {\n+\t\t(\n+\t\t\ttest -f \"$clone_fails\" &&\n+\t\t\ttest_must_fail git p4 clone --dest=\"$git\" //depot@all 2>error &&\n+\t\t\tgrep \"UTF-8 decoding failed. Consider using git config git-p4.fallbackEncoding\" error\n+\t\t) ||\n+\t\t(\n+\t\t\t! test -f \"$clone_fails\" &&\n+\t\t\tgit p4 clone --dest=\"$git\" //depot@all 2>error\n+\t\t)\n+\t}\n+'\n+\n+# Whether or not \"clone_fails\" exists, setting git-p4.fallbackEncoding\n+# to \"cp1252\" should cause clone to succeed and get the right description\n+\n+test_expect_success 'clone with git-p4.fallbackEncoding set to \"cp1252\"' '\n+\tgit config --global git-p4.fallbackEncoding cp1252 &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t\tcd \"$git\" &&\n+\t\tgit log --oneline >log &&\n+\t\tdesc=$(head -1 log | cut -d\" \" -f2) &&\n+\t\ttest \"$desc\" = \"documentación\"\n+\t)\n+'\n+\n+# Setting git-p4.fallbackEncoding to \"replace\" should always cause clone to succeed.\n+# If \"clone_fails\" exists, the description should contain the Unicode replacement\n+# character, otherwise the description should be correct (since we're on a system that\n+# doesn't have the Unicode issue)\n+\n+test_expect_success 'clone with git-p4.fallbackEncoding set to \"replace\"' '\n+\tgit config --global git-p4.fallbackEncoding replace &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t\tcd \"$git\" &&\n+\t\tgit log --oneline >log &&\n+\t\tdesc=$(head -1 log | cut -d\" \" -f2) &&\n+\t\t{\n+\t\t\t(test -f \"$clone_fails\" &&\n+\t\t\t\ttest \"$desc\" = \"documentaci�n\"\n+\t\t\t) ||\n+\t\t\t(! test -f \"$clone_fails\" &&\n+\t\t\t\ttest \"$desc\" = \"documentación\"\n+\t\t\t)\n+\t\t}\n+\t)\n+'\n+\n+test_done\n-- \n2.31.1\n\n"},{"id":"423254","messageId":"179cd4f0-def6-1b3a-2802-139b19d3301d@diamand.org","threadId":"55456","inReplyTo":"20210429073905.837-1-tzadik.vanderhoof@gmail.com","subject":"Re: [PATCH v6] Add git-p4.fallbackEncoding","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2021-04-29T08:36:20Z","receivedAt":"2021-04-29T08:36:17Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"\n\nOn 29/04/2021 07:39, Tzadik Vanderhoof wrote:\n> Add git-p4.fallbackEncoding config variable, to prevent git-p4 from\n> crashing on non UTF-8 changeset descriptions.\n> \n> When git-p4 reads the output from a p4 command, it assumes it will\n> be 100% UTF-8. If even one character in the output of one p4 command is\n> not UTF-8, git-p4 crashes with:\n> \n> File \"C:/Program Files/Git/bin/git-p4.py\", line 774, in p4CmdList\n> value = value.decode() UnicodeDecodeError: 'utf-8' codec can't\n> decode byte Ox93 in position 42: invalid start byte\n> \n> This is especially a problem for the \"git p4 clone ... @all\" command,\n> where git-p4 needs to read thousands of changeset descriptions, one of\n> which may have a stray smart quote, causing the whole clone operation\n> to fail.\n> \n> Add a new config setting, allowing git-p4 to try a fallback encoding\n> (for example, \"cp1252\") and/or use the Unicode replacement character,\n> to prevent the whole program from crashing on such a minor problem.\n\nI think Andrew Oakley pointed this out earlier - but in the days of \nPython2 this was (I think) never a problem. Python2 just took in the \nbinary data, in whatever encoding, and passed it untouched on to git, \nwhich in turn just stored it.\n\nhttps://lore.kernel.org/git/20210412085251.51475-1-andrew@adoakley.name/\n\nIt was only whatever was trying to render the bytestream that needed to \nworry about the encoding.\n\nNow we're making the decision in git-p4 when we ingest it - did we \nconsider just passing it along untouched?\n\nThe problem at hand is that git-p4 is trying to store it internally as a \n`string' which now is unicode-aware, when perhaps it should not be.\n\nIt's going to get very confusing if anyone ingests something from \nPerforce having set the encoding to one thing, and it turns out to be a \ndifferent encoding, or worse, multiple encodings for the same repo.\n\nI also worry that if someone has connected to a Unicode-aware Perforce \nserver, and then unwittingly set P4CHARSET, then are we going to end up \nsilently scrambling everything?\n\nI'm not sure, encodings make my head hurt.\n\nLuke\n\n\n\n> \n> Signed-off-by: Tzadik Vanderhoof \n> ---\n> Documentation/git-p4.txt | 9 ++\n> git-p4.py | 11 ++-\n> t/t9836-git-p4-config-fallback-encoding.sh | 98 ++++++++++++++++++++++\n> 3 files changed, 117 insertions(+), 1 deletion(-)\n> create mode 100755 t/t9836-git-p4-config-fallback-encoding.sh\n> \n> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n> index f89e68b424..86d3ffa644 100644\n> --- a/Documentation/git-p4.txt\n> +++ b/Documentation/git-p4.txt\n> @@ -638,6 +638,15 @@ git-p4.pathEncoding::\n> \tto transcode the paths to UTF-8. As an example, Perforce on Windows\n> \toften uses \"cp1252\" to encode path names.\n> \n> +git-p4.fallbackEncoding::\n> +\tPerforce changeset descriptions can be stored in any encoding.\n> +\tGit-p4 first tries to interpret each description as UTF-8. If that\n> +\tfails, this config allows another encoding to be tried. You can specify,\n> +\tfor example, \"cp1252\". If git-p4.fallbackEncoding is \"replace\", UTF-8 will\n> +\tbe used, with invalid UTF-8 characters replaced by the Unicode replacement\n> +\tcharacter. The default is \"none\": there is no fallback, and any non UTF-8\n> +\tcharacter will cause git-p4 to immediately fail.\n> +\n> git-p4.largeFileSystem::\n> \tSpecify the system that is used for large (binary) files. Please note\n> \tthat large file systems do not support the 'git p4 submit' command.\n> diff --git a/git-p4.py b/git-p4.py\n> index 09c9e93ac4..202fb01bdf 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -771,7 +771,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\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> - value = value.decode()\n> + try:\n> + value = value.decode()\n> + except UnicodeDecodeError:\n> + fallbackEncoding = gitConfig(\"git-p4.fallbackEncoding\").lower() or 'none'\n> + if fallbackEncoding == 'none':\n> + raise Exception(\"UTF-8 decoding failed. Consider using git config git-p4.fallbackEncoding\")\n> + elif fallbackEncoding == 'replace':\n> + value = value.decode(errors='replace')\n> + else:\n> + value = value.decode(encoding=fallbackEncoding)\n> decoded_entry[key] = value\n> # Parse out data if it's an error response\n> if decoded_entry.get('code') == 'error' and 'data' in decoded_entry:\n> diff --git a/t/t9836-git-p4-config-fallback-encoding.sh b/t/t9836-git-p4-config-fallback-encoding.sh\n> new file mode 100755\n> index 0000000000..901bb3759d\n> --- /dev/null\n> +++ b/t/t9836-git-p4-config-fallback-encoding.sh\n> @@ -0,0 +1,98 @@\n> +#!/bin/sh\n> +\n> +test_description='test git-p4.fallbackEncoding config'\n> +\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n> +. ./lib-git-p4.sh\n> +\n> +test_expect_success 'start p4d' '\n> +\tstart_p4d\n> +'\n> +\n> +test_expect_success 'add Unicode description' '\n> +\tcd \"$cli\" &&\n> +\techo file1 >file1 &&\n> +\tp4 add file1 &&\n> +\tp4 submit -d documentación\n> +'\n> +\n> +# Unicode descriptions cause \"git p4 clone\" to crash with a UnicodeDecodeError in some\n> +# environments. This test determines if that is the case in our environment. If so,\n> +# we create a file called \"clone_fails\". In subsequent tests, we check whether that\n> +# file exists to determine what behavior to expect.\n> +\n> +clone_fails=\"$TRASH_DIRECTORY/clone_fails\"\n> +\n> +# If clone fails with git-p4.fallbackEncoding set to \"none\", create the \"clone_fails\" file,\n> +# and make sure the error message is correct\n> +\n> +test_expect_success 'clone with git-p4.fallbackEncoding set to \"none\"' '\n> +\tgit config --global git-p4.fallbackEncoding none &&\n> +\ttest_when_finished cleanup_git && {\n> +\t\tgit p4 clone --dest=\"$git\" //depot@all 2>error || (\n> +\t\t\t>\"$clone_fails\" &&\n> +\t\t\tgrep \"UTF-8 decoding failed. Consider using git config git-p4.fallbackEncoding\" error\n> +\t\t)\n> +\t}\n> +'\n> +\n> +# If clone fails with git-p4.fallbackEncoding set to \"none\", it should also fail when it's unset,\n> +# also with the correct error message. Otherwise the clone should succeed.\n> +\n> +test_expect_success 'clone with git-p4.fallbackEncoding unset' '\n> +\tgit config --global --unset git-p4.fallbackEncoding &&\n> +\ttest_when_finished cleanup_git && {\n> +\t\t(\n> +\t\t\ttest -f \"$clone_fails\" &&\n> +\t\t\ttest_must_fail git p4 clone --dest=\"$git\" //depot@all 2>error &&\n> +\t\t\tgrep \"UTF-8 decoding failed. Consider using git config git-p4.fallbackEncoding\" error\n> +\t\t) ||\n> +\t\t(\n> +\t\t\t! test -f \"$clone_fails\" &&\n> +\t\t\tgit p4 clone --dest=\"$git\" //depot@all 2>error\n> +\t\t)\n> +\t}\n> +'\n> +\n> +# Whether or not \"clone_fails\" exists, setting git-p4.fallbackEncoding\n> +# to \"cp1252\" should cause clone to succeed and get the right description\n> +\n> +test_expect_success 'clone with git-p4.fallbackEncoding set to \"cp1252\"' '\n> +\tgit config --global git-p4.fallbackEncoding cp1252 &&\n> +\ttest_when_finished cleanup_git &&\n> +\t(\n> +\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n> +\t\tcd \"$git\" &&\n> +\t\tgit log --oneline >log &&\n> +\t\tdesc=$(head -1 log | cut -d\" \" -f2) &&\n> +\t\ttest \"$desc\" = \"documentación\"\n> +\t)\n> +'\n> +\n> +# Setting git-p4.fallbackEncoding to \"replace\" should always cause clone to succeed.\n> +# If \"clone_fails\" exists, the description should contain the Unicode replacement\n> +# character, otherwise the description should be correct (since we're on a system that\n> +# doesn't have the Unicode issue)\n> +\n> +test_expect_success 'clone with git-p4.fallbackEncoding set to \"replace\"' '\n> +\tgit config --global git-p4.fallbackEncoding replace &&\n> +\ttest_when_finished cleanup_git &&\n> +\t(\n> +\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n> +\t\tcd \"$git\" &&\n> +\t\tgit log --oneline >log &&\n> +\t\tdesc=$(head -1 log | cut -d\" \" -f2) &&\n> +\t\t{\n> +\t\t\t(test -f \"$clone_fails\" &&\n> +\t\t\t\ttest \"$desc\" = \"documentaci�n\"\n> +\t\t\t) ||\n> +\t\t\t(! test -f \"$clone_fails\" &&\n> +\t\t\t\ttest \"$desc\" = \"documentación\"\n> +\t\t\t)\n> +\t\t}\n> +\t)\n> +'\n> +\n> +test_done\n> \n"},{"id":"423285","messageId":"CAKu1iLXJAdtKYQq25mZ4vvQUO=4K1S-p5vg0UioUQMBKzRtumA@mail.gmail.com","threadId":"55456","inReplyTo":"c4c48615-d1f4-fd37-0960-979535907f15@web.de","subject":"Re: [PATCH v6] Add git-p4.fallbackEncoding","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-29T17:14:33Z","receivedAt":"2021-04-29T17:14:49Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"On Thu, Apr 29, 2021 at 7:12 AM Torsten Bögershausen wrote:\n>\n> Hej Tzadik,\n>\n> This version went only to my email ?\n>\n\nv6 went to the list as well as your email. I just forgot to include\nyou in the email to the list, so I sent you another copy with just\nyou.\n\n> The test case number seems to be fixed, thanks.\n>\n> (Normally we don't have this collision, but right now\n> it seem as if there is much going on in the git-p4 area,\n> which is good)\n>\n> The \"headline\" is still overlong, it seams.\n>\n\nI did shorten the first line of my commit as you asked and used that\ncommit to create the v6 path. That first (short) line goes into the\nSubject line of the patch. When you do \"git am\" it will use the\nsubject (which is short) as the first line of the commit. The overlong\nsummary will become the 3rd line of the commit (after a blank second\nline)\n"},{"id":"423287","messageId":"CAKu1iLWUZqRu5fpGxdwLKjMwRCexcFJdz+uxo+AF3u32W4dOtQ@mail.gmail.com","threadId":"55456","inReplyTo":"179cd4f0-def6-1b3a-2802-139b19d3301d@diamand.org","subject":"Re: [PATCH v6] Add git-p4.fallbackEncoding","fromName":"Tzadik Vanderhoof","fromEmail":"tzadik.vanderhoof@gmail.com","sentAt":"2021-04-29T17:29:39Z","receivedAt":"2021-04-29T17:29:54Z","isPatch":true,"sender":{"key":"tzadik.vanderhoof@gmail.com","avatar":null},"body":"On Thu, Apr 29, 2021 at 1:36 AM Luke Diamand wrote:\n>\n> I think Andrew Oakley pointed this out earlier - but in the days of\n> Python2 this was (I think) never a problem. Python2 just took in the\n> binary data, in whatever encoding, and passed it untouched on to git,\n> which in turn just stored it.\n>\n\nUnfortunately, I just became aware yesterday that Andrew Oakley was\nalso working on this issue (his CC to me 2 weeks ago somehow ended up\nin my Spam folder, and I only dug it out of there after finding out\nthat we both created a test with the same number).\n\nWhen I first became aware of Andrew's work (yesterday), I thought it\nwould make mine unnecessary, but upon further investigation, I don't\nthink Andrew's work will solve this problem. Please see my reply\nyesterday to Andew's thread:\nhttps://lore.kernel.org/git/CAKu1iLXRrsB4mRsDfhBH5aahWzDjpfqLuWP9t47RMB=RdpL1iA@mail.gmail.com\n"}]}