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

[John Keeping <john@keeping.me.uk>]

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)