Re: [PATCH v4 01/11] git-p4: select p4 binary by operating-system

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:

quoted

From: Ben Keene <redacted>

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

quoted

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.

quoted

This change is Python2 and Python3 compatible.

Thanks to: Junio C Hamano [off-list ref] and  Denton Liu [off-list ref] 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 [off-list ref]
	Helped-by: Denton Liu [off-list ref]

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!

quoted

Signed-off-by: Ben Keene <redacted>
(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.

quoted

---

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

quoted

  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.

quoted

+    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

quoted

+    else:
+        real_cmd = ["p4"]
  
      user = gitConfig("git-p4.user")
      if len(user) > 0:
-- 
gitgitgadget