{"thread":{"id":"32764","subject":"[PATCH] git p4: chdir resolves symlinks only for relative paths","startedAt":"2013-01-29T08:37:52Z","lastAt":"2013-03-11T21:45:29Z","messageCount":13,"participants":["Miklós Fazekas","Pete Wyckoff","John Keeping","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"208164","messageId":"CAAMmcSSEzs3+vZDO=FDMV9c2rp-8HTdMuPeeQCkok6y7sRDYJw@mail.gmail.com","threadId":"32764","inReplyTo":"CAAMmcSSvrsZqEVf68Nrqy_ZG6r5ESKhtx7JdQ7vzypkZ3gOFnA@mail.gmail.com","subject":"[PATCH] git p4: chdir resolves symlinks only for relative paths","fromName":"Miklós Fazekas","fromEmail":"mfazekas@szemafor.com","sentAt":"2013-01-29T08:37:52Z","receivedAt":"2013-01-29T08:37:52Z","isPatch":true,"sender":{"key":"mfazekas@szemafor.com","avatar":null},"body":"[resending as plain text]\n\nIf a p4 client is configured to /p/foo which is a symlink\nto /vol/bar/projects/foo, then resolving symlink, which\nis done by git-p4's chdir will confuse p4: \"Path\n/vol/bar/projects/foo/... is not under client root /p/foo\"\nWhile AltRoots in p4 client specification can be used as a\nworkaround on p4 side, git-p4 should not resolve symlinks\nin client paths.\nchdir(dir) uses os.getcwd() after os.chdir(dir) to resolve\nrelative paths, but as a side effect it resolves symlinks\ntoo. Now it checks if the dir is relative before resolving.\n\nSigned-off-by: Miklós Fazekas <mfazekas@szemafor.com>\n---\n git-p4.py |    5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 2da5649..5d74649 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -64,7 +64,10 @@ def chdir(dir):\n     # not using the shell, we have to set it ourselves.  This path could\n     # be relative, so go there first, then figure out where we ended up.\n     os.chdir(dir)\n-    os.environ['PWD'] = os.getcwd()\n+    if os.path.isabs(dir):\n+        os.environ['PWD'] = dir\n+    else:\n+        os.environ['PWD'] = os.getcwd()\n\n def die(msg):\n     if verbose:\n-- \n1.7.10.2 (Apple Git-33)\n"},{"id":"208572","messageId":"20130203230803.GA25555@padd.com","threadId":"32764","inReplyTo":"CAAMmcSSEzs3+vZDO=FDMV9c2rp-8HTdMuPeeQCkok6y7sRDYJw@mail.gmail.com","subject":"Re: [PATCH] git p4: chdir resolves symlinks only for relative paths","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-02-03T23:08:03Z","receivedAt":"2013-02-03T23:08:03Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"mfazekas@szemafor.com wrote on Tue, 29 Jan 2013 09:37 +0100:\n> If a p4 client is configured to /p/foo which is a symlink\n> to /vol/bar/projects/foo, then resolving symlink, which\n> is done by git-p4's chdir will confuse p4: \"Path\n> /vol/bar/projects/foo/... is not under client root /p/foo\"\n> While AltRoots in p4 client specification can be used as a\n> workaround on p4 side, git-p4 should not resolve symlinks\n> in client paths.\n> chdir(dir) uses os.getcwd() after os.chdir(dir) to resolve\n> relative paths, but as a side effect it resolves symlinks\n> too. Now it checks if the dir is relative before resolving.\n> \n> Signed-off-by: Miklós Fazekas <mfazekas@szemafor.com>\n> ---\n>  git-p4.py |    5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/git-p4.py b/git-p4.py\n> index 2da5649..5d74649 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -64,7 +64,10 @@ def chdir(dir):\n>      # not using the shell, we have to set it ourselves.  This path could\n>      # be relative, so go there first, then figure out where we ended up.\n>      os.chdir(dir)\n> -    os.environ['PWD'] = os.getcwd()\n> +    if os.path.isabs(dir):\n> +        os.environ['PWD'] = dir\n> +    else:\n> +        os.environ['PWD'] = os.getcwd()\n> \n>  def die(msg):\n>      if verbose:\n\nThanks, this is indeed a bug and I have reproduced it with a test\ncase.  Your patch works, but I think it would be better to\nseparate the callers of chdir():  those that know they are\ncd-ing to a path from a p4 client, and everybody else.  The former\nshould not use os.getcwd(), as you show.\n\nI'll whip something up soon, unless you beat me to it.\n\n\t\t-- Pete\n"},{"id":"210750","messageId":"CAAMmcSQszVbDERd964VLu1d4UG7SihC+Pn99D0gPvG7HAZp2UQ@mail.gmail.com","threadId":"32764","inReplyTo":"20130203230803.GA25555@padd.com","subject":"Re: [PATCH] git p4: chdir resolves symlinks only for relative paths","fromName":"Miklós Fazekas","fromEmail":"mfazekas@szemafor.com","sentAt":"2013-03-07T08:36:06Z","receivedAt":"2013-03-07T08:36:06Z","isPatch":true,"sender":{"key":"mfazekas@szemafor.com","avatar":null},"body":"Sorry for the late turnaround here is an improved version. Now chdir\nhas an optional argument client_path, if it's true then we don't do\nos.getcwd. I think that my first patch is also valid too - when the\npath is absolute no need for getcwd no matter what is the context,\nwhen it's relative we have to use os.getcwd() no matter of the\ncontext.\n\n---\nIf p4 client is configured to /p/foo which is a symlink:\n/p/foo -> /vol/barvol/projects/foo.  Then resolving the\nsymlink will confuse p4:\n\"Path /vol/barvol/projects/foo/... is not under client root\n/p/foo\". While AltRoots in p4 client specification can be\nused as a workaround on p4 side, git-p4 should not resolve\nsymlinks in client paths.\nchdir(dir) uses os.getcwd() after os.chdir(dir) to resolve\nrelative paths, but as a sideeffect it resolves symlinks\ntoo. Now for client paths we don't call os.getcwd().\n\nSigned-off-by: Miklós Fazekas <mfazekas@szemafor.com>\n---\n git-p4.py |   11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 0682e61..2bd8cc2 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -68,12 +68,17 @@ def p4_build_cmd(cmd):\n         real_cmd += cmd\n     return real_cmd\n\n-def chdir(dir):\n+def chdir(dir,client_path=False):\n     # P4 uses the PWD environment variable rather than getcwd(). Since we're\n     # not using the shell, we have to set it ourselves.  This path could\n     # be relative, so go there first, then figure out where we ended up.\n+    # os.getcwd() will resolve symlinks, so we should avoid it for\n+    # client_paths.\n     os.chdir(dir)\n-    os.environ['PWD'] = os.getcwd()\n+    if client_path:\n+        os.environ['PWD'] = dir\n+    else:\n+               os.environ['PWD'] = os.getcwd()\n\n def die(msg):\n     if verbose:\n@@ -1554,7 +1559,7 @@ class P4Submit(Command, P4UserMap):\n             new_client_dir = True\n             os.makedirs(self.clientPath)\n\n-        chdir(self.clientPath)\n+        chdir(self.clientPath,client_path=True)\n         if self.dry_run:\n             print \"Would synchronize p4 checkout in %s\" % self.clientPath\n         else:\n-- \n1.7.10.2 (Apple Git-33)\n\n\nOn Mon, Feb 4, 2013 at 12:08 AM, Pete Wyckoff <pw@padd.com> wrote:\n> mfazekas@szemafor.com wrote on Tue, 29 Jan 2013 09:37 +0100:\n>> If a p4 client is configured to /p/foo which is a symlink\n>> to /vol/bar/projects/foo, then resolving symlink, which\n>> is done by git-p4's chdir will confuse p4: \"Path\n>> /vol/bar/projects/foo/... is not under client root /p/foo\"\n>> While AltRoots in p4 client specification can be used as a\n>> workaround on p4 side, git-p4 should not resolve symlinks\n>> in client paths.\n>> chdir(dir) uses os.getcwd() after os.chdir(dir) to resolve\n>> relative paths, but as a side effect it resolves symlinks\n>> too. Now it checks if the dir is relative before resolving.\n>>\n>> Signed-off-by: Miklós Fazekas <mfazekas@szemafor.com>\n>> ---\n>>  git-p4.py |    5 ++++-\n>>  1 file changed, 4 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/git-p4.py b/git-p4.py\n>> index 2da5649..5d74649 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -64,7 +64,10 @@ def chdir(dir):\n>>      # not using the shell, we have to set it ourselves.  This path could\n>>      # be relative, so go there first, then figure out where we ended up.\n>>      os.chdir(dir)\n>> -    os.environ['PWD'] = os.getcwd()\n>> +    if os.path.isabs(dir):\n>> +        os.environ['PWD'] = dir\n>> +    else:\n>> +        os.environ['PWD'] = os.getcwd()\n>>\n>>  def die(msg):\n>>      if verbose:\n>\n> Thanks, this is indeed a bug and I have reproduced it with a test\n> case.  Your patch works, but I think it would be better to\n> separate the callers of chdir():  those that know they are\n> cd-ing to a path from a p4 client, and everybody else.  The former\n> should not use os.getcwd(), as you show.\n>\n> I'll whip something up soon, unless you beat me to it.\n>\n>                 -- Pete\n"},{"id":"210753","messageId":"20130307091317.GY7738@serenity.lan","threadId":"32764","inReplyTo":"CAAMmcSQszVbDERd964VLu1d4UG7SihC+Pn99D0gPvG7HAZp2UQ@mail.gmail.com","subject":"Re: [PATCH] git p4: chdir resolves symlinks only for relative paths","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-07T09:13:18Z","receivedAt":"2013-03-07T09:13:18Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Mar 07, 2013 at 09:36:06AM +0100, Miklós Fazekas wrote:\n> Sorry for the late turnaround here is an improved version. Now chdir\n> has an optional argument client_path, if it's true then we don't do\n> os.getcwd. I think that my first patch is also valid too - when the\n> path is absolute no need for getcwd no matter what is the context,\n> when it's relative we have to use os.getcwd() no matter of the\n> context.\n> \n> ---\n> If p4 client is configured to /p/foo which is a symlink:\n> /p/foo -> /vol/barvol/projects/foo.  Then resolving the\n> symlink will confuse p4:\n> \"Path /vol/barvol/projects/foo/... is not under client root\n> /p/foo\". While AltRoots in p4 client specification can be\n> used as a workaround on p4 side, git-p4 should not resolve\n> symlinks in client paths.\n> chdir(dir) uses os.getcwd() after os.chdir(dir) to resolve\n> relative paths, but as a sideeffect it resolves symlinks\n> too. Now for client paths we don't call os.getcwd().\n> \n> Signed-off-by: Miklós Fazekas <mfazekas@szemafor.com>\n> ---\n>  git-p4.py |   11 ++++++++---\n>  1 file changed, 8 insertions(+), 3 deletions(-)\n> \n> diff --git a/git-p4.py b/git-p4.py\n> index 0682e61..2bd8cc2 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -68,12 +68,17 @@ def p4_build_cmd(cmd):\n>          real_cmd += cmd\n>      return real_cmd\n> \n> -def chdir(dir):\n> +def chdir(dir,client_path=False):\n\nStyle (space after comma):\n\n    def chdir(dir, client_path=False):\n\n>      # P4 uses the PWD environment variable rather than getcwd(). Since we're\n>      # not using the shell, we have to set it ourselves.  This path could\n>      # be relative, so go there first, then figure out where we ended up.\n> +    # os.getcwd() will resolve symlinks, so we should avoid it for\n> +    # client_paths.\n>      os.chdir(dir)\n> -    os.environ['PWD'] = os.getcwd()\n> +    if client_path:\n> +        os.environ['PWD'] = dir\n> +    else:\n> +               os.environ['PWD'] = os.getcwd()\n\nIndentation seems to have gone a bit wrong here...\n\n> \n>  def die(msg):\n>      if verbose:\n> @@ -1554,7 +1559,7 @@ class P4Submit(Command, P4UserMap):\n>              new_client_dir = True\n>              os.makedirs(self.clientPath)\n> \n> -        chdir(self.clientPath)\n> +        chdir(self.clientPath,client_path=True)\n\nAgain, there should be a space after the comma here.\n\n>          if self.dry_run:\n>              print \"Would synchronize p4 checkout in %s\" % self.clientPath\n>          else:\n> -- \n> 1.7.10.2 (Apple Git-33)\n"},{"id":"210797","messageId":"1362698357-7334-1-git-send-email-pw@padd.com","threadId":"32764","inReplyTo":"20130307091317.GY7738@serenity.lan","subject":"[PATCH 0/3] fix git-p4 client root symlink problems","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-03-07T23:19:14Z","receivedAt":"2013-03-07T23:19:14Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"Miklós pointed out in\n\n    http://thread.gmane.org/gmane.comp.version-control.git/214915\n\nthat when the p4 client root included a symlink, bad things\nhappen.  It is fixable, but inconvenient, to use an absolute path\nin one's p4 client.  It's not too hard to be smarter about this\nin git-p4.\n\nThanks to Miklós for the patch, and to John for the style\nsuggestions.  I wrote a couple of tests to make sure this part\ndoesn't break again.\n\nThis is maybe a bug introduced by bf1d68f (git-p4: use absolute\ndirectory for PWD env var, 2011-12-09), but that's so long ago\nthat I don't think this is a candidate for maint.\n\n\t\t-- Pete\n\nMiklós Fazekas (1):\n  git p4: avoid expanding client paths in chdir\n\nPete Wyckoff (2):\n  git p4 test: make sure P4CONFIG relative path works\n  git p4 test: should honor symlink in p4 client root\n\n git-p4.py               | 29 ++++++++++++++++++++++-------\n t/t9808-git-p4-chdir.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 7 deletions(-)\n\n-- \n1.8.2.rc2.64.g8335025\n"},{"id":"210798","messageId":"1362698357-7334-2-git-send-email-pw@padd.com","threadId":"32764","inReplyTo":"1362698357-7334-1-git-send-email-pw@padd.com","subject":"[PATCH 1/3] git p4 test: make sure P4CONFIG relative path works","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-03-07T23:19:15Z","receivedAt":"2013-03-07T23:19:15Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"This adds a test for the fix in bf1d68f (git-p4: use absolute\ndirectory for PWD env var, 2011-12-09).  It is necessary to\nset PWD to an absolute path so that p4 can find files referenced\nby non-absolute paths, like the value of the P4CONFIG environment\nvariable.\n\nP4 does not open files directly; it builds a path by prepending\nthe contents of the PWD environment variable.\n\nSigned-off-by: Pete Wyckoff <pw@padd.com>\n---\n t/t9808-git-p4-chdir.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh\nindex dc92e60..55c5e36 100755\n--- a/t/t9808-git-p4-chdir.sh\n+++ b/t/t9808-git-p4-chdir.sh\n@@ -42,6 +42,20 @@ test_expect_success 'P4CONFIG and relative dir clone' '\n \t)\n '\n \n+# Common setup using .p4config to set P4CLIENT and P4PORT breaks\n+# if clone destination is relative.  Make sure that chdir() expands\n+# the relative path in --dest to absolute.\n+test_expect_success 'p4 client root would be relative due to clone --dest' '\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\techo P4PORT=$P4PORT >git/.p4config &&\n+\t\tP4CONFIG=.p4config &&\n+\t\texport P4CONFIG &&\n+\t\tunset P4PORT &&\n+\t\tgit p4 clone --dest=\"git\" //depot\n+\t)\n+'\n+\n test_expect_success 'kill p4d' '\n \tkill_p4d\n '\n-- \n1.8.2.rc2.64.g8335025\n"},{"id":"210799","messageId":"1362698357-7334-3-git-send-email-pw@padd.com","threadId":"32764","inReplyTo":"1362698357-7334-1-git-send-email-pw@padd.com","subject":"[PATCH 2/3] git p4 test: should honor symlink in p4 client root","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-03-07T23:19:16Z","receivedAt":"2013-03-07T23:19:16Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"This test fails when the p4 client root includes\na symlink.  It complains:\n\n    Path /vol/bar/projects/foo/... is not under client root /p/foo\n\nand dumps a traceback.\n\nSigned-off-by: Pete Wyckoff <pw@padd.com>\n---\n t/t9808-git-p4-chdir.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh\nindex 55c5e36..af8bd8a 100755\n--- a/t/t9808-git-p4-chdir.sh\n+++ b/t/t9808-git-p4-chdir.sh\n@@ -56,6 +56,33 @@ test_expect_success 'p4 client root would be relative due to clone --dest' '\n \t)\n '\n \n+# When the p4 client Root is a symlink, make sure chdir() does not use\n+# getcwd() to convert it to a physical path.\n+test_expect_failure 'p4 client root symlink should stay symbolic' '\n+\tphysical=\"$TRASH_DIRECTORY/physical\" &&\n+\tsymbolic=\"$TRASH_DIRECTORY/symbolic\" &&\n+\ttest_when_finished \"rm -rf \\\"$physical\\\"\" &&\n+\ttest_when_finished \"rm \\\"$symbolic\\\"\" &&\n+\tmkdir -p \"$physical\" &&\n+\tln -s \"$physical\" \"$symbolic\" &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tP4CLIENT=client-sym &&\n+\t\tp4 client -i <<-EOF &&\n+\t\tClient: $P4CLIENT\n+\t\tDescription: $P4CLIENT\n+\t\tRoot: $symbolic\n+\t\tLineEnd: unix\n+\t\tView: //depot/... //$P4CLIENT/...\n+\t\tEOF\n+\t\tgit p4 clone --dest=\"$git\" //depot &&\n+\t\tcd \"$git\" &&\n+\t\ttest_commit file2 &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\tgit p4 submit\n+\t)\n+'\n+\n test_expect_success 'kill p4d' '\n \tkill_p4d\n '\n-- \n1.8.2.rc2.64.g8335025\n"},{"id":"210800","messageId":"1362698357-7334-4-git-send-email-pw@padd.com","threadId":"32764","inReplyTo":"1362698357-7334-1-git-send-email-pw@padd.com","subject":"[PATCH 3/3] git p4: avoid expanding client paths in chdir","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-03-07T23:19:17Z","receivedAt":"2013-03-07T23:19:17Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"From: Miklós Fazekas <mfazekas@szemafor.com>\n\nThe generic chdir() helper sets the PWD environment\nvariable, as that is what is used by p4 to know its\ncurrent working directory.  Normally the shell would\ndo this, but in git-p4, we must do it by hand.\n\nHowever, when the path contains a symbolic link,\nos.getcwd() will return the physical location.  If the\np4 client specification includes symlinks, setting PWD\nto the physical location causes p4 to think it is not\ninside the client workspace.  It complains, e.g.\n\n    Path /vol/bar/projects/foo/... is not under client root /p/foo\n\nOne workaround is to use AltRoots in the p4 client specification,\nbut it is cleaner to handle it directly in git-p4.\n\nOther uses of chdir still require setting PWD to an\nabsolute path so p4 features like P4CONFIG work.  See\nbf1d68f (git-p4: use absolute directory for PWD env\nvar, 2011-12-09).\n\n[ pw: tweak patch and commit message ]\n\nThanks-to: John Keeping <john@keeping.me.uk>\nSigned-off-by: Pete Wyckoff <pw@padd.com>\n---\n git-p4.py               | 29 ++++++++++++++++++++++-------\n t/t9808-git-p4-chdir.sh |  2 +-\n 2 files changed, 23 insertions(+), 8 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 647f110..7288c0b 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -79,12 +79,27 @@ def p4_build_cmd(cmd):\n         real_cmd += cmd\n     return real_cmd\n \n-def chdir(dir):\n-    # P4 uses the PWD environment variable rather than getcwd(). Since we're\n-    # not using the shell, we have to set it ourselves.  This path could\n-    # be relative, so go there first, then figure out where we ended up.\n-    os.chdir(dir)\n-    os.environ['PWD'] = os.getcwd()\n+def chdir(path, is_client_path=False):\n+    \"\"\"Do chdir to the given path, and set the PWD environment\n+       variable for use by P4.  It does not look at getcwd() output.\n+       Since we're not using the shell, it is necessary to set the\n+       PWD environment variable explicitly.\n+       \n+       Normally, expand the path to force it to be absolute.  This\n+       addresses the use of relative path names inside P4 settings,\n+       e.g. P4CONFIG=.p4config.  P4 does not simply open the filename\n+       as given; it looks for .p4config using PWD.\n+\n+       If is_client_path, the path was handed to us directly by p4,\n+       and may be a symbolic link.  Do not call os.getcwd() in this\n+       case, because it will cause p4 to think that PWD is not inside\n+       the client path.\n+       \"\"\"\n+\n+    os.chdir(path)\n+    if not is_client_path:\n+        path = os.getcwd()\n+    os.environ['PWD'] = path\n \n def die(msg):\n     if verbose:\n@@ -1624,7 +1639,7 @@ class P4Submit(Command, P4UserMap):\n             new_client_dir = True\n             os.makedirs(self.clientPath)\n \n-        chdir(self.clientPath)\n+        chdir(self.clientPath, is_client_path=True)\n         if self.dry_run:\n             print \"Would synchronize p4 checkout in %s\" % self.clientPath\n         else:\ndiff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh\nindex af8bd8a..09b2cc4 100755\n--- a/t/t9808-git-p4-chdir.sh\n+++ b/t/t9808-git-p4-chdir.sh\n@@ -58,7 +58,7 @@ test_expect_success 'p4 client root would be relative due to clone --dest' '\n \n # When the p4 client Root is a symlink, make sure chdir() does not use\n # getcwd() to convert it to a physical path.\n-test_expect_failure 'p4 client root symlink should stay symbolic' '\n+test_expect_success 'p4 client root symlink should stay symbolic' '\n \tphysical=\"$TRASH_DIRECTORY/physical\" &&\n \tsymbolic=\"$TRASH_DIRECTORY/symbolic\" &&\n \ttest_when_finished \"rm -rf \\\"$physical\\\"\" &&\n-- \n1.8.2.rc2.64.g8335025\n"},{"id":"210811","messageId":"5139883C.6080308@viscovery.net","threadId":"32764","inReplyTo":"1362698357-7334-3-git-send-email-pw@padd.com","subject":"Re: [PATCH 2/3] git p4 test: should honor symlink in p4 client root","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-08T06:42:04Z","receivedAt":"2013-03-08T06:42:04Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/8/2013 0:19, schrieb Pete Wyckoff:\n> +# When the p4 client Root is a symlink, make sure chdir() does not use\n> +# getcwd() to convert it to a physical path.\n> +test_expect_failure 'p4 client root symlink should stay symbolic' '\n> +\tphysical=\"$TRASH_DIRECTORY/physical\" &&\n> +\tsymbolic=\"$TRASH_DIRECTORY/symbolic\" &&\n> +\ttest_when_finished \"rm -rf \\\"$physical\\\"\" &&\n> +\ttest_when_finished \"rm \\\"$symbolic\\\"\" &&\n> +\tmkdir -p \"$physical\" &&\n> +\tln -s \"$physical\" \"$symbolic\" &&\n\nThis test needs a SYMLINKS prerequisite to future-proof it, in case the\nWindows port gains p4 support some time.\n\n> +\ttest_when_finished cleanup_git &&\n> +\t(\n> +\t\tP4CLIENT=client-sym &&\n> +\t\tp4 client -i <<-EOF &&\n> +\t\tClient: $P4CLIENT\n> +\t\tDescription: $P4CLIENT\n> +\t\tRoot: $symbolic\n> +\t\tLineEnd: unix\n> +\t\tView: //depot/... //$P4CLIENT/...\n> +\t\tEOF\n> +\t\tgit p4 clone --dest=\"$git\" //depot &&\n> +\t\tcd \"$git\" &&\n> +\t\ttest_commit file2 &&\n> +\t\tgit config git-p4.skipSubmitEdit true &&\n> +\t\tgit p4 submit\n> +\t)\n> +'\n\n-- Hannes\n"},{"id":"211081","messageId":"1363038329-20185-1-git-send-email-pw@padd.com","threadId":"32764","inReplyTo":"5139883C.6080308@viscovery.net","subject":"[PATCH v2 0/3] fix git-p4 client root symlink problems","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-03-11T21:45:26Z","receivedAt":"2013-03-11T21:45:26Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"Update from v1:\n\n    * add SYMLINKS prerequisite to the new symlink test\n\nThanks Hannes.\n\n\nMiklós pointed out in\n\n    http://thread.gmane.org/gmane.comp.version-control.git/214915\n\nthat when the p4 client root included a symlink, bad things\nhappen.  It is fixable, but inconvenient, to use an absolute path\nin one's p4 client.  It's not too hard to be smarter about this\nin git-p4.\n\nThanks to Miklós for the patch, and to John for the style\nsuggestions.  I wrote a couple of tests to make sure this part\ndoesn't break again.\n\nThis is maybe a bug introduced by bf1d68f (git-p4: use absolute\ndirectory for PWD env var, 2011-12-09), but that's so long ago\nthat I don't think this is a candidate for maint.\n\n\t\t-- Pete\n\n\nMiklós Fazekas (1):\n  git p4: avoid expanding client paths in chdir\n\nPete Wyckoff (2):\n  git p4 test: make sure P4CONFIG relative path works\n  git p4 test: should honor symlink in p4 client root\n\n git-p4.py               | 29 ++++++++++++++++++++++-------\n t/t9808-git-p4-chdir.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 7 deletions(-)\n\n-- \n1.8.2.rc2.65.g92f3e2d\n"},{"id":"211082","messageId":"1363038329-20185-2-git-send-email-pw@padd.com","threadId":"32764","inReplyTo":"1363038329-20185-1-git-send-email-pw@padd.com","subject":"[PATCH v2 1/3] git p4 test: make sure P4CONFIG relative path works","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-03-11T21:45:27Z","receivedAt":"2013-03-11T21:45:27Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"This adds a test for the fix in bf1d68f (git-p4: use absolute\ndirectory for PWD env var, 2011-12-09).  It is necessary to\nset PWD to an absolute path so that p4 can find files referenced\nby non-absolute paths, like the value of the P4CONFIG environment\nvariable.\n\nP4 does not open files directly; it builds a path by prepending\nthe contents of the PWD environment variable.\n\nSigned-off-by: Pete Wyckoff <pw@padd.com>\n---\n t/t9808-git-p4-chdir.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh\nindex dc92e60..55c5e36 100755\n--- a/t/t9808-git-p4-chdir.sh\n+++ b/t/t9808-git-p4-chdir.sh\n@@ -42,6 +42,20 @@ test_expect_success 'P4CONFIG and relative dir clone' '\n \t)\n '\n \n+# Common setup using .p4config to set P4CLIENT and P4PORT breaks\n+# if clone destination is relative.  Make sure that chdir() expands\n+# the relative path in --dest to absolute.\n+test_expect_success 'p4 client root would be relative due to clone --dest' '\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\techo P4PORT=$P4PORT >git/.p4config &&\n+\t\tP4CONFIG=.p4config &&\n+\t\texport P4CONFIG &&\n+\t\tunset P4PORT &&\n+\t\tgit p4 clone --dest=\"git\" //depot\n+\t)\n+'\n+\n test_expect_success 'kill p4d' '\n \tkill_p4d\n '\n-- \n1.8.2.rc2.65.g92f3e2d\n"},{"id":"211083","messageId":"1363038329-20185-3-git-send-email-pw@padd.com","threadId":"32764","inReplyTo":"1363038329-20185-1-git-send-email-pw@padd.com","subject":"[PATCH v2 2/3] git p4 test: should honor symlink in p4 client root","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-03-11T21:45:28Z","receivedAt":"2013-03-11T21:45:28Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"This test fails when the p4 client root includes\na symlink.  It complains:\n\n    Path /vol/bar/projects/foo/... is not under client root /p/foo\n\nand dumps a traceback.\n\nSigned-off-by: Pete Wyckoff <pw@padd.com>\n---\n t/t9808-git-p4-chdir.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh\nindex 55c5e36..4773296 100755\n--- a/t/t9808-git-p4-chdir.sh\n+++ b/t/t9808-git-p4-chdir.sh\n@@ -56,6 +56,33 @@ test_expect_success 'p4 client root would be relative due to clone --dest' '\n \t)\n '\n \n+# When the p4 client Root is a symlink, make sure chdir() does not use\n+# getcwd() to convert it to a physical path.\n+test_expect_failure SYMLINKS 'p4 client root symlink should stay symbolic' '\n+\tphysical=\"$TRASH_DIRECTORY/physical\" &&\n+\tsymbolic=\"$TRASH_DIRECTORY/symbolic\" &&\n+\ttest_when_finished \"rm -rf \\\"$physical\\\"\" &&\n+\ttest_when_finished \"rm \\\"$symbolic\\\"\" &&\n+\tmkdir -p \"$physical\" &&\n+\tln -s \"$physical\" \"$symbolic\" &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tP4CLIENT=client-sym &&\n+\t\tp4 client -i <<-EOF &&\n+\t\tClient: $P4CLIENT\n+\t\tDescription: $P4CLIENT\n+\t\tRoot: $symbolic\n+\t\tLineEnd: unix\n+\t\tView: //depot/... //$P4CLIENT/...\n+\t\tEOF\n+\t\tgit p4 clone --dest=\"$git\" //depot &&\n+\t\tcd \"$git\" &&\n+\t\ttest_commit file2 &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\tgit p4 submit\n+\t)\n+'\n+\n test_expect_success 'kill p4d' '\n \tkill_p4d\n '\n-- \n1.8.2.rc2.65.g92f3e2d\n"},{"id":"211085","messageId":"1363038329-20185-4-git-send-email-pw@padd.com","threadId":"32764","inReplyTo":"1363038329-20185-1-git-send-email-pw@padd.com","subject":"[PATCH v2 3/3] git p4: avoid expanding client paths in chdir","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-03-11T21:45:29Z","receivedAt":"2013-03-11T21:45:29Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"From: Miklós Fazekas <mfazekas@szemafor.com>\n\nThe generic chdir() helper sets the PWD environment\nvariable, as that is what is used by p4 to know its\ncurrent working directory.  Normally the shell would\ndo this, but in git-p4, we must do it by hand.\n\nHowever, when the path contains a symbolic link,\nos.getcwd() will return the physical location.  If the\np4 client specification includes symlinks, setting PWD\nto the physical location causes p4 to think it is not\ninside the client workspace.  It complains, e.g.\n\n    Path /vol/bar/projects/foo/... is not under client root /p/foo\n\nOne workaround is to use AltRoots in the p4 client specification,\nbut it is cleaner to handle it directly in git-p4.\n\nOther uses of chdir still require setting PWD to an\nabsolute path so p4 features like P4CONFIG work.  See\nbf1d68f (git-p4: use absolute directory for PWD env\nvar, 2011-12-09).\n\n[ pw: tweak patch and commit message ]\n\nThanks-to: John Keeping <john@keeping.me.uk>\nSigned-off-by: Pete Wyckoff <pw@padd.com>\n---\n git-p4.py               | 29 ++++++++++++++++++++++-------\n t/t9808-git-p4-chdir.sh |  2 +-\n 2 files changed, 23 insertions(+), 8 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 647f110..7288c0b 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -79,12 +79,27 @@ def p4_build_cmd(cmd):\n         real_cmd += cmd\n     return real_cmd\n \n-def chdir(dir):\n-    # P4 uses the PWD environment variable rather than getcwd(). Since we're\n-    # not using the shell, we have to set it ourselves.  This path could\n-    # be relative, so go there first, then figure out where we ended up.\n-    os.chdir(dir)\n-    os.environ['PWD'] = os.getcwd()\n+def chdir(path, is_client_path=False):\n+    \"\"\"Do chdir to the given path, and set the PWD environment\n+       variable for use by P4.  It does not look at getcwd() output.\n+       Since we're not using the shell, it is necessary to set the\n+       PWD environment variable explicitly.\n+       \n+       Normally, expand the path to force it to be absolute.  This\n+       addresses the use of relative path names inside P4 settings,\n+       e.g. P4CONFIG=.p4config.  P4 does not simply open the filename\n+       as given; it looks for .p4config using PWD.\n+\n+       If is_client_path, the path was handed to us directly by p4,\n+       and may be a symbolic link.  Do not call os.getcwd() in this\n+       case, because it will cause p4 to think that PWD is not inside\n+       the client path.\n+       \"\"\"\n+\n+    os.chdir(path)\n+    if not is_client_path:\n+        path = os.getcwd()\n+    os.environ['PWD'] = path\n \n def die(msg):\n     if verbose:\n@@ -1624,7 +1639,7 @@ class P4Submit(Command, P4UserMap):\n             new_client_dir = True\n             os.makedirs(self.clientPath)\n \n-        chdir(self.clientPath)\n+        chdir(self.clientPath, is_client_path=True)\n         if self.dry_run:\n             print \"Would synchronize p4 checkout in %s\" % self.clientPath\n         else:\ndiff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh\nindex 4773296..11d2b51 100755\n--- a/t/t9808-git-p4-chdir.sh\n+++ b/t/t9808-git-p4-chdir.sh\n@@ -58,7 +58,7 @@ test_expect_success 'p4 client root would be relative due to clone --dest' '\n \n # When the p4 client Root is a symlink, make sure chdir() does not use\n # getcwd() to convert it to a physical path.\n-test_expect_failure SYMLINKS 'p4 client root symlink should stay symbolic' '\n+test_expect_success SYMLINKS 'p4 client root symlink should stay symbolic' '\n \tphysical=\"$TRASH_DIRECTORY/physical\" &&\n \tsymbolic=\"$TRASH_DIRECTORY/symbolic\" &&\n \ttest_when_finished \"rm -rf \\\"$physical\\\"\" &&\n-- \n1.8.2.rc2.65.g92f3e2d\n"}]}