From: Ben Keene Date: Thu, 05 Dec 2019 19:37:41 GMT Subject: Re: [PATCH v4 06/11] git-p4: Fix assumed path separators to be more Windows friendly Message-ID: In-Reply-To: On 12/5/2019 8:38 AM, Junio C Hamano wrote: > "Ben Keene via GitGitGadget" writes: > >> From: Ben Keene >> >> 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 >> (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)