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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 16, 2019, 02:50 UTC
Message-ID: <xmqq8sog5ycj.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <98bae92fda9ca01d01b2e9fb70b04b00470e7bec.1573828978.git.gitgitgadget@gmail.com>

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

Show 54 quoted lines

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