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@xxxxxxxxx>
>
> 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@xxxxxxxxx> and Denton Liu <liu.denton@xxxxxxxxx> 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@xxxxxxxxx>
Helped-by: Denton Liu <liu.denton@xxxxxxxxx>
tags before your signoff but I don't think we've done anything to
warrant it.
>
> Signed-off-by: Ben Keene <seraphire@xxxxxxxxx>
> (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
>
