From: Ben Keene <seraphire@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
Ben Keene via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v4 06/11] git-p4: Fix assumed path separators to be more Windows friendly
Date: Thu, 5 Dec 2019 14:37:41 -0500 [thread overview]
Message-ID: <e41c1532-9e2e-ef6b-4c3d-af006a1f2ef9@gmail.com> (raw)
In-Reply-To: <xmqqsgly28qs.fsf@gitster-ct.c.googlers.com>
On 12/5/2019 8:38 AM, Junio C Hamano wrote:
> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Ben Keene <seraphire@gmail.com>
>>
>> When a computer is configured to use Git for windows and Python for windows, and not a Unix subsystem like cygwin or WSL, the directory separator changes and causes git-p4 to fail to properly determine paths.
>>
>> Fix 3 path separator errors:
>>
>> 1. getUserCacheFilename should not use string concatenation. Change this code to use os.path.join to build an OS tolerant path.
>> 2. defaultDestiantion used the OS.path.split to split depot paths. This is incorrect on windows. Change the code to split on a forward slash(/) instead since depot paths use this character regardless of the operating system.
>> 3. The call to isvalidGitDir() in the main code also used a literal forward slash. Change the cose to use os.path.join to correctly format the path for the operating system.
> s/isvalid/isValid/;
> s/cose/code/;
>
> Also please wrap your lines at around 72 columns (that will let
> reviewers quote what you write (which adds "> " prefix and consumes
> 2 more columns), and would allow us a handful of exchanges (each
> round adding ">" prefix to consume 1 more column) before bumping
> into the right edge of the terminal at 80 columns.
>
>> These three changes allow the suggested windows configuration to properly locate files while retaining the existing behavior on non-windows operating systems.
>>
>> Signed-off-by: Ben Keene <seraphire@gmail.com>
>> (cherry picked from commit a5b45c12c3861638a933b05a1ffee0c83978dcb2)
> As Denton mentioned, general public do not care if you "cherry
> picked" it from your earlier unpublished work. Remove it.
>
> Aside from these small nits, the proposed log message for this step
> is quite cleanly done and easily readable. All the decisions are
> clearly written and agreeable. Nicely done.
Thank you. I've been working through all the commits and updating them.
>> ---
>> git-p4.py | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 2659531c2e..7ac8cb42ef 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1454,8 +1454,10 @@ def p4UserIsMe(self, p4User):
>> return True
>>
>> def getUserCacheFilename(self):
>> + """ Returns the filename of the username cache
>> + """
> Inconsistent use of spaces and a tab I see on these two lines.
> Intended?
Good catch! It should have been spaces. Corrected.
>
>> home = os.environ.get("HOME", os.environ.get("USERPROFILE"))
>> - return home + "/.gitp4-usercache.txt"
>> + return os.path.join(home, ".gitp4-usercache.txt")
>>
>> def getUserMapFromPerforceServer(self):
>> if self.userMapFromPerforceServer:
>> @@ -3973,13 +3975,16 @@ def __init__(self):
>> self.cloneBare = False
>>
>> def defaultDestination(self, args):
>> + """ Returns the last path component as the default git
>> + repository directory name
>> + """
>> ## TODO: use common prefix of args?
>> depotPath = args[0]
>> depotDir = re.sub("(@[^@]*)$", "", depotPath)
>> depotDir = re.sub("(#[^#]*)$", "", depotDir)
>> depotDir = re.sub(r"\.\.\.$", "", depotDir)
>> depotDir = re.sub(r"/$", "", depotDir)
>> - return os.path.split(depotDir)[1]
>> + return depotDir.split('/')[-1]
>>
>> def run(self, args):
>> if len(args) < 1:
>> @@ -4252,8 +4257,8 @@ def main():
>> chdir(cdup);
>>
>> if not isValidGitDir(cmd.gitdir):
>> - if isValidGitDir(cmd.gitdir + "/.git"):
>> - cmd.gitdir += "/.git"
>> + if isValidGitDir(os.path.join(cmd.gitdir, ".git")):
>> + cmd.gitdir = os.path.join(cmd.gitdir, ".git")
>> else:
>> die("fatal: cannot locate git repository at %s" % cmd.gitdir)
next prev parent reply other threads:[~2019-12-05 19:37 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
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 [this message]
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=e41c1532-9e2e-ef6b-4c3d-af006a1f2ef9@gmail.com \
--to=seraphire@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.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).