From: Ben Keene <seraphire@gmail.com>
To: Denton Liu <liu.denton@gmail.com>,
Ben Keene via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v4 01/11] git-p4: select p4 binary by operating-system
Date: Thu, 5 Dec 2019 11:32:19 -0500 [thread overview]
Message-ID: <d5f5bbc6-64aa-f2ce-6975-7ba7d5e90154@gmail.com> (raw)
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 <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
>>
next prev parent reply other threads:[~2019-12-05 16:32 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-13 21:07 [PATCH 0/1] git-p4.py: Cast byte strings to unicode strings in python3 Ben Keene via GitGitGadget
2019-11-13 21:07 ` [PATCH 1/1] " Ben Keene via GitGitGadget
2019-11-14 2:25 ` [PATCH 0/1] git-p4.py: " Junio C Hamano
2019-11-14 9:46 ` Luke Diamand
2019-11-15 14:39 ` [PATCH v2 0/3] " Ben Keene via GitGitGadget
2019-11-15 14:39 ` [PATCH v2 1/3] " Ben Keene via GitGitGadget
2019-11-15 14:39 ` [PATCH v2 2/3] FIX: cast as unicode fails when a value is already unicode Ben Keene via GitGitGadget
2019-11-15 14:39 ` [PATCH v2 3/3] FIX: wrap return for read_pipe_lines in ustring() and wrap GitLFS read of the pointer file in ustring() Ben Keene via GitGitGadget
2019-12-02 19:02 ` [PATCH v3 0/1] git-p4.py: Cast byte strings to unicode strings in python3 Ben Keene via GitGitGadget
2019-12-02 19:02 ` [PATCH v3 1/1] Python3 support for t9800 tests. Basic P4/Python3 support Ben Keene via GitGitGadget
2019-12-03 0:18 ` Denton Liu
2019-12-03 16:03 ` Ben Keene
2019-12-04 6:14 ` Denton Liu
2019-12-04 22:29 ` [PATCH v4 00/11] git-p4.py: Cast byte strings to unicode strings in python3 Ben Keene via GitGitGadget
2019-12-04 22:29 ` [PATCH v4 01/11] git-p4: select p4 binary by operating-system Ben Keene via GitGitGadget
2019-12-05 10:19 ` Denton Liu
2019-12-05 16:32 ` Ben Keene [this message]
2019-12-04 22:29 ` [PATCH v4 02/11] git-p4: change the expansion test from basestring to list Ben Keene via GitGitGadget
2019-12-05 10:27 ` Denton Liu
2019-12-05 17:05 ` Ben Keene
2019-12-04 22:29 ` [PATCH v4 03/11] git-p4: add new helper functions for python3 conversion Ben Keene via GitGitGadget
2019-12-05 10:40 ` Denton Liu
2019-12-05 18:42 ` Ben Keene
2019-12-04 22:29 ` [PATCH v4 04/11] git-p4: python3 syntax changes Ben Keene via GitGitGadget
2019-12-05 11:02 ` Denton Liu
2019-12-04 22:29 ` [PATCH v4 05/11] git-p4: Add new functions in preparation of usage Ben Keene via GitGitGadget
2019-12-05 10:50 ` Denton Liu
2019-12-05 19:23 ` Ben Keene
2019-12-04 22:29 ` [PATCH v4 06/11] git-p4: Fix assumed path separators to be more Windows friendly Ben Keene via GitGitGadget
2019-12-05 13:38 ` Junio C Hamano
2019-12-05 19:37 ` Ben Keene
2019-12-04 22:29 ` [PATCH v4 07/11] git-p4: Add a helper class for stream writing Ben Keene via GitGitGadget
2019-12-05 13:42 ` Junio C Hamano
2019-12-05 19:52 ` Ben Keene
2019-12-04 22:29 ` [PATCH v4 08/11] git-p4: p4CmdList - support Unicode encoding Ben Keene via GitGitGadget
2019-12-05 13:55 ` Junio C Hamano
2019-12-05 20:23 ` Ben Keene
2019-12-04 22:29 ` [PATCH v4 09/11] git-p4: Add usability enhancements Ben Keene via GitGitGadget
2019-12-05 14:04 ` Junio C Hamano
2019-12-05 15:40 ` Ben Keene
2019-12-04 22:29 ` [PATCH v4 10/11] git-p4: Support python3 for basic P4 clone, sync, and submit Ben Keene via GitGitGadget
2019-12-04 22:29 ` [PATCH v4 11/11] git-p4: Added --encoding parameter to p4 clone Ben Keene via GitGitGadget
2019-12-05 9:54 ` [PATCH v4 00/11] git-p4.py: Cast byte strings to unicode strings in python3 Luke Diamand
2019-12-05 16:16 ` Ben Keene
2019-12-05 18:51 ` Denton Liu
2019-12-05 20:47 ` Ben Keene
2019-12-07 17:47 ` [PATCH v5 00/15] " Ben Keene via GitGitGadget
2019-12-07 17:47 ` [PATCH v5 01/15] t/gitweb-lib.sh: drop confusing quotes Jeff King via GitGitGadget
2019-12-07 17:47 ` [PATCH v5 02/15] t/gitweb-lib.sh: set $REQUEST_URI Jeff King via GitGitGadget
2019-12-07 17:47 ` [PATCH v5 03/15] git-p4: select P4 binary by operating-system Ben Keene via GitGitGadget
2019-12-09 19:47 ` Junio C Hamano
2019-12-07 17:47 ` [PATCH v5 04/15] git-p4: change the expansion test from basestring to list Ben Keene via GitGitGadget
2019-12-09 20:25 ` Junio C Hamano
2019-12-13 14:40 ` Ben Keene
2019-12-07 17:47 ` [PATCH v5 05/15] git-p4: promote encodeWithUTF8() to a global function Ben Keene via GitGitGadget
2019-12-11 16:39 ` Junio C Hamano
2019-12-07 17:47 ` [PATCH v5 06/15] git-p4: remove p4_write_pipe() and write_pipe() return values Ben Keene via GitGitGadget
2019-12-07 17:47 ` [PATCH v5 07/15] git-p4: add new support function gitConfigSet() Ben Keene via GitGitGadget
2019-12-11 17:11 ` Junio C Hamano
2019-12-07 17:47 ` [PATCH v5 08/15] git-p4: add casting helper functions for python 3 conversion Ben Keene via GitGitGadget
2019-12-07 17:47 ` [PATCH v5 09/15] git-p4: python 3 syntax changes Ben Keene via GitGitGadget
2019-12-07 17:47 ` [PATCH v5 10/15] git-p4: fix assumed path separators to be more Windows friendly Ben Keene via GitGitGadget
2019-12-07 17:47 ` [PATCH v5 11/15] git-p4: add Py23File() - helper class for stream writing Ben Keene via GitGitGadget
2019-12-07 17:47 ` [PATCH v5 12/15] git-p4: p4CmdList - support Unicode encoding Ben Keene via GitGitGadget
2019-12-07 17:47 ` [PATCH v5 13/15] git-p4: support Python 3 for basic P4 clone, sync, and submit (t9800) Ben Keene via GitGitGadget
2019-12-07 17:47 ` [PATCH v5 14/15] git-p4: added --encoding parameter to p4 clone Ben Keene via GitGitGadget
2019-12-07 17:47 ` [PATCH v5 15/15] git-p4: Add depot manipulation functions Ben Keene via GitGitGadget
2019-12-07 19:47 ` [PATCH v5 00/15] git-p4.py: Cast byte strings to unicode strings in python3 Jeff King
2019-12-07 21:27 ` Ben Keene
2019-12-11 16:54 ` Junio C Hamano
2019-12-11 17:13 ` Denton Liu
2019-12-11 17:57 ` Junio C Hamano
2019-12-11 20:19 ` Luke Diamand
2019-12-11 21:46 ` Junio C Hamano
2019-12-11 22:30 ` Yang Zhao
2019-12-12 14:13 ` Ben Keene
2019-12-13 19:42 ` [PATCH v5 00/15] git-p4.py: Cast byte strings to unicode strings in python3 - Code Review Ben Keene
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d5f5bbc6-64aa-f2ce-6975-7ba7d5e90154@gmail.com \
--to=seraphire@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=liu.denton@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).