git-p4: fix two undefined variables #1297
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Moritz Baumann <moritz.baumann@sap.com>
|
/preview |
|
Preview email sent as pull.1297.git.git.1658298463.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1297.git.git.1658298900.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
rimrul
reviewed
Jul 20, 2022
![gitgitgadget-git[bot]](https://avatars.githubusercontent.com/in/46807?s=60&v=4){kind=small}
| @@ -2226,7 +2226,7 @@ def applyCommit(self, id): | |||
| raw=True): | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Moritz Baumann via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Moritz Baumann <moritz.baumann@sap.com>
>
> Signed-off-by: Moritz Baumann <moritz.baumann@sap.com>
> ---
> git-p4.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 8fbf6eb1fe3..1de3d6f1cd4 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2226,7 +2226,7 @@ class P4Submit(Command, P4UserMap):
> raw=True):
> if regexp.search(line):
> if verbose:
> - print("got keyword match on %s in %s in %s" % (regex.pattern, line, file))
> + print("got keyword match on %s in %s in %s" % (regexp.pattern, line, file))
OK. That's an obvious fix.
> kwfiles[file] = regexp
> break
| @@ -2226,7 +2226,7 @@ def applyCommit(self, id): | |||
| raw=True): | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Moritz Baumann via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Moritz Baumann <moritz.baumann@sap.com>
>
> The error handling code referenced a variable that does not exist.
> Also, the condition could never evaluate to True.
>
> Signed-off-by: Moritz Baumann <moritz.baumann@sap.com>
> ---
> git-p4.py | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 1de3d6f1cd4..8f20d15f272 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -4369,19 +4369,16 @@ class P4Unshelve(Command):
> def renameBranch(self, branch_name):
> """Rename the existing branch to branch_name.N ."""
>
> - found = True
This has to be initialized to False, because ...
> for i in range(0, 1000):
> backup_branch_name = "{0}.{1}".format(branch_name, i)
> if not gitBranchExists(backup_branch_name):
> # Copy ref to backup
> gitUpdateRef(backup_branch_name, branch_name)
> gitDeleteRef(branch_name)
> - found = True
> print("renamed old unshelve branch to {0}".format(backup_branch_name))
... we flip it to True when we find an available unused name and
break out ...
> break
> -
> - if not found:
> - sys.exit("gave up trying to rename existing branch {0}".format(sync.branch))
... so that we can complain when we didn't find anything usable.
So a minimum fix would be to initialize found correctly, and
rewriting the logic to use "for ... else" is an unrelated style
change. The version using "for ... else" may be more idiomatic
Python, and I do not think people would mind it, but it should
be mentioned in the proposed log mesage, perhaps like:
The code tries to see if there is an available name by setting
the variable 'found' to true when it finds one and breaks out of
the loop, but because the variable is incorrectly initialized to
true (it should be initialized to false), the code after the
loop cannot tell if it found an available name or not.
It would be the minimal fix to initialize the variable to false,
but in modern Python it is more idiomatic to add else: clause
after a loop to write what happens when the loop did not break
out, so let's do that instead.
Also, fix the error message that refers to a wrong variable
name.
or something.
Thanks. Will queue.
> + else:
> + sys.exit("gave up trying to rename existing branch {0}".format(branch_name))
>
> def findLastP4Revision(self, starting_point):
> """Look back from starting_point for the first commit created by git-p4There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Junio C Hamano <gitster@pobox.com> writes:
>>
>> - found = True
>
> This has to be initialized to False, because ...
>
>> for i in range(0, 1000):
>> backup_branch_name = "{0}.{1}".format(branch_name, i)
>> if not gitBranchExists(backup_branch_name):
>> # Copy ref to backup
>> gitUpdateRef(backup_branch_name, branch_name)
>> gitDeleteRef(branch_name)
>> - found = True
>> print("renamed old unshelve branch to {0}".format(backup_branch_name))
>
> ... we flip it to True when we find an available unused name and
> break out ...
>
>> break
>> -
>> - if not found:
>> - sys.exit("gave up trying to rename existing branch {0}".format(sync.branch))
>
> ... so that we can complain when we didn't find anything usable.
By the way, isn't this a typical time-of-check to time-of-use bug?
Not the problem with the fix in the patch but in the original, but
regardless of whose fault it is, it may be good to fix it (outside
the topic of this patch).
Thanks.
There was a problem hiding this comment.
On the Git mailing list, "Baumann, Moritz" wrote (reply to this):
Thank you for your criticism, I will keep it in mind and submit more detailed descriptions in the future.
> By the way, isn't this a typical time-of-check to time-of-use bug?
> Not the problem with the fix in the patch but in the original, but regardless of
> whose fault it is, it may be good to fix it (outside the topic of this patch).
Is 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.
Best regards,
Moritz
|
User |
The error handling code path is meant to be triggered when the loop does not exit early via "break". This fails, as the boolean variable "found", which is used to track whether the loop was exited early, is initialized incorrectly. It would be possible to fix this issue by correcting the initialization, but Python supports a for:-else: control flow construct for this exact use case (executing code if a loop does not exit early), so it is more idiomatic to remove the tracking variable entirely. In addition, the error message no longer refers to a variable that does not exist. Signed-off-by: Moritz Baumann <moritz.baumann@sap.com>
mbs-c
force-pushed
the
fix-undefined-variables
branch
from
69c9fd5
to
f7566dd
Compare
|
/preview |
|
Preview email sent as pull.1297.v2.git.git.1658342543.gitgitgadget@gmail.com |
|
/submit |
|
This branch is now known as |
|
This patch series was integrated into seen via a17fcea. |
|
Submitted as pull.1297.v2.git.git.1658343330.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Moritz Baumann via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Moritz Baumann (2):
> git-p4: fix typo in P4Submit.applyCommit()
> git-p4: fix error handling in P4Unshelve.renameBranch()
>
> git-p4.py | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
Perfect. Thanks. |
|
This patch series was integrated into seen via 6c67f17. |
|
This patch series was integrated into next via 7942d72. |
|
This patch series was integrated into seen via 4d352e3. |
|
This patch series was integrated into seen via 9080e08. |
|
This patch series was integrated into seen via 87e7623. |
tuckstarrydell
referenced
this pull request
Jul 26, 2022
Git now shows better information in the GitHub workflow runs when a test case failed. However, when a test case was implemented incorrectly and therefore does not even run, nothing is shown. Let's bring back the step that prints the full logs of the failed tests, and to improve the user experience, print out an informational message for readers so that they do not have to know/remember where to see the full logs. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
This patch series was integrated into seen via 04e340b. |
|
This patch series was integrated into master via 04e340b. |
|
This patch series was integrated into next via 04e340b. |
|
Closed via 04e340b. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CC: Luke Diamand luke@diamand.org