{"thread":{"id":"58195","subject":"[PATCH 0/2] git-p4: fix two undefined variables","startedAt":"2022-07-20T06:35:09Z","lastAt":"2022-07-20T20:54:51Z","messageCount":11,"participants":["Moritz Baumann via GitGitGadget","Junio C Hamano","Baumann, Moritz"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"459497","messageId":"pull.1297.git.git.1658298900.gitgitgadget@gmail.com","threadId":"58195","inReplyTo":null,"subject":"[PATCH 0/2] git-p4: fix two undefined variables","fromName":"Moritz Baumann via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-20T06:34:57Z","receivedAt":"2022-07-20T06:35:09Z","isPatch":true,"sender":{"key":"name:Moritz Baumann","avatar":null},"body":"Moritz Baumann (2):\n git-p4: fix typo in P4Submit.applyCommit()\n git-p4: fix error handling in P4Unshelve.renameBranch()\n\n git-p4.py | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\n\nbase-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1297%2Fmbs-c%2Ffix-undefined-variables-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1297/mbs-c/fix-undefined-variables-v1\nPull-Request: https://github.com/git/git/pull/1297\n-- \ngitgitgadget\n"},{"id":"459498","messageId":"1484eee8961227a9901e3baa052c74a6d992c220.1658298900.git.gitgitgadget@gmail.com","threadId":"58195","inReplyTo":"pull.1297.git.git.1658298900.gitgitgadget@gmail.com","subject":"[PATCH 1/2] git-p4: fix typo in P4Submit.applyCommit()","fromName":"Moritz Baumann via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-20T06:34:58Z","receivedAt":"2022-07-20T06:35:10Z","isPatch":true,"sender":{"key":"name:Moritz Baumann","avatar":null},"body":"From: Moritz Baumann \n\nSigned-off-by: Moritz Baumann \n---\n git-p4.py | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8fbf6eb1fe3..1de3d6f1cd4 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -2226,7 +2226,7 @@ class P4Submit(Command, P4UserMap):\n raw=True):\n if regexp.search(line):\n if verbose:\n- print(\"got keyword match on %s in %s in %s\" % (regex.pattern, line, file))\n+ print(\"got keyword match on %s in %s in %s\" % (regexp.pattern, line, file))\n kwfiles[file] = regexp\n break\n \n-- \ngitgitgadget\n\n"},{"id":"459499","messageId":"69c9fd5fbec859c2cced95930ac4d427a09aee90.1658298900.git.gitgitgadget@gmail.com","threadId":"58195","inReplyTo":"pull.1297.git.git.1658298900.gitgitgadget@gmail.com","subject":"[PATCH 2/2] git-p4: fix error handling in P4Unshelve.renameBranch()","fromName":"Moritz Baumann via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-20T06:34:59Z","receivedAt":"2022-07-20T06:35:38Z","isPatch":true,"sender":{"key":"name:Moritz Baumann","avatar":null},"body":"From: Moritz Baumann \n\nThe error handling code referenced a variable that does not exist.\nAlso, the condition could never evaluate to True.\n\nSigned-off-by: Moritz Baumann \n---\n git-p4.py | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 1de3d6f1cd4..8f20d15f272 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -4369,19 +4369,16 @@ class P4Unshelve(Command):\n def renameBranch(self, branch_name):\n \"\"\"Rename the existing branch to branch_name.N .\"\"\"\n \n- found = True\n for i in range(0, 1000):\n backup_branch_name = \"{0}.{1}\".format(branch_name, i)\n if not gitBranchExists(backup_branch_name):\n # Copy ref to backup\n gitUpdateRef(backup_branch_name, branch_name)\n gitDeleteRef(branch_name)\n- found = True\n print(\"renamed old unshelve branch to {0}\".format(backup_branch_name))\n break\n-\n- if not found:\n- sys.exit(\"gave up trying to rename existing branch {0}\".format(sync.branch))\n+ else:\n+ sys.exit(\"gave up trying to rename existing branch {0}\".format(branch_name))\n \n def findLastP4Revision(self, starting_point):\n \"\"\"Look back from starting_point for the first commit created by git-p4\n-- \ngitgitgadget\n"},{"id":"459511","messageId":"xmqqedyf4vg8.fsf@gitster.g","threadId":"58195","inReplyTo":"1484eee8961227a9901e3baa052c74a6d992c220.1658298900.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] git-p4: fix typo in P4Submit.applyCommit()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-20T16:09:43Z","receivedAt":"2022-07-20T16:09:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Moritz Baumann via GitGitGadget\" writes:\n\n> From: Moritz Baumann \n>\n> Signed-off-by: Moritz Baumann \n> ---\n> git-p4.py | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 8fbf6eb1fe3..1de3d6f1cd4 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -2226,7 +2226,7 @@ class P4Submit(Command, P4UserMap):\n> raw=True):\n> if regexp.search(line):\n> if verbose:\n> - print(\"got keyword match on %s in %s in %s\" % (regex.pattern, line, file))\n> + print(\"got keyword match on %s in %s in %s\" % (regexp.pattern, line, file))\n\nOK. That's an obvious fix.\n\n> kwfiles[file] = regexp\n> break\n"},{"id":"459512","messageId":"xmqqa6934v2c.fsf@gitster.g","threadId":"58195","inReplyTo":"69c9fd5fbec859c2cced95930ac4d427a09aee90.1658298900.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] git-p4: fix error handling in P4Unshelve.renameBranch()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-20T16:18:03Z","receivedAt":"2022-07-20T16:18:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Moritz Baumann via GitGitGadget\" writes:\n\n> From: Moritz Baumann \n>\n> The error handling code referenced a variable that does not exist.\n> Also, the condition could never evaluate to True.\n\n\n\n>\n> Signed-off-by: Moritz Baumann \n> ---\n> git-p4.py | 7 ++-----\n> 1 file changed, 2 insertions(+), 5 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 1de3d6f1cd4..8f20d15f272 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -4369,19 +4369,16 @@ class P4Unshelve(Command):\n> def renameBranch(self, branch_name):\n> \"\"\"Rename the existing branch to branch_name.N .\"\"\"\n> \n> - found = True\n\nThis has to be initialized to False, because ...\n\n> for i in range(0, 1000):\n> backup_branch_name = \"{0}.{1}\".format(branch_name, i)\n> if not gitBranchExists(backup_branch_name):\n> # Copy ref to backup\n> gitUpdateRef(backup_branch_name, branch_name)\n> gitDeleteRef(branch_name)\n> - found = True\n> print(\"renamed old unshelve branch to {0}\".format(backup_branch_name))\n\n... we flip it to True when we find an available unused name and\nbreak out ...\n\n> break\n> -\n> - if not found:\n> - sys.exit(\"gave up trying to rename existing branch {0}\".format(sync.branch))\n\n... so that we can complain when we didn't find anything usable.\n\nSo a minimum fix would be to initialize found correctly, and\nrewriting the logic to use \"for ... else\" is an unrelated style\nchange. The version using \"for ... else\" may be more idiomatic\nPython, and I do not think people would mind it, but it should\nbe mentioned in the proposed log mesage, perhaps like:\n\n The code tries to see if there is an available name by setting\n the variable 'found' to true when it finds one and breaks out of\n the loop, but because the variable is incorrectly initialized to\n true (it should be initialized to false), the code after the\n loop cannot tell if it found an available name or not.\n\n It would be the minimal fix to initialize the variable to false,\n but in modern Python it is more idiomatic to add else: clause\n after a loop to write what happens when the loop did not break\n out, so let's do that instead.\n\n Also, fix the error message that refers to a wrong variable\n name.\n\nor something.\n\nThanks. Will queue.\n\n> + else:\n> + sys.exit(\"gave up trying to rename existing branch {0}\".format(branch_name))\n> \n> def findLastP4Revision(self, starting_point):\n> \"\"\"Look back from starting_point for the first commit created by git-p4\n"},{"id":"459513","messageId":"xmqq5yjr4ux8.fsf@gitster.g","threadId":"58195","inReplyTo":"xmqqa6934v2c.fsf@gitster.g","subject":"Re: [PATCH 2/2] git-p4: fix error handling in P4Unshelve.renameBranch()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-20T16:21:07Z","receivedAt":"2022-07-20T16:21:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> \n>> - found = True\n>\n> This has to be initialized to False, because ...\n>\n>> for i in range(0, 1000):\n>> backup_branch_name = \"{0}.{1}\".format(branch_name, i)\n>> if not gitBranchExists(backup_branch_name):\n>> # Copy ref to backup\n>> gitUpdateRef(backup_branch_name, branch_name)\n>> gitDeleteRef(branch_name)\n>> - found = True\n>> print(\"renamed old unshelve branch to {0}\".format(backup_branch_name))\n>\n> ... we flip it to True when we find an available unused name and\n> break out ...\n>\n>> break\n>> -\n>> - if not found:\n>> - sys.exit(\"gave up trying to rename existing branch {0}\".format(sync.branch))\n>\n> ... so that we can complain when we didn't find anything usable.\n\nBy the way, isn't this a typical time-of-check to time-of-use bug?\nNot the problem with the fix in the patch but in the original, but\nregardless of whose fault it is, it may be good to fix it (outside\nthe topic of this patch).\n\nThanks.\n\n"},{"id":"459518","messageId":"AS1PR02MB8185DC4EE3C241B0F9A9234D948E9@AS1PR02MB8185.eurprd02.prod.outlook.com","threadId":"58195","inReplyTo":"xmqq5yjr4ux8.fsf@gitster.g","subject":"RE: [PATCH 2/2] git-p4: fix error handling in P4Unshelve.renameBranch()","fromName":"Baumann, Moritz","fromEmail":"moritz.baumann@sap.com","sentAt":"2022-07-20T17:01:54Z","receivedAt":"2022-07-20T17:01:59Z","isPatch":true,"sender":{"key":"moritz.baumann@sap.com","avatar":"https://avatars.githubusercontent.com/u/67415316?v=4"},"body":"Thank you for your criticism, I will keep it in mind and submit more detailed descriptions in the future.\n\n> By the way, isn't this a typical time-of-check to time-of-use bug?\n> Not the problem with the fix in the patch but in the original, but regardless of\n> whose fault it is, it may be good to fix it (outside the topic of this patch).\n\nIs concurrent use even meant to be supported in general? I have not done a thorough review, but judging from what I have seen so far, I highly doubt that the majority of git-p4.py was written with potential concurrency problems in mind.\n\nBest regards,\nMoritz\n"},{"id":"459532","messageId":"1484eee8961227a9901e3baa052c74a6d992c220.1658343330.git.gitgitgadget@gmail.com","threadId":"58195","inReplyTo":"pull.1297.v2.git.git.1658343330.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] git-p4: fix typo in P4Submit.applyCommit()","fromName":"Moritz Baumann via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-20T18:55:29Z","receivedAt":"2022-07-20T18:56:33Z","isPatch":true,"sender":{"key":"name:Moritz Baumann","avatar":null},"body":"From: Moritz Baumann \n\nSigned-off-by: Moritz Baumann \n---\n git-p4.py | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8fbf6eb1fe3..1de3d6f1cd4 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -2226,7 +2226,7 @@ class P4Submit(Command, P4UserMap):\n raw=True):\n if regexp.search(line):\n if verbose:\n- print(\"got keyword match on %s in %s in %s\" % (regex.pattern, line, file))\n+ print(\"got keyword match on %s in %s in %s\" % (regexp.pattern, line, file))\n kwfiles[file] = regexp\n break\n \n-- \ngitgitgadget\n\n"},{"id":"459533","messageId":"pull.1297.v2.git.git.1658343330.gitgitgadget@gmail.com","threadId":"58195","inReplyTo":"pull.1297.git.git.1658298900.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] git-p4: fix two undefined variables","fromName":"Moritz Baumann via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-20T18:55:28Z","receivedAt":"2022-07-20T18:56:33Z","isPatch":true,"sender":{"key":"name:Moritz Baumann","avatar":null},"body":"Moritz Baumann (2):\n git-p4: fix typo in P4Submit.applyCommit()\n git-p4: fix error handling in P4Unshelve.renameBranch()\n\n git-p4.py | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\n\nbase-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1297%2Fmbs-c%2Ffix-undefined-variables-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1297/mbs-c/fix-undefined-variables-v2\nPull-Request: https://github.com/git/git/pull/1297\n\nRange-diff vs v1:\n\n 1: 1484eee8961 = 1: 1484eee8961 git-p4: fix typo in P4Submit.applyCommit()\n 2: 69c9fd5fbec ! 2: f7566dd5fc3 git-p4: fix error handling in P4Unshelve.renameBranch()\n @@ Metadata\n ## Commit message ##\n git-p4: fix error handling in P4Unshelve.renameBranch()\n \n - The error handling code referenced a variable that does not exist.\n - Also, the condition could never evaluate to True.\n + The error handling code path is meant to be triggered when the loop does\n + not exit early via \"break\". This fails, as the boolean variable \"found\",\n + which is used to track whether the loop was exited early, is initialized\n + incorrectly.\n +\n + It would be possible to fix this issue by correcting the initialization,\n + but Python supports a for:-else: control flow construct for this exact\n + use case (executing code if a loop does not exit early), so it is more\n + idiomatic to remove the tracking variable entirely.\n +\n + In addition, the error message no longer refers to a variable that does\n + not exist.\n \n Signed-off-by: Moritz Baumann \n \n\n-- \ngitgitgadget\n"},{"id":"459534","messageId":"f7566dd5fc36fd3ae648abf7054fc0e1269b6874.1658343330.git.gitgitgadget@gmail.com","threadId":"58195","inReplyTo":"pull.1297.v2.git.git.1658343330.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] git-p4: fix error handling in P4Unshelve.renameBranch()","fromName":"Moritz Baumann via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-20T18:55:30Z","receivedAt":"2022-07-20T18:56:33Z","isPatch":true,"sender":{"key":"name:Moritz Baumann","avatar":null},"body":"From: Moritz Baumann \n\nThe error handling code path is meant to be triggered when the loop does\nnot exit early via \"break\". This fails, as the boolean variable \"found\",\nwhich is used to track whether the loop was exited early, is initialized\nincorrectly.\n\nIt would be possible to fix this issue by correcting the initialization,\nbut Python supports a for:-else: control flow construct for this exact\nuse case (executing code if a loop does not exit early), so it is more\nidiomatic to remove the tracking variable entirely.\n\nIn addition, the error message no longer refers to a variable that does\nnot exist.\n\nSigned-off-by: Moritz Baumann \n---\n git-p4.py | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 1de3d6f1cd4..8f20d15f272 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -4369,19 +4369,16 @@ class P4Unshelve(Command):\n def renameBranch(self, branch_name):\n \"\"\"Rename the existing branch to branch_name.N .\"\"\"\n \n- found = True\n for i in range(0, 1000):\n backup_branch_name = \"{0}.{1}\".format(branch_name, i)\n if not gitBranchExists(backup_branch_name):\n # Copy ref to backup\n gitUpdateRef(backup_branch_name, branch_name)\n gitDeleteRef(branch_name)\n- found = True\n print(\"renamed old unshelve branch to {0}\".format(backup_branch_name))\n break\n-\n- if not found:\n- sys.exit(\"gave up trying to rename existing branch {0}\".format(sync.branch))\n+ else:\n+ sys.exit(\"gave up trying to rename existing branch {0}\".format(branch_name))\n \n def findLastP4Revision(self, starting_point):\n \"\"\"Look back from starting_point for the first commit created by git-p4\n-- \ngitgitgadget\n"},{"id":"459537","messageId":"xmqq35ev33ov.fsf@gitster.g","threadId":"58195","inReplyTo":"pull.1297.v2.git.git.1658343330.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] git-p4: fix two undefined variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-20T20:54:40Z","receivedAt":"2022-07-20T20:54:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Moritz Baumann via GitGitGadget\" writes:\n\n> Moritz Baumann (2):\n> git-p4: fix typo in P4Submit.applyCommit()\n> git-p4: fix error handling in P4Unshelve.renameBranch()\n>\n> git-p4.py | 9 +++------\n> 1 file changed, 3 insertions(+), 6 deletions(-)\n\nPerfect. Thanks.\n"}]}