Merge branch 'pw/p4-symlinked-root'

"git p4" did not behave well when the path to the root of the P4
client was not its real path.

* pw/p4-symlinked-root:
  git p4: avoid expanding client paths in chdir
  git p4 test: should honor symlink in p4 client root
  git p4 test: make sure P4CONFIG relative path works

2 files changed, 63 insertions, 7 deletions

diff --git a/git-p4.py b/git-p4.py
index 647f110202..911bbce6c5 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 dc92e60cd6..11d2b5102c 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -42,6 +42,47 @@ 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
+	)
+'
+
+# When the p4 client Root is a symlink, make sure chdir() does not use
+# getcwd() to convert it to a physical path.
+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\"" &&
+	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
 '