From: Ben Keene Date: Thu, 05 Dec 2019 16:32:19 GMT Subject: Re: [PATCH v4 01/11] git-p4: select p4 binary by operating-system Message-ID: In-Reply-To: <20191205101935.GA315203@generichostname> 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 >> >> 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 and Denton Liu 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 > Helped-by: Denton Liu > > 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 >> (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 >>