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

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

[Miklós Fazekas]

[resending as plain text]

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:
-- 
1.7.10.2 (Apple Git-33)

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

[Pete Wyckoff]

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

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

[Miklós Fazekas]

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

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

[John Keeping]

On Thu, Mar 07, 2013 at 09:36:06AM +0100, Miklós Fazekas wrote:
> 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):

Style (space after comma):

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

Indentation seems to have gone a bit wrong here...

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

Again, there should be a space after the comma here.

>          if self.dry_run:
>              print "Would synchronize p4 checkout in %s" % self.clientPath
>          else:
> -- 
> 1.7.10.2 (Apple Git-33)

[PATCH 0/3] fix git-p4 client root symlink problems

[Pete Wyckoff]

Miklós pointed out in

    http://thread.gmane.org/gmane.comp.version-control.git/214915

that when the p4 client root included a symlink, bad things
happen.  It is fixable, but inconvenient, to use an absolute path
in one's p4 client.  It's not too hard to be smarter about this
in git-p4.

Thanks to Miklós for the patch, and to John for the style
suggestions.  I wrote a couple of tests to make sure this part
doesn't break again.

This is maybe a bug introduced by bf1d68f (git-p4: use absolute
directory for PWD env var, 2011-12-09), but that's so long ago
that I don't think this is a candidate for maint.

		-- Pete

Miklós Fazekas (1):
  git p4: avoid expanding client paths in chdir

Pete Wyckoff (2):
  git p4 test: make sure P4CONFIG relative path works
  git p4 test: should honor symlink in p4 client root

 git-p4.py               | 29 ++++++++++++++++++++++-------
 t/t9808-git-p4-chdir.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 7 deletions(-)

-- 
1.8.2.rc2.64.g8335025

[PATCH 1/3] git p4 test: make sure P4CONFIG relative path works

[Pete Wyckoff]

This adds a test for the fix in bf1d68f (git-p4: use absolute
directory for PWD env var, 2011-12-09).  It is necessary to
set PWD to an absolute path so that p4 can find files referenced
by non-absolute paths, like the value of the P4CONFIG environment
variable.

P4 does not open files directly; it builds a path by prepending
the contents of the PWD environment variable.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9808-git-p4-chdir.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index dc92e60..55c5e36 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -42,6 +42,20 @@ test_expect_success 'P4CONFIG and relative dir clone' '
 	)
 '
 
+# Common setup using .p4config to set P4CLIENT and P4PORT breaks
+# if clone destination is relative.  Make sure that chdir() expands
+# the relative path in --dest to absolute.
+test_expect_success 'p4 client root would be relative due to clone --dest' '
+	test_when_finished cleanup_git &&
+	(
+		echo P4PORT=$P4PORT >git/.p4config &&
+		P4CONFIG=.p4config &&
+		export P4CONFIG &&
+		unset P4PORT &&
+		git p4 clone --dest="git" //depot
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.8.2.rc2.64.g8335025

[PATCH 2/3] git p4 test: should honor symlink in p4 client root

[Pete Wyckoff]

This test fails when the p4 client root includes
a symlink.  It complains:

    Path /vol/bar/projects/foo/... is not under client root /p/foo

and dumps a traceback.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9808-git-p4-chdir.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index 55c5e36..af8bd8a 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -56,6 +56,33 @@ test_expect_success 'p4 client root would be relative due to clone --dest' '
 	)
 '
 
+# When the p4 client Root is a symlink, make sure chdir() does not use
+# getcwd() to convert it to a physical path.
+test_expect_failure 'p4 client root symlink should stay symbolic' '
+	physical="$TRASH_DIRECTORY/physical" &&
+	symbolic="$TRASH_DIRECTORY/symbolic" &&
+	test_when_finished "rm -rf \"$physical\"" &&
+	test_when_finished "rm \"$symbolic\"" &&
+	mkdir -p "$physical" &&
+	ln -s "$physical" "$symbolic" &&
+	test_when_finished cleanup_git &&
+	(
+		P4CLIENT=client-sym &&
+		p4 client -i <<-EOF &&
+		Client: $P4CLIENT
+		Description: $P4CLIENT
+		Root: $symbolic
+		LineEnd: unix
+		View: //depot/... //$P4CLIENT/...
+		EOF
+		git p4 clone --dest="$git" //depot &&
+		cd "$git" &&
+		test_commit file2 &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.8.2.rc2.64.g8335025

[PATCH 3/3] git p4: avoid expanding client paths in chdir

[Pete Wyckoff]

From: Miklós Fazekas <mfazekas@szemafor.com>

The generic chdir() helper sets the PWD environment
variable, as that is what is used by p4 to know its
current working directory.  Normally the shell would
do this, but in git-p4, we must do it by hand.

However, when the path contains a symbolic link,
os.getcwd() will return the physical location.  If the
p4 client specification includes symlinks, setting PWD
to the physical location causes p4 to think it is not
inside the client workspace.  It complains, e.g.

    Path /vol/bar/projects/foo/... is not under client root /p/foo

One workaround is to use AltRoots in the p4 client specification,
but it is cleaner to handle it directly in git-p4.

Other uses of chdir still require setting PWD to an
absolute path so p4 features like P4CONFIG work.  See
bf1d68f (git-p4: use absolute directory for PWD env
var, 2011-12-09).

[ pw: tweak patch and commit message ]

Thanks-to: John Keeping <john@keeping.me.uk>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py               | 29 ++++++++++++++++++++++-------
 t/t9808-git-p4-chdir.sh |  2 +-
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 647f110..7288c0b 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -79,12 +79,27 @@ def p4_build_cmd(cmd):
         real_cmd += cmd
     return real_cmd
 
-def chdir(dir):
-    # 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.chdir(dir)
-    os.environ['PWD'] = os.getcwd()
+def chdir(path, is_client_path=False):
+    """Do chdir to the given path, and set the PWD environment
+       variable for use by P4.  It does not look at getcwd() output.
+       Since we're not using the shell, it is necessary to set the
+       PWD environment variable explicitly.
+       
+       Normally, expand the path to force it to be absolute.  This
+       addresses the use of relative path names inside P4 settings,
+       e.g. P4CONFIG=.p4config.  P4 does not simply open the filename
+       as given; it looks for .p4config using PWD.
+
+       If is_client_path, the path was handed to us directly by p4,
+       and may be a symbolic link.  Do not call os.getcwd() in this
+       case, because it will cause p4 to think that PWD is not inside
+       the client path.
+       """
+
+    os.chdir(path)
+    if not is_client_path:
+        path = os.getcwd()
+    os.environ['PWD'] = path
 
 def die(msg):
     if verbose:
@@ -1624,7 +1639,7 @@ class P4Submit(Command, P4UserMap):
             new_client_dir = True
             os.makedirs(self.clientPath)
 
-        chdir(self.clientPath)
+        chdir(self.clientPath, is_client_path=True)
         if self.dry_run:
             print "Would synchronize p4 checkout in %s" % self.clientPath
         else:
diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index af8bd8a..09b2cc4 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -58,7 +58,7 @@ test_expect_success 'p4 client root would be relative due to clone --dest' '
 
 # When the p4 client Root is a symlink, make sure chdir() does not use
 # getcwd() to convert it to a physical path.
-test_expect_failure 'p4 client root symlink should stay symbolic' '
+test_expect_success 'p4 client root symlink should stay symbolic' '
 	physical="$TRASH_DIRECTORY/physical" &&
 	symbolic="$TRASH_DIRECTORY/symbolic" &&
 	test_when_finished "rm -rf \"$physical\"" &&
-- 
1.8.2.rc2.64.g8335025

Re: [PATCH 2/3] git p4 test: should honor symlink in p4 client root

[Johannes Sixt]

Am 3/8/2013 0:19, schrieb Pete Wyckoff:
> +# When the p4 client Root is a symlink, make sure chdir() does not use
> +# getcwd() to convert it to a physical path.
> +test_expect_failure 'p4 client root symlink should stay symbolic' '
> +	physical="$TRASH_DIRECTORY/physical" &&
> +	symbolic="$TRASH_DIRECTORY/symbolic" &&
> +	test_when_finished "rm -rf \"$physical\"" &&
> +	test_when_finished "rm \"$symbolic\"" &&
> +	mkdir -p "$physical" &&
> +	ln -s "$physical" "$symbolic" &&

This test needs a SYMLINKS prerequisite to future-proof it, in case the
Windows port gains p4 support some time.

> +	test_when_finished cleanup_git &&
> +	(
> +		P4CLIENT=client-sym &&
> +		p4 client -i <<-EOF &&
> +		Client: $P4CLIENT
> +		Description: $P4CLIENT
> +		Root: $symbolic
> +		LineEnd: unix
> +		View: //depot/... //$P4CLIENT/...
> +		EOF
> +		git p4 clone --dest="$git" //depot &&
> +		cd "$git" &&
> +		test_commit file2 &&
> +		git config git-p4.skipSubmitEdit true &&
> +		git p4 submit
> +	)
> +'

-- Hannes

[PATCH v2 0/3] fix git-p4 client root symlink problems

[Pete Wyckoff]

Update from v1:

    * add SYMLINKS prerequisite to the new symlink test

Thanks Hannes.


Miklós pointed out in

    http://thread.gmane.org/gmane.comp.version-control.git/214915

that when the p4 client root included a symlink, bad things
happen.  It is fixable, but inconvenient, to use an absolute path
in one's p4 client.  It's not too hard to be smarter about this
in git-p4.

Thanks to Miklós for the patch, and to John for the style
suggestions.  I wrote a couple of tests to make sure this part
doesn't break again.

This is maybe a bug introduced by bf1d68f (git-p4: use absolute
directory for PWD env var, 2011-12-09), but that's so long ago
that I don't think this is a candidate for maint.

		-- Pete


Miklós Fazekas (1):
  git p4: avoid expanding client paths in chdir

Pete Wyckoff (2):
  git p4 test: make sure P4CONFIG relative path works
  git p4 test: should honor symlink in p4 client root

 git-p4.py               | 29 ++++++++++++++++++++++-------
 t/t9808-git-p4-chdir.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 7 deletions(-)

-- 
1.8.2.rc2.65.g92f3e2d

[PATCH v2 1/3] git p4 test: make sure P4CONFIG relative path works

[Pete Wyckoff]

This adds a test for the fix in bf1d68f (git-p4: use absolute
directory for PWD env var, 2011-12-09).  It is necessary to
set PWD to an absolute path so that p4 can find files referenced
by non-absolute paths, like the value of the P4CONFIG environment
variable.

P4 does not open files directly; it builds a path by prepending
the contents of the PWD environment variable.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9808-git-p4-chdir.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index dc92e60..55c5e36 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -42,6 +42,20 @@ test_expect_success 'P4CONFIG and relative dir clone' '
 	)
 '
 
+# Common setup using .p4config to set P4CLIENT and P4PORT breaks
+# if clone destination is relative.  Make sure that chdir() expands
+# the relative path in --dest to absolute.
+test_expect_success 'p4 client root would be relative due to clone --dest' '
+	test_when_finished cleanup_git &&
+	(
+		echo P4PORT=$P4PORT >git/.p4config &&
+		P4CONFIG=.p4config &&
+		export P4CONFIG &&
+		unset P4PORT &&
+		git p4 clone --dest="git" //depot
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.8.2.rc2.65.g92f3e2d

[PATCH v2 2/3] git p4 test: should honor symlink in p4 client root

[Pete Wyckoff]

This test fails when the p4 client root includes
a symlink.  It complains:

    Path /vol/bar/projects/foo/... is not under client root /p/foo

and dumps a traceback.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9808-git-p4-chdir.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index 55c5e36..4773296 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -56,6 +56,33 @@ test_expect_success 'p4 client root would be relative due to clone --dest' '
 	)
 '
 
+# When the p4 client Root is a symlink, make sure chdir() does not use
+# getcwd() to convert it to a physical path.
+test_expect_failure SYMLINKS 'p4 client root symlink should stay symbolic' '
+	physical="$TRASH_DIRECTORY/physical" &&
+	symbolic="$TRASH_DIRECTORY/symbolic" &&
+	test_when_finished "rm -rf \"$physical\"" &&
+	test_when_finished "rm \"$symbolic\"" &&
+	mkdir -p "$physical" &&
+	ln -s "$physical" "$symbolic" &&
+	test_when_finished cleanup_git &&
+	(
+		P4CLIENT=client-sym &&
+		p4 client -i <<-EOF &&
+		Client: $P4CLIENT
+		Description: $P4CLIENT
+		Root: $symbolic
+		LineEnd: unix
+		View: //depot/... //$P4CLIENT/...
+		EOF
+		git p4 clone --dest="$git" //depot &&
+		cd "$git" &&
+		test_commit file2 &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.8.2.rc2.65.g92f3e2d

[PATCH v2 3/3] git p4: avoid expanding client paths in chdir

[Pete Wyckoff]

From: Miklós Fazekas <mfazekas@szemafor.com>

The generic chdir() helper sets the PWD environment
variable, as that is what is used by p4 to know its
current working directory.  Normally the shell would
do this, but in git-p4, we must do it by hand.

However, when the path contains a symbolic link,
os.getcwd() will return the physical location.  If the
p4 client specification includes symlinks, setting PWD
to the physical location causes p4 to think it is not
inside the client workspace.  It complains, e.g.

    Path /vol/bar/projects/foo/... is not under client root /p/foo

One workaround is to use AltRoots in the p4 client specification,
but it is cleaner to handle it directly in git-p4.

Other uses of chdir still require setting PWD to an
absolute path so p4 features like P4CONFIG work.  See
bf1d68f (git-p4: use absolute directory for PWD env
var, 2011-12-09).

[ pw: tweak patch and commit message ]

Thanks-to: John Keeping <john@keeping.me.uk>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py               | 29 ++++++++++++++++++++++-------
 t/t9808-git-p4-chdir.sh |  2 +-
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 647f110..7288c0b 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -79,12 +79,27 @@ def p4_build_cmd(cmd):
         real_cmd += cmd
     return real_cmd
 
-def chdir(dir):
-    # 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.chdir(dir)
-    os.environ['PWD'] = os.getcwd()
+def chdir(path, is_client_path=False):
+    """Do chdir to the given path, and set the PWD environment
+       variable for use by P4.  It does not look at getcwd() output.
+       Since we're not using the shell, it is necessary to set the
+       PWD environment variable explicitly.
+       
+       Normally, expand the path to force it to be absolute.  This
+       addresses the use of relative path names inside P4 settings,
+       e.g. P4CONFIG=.p4config.  P4 does not simply open the filename
+       as given; it looks for .p4config using PWD.
+
+       If is_client_path, the path was handed to us directly by p4,
+       and may be a symbolic link.  Do not call os.getcwd() in this
+       case, because it will cause p4 to think that PWD is not inside
+       the client path.
+       """
+
+    os.chdir(path)
+    if not is_client_path:
+        path = os.getcwd()
+    os.environ['PWD'] = path
 
 def die(msg):
     if verbose:
@@ -1624,7 +1639,7 @@ class P4Submit(Command, P4UserMap):
             new_client_dir = True
             os.makedirs(self.clientPath)
 
-        chdir(self.clientPath)
+        chdir(self.clientPath, is_client_path=True)
         if self.dry_run:
             print "Would synchronize p4 checkout in %s" % self.clientPath
         else:
diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index 4773296..11d2b51 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -58,7 +58,7 @@ test_expect_success 'p4 client root would be relative due to clone --dest' '
 
 # When the p4 client Root is a symlink, make sure chdir() does not use
 # getcwd() to convert it to a physical path.
-test_expect_failure SYMLINKS 'p4 client root symlink should stay symbolic' '
+test_expect_success SYMLINKS 'p4 client root symlink should stay symbolic' '
 	physical="$TRASH_DIRECTORY/physical" &&
 	symbolic="$TRASH_DIRECTORY/symbolic" &&
 	test_when_finished "rm -rf \"$physical\"" &&
-- 
1.8.2.rc2.65.g92f3e2d