| Message ID | 40124269933691796ef57fd8df50f9e740d103b1.1575498577.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-p4.py: Cast byte strings to unicode strings in python3 | expand |
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 >
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 >>
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: