Feature: New Variable git-p4.binary

Issue: Using git-p4.py on Windows does not resolve properly to the p4.exe binary in all instances.

Two new code features are added to resolve the p4 executable location:

1. A new variable, git-p4.binary, has been added that takes precedence over the default p4 executable name. If this git option is set and the path.exists() passes for this file it will be used as executable for the system.popen calls.

2. If the new variable git-p4.binary is not set, the program checks if the operating system is Windows. If it is, the executable is changed to 'p4.exe'. All other operating systems
(those that do not report 'Windows' in the platform.system() call) continue to use the current executable of 'p4'.

Ben Keene (2):
Cast byte strings to unicode strings in python3
Added general variable git-p4.binary and added a default for windows

    of 'P4.EXE'

 Documentation/git-p4.txt |  5 +++++
 git-p4.py                | 40 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 3 deletions(-)

base-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-465%2Fseraphire%2Fseraphire%2Fp4-binary-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-465/seraphire/seraphire/p4-binary-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/465

gitgitgadget

From: Ben Keene <[email protected]>

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

index 60c73b6a37..6e8b3a26cd 100755
@@ -36,12 +36,22 @@
     unicode = str
     bytes = bytes
     basestring = (str,bytes)
+    isunicode = True
+    def ustring(text):
+        """Returns the byte string as a unicode string"""
+        if text == '' or text == b'':
+            return ''
+        return unicode(text, "utf-8")
 else:
     # 'unicode' exists, must be Python 2
     str = str
     unicode = unicode
     bytes = str
     basestring = basestring
+    isunicode = False
+    def ustring(text):
+        """Returns the byte string unchanged"""
+        return text
 
 try:
     from subprocess import CalledProcessError

@@ -196,6 +206,8 @@ def read_pipe_full(c):
     expand = isinstance(c,basestring)
     p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=expand)
     (out, err) = p.communicate()
+    out = ustring(out)
+    err = ustring(err)
     return (p.returncode, out, err)
 
 def read_pipe(c, ignore_error=False):

@@ -263,6 +275,7 @@ def p4_has_move_command():
     cmd = p4_build_cmd(["move", "-k", "@from", "@to"])
     p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
     (out, err) = p.communicate()
+    err = ustring(err)
     # return code will be 1 in either case
     if err.find("Invalid option") >= 0:
         return False

@@ -646,10 +659,18 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
             if skip_info:
                 if 'code' in entry and entry['code'] == 'info':
                     continue
+                if b'code' in entry and entry[b'code'] == b'info':
+                    continue
             if cb is not None:
                 cb(entry)
             else:
-                result.append(entry)
+                if isunicode:
+                    out = {}
+                    for key, value in entry.items():
+                        out[ustring(key)] = ustring(value)
+                    result.append(out)
+                else:
+                    result.append(entry)
     except EOFError:
         pass
     exitCode = p4.wait()

@@ -792,7 +813,7 @@ def gitConfig(key, typeSpecifier=None):
         cmd += [ key ]
         s = read_pipe(cmd, ignore_error=True)
         _gitConfig[key] = s.strip()
-    return _gitConfig[key]
+    return ustring(_gitConfig[key])
 
 def gitConfigBool(key):
     """Return a bool, using git config --bool.  It is True only if the

@@ -860,6 +881,7 @@ def branch_exists(branch):
     cmd = [ "git", "rev-parse", "--symbolic", "--verify", branch ]
     p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
     out, _ = p.communicate()
+    out = ustring(out)
     if p.returncode:
         return False
     # expect exactly one line of output: the branch name

gitgitgadget

From: Ben Keene <[email protected]>

---
 Documentation/git-p4.txt |  5 +++++
 git-p4.py                | 14 +++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

index 3494a1db3e..e206e69250 100644
@@ -547,6 +547,11 @@ git-p4.retries::
 	Set the value to 0 to disable retries or if your p4 version
 	does not support retries (pre 2012.2).
 
+git-p4.binary::
+	Specifies the p4 executable used by git-p4 to process commands.
+	The default value for Windows is `p4.exe` and for all other
+	systems the default is `p4`. 
+
 Clone and sync variables
 ~~~~~~~~~~~~~~~~~~~~~~~~
 git-p4.syncFromOrigin::

index 6e8b3a26cd..160d966ee1 100755
@@ -26,6 +26,8 @@
 import zlib
 import ctypes
 import errno
+import os.path
+from os import path
 
 # support basestring in python3
 try:

@@ -85,7 +87,17 @@ def p4_build_cmd(cmd):
     location. It means that hooking into the environment, or other configuration
     can be done more easily.
     """
-    real_cmd = ["p4"]
+    # Look for the P4 binary
+    p4bin = gitConfig("git-p4.binary")
+    real_cmd = []
+    if p4bin != "":
+        if path.exists(p4bin):
+            real_cmd = [p4bin]
+    if real_cmd == []:
+        if (platform.system() == "Windows"):
+            real_cmd = ["p4.exe"]    
+        else:
+            real_cmd = ["p4"]
 
     user = gitConfig("git-p4.user")
     if len(user) > 0:

gitgitgadget

I do not use Windows nor git-p4, but the above two changes make sense to me at the design level. One thing that needs to be updated is the configuration variable, though. It is more in line with the other parts of the system to name this "git-p4.program", because binary-ness does not matter much to you, as long as the named thing works correctly as "p4" program replacement.

Seeing "gpg.program" is used in a similar way, it also is tempting to call it "p4.program", but no other parts of Git other than "git-p4" uses P4, and the "git-p4." prefix is to collect variables related to "git-p4" together, so calling it "p4.program" may not be a good idea---it would hurt discoverability. "git-p4.p4program" may be OK, if we anticipate that git-p4 may need to use (and its users need to specify paths to) external programs other than "p4", but it probably is overkill.

Thanks.

There's some trailing whitespace in 98bae, can we get that tidied up?

Otherwise, modulo Junio's comments, it looks good.

I agree that "git-p4.binary" might be harder to discover ("Oh, I assumed that enabled binary mode!"). p4program should be fine.

I have updated the Pull Request to change the variable name from 'binary' to 'p4program'. I have also updated the PR description.

Here's the new commit message:

Issue: Using git-p4.py on Windows does not resolve properly to the p4.exe binary in all instances.

Changes since v1:
Commit: (dc6817e) 2019-11-14

Renamed the variable "git-p4.binary" to "git-p4.p4program"

v1:

Two new code features are added to resolve the p4 executable location:

A new variable, git-p4.binary, has been added that takes precedence over the default
p4 executable name. If this git option is set and the path.exists() passes for this file
it will be used as executable for the system.popen calls.

If the new variable git-p4.binary is not set, the program checks if the operating system
is Windows. If it is, the executable is changed to 'p4.exe'. All other operating systems
(those that do not report 'Windows' in the platform.system() call) continue to use the
current executable of 'p4'.

I couldn't figure out how to download this. I tried p4-binary-1, but it gave me the old one. Maybe I was just using it wrong?

Could you update the commit message to say why we needed to do this; if we have to dig through history in the future to figure out what's going on, it will be a lot easier if the explanation is in the commit message, rather than in an email thread. The branch description on gitgitgadget won't be around either.

Issue: Using git-p4.py on Windows does not resolve properly to the p4.exe binary in all instances.

Changes since v1: Commit: (dc6817e) 2019-11-14

Renamed the variable "git-p4.binary" to "git-p4.p4program" per the thread discussion.

v1:

Two new code features are added to resolve the p4 executable location:

1. A new variable, git-p4.binary, has been added that takes precedence over the default p4 executable name. If this git option is set and the path.exists() passes for this file it will be used as executable for the system.popen calls.

2. If the new variable git-p4.binary is not set, the program checks if the operating system is Windows. If it is, the executable is changed to 'p4.exe'. All other operating systems
(those that do not report 'Windows' in the platform.system() call) continue to use the current executable of 'p4'.

Ben Keene (3):
Cast byte strings to unicode strings in python3
Added general variable git-p4.binary and added a default for windows

    of 'P4.EXE'

Changed the name of the parameter from git-p4.binary to
git-p4.p4program

 Documentation/git-p4.txt |  5 +++++
 git-p4.py                | 40 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 3 deletions(-)

base-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-465%2Fseraphire%2Fseraphire%2Fp4-binary-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-465/seraphire/seraphire/p4-binary-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/465

Range-diff vs v1:

 1:  0bca930ff8 = 1:  0bca930ff8 Cast byte strings to unicode strings in python3
 2:  98bae92fda = 2:  98bae92fda Added general variable git-p4.binary and added a default for windows of 'P4.EXE'
 -:  ---------- > 3:  dc6817eea3 Changed the name of the parameter from git-p4.binary to git-p4.p4program

gitgitgadget

From: Ben Keene <[email protected]>

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

index 60c73b6a37..6e8b3a26cd 100755
@@ -36,12 +36,22 @@
     unicode = str
     bytes = bytes
     basestring = (str,bytes)
+    isunicode = True
+    def ustring(text):
+        """Returns the byte string as a unicode string"""
+        if text == '' or text == b'':
+            return ''
+        return unicode(text, "utf-8")
 else:
     # 'unicode' exists, must be Python 2
     str = str
     unicode = unicode
     bytes = str
     basestring = basestring
+    isunicode = False
+    def ustring(text):
+        """Returns the byte string unchanged"""
+        return text
 
 try:
     from subprocess import CalledProcessError

@@ -196,6 +206,8 @@ def read_pipe_full(c):
     expand = isinstance(c,basestring)
     p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=expand)
     (out, err) = p.communicate()
+    out = ustring(out)
+    err = ustring(err)
     return (p.returncode, out, err)
 
 def read_pipe(c, ignore_error=False):

@@ -263,6 +275,7 @@ def p4_has_move_command():
     cmd = p4_build_cmd(["move", "-k", "@from", "@to"])
     p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
     (out, err) = p.communicate()
+    err = ustring(err)
     # return code will be 1 in either case
     if err.find("Invalid option") >= 0:
         return False

@@ -646,10 +659,18 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
             if skip_info:
                 if 'code' in entry and entry['code'] == 'info':
                     continue
+                if b'code' in entry and entry[b'code'] == b'info':
+                    continue
             if cb is not None:
                 cb(entry)
             else:
-                result.append(entry)
+                if isunicode:
+                    out = {}
+                    for key, value in entry.items():
+                        out[ustring(key)] = ustring(value)
+                    result.append(out)
+                else:
+                    result.append(entry)
     except EOFError:
         pass
     exitCode = p4.wait()

@@ -792,7 +813,7 @@ def gitConfig(key, typeSpecifier=None):
         cmd += [ key ]
         s = read_pipe(cmd, ignore_error=True)
         _gitConfig[key] = s.strip()
-    return _gitConfig[key]
+    return ustring(_gitConfig[key])
 
 def gitConfigBool(key):
     """Return a bool, using git config --bool.  It is True only if the

@@ -860,6 +881,7 @@ def branch_exists(branch):
     cmd = [ "git", "rev-parse", "--symbolic", "--verify", branch ]
     p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
     out, _ = p.communicate()
+    out = ustring(out)
     if p.returncode:
         return False
     # expect exactly one line of output: the branch name

gitgitgadget

That is "how" this patch tries to achieve something that is not explained in this proposed log message. Worse, the rest of the proposed log message is entirely empty and does not help readers understand what the author wanted to do, or decide if applying these patches is a good idea.

You wrote something about Python 3 having issues while comparing the strings read out of process.popen() against string literals in the code elsewhere in the cover letter. That information is probably very relevant to explain why this commit exists, i.e. it should be explained here.

Here is what I _think_ is close to why you wrote this patch, but as I already said, I am far from being familiar with Py2to3 differences, so there may be factual misunderstanding you would want to fix, even if you decide to pick some pieces out of it when sending an updated patch.

Subject: [PATCH] git-p4: compare "p4" output with the right string type

Communicating with a subprocess yields a byte string in both Python 2 and Python 3. Comparing the output or the error string with string literals makes the code fail, as string literals in Python 3 are unicode strings and not byte strings.

Introduce a ustring() helper, which returns the corresponding unicode string for a given byte string (or return the argument as-is, when a unicode string is given) in Python 3, but returns the given bytestring as-is in Python 2. Use it to turn strings obtained from subprocess to the type suitable to compare with the string literals in both versions of Python.

What does the name "ustring()" stand for?

What purpose does the function serve at the higher conceptual level? Name the function after the answer to that question and the code would become easier to explain.

It certainly is not "make sure we have unicode string", as the name is shared between Python 2 and Python 3, and in the older Python 2 world, you want the helper to mean "keep the string a bytestring", not turning it into a unicode string. So I doubt ustring() is a great name for this helper.

Oops, I did not even realize read_pipe() is already our own. So there is no need to any wrapper around it, if we were to declare that all strings we use are unicode under Python 3---we just need to have the ustring() call (after giviing the helper a better name) directly inside read_pipe() function.

From: Ben Keene <[email protected]>

---
 Documentation/git-p4.txt |  5 +++++
 git-p4.py                | 14 +++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

index 3494a1db3e..e206e69250 100644
@@ -547,6 +547,11 @@ git-p4.retries::
 	Set the value to 0 to disable retries or if your p4 version
 	does not support retries (pre 2012.2).
 
+git-p4.binary::
+	Specifies the p4 executable used by git-p4 to process commands.
+	The default value for Windows is `p4.exe` and for all other
+	systems the default is `p4`. 
+
 Clone and sync variables
 ~~~~~~~~~~~~~~~~~~~~~~~~
 git-p4.syncFromOrigin::

index 6e8b3a26cd..160d966ee1 100755
@@ -26,6 +26,8 @@
 import zlib
 import ctypes
 import errno
+import os.path
+from os import path
 
 # support basestring in python3
 try:

@@ -85,7 +87,17 @@ def p4_build_cmd(cmd):
     location. It means that hooking into the environment, or other configuration
     can be done more easily.
     """
-    real_cmd = ["p4"]
+    # Look for the P4 binary
+    p4bin = gitConfig("git-p4.binary")
+    real_cmd = []
+    if p4bin != "":
+        if path.exists(p4bin):
+            real_cmd = [p4bin]
+    if real_cmd == []:
+        if (platform.system() == "Windows"):
+            real_cmd = ["p4.exe"]    
+        else:
+            real_cmd = ["p4"]
 
     user = gitConfig("git-p4.user")
     if len(user) > 0:

gitgitgadget

Again, this is "what" you did, without explaining "why". You give readers no hint to help them judge if/why it is a good idea to apply this patch.

Perhaps

Subject: [PATCH] git-p4: allow path to the "p4" program configurable

Introduce "git-p4.p4program" configuration variable that can be used to explicitly tell the path to the perforce client program. When unspecified, this defaults to "p4" except for Windows where "p4.exe" is used instead.

or something? This is still weaker than ideal as an explanation of "why", but better than not saying anything as your original did ;-)

The reason why I find it weaker than ideal is because I would expect any P4 user to have their system set up so that they can say "p4" and get "p4" without configuring anything specifically (iow, "%PATH%" would have been set up to have something with the p4.exe you want), and this change is to help those users, to whom that expectation does not hold (and in that case, it is better to explain in the proposed log message why their system would not let them run Perforce without first configuring this variable).

Again, as I said, you do not want to introduce "git-p4.binary" in 2/3 and then turn around to "oops, that was a mistake and it was not a good name, so let's rename it" in 3/3. Pretend as if you are a perfect developer with 100% foresight and have chosen the right name from the get-go by using "git-p4.p4program" in this step and drop patch 3/3.

Thanks.

From: Ben Keene <[email protected]>

---
 Documentation/git-p4.txt | 2 +-
 git-p4.py                | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index e206e69250..21ef100c28 100644
@@ -547,7 +547,7 @@ git-p4.retries::
 	Set the value to 0 to disable retries or if your p4 version
 	does not support retries (pre 2012.2).
 
-git-p4.binary::
+git-p4.p4program::
 	Specifies the p4 executable used by git-p4 to process commands.
 	The default value for Windows is `p4.exe` and for all other
 	systems the default is `p4`. 

index 160d966ee1..4fbb7344f1 100755
@@ -88,7 +88,7 @@ def p4_build_cmd(cmd):
     can be done more easily.
     """
     # Look for the P4 binary
-    p4bin = gitConfig("git-p4.binary")
+    p4bin = gitConfig("git-p4.p4program")
     real_cmd = []
     if p4bin != "":
         if path.exists(p4bin):

gitgitgadget

That's rather poor organization. In the larger picture, nobody wants to even know jthat you used to call git-p4.p4program with a different name or what that different name was. They do not even need to know why the new name is an improvement (but you do not even discuss to justify the name change in the proposed log message of 3/3, which is even worse X-<).

When presenting your work to the public, Git lets you pretend to be a perfect human who never made mistake while preparing it and built the series as a logical progression with perfect foresight. That is how "git rebase -i" is useful. Learn to take advantage of that ;-)

As to individual patches, our local (read: project specific) convention around here is to state the are the patch touches (in the case of these patches, "git-p4" is the appropriate area name), colon, and then a one-line summary of what the step is about (the last one is done without initial capitalization). The summary is written with the focus more on why and what than how.

Thanks.

Sorry for a late typofix. "to state the AREA the patch touches" was what I meant. So I would expect all patches to this file (which is the only component of 'git-p4' subsystem) to begin with "git-p4: " prefix.

Thanks.

I was having email trouble trying to reply to your suggestions. (Downloaded yet another mail client, and this seems to work)  I'll make those changes.