git-p4: use raw string literals for regular expressions

PATCH Superseded James Touton via GitGitGadget 3 participants 6 messages Jan 20, 2024 – Jan 29, 2024

lore

Share & tools

lore

James Touton via GitGitGadget

author

2024-01-20 18:25 UTC #0 lore raw

PATCH 1/1 git-p4: use raw string literals for regular expressions

From: James Touton <[email protected]>

Fixes several Python diagnostics about invalid escape sequences.

Signed-off-by James Touton <[email protected]>

---
git-p4: use raw string literals for regular expressions

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1639%2FBekenn%2Fp4-raw-strings-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1639/Bekenn/p4-raw-strings-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1639

 git-p4.py | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

git-p4.py +13 ~~-13~~

index 0eb3bb4c47d..156597adb59 100755
@@ -689,8 +689,8 @@ def setP4ExecBit(file, mode):
 
     if not isModeExec(mode):
         p4Type = getP4OpenedType(file)
-        p4Type = re.sub('^([cku]?)x(.*)', '\\1\\2', p4Type)
-        p4Type = re.sub('(.*?\+.*?)x(.*?)', '\\1\\2', p4Type)
+        p4Type = re.sub(r'^([cku]?)x(.*)', r'\1\2', p4Type)
+        p4Type = re.sub(r'(.*?\+.*?)x(.*?)', r'\1\2', p4Type)
         if p4Type[-1] == "+":
             p4Type = p4Type[0:-1]

@@ -701,7 +701,7 @@ def getP4OpenedType(file):
     """Returns the perforce file type for the given file."""
 
     result = p4_read_pipe(["opened", wildcard_encode(file)])
-    match = re.match(".*\((.+)\)( \*exclusive\*)?\r?$", result)
+    match = re.match(r".*\((.+)\)( \*exclusive\*)?\r?$", result)
     if match:
         return match.group(1)
     else:

@@ -757,7 +757,7 @@ def parseDiffTreeEntry(entry):
 
     global _diff_tree_pattern
     if not _diff_tree_pattern:
-        _diff_tree_pattern = re.compile(':(\d+) (\d+) (\w+) (\w+) ([A-Z])(\d+)?\t(.*?)((\t(.*))|$)')
+        _diff_tree_pattern = re.compile(r':(\d+) (\d+) (\w+) (\w+) ([A-Z])(\d+)?\t(.*?)((\t(.*))|$)')
 
     match = _diff_tree_pattern.match(entry)
     if match:

@@ -918,9 +918,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
             if len(result) > 0:
                 data = result[0].get('data')
                 if data:
-                    m = re.search('Too many rows scanned \(over (\d+)\)', data)
+                    m = re.search(r'Too many rows scanned \(over (\d+)\)', data)
                     if not m:
-                        m = re.search('Request too large \(over (\d+)\)', data)
+                        m = re.search(r'Request too large \(over (\d+)\)', data)
 
                     if m:
                         limit = int(m.group(1))

@@ -1452,7 +1452,7 @@ def wildcard_encode(path):
 
 
 def wildcard_present(path):
-    m = re.search("[*#@%]", path)
+    m = re.search(r"[*#@%]", path)
     return m is not None

@@ -3048,7 +3048,7 @@ def stripRepoPath(self, path, prefixes):
             # Preserve everything in relative path name except leading
             # //depot/; just look at first prefix as they all should
             # be in the same depot.
-            depot = re.sub("^(//[^/]+/).*", r'\1', prefixes[0])
+            depot = re.sub(r"^(//[^/]+/).*", r'\1', prefixes[0])
             if p4PathStartsWith(path, depot):
                 path = path[len(depot):]

@@ -3603,7 +3603,7 @@ def importP4Labels(self, stream, p4Labels):
                     commitFound = True
                 else:
                     gitCommit = read_pipe(["git", "rev-list", "--max-count=1",
-                        "--reverse", ":/\[git-p4:.*change = %d\]" % changelist], ignore_error=True)
+                        "--reverse", r":/\[git-p4:.*change = %d\]" % changelist], ignore_error=True)
                     if len(gitCommit) == 0:
                         print("importing label %s: could not find git commit for changelist %d" % (name, changelist))
                     else:

@@ -4182,7 +4182,7 @@ def run(self, args):
                 if len(self.changesFile) == 0:
                     revision = "#head"
 
-            p = re.sub("\.\.\.$", "", p)
+            p = re.sub(r"\.\.\.$", "", p)
             if not p.endswith("/"):
                 p += "/"

@@ -4291,7 +4291,7 @@ def rebase(self):
             die("Cannot find upstream branchpoint for rebase")
 
         # the branchpoint may be p4/foo~3, so strip off the parent
-        upstream = re.sub("~[0-9]+$", "", upstream)
+        upstream = re.sub(r"~[0-9]+$", "", upstream)
 
         print("Rebasing the current branch onto %s" % upstream)
         oldHead = read_pipe(["git", "rev-parse", "HEAD"]).strip()

@@ -4320,8 +4320,8 @@ def __init__(self):
     def defaultDestination(self, args):
         # TODO: use common prefix of args?
         depotPath = args[0]
-        depotDir = re.sub("(@[^@]*)$", "", depotPath)
-        depotDir = re.sub("(#[^#]*)$", "", depotDir)
+        depotDir = re.sub(r"(@[^@]*)$", "", depotPath)
+        depotDir = re.sub(r"(#[^#]*)$", "", depotDir)
         depotDir = re.sub(r"\.\.\.$", "", depotDir)
         depotDir = re.sub(r"/$", "", depotDir)
         return os.path.split(depotDir)[1]

base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-01-20 18:47 UTC #1 lore raw

Quoted 3 lines · James Touton via GitGitGadget

From: James Touton <[email protected]>
Fixes several Python diagnostics about invalid escape sequences.

Thanks for noticing, but we want a bit more backstory explained here in the proposed commit log message, outlining:

1. With what version of Python the deprecation warning started.

This will help us judge the urgency of the fix. If I am reading the docs.python.org/$version/howto/regex.html right, we do not see this

In addition, special escape sequences that are valid in regular expressions, but not valid as Python string literals, now result in a DeprecationWarning and will eventually become a SyntaxError, which means the sequences will be invalid if raw string notation or escaping the backslashes isn’t used.

in Python 3.5's document, but Python 3.6's document starts talking about the warning. Python 3.6 was released at the end of 2016 so it is 7 years old---users have lived with the warning for this many years, so if the above reasoning is correct, this is not all that urgent to require a maintenance release.

2. How well the new construct, used by the code after applying this patch, is supported by older version of Python.

This will assure us that the change will not be robbing from users of older versions of Python to pay users of newer versions of Python. Again, if I am reading the documentation right, feeding r'' raw strings to regexp engine was supported even by Python 2.7, which is what git-p4.py already requires, so we should be OK.

But we want the developers who propose a change to explain why it is a good idea, and why it is a safe change to make, in their proposed commit log message, instead of forcing the reviewers to do that for them.

For other syntactic and linguistic hints on writing a proposed log message, please check Documentation/SubmittingPatches document.

Thanks, again.

Quoted 114 lines

Signed-off-by James Touton <[email protected]>

---
git-p4: use raw string literals for regular expressions

 git-p4.py | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

git-p4.py +13 ~~-13~~

index 0eb3bb4c47d..156597adb59 100755
@@ -689,8 +689,8 @@ def setP4ExecBit(file, mode):
 
     if not isModeExec(mode):
         p4Type = getP4OpenedType(file)
-        p4Type = re.sub('^([cku]?)x(.*)', '\\1\\2', p4Type)
-        p4Type = re.sub('(.*?\+.*?)x(.*?)', '\\1\\2', p4Type)
+        p4Type = re.sub(r'^([cku]?)x(.*)', r'\1\2', p4Type)
+        p4Type = re.sub(r'(.*?\+.*?)x(.*?)', r'\1\2', p4Type)
         if p4Type[-1] == "+":
             p4Type = p4Type[0:-1]

@@ -701,7 +701,7 @@ def getP4OpenedType(file):
     """Returns the perforce file type for the given file."""
 
     result = p4_read_pipe(["opened", wildcard_encode(file)])
-    match = re.match(".*\((.+)\)( \*exclusive\*)?\r?$", result)
+    match = re.match(r".*\((.+)\)( \*exclusive\*)?\r?$", result)
     if match:
         return match.group(1)
     else:

@@ -757,7 +757,7 @@ def parseDiffTreeEntry(entry):
 
     global _diff_tree_pattern
     if not _diff_tree_pattern:
-        _diff_tree_pattern = re.compile(':(\d+) (\d+) (\w+) (\w+) ([A-Z])(\d+)?\t(.*?)((\t(.*))|$)')
+        _diff_tree_pattern = re.compile(r':(\d+) (\d+) (\w+) (\w+) ([A-Z])(\d+)?\t(.*?)((\t(.*))|$)')
 
     match = _diff_tree_pattern.match(entry)
     if match:

@@ -918,9 +918,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
             if len(result) > 0:
                 data = result[0].get('data')
                 if data:
-                    m = re.search('Too many rows scanned \(over (\d+)\)', data)
+                    m = re.search(r'Too many rows scanned \(over (\d+)\)', data)
                     if not m:
-                        m = re.search('Request too large \(over (\d+)\)', data)
+                        m = re.search(r'Request too large \(over (\d+)\)', data)
 
                     if m:
                         limit = int(m.group(1))

@@ -1452,7 +1452,7 @@ def wildcard_encode(path):
 
 
 def wildcard_present(path):
-    m = re.search("[*#@%]", path)
+    m = re.search(r"[*#@%]", path)
     return m is not None

@@ -3048,7 +3048,7 @@ def stripRepoPath(self, path, prefixes):
             # Preserve everything in relative path name except leading
             # //depot/; just look at first prefix as they all should
             # be in the same depot.
-            depot = re.sub("^(//[^/]+/).*", r'\1', prefixes[0])
+            depot = re.sub(r"^(//[^/]+/).*", r'\1', prefixes[0])
             if p4PathStartsWith(path, depot):
                 path = path[len(depot):]

@@ -3603,7 +3603,7 @@ def importP4Labels(self, stream, p4Labels):
                     commitFound = True
                 else:
                     gitCommit = read_pipe(["git", "rev-list", "--max-count=1",
-                        "--reverse", ":/\[git-p4:.*change = %d\]" % changelist], ignore_error=True)
+                        "--reverse", r":/\[git-p4:.*change = %d\]" % changelist], ignore_error=True)
                     if len(gitCommit) == 0:
                         print("importing label %s: could not find git commit for changelist %d" % (name, changelist))
                     else:

@@ -4182,7 +4182,7 @@ def run(self, args):
                 if len(self.changesFile) == 0:
                     revision = "#head"
 
-            p = re.sub("\.\.\.$", "", p)
+            p = re.sub(r"\.\.\.$", "", p)
             if not p.endswith("/"):
                 p += "/"

@@ -4291,7 +4291,7 @@ def rebase(self):
             die("Cannot find upstream branchpoint for rebase")
 
         # the branchpoint may be p4/foo~3, so strip off the parent
-        upstream = re.sub("~[0-9]+$", "", upstream)
+        upstream = re.sub(r"~[0-9]+$", "", upstream)
 
         print("Rebasing the current branch onto %s" % upstream)
         oldHead = read_pipe(["git", "rev-parse", "HEAD"]).strip()

@@ -4320,8 +4320,8 @@ def __init__(self):
     def defaultDestination(self, args):
         # TODO: use common prefix of args?
         depotPath = args[0]
-        depotDir = re.sub("(@[^@]*)$", "", depotPath)
-        depotDir = re.sub("(#[^#]*)$", "", depotDir)
+        depotDir = re.sub(r"(@[^@]*)$", "", depotPath)
+        depotDir = re.sub(r"(#[^#]*)$", "", depotDir)
         depotDir = re.sub(r"\.\.\.$", "", depotDir)
         depotDir = re.sub(r"/$", "", depotDir)
         return os.path.split(depotDir)[1]

base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d

James Touton

2024-01-20 19:34 UTC #2 lore raw

Quoted 41 lines · Junio C Hamano

Quoted 3 lines · James Touton via GitGitGadget
From: James Touton <[email protected]>
Fixes several Python diagnostics about invalid escape sequences.
Thanks for noticing, but we want a bit more backstory explained here in the proposed commit log message, outlining:
1. With what version of Python the deprecation warning started.
This will help us judge the urgency of the fix. If I am reading the docs.python.org/$version/howto/regex.html right, we do not see this
In addition, special escape sequences that are valid in regular expressions, but not valid as Python string literals, now result in a DeprecationWarning and will eventually become a SyntaxError, which means the sequences will be invalid if raw string notation or escaping the backslashes isn’t used.
in Python 3.5's document, but Python 3.6's document starts talking about the warning. Python 3.6 was released at the end of 2016 so it is 7 years old---users have lived with the warning for this many years, so if the above reasoning is correct, this is not all that urgent to require a maintenance release.
2. How well the new construct, used by the code after applying this patch, is supported by older version of Python.
This will assure us that the change will not be robbing from users of older versions of Python to pay users of newer versions of Python. Again, if I am reading the documentation right, feeding r'' raw strings to regexp engine was supported even by Python 2.7, which is what git-p4.py already requires, so we should be OK.
But we want the developers who propose a change to explain why it is a good idea, and why it is a safe change to make, in their proposed commit log message, instead of forcing the reviewers to do that for them.

Quoted 4 lines

For other syntactic and linguistic hints on writing a proposed log message, please check Documentation/SubmittingPatches document.
Thanks, again.

Thanks for the notes, I will update this when I have the opportunity.

Raw strings were already present in the file, just not in these particular locations. Given that, I wouldn't expect them to need any particular explanation; do you still want some mention of compatibility constraints, or is it enough to mention that they're already in use?

Junio C Hamano

2024-01-20 21:54 UTC #3 lore raw

Quoted 2 lines · James Touton

... is it enough to mention that they're
already in use?

Sure. Thanks.

James Touton via GitGitGadget

author

2024-01-26 23:41 UTC #4 lore raw

PATCH v2 1/1 git-p4: use raw string literals for regular expressions

In reply to James Touton via GitGitGadget

From: James Touton <[email protected]>

Fixes several Python diagnostics about invalid escape sequences. The diagnostics appear for me in Python 3.12, and may appear in earlier versions. The fix is to use raw string literals so that backslashes are not interpreted as introducing escape sequences. Raw string literals are already in use in this file, so adding more does not impact toolchain compatibility.

Signed-off-by James Touton <[email protected]>

---
git-p4: use raw string literals for regular expressions

Changes since v1:

* Updated commit message to include the Python version where the diagnostics were observed.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1639%2FBekenn%2Fp4-raw-strings-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1639/Bekenn/p4-raw-strings-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1639

Range-diff vs v1:

1: 1ea38dc4643 ! 1: 122ff28ffbd git-p4: use raw string literals for regular expressions
@@ Metadata
## Commit message ##
git-p4: use raw string literals for regular expressions

- Fixes several Python diagnostics about invalid escape sequences.
+ Fixes several Python diagnostics about invalid escape sequences. The
+ diagnostics appear for me in Python 3.12, and may appear in earlier
+ versions. The fix is to use raw string literals so that backslashes are
+ not interpreted as introducing escape sequences. Raw string literals
+ are already in use in this file, so adding more does not impact
+ toolchain compatibility.

Signed-off-by James Touton <[email protected]>

 git-p4.py | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

git-p4.py +13 ~~-13~~

index 0eb3bb4c47d..156597adb59 100755
@@ -689,8 +689,8 @@ def setP4ExecBit(file, mode):
 
     if not isModeExec(mode):
         p4Type = getP4OpenedType(file)
-        p4Type = re.sub('^([cku]?)x(.*)', '\\1\\2', p4Type)
-        p4Type = re.sub('(.*?\+.*?)x(.*?)', '\\1\\2', p4Type)
+        p4Type = re.sub(r'^([cku]?)x(.*)', r'\1\2', p4Type)
+        p4Type = re.sub(r'(.*?\+.*?)x(.*?)', r'\1\2', p4Type)
         if p4Type[-1] == "+":
             p4Type = p4Type[0:-1]

@@ -701,7 +701,7 @@ def getP4OpenedType(file):
     """Returns the perforce file type for the given file."""
 
     result = p4_read_pipe(["opened", wildcard_encode(file)])
-    match = re.match(".*\((.+)\)( \*exclusive\*)?\r?$", result)
+    match = re.match(r".*\((.+)\)( \*exclusive\*)?\r?$", result)
     if match:
         return match.group(1)
     else:

@@ -757,7 +757,7 @@ def parseDiffTreeEntry(entry):
 
     global _diff_tree_pattern
     if not _diff_tree_pattern:
-        _diff_tree_pattern = re.compile(':(\d+) (\d+) (\w+) (\w+) ([A-Z])(\d+)?\t(.*?)((\t(.*))|$)')
+        _diff_tree_pattern = re.compile(r':(\d+) (\d+) (\w+) (\w+) ([A-Z])(\d+)?\t(.*?)((\t(.*))|$)')
 
     match = _diff_tree_pattern.match(entry)
     if match:

@@ -918,9 +918,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
             if len(result) > 0:
                 data = result[0].get('data')
                 if data:
-                    m = re.search('Too many rows scanned \(over (\d+)\)', data)
+                    m = re.search(r'Too many rows scanned \(over (\d+)\)', data)
                     if not m:
-                        m = re.search('Request too large \(over (\d+)\)', data)
+                        m = re.search(r'Request too large \(over (\d+)\)', data)
 
                     if m:
                         limit = int(m.group(1))

@@ -1452,7 +1452,7 @@ def wildcard_encode(path):
 
 
 def wildcard_present(path):
-    m = re.search("[*#@%]", path)
+    m = re.search(r"[*#@%]", path)
     return m is not None

@@ -3048,7 +3048,7 @@ def stripRepoPath(self, path, prefixes):
             # Preserve everything in relative path name except leading
             # //depot/; just look at first prefix as they all should
             # be in the same depot.
-            depot = re.sub("^(//[^/]+/).*", r'\1', prefixes[0])
+            depot = re.sub(r"^(//[^/]+/).*", r'\1', prefixes[0])
             if p4PathStartsWith(path, depot):
                 path = path[len(depot):]

@@ -3603,7 +3603,7 @@ def importP4Labels(self, stream, p4Labels):
                     commitFound = True
                 else:
                     gitCommit = read_pipe(["git", "rev-list", "--max-count=1",
-                        "--reverse", ":/\[git-p4:.*change = %d\]" % changelist], ignore_error=True)
+                        "--reverse", r":/\[git-p4:.*change = %d\]" % changelist], ignore_error=True)
                     if len(gitCommit) == 0:
                         print("importing label %s: could not find git commit for changelist %d" % (name, changelist))
                     else:

@@ -4182,7 +4182,7 @@ def run(self, args):
                 if len(self.changesFile) == 0:
                     revision = "#head"
 
-            p = re.sub("\.\.\.$", "", p)
+            p = re.sub(r"\.\.\.$", "", p)
             if not p.endswith("/"):
                 p += "/"

@@ -4291,7 +4291,7 @@ def rebase(self):
             die("Cannot find upstream branchpoint for rebase")
 
         # the branchpoint may be p4/foo~3, so strip off the parent
-        upstream = re.sub("~[0-9]+$", "", upstream)
+        upstream = re.sub(r"~[0-9]+$", "", upstream)
 
         print("Rebasing the current branch onto %s" % upstream)
         oldHead = read_pipe(["git", "rev-parse", "HEAD"]).strip()

@@ -4320,8 +4320,8 @@ def __init__(self):
     def defaultDestination(self, args):
         # TODO: use common prefix of args?
         depotPath = args[0]
-        depotDir = re.sub("(@[^@]*)$", "", depotPath)
-        depotDir = re.sub("(#[^#]*)$", "", depotDir)
+        depotDir = re.sub(r"(@[^@]*)$", "", depotPath)
+        depotDir = re.sub(r"(#[^#]*)$", "", depotDir)
         depotDir = re.sub(r"\.\.\.$", "", depotDir)
         depotDir = re.sub(r"/$", "", depotDir)
         return os.path.split(depotDir)[1]

base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-01-29 17:26 UTC #5 lore raw

Quoted 37 lines · James Touton via GitGitGadget

From: James Touton <[email protected]>
Fixes several Python diagnostics about invalid escape sequences. The diagnostics appear for me in Python 3.12, and may appear in earlier versions. The fix is to use raw string literals so that backslashes are not interpreted as introducing escape sequences. Raw string literals are already in use in this file, so adding more does not impact toolchain compatibility.
Signed-off-by James Touton <[email protected]>
---
git-p4: use raw string literals for regular expressions
Changes since v1:
* Updated commit message to include the Python version where the diagnostics were observed.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1639%2FBekenn%2Fp4-raw-strings-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1639/Bekenn/p4-raw-strings-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1639
Range-diff vs v1:
1: 1ea38dc4643 ! 1: 122ff28ffbd git-p4: use raw string literals for regular expressions
@@ Metadata
## Commit message ##
git-p4: use raw string literals for regular expressions
- Fixes several Python diagnostics about invalid escape sequences.
+ Fixes several Python diagnostics about invalid escape sequences. The
+ diagnostics appear for me in Python 3.12, and may appear in earlier
+ versions. The fix is to use raw string literals so that backslashes are
+ not interpreted as introducing escape sequences. Raw string literals
+ are already in use in this file, so adding more does not impact
+ toolchain compatibility.

Quoted 2 lines

Signed-off-by James Touton <[email protected]>

Thanks. Let's merge it down to 'next'.