Re: [PATCH] git p4: chdir resolves symlinks only for relative paths
Sorry for the late turnaround here is an improved version. Now chdir has an optional argument client_path, if it's true then we don't do os.getcwd. I think that my first patch is also valid too - when the path is absolute no need for getcwd no matter what is the context, when it's relative we have to use os.getcwd() no matter of the context.
--- If p4 client is configured to /p/foo which is a symlink: /p/foo -> /vol/barvol/projects/foo. Then resolving the symlink will confuse p4: "Path /vol/barvol/projects/foo/... is not under client root /p/foo". While AltRoots in p4 client specification can be used as a workaround on p4 side, git-p4 should not resolve symlinks in client paths. chdir(dir) uses os.getcwd() after os.chdir(dir) to resolve relative paths, but as a sideeffect it resolves symlinks too. Now for client paths we don't call os.getcwd().
Signed-off-by: Miklós Fazekas <mfazekas@szemafor.com>
---
git-p4.py | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
Show changes to git-p4.py +8 −3
diff --git a/git-p4.py b/git-p4.py
index 0682e61..2bd8cc2 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -68,12 +68,17 @@ def p4_build_cmd(cmd):
real_cmd += cmd
return real_cmd
-def chdir(dir):
+def chdir(dir,client_path=False):
# P4 uses the PWD environment variable rather than getcwd(). Since we're
# not using the shell, we have to set it ourselves. This path could
# be relative, so go there first, then figure out where we ended up.
+ # os.getcwd() will resolve symlinks, so we should avoid it for
+ # client_paths.
os.chdir(dir)
- os.environ['PWD'] = os.getcwd()
+ if client_path:
+ os.environ['PWD'] = dir
+ else:
+ os.environ['PWD'] = os.getcwd()
def die(msg):
if verbose:
@@ -1554,7 +1559,7 @@ class P4Submit(Command, P4UserMap):
new_client_dir = True
os.makedirs(self.clientPath)
- chdir(self.clientPath)
+ chdir(self.clientPath,client_path=True)
if self.dry_run:
print "Would synchronize p4 checkout in %s" % self.clientPath
else:
--
1.7.10.2 (Apple Git-33)
On Mon, Feb 4, 2013 at 12:08 AM, Pete Wyckoff <pw@padd.com> wrote:
> mfazekas@szemafor.com wrote on Tue, 29 Jan 2013 09:37 +0100:
>> If a p4 client is configured to /p/foo which is a symlink
>> to /vol/bar/projects/foo, then resolving symlink, which
>> is done by git-p4's chdir will confuse p4: "Path
>> /vol/bar/projects/foo/... is not under client root /p/foo"
>> While AltRoots in p4 client specification can be used as a
>> workaround on p4 side, git-p4 should not resolve symlinks
>> in client paths.
>> chdir(dir) uses os.getcwd() after os.chdir(dir) to resolve
>> relative paths, but as a side effect it resolves symlinks
>> too. Now it checks if the dir is relative before resolving.
>>
>> Signed-off-by: Miklós Fazekas <mfazekas@szemafor.com>
>> ---
>> git-p4.py | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 2da5649..5d74649 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -64,7 +64,10 @@ def chdir(dir):
>> # not using the shell, we have to set it ourselves. This path could
>> # be relative, so go there first, then figure out where we ended up.
>> os.chdir(dir)
>> - os.environ['PWD'] = os.getcwd()
>> + if os.path.isabs(dir):
>> + os.environ['PWD'] = dir
>> + else:
>> + os.environ['PWD'] = os.getcwd()
>>
>> def die(msg):
>> if verbose:
>
> Thanks, this is indeed a bug and I have reproduced it with a test
> case. Your patch works, but I think it would be better to
> separate the callers of chdir(): those that know they are
> cd-ing to a path from a p4 client, and everybody else. The former
> should not use os.getcwd(), as you show.
>
> I'll whip something up soon, unless you beat me to it.
>
> -- Pete
