Re: [PATCH] git p4: chdir resolves symlinks only for relative paths

["Miklós Fazekas" <mfazekas@szemafor.com>]

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(-)

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