[2/2] git-p4: fix error handling in P4Unshelve.renameBranch()

Commit Message

Baumann, Moritz July 20, 2022, 6:34 a.m. UTC

From: Moritz Baumann <[e-mail]>

The error handling code referenced a variable that does not exist.
Also, the condition could never evaluate to True.

Signed-off-by: Moritz Baumann <[e-mail]>
---
 git-p4.py | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Junio C Hamano July 20, 2022, 4:18 p.m. UTC | #1

"Moritz Baumann via GitGitGadget" <[e-mail]> writes:

> From: Moritz Baumann <[e-mail]>
>
> The error handling code referenced a variable that does not exist.
> Also, the condition could never evaluate to True.



>
> Signed-off-by: Moritz Baumann <[e-mail]>
> ---
>  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-p4

Junio C Hamano July 20, 2022, 4:21 p.m. UTC | #2

Junio C Hamano <[e-mail]> 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.

Baumann, Moritz July 20, 2022, 5:01 p.m. UTC | #3

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

Patch

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
         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))
                 break
-
-        if not found:
-            sys.exit("gave up trying to rename existing branch {0}".format(sync.branch))
+        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-p4