{"thread":{"id":"57078","subject":"[PATCH 0/4] git-p4: fix RCS keyword processing encoding errors","startedAt":"2021-12-13T22:55:12Z","lastAt":"2021-12-15T21:41:07Z","messageCount":9,"participants":["Joel Holdsworth","Junio C Hamano","Andrew Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"444044","messageId":"20211213225441.1865782-1-jholdsworth@nvidia.com","threadId":"57078","inReplyTo":null,"subject":"[PATCH 0/4] git-p4: fix RCS keyword processing encoding errors","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-13T22:54:37Z","receivedAt":"2021-12-13T22:55:12Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"This patch-set fixes a family of issues with git-p4's handling of\nincoming text data that contains RCS keywords, when those files contain\nbytes which are invalid UTF-8 codes.\n\nAmong the patches is a fix for the issue, as well as some peripheral\ntidy-ups and improvements to the existing code.\n\nThis patch-set is compatible and has been tested with both Python 2 and\n3, and includes a test.\n\nJoel Holdsworth (4):\n  git-p4: use with statements to close files after use in\n    patchRCSKeywords\n  git-p4: pre-compile RCS keyword regexes\n  git-p4: add raw option to read_pipelines\n  git-p4: resolve RCS keywords in binary\n\n git-p4.py             | 66 ++++++++++++++++++-------------------------\n t/t9810-git-p4-rcs.sh | 15 ++++++++++\n 2 files changed, 42 insertions(+), 39 deletions(-)\n\n-- \n2.33.0\n\n"},{"id":"444045","messageId":"20211213225441.1865782-2-jholdsworth@nvidia.com","threadId":"57078","inReplyTo":"20211213225441.1865782-1-jholdsworth@nvidia.com","subject":"[PATCH 1/4] git-p4: use with statements to close files after use in patchRCSKeywords","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-13T22:54:38Z","receivedAt":"2021-12-13T22:55:19Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"Python with statements are used to wrap the execution of a block of code\nso that an object can be safely released when execution leaves the\nscope.\n\nThey are desirable for improving code tidyness, and to ensure that\nobjects are properly destroyed even when exceptions are thrown.\n\nSigned-off-by: Joel Holdsworth <jholdsworth@nvidia.com>\n---\n git-p4.py | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 2b4500226a..226cdef424 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1757,14 +1757,11 @@ def patchRCSKeywords(self, file, pattern):\n         # Attempt to zap the RCS keywords in a p4 controlled file matching the given pattern\n         (handle, outFileName) = tempfile.mkstemp(dir='.')\n         try:\n-            outFile = os.fdopen(handle, \"w+\")\n-            inFile = open(file, \"r\")\n-            regexp = re.compile(pattern, re.VERBOSE)\n-            for line in inFile.readlines():\n-                line = regexp.sub(r'$\\1$', line)\n-                outFile.write(line)\n-            inFile.close()\n-            outFile.close()\n+            with os.fdopen(handle, \"w+\") as outFile, open(file, \"r\") as inFile:\n+                regexp = re.compile(pattern, re.VERBOSE)\n+                for line in inFile.readlines():\n+                    line = regexp.sub(r'$\\1$', line)\n+                    outFile.write(line)\n             # Forcibly overwrite the original file\n             os.unlink(file)\n             shutil.move(outFileName, file)\n-- \n2.33.0\n\n"},{"id":"444046","messageId":"20211213225441.1865782-3-jholdsworth@nvidia.com","threadId":"57078","inReplyTo":"20211213225441.1865782-1-jholdsworth@nvidia.com","subject":"[PATCH 2/4] git-p4: pre-compile RCS keyword regexes","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-13T22:54:39Z","receivedAt":"2021-12-13T22:55:24Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"Previously git-p4.py would compile one of two regular expressions for\never RCS keyword-enabled file. This patch improves simplifies the code\nby pre-compiling the two regular expressions when the script first\nloads.\n\nSigned-off-by: Joel Holdsworth <jholdsworth@nvidia.com>\n---\n git-p4.py | 48 ++++++++++++++++++------------------------------\n 1 file changed, 18 insertions(+), 30 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 226cdef424..0af83b9c72 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -56,6 +56,9 @@\n \n p4_access_checked = False\n \n+re_ko_keywords = re.compile(r'\\$(Id|Header)(:[^$\\n]+)?\\$')\n+re_k_keywords = re.compile(r'\\$(Id|Header|Author|Date|DateTime|Change|File|Revision)(:[^$\\n]+)?\\$')\n+\n def p4_build_cmd(cmd):\n     \"\"\"Build a suitable p4 command line.\n \n@@ -577,20 +580,12 @@ def p4_type(f):\n #\n def p4_keywords_regexp_for_type(base, type_mods):\n     if base in (\"text\", \"unicode\", \"binary\"):\n-        kwords = None\n         if \"ko\" in type_mods:\n-            kwords = 'Id|Header'\n+            return re_ko_keywords\n         elif \"k\" in type_mods:\n-            kwords = 'Id|Header|Author|Date|DateTime|Change|File|Revision'\n+            return re_k_keywords\n         else:\n             return None\n-        pattern = r\"\"\"\n-            \\$              # Starts with a dollar, followed by...\n-            (%s)            # one of the keywords, followed by...\n-            (:[^$\\n]+)?     # possibly an old expansion, followed by...\n-            \\$              # another dollar\n-            \"\"\" % kwords\n-        return pattern\n     else:\n         return None\n \n@@ -1753,15 +1748,13 @@ def prepareLogMessage(self, template, message, jobs):\n \n         return result\n \n-    def patchRCSKeywords(self, file, pattern):\n-        # Attempt to zap the RCS keywords in a p4 controlled file matching the given pattern\n+    def patchRCSKeywords(self, file, regexp):\n+        # Attempt to zap the RCS keywords in a p4 controlled file matching the given regex\n         (handle, outFileName) = tempfile.mkstemp(dir='.')\n         try:\n             with os.fdopen(handle, \"w+\") as outFile, open(file, \"r\") as inFile:\n-                regexp = re.compile(pattern, re.VERBOSE)\n                 for line in inFile.readlines():\n-                    line = regexp.sub(r'$\\1$', line)\n-                    outFile.write(line)\n+                    outFile.write(regexp.sub(r'$\\1$', line))\n             # Forcibly overwrite the original file\n             os.unlink(file)\n             shutil.move(outFileName, file)\n@@ -2088,25 +2081,22 @@ def applyCommit(self, id):\n             # the patch to see if that's possible.\n             if gitConfigBool(\"git-p4.attemptRCSCleanup\"):\n                 file = None\n-                pattern = None\n                 kwfiles = {}\n                 for file in editedFiles | filesToDelete:\n                     # did this file's delta contain RCS keywords?\n-                    pattern = p4_keywords_regexp_for_file(file)\n-\n-                    if pattern:\n+                    regexp = p4_keywords_regexp_for_file(file)\n+                    if regexp:\n                         # this file is a possibility...look for RCS keywords.\n-                        regexp = re.compile(pattern, re.VERBOSE)\n                         for line in read_pipe_lines([\"git\", \"diff\", \"%s^..%s\" % (id, id), file]):\n                             if regexp.search(line):\n                                 if verbose:\n-                                    print(\"got keyword match on %s in %s in %s\" % (pattern, line, file))\n-                                kwfiles[file] = pattern\n+                                    print(\"got keyword match on %s in %s in %s\" % (regex.pattern, line, file))\n+                                kwfiles[file] = regexp\n                                 break\n \n-                for file in kwfiles:\n+                for file, regexp in kwfiles.items():\n                     if verbose:\n-                        print(\"zapping %s with %s\" % (line,pattern))\n+                        print(\"zapping %s with %s\" % (line, regexp.pattern))\n                     # File is being deleted, so not open in p4.  Must\n                     # disable the read-only bit on windows.\n                     if self.isWindows and file not in editedFiles:\n@@ -3026,12 +3016,10 @@ def streamOneP4File(self, file, contents):\n \n         # Note that we do not try to de-mangle keywords on utf16 files,\n         # even though in theory somebody may want that.\n-        pattern = p4_keywords_regexp_for_type(type_base, type_mods)\n-        if pattern:\n-            regexp = re.compile(pattern, re.VERBOSE)\n-            text = ''.join(decode_text_stream(c) for c in contents)\n-            text = regexp.sub(r'$\\1$', text)\n-            contents = [ encode_text_stream(text) ]\n+        regexp = p4_keywords_regexp_for_type(type_base, type_mods)\n+        if regexp:\n+            contents = [encode_text_stream(regexp.sub(\n+                r'$\\1$', ''.join(decode_text_stream(c) for c in contents)))]\n \n         if self.largeFileSystem:\n             (git_mode, contents) = self.largeFileSystem.processContent(git_mode, relPath, contents)\n-- \n2.33.0\n\n"},{"id":"444047","messageId":"20211213225441.1865782-4-jholdsworth@nvidia.com","threadId":"57078","inReplyTo":"20211213225441.1865782-1-jholdsworth@nvidia.com","subject":"[PATCH 3/4] git-p4: add raw option to read_pipelines","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-13T22:54:40Z","receivedAt":"2021-12-13T22:55:30Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"Previously the read_lines function always decoded the result lines. In\norder to improve support for non-decoded binary processing of data in\ngit-p4.py, this patch adds a raw option to the function that allows\ndecoding to be disabled.\n\nSigned-off-by: Joel Holdsworth <jholdsworth@nvidia.com>\n---\n git-p4.py | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 0af83b9c72..509feac2d8 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -340,17 +340,19 @@ def p4_read_pipe(c, ignore_error=False, raw=False):\n     real_cmd = p4_build_cmd(c)\n     return read_pipe(real_cmd, ignore_error, raw=raw)\n \n-def read_pipe_lines(c):\n+def read_pipe_lines(c, raw=False):\n     if verbose:\n         sys.stderr.write('Reading pipe: %s\\n' % str(c))\n \n     expand = not isinstance(c, list)\n     p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)\n     pipe = p.stdout\n-    val = [decode_text_stream(line) for line in pipe.readlines()]\n+    lines = pipe.readlines()\n+    if not raw:\n+        lines = [decode_text_stream(line) for line in lines]\n     if pipe.close() or p.wait():\n         die('Command failed: %s' % str(c))\n-    return val\n+    return lines\n \n def p4_read_pipe_lines(c):\n     \"\"\"Specifically invoke p4 on the command supplied. \"\"\"\n-- \n2.33.0\n\n"},{"id":"444048","messageId":"20211213225441.1865782-5-jholdsworth@nvidia.com","threadId":"57078","inReplyTo":"20211213225441.1865782-1-jholdsworth@nvidia.com","subject":"[PATCH 4/4] git-p4: resolve RCS keywords in binary","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-13T22:54:41Z","receivedAt":"2021-12-13T22:55:52Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"RCS keywords are strings that will are replaced with information from\nPerforce. Examples include $Date$, $Author$, $File$, $Change$ etc.\n\nPerforce resolves these by expanding them with their expanded values\nwhen files are synced, but Git's data model requires these expanded\nvalues to be converted back into their unexpanded form.\n\nPreviously, git-p4.py would implement this behaviour through the use of\nregular expressions. However, the regular expression substitution was\napplied using decoded strings i.e. the content of incoming commit diffs\nwas first decoded from bytes into UTF-8, processed with regular\nexpressions, then converted back to bytes.\n\nNot only is this behaviour inefficient, but it is also a cause of a\ncommon issue caused by text files containing invalid UTF-8 data. For\nfiles created in Windows, CP1252 Smart Quote Characters (0x93 and 0x94)\nare seen fairly frequently. These codes are invalid in UTF-8, so if the\nscript encountered any file containing them, on Python 2 the symbols\nwill be corrupted, and on Python 3 the script will fail with an\nexception.\n\nThis patch replaces this decoding/encoding with bytes object regular\nexpressions, so that the substitution is performed directly upon the\nsource data with not conversions.\n\nA test for smart quote handling has been added to the\nt9810-git-p4-rcs.sh test suite.\n\nSigned-off-by: Joel Holdsworth <jholdsworth@nvidia.com>\n---\n git-p4.py             | 15 ++++++++-------\n t/t9810-git-p4-rcs.sh | 15 +++++++++++++++\n 2 files changed, 23 insertions(+), 7 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 509feac2d8..986595bef0 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -56,8 +56,8 @@\n \n p4_access_checked = False\n \n-re_ko_keywords = re.compile(r'\\$(Id|Header)(:[^$\\n]+)?\\$')\n-re_k_keywords = re.compile(r'\\$(Id|Header|Author|Date|DateTime|Change|File|Revision)(:[^$\\n]+)?\\$')\n+re_ko_keywords = re.compile(br'\\$(Id|Header)(:[^$\\n]+)?\\$')\n+re_k_keywords = re.compile(br'\\$(Id|Header|Author|Date|DateTime|Change|File|Revision)(:[^$\\n]+)?\\$')\n \n def p4_build_cmd(cmd):\n     \"\"\"Build a suitable p4 command line.\n@@ -1754,9 +1754,9 @@ def patchRCSKeywords(self, file, regexp):\n         # Attempt to zap the RCS keywords in a p4 controlled file matching the given regex\n         (handle, outFileName) = tempfile.mkstemp(dir='.')\n         try:\n-            with os.fdopen(handle, \"w+\") as outFile, open(file, \"r\") as inFile:\n+            with os.fdopen(handle, \"wb\") as outFile, open(file, \"rb\") as inFile:\n                 for line in inFile.readlines():\n-                    outFile.write(regexp.sub(r'$\\1$', line))\n+                    outFile.write(regexp.sub(br'$\\1$', line))\n             # Forcibly overwrite the original file\n             os.unlink(file)\n             shutil.move(outFileName, file)\n@@ -2089,7 +2089,9 @@ def applyCommit(self, id):\n                     regexp = p4_keywords_regexp_for_file(file)\n                     if regexp:\n                         # this file is a possibility...look for RCS keywords.\n-                        for line in read_pipe_lines([\"git\", \"diff\", \"%s^..%s\" % (id, id), file]):\n+                        for line in read_pipe_lines(\n+                            [\"git\", \"diff\", \"%s^..%s\" % (id, id), file],\n+                            raw=True):\n                             if regexp.search(line):\n                                 if verbose:\n                                     print(\"got keyword match on %s in %s in %s\" % (regex.pattern, line, file))\n@@ -3020,8 +3022,7 @@ def streamOneP4File(self, file, contents):\n         # even though in theory somebody may want that.\n         regexp = p4_keywords_regexp_for_type(type_base, type_mods)\n         if regexp:\n-            contents = [encode_text_stream(regexp.sub(\n-                r'$\\1$', ''.join(decode_text_stream(c) for c in contents)))]\n+            contents = [regexp.sub(br'$\\1$', c) for c in contents]\n \n         if self.largeFileSystem:\n             (git_mode, contents) = self.largeFileSystem.processContent(git_mode, relPath, contents)\ndiff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh\nindex e3836888ec..5fe83315ec 100755\n--- a/t/t9810-git-p4-rcs.sh\n+++ b/t/t9810-git-p4-rcs.sh\n@@ -4,6 +4,8 @@ test_description='git p4 rcs keywords'\n \n . ./lib-git-p4.sh\n \n+CP1252=\"\\223\\224\"\n+\n test_expect_success 'start p4d' '\n \tstart_p4d\n '\n@@ -32,6 +34,9 @@ test_expect_success 'init depot' '\n \t\tp4 submit -d \"filek\" &&\n \t\tp4 add -t text+ko fileko &&\n \t\tp4 submit -d \"fileko\" &&\n+\t\tprintf \"$CP1252\" >fileko_cp1252 &&\n+\t\tp4 add -t text+ko fileko_cp1252 &&\n+\t\tp4 submit -d \"fileko_cp1252\" &&\n \t\tp4 add -t text file_text &&\n \t\tp4 submit -d \"file_text\"\n \t)\n@@ -359,4 +364,14 @@ test_expect_failure 'Add keywords in git which do not match the default p4 value\n \t)\n '\n \n+test_expect_success 'check cp1252 smart quote are preserved through RCS keyword processing' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tprintf \"$CP1252\" >expect &&\n+\t\ttest_cmp_bin expect fileko_cp1252\n+\t)\n+'\n+\n test_done\n-- \n2.33.0\n\n"},{"id":"444052","messageId":"xmqqzgp484f3.fsf@gitster.g","threadId":"57078","inReplyTo":"20211213225441.1865782-5-jholdsworth@nvidia.com","subject":"Re: [PATCH 4/4] git-p4: resolve RCS keywords in binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-13T23:34:56Z","receivedAt":"2021-12-13T23:35:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Holdsworth <jholdsworth@nvidia.com> writes:\n\n> RCS keywords are strings that will are replaced with information from\n> Perforce. Examples include $Date$, $Author$, $File$, $Change$ etc.\n>\n> Perforce resolves these by expanding them with their expanded values\n> when files are synced, but Git's data model requires these expanded\n> values to be converted back into their unexpanded form.\n>\n> Previously, git-p4.py would implement this behaviour through the use of\n> regular expressions. However, the regular expression substitution was\n> applied using decoded strings i.e. the content of incoming commit diffs\n> was first decoded from bytes into UTF-8, processed with regular\n> expressions, then converted back to bytes.\n>\n> Not only is this behaviour inefficient, but it is also a cause of a\n> common issue caused by text files containing invalid UTF-8 data. For\n> files created in Windows, CP1252 Smart Quote Characters (0x93 and 0x94)\n> are seen fairly frequently. These codes are invalid in UTF-8, so if the\n> script encountered any file containing them, on Python 2 the symbols\n> will be corrupted, and on Python 3 the script will fail with an\n> exception.\n\nMakes sense, and I am with others who commented on the previous\ndiscussion thread that the right approach to take is to take the\nstuff coming from Perforce as byte strings, process them as such and\nwrite them out as byte strings, UNLESS we positively know what the\nsource and destination encodings are.\n\nAnd this change we see here, matching with patterns, is perfectly in\nline with that direction.  Very nice.\n\n>          try:\n> -            with os.fdopen(handle, \"w+\") as outFile, open(file, \"r\") as inFile:\n> +            with os.fdopen(handle, \"wb\") as outFile, open(file, \"rb\") as inFile:\n\nWe seem to have lost \"w+\" and now it is \"wb\".  I do not see a reason\nto make outFile anything but write-only, so the end result looks\ngood to me, but is it an unrelated \"bug\"fix that should be explained\nas such (e.g. \"there is no reason to make outFile read-write, so\ninstead of using 'w+' just use 'wb' while we make it unencoded\noutput by adding 'b' to it\")?\n"},{"id":"444101","messageId":"BN8PR12MB336146CB8B6FD458AE3BE42BC8759@BN8PR12MB3361.namprd12.prod.outlook.com","threadId":"57078","inReplyTo":"xmqqzgp484f3.fsf@gitster.g","subject":"RE: [PATCH 4/4] git-p4: resolve RCS keywords in binary","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-14T13:12:13Z","receivedAt":"2021-12-14T13:12:17Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"> Makes sense, and I am with others who commented on the previous\n> discussion thread that the right approach to take is to take the stuff coming\n> from Perforce as byte strings, process them as such and write them out as\n> byte strings, UNLESS we positively know what the source and destination\n> encodings are.\n> \n> And this change we see here, matching with patterns, is perfectly in line with\n> that direction.  Very nice.\n\nNot bad. Fortunately, it's not possible for $ characters to appear as a component of a multi-byte UTF-8 character, so it's possible to do the matching byte-wise.\n\n> \n> >          try:\n> > -            with os.fdopen(handle, \"w+\") as outFile, open(file, \"r\") as inFile:\n> > +            with os.fdopen(handle, \"wb\") as outFile, open(file, \"rb\") as inFile:\n> \n> We seem to have lost \"w+\" and now it is \"wb\".  I do not see a reason to make\n> outFile anything but write-only, so the end result looks good to me, but is it\n> an unrelated \"bug\"fix that should be explained as such (e.g. \"there is no\n> reason to make outFile read-write, so instead of using 'w+' just use 'wb'\n> while we make it unencoded output by adding 'b' to it\")?\n\nI am happy to split this change into a separate patch if this is preferred.\n\nJoel\n"},{"id":"444131","messageId":"20211214223601.420d8b54@ado-tr.dyn.home.arpa","threadId":"57078","inReplyTo":"20211213225441.1865782-1-jholdsworth@nvidia.com","subject":"Re: [PATCH 0/4] git-p4: fix RCS keyword processing encoding errors","fromName":"Andrew Oakley","fromEmail":"andrew@adoakley.name","sentAt":"2021-12-14T22:36:01Z","receivedAt":"2021-12-14T22:36:07Z","isPatch":true,"sender":{"key":"andrew@adoakley.name","avatar":"https://avatars.githubusercontent.com/u/1107440?v=4"},"body":"On Mon, 13 Dec 2021 22:54:37 +0000\nJoel Holdsworth <jholdsworth@nvidia.com> wrote:\n\n> This patch-set fixes a family of issues with git-p4's handling of\n> incoming text data that contains RCS keywords, when those files\n> contain bytes which are invalid UTF-8 codes.\n> \n> Among the patches is a fix for the issue, as well as some peripheral\n> tidy-ups and improvements to the existing code.\n\nFWIW, these patches look good to me.\n\nI spent a while trying to understand exactly how perforce handles the\nkeyword expansion stuff a few years ago.  Other quirks which I can\nremember are:\n- Files with a filetype of \"utf16\" files get expanded before we see\n  them.  If we want to support that in git-p4 then I think some special\n  handling will be required.\n- Lines longer than lbr.rcs.maxlen at time of commit are not considered\n  to be keyword expansions.  I don't think there is any way to handle\n  this, but hopefully it won't ever occur in practice.\n\nI'm not suggesting that these issues need to be solved as part of this\nset of patches, just thought that you might want to be aware that there\nare some more unsolved issues here.\n\nThanks\n"},{"id":"444174","messageId":"xmqqczlx4kcy.fsf@gitster.g","threadId":"57078","inReplyTo":"BN8PR12MB336146CB8B6FD458AE3BE42BC8759@BN8PR12MB3361.namprd12.prod.outlook.com","subject":"Re: [PATCH 4/4] git-p4: resolve RCS keywords in binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-15T21:41:01Z","receivedAt":"2021-12-15T21:41:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Holdsworth <jholdsworth@nvidia.com> writes:\n\n>> Makes sense, and I am with others who commented on the previous\n>> discussion thread that the right approach to take is to take the stuff coming\n>> from Perforce as byte strings, process them as such and write them out as\n>> byte strings, UNLESS we positively know what the source and destination\n>> encodings are.\n>> \n>> And this change we see here, matching with patterns, is perfectly in line with\n>> that direction.  Very nice.\n>\n> Not bad. Fortunately, it's not possible for $ characters to appear as a component of a multi-byte UTF-8 character, so it's possible to do the matching byte-wise.\n>\n>> \n>> >          try:\n>> > -            with os.fdopen(handle, \"w+\") as outFile, open(file, \"r\") as inFile:\n>> > +            with os.fdopen(handle, \"wb\") as outFile, open(file, \"rb\") as inFile:\n>> \n>> We seem to have lost \"w+\" and now it is \"wb\".  I do not see a reason to make\n>> outFile anything but write-only, so the end result looks good to me, but is it\n>> an unrelated \"bug\"fix that should be explained as such (e.g. \"there is no\n>> reason to make outFile read-write, so instead of using 'w+' just use 'wb'\n>> while we make it unencoded output by adding 'b' to it\")?\n>\n> I am happy to split this change into a separate patch if this is preferred.\n\nI do not think this is big enough for a separate patch; just a\nmention in the log message is sufficient.\n"}]}