[PATCH 0/2] Feature: New Variable git-p4.binary

[PATCH 0/2] Feature: New Variable git-p4.binary

[Ben Keene via GitGitGadget]

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

[PATCH 1/2] Cast byte strings to unicode strings in python3

[Ben Keene via GitGitGadget]

From: Ben Keene <bkeene@partswatch.com>

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 60c73b6a37..6e8b3a26cd 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -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

[PATCH 2/2] Added general variable git-p4.binary and added a default for windows of 'P4.EXE'

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 Documentation/git-p4.txt |  5 +++++
 git-p4.py                | 14 +++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 3494a1db3e..e206e69250 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -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::
diff --git a/git-p4.py b/git-p4.py
index 6e8b3a26cd..160d966ee1 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -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

Re: [PATCH 0/2] Feature: New Variable git-p4.binary

[Junio C Hamano]

"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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'.

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.

Re: [PATCH 0/2] Feature: New Variable git-p4.binary

[Luke Diamand]

On Thu, 14 Nov 2019 at 02:36, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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'.
>
> 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.

Re: [PATCH 0/2] Feature: New Variable git-p4.binary

[Ben Keene]

On Thu, Nov 14, 2019 at 4:52 AM Luke Diamand <luke@diamand.org> wrote:
>
> On Thu, 14 Nov 2019 at 02:36, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > 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'.
> >
> > 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'.

Re: [PATCH 0/2] Feature: New Variable git-p4.binary

[Luke Diamand]

On Thu, 14 Nov 2019 at 20:17, Ben Keene <seraphire@gmail.com> wrote:
>
> On Thu, Nov 14, 2019 at 4:52 AM Luke Diamand <luke@diamand.org> wrote:
> >
> > On Thu, 14 Nov 2019 at 02:36, Junio C Hamano <gitster@pobox.com> wrote:
> > >
> > > "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > >
> > > > 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'.
> > >
> > > 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.

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.

>
> 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'.

[PATCH v2 0/3] Feature: New Variable git-p4.p4program

[Ben Keene via GitGitGadget]

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

[PATCH v2 1/3] Cast byte strings to unicode strings in python3

[Ben Keene via GitGitGadget]

From: Ben Keene <bkeene@partswatch.com>

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 60c73b6a37..6e8b3a26cd 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -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

Re: [PATCH v2 1/3] Cast byte strings to unicode strings in python3

[Junio C Hamano]

"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ben Keene <bkeene@partswatch.com>
> Subject: Re: [PATCH v2 1/3] Cast byte strings to unicode strings in python3

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.

>              if skip_info:
>                  if 'code' in entry and entry['code'] == 'info':
>                      continue
> +                if b'code' in entry and entry[b'code'] == b'info':
> +                    continue

This one explicitly calls for 'bytes', which I think would work
correctly with Python 2.  But why would we have to prepare for both
variants existing in the entry[] hash (especially when working with
Python3)?  Shouldn't the code that populates entry[] be responsible
for turning a byte string into a unicode string?

Also, is subprocess.communicate the only way bytestrings can come to
"git-p4" program, or are there other avenues?  What I am wondering
is if it is sensible to declare that when running under Python 3,
the program will internally use unicode strings for everything and
convert any bytestring to unicode string as soon as it enters [*1*]

That would mean ...

>              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)

... a hunk like this would become entirely unnecessary, as keys and
values of entry[] that are read from outside world would have
already been made into unicode strings under Python 3.

By the way, would values in entry[] always some string (iow, can
they have a literal number like 47 and 3.14)?

>      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])

Likewise.  I'd expect, if we were to declare that our internal
strings are all unicode in Python 3, we'd be using a thin wrapper of
read_pipe() that yields a unicode string, so 's' would not be a
bytestring, and s.strip() would not be either.

>  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)

But perhaps it is a bit too much and it is easier to call the
conversion helper on the results obtained by read_pipe() that gives
us bytestring, like this one (and one above that) does?  I dunno.

Thanks.


[Footnote]

*1* If it is not just string comparison but the general direction
    were to consistently use unicode strings under Python 3, which
    I think makes sense at the conceptual level, the sample log
    message I prepared in the earlier part of this response needs to
    be updated, so that it is not so message/comparison centric.

    I said "at the conceptual level", because there may be practical
    reasons not to use unicode everywhere (e.g. performance might
    degrade and some bytestrings used may not be text data in the
    first place).

Re: [PATCH v2 1/3] Cast byte strings to unicode strings in python3

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>          cmd += [ key ]
>>          s = read_pipe(cmd, ignore_error=True)
>>          _gitConfig[key] = s.strip()
>> -    return _gitConfig[key]
>> +    return ustring(_gitConfig[key])
>
> Likewise.  I'd expect, if we were to declare that our internal
> strings are all unicode in Python 3, we'd be using a thin wrapper of
> read_pipe() that yields a unicode string, so 's' would not be a
> bytestring, and s.strip() would not be either.

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.

[PATCH v2 2/3] Added general variable git-p4.binary and added a default for windows of 'P4.EXE'

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 Documentation/git-p4.txt |  5 +++++
 git-p4.py                | 14 +++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 3494a1db3e..e206e69250 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -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::
diff --git a/git-p4.py b/git-p4.py
index 6e8b3a26cd..160d966ee1 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -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

Re: [PATCH v2 2/3] Added general variable git-p4.binary and added a default for windows of 'P4.EXE'

[Junio C Hamano]

"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ben Keene <seraphire@gmail.com>
> Subject: Re: [PATCH v2 2/3] Added general variable git-p4.binary and added a default for windows of 'P4.EXE'

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.

> Signed-off-by: Ben Keene <seraphire@gmail.com>
> ---
>  Documentation/git-p4.txt |  5 +++++
>  git-p4.py                | 14 +++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index 3494a1db3e..e206e69250 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -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::
> diff --git a/git-p4.py b/git-p4.py
> index 6e8b3a26cd..160d966ee1 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -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:

[PATCH v2 3/3] Changed the name of the parameter from git-p4.binary to git-p4.p4program

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 Documentation/git-p4.txt | 2 +-
 git-p4.py                | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index e206e69250..21ef100c28 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -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`. 
diff --git a/git-p4.py b/git-p4.py
index 160d966ee1..4fbb7344f1 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -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

Re: [PATCH v2 0/3] Feature: New Variable git-p4.p4program

[Junio C Hamano]

"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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

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.

Re: [PATCH v2 0/3] Feature: New Variable git-p4.p4program

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.


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.

Re: [PATCH v2 0/3] Feature: New Variable git-p4.p4program

[Ben Keene]

On 11/17/2019 8:15 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.
>
> 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.