[v4,01/11] git-p4: select p4 binary by operating-system
diff mbox series

Message ID 40124269933691796ef57fd8df50f9e740d103b1.1575498577.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • git-p4.py: Cast byte strings to unicode strings in python3
Related show

Commit Message

Matthew Rogers via GitGitGadget Dec. 4, 2019, 10:29 p.m. UTC
From: Ben Keene <seraphire@gmail.com>

Depending on the version of GIT and Python installed, the perforce program (p4) may not resolve on Windows without the program extension.

Check the operating system (platform.system) and if it is reporting that it is Windows, use the full filename of "p4.exe" instead of "p4"

The original code unconditionally used "p4" as the binary filename.

This change is Python2 and Python3 compatible.

Thanks to: Junio C Hamano <gitster@pobox.com> and  Denton Liu <liu.denton@gmail.com> for patiently explaining proper format for my submissions.

Signed-off-by: Ben Keene <seraphire@gmail.com>
(cherry picked from commit 9a3a5c4e6d29dbef670072a9605c7a82b3729434)
---
 git-p4.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Denton Liu Dec. 5, 2019, 10:19 a.m. UTC | #1
Hi Ben,

First of all, as a note to you and possibly others, I don't have much
(read: any) experience with git-p4. I do have experience with Python and
how git.git generally does things so I'll be reviewing from that
perspective.

On Wed, Dec 04, 2019 at 10:29:27PM +0000, Ben Keene via GitGitGadget wrote:
> From: Ben Keene <seraphire@gmail.com>
> 
> Depending on the version of GIT and Python installed, the perforce program (p4) may not resolve on Windows without the program extension.

Nit: "GIT" should be written as "Git" when referring to the whole
project and "git" when referring to the command. Never in all-caps.

Also, please wrap your paragraphs at 72 characters. I'll say it once
here but it applies to your whole series.

> 
> Check the operating system (platform.system) and if it is reporting that it is Windows, use the full filename of "p4.exe" instead of "p4"
> 
> The original code unconditionally used "p4" as the binary filename.

As a rule of thumb, we want to state the problem first before we state
what we did (and why). I'd move this paragraph up.

> 
> This change is Python2 and Python3 compatible.
> 
> Thanks to: Junio C Hamano <gitster@pobox.com> and  Denton Liu <liu.denton@gmail.com> for patiently explaining proper format for my submissions.

I appreciate the credit but I don't think it's necessary. At _most_, you
could include the

	Helped-by: Junio C Hamano <gitster@pobox.com>
	Helped-by: Denton Liu <liu.denton@gmail.com>

tags before your signoff but I don't think we've done anything to
warrant it.

> 
> Signed-off-by: Ben Keene <seraphire@gmail.com>
> (cherry picked from commit 9a3a5c4e6d29dbef670072a9605c7a82b3729434)

You should remove this line in all of your commits. The referenced
commit isn't public so the information isn't very useful. Also, try to
not include anything after your signoff so if this hypothetically were
useful information, you'd include it before your signoff.

If it's information that's ephemerally useful for current reviewers but
not for future readers of your commit in the log message, you can
include it after the three hyphens...

> ---
like this and it won't be included as part of the log message.

>  git-p4.py | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index 60c73b6a37..b2ffbc057b 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -75,7 +75,11 @@ 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

I don't think this comment is necessary as the code itself is pretty
self-explanatory.

> +    if (platform.system() == "Windows"):
> +        real_cmd = ["p4.exe"]    

You have trailing whitespace here. Try to run `git diff --check` before
committing to ensure that you have no whitespace errors.

Thanks,

Denton

> +    else:
> +        real_cmd = ["p4"]
>  
>      user = gitConfig("git-p4.user")
>      if len(user) > 0:
> -- 
> gitgitgadget
>
Ben Keene Dec. 5, 2019, 4:32 p.m. UTC | #2
On 12/5/2019 5:19 AM, Denton Liu wrote:
> Hi Ben,
>
> First of all, as a note to you and possibly others, I don't have much
> (read: any) experience with git-p4. I do have experience with Python and
> how git.git generally does things so I'll be reviewing from that
> perspective.
>
> On Wed, Dec 04, 2019 at 10:29:27PM +0000, Ben Keene via GitGitGadget wrote:
>> From: Ben Keene <seraphire@gmail.com>
>>
>> Depending on the version of GIT and Python installed, the perforce program (p4) may not resolve on Windows without the program extension.
> Nit: "GIT" should be written as "Git" when referring to the whole
> project and "git" when referring to the command. Never in all-caps.
>
> Also, please wrap your paragraphs at 72 characters. I'll say it once
> here but it applies to your whole series.


Got it. I'll update all my commit messages to fit within this space.  I 
didn't realize
they didn't word wrap properly. (I'm using a GUI tool to manage this.)


>> Check the operating system (platform.system) and if it is reporting that it is Windows, use the full filename of "p4.exe" instead of "p4"
>>
>> The original code unconditionally used "p4" as the binary filename.
> As a rule of thumb, we want to state the problem first before we state
> what we did (and why). I'd move this paragraph up.
>
>> This change is Python2 and Python3 compatible.
>>
>> Thanks to: Junio C Hamano <gitster@pobox.com> and  Denton Liu <liu.denton@gmail.com> for patiently explaining proper format for my submissions.
> I appreciate the credit but I don't think it's necessary. At _most_, you
> could include the
>
> 	Helped-by: Junio C Hamano <gitster@pobox.com>
> 	Helped-by: Denton Liu <liu.denton@gmail.com>
>
> tags before your signoff but I don't think we've done anything to
> warrant it.


Thank you, I'll keep that in mind for the next submission!


>> Signed-off-by: Ben Keene <seraphire@gmail.com>
>> (cherry picked from commit 9a3a5c4e6d29dbef670072a9605c7a82b3729434)
> You should remove this line in all of your commits. The referenced
> commit isn't public so the information isn't very useful. Also, try to
> not include anything after your signoff so if this hypothetically were
> useful information, you'd include it before your signoff.
>
> If it's information that's ephemerally useful for current reviewers but
> not for future readers of your commit in the log message, you can
> include it after the three hyphens...


I'll look to pull these out before I update my submission.


>> ---
> like this and it won't be included as part of the log message.
>
>>   git-p4.py | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 60c73b6a37..b2ffbc057b 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -75,7 +75,11 @@ 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
> I don't think this comment is necessary as the code itself is pretty
> self-explanatory.
>
>> +    if (platform.system() == "Windows"):
>> +        real_cmd = ["p4.exe"]
> You have trailing whitespace here. Try to run `git diff --check` before
> committing to ensure that you have no whitespace errors.
>
> Thanks,
>
> Denton
>
>> +    else:
>> +        real_cmd = ["p4"]
>>   
>>       user = gitConfig("git-p4.user")
>>       if len(user) > 0:
>> -- 
>> gitgitgadget
>>

Patch
diff mbox series

diff --git a/git-p4.py b/git-p4.py
index 60c73b6a37..b2ffbc057b 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -75,7 +75,11 @@  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
+    if (platform.system() == "Windows"):
+        real_cmd = ["p4.exe"]    
+    else:
+        real_cmd = ["p4"]
 
     user = gitConfig("git-p4.user")
     if len(user) > 0: