{"thread":{"id":"57062","subject":"[PATCH 0/6] Transition git-p4.py to support Python 3 only","startedAt":"2021-12-09T20:10:52Z","lastAt":"2021-12-13T19:58:27Z","messageCount":31,"participants":["Joel Holdsworth","Junio C Hamano","rsbecker@nexbridge.com","Ævar Arnfjörð Bjarmason","David Aguilar","Luke Diamand","Fabian Stelzer","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"443687","messageId":"20211209201029.136886-1-jholdsworth@nvidia.com","threadId":"57062","inReplyTo":null,"subject":"[PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-09T20:10:23Z","receivedAt":"2021-12-09T20:10:52Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"The git-p4.py script currently implements code-paths for both Python 2 and\n3.\n\nPython 2 was discontinued in 2020, and there is no longer any officially\nsupported interpreter. Further development of git-p4.py will require\nwould-be developers to test their changes with all supported dialects of\nthe language. However, if there is no longer any supported runtime\nenvironment available, this places an unreasonable burden on the Git\nproject to maintain support for an obselete dialect of the language.\n\nThis patch-set removes all Python 2-specific code-paths, and then\napplies some simplifications to the code which are available given\nPython 3's improve delineation between bytes and strings.\n\nJoel Holdsworth (6):\n git-p4: Always pass cmd arguments to subprocess as a python lists\n git-p4: Don't print shell commands as python lists\n git-p4: Removed support for Python 2\n git-p4: Decode byte strings before printing\n git-p4: Eliminate decode_stream and encode_stream\n git-p4: Resolve RCS keywords in binary\n\n git-p4.py | 319 +++++++++++++++++++++---------------------------------\n 1 file changed, 123 insertions(+), 196 deletions(-)\n\n-- \n2.33.0\n\n"},{"id":"443689","messageId":"20211209201029.136886-2-jholdsworth@nvidia.com","threadId":"57062","inReplyTo":"20211209201029.136886-1-jholdsworth@nvidia.com","subject":"[PATCH 1/6] git-p4: Always pass cmd arguments to subprocess as a python lists","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-09T20:10:24Z","receivedAt":"2021-12-09T20:10:54Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"Signed-off-by: Joel Holdsworth \n---\n git-p4.py | 138 +++++++++++++++++++++++-------------------------------\n 1 file changed, 58 insertions(+), 80 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 2b4500226a..1a4b7331d2 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -93,10 +93,7 @@ def p4_build_cmd(cmd):\n # Provide a way to not pass this option by setting git-p4.retries to 0\n real_cmd += [\"-r\", str(retries)]\n \n- if not isinstance(cmd, list):\n- real_cmd = ' '.join(real_cmd) + ' ' + cmd\n- else:\n- real_cmd += cmd\n+ real_cmd += cmd\n \n # now check that we can actually talk to the server\n global p4_access_checked\n@@ -273,12 +270,11 @@ def run_hook_command(cmd, param):\n return subprocess.call(cli, shell=use_shell)\n \n \n-def write_pipe(c, stdin):\n+def write_pipe(c, stdin, *k, **kw):\n if verbose:\n sys.stderr.write('Writing pipe: %s\\n' % str(c))\n \n- expand = not isinstance(c, list)\n- p = subprocess.Popen(c, stdin=subprocess.PIPE, shell=expand)\n+ p = subprocess.Popen(c, stdin=subprocess.PIPE, *k, **kw)\n pipe = p.stdin\n val = pipe.write(stdin)\n pipe.close()\n@@ -293,7 +289,7 @@ def p4_write_pipe(c, stdin):\n stdin = encode_text_stream(stdin)\n return write_pipe(real_cmd, stdin)\n \n-def read_pipe_full(c):\n+def read_pipe_full(c, *k, **kw):\n \"\"\" Read output from command. Returns a tuple\n of the return status, stdout text and stderr\n text.\n@@ -301,8 +297,8 @@ def read_pipe_full(c):\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, stderr=subprocess.PIPE, shell=expand)\n+ p = subprocess.Popen(\n+ c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, *k, **kw)\n (out, err) = p.communicate()\n return (p.returncode, out, decode_text_stream(err))\n \n@@ -337,12 +333,11 @@ 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, *k, **kw):\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+ p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)\n pipe = p.stdout\n val = [decode_text_stream(line) for line in pipe.readlines()]\n if pipe.close() or p.wait():\n@@ -383,11 +378,10 @@ def p4_has_move_command():\n # assume it failed because @... was invalid changelist\n return True\n \n-def system(cmd, ignore_error=False):\n- expand = not isinstance(cmd, list)\n+def system(cmd, ignore_error=False, *k, **kw):\n if verbose:\n sys.stderr.write(\"executing %s\\n\" % str(cmd))\n- retcode = subprocess.call(cmd, shell=expand)\n+ retcode = subprocess.call(cmd, *k, **kw)\n if retcode and not ignore_error:\n raise CalledProcessError(retcode, cmd)\n \n@@ -396,8 +390,7 @@ def system(cmd, ignore_error=False):\n def p4_system(cmd):\n \"\"\"Specifically invoke p4 as the system command. \"\"\"\n real_cmd = p4_build_cmd(cmd)\n- expand = not isinstance(real_cmd, list)\n- retcode = subprocess.call(real_cmd, shell=expand)\n+ retcode = subprocess.call(real_cmd)\n if retcode:\n raise CalledProcessError(retcode, real_cmd)\n \n@@ -728,14 +721,7 @@ def isModeExecChanged(src_mode, dst_mode):\n def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n errors_as_exceptions=False):\n \n- if not isinstance(cmd, list):\n- cmd = \"-G \" + cmd\n- expand = True\n- else:\n- cmd = [\"-G\"] + cmd\n- expand = False\n-\n- cmd = p4_build_cmd(cmd)\n+ cmd = p4_build_cmd([\"-G\"] + cmd)\n if verbose:\n sys.stderr.write(\"Opening pipe: %s\\n\" % str(cmd))\n \n@@ -745,17 +731,13 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n stdin_file = None\n if stdin is not None:\n stdin_file = tempfile.TemporaryFile(prefix='p4-stdin', mode=stdin_mode)\n- if not isinstance(stdin, list):\n- stdin_file.write(stdin)\n- else:\n- for i in stdin:\n- stdin_file.write(encode_text_stream(i))\n- stdin_file.write(b'\\n')\n+ for i in stdin:\n+ stdin_file.write(encode_text_stream(i))\n+ stdin_file.write(b'\\n')\n stdin_file.flush()\n stdin_file.seek(0)\n \n p4 = subprocess.Popen(cmd,\n- shell=expand,\n stdin=stdin_file,\n stdout=subprocess.PIPE)\n \n@@ -860,7 +842,7 @@ def isValidGitDir(path):\n return git_dir(path) != None\n \n def parseRevision(ref):\n- return read_pipe(\"git rev-parse %s\" % ref).strip()\n+ return read_pipe([\"git\", \"rev-parse\", ref]).strip()\n \n def branchExists(ref):\n rev = read_pipe([\"git\", \"rev-parse\", \"-q\", \"--verify\", ref],\n@@ -966,11 +948,11 @@ def p4BranchesInGit(branchesAreInRemotes=True):\n \n branches = {}\n \n- cmdline = \"git rev-parse --symbolic \"\n+ cmdline = [\"git\", \"rev-parse\", \"--symbolic\"]\n if branchesAreInRemotes:\n- cmdline += \"--remotes\"\n+ cmdline.append(\"--remotes\")\n else:\n- cmdline += \"--branches\"\n+ cmdline.append(\"--branches\")\n \n for line in read_pipe_lines(cmdline):\n line = line.strip()\n@@ -1035,7 +1017,7 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = \"refs/remotes/p4/\", silent\n \n originPrefix = \"origin/p4/\"\n \n- for line in read_pipe_lines(\"git rev-parse --symbolic --remotes\"):\n+ for line in read_pipe_lines([\"git\", \"rev-parse\", \"--symbolic\", \"--remotes\"]):\n line = line.strip()\n if (not line.startswith(originPrefix)) or line.endswith(\"HEAD\"):\n continue\n@@ -1073,7 +1055,7 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = \"refs/remotes/p4/\", silent\n remoteHead, ','.join(settings['depot-paths'])))\n \n if update:\n- system(\"git update-ref %s %s\" % (remoteHead, originHead))\n+ system([\"git\", \"update-ref\", remoteHead, originHead])\n \n def originP4BranchesExist():\n return gitBranchExists(\"origin\") or gitBranchExists(\"origin/p4\") or gitBranchExists(\"origin/p4/master\")\n@@ -1187,7 +1169,7 @@ def getClientSpec():\n \"\"\"Look at the p4 client spec, create a View() object that contains\n all the mappings, and return it.\"\"\"\n \n- specList = p4CmdList(\"client -o\")\n+ specList = p4CmdList([\"client\", \"-o\"])\n if len(specList) != 1:\n die('Output from \"client -o\" is %d lines, expecting 1' %\n len(specList))\n@@ -1216,7 +1198,7 @@ def getClientSpec():\n def getClientRoot():\n \"\"\"Grab the client directory.\"\"\"\n \n- output = p4CmdList(\"client -o\")\n+ output = p4CmdList([\"client\", \"-o\"])\n if len(output) != 1:\n die('Output from \"client -o\" is %d lines, expecting 1' % len(output))\n \n@@ -1471,7 +1453,7 @@ def p4UserId(self):\n if self.myP4UserId:\n return self.myP4UserId\n \n- results = p4CmdList(\"user -o\")\n+ results = p4CmdList([\"user\", \"-o\"])\n for r in results:\n if 'User' in r:\n self.myP4UserId = r['User']\n@@ -1496,7 +1478,7 @@ def getUserMapFromPerforceServer(self):\n self.users = {}\n self.emails = {}\n \n- for output in p4CmdList(\"users\"):\n+ for output in p4CmdList([\"users\"]):\n if \"User\" not in output:\n continue\n self.users[output[\"User\"]] = output[\"FullName\"] + \" <\" + output[\"Email\"] + \">\"\n@@ -1566,10 +1548,10 @@ def run(self, args):\n \n if self.rollbackLocalBranches:\n refPrefix = \"refs/heads/\"\n- lines = read_pipe_lines(\"git rev-parse --symbolic --branches\")\n+ lines = read_pipe_lines([\"git\", \"rev-parse\", \"--symbolic\", \"--branches\"])\n else:\n refPrefix = \"refs/remotes/\"\n- lines = read_pipe_lines(\"git rev-parse --symbolic --remotes\")\n+ lines = read_pipe_lines([\"git\", \"rev-parse\", \"--symbolic\", \"--remotes\"])\n \n for line in lines:\n if self.rollbackLocalBranches or (line.startswith(\"p4/\") and line != \"p4/HEAD\\n\"):\n@@ -1586,14 +1568,14 @@ def run(self, args):\n if len(p4Cmd(\"changes -m 1 \" + ' '.join (['%s...@%s' % (p, maxChange)\n for p in depotPaths]))) == 0:\n print(\"Branch %s did not exist at change %s, deleting.\" % (ref, maxChange))\n- system(\"git update-ref -d %s `git rev-parse %s`\" % (ref, ref))\n+ system(\"git update-ref -d {ref} `git rev-parse {ref}`\".format(ref=ref), shell=True)\n continue\n \n while change and int(change) > maxChange:\n changed = True\n if self.verbose:\n print(\"%s is at %s ; rewinding towards %s\" % (ref, change, maxChange))\n- system(\"git update-ref %s \\\"%s^\\\"\" % (ref, ref))\n+ system([\"git\", \"update-ref\", ref, \"{}^\".format(ref)])\n log = extractLogMessageFromGitCommit(ref)\n settings = extractSettingsGitLog(log)\n \n@@ -1694,7 +1676,7 @@ def __init__(self):\n die(\"Large file system not supported for git-p4 submit command. Please remove it from config.\")\n \n def check(self):\n- if len(p4CmdList(\"opened ...\")) > 0:\n+ if len(p4CmdList([\"opened\", \"...\"])) > 0:\n die(\"You have files opened with perforce! Close them before starting the sync.\")\n \n def separate_jobs_from_description(self, message):\n@@ -1803,7 +1785,7 @@ def lastP4Changelist(self):\n # then gets used to patch up the username in the change. If the same\n # client spec is being used by multiple processes then this might go\n # wrong.\n- results = p4CmdList(\"client -o\") # find the current client\n+ results = p4CmdList([\"client\", \"-o\"]) # find the current client\n client = None\n for r in results:\n if 'Client' in r:\n@@ -1819,7 +1801,7 @@ def lastP4Changelist(self):\n \n def modifyChangelistUser(self, changelist, newUser):\n # fixup the user field of a changelist after it has been submitted.\n- changes = p4CmdList(\"change -o %s\" % changelist)\n+ changes = p4CmdList([\"change\", \"-o\", changelist])\n if len(changes) != 1:\n die(\"Bad output from p4 change modifying %s to user %s\" %\n (changelist, newUser))\n@@ -1830,7 +1812,7 @@ def modifyChangelistUser(self, changelist, newUser):\n # p4 does not understand format version 3 and above\n input = marshal.dumps(c, 2)\n \n- result = p4CmdList(\"change -f -i\", stdin=input)\n+ result = p4CmdList([\"change\", \"-f\", \"-i\"], stdin=input)\n for r in result:\n if 'code' in r:\n if r['code'] == 'error':\n@@ -1936,7 +1918,7 @@ def edit_template(self, template_file):\n if \"P4EDITOR\" in os.environ and (os.environ.get(\"P4EDITOR\") != \"\"):\n editor = os.environ.get(\"P4EDITOR\")\n else:\n- editor = read_pipe(\"git var GIT_EDITOR\").strip()\n+ editor = read_pipe([\"git\", \"var\", \"GIT_EDITOR\"]).strip()\n system([\"sh\", \"-c\", ('%s \"$@\"' % editor), editor, template_file])\n \n # If the file was not saved, prompt to see if this patch should\n@@ -1994,7 +1976,7 @@ def applyCommit(self, id):\n \n (p4User, gitEmail) = self.p4UserForCommit(id)\n \n- diff = read_pipe_lines(\"git diff-tree -r %s \\\"%s^\\\" \\\"%s\\\"\" % (self.diffOpts, id, id))\n+ diff = read_pipe_lines([\"git\", \"diff-tree\", \"-r\"] + self.diffOpts + [\"{}^\".format(id), id])\n filesToAdd = set()\n filesToChangeType = set()\n filesToDelete = set()\n@@ -2131,7 +2113,7 @@ def applyCommit(self, id):\n #\n # Apply the patch for real, and do add/delete/+x handling.\n #\n- system(applyPatchCmd)\n+ system(applyPatchCmd, shell=True)\n \n for f in filesToChangeType:\n p4_edit(f, \"-t\", \"auto\")\n@@ -2481,17 +2463,17 @@ def run(self, args):\n #\n if self.detectRenames:\n # command-line -M arg\n- self.diffOpts = \"-M\"\n+ self.diffOpts = [\"-M\"]\n else:\n # If not explicitly set check the config variable\n detectRenames = gitConfig(\"git-p4.detectRenames\")\n \n if detectRenames.lower() == \"false\" or detectRenames == \"\":\n- self.diffOpts = \"\"\n+ self.diffOpts = []\n elif detectRenames.lower() == \"true\":\n- self.diffOpts = \"-M\"\n+ self.diffOpts = [\"-M\"]\n else:\n- self.diffOpts = \"-M%s\" % detectRenames\n+ self.diffOpts = [\"-M{}\".format(detectRenames)]\n \n # no command-line arg for -C or --find-copies-harder, just\n # config variables\n@@ -2499,12 +2481,12 @@ def run(self, args):\n if detectCopies.lower() == \"false\" or detectCopies == \"\":\n pass\n elif detectCopies.lower() == \"true\":\n- self.diffOpts += \" -C\"\n+ self.diffOpts.append(\"-C\")\n else:\n- self.diffOpts += \" -C%s\" % detectCopies\n+ self.diffOpts.append(\"-C{}\".format(detectCopies))\n \n if gitConfigBool(\"git-p4.detectCopiesHarder\"):\n- self.diffOpts += \" --find-copies-harder\"\n+ self.diffOpts.append(\"--find-copies-harder\")\n \n num_shelves = len(self.update_shelve)\n if num_shelves > 0 and num_shelves != len(commits):\n@@ -3453,10 +3435,9 @@ def getBranchMapping(self):\n lostAndFoundBranches = set()\n \n user = gitConfig(\"git-p4.branchUser\")\n+ command = [\"branches\"]\n if len(user) > 0:\n- command = \"branches -u %s\" % user\n- else:\n- command = \"branches\"\n+ command += [\"-u\", user]\n \n for info in p4CmdList(command):\n details = p4Cmd([\"branch\", \"-o\", info[\"branch\"]])\n@@ -3549,7 +3530,7 @@ def gitCommitByP4Change(self, ref, change):\n while True:\n if self.verbose:\n print(\"trying: earliest %s latest %s\" % (earliestCommit, latestCommit))\n- next = read_pipe(\"git rev-list --bisect %s %s\" % (latestCommit, earliestCommit)).strip()\n+ next = read_pipe([\"git\", \"rev-list\", \"--bisect\", latestCommit, earliestCommit]).strip()\n if len(next) == 0:\n if self.verbose:\n print(\"argh\")\n@@ -3704,7 +3685,7 @@ def sync_origin_only(self):\n if self.hasOrigin:\n if not self.silent:\n print('Syncing with origin first, using \"git fetch origin\"')\n- system(\"git fetch origin\")\n+ system([\"git\", \"fetch\", \"origin\"])\n \n def importHeadRevision(self, revision):\n print(\"Doing initial import of %s from revision %s into %s\" % (' '.join(self.depotPaths), revision, self.branch))\n@@ -3871,8 +3852,8 @@ def run(self, args):\n if len(self.branch) == 0:\n self.branch = self.refPrefix + \"master\"\n if gitBranchExists(\"refs/heads/p4\") and self.importIntoRemotes:\n- system(\"git update-ref %s refs/heads/p4\" % self.branch)\n- system(\"git branch -D p4\")\n+ system([\"git\", \"update-ref\", self.branch, \"refs/heads/p4\"])\n+ system([\"git\", \"branch\", \"-D\", \"p4\"])\n \n # accept either the command-line option, or the configuration variable\n if self.useClientSpec:\n@@ -4075,7 +4056,7 @@ def run(self, args):\n # Cleanup temporary branches created during import\n if self.tempBranches != []:\n for branch in self.tempBranches:\n- read_pipe(\"git update-ref -d %s\" % branch)\n+ read_pipe([\"git\", \"update-ref\", \"-d\", branch])\n os.rmdir(os.path.join(os.environ.get(\"GIT_DIR\", \".git\"), self.tempBranchLocation))\n \n # Create a symbolic ref p4/HEAD pointing to p4/ to allow\n@@ -4107,7 +4088,7 @@ def run(self, args):\n def rebase(self):\n if os.system(\"git update-index --refresh\") != 0:\n die(\"Some files in your working directory are modified and different than what is in your index. You can use git update-index to bring the index up to date or stash away all your changes with git stash.\");\n- if len(read_pipe(\"git diff-index HEAD --\")) > 0:\n+ if len(read_pipe([\"git\", \"diff-index\", \"HEAD\", \"--\"])) > 0:\n die(\"You have uncommitted changes. Please commit them before rebasing or stash them away with git stash.\");\n \n [upstream, settings] = findUpstreamBranchPoint()\n@@ -4118,9 +4099,9 @@ def rebase(self):\n upstream = re.sub(\"~[0-9]+$\", \"\", upstream)\n \n print(\"Rebasing the current branch onto %s\" % upstream)\n- oldHead = read_pipe(\"git rev-parse HEAD\").strip()\n- system(\"git rebase %s\" % upstream)\n- system(\"git diff-tree --stat --summary -M %s HEAD --\" % oldHead)\n+ oldHead = read_pipe([\"git\", \"rev-parse\", \"HEAD\"]).strip()\n+ system([\"git\", \"rebase\", upstream])\n+ system([\"git\", \"diff-tree\", \"--stat\", \"--summary\", \"-M\", oldHead, \"HEAD\", \"--\"])\n return True\n \n class P4Clone(P4Sync):\n@@ -4197,7 +4178,7 @@ def run(self, args):\n \n # auto-set this variable if invoked with --use-client-spec\n if self.useClientSpec_from_options:\n- system(\"git config --bool git-p4.useclientspec true\")\n+ system([\"git\", \"config\", \"--bool\", \"git-p4.useclientspec\", \"true\"])\n \n return True\n \n@@ -4328,10 +4309,7 @@ def run(self, args):\n if originP4BranchesExist():\n createOrUpdateBranchesFromOrigin()\n \n- cmdline = \"git rev-parse --symbolic \"\n- cmdline += \" --remotes\"\n-\n- for line in read_pipe_lines(cmdline):\n+ for line in read_pipe_lines([\"git\", \"rev-parse\", \"--symbolic\", \"--remotes\"]):\n line = line.strip()\n \n if not line.startswith('p4/') or line == \"p4/HEAD\":\n@@ -4416,9 +4394,9 @@ def main():\n cmd.gitdir = os.path.abspath(\".git\")\n if not isValidGitDir(cmd.gitdir):\n # \"rev-parse --git-dir\" without arguments will try $PWD/.git\n- cmd.gitdir = read_pipe(\"git rev-parse --git-dir\").strip()\n+ cmd.gitdir = read_pipe([\"git\", \"rev-parse\", \"--git-dir\"]).strip()\n if os.path.exists(cmd.gitdir):\n- cdup = read_pipe(\"git rev-parse --show-cdup\").strip()\n+ cdup = read_pipe([\"git\", \"rev-parse\", \"--show-cdup\"]).strip()\n if len(cdup) > 0:\n chdir(cdup);\n \n-- \n2.33.0\n\n"},{"id":"443688","messageId":"20211209201029.136886-3-jholdsworth@nvidia.com","threadId":"57062","inReplyTo":"20211209201029.136886-1-jholdsworth@nvidia.com","subject":"[PATCH 2/6] git-p4: Don't print shell commands as python lists","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-09T20:10:25Z","receivedAt":"2021-12-09T20:10:55Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"Signed-off-by: Joel Holdsworth \n---\n git-p4.py | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 1a4b7331d2..32f30e5f9a 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -272,14 +272,14 @@ def run_hook_command(cmd, param):\n \n def write_pipe(c, stdin, *k, **kw):\n if verbose:\n- sys.stderr.write('Writing pipe: %s\\n' % str(c))\n+ sys.stderr.write('Writing pipe: {}\\n'.format(' '.join(c)))\n \n p = subprocess.Popen(c, stdin=subprocess.PIPE, *k, **kw)\n pipe = p.stdin\n val = pipe.write(stdin)\n pipe.close()\n if p.wait():\n- die('Command failed: %s' % str(c))\n+ die('Command failed: {}'.format(' '.join(c)))\n \n return val\n \n@@ -295,7 +295,7 @@ def read_pipe_full(c, *k, **kw):\n text.\n \"\"\"\n if verbose:\n- sys.stderr.write('Reading pipe: %s\\n' % str(c))\n+ sys.stderr.write('Reading pipe: {}\\n'.format(' '.join(c)))\n \n p = subprocess.Popen(\n c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, *k, **kw)\n@@ -314,7 +314,7 @@ def read_pipe(c, ignore_error=False, raw=False):\n if ignore_error:\n out = \"\"\n else:\n- die('Command failed: %s\\nError: %s' % (str(c), err))\n+ die('Command failed: {}\\nError: {}'.format(' '.join(c), err))\n if not raw:\n out = decode_text_stream(out)\n return out\n@@ -335,13 +335,13 @@ def p4_read_pipe(c, ignore_error=False, raw=False):\n \n def read_pipe_lines(c, *k, **kw):\n if verbose:\n- sys.stderr.write('Reading pipe: %s\\n' % str(c))\n+ sys.stderr.write('Reading pipe: {}\\n'.format(' '.join(c)))\n \n p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)\n pipe = p.stdout\n val = [decode_text_stream(line) for line in pipe.readlines()]\n if pipe.close() or p.wait():\n- die('Command failed: %s' % str(c))\n+ die('Command failed: {}'.format(' '.join(c)))\n return val\n \n def p4_read_pipe_lines(c):\n@@ -380,7 +380,8 @@ def p4_has_move_command():\n \n def system(cmd, ignore_error=False, *k, **kw):\n if verbose:\n- sys.stderr.write(\"executing %s\\n\" % str(cmd))\n+ sys.stderr.write(\"executing {}\\n\".format(\n+ ' '.join(cmd) if isinstance(cmd, list) else cmd))\n retcode = subprocess.call(cmd, *k, **kw)\n if retcode and not ignore_error:\n raise CalledProcessError(retcode, cmd)\n@@ -723,7 +724,7 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n \n cmd = p4_build_cmd([\"-G\"] + cmd)\n if verbose:\n- sys.stderr.write(\"Opening pipe: %s\\n\" % str(cmd))\n+ sys.stderr.write(\"Opening pipe: {}\\n\".format(' '.join(cmd)))\n \n # Use a temporary file to avoid deadlocks without\n # subprocess.communicate(), which would put another copy\n-- \n2.33.0\n\n"},{"id":"443691","messageId":"20211209201029.136886-4-jholdsworth@nvidia.com","threadId":"57062","inReplyTo":"20211209201029.136886-1-jholdsworth@nvidia.com","subject":"[PATCH 3/6] git-p4: Removed support for Python 2","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-09T20:10:26Z","receivedAt":"2021-12-09T20:10:56Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"Signed-off-by: Joel Holdsworth \n---\n git-p4.py | 89 +++++++++++++++++--------------------------------------\n 1 file changed, 28 insertions(+), 61 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 32f30e5f9a..b5d4fc1176 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1,4 +1,4 @@\n-#!/usr/bin/env python\n+#!/usr/bin/env python3\n #\n # git-p4.py -- A tool for bidirectional operation between a Perforce depot and git.\n #\n@@ -16,8 +16,8 @@\n # pylint: disable=too-many-branches,too-many-nested-blocks\n #\n import sys\n-if sys.version_info.major < 3 and sys.version_info.minor < 7:\n- sys.stderr.write(\"git-p4: requires Python 2.7 or later.\\n\")\n+if sys.version_info.major < 3 or (sys.version_info.major == 3 and sys.version_info.minor < 7):\n+ sys.stderr.write(\"git-p4: requires Python 3.7 or later.\\n\")\n sys.exit(1)\n import os\n import optparse\n@@ -36,16 +36,6 @@\n import errno\n import glob\n \n-# On python2.7 where raw_input() and input() are both availble,\n-# we want raw_input's semantics, but aliased to input for python3\n-# compatibility\n-# support basestring in python3\n-try:\n- if raw_input and input:\n- input = raw_input\n-except:\n- pass\n-\n verbose = False\n \n # Only labels/tags matching this will be imported/exported\n@@ -173,35 +163,16 @@ def prompt(prompt_text):\n if response in choices:\n return response\n \n-# We need different encoding/decoding strategies for text data being passed\n-# around in pipes depending on python version\n-if bytes is not str:\n- # For python3, always encode and decode as appropriate\n- def decode_text_stream(s):\n- return s.decode() if isinstance(s, bytes) else s\n- def encode_text_stream(s):\n- return s.encode() if isinstance(s, str) else s\n-else:\n- # For python2.7, pass read strings as-is, but also allow writing unicode\n- def decode_text_stream(s):\n- return s\n- def encode_text_stream(s):\n- return s.encode('utf_8') if isinstance(s, unicode) else s\n+def decode_text_stream(s):\n+ return s.decode() if isinstance(s, bytes) else s\n+def encode_text_stream(s):\n+ return s.encode() if isinstance(s, str) else s\n \n def decode_path(path):\n \"\"\"Decode a given string (bytes or otherwise) using configured path encoding options\n \"\"\"\n encoding = gitConfig('git-p4.pathEncoding') or 'utf_8'\n- if bytes is not str:\n- return path.decode(encoding, errors='replace') if isinstance(path, bytes) else path\n- else:\n- try:\n- path.decode('ascii')\n- except:\n- path = path.decode(encoding, errors='replace')\n- if verbose:\n- print('Path with non-ASCII characters detected. Used {} to decode: {}'.format(encoding, path))\n- return path\n+ return path.decode(encoding, errors='replace') if isinstance(path, bytes) else path\n \n def run_git_hook(cmd, param=[]):\n \"\"\"Execute a hook if the hook exists.\"\"\"\n@@ -285,8 +256,8 @@ def write_pipe(c, stdin, *k, **kw):\n \n def p4_write_pipe(c, stdin):\n real_cmd = p4_build_cmd(c)\n- if bytes is not str and isinstance(stdin, str):\n- stdin = encode_text_stream(stdin)\n+ if isinstance(stdin, str):\n+ stdin = stdin.encode()\n return write_pipe(real_cmd, stdin)\n \n def read_pipe_full(c, *k, **kw):\n@@ -745,21 +716,18 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n result = []\n try:\n while True:\n- entry = marshal.load(p4.stdout)\n- if bytes is not str:\n- # Decode unmarshalled dict to use str keys and values, except for:\n- # - `data` which may contain arbitrary binary data\n- # - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text\n- decoded_entry = {}\n- for key, value in entry.items():\n- key = key.decode()\n- if isinstance(value, bytes) and not (key in ('data', 'path', 'clientFile') or key.startswith('depotFile')):\n- value = value.decode()\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- decoded_entry['data'] = decoded_entry['data'].decode()\n- entry = decoded_entry\n+ # Decode unmarshalled dict to use str keys and values, except for:\n+ # - `data` which may contain arbitrary binary data\n+ # - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text\n+ entry = {}\n+ for key, value in marshal.load(p4.stdout).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+ entry[key] = value\n+ # Parse out data if it's an error response\n+ if entry.get('code') == 'error' and 'data' in entry:\n+ entry['data'] = entry['data'].decode()\n if skip_info:\n if 'code' in entry and entry['code'] == 'info':\n continue\n@@ -3822,14 +3790,13 @@ def openStreams(self):\n self.gitStream = self.importProcess.stdin\n self.gitError = self.importProcess.stderr\n \n- if bytes is not str:\n- # Wrap gitStream.write() so that it can be called using `str` arguments\n- def make_encoded_write(write):\n- def encoded_write(s):\n- return write(s.encode() if isinstance(s, str) else s)\n- return encoded_write\n+ # Wrap gitStream.write() so that it can be called using `str` arguments\n+ def make_encoded_write(write):\n+ def encoded_write(s):\n+ return write(s.encode() if isinstance(s, str) else s)\n+ return encoded_write\n \n- self.gitStream.write = make_encoded_write(self.gitStream.write)\n+ self.gitStream.write = make_encoded_write(self.gitStream.write)\n \n def closeStreams(self):\n if self.gitStream is None:\n-- \n2.33.0\n\n"},{"id":"443690","messageId":"20211209201029.136886-5-jholdsworth@nvidia.com","threadId":"57062","inReplyTo":"20211209201029.136886-1-jholdsworth@nvidia.com","subject":"[PATCH 4/6] git-p4: Decode byte strings before printing","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-09T20:10:27Z","receivedAt":"2021-12-09T20:10:57Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"Signed-off-by: Joel Holdsworth \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 b5d4fc1176..b5945a0306 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -2917,7 +2917,8 @@ def streamOneP4File(self, file, contents):\n size = int(self.stream_file['fileSize'])\n else:\n size = 0 # deleted files don't get a fileSize apparently\n- sys.stdout.write('\\r%s --> %s (%i MB)\\n' % (file_path, relPath, size/1024/1024))\n+ sys.stdout.write('\\r{} --> {} ({} MB)\\n'.format(\n+ file_path.decode(), relPath, size/1024/1024))\n sys.stdout.flush()\n \n (type_base, type_mods) = split_p4_type(file[\"type\"])\n@@ -3061,7 +3062,8 @@ def streamP4FilesCb(self, marshalled):\n size = int(self.stream_file[\"fileSize\"])\n if size > 0:\n progress = 100*self.stream_file['streamContentSize']/size\n- sys.stdout.write('\\r%s %d%% (%i MB)' % (self.stream_file['depotFile'], progress, int(size/1024/1024)))\n+ sys.stdout.write('\\r{} {}% ({} MB)'.format(\n+ self.stream_file['depotFile'].decode(), progress, int(size/1024/1024)))\n sys.stdout.flush()\n \n self.stream_have_file_info = True\n@@ -3803,7 +3805,7 @@ def closeStreams(self):\n return\n self.gitStream.close()\n if self.importProcess.wait() != 0:\n- die(\"fast-import failed: %s\" % self.gitError.read())\n+ die(\"fast-import failed: {}\".format(self.gitError.read().decode()))\n self.gitOutput.close()\n self.gitError.close()\n self.gitStream = None\n-- \n2.33.0\n\n"},{"id":"443692","messageId":"20211209201029.136886-6-jholdsworth@nvidia.com","threadId":"57062","inReplyTo":"20211209201029.136886-1-jholdsworth@nvidia.com","subject":"[PATCH 5/6] git-p4: Eliminate decode_stream and encode_stream","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-09T20:10:28Z","receivedAt":"2021-12-09T20:11:03Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"Signed-off-by: Joel Holdsworth \n---\n git-p4.py | 50 ++++++++++++++++++++------------------------------\n 1 file changed, 20 insertions(+), 30 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex b5945a0306..c362a5fa38 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -163,11 +163,6 @@ def prompt(prompt_text):\n if response in choices:\n return response\n \n-def decode_text_stream(s):\n- return s.decode() if isinstance(s, bytes) else s\n-def encode_text_stream(s):\n- return s.encode() if isinstance(s, str) else s\n-\n def decode_path(path):\n \"\"\"Decode a given string (bytes or otherwise) using configured path encoding options\n \"\"\"\n@@ -271,7 +266,7 @@ def read_pipe_full(c, *k, **kw):\n p = subprocess.Popen(\n c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, *k, **kw)\n (out, err) = p.communicate()\n- return (p.returncode, out, decode_text_stream(err))\n+ return (p.returncode, out, err.decode())\n \n def read_pipe(c, ignore_error=False, raw=False):\n \"\"\" Read output from command. Returns the output text on\n@@ -283,22 +278,17 @@ def read_pipe(c, ignore_error=False, raw=False):\n (retcode, out, err) = read_pipe_full(c)\n if retcode != 0:\n if ignore_error:\n- out = \"\"\n+ out = b\"\"\n else:\n die('Command failed: {}\\nError: {}'.format(' '.join(c), err))\n- if not raw:\n- out = decode_text_stream(out)\n- return out\n+ return out if raw else out.decode()\n \n def read_pipe_text(c):\n \"\"\" Read output from a command with trailing whitespace stripped.\n On error, returns None.\n \"\"\"\n (retcode, out, err) = read_pipe_full(c)\n- if retcode != 0:\n- return None\n- else:\n- return decode_text_stream(out).rstrip()\n+ return out.decode().rstrip() if retcode == 0 else None\n \n def p4_read_pipe(c, ignore_error=False, raw=False):\n real_cmd = p4_build_cmd(c)\n@@ -310,7 +300,7 @@ def read_pipe_lines(c, *k, **kw):\n \n p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)\n pipe = p.stdout\n- val = [decode_text_stream(line) for line in pipe.readlines()]\n+ val = [line.decode() for line in pipe.readlines()]\n if pipe.close() or p.wait():\n die('Command failed: {}'.format(' '.join(c)))\n return val\n@@ -340,7 +330,7 @@ def p4_has_move_command():\n cmd = p4_build_cmd([\"move\", \"-k\", \"@from\", \"@to\"])\n p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)\n (out, err) = p.communicate()\n- err = decode_text_stream(err)\n+ err = err.decode()\n # return code will be 1 in either case\n if err.find(\"Invalid option\") >= 0:\n return False\n@@ -704,7 +694,7 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,\n if stdin is not None:\n stdin_file = tempfile.TemporaryFile(prefix='p4-stdin', mode=stdin_mode)\n for i in stdin:\n- stdin_file.write(encode_text_stream(i))\n+ stdin_file.write(i.encode())\n stdin_file.write(b'\\n')\n stdin_file.flush()\n stdin_file.seek(0)\n@@ -945,8 +935,7 @@ def branch_exists(branch):\n \n cmd = [ \"git\", \"rev-parse\", \"--symbolic\", \"--verify\", branch ]\n p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)\n- out, _ = p.communicate()\n- out = decode_text_stream(out)\n+ out = p.communicate()[0].decode()\n if p.returncode:\n return False\n # expect exactly one line of output: the branch name\n@@ -1331,7 +1320,7 @@ def generatePointer(self, contentFile):\n ['git', 'lfs', 'pointer', '--file=' + contentFile],\n stdout=subprocess.PIPE\n )\n- pointerFile = decode_text_stream(pointerProcess.stdout.read())\n+ pointerFile = pointerProcess.stdout.read().decode()\n if pointerProcess.wait():\n os.remove(contentFile)\n die('git-lfs pointer command failed. Did you install the extension?')\n@@ -2130,7 +2119,7 @@ def applyCommit(self, id):\n tmpFile = os.fdopen(handle, \"w+b\")\n if self.isWindows:\n submitTemplate = submitTemplate.replace(\"\\n\", \"\\r\\n\")\n- tmpFile.write(encode_text_stream(submitTemplate))\n+ tmpFile.write(submitTemplate.encode())\n tmpFile.close()\n \n submitted = False\n@@ -2186,8 +2175,8 @@ def applyCommit(self, id):\n return False\n \n # read the edited message and submit\n- tmpFile = open(fileName, \"rb\")\n- message = decode_text_stream(tmpFile.read())\n+ with open(fileName, \"r\") as tmpFile:\n+ message = tmpFile.read()\n tmpFile.close()\n if self.isWindows:\n message = message.replace(\"\\r\\n\", \"\\n\")\n@@ -2887,7 +2876,7 @@ def splitFilesIntoBranches(self, commit):\n return branches\n \n def writeToGitStream(self, gitMode, relPath, contents):\n- self.gitStream.write(encode_text_stream(u'M {} inline {}\\n'.format(gitMode, relPath)))\n+ self.gitStream.write('M {} inline {}\\n'.format(gitMode, relPath))\n self.gitStream.write('data %d\\n' % sum(len(d) for d in contents))\n for d in contents:\n self.gitStream.write(d)\n@@ -2930,7 +2919,7 @@ def streamOneP4File(self, file, contents):\n git_mode = \"120000\"\n # p4 print on a symlink sometimes contains \"target\\n\";\n # if it does, remove the newline\n- data = ''.join(decode_text_stream(c) for c in contents)\n+ data = ''.join(c.decode() for c in contents)\n if not data:\n # Some version of p4 allowed creating a symlink that pointed\n # to nothing. This causes p4 errors when checking out such\n@@ -2984,9 +2973,9 @@ def streamOneP4File(self, file, contents):\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 = ''.join(c.decode() for c in contents)\n text = regexp.sub(r'$\\1$', text)\n- contents = [ encode_text_stream(text) ]\n+ contents = [text.encode()]\n \n if self.largeFileSystem:\n (git_mode, contents) = self.largeFileSystem.processContent(git_mode, relPath, contents)\n@@ -2998,7 +2987,7 @@ def streamOneP4Deletion(self, file):\n if verbose:\n sys.stdout.write(\"delete %s\\n\" % relPath)\n sys.stdout.flush()\n- self.gitStream.write(encode_text_stream(u'D {}\\n'.format(relPath)))\n+ self.gitStream.write('D {}\\n'.format(relPath))\n \n if self.largeFileSystem and self.largeFileSystem.isLargeFile(relPath):\n self.largeFileSystem.removeLargeFile(relPath)\n@@ -3096,12 +3085,13 @@ def streamP4FilesCbSelf(entry):\n \n fileArgs = []\n for f in filesToRead:\n+ fileArg = f['path'].decode()\n if 'shelved_cl' in f:\n # Handle shelved CLs using the \"p4 print file@=N\" syntax to print\n # the contents\n- fileArg = f['path'] + encode_text_stream('@={}'.format(f['shelved_cl']))\n+ fileArg += '@={}'.format(f['shelved_cl'])\n else:\n- fileArg = f['path'] + encode_text_stream('#{}'.format(f['rev']))\n+ fileArg += '#{}'.format(f['rev'])\n \n fileArgs.append(fileArg)\n \n-- \n2.33.0\n\n"},{"id":"443693","messageId":"20211209201029.136886-7-jholdsworth@nvidia.com","threadId":"57062","inReplyTo":"20211209201029.136886-1-jholdsworth@nvidia.com","subject":"[PATCH 6/6] git-p4: Resolve RCS keywords in binary","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-09T20:10:29Z","receivedAt":"2021-12-09T20:11:04Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"Signed-off-by: Joel Holdsworth \n---\n git-p4.py | 31 ++++++++++---------------------\n 1 file changed, 10 insertions(+), 21 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex c362a5fa38..87e6685eb6 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -46,6 +46,9 @@\n \n p4_access_checked = False\n \n+re_ko_keywords = re.compile(rb'\\$(Id|Header)(:[^$\\n]+)?\\$')\n+re_k_keywords = re.compile(rb'\\$(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@@ -532,20 +535,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@@ -2035,11 +2030,10 @@ def applyCommit(self, id):\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+ regexp = p4_keywords_regexp_for_file(file)\n \n- if pattern:\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@@ -2968,14 +2962,9 @@ def streamOneP4File(self, file, contents):\n print(\"\\nIgnoring apple filetype file %s\" % file['depotFile'])\n return\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(c.decode() for c in contents)\n- text = regexp.sub(r'$\\1$', text)\n- contents = [text.encode()]\n+ regexp = p4_keywords_regexp_for_type(type_base, type_mods)\n+ if regexp:\n+ contents = [regexp.sub(rb'$\\1$', 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":"443710","messageId":"xmqqo85po0xk.fsf@gitster.g","threadId":"57062","inReplyTo":"20211209201029.136886-2-jholdsworth@nvidia.com","subject":"Re: [PATCH 1/6] git-p4: Always pass cmd arguments to subprocess as a python lists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-09T22:42:15Z","receivedAt":"2021-12-09T22:42:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Holdsworth writes:\n\n> Subject: Re: [PATCH 1/6] git-p4: Always pass cmd arguments to subprocess as a python lists\n\nStyle: downcase \"Always\" or anything that comes after the initial\n\"area\" designator, i.e. \"git-p4:\".\n\nThe most important bit is missing from the proposed log message.\n\nIt says what the updated code does clearly (i.e. if we find a code\nthat still passes cmd arguments to subprocess not as a list, the\ntitle tells us that this patch did not do a thorough job at it),\nwhich is good.\n\nBut it does not explain why we want to do so. There must be some\nbackstory about wanting to do so (e.g. \"because it is more Python3\nway of doing things\") that we can explain to the future readers of\n\"git log\" output. Being aware of the general direction these\npatches are taking us early would help the readers.\n"},{"id":"443711","messageId":"xmqqh7bho0to.fsf@gitster.g","threadId":"57062","inReplyTo":"20211209201029.136886-4-jholdsworth@nvidia.com","subject":"Re: [PATCH 3/6] git-p4: Removed support for Python 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-09T22:44:35Z","receivedAt":"2021-12-09T22:44:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Holdsworth writes:\n\n> Subject: Re: [PATCH 3/6] git-p4: Removed support for Python 2\n\n\"Removed\" -> \"remove\".\n\nLosing unused/no longer usable code is good.\n\n> Signed-off-by: Joel Holdsworth \n> ---\n> git-p4.py | 89 +++++++++++++++++--------------------------------------\n> 1 file changed, 28 insertions(+), 61 deletions(-)\n\nIn these 28 new/replacement lines, there is nothing that deserves\nany mention in the proposed log message?\n"},{"id":"443713","messageId":"xmqqczm5o0pa.fsf@gitster.g","threadId":"57062","inReplyTo":"20211209201029.136886-5-jholdsworth@nvidia.com","subject":"Re: [PATCH 4/6] git-p4: Decode byte strings before printing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-09T22:47:13Z","receivedAt":"2021-12-09T22:47:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Holdsworth writes:\n\n> Signed-off-by: Joel Holdsworth \n> ---\n> git-p4.py | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n\nIs the use of strings with {} placeholders and their .format() method\nintegral part of \"decoding byte strings before printing\", or it is just\na new/better/improved/subjectively-preferred/whatever style?\n\nIf the latter, such a change should be separated into its own step,\nor at least needs to be mentioned and justified in the proposed log\nmessage.\n\nLack of explanation on \"why\" is shared among all these patches, it\nseems, so I won't repeat, but the patches need to explain why to\ntheir readers.\n\n> diff --git a/git-p4.py b/git-p4.py\n> index b5d4fc1176..b5945a0306 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -2917,7 +2917,8 @@ def streamOneP4File(self, file, contents):\n> size = int(self.stream_file['fileSize'])\n> else:\n> size = 0 # deleted files don't get a fileSize apparently\n> - sys.stdout.write('\\r%s --> %s (%i MB)\\n' % (file_path, relPath, size/1024/1024))\n> + sys.stdout.write('\\r{} --> {} ({} MB)\\n'.format(\n> + file_path.decode(), relPath, size/1024/1024))\n> sys.stdout.flush()\n> \n> (type_base, type_mods) = split_p4_type(file[\"type\"])\n> @@ -3061,7 +3062,8 @@ def streamP4FilesCb(self, marshalled):\n> size = int(self.stream_file[\"fileSize\"])\n> if size > 0:\n> progress = 100*self.stream_file['streamContentSize']/size\n> - sys.stdout.write('\\r%s %d%% (%i MB)' % (self.stream_file['depotFile'], progress, int(size/1024/1024)))\n> + sys.stdout.write('\\r{} {}% ({} MB)'.format(\n> + self.stream_file['depotFile'].decode(), progress, int(size/1024/1024)))\n> sys.stdout.flush()\n> \n> self.stream_have_file_info = True\n> @@ -3803,7 +3805,7 @@ def closeStreams(self):\n> return\n> self.gitStream.close()\n> if self.importProcess.wait() != 0:\n> - die(\"fast-import failed: %s\" % self.gitError.read())\n> + die(\"fast-import failed: {}\".format(self.gitError.read().decode()))\n> self.gitOutput.close()\n> self.gitError.close()\n> self.gitStream = None\n"},{"id":"443716","messageId":"01db01d7ed51$8ba5c5f0$a2f151d0$@nexbridge.com","threadId":"57062","inReplyTo":"xmqqh7bho0to.fsf@gitster.g","subject":"RE: [PATCH 3/6] git-p4: Removed support for Python 2","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-12-09T23:07:26Z","receivedAt":"2021-12-09T23:07:38Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 9, 2021 5:45 PM, Junio C Hamano wrote:\n> Joel Holdsworth writes:\n> \n> > Subject: Re: [PATCH 3/6] git-p4: Removed support for Python 2\n> \n> \"Removed\" -> \"remove\".\n> \n> Losing unused/no longer usable code is good.\n> \n> > Signed-off-by: Joel Holdsworth \n> > ---\n> > git-p4.py | 89\n> > +++++++++++++++++--------------------------------------\n> > 1 file changed, 28 insertions(+), 61 deletions(-)\n> \n> In these 28 new/replacement lines, there is nothing that deserves any\n> mention in the proposed log message?\n\nJust a reminder that Python 2 is the only option available on some older\n(still supported) platforms for the next year or so.\n\nSincerely,\nRandall\n\n"},{"id":"443720","messageId":"211210.86r1ale0o0.gmgdl@evledraar.gmail.com","threadId":"57062","inReplyTo":"20211209201029.136886-1-jholdsworth@nvidia.com","subject":"Re: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-10T00:48:23Z","receivedAt":"2021-12-10T00:58:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 09 2021, Joel Holdsworth wrote:\n\n> Python 2 was discontinued in 2020, and there is no longer any officially\n> supported interpreter. Further development of git-p4.py will require\n> would-be developers to test their changes with all supported dialects of\n> the language. However, if there is no longer any supported runtime\n> environment available, this places an unreasonable burden on the Git\n> project to maintain support for an obselete dialect of the language.\n\nDoes it? I can still install Python 2.7 on Debian, presumably other OS's\nhave similar ways to easily test it.\n\nI'm not that familiar with our python integration and have never used\ngit-py, but I found this series hard to read through.\n\nYou've got [12]/6 which don't make it clear whether they're needed for\npython3, or are some mixture of requirenments and a matter of taste (or\na newer API?). E.g. isn't the formatting you're changing in 2/6\nsupported in Python3?\n\nThen for 1/6 \"pass cmd arguments to subprocess as a python lists\" if\nit's not just a matter of taste can we lead with a narrow change to the\nnew API (presumably we can pass to our own function as a string, split\non whitespace, and then pass to whatever python API executes it as a\nlist first.\n\nSome of these changes also just seem to be entirely unrelated\nrefactorings, e.g. 6/6 where you're changing a multi-line commented\nregexp into something that's a dense one-liner. Does Python 3 not\nsupport the equivalent of Perl's /x, or is something else going on here?\n\nYou then change the requirenment not to python 3.0, but 3.7, which\nAFAICT was released a couple of years ago. We tend to try to capture\nsome of the oldest LTS OS's in common use, e.g. the last 2-3 RHEL\nreleases.\n\nWe still \"support\" Perl 5.8, which was released in 2002 (although that\ncould probably do with a version bump, but not to a release to 2018).\n\nI'm not at all opposed to this Python version bump, I truly don't know\nenough to know if it's a good change. Maybe we can/should also be more\naggressive with a version dependency with git-p4 than with something\nmore central to git like perl or curl.\n\nThe commit messages could just really use some extra hand-holding and\nexplanation, and a clear split-out of things related to the version bump\nv.s. things not needed for that, or unrelated refactorings.\n"},{"id":"443727","messageId":"CAJDDKr5N-2WUaspsyciq3WW+pMW--Mjiu5+3F_gOmHYENAkCUA@mail.gmail.com","threadId":"57062","inReplyTo":"20211209201029.136886-4-jholdsworth@nvidia.com","subject":"Re: [PATCH 3/6] git-p4: Removed support for Python 2","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-12-10T03:25:55Z","receivedAt":"2021-12-10T03:26:35Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Dec 9, 2021 at 1:13 PM Joel Holdsworth wrote:\n>\n> Signed-off-by: Joel Holdsworth \n> ---\n> git-p4.py | 89 +++++++++++++++++--------------------------------------\n> 1 file changed, 28 insertions(+), 61 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 32f30e5f9a..b5d4fc1176 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1,4 +1,4 @@\n> -#!/usr/bin/env python\n> +#!/usr/bin/env python3\n> #\n> # git-p4.py -- A tool for bidirectional operation between a Perforce depot and git.\n> #\n> @@ -16,8 +16,8 @@\n> # pylint: disable=too-many-branches,too-many-nested-blocks\n> #\n> import sys\n> -if sys.version_info.major < 3 and sys.version_info.minor < 7:\n> - sys.stderr.write(\"git-p4: requires Python 2.7 or later.\\n\")\n> +if sys.version_info.major < 3 or (sys.version_info.major == 3 and sys.version_info.minor < 7):\n> + sys.stderr.write(\"git-p4: requires Python 3.7 or later.\\n\")\n> sys.exit(1)\n> import os\n> import optparse\n\n\nThere are pretty large user bases on centos/rhel 7+8 where python3.6\nis the default version.\n\nIf we don't necessarily require any features from 3.7 then it might be\nworth lowering this constraint to allow 3.6 or maybe even 3.4 to\nmaximize the number of users that would benefit.\n\nI realize these python versions are retired according to the python\ncore team, but I tend to be a little more sympathetic to users when\nit doesn't have any impact on the code.\n\n--\nDavid\n"},{"id":"443749","messageId":"CAE5ih7872E8X-qRfBrBOHmKcUCX46GkXwq2WD3UUX8TuYczZDw@mail.gmail.com","threadId":"57062","inReplyTo":"20211209201029.136886-1-jholdsworth@nvidia.com","subject":"Re: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2021-12-10T07:53:30Z","receivedAt":"2021-12-10T07:53:46Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Thu, 9 Dec 2021 at 20:10, Joel Holdsworth wrote:\n>\n> The git-p4.py script currently implements code-paths for both Python 2 and\n> 3.\n>\n> Python 2 was discontinued in 2020, and there is no longer any officially\n> supported interpreter. Further development of git-p4.py will require\n> would-be developers to test their changes with all supported dialects of\n> the language. However, if there is no longer any supported runtime\n> environment available, this places an unreasonable burden on the Git\n> project to maintain support for an obselete dialect of the language.\n>\n> This patch-set removes all Python 2-specific code-paths, and then\n> applies some simplifications to the code which are available given\n> Python 3's improve delineation between bytes and strings.\n\nI might as well take this opportunity to say that I've stopped needing\nto worry about git-p4!\n\nHurrah!\n\nI'm finding that the unit tests no longer pass with this change. I'm\nnot exactly sure why.\n\n\nLuke\n\n\n\n\n\n>\n> Joel Holdsworth (6):\n> git-p4: Always pass cmd arguments to subprocess as a python lists\n> git-p4: Don't print shell commands as python lists\n> git-p4: Removed support for Python 2\n> git-p4: Decode byte strings before printing\n> git-p4: Eliminate decode_stream and encode_stream\n> git-p4: Resolve RCS keywords in binary\n>\n> git-p4.py | 319 +++++++++++++++++++++---------------------------------\n> 1 file changed, 123 insertions(+), 196 deletions(-)\n>\n> --\n> 2.33.0\n>\n"},{"id":"443750","messageId":"CAE5ih7_gvbOwvoO4deqDm_8Nk9XWzrdHGHEsgdiEb7+7YxtGwg@mail.gmail.com","threadId":"57062","inReplyTo":"20211209201029.136886-7-jholdsworth@nvidia.com","subject":"Re: [PATCH 6/6] git-p4: Resolve RCS keywords in binary","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2021-12-10T07:57:58Z","receivedAt":"2021-12-10T07:58:12Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Thu, 9 Dec 2021 at 20:11, Joel Holdsworth wrote:\n>\n> Signed-off-by: Joel Holdsworth \n> ---\n> git-p4.py | 31 ++++++++++---------------------\n> 1 file changed, 10 insertions(+), 21 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index c362a5fa38..87e6685eb6 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -46,6 +46,9 @@\n>\n> p4_access_checked = False\n>\n> +re_ko_keywords = re.compile(rb'\\$(Id|Header)(:[^$\\n]+)?\\$')\n> +re_k_keywords = re.compile(rb'\\$(Id|Header|Author|Date|DateTime|Change|File|Revision)(:[^$\\n]+)?\\$')\n\nI'm not sure what's going on here, but it does not look like just\nturning off support for python2.x.\n\n\n> +\n> def p4_build_cmd(cmd):\n> \"\"\"Build a suitable p4 command line.\n>\n> @@ -532,20 +535,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> @@ -2035,11 +2030,10 @@ def applyCommit(self, id):\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> + regexp = p4_keywords_regexp_for_file(file)\n>\n> - if pattern:\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> @@ -2968,14 +2962,9 @@ def streamOneP4File(self, file, contents):\n> print(\"\\nIgnoring apple filetype file %s\" % file['depotFile'])\n> return\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\nThis comment appears to have been stripped out, does that mean that we\nnow *do* try to demangle keywords on utf16?\n\n> - pattern = p4_keywords_regexp_for_type(type_base, type_mods)\n> - if pattern:\n> - regexp = re.compile(pattern, re.VERBOSE)\n> - text = ''.join(c.decode() for c in contents)\n> - text = regexp.sub(r'$\\1$', text)\n> - contents = [text.encode()]\n> + regexp = p4_keywords_regexp_for_type(type_base, type_mods)\n> + if regexp:\n> + contents = [regexp.sub(rb'$\\1$', c) for c in contents]\n>\n> if self.largeFileSystem:\n> (git_mode, contents) = self.largeFileSystem.processContent(git_mode, relPath, contents)\n> --\n> 2.33.0\n>\n"},{"id":"443753","messageId":"20211210084021.k4pzkckrmocoqfgg@fs","threadId":"57062","inReplyTo":"xmqqczm5o0pa.fsf@gitster.g","subject":"Re: [PATCH 4/6] git-p4: Decode byte strings before printing","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-10T08:40:21Z","receivedAt":"2021-12-10T08:40:27Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 09.12.2021 14:47, Junio C Hamano wrote:\n>Joel Holdsworth writes:\n>\n>> Signed-off-by: Joel Holdsworth \n>> ---\n>> git-p4.py | 8 +++++---\n>> 1 file changed, 5 insertions(+), 3 deletions(-)\n>\n>Is the use of strings with {} placeholders and their .format() method\n>integral part of \"decoding byte strings before printing\", or it is just\n>a new/better/improved/subjectively-preferred/whatever style?\n>\n\nIf the new minimum python version will be 3.6 or above I'd vote for using \nf-Strings instead of .format() which I think are more readable and are also \nsupposed to be faster.\n\nSo:\nsys.stdout.write(f'\\r{file_path} --> {rel_path} ({size/1024/1024} MB)\\n')\n\ninstead of one of these:\nsys.stdout.write('\\r%s --> %s (%i MB)\\n' % (file_path, relPath, size/1024/1024))\nsys.stdout.write('\\r{} --> {} ({} MB)\\n'.format(file_path.decode(), relPath, \nsize/1024/1024))\n\n>> diff --git a/git-p4.py b/git-p4.py\n>> index b5d4fc1176..b5945a0306 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -2917,7 +2917,8 @@ def streamOneP4File(self, file, contents):\n>> size = int(self.stream_file['fileSize'])\n>> else:\n>> size = 0 # deleted files don't get a fileSize apparently\n>> - sys.stdout.write('\\r%s --> %s (%i MB)\\n' % (file_path, relPath, size/1024/1024))\n>> + sys.stdout.write('\\r{} --> {} ({} MB)\\n'.format(\n>> + file_path.decode(), relPath, size/1024/1024))\n>> sys.stdout.flush()\n>>\n>> (type_base, type_mods) = split_p4_type(file[\"type\"])\n>> @@ -3061,7 +3062,8 @@ def streamP4FilesCb(self, marshalled):\n>> size = int(self.stream_file[\"fileSize\"])\n>> if size > 0:\n>> progress = 100*self.stream_file['streamContentSize']/size\n>> - sys.stdout.write('\\r%s %d%% (%i MB)' % (self.stream_file['depotFile'], progress, int(size/1024/1024)))\n>> + sys.stdout.write('\\r{} {}% ({} MB)'.format(\n>> + self.stream_file['depotFile'].decode(), progress, int(size/1024/1024)))\n>> sys.stdout.flush()\n>>\n>> self.stream_have_file_info = True\n>> @@ -3803,7 +3805,7 @@ def closeStreams(self):\n>> return\n>> self.gitStream.close()\n>> if self.importProcess.wait() != 0:\n>> - die(\"fast-import failed: %s\" % self.gitError.read())\n>> + die(\"fast-import failed: {}\".format(self.gitError.read().decode()))\n>> self.gitOutput.close()\n>> self.gitError.close()\n>> self.gitStream = None\n"},{"id":"443784","messageId":"BN8PR12MB3361388476E57E097DEBF9F7C8719@BN8PR12MB3361.namprd12.prod.outlook.com","threadId":"57062","inReplyTo":"211210.86r1ale0o0.gmgdl@evledraar.gmail.com","subject":"RE: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-10T10:37:56Z","receivedAt":"2021-12-10T10:38:00Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"> The commit messages could just really use some extra hand-holding and\n> explanation, and a clear split-out of things related to the version bump v.s.\n> things not needed for that, or unrelated refactorings.\n\nYes, I am getting this message loud and clear. I will resubmit with more detailed commit messages today.\n\nTo explain the story here: I started using git-p4 as part of my work-flow, and I expect to need it for several years to come. As I began to use it, I found that a series of bugs - mostly related to character encoding. In fixing these, I found that some of the troubles were specific to Python 3 - or rather Python 2's less strict approach to distinguishing between byte sequences and textual strings allowed the script to proceed Python 2 even though what it was doing was in fact invalid, and was potentially corrupting data.\n\nA common problem that users are encountering is that the script attempts to decode incoming textual byte-streams into UTF-8 strings. On Python 3 this fails with an exception if the data contains invalid UTF-8 codes. For text files created in Windows, CP1252 Smart Quote characters: 0x93 and 0x94 are seen fairly frequently. These codes are invalid in UTF-8, so if the script encounters any file or file name containing them, it will fail with an exception.\n\nTzadik Vanderhoof submitted a patch attempting to fix some of these issues in April 2021:\nhttps://lore.kernel.org/git/20210429073905.837-1-tzadik.vanderhoof@gmail.com/\n\nMy two comments about this patch are that 1. It doesn't fix my issue, and 2. Even with the proposed fallbackEncoding option it still leaves git-p4 broken by default.\n\nA fallbackEncoding option may still be necessary, but I found that most of the issues I encountered could be side-stepped by simply avoiding decoding incoming data into UTF-8 in the first place.\n\nKeeping a clean separation between encoded and decoded text is much easier to do in Python 3. If Python 2 support must be maintained, this will require careful testing of separate code-paths both platforms which I don't regard as reasonable given that Python 2 is thoroughly deprecated. Therefore, this first patch-set focusses primarily on removing Python 2 support.\n\nFurthermore, because I expect to be using git-p4 in my daily work-flow for some time to come, I am interested in investing some effort into improving it. There are bugs, unreliable behaviour, user-hostile behaviour, as well as code that would benefit from clean-up and modernisation. In submitting these patches, I am trying to get a read on to what extent such efforts would be accepted by the Git maintainers. \n\nIs it preferable that patch-sets have a tight focus on a single topic? I am already dividing up my full patch collection. I can divide it further if requested. I am happy to do this, I was just worried that it just might make longer to get all my patches through review.\n\n\n> Some of these changes also just seem to be entirely unrelated refactorings,\n> e.g. 6/6 where you're changing a multi-line commented regexp into\n> something that's a dense one-liner. Does Python 3 not support the\n> equivalent of Perl's /x, or is something else going on here?\n\nI will improve the commit message to explain the changes being made here.\n\nThe regexp is matching RCS keywords: https://www.perforce.com/manuals/p4guide/Content/P4Guide/filetypes.rcs.html - $File$, $Author$, $Author$ etc., a very simple match. We could keep it multi-line, though this seems overkill to me.\n\nThe main significance of this change that previously git-p4 would compile one of these two regexes for every single file processed. This patch just pre-compiles the two regexes (now binary regexes, not utf-8 regexes) at the start of the script.\n \n"},{"id":"443785","messageId":"BN8PR12MB3361A80F11E68CB4808D7423C8719@BN8PR12MB3361.namprd12.prod.outlook.com","threadId":"57062","inReplyTo":"xmqqczm5o0pa.fsf@gitster.g","subject":"RE: [PATCH 4/6] git-p4: Decode byte strings before printing","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-10T10:41:49Z","receivedAt":"2021-12-10T10:41:52Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"> Is the use of strings with {} placeholders and their .format() method integral\n> part of \"decoding byte strings before printing\", or it is just a\n> new/better/improved/subjectively-preferred/whatever style?\n> \n> If the latter, such a change should be separated into its own step, or at least\n> needs to be mentioned and justified in the proposed log message.\n\nAs I mentioned in my other message, I would like to invest some time into tidying and modernising the script - as well as fixing bugs and improving behaviour. If I submit patches that only make subjective style improvements, are these likely to be accepted?\n\n> Lack of explanation on \"why\" is shared among all these patches, it seems, so I\n> won't repeat, but the patches need to explain why to their readers.\n\nFair enough. I will resubmit.\n\n"},{"id":"443786","messageId":"BN8PR12MB33615F9CF2F6838507A06C46C8719@BN8PR12MB3361.namprd12.prod.outlook.com","threadId":"57062","inReplyTo":"CAJDDKr5N-2WUaspsyciq3WW+pMW--Mjiu5+3F_gOmHYENAkCUA@mail.gmail.com","subject":"RE: [PATCH 3/6] git-p4: Removed support for Python 2","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-10T10:44:31Z","receivedAt":"2021-12-10T10:44:35Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"> There are pretty large user bases on centos/rhel 7+8 where python3.6 is the\n> default version.\n> \n> If we don't necessarily require any features from 3.7 then it might be worth\n> lowering this constraint to allow 3.6 or maybe even 3.4 to maximize the\n> number of users that would benefit.\n> \n> I realize these python versions are retired according to the python core\n> team, but I tend to be a little more sympathetic to users when it doesn't\n> have any impact on the code.\n\nI don't mind 3.6 - I don't need any features from 3.7 or newer.\n\nI draw the line at Python 2, though.\n"},{"id":"443787","messageId":"BN8PR12MB3361E7641EE4796C80220CF9C8719@BN8PR12MB3361.namprd12.prod.outlook.com","threadId":"57062","inReplyTo":"20211210084021.k4pzkckrmocoqfgg@fs","subject":"RE: [PATCH 4/6] git-p4: Decode byte strings before printing","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-10T10:48:46Z","receivedAt":"2021-12-10T10:48:51Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"> If the new minimum python version will be 3.6 or above I'd vote for using f-\n> Strings instead of .format() which I think are more readable and are also\n> supposed to be faster.\n\nTime passes so fast - I would prefer to use f-strings, but I didn't realise that they were universally available yet. They're still a \"new thing\" as far as I'm concerned.\n\nI would prefer f-strings, I just used the str.format() method as a middle-ground.\n\n> So:\n> sys.stdout.write(f'\\r{file_path} --> {rel_path} ({size/1024/1024} MB)\\n')\n\nBy the way, I have a patch coming soon that can print the size in human readable units: b, kb, Mb, Gb etc. rather than always converting it to Mb.\n\n"},{"id":"443788","messageId":"BN8PR12MB33611BD4A606919F343F0639C8719@BN8PR12MB3361.namprd12.prod.outlook.com","threadId":"57062","inReplyTo":"CAE5ih7_gvbOwvoO4deqDm_8Nk9XWzrdHGHEsgdiEb7+7YxtGwg@mail.gmail.com","subject":"RE: [PATCH 6/6] git-p4: Resolve RCS keywords in binary","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-10T10:51:53Z","receivedAt":"2021-12-10T10:51:57Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"> This comment appears to have been stripped out, does that mean that we\n> now *do* try to demangle keywords on utf16?\n\nGood point. I guess the comment should stay.\n\nThough really... I'm not sure what we should do with utf16 files. Currently the RCS keywords just won't be resolved, with no warning for the user! I guess we could resolve them, if we had a reliable way of detecting UTF-16?\n"},{"id":"443789","messageId":"BN8PR12MB33613E4CCDF13E6D0DE155BEC8719@BN8PR12MB3361.namprd12.prod.outlook.com","threadId":"57062","inReplyTo":"CAE5ih7872E8X-qRfBrBOHmKcUCX46GkXwq2WD3UUX8TuYczZDw@mail.gmail.com","subject":"RE: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-10T10:54:06Z","receivedAt":"2021-12-10T10:54:08Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"> I might as well take this opportunity to say that I've stopped needing to\n> worry about git-p4!\n> \n> Hurrah!\n\nLucky you. It looks like I'm going to be working with Perforce a lot in the coming years, but if I can get the bridge to git working really nicely, then I am hoping to have a happy workflow even so.\n\n> I'm finding that the unit tests no longer pass with this change. I'm not exactly\n> sure why.\n\nWhat unit tests are these? I am happy to test with them.\n"},{"id":"443791","messageId":"211210.86h7bgd6wj.gmgdl@evledraar.gmail.com","threadId":"57062","inReplyTo":"BN8PR12MB3361388476E57E097DEBF9F7C8719@BN8PR12MB3361.namprd12.prod.outlook.com","subject":"Re: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-10T11:30:01Z","receivedAt":"2021-12-10T11:41:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 10 2021, Joel Holdsworth wrote:\n\n>> The commit messages could just really use some extra hand-holding and\n>> explanation, and a clear split-out of things related to the version bump v.s.\n>> things not needed for that, or unrelated refactorings.\n>\n> Yes, I am getting this message loud and clear. I will resubmit with more detailed commit messages today.\n\nThanks...\n\n> To explain the story here: I started using git-p4 as part of my\n> work-flow, and I expect to need it for several years to come. As I\n> began to use it, I found that a series of bugs - mostly related to\n> character encoding. In fixing these, I found that some of the troubles\n> were specific to Python 3 - or rather Python 2's less strict approach\n> to distinguishing between byte sequences and textual strings allowed\n> the script to proceed Python 2 even though what it was doing was in\n> fact invalid, and was potentially corrupting data.\n>\n> A common problem that users are encountering is that the script\n> attempts to decode incoming textual byte-streams into UTF-8\n> strings. On Python 3 this fails with an exception if the data contains\n> invalid UTF-8 codes. For text files created in Windows, CP1252 Smart\n> Quote characters: 0x93 and 0x94 are seen fairly frequently. These\n> codes are invalid in UTF-8, so if the script encounters any file or\n> file name containing them, it will fail with an exception.\n>\n> Tzadik Vanderhoof submitted a patch attempting to fix some of these issues in April 2021:\n> https://lore.kernel.org/git/20210429073905.837-1-tzadik.vanderhoof@gmail.com/\n>\n> My two comments about this patch are that 1. It doesn't fix my issue, and 2. Even with the proposed fallbackEncoding option it still leaves git-p4 broken by default.\n>\n> A fallbackEncoding option may still be necessary, but I found that most of the issues I encountered could be side-stepped by simply avoiding decoding incoming data into UTF-8 in the first place.\n>\n> Keeping a clean separation between encoded and decoded text is much\n> easier to do in Python 3. If Python 2 support must be maintained, this\n> will require careful testing of separate code-paths both platforms\n> which I don't regard as reasonable given that Python 2 is thoroughly\n> deprecated. Therefore, this first patch-set focusses primarily on\n> removing Python 2 support.\n\nThis all makes perfect sense to me (having never used git-p4). Having\nthis sort of explanation be part of the relevant commit message would be\ngreat :)\n\nI haven't worked extensively with Python myself, but I've understood\nthat its Unicode support was a big pain before v3 as you describe, which\nis just the sort of thing that would justify a version prereq bump, even\nif it's a bit painful to some users on older systems (if even that,\nmaybe everyone's upgraded already...)\n\n> Furthermore, because I expect to be using git-p4 in my daily work-flow\n> for some time to come, I am interested in investing some effort into\n> improving it. There are bugs, unreliable behaviour, user-hostile\n> behaviour, as well as code that would benefit from clean-up and\n> modernisation. In submitting these patches, I am trying to get a read\n> on to what extent such efforts would be accepted by the Git\n> maintainers.\n\nI don't think there's any reason we wouldn't accept these sorts of\nchanges.\n\nThe comment from me (and others I see) is purely on the topic of making\nthem easier to review, i.e. splitting out \"this is for a version\nupgrade\" v.s. \"this is just better Python style\" or whatever.\n\n> Is it preferable that patch-sets have a tight focus on a single topic?\n> I am already dividing up my full patch collection. I can divide it\n> further if requested. I am happy to do this, I was just worried that\n> it just might make longer to get all my patches through review.\n\nYeah this project really prefers to do it that way. A good example is\nthis recent 19-part series:\nhttps://lore.kernel.org/git/20211210095757.gu7w4n2rqulx2dvg@fs/T/#m5d9e8180551907578d56cdd6cd6244b9df6b59d5\n\nThis would probably be 1-3 patches, or even 1 patch in some other\nprojects, but especially with repetitive formatting changes I think it's\nfair to say that we prefer for them to be split up closer to that,\ni.e. one commit with some %s -> {} formatting change explaining why\n(probably just style, preferenc) etc.\n\nThere's also the option of splitting things into different patch\nseries. I'd say if you e.g. have one series of \"we're dropping python 2\nsupport\" and another \"here's nice formatting changes\" it would be nice\nto split those into two different patch serieses.\n\nBut that's always a matter of taste & how easy it is. If they\nextensively textually conflict it's often worth it to just stack them\ntogether, or if they changes are all small/easy enough to review some\n\"while we're at it...\" changes are generally fine.\n\nFinally, for a re-submission it's also nice to find people who've worked\non the relevant code (with some fuzzing for \"is this person likely to\nstill be active in the project?\") and CC them on the series, or in this\ncase people who've made recent changes to git-p4.py.\n\n>> Some of these changes also just seem to be entirely unrelated refactorings,\n>> e.g. 6/6 where you're changing a multi-line commented regexp into\n>> something that's a dense one-liner. Does Python 3 not support the\n>> equivalent of Perl's /x, or is something else going on here?\n>\n> I will improve the commit message to explain the changes being made here.\n>\n> The regexp is matching RCS keywords:\n> https://www.perforce.com/manuals/p4guide/Content/P4Guide/filetypes.rcs.html\n> - $File$, $Author$, $Author$ etc., a very simple match. We could keep\n> it multi-line, though this seems overkill to me.\n\nSure, my preference in Perl would be to split it, but I'm never going to\nbe maintaining git-p4.py, so... :)\n\nI.e. I think it's perfectly fair to roll it into some general \"this\nimproves readability\" changes, just as long as they're clearly labeled\nas such.\n\n> The main significance of this change that previously git-p4 would\n> compile one of these two regexes for every single file processed. This\n> patch just pre-compiles the two regexes (now binary regexes, not utf-8\n> regexes) at the start of the script.\n\nMakes sens, and another thing that would be perfect for pretty much\ncopy/pasting as-is to a re-rolled commit's message :)\n"},{"id":"443835","messageId":"xmqqsfv0m9f5.fsf@gitster.g","threadId":"57062","inReplyTo":"211210.86r1ale0o0.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-10T21:34:06Z","receivedAt":"2021-12-10T21:34:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Dec 09 2021, Joel Holdsworth wrote:\n>\n>> Python 2 was discontinued in 2020, and there is no longer any officially\n>> supported interpreter. Further development of git-p4.py will require\n>> would-be developers to test their changes with all supported dialects of\n>> the language. However, if there is no longer any supported runtime\n>> environment available, this places an unreasonable burden on the Git\n>> project to maintain support for an obselete dialect of the language.\n>\n> Does it? I can still install Python 2.7 on Debian, presumably other OS's\n> have similar ways to easily test it.\n\nYes, that is a good point to make against \"we cannot maintain the\nhalf meant to cater to Python2 of the script\". Developers should be\nable to keep and test Python2 support, if it is necessary.\n\nSo the more important question is if there are end-users that have\nno choice but sticking to Python2. Is there distributions and\nsystems that do not offer Python3, on which end-users have happily\nbeen using Python2? If some users with vendor supported Python2 do\nnot have access to Python3, cutting them off may be premature.\n\nAs the general direction, I do not mind deprecating support for\nPython2, and then eventually removing it. I just do not know if 2\nyears is long enough for the latter (IIRC, the sunset happened at\nthe beginning of 2020, and we are about to end 2021).\n\nThanks.\n"},{"id":"443838","messageId":"021001d7ee10$517309a0$f4591ce0$@nexbridge.com","threadId":"57062","inReplyTo":"xmqqsfv0m9f5.fsf@gitster.g","subject":"RE: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-12-10T21:53:02Z","receivedAt":"2021-12-10T21:53:16Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 10, 2021 4:34 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > On Thu, Dec 09 2021, Joel Holdsworth wrote:\n> >\n> >> Python 2 was discontinued in 2020, and there is no longer any\n> >> officially supported interpreter. Further development of git-p4.py\n> >> will require would-be developers to test their changes with all\n> >> supported dialects of the language. However, if there is no longer\n> >> any supported runtime environment available, this places an\n> >> unreasonable burden on the Git project to maintain support for an\n> obselete dialect of the language.\n> >\n> > Does it? I can still install Python 2.7 on Debian, presumably other\n> > OS's have similar ways to easily test it.\n> \n> Yes, that is a good point to make against \"we cannot maintain the half meant\n> to cater to Python2 of the script\". Developers should be able to keep and\n> test Python2 support, if it is necessary.\n> \n> So the more important question is if there are end-users that have no choice\n> but sticking to Python2. Is there distributions and systems that do not offer\n> Python3, on which end-users have happily been using Python2? If some\n> users with vendor supported Python2 do not have access to Python3, cutting\n> them off may be premature.\n> \n> As the general direction, I do not mind deprecating support for Python2, and\n> then eventually removing it. I just do not know if 2 years is long enough for\n> the latter (IIRC, the sunset happened at the beginning of 2020, and we are\n> about to end 2021).\n\nThe HPE NonStop Itanium platform only provides Python 2.7. That is the last version that will be available on that platform until it goes off support some time in the next few years (there are known very large US companies who are git users on that platform but I cannot share their names here). The NonStop x86 platform is currently on Python 3.6.8 but I have to take action to select the python3 object - not a big deal. Since I am continually running the git test suite with each release, the python 2 code can continue to be tested. Python 2 is also available on our x86 machine for backward compatibility reasons - it may vanish at some point but that isn’t scheduled yet.\n\n-Randall\n\n"},{"id":"443880","messageId":"CAE5ih7-ZoKThXefBN=znytQi=z4_notihQdSksYdMTzKDTAb=w@mail.gmail.com","threadId":"57062","inReplyTo":"BN8PR12MB33613E4CCDF13E6D0DE155BEC8719@BN8PR12MB3361.namprd12.prod.outlook.com","subject":"Re: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2021-12-11T09:58:22Z","receivedAt":"2021-12-11T09:58:36Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Fri, 10 Dec 2021 at 10:54, Joel Holdsworth wrote:\n>\n> > I might as well take this opportunity to say that I've stopped needing to\n> > worry about git-p4!\n> >\n> > Hurrah!\n>\n> Lucky you. It looks like I'm going to be working with Perforce a lot in the coming years, but if I can get the bridge to git working really nicely, then I am hoping to have a happy workflow even so.\n\nI cannot begin to tell you how liberating it is to just have a pure\ngit workflow!\n\nBut yes, git-p4 at least stops some of the pain.\n\n>\n> > I'm finding that the unit tests no longer pass with this change. I'm not exactly\n> > sure why.\n>\n> What unit tests are these? I am happy to test with them.\n\n cd t\n make T=t98* -j$(nproc)\n"},{"id":"443893","messageId":"CABPp-BHjc1i4o-Oe2U2fV8_TRgRPfve_mYt=kveTYMy-3BdpCA@mail.gmail.com","threadId":"57062","inReplyTo":"xmqqsfv0m9f5.fsf@gitster.g","subject":"Re: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-12-11T21:00:30Z","receivedAt":"2021-12-11T21:00:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Fri, Dec 10, 2021 at 10:07 PM Junio C Hamano wrote:\n>\n> Ævar Arnfjörð Bjarmason writes:\n>\n> > On Thu, Dec 09 2021, Joel Holdsworth wrote:\n> >\n> >> Python 2 was discontinued in 2020, and there is no longer any officially\n> >> supported interpreter. Further development of git-p4.py will require\n> >> would-be developers to test their changes with all supported dialects of\n> >> the language. However, if there is no longer any supported runtime\n> >> environment available, this places an unreasonable burden on the Git\n> >> project to maintain support for an obselete dialect of the language.\n> >\n> > Does it? I can still install Python 2.7 on Debian, presumably other OS's\n> > have similar ways to easily test it.\n>\n> Yes, that is a good point to make against \"we cannot maintain the\n> half meant to cater to Python2 of the script\". Developers should be\n> able to keep and test Python2 support, if it is necessary.\n\nI also disagree with the reason Joel gave in the quoted paragraph for\ndropping Python2 support, but I think there are other good reasons to\ndrop it.\n\n> So the more important question is if there are end-users that have\n> no choice but sticking to Python2. Is there distributions and\n> systems that do not offer Python3, on which end-users have happily\n> been using Python2? If some users with vendor supported Python2 do\n> not have access to Python3, cutting them off may be premature.\n\nThese are good questions, though I think there's more to it than this,\nas I'll mention in just a minute...\n\n> As the general direction, I do not mind deprecating support for\n> Python2, and then eventually removing it. I just do not know if 2\n> years is long enough for the latter (IIRC, the sunset happened at\n> the beginning of 2020, and we are about to end 2021).\n\nPython2 was deprecated by the python project in 2008, with announced\nplans to stop all support (including security fixes) in 2015. They\npushed the sunset date back to Jan 1, 2020. So it has only been\nend-of-life for just under 2 years, but it's been deprecated for over\n13 years.\n\nIn regards to your good questions about Python3 availability on some\nplatforms: If such platforms exist, they have had over a decade's\nheads up...so let's ask a few extra questions. If these platforms\nstill haven't made python3 available, would newer versions of Git even\nbe available on these platforms? Even if newer Git versions are\navailable, would users on such platforms have any qualms with using an\nolder Git version given the platform insistence of only providing an\nold Python version lacking any support (even security fixes)?\n\n\nSome of my personal python2/python3 experience, if it's useful in\nweighing decisions:\n\n* There are python projects for which I still continue to support\nsimultaneous python2 and python3 usage, though for projects that are\nsmaller then git-p4.py (e.g. 1/2 to 1/3 the size). Such multi-version\nsupport is painful, and it causes occasional bugs that hit users that\nwouldn't arise if there was only one supported python version.\n\n* I initially wanted to also do the multi-version support for\ngit-filter-repo (which is approximately the same size as git-p4.py,\nand obviously also interfaces with git somewhat deeply). I gave up on\nit, and didn't consider it justified, especially with the\nthen-soon-impending End-Of-Life for python2. I instead just switched\nfrom python2 -> python3 (in 2019; yes, I'm a straggler.) Granted, I\ndid benefit from the fact that git-filter-repo is a\nonce-in-a-blue-moon usage tool (and only by one member on the team),\nrather than a daily usage tool, but I may have come to the same\ndecision anyway even back then.\n\n* (Slight tangent) I tried to use unicode strings everywhere in\ngit-filter-repo a few times, but invariably found it to be buggy and\nslow. It was a mistake, and I eventually switched over to bytestrings\neverywhere, only converting to unicode (when possible) when printing\nmessages for the user on the console. bytestrings are ugly to use\n(IMO), but they're a better data model when dealing with file\ncontents, process output, filenames, etc. I think git-p4's decision\nto attempt to use unicode strings everywhere is a mistake that'll\nprobably result in bugs based on that experience of mine; it's not an\nappropriate model of the relevant data. It'll also make things\nslower.\n\n[I actually think the unicode vs. bytestring thing might be more\nimportant for bug fixing than limiting to python3. Though I think\nboth are worthwhile.]\n"},{"id":"443897","messageId":"CAE5ih7-7eH_ezsvZ6TWjZoHg0PZ2nh7C0rKrWSzCnNe44bR2zw@mail.gmail.com","threadId":"57062","inReplyTo":"CABPp-BHjc1i4o-Oe2U2fV8_TRgRPfve_mYt=kveTYMy-3BdpCA@mail.gmail.com","subject":"Re: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2021-12-12T08:55:20Z","receivedAt":"2021-12-12T08:55:36Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Sat, 11 Dec 2021 at 21:00, Elijah Newren wrote:\n>\n> Hi,\n>\n> On Fri, Dec 10, 2021 at 10:07 PM Junio C Hamano wrote:\n> >\n> > Ævar Arnfjörð Bjarmason writes:\n> >\n> > > On Thu, Dec 09 2021, Joel Holdsworth wrote:\n> > >\n> > >> Python 2 was discontinued in 2020, and there is no longer any officially\n> > >> supported interpreter. Further development of git-p4.py will require\n> > >> would-be developers to test their changes with all supported dialects of\n> > >> the language. However, if there is no longer any supported runtime\n> > >> environment available, this places an unreasonable burden on the Git\n> > >> project to maintain support for an obselete dialect of the language.\n> > >\n> > > Does it? I can still install Python 2.7 on Debian, presumably other OS's\n> > > have similar ways to easily test it.\n> >\n> > Yes, that is a good point to make against \"we cannot maintain the\n> > half meant to cater to Python2 of the script\". Developers should be\n> > able to keep and test Python2 support, if it is necessary.\n>\n> I also disagree with the reason Joel gave in the quoted paragraph for\n> dropping Python2 support, but I think there are other good reasons to\n> drop it.\n>\n> > So the more important question is if there are end-users that have\n> > no choice but sticking to Python2. Is there distributions and\n> > systems that do not offer Python3, on which end-users have happily\n> > been using Python2? If some users with vendor supported Python2 do\n> > not have access to Python3, cutting them off may be premature.\n>\n> These are good questions, though I think there's more to it than this,\n> as I'll mention in just a minute...\n>\n> > As the general direction, I do not mind deprecating support for\n> > Python2, and then eventually removing it. I just do not know if 2\n> > years is long enough for the latter (IIRC, the sunset happened at\n> > the beginning of 2020, and we are about to end 2021).\n>\n> Python2 was deprecated by the python project in 2008, with announced\n> plans to stop all support (including security fixes) in 2015. They\n> pushed the sunset date back to Jan 1, 2020. So it has only been\n> end-of-life for just under 2 years, but it's been deprecated for over\n> 13 years.\n>\n> In regards to your good questions about Python3 availability on some\n> platforms: If such platforms exist, they have had over a decade's\n> heads up...so let's ask a few extra questions. If these platforms\n> still haven't made python3 available, would newer versions of Git even\n> be available on these platforms? Even if newer Git versions are\n> available, would users on such platforms have any qualms with using an\n> older Git version given the platform insistence of only providing an\n> old Python version lacking any support (even security fixes)?\n>\n>\n> Some of my personal python2/python3 experience, if it's useful in\n> weighing decisions:\n>\n> * There are python projects for which I still continue to support\n> simultaneous python2 and python3 usage, though for projects that are\n> smaller then git-p4.py (e.g. 1/2 to 1/3 the size). Such multi-version\n> support is painful, and it causes occasional bugs that hit users that\n> wouldn't arise if there was only one supported python version.\n>\n> * I initially wanted to also do the multi-version support for\n> git-filter-repo (which is approximately the same size as git-p4.py,\n> and obviously also interfaces with git somewhat deeply). I gave up on\n> it, and didn't consider it justified, especially with the\n> then-soon-impending End-Of-Life for python2. I instead just switched\n> from python2 -> python3 (in 2019; yes, I'm a straggler.) Granted, I\n> did benefit from the fact that git-filter-repo is a\n> once-in-a-blue-moon usage tool (and only by one member on the team),\n> rather than a daily usage tool, but I may have come to the same\n> decision anyway even back then.\n>\n> * (Slight tangent) I tried to use unicode strings everywhere in\n> git-filter-repo a few times, but invariably found it to be buggy and\n> slow. It was a mistake, and I eventually switched over to bytestrings\n> everywhere, only converting to unicode (when possible) when printing\n> messages for the user on the console. bytestrings are ugly to use\n> (IMO), but they're a better data model when dealing with file\n> contents, process output, filenames, etc. I think git-p4's decision\n> to attempt to use unicode strings everywhere is a mistake that'll\n> probably result in bugs based on that experience of mine; it's not an\n> appropriate model of the relevant data. It'll also make things\n> slower.\n\n+1\n\nWhen using python2, git-p4 just ignores all the encoding issues, and I\nthink this is actually exactly what we want. This means that in some\nways, the Python2 version is currently more correct than the Python3\none.\n\nWith Python3, as you say, it attempts to convert to/from unicode\nstrings, and probably this is the root of the problems.\n\nPerforce has a mode where it works in unicode internally. This gets\nconfigured in the server, and clients then do The Right Thing.\n\nhttps://www.perforce.com/manuals/p4sag/Content/P4SAG/superuser.unicode.clients.html\n\nHowever, many P4 shops don't bother to set this (I know we don't) so\nyou end up with BIG5 and CP1252 just being dumped raw into Perforce,\nand then coming back out again. This causes problems for Perforce\nclients just as much as for git-p4 clients.\n\nWhen git-p4 is used with python2, provided your client has the same\ncharacter encoding as the author of the code you are looking at, it\nwill appear to work. Otherwise you get character encoding errors.\n\nTo fix this with python3, we need to make a choice:\n\n- either work out the character encoding that Perforce is sending us\nwhich it doesn't know and can't tell us and then convert that to/from\nunicode. There was a patch series a while ago which tried to let you\nconfigure a default encoding.\n- or to just preserve everything that Perforce sends us and let the\nclient figure it out.\n\nThe current patch series is (I think) tending towards the first of\nthose options. But we're trying to recreate information that just\ndoesn't exist.\n\nHere's the patch that Andrew Oakley sent a while back which attempts\nto get git-p4 to just preserve what it gets:\n\nhttps://lore.kernel.org/git/20210412085251.51475-1-andrew@adoakley.name/\n\nLuke\n\n\n>\n> [I actually think the unicode vs. bytestring thing might be more\n> important for bug fixing than limiting to python3. Though I think\n> both are worthwhile.]\n\n+1\n"},{"id":"443977","messageId":"BN8PR12MB33619656D91E92C50FF1B86CC8749@BN8PR12MB3361.namprd12.prod.outlook.com","threadId":"57062","inReplyTo":"CAE5ih7-ZoKThXefBN=znytQi=z4_notihQdSksYdMTzKDTAb=w@mail.gmail.com","subject":"RE: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-13T13:47:49Z","receivedAt":"2021-12-13T13:47:56Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"> > What unit tests are these? I am happy to test with them.\n> \n> cd t\n> make T=t98* -j$(nproc)\n\nAwesome! Just ran the tests. We got a clean sweep.\n\nWith one proviso:\n\nWhen running the current upstream master git-p4 version, there are errors if /usr/bin/python is not present.\n\nlib-git-p4.sh checks for the presence of python with \"test_have_prereq PYTHON\" - but if I only have /usr/bin/python3 installed, the prerequisite check passes, but git-p4.py itself fails because the shebang points at python not python3.\n\nOn Debian installing the package \"python-is-python3\" fixes the issue.\n\nPerhaps it might help to have something like \"test_have_prereq PYTHON3\".\n\nRegardless, if python3 is installed, this patch-set passes the tests just fine.\n\nJoel\n"},{"id":"444021","messageId":"xmqqv8zsb8xc.fsf@gitster.g","threadId":"57062","inReplyTo":"BN8PR12MB33619656D91E92C50FF1B86CC8749@BN8PR12MB3361.namprd12.prod.outlook.com","subject":"Re: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-13T19:29:19Z","receivedAt":"2021-12-13T19:29:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Holdsworth writes:\n\n>> > What unit tests are these? I am happy to test with them.\n>> \n>> cd t\n>> make T=t98* -j$(nproc)\n>\n> Awesome! Just ran the tests. We got a clean sweep.\n>\n> With one proviso:\n>\n> When running the current upstream master git-p4 version, there are errors if /usr/bin/python is not present.\n>\n> lib-git-p4.sh checks for the presence of python with \"test_have_prereq PYTHON\" - but if I only have /usr/bin/python3 installed, the prerequisite check passes, but git-p4.py itself fails because the shebang points at python not python3.\n>\n> On Debian installing the package \"python-is-python3\" fixes the issue.\n\nCan I take that a distro allowing an installation without\npython-is-python3 (or just having only python2) as a sign that\n\"transtion to 3 only\" is a bit premature?\n\n> Perhaps it might help to have something like \"test_have_prereq PYTHON3\".\n\nSure, but that defeats the whole notion of \"python3 is everywhere,\npython2 is dead, and nobody should be using the 2-year dead\nversion\". \"test_have_prereq PYTHON\" should be sufficient in such a\nworld, no?\n"},{"id":"444025","messageId":"BN8PR12MB336129AD5C148267925FB387C8749@BN8PR12MB3361.namprd12.prod.outlook.com","threadId":"57062","inReplyTo":"xmqqv8zsb8xc.fsf@gitster.g","subject":"RE: [PATCH 0/6] Transition git-p4.py to support Python 3 only","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-13T19:58:23Z","receivedAt":"2021-12-13T19:58:27Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"> Sure, but that defeats the whole notion of \"python3 is everywhere,\n> python2 is dead, and nobody should be using the 2-year dead version\".\n> \"test_have_prereq PYTHON\" should be sufficient in such a world, no?\n\nThat's a bit of a stretch.\n\nAs Elijah said:\n\n> Python2 was deprecated by the python project in 2008, with announced\n> plans to stop all support (including security fixes) in 2015. They pushed the\n> sunset date back to Jan 1, 2020. So it has only been end-of-life for just under\n> 2 years, but it's been deprecated for over\n> 13 years.\n\nPython 3 *is* everywhere. During the transitionary period, Debian allowed python2 and python3 to coexist on a system by giving them the names /usr/bin/python and /usr/bin/python3 respectively, because they are effectively different languages. This allowed legacy code to continue to function in view that it would eventually get ported over to python 3, opting into it by changing the shebang to point at /usr/bin/python3\n\n\"test_have_prereq PYTHON\" lumps all python versions together as if they were one thing, which they are not. It's as meaningful as lumping together all the Perl versions, or lumping C++98 together with modern C++. If a system has Python 1 installed, strictly speaking the configuration script should indicate that Python is present! - but there's a bit more\n\nI am quite sure the Python 2 will linger on in some form or other - maybe forever, but that doesn't mean the Git project should be developing, maintaining, testing or releasing Python 2 code in 2021.\n\nPython 3 is so well established, that even the minimum version requirement I want to bump to: Python 3.6, is end-of-life.\n\nJoel\n"}]}