{"thread":{"id":"57107","subject":"[PATCH v2 0/2] git-p4: remove \"debug\" and \"rollback\" verbs","startedAt":"2021-12-17T14:06:24Z","lastAt":"2021-12-21T10:32:39Z","messageCount":7,"participants":["Joel Holdsworth","Junio C Hamano","Johannes Schindelin","Luke Diamand"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"444361","messageId":"20211217140547.2115095-1-jholdsworth@nvidia.com","threadId":"57107","inReplyTo":null,"subject":"[PATCH v2 0/2] git-p4: remove \"debug\" and \"rollback\" verbs","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-17T14:05:45Z","receivedAt":"2021-12-17T14:06:24Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"git-p4 contains a selection of verbs for various functions of the\nscript. The \"debug\" and \"rollback\" verbs appear to have been added early\nin the development life of git-p4. They were once used as debugging\ntools, but no longer serve any useful purpose either for developers or\nusers. Removing these verbs simplifies the script by removing dead code,\nand increases usability by reducing complaexity.\n\nJoel Holdsworth (2):\n git-p4: remove \"delete\" verb\n git-p4: remove \"rollback\" verb\n\n git-p4.py | 76 -------------------------------------------------------\n 1 file changed, 76 deletions(-)\n\n-- \n2.34.1\n\n"},{"id":"444362","messageId":"20211217140547.2115095-2-jholdsworth@nvidia.com","threadId":"57107","inReplyTo":"20211217140547.2115095-1-jholdsworth@nvidia.com","subject":"[PATCH v2 1/2] git-p4: remove \"delete\" verb","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-17T14:05:46Z","receivedAt":"2021-12-17T14:06:25Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"The git-p4 \"delete\" verb is described as \"A tool to debug the output of\np4 -G\". However, the implementation provided is of no useful benefit to\neither users or developers.\n\nSigned-off-by: Joel Holdsworth \n---\nAdds Signed-off-by tag\n\n git-p4.py | 16 ----------------\n 1 file changed, 16 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 2b4500226a..b7ed8e41ff 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1532,21 +1532,6 @@ def loadUserMapFromCache(self):\n except IOError:\n self.getUserMapFromPerforceServer()\n \n-class P4Debug(Command):\n- def __init__(self):\n- Command.__init__(self)\n- self.options = []\n- self.description = \"A tool to debug the output of p4 -G.\"\n- self.needsGit = False\n-\n- def run(self, args):\n- j = 0\n- for output in p4CmdList(args):\n- print('Element: %d' % j)\n- j += 1\n- print(output)\n- return True\n-\n class P4RollBack(Command):\n def __init__(self):\n Command.__init__(self)\n@@ -4363,7 +4348,6 @@ def printUsage(commands):\n print(\"\")\n \n commands = {\n- \"debug\" : P4Debug,\n \"submit\" : P4Submit,\n \"commit\" : P4Submit,\n \"sync\" : P4Sync,\n-- \n2.34.1\n\n"},{"id":"444363","messageId":"20211217140547.2115095-3-jholdsworth@nvidia.com","threadId":"57107","inReplyTo":"20211217140547.2115095-1-jholdsworth@nvidia.com","subject":"[PATCH v2 2/2] git-p4: remove \"rollback\" verb","fromName":"Joel Holdsworth","fromEmail":"jholdsworth@nvidia.com","sentAt":"2021-12-17T14:05:47Z","receivedAt":"2021-12-17T14:06:26Z","isPatch":true,"sender":{"key":"jholdsworth@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/1449493?v=4"},"body":"The git-p4 \"rollback\" verb is described as \"A tool to debug the\nmulti-branch import. Don't use :)\". The implementation provided is of no\nuseful benefit to either users or developers.\n\nSigned-off-by: Joel Holdsworth \n---\nAdds Signed-off-by tag\n\n git-p4.py | 60 -------------------------------------------------------\n 1 file changed, 60 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex b7ed8e41ff..a7cb321f75 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1532,65 +1532,6 @@ def loadUserMapFromCache(self):\n except IOError:\n self.getUserMapFromPerforceServer()\n \n-class P4RollBack(Command):\n- def __init__(self):\n- Command.__init__(self)\n- self.options = [\n- optparse.make_option(\"--local\", dest=\"rollbackLocalBranches\", action=\"store_true\")\n- ]\n- self.description = \"A tool to debug the multi-branch import. Don't use :)\"\n- self.rollbackLocalBranches = False\n-\n- def run(self, args):\n- if len(args) != 1:\n- return False\n- maxChange = int(args[0])\n-\n- if \"p4ExitCode\" in p4Cmd(\"changes -m 1\"):\n- die(\"Problems executing p4\");\n-\n- if self.rollbackLocalBranches:\n- refPrefix = \"refs/heads/\"\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-\n- for line in lines:\n- if self.rollbackLocalBranches or (line.startswith(\"p4/\") and line != \"p4/HEAD\\n\"):\n- line = line.strip()\n- ref = refPrefix + line\n- log = extractLogMessageFromGitCommit(ref)\n- settings = extractSettingsGitLog(log)\n-\n- depotPaths = settings['depot-paths']\n- change = settings['change']\n-\n- changed = False\n-\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- 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- log = extractLogMessageFromGitCommit(ref)\n- settings = extractSettingsGitLog(log)\n-\n-\n- depotPaths = settings['depot-paths']\n- change = settings['change']\n-\n- if changed:\n- print(\"%s rewound to %s\" % (ref, change))\n-\n- return True\n-\n class P4Submit(Command, P4UserMap):\n \n conflict_behavior_choices = (\"ask\", \"skip\", \"quit\")\n@@ -4353,7 +4294,6 @@ def printUsage(commands):\n \"sync\" : P4Sync,\n \"rebase\" : P4Rebase,\n \"clone\" : P4Clone,\n- \"rollback\" : P4RollBack,\n \"branches\" : P4Branches,\n \"unshelve\" : P4Unshelve,\n }\n-- \n2.34.1\n\n"},{"id":"444525","messageId":"xmqqlf0fvzrf.fsf@gitster.g","threadId":"57107","inReplyTo":"20211217140547.2115095-3-jholdsworth@nvidia.com","subject":"Re: [PATCH v2 2/2] git-p4: remove \"rollback\" verb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-20T19:32:04Z","receivedAt":"2021-12-20T19:32:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Holdsworth writes:\n\n> The git-p4 \"rollback\" verb is described as \"A tool to debug the\n> multi-branch import. Don't use :)\".\n\n;-).\n\n> The implementation provided is of no\n> useful benefit to either users or developers.\n\nIt is hard to make such a broad \"no useful benefit\" claim without\ngiving a bit more analysis [*]. With only that single sentence, it\nis far worse justification than saying that \"The author tells us not\nto use it, and it is not even documented\" to convince others why\nremoval is a good idea.\n\nI'd suggest dropping that sentence (replace it with something we can\nobjectively verify, like \"it is not even documented\", if you want to).\n\nThanks.\n\n[Footnote]\n\n * what the code tried to do, how it didn't do it well that it was\n clear that it couldn't possibly be useful, etc. etc.\n\n\n> Signed-off-by: Joel Holdsworth \n> ---\n> Adds Signed-off-by tag\n>\n> git-p4.py | 60 -------------------------------------------------------\n> 1 file changed, 60 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index b7ed8e41ff..a7cb321f75 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1532,65 +1532,6 @@ def loadUserMapFromCache(self):\n> except IOError:\n> self.getUserMapFromPerforceServer()\n> \n> -class P4RollBack(Command):\n> - def __init__(self):\n> - Command.__init__(self)\n> - self.options = [\n> - optparse.make_option(\"--local\", dest=\"rollbackLocalBranches\", action=\"store_true\")\n> - ]\n> - self.description = \"A tool to debug the multi-branch import. Don't use :)\"\n> - self.rollbackLocalBranches = False\n> -\n> - def run(self, args):\n> - if len(args) != 1:\n> - return False\n> - maxChange = int(args[0])\n> -\n> - if \"p4ExitCode\" in p4Cmd(\"changes -m 1\"):\n> - die(\"Problems executing p4\");\n> -\n> - if self.rollbackLocalBranches:\n> - refPrefix = \"refs/heads/\"\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> -\n> - for line in lines:\n> - if self.rollbackLocalBranches or (line.startswith(\"p4/\") and line != \"p4/HEAD\\n\"):\n> - line = line.strip()\n> - ref = refPrefix + line\n> - log = extractLogMessageFromGitCommit(ref)\n> - settings = extractSettingsGitLog(log)\n> -\n> - depotPaths = settings['depot-paths']\n> - change = settings['change']\n> -\n> - changed = False\n> -\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> - 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> - log = extractLogMessageFromGitCommit(ref)\n> - settings = extractSettingsGitLog(log)\n> -\n> -\n> - depotPaths = settings['depot-paths']\n> - change = settings['change']\n> -\n> - if changed:\n> - print(\"%s rewound to %s\" % (ref, change))\n> -\n> - return True\n> -\n> class P4Submit(Command, P4UserMap):\n> \n> conflict_behavior_choices = (\"ask\", \"skip\", \"quit\")\n> @@ -4353,7 +4294,6 @@ def printUsage(commands):\n> \"sync\" : P4Sync,\n> \"rebase\" : P4Rebase,\n> \"clone\" : P4Clone,\n> - \"rollback\" : P4RollBack,\n> \"branches\" : P4Branches,\n> \"unshelve\" : P4Unshelve,\n> }\n"},{"id":"444527","messageId":"xmqqh7b3vzgp.fsf@gitster.g","threadId":"57107","inReplyTo":"20211217140547.2115095-2-jholdsworth@nvidia.com","subject":"Re: [PATCH v2 1/2] git-p4: remove \"delete\" verb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-20T19:38:30Z","receivedAt":"2021-12-20T19:39:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joel Holdsworth writes:\n\n> The git-p4 \"delete\" verb is described as \"A tool to debug the output of\n> p4 -G\". However, the implementation provided is of no useful benefit to\n> either users or developers.\n\nThe same comment as 2/2 applies here, too. This time, however, I\nthink it would be much easier to give a proper analysis of what it\nis designed to do and what it does, to convince readers why it is\ntoo trivial to be worth having the code (compared to the PATCH 2/2).\n\nAlso, this is not about the \"delete\" verb, but the \"debug\" verb, no?\n\n\n> Signed-off-by: Joel Holdsworth \n> ---\n> Adds Signed-off-by tag\n>\n> git-p4.py | 16 ----------------\n> 1 file changed, 16 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 2b4500226a..b7ed8e41ff 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1532,21 +1532,6 @@ def loadUserMapFromCache(self):\n> except IOError:\n> self.getUserMapFromPerforceServer()\n> \n> -class P4Debug(Command):\n> - def __init__(self):\n> - Command.__init__(self)\n> - self.options = []\n> - self.description = \"A tool to debug the output of p4 -G.\"\n> - self.needsGit = False\n> -\n> - def run(self, args):\n> - j = 0\n> - for output in p4CmdList(args):\n> - print('Element: %d' % j)\n> - j += 1\n> - print(output)\n> - return True\n> -\n> class P4RollBack(Command):\n> def __init__(self):\n> Command.__init__(self)\n> @@ -4363,7 +4348,6 @@ def printUsage(commands):\n> print(\"\")\n> \n> commands = {\n> - \"debug\" : P4Debug,\n> \"submit\" : P4Submit,\n> \"commit\" : P4Submit,\n> \"sync\" : P4Sync,\n"},{"id":"444543","messageId":"nycvar.QRO.7.76.6.2112202217570.347@tvgsbejvaqbjf.bet","threadId":"57107","inReplyTo":"20211217140547.2115095-1-jholdsworth@nvidia.com","subject":"Re: [PATCH v2 0/2] git-p4: remove \"debug\" and \"rollback\" verbs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-12-20T21:19:28Z","receivedAt":"2021-12-20T21:20:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Joel,\n\nOn Fri, 17 Dec 2021, Joel Holdsworth wrote:\n\n> git-p4 contains a selection of verbs for various functions of the\n> script. The \"debug\" and \"rollback\" verbs appear to have been added early\n> in the development life of git-p4. They were once used as debugging\n> tools, but no longer serve any useful purpose either for developers or\n> users. Removing these verbs simplifies the script by removing dead code,\n> and increases usability by reducing complaexity.\n\nThank you for Cc:ing me, but I am not really familiar with Perforce. All\nmy modifications in the git-p4 area stem from trying to let the CI/PR\nbuilds pass... So I don't feel that I am competent enough a reviewer for\nthis patch series.\n\nCiao,\nJohannes\n"},{"id":"444595","messageId":"CAE5ih78dLwqpHChkJ6JWmFYZh58Yca8tWke9Oa8JDzzOGi78AQ@mail.gmail.com","threadId":"57107","inReplyTo":"xmqqh7b3vzgp.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] git-p4: remove \"delete\" verb","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2021-12-21T10:32:25Z","receivedAt":"2021-12-21T10:32:39Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Mon, 20 Dec 2021 at 19:38, Junio C Hamano wrote:\n>\n> Joel Holdsworth writes:\n>\n> > The git-p4 \"delete\" verb is described as \"A tool to debug the output of\n> > p4 -G\". However, the implementation provided is of no useful benefit to\n> > either users or developers.\n>\n> The same comment as 2/2 applies here, too. This time, however, I\n> think it would be much easier to give a proper analysis of what it\n> is designed to do and what it does, to convince readers why it is\n> too trivial to be worth having the code (compared to the PATCH 2/2).\n>\n> Also, this is not about the \"delete\" verb, but the \"debug\" verb, no?\n\nI have never used the P4Debug subcommand for debugging git-p4, and I'm\nnot sure what I would use it for. Removing it seems sensible.\n\nAs noted by Junio, s/delete/debug/.\n\nLuke\n"}]}