Feature: New Variable git-p4.p4program #465
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
Welcome to GitGitGadgetHi @seraphire, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that this Pull Request has a good description, as it will be used as cover letter. Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions. If you want to see what email(s) would be sent for a submit request, add a PR comment If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via: curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt |
seraphire
force-pushed
the
seraphire/p4-binary
branch
from
55ccc47
to
98bae92
Compare
|
/preview |
|
Preview email sent as pull.465.git.1573679500.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.465.git.1573679665.gitgitgadget@gmail.com |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Luke Diamand wrote (reply to this): |
|
On Wed, Nov 13, 2019 at 9:40 PM gitgitgadget[bot] ***@***.***> wrote:
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Ben Keene via GitGitGadget" ***@***.***> 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.
I agree with "git-p4.p4program". the git-p4 is the current INI
section used by p4, so while it may be overkill, it does make it more
self-documenting and self-discoverable.
Since I submitted my patch through gitgitgadet, what is the preferred
mechanism for me to submit this change? Should I just commit it to my
existing branch or do will I need to open a new pull request?
Thank you.
|
|
Renamed the variable "git-p4.binary" to "git-p4.p4program" |
|
@seraphire sorry, GitGitGadget is not fully bidirectional (yet). You will need to follow the directions given via the (reply to this) link to reply to the mail(s) on the Git mailing list. |
The convention in GitGitGadget is to edit the PR description by adding a "Changes since v" section, see e.g. #170. |
|
On Thu, Nov 14, 2019 at 9:14 AM Ben Keene ***@***.***> wrote:
On Wed, Nov 13, 2019 at 9:40 PM gitgitgadget[bot]
***@***.***> wrote:
>
> On the Git mailing list, Junio C Hamano wrote (reply to this):
>
> "Ben Keene via GitGitGadget" ***@***.***> 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.
>
I agree with "git-p4.p4program". the git-p4 is the current INI
section used by p4, so while it may be overkill, it does make it more
self-documenting and self-discoverable.
Since I submitted my patch through gitgitgadet, what is the preferred
mechanism for me to submit this change? Should I just commit it to my
existing branch or do will I need to open a new pull request?
Thank you.
This change has been submitted and the PR description has been
updated. to include the new changes.
|
|
On the Git mailing list, Ben Keene wrote (reply to this): |
seraphire
changed the title
|
On the Git mailing list, Luke Diamand wrote (reply to this): |
|
/submit |
|
Submitted as pull.465.v2.git.1573828978.gitgitgadget@gmail.com |
![gitgitgadget[bot]](https://github.com/avatars/in/12836?s=60&v=4){kind=small}
git-p4.py
Outdated
| @@ -36,12 +36,22 @@ | |||
| unicode = str | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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).
There 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:
>> 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.
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
Documentation/git-p4.txt
Outdated
| @@ -547,6 +547,11 @@ git-p4.retries:: | |||
| Set the value to 0 to disable retries or if your p4 version | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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:
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On Sun, Nov 17, 2019 at 8:19 PM gitgitgadget[bot] ***@***.***> wrote:
On the Git mailing list, Junio C Hamano wrote (reply to this):
Junio C Hamano ***@***.***> 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.
Thanks Junio – I have taken a step back and I’m looking at my code
again. I’ll resubmit it after I get it in better shape and I’ll make
the changes to my commits as you suggest.
Thank you!
|
|
@seraphire Junio cannot see your comment at git-for-windows/build-extra@eeee74cfe238#r36027386; GitGitGadget still is not bidirectional. |
@seraphire read: please reply via mail instead. |
seraphire
force-pushed
the
seraphire/p4-binary
branch
2 times, most recently
from
98bae92
to
0bca930
Compare
seraphire
force-pushed
the
seraphire/p4-binary
branch
from
0bca930
to
d9f6f3b
Compare
|
On the Git mailing list, Ben Keene wrote (reply to this): |
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.
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:
A new variable,
git-p4.binary, has been added that takes precedence over the defaultp4 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.popencalls.If the new variable
git-p4.binaryis not set, the program checks if the operating systemis 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 thecurrent executable of 'p4'.