# [PATCH] git p4: chdir resolves symlinks only for relative paths 13 messages from 2013-01-29 to 2013-03-11. Participants: Miklós Fazekas, Pete Wyckoff, John Keeping, Johannes Sixt. Thread: https://gitlist.dev/t/32764 ## Miklós Fazekas, 2013-01-29 08:37 Subject: [PATCH] git p4: chdir resolves symlinks only for relative paths Message-ID: URL: https://gitlist.dev/e/CAAMmcSSEzs3%2BvZDO%3DFDMV9c2rp-8HTdMuPeeQCkok6y7sRDYJw%40mail.gmail.com In-Reply-To: ``` [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 --- 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) ``` ## Pete Wyckoff, 2013-02-03 23:08 Subject: Re: [PATCH] git p4: chdir resolves symlinks only for relative paths Message-ID: <20130203230803.GA25555@padd.com> URL: https://gitlist.dev/e/20130203230803.GA25555%40padd.com In-Reply-To: ``` 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 > --- > 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 ``` ## Miklós Fazekas, 2013-03-07 08:36 Subject: Re: [PATCH] git p4: chdir resolves symlinks only for relative paths Message-ID: URL: https://gitlist.dev/e/CAAMmcSQszVbDERd964VLu1d4UG7SihC%2BPn99D0gPvG7HAZp2UQ%40mail.gmail.com In-Reply-To: <20130203230803.GA25555@padd.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 --- 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 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 >> --- >> 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 ``` ## John Keeping, 2013-03-07 09:13 Subject: Re: [PATCH] git p4: chdir resolves symlinks only for relative paths Message-ID: <20130307091317.GY7738@serenity.lan> URL: https://gitlist.dev/e/20130307091317.GY7738%40serenity.lan In-Reply-To: ``` 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 > --- > 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) ``` ## Pete Wyckoff, 2013-03-07 23:19 Subject: [PATCH 0/3] fix git-p4 client root symlink problems Message-ID: <1362698357-7334-1-git-send-email-pw@padd.com> URL: https://gitlist.dev/e/1362698357-7334-1-git-send-email-pw%40padd.com In-Reply-To: <20130307091317.GY7738@serenity.lan> ``` 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 ``` ## Pete Wyckoff, 2013-03-07 23:19 Subject: [PATCH 1/3] git p4 test: make sure P4CONFIG relative path works Message-ID: <1362698357-7334-2-git-send-email-pw@padd.com> URL: https://gitlist.dev/e/1362698357-7334-2-git-send-email-pw%40padd.com In-Reply-To: <1362698357-7334-1-git-send-email-pw@padd.com> ``` 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 --- 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 ``` ## Pete Wyckoff, 2013-03-07 23:19 Subject: [PATCH 2/3] git p4 test: should honor symlink in p4 client root Message-ID: <1362698357-7334-3-git-send-email-pw@padd.com> URL: https://gitlist.dev/e/1362698357-7334-3-git-send-email-pw%40padd.com In-Reply-To: <1362698357-7334-1-git-send-email-pw@padd.com> ``` 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 --- 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 ``` ## Pete Wyckoff, 2013-03-07 23:19 Subject: [PATCH 3/3] git p4: avoid expanding client paths in chdir Message-ID: <1362698357-7334-4-git-send-email-pw@padd.com> URL: https://gitlist.dev/e/1362698357-7334-4-git-send-email-pw%40padd.com In-Reply-To: <1362698357-7334-1-git-send-email-pw@padd.com> ``` From: Miklós Fazekas 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 Signed-off-by: Pete Wyckoff --- 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 ``` ## Johannes Sixt, 2013-03-08 06:42 Subject: Re: [PATCH 2/3] git p4 test: should honor symlink in p4 client root Message-ID: <5139883C.6080308@viscovery.net> URL: https://gitlist.dev/e/5139883C.6080308%40viscovery.net In-Reply-To: <1362698357-7334-3-git-send-email-pw@padd.com> ``` 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 ``` ## Pete Wyckoff, 2013-03-11 21:45 Subject: [PATCH v2 0/3] fix git-p4 client root symlink problems Message-ID: <1363038329-20185-1-git-send-email-pw@padd.com> URL: https://gitlist.dev/e/1363038329-20185-1-git-send-email-pw%40padd.com In-Reply-To: <5139883C.6080308@viscovery.net> ``` 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 ``` ## Pete Wyckoff, 2013-03-11 21:45 Subject: [PATCH v2 1/3] git p4 test: make sure P4CONFIG relative path works Message-ID: <1363038329-20185-2-git-send-email-pw@padd.com> URL: https://gitlist.dev/e/1363038329-20185-2-git-send-email-pw%40padd.com In-Reply-To: <1363038329-20185-1-git-send-email-pw@padd.com> ``` 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 --- 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 ``` ## Pete Wyckoff, 2013-03-11 21:45 Subject: [PATCH v2 2/3] git p4 test: should honor symlink in p4 client root Message-ID: <1363038329-20185-3-git-send-email-pw@padd.com> URL: https://gitlist.dev/e/1363038329-20185-3-git-send-email-pw%40padd.com In-Reply-To: <1363038329-20185-1-git-send-email-pw@padd.com> ``` 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 --- 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 ``` ## Pete Wyckoff, 2013-03-11 21:45 Subject: [PATCH v2 3/3] git p4: avoid expanding client paths in chdir Message-ID: <1363038329-20185-4-git-send-email-pw@padd.com> URL: https://gitlist.dev/e/1363038329-20185-4-git-send-email-pw%40padd.com In-Reply-To: <1363038329-20185-1-git-send-email-pw@padd.com> ``` From: Miklós Fazekas 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 Signed-off-by: Pete Wyckoff --- 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 ```