Re: [PATCH v4 06/11] git-p4: Fix assumed path separators to be more Windows friendly
On 12/5/2019 8:38 AM, Junio C Hamano wrote:
Show 30 quoted lines
> "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.
Show 16 quoted lines
>> ---
>> 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.
Show 35 quoted lines
>
>> 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)Ben Keene <seraphire@gmail.com>