[PATCH] git-p4: Fix depot path stripping

[PATCH] git-p4: Fix depot path stripping

[Nuno Subtil]

When useClientSpec=true, stripping of P4 depot paths doesn't happen
correctly on sync. Modifies stripRepoPath to handle this case better.

Signed-off-by: Nuno Subtil <subtil@gmail.com>
---
 git-p4.py | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 7bb9cadc69738..3df95d0fd1d83 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2480,7 +2480,7 @@ def stripRepoPath(self, path, prefixes):
                     if path.startswith(b + "/"):
                         path = path[len(b)+1:]
 
-        elif self.keepRepoPath:
+        if self.keepRepoPath:
             # Preserve everything in relative path name except leading
             # //depot/; just look at first prefix as they all should
             # be in the same depot.
@@ -2490,6 +2490,12 @@ def stripRepoPath(self, path, prefixes):
 
         else:
             for p in prefixes:
+		if self.useClientSpec and not self.keepRepoPath:
+                    # when useClientSpec is false, the prefix will contain the depot name but the path will not
+                    # extract the depot name and add it to the path so the match below will do the right thing
+                    depot = re.sub("^(//[^/]+/).*", r'\1', p)
+                    path = depot + path
+
                 if p4PathStartsWith(path, p):
                     path = path[len(p):]
                     break
@@ -2526,8 +2532,8 @@ def splitFilesIntoBranches(self, commit):
             # go in a p4 client
             if self.useClientSpec:
                 relPath = self.clientSpecDirs.map_in_client(path)
-            else:
-                relPath = self.stripRepoPath(path, self.depotPaths)
+
+            relPath = self.stripRepoPath(path, self.depotPaths)
 
             for branch in self.knownBranches.keys():
                 # add a trailing slash so that a commit into qt/4.2foo

--
https://github.com/git/git/pull/463

Re: [PATCH] git-p4: Fix depot path stripping

[Luke Diamand]

On 27 February 2018 at 04:16, Nuno Subtil <subtil@gmail.com> wrote:
> When useClientSpec=true, stripping of P4 depot paths doesn't happen
> correctly on sync. Modifies stripRepoPath to handle this case better.

Can you give an example of how this shows up? I could quickly write a
test case for this if I knew what was going on.

Thanks
Luke


>
> Signed-off-by: Nuno Subtil <subtil@gmail.com>
> ---
>  git-p4.py | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 7bb9cadc69738..3df95d0fd1d83 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2480,7 +2480,7 @@ def stripRepoPath(self, path, prefixes):
>                      if path.startswith(b + "/"):
>                          path = path[len(b)+1:]
>
> -        elif self.keepRepoPath:
> +        if self.keepRepoPath:
>              # Preserve everything in relative path name except leading
>              # //depot/; just look at first prefix as they all should
>              # be in the same depot.
> @@ -2490,6 +2490,12 @@ def stripRepoPath(self, path, prefixes):
>
>          else:
>              for p in prefixes:
> +               if self.useClientSpec and not self.keepRepoPath:
> +                    # when useClientSpec is false, the prefix will contain the depot name but the path will not
> +                    # extract the depot name and add it to the path so the match below will do the right thing
> +                    depot = re.sub("^(//[^/]+/).*", r'\1', p)
> +                    path = depot + path
> +
>                  if p4PathStartsWith(path, p):
>                      path = path[len(p):]
>                      break
> @@ -2526,8 +2532,8 @@ def splitFilesIntoBranches(self, commit):
>              # go in a p4 client
>              if self.useClientSpec:
>                  relPath = self.clientSpecDirs.map_in_client(path)
> -            else:
> -                relPath = self.stripRepoPath(path, self.depotPaths)
> +
> +            relPath = self.stripRepoPath(path, self.depotPaths)
>
>              for branch in self.knownBranches.keys():
>                  # add a trailing slash so that a commit into qt/4.2foo
>
> --
> https://github.com/git/git/pull/463

Re: [PATCH] git-p4: Fix depot path stripping

[Luke Diamand]

On 27 February 2018 at 08:43, Nuno Subtil <subtil@gmail.com> wrote:
> The issue is that passing in --use-client-spec will always cause git-p4 to
> preserve the full depot path in the working tree that it creates, even when
> --keep-path is not used.
>
> The repro case is fairly simple: cloning a given p4 path that is nested 2 or
> more levels below the depot root will have different results with and
> without --use-client-spec (even when the client spec is just a
> straightforward map of the entire depot).
>
> E.g., 'git p4 clone //p4depot/path/to/some/project/...' will create a
> working tree rooted at project. However, 'git p4 clone --use-client-spec
> //p4depot/path/to/some/project/...' will create a working tree rooted at the
> root of the depot.

I think it _might_ be by design.

At least, the test ./t9809-git-p4-client-view.sh seems to fail for me
with your change, although I haven't investigated what's going on:

$ ./t9809-git-p4-client-view.sh
...
...
Doing initial import of //depot/dir1/ from revision #head into
refs/remotes/p4/master
./t9809-git-p4-client-view.sh: 10: eval: cannot create dir1/file13:
Directory nonexistent
not ok 23 - subdir clone, submit add

I think originally the logic came from this change:

   21ef5df43 git p4: make branch detection work with --use-client-spec

which was fixing what seems like the same problem but with branch
detection enabled.


>
> Thanks,
> Nuno
>
>
> On Tue, Feb 27, 2018 at 12:10 AM, Luke Diamand <luke@diamand.org> wrote:
>>
>> On 27 February 2018 at 04:16, Nuno Subtil <subtil@gmail.com> wrote:
>> > When useClientSpec=true, stripping of P4 depot paths doesn't happen
>> > correctly on sync. Modifies stripRepoPath to handle this case better.
>>
>> Can you give an example of how this shows up? I could quickly write a
>> test case for this if I knew what was going on.
>>
>> Thanks
>> Luke
>>
>>
>> >
>> > Signed-off-by: Nuno Subtil <subtil@gmail.com>
>> > ---
>> >  git-p4.py | 12 +++++++++---
>> >  1 file changed, 9 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/git-p4.py b/git-p4.py
>> > index 7bb9cadc69738..3df95d0fd1d83 100755
>> > --- a/git-p4.py
>> > +++ b/git-p4.py
>> > @@ -2480,7 +2480,7 @@ def stripRepoPath(self, path, prefixes):
>> >                      if path.startswith(b + "/"):
>> >                          path = path[len(b)+1:]
>> >
>> > -        elif self.keepRepoPath:
>> > +        if self.keepRepoPath:
>> >              # Preserve everything in relative path name except leading
>> >              # //depot/; just look at first prefix as they all should
>> >              # be in the same depot.
>> > @@ -2490,6 +2490,12 @@ def stripRepoPath(self, path, prefixes):
>> >
>> >          else:
>> >              for p in prefixes:
>> > +               if self.useClientSpec and not self.keepRepoPath:
>> > +                    # when useClientSpec is false, the prefix will
>> > contain the depot name but the path will not
>> > +                    # extract the depot name and add it to the path so
>> > the match below will do the right thing
>> > +                    depot = re.sub("^(//[^/]+/).*", r'\1', p)
>> > +                    path = depot + path
>> > +
>> >                  if p4PathStartsWith(path, p):
>> >                      path = path[len(p):]
>> >                      break
>> > @@ -2526,8 +2532,8 @@ def splitFilesIntoBranches(self, commit):
>> >              # go in a p4 client
>> >              if self.useClientSpec:
>> >                  relPath = self.clientSpecDirs.map_in_client(path)
>> > -            else:
>> > -                relPath = self.stripRepoPath(path, self.depotPaths)
>> > +
>> > +            relPath = self.stripRepoPath(path, self.depotPaths)
>> >
>> >              for branch in self.knownBranches.keys():
>> >                  # add a trailing slash so that a commit into qt/4.2foo
>> >
>> > --
>> > https://github.com/git/git/pull/463
>
>

Re: [PATCH] git-p4: Fix depot path stripping

[Luke Diamand]

On 27 February 2018 at 19:00, Nuno Subtil <subtil@gmail.com> wrote:
> I originally thought this had been designed such that the p4 client spec
> determines the paths, which would make sense. However, git-p4 still ignores
> the local path mappings in the client spec when syncing p4 depot paths
> w/--use-client-spec. Effectively, it looks as though --use-client-spec
> always implies --keep-paths, which didn't seem to me like what was intended.
>
> For the use case I'm dealing with (syncing a p4 path but ignoring some
> directories inside it), there seems to be no way today to generate a git
> tree rooted at the p4 path location instead of the root of the depot, which
> looks like a bug to me. I don't really understand the overall design well
> enough to tell whether the bug lies in stripRepoPath or somewhere else, to
> be honest, but that's where I saw the inconsistency manifest itself.

I replied but managed to drop git-users off the thread. So trying again!

The behavior is a touch surprising, but I _think_ it's correct.

With --use-client-spec enabled, paths in the git repot get mapped as
if you had used the file mapping in the client spec, using "p4 where".

So, for example, if you have a client spec which looks like:
   //depot/... //my_client_spec/...

then you're going to get the full repo structure, even if you only
clone a subdirectory.

e.g. if you clone //depot/a/b/c then with "--use-client-spec" enabled,
you will get a/b/c in your git repo, and with it not enabled, you will
just get c/.

If instead your client spec looks like:
  //depot/a/b/... //my_client_spec/...

then you should only get c/d. (And a quick test confirms that).

I think Nuno's original question comes from wanting to map some files
into place which the clientspec mapping emulation in git-p4 was
possibly not handling - if we can get a test case for that (or an
example) then we can see if it's just that the client mapping code
that Pete put in is insufficient, or if there's some other way around.
Or if indeed I'm wrong, and there's a bug! There may be some weird
corner-cases where it might do the wrong thing.

Thanks,
Luke


>
> Nuno
>
>
> On Tue, Feb 27, 2018 at 3:12 AM, Luke Diamand <luke@diamand.org> wrote:
>>
>> On 27 February 2018 at 08:43, Nuno Subtil <subtil@gmail.com> wrote:
>> > The issue is that passing in --use-client-spec will always cause git-p4
>> > to
>> > preserve the full depot path in the working tree that it creates, even
>> > when
>> > --keep-path is not used.
>> >
>> > The repro case is fairly simple: cloning a given p4 path that is nested
>> > 2 or
>> > more levels below the depot root will have different results with and
>> > without --use-client-spec (even when the client spec is just a
>> > straightforward map of the entire depot).
>> >
>> > E.g., 'git p4 clone //p4depot/path/to/some/project/...' will create a
>> > working tree rooted at project. However, 'git p4 clone --use-client-spec
>> > //p4depot/path/to/some/project/...' will create a working tree rooted at
>> > the
>> > root of the depot.
>>
>> I think it _might_ be by design.
>>
>> At least, the test ./t9809-git-p4-client-view.sh seems to fail for me
>> with your change, although I haven't investigated what's going on:
>>
>> $ ./t9809-git-p4-client-view.sh
>> ...
>> ...
>> Doing initial import of //depot/dir1/ from revision #head into
>> refs/remotes/p4/master
>> ./t9809-git-p4-client-view.sh: 10: eval: cannot create dir1/file13:
>> Directory nonexistent
>> not ok 23 - subdir clone, submit add
>>
>> I think originally the logic came from this change:
>>
>>    21ef5df43 git p4: make branch detection work with --use-client-spec
>>
>> which was fixing what seems like the same problem but with branch
>> detection enabled.
>>
>>
>> >
>> > Thanks,
>> > Nuno
>> >
>> >
>> > On Tue, Feb 27, 2018 at 12:10 AM, Luke Diamand <luke@diamand.org> wrote:
>> >>
>> >> On 27 February 2018 at 04:16, Nuno Subtil <subtil@gmail.com> wrote:
>> >> > When useClientSpec=true, stripping of P4 depot paths doesn't happen
>> >> > correctly on sync. Modifies stripRepoPath to handle this case better.
>> >>
>> >> Can you give an example of how this shows up? I could quickly write a
>> >> test case for this if I knew what was going on.
>> >>
>> >> Thanks
>> >> Luke
>> >>
>> >>
>> >> >
>> >> > Signed-off-by: Nuno Subtil <subtil@gmail.com>
>> >> > ---
>> >> >  git-p4.py | 12 +++++++++---
>> >> >  1 file changed, 9 insertions(+), 3 deletions(-)
>> >> >
>> >> > diff --git a/git-p4.py b/git-p4.py
>> >> > index 7bb9cadc69738..3df95d0fd1d83 100755
>> >> > --- a/git-p4.py
>> >> > +++ b/git-p4.py
>> >> > @@ -2480,7 +2480,7 @@ def stripRepoPath(self, path, prefixes):
>> >> >                      if path.startswith(b + "/"):
>> >> >                          path = path[len(b)+1:]
>> >> >
>> >> > -        elif self.keepRepoPath:
>> >> > +        if self.keepRepoPath:
>> >> >              # Preserve everything in relative path name except
>> >> > leading
>> >> >              # //depot/; just look at first prefix as they all should
>> >> >              # be in the same depot.
>> >> > @@ -2490,6 +2490,12 @@ def stripRepoPath(self, path, prefixes):
>> >> >
>> >> >          else:
>> >> >              for p in prefixes:
>> >> > +               if self.useClientSpec and not self.keepRepoPath:
>> >> > +                    # when useClientSpec is false, the prefix will
>> >> > contain the depot name but the path will not
>> >> > +                    # extract the depot name and add it to the path
>> >> > so
>> >> > the match below will do the right thing
>> >> > +                    depot = re.sub("^(//[^/]+/).*", r'\1', p)
>> >> > +                    path = depot + path
>> >> > +
>> >> >                  if p4PathStartsWith(path, p):
>> >> >                      path = path[len(p):]
>> >> >                      break
>> >> > @@ -2526,8 +2532,8 @@ def splitFilesIntoBranches(self, commit):
>> >> >              # go in a p4 client
>> >> >              if self.useClientSpec:
>> >> >                  relPath = self.clientSpecDirs.map_in_client(path)
>> >> > -            else:
>> >> > -                relPath = self.stripRepoPath(path, self.depotPaths)
>> >> > +
>> >> > +            relPath = self.stripRepoPath(path, self.depotPaths)
>> >> >
>> >> >              for branch in self.knownBranches.keys():
>> >> >                  # add a trailing slash so that a commit into
>> >> > qt/4.2foo
>> >> >
>> >> > --
>> >> > https://github.com/git/git/pull/463
>> >
>> >
>
>

Re: [PATCH] git-p4: Fix depot path stripping

[Nuno Subtil]

Yes, that's where we left off. I owe you better testing of the
clientspec-side path remapping case, which I hope to get to soon. Will
get back to you as soon as I can.

Thanks,
Nuno


On Fri, Mar 2, 2018 at 2:09 AM, Luke Diamand <luke@diamand.org> wrote:
> On 27 February 2018 at 19:00, Nuno Subtil <subtil@gmail.com> wrote:
>> I originally thought this had been designed such that the p4 client spec
>> determines the paths, which would make sense. However, git-p4 still ignores
>> the local path mappings in the client spec when syncing p4 depot paths
>> w/--use-client-spec. Effectively, it looks as though --use-client-spec
>> always implies --keep-paths, which didn't seem to me like what was intended.
>>
>> For the use case I'm dealing with (syncing a p4 path but ignoring some
>> directories inside it), there seems to be no way today to generate a git
>> tree rooted at the p4 path location instead of the root of the depot, which
>> looks like a bug to me. I don't really understand the overall design well
>> enough to tell whether the bug lies in stripRepoPath or somewhere else, to
>> be honest, but that's where I saw the inconsistency manifest itself.
>
> I replied but managed to drop git-users off the thread. So trying again!
>
> The behavior is a touch surprising, but I _think_ it's correct.
>
> With --use-client-spec enabled, paths in the git repot get mapped as
> if you had used the file mapping in the client spec, using "p4 where".
>
> So, for example, if you have a client spec which looks like:
>    //depot/... //my_client_spec/...
>
> then you're going to get the full repo structure, even if you only
> clone a subdirectory.
>
> e.g. if you clone //depot/a/b/c then with "--use-client-spec" enabled,
> you will get a/b/c in your git repo, and with it not enabled, you will
> just get c/.
>
> If instead your client spec looks like:
>   //depot/a/b/... //my_client_spec/...
>
> then you should only get c/d. (And a quick test confirms that).
>
> I think Nuno's original question comes from wanting to map some files
> into place which the clientspec mapping emulation in git-p4 was
> possibly not handling - if we can get a test case for that (or an
> example) then we can see if it's just that the client mapping code
> that Pete put in is insufficient, or if there's some other way around.
> Or if indeed I'm wrong, and there's a bug! There may be some weird
> corner-cases where it might do the wrong thing.
>
> Thanks,
> Luke
>
>
>>
>> Nuno
>>
>>
>> On Tue, Feb 27, 2018 at 3:12 AM, Luke Diamand <luke@diamand.org> wrote:
>>>
>>> On 27 February 2018 at 08:43, Nuno Subtil <subtil@gmail.com> wrote:
>>> > The issue is that passing in --use-client-spec will always cause git-p4
>>> > to
>>> > preserve the full depot path in the working tree that it creates, even
>>> > when
>>> > --keep-path is not used.
>>> >
>>> > The repro case is fairly simple: cloning a given p4 path that is nested
>>> > 2 or
>>> > more levels below the depot root will have different results with and
>>> > without --use-client-spec (even when the client spec is just a
>>> > straightforward map of the entire depot).
>>> >
>>> > E.g., 'git p4 clone //p4depot/path/to/some/project/...' will create a
>>> > working tree rooted at project. However, 'git p4 clone --use-client-spec
>>> > //p4depot/path/to/some/project/...' will create a working tree rooted at
>>> > the
>>> > root of the depot.
>>>
>>> I think it _might_ be by design.
>>>
>>> At least, the test ./t9809-git-p4-client-view.sh seems to fail for me
>>> with your change, although I haven't investigated what's going on:
>>>
>>> $ ./t9809-git-p4-client-view.sh
>>> ...
>>> ...
>>> Doing initial import of //depot/dir1/ from revision #head into
>>> refs/remotes/p4/master
>>> ./t9809-git-p4-client-view.sh: 10: eval: cannot create dir1/file13:
>>> Directory nonexistent
>>> not ok 23 - subdir clone, submit add
>>>
>>> I think originally the logic came from this change:
>>>
>>>    21ef5df43 git p4: make branch detection work with --use-client-spec
>>>
>>> which was fixing what seems like the same problem but with branch
>>> detection enabled.
>>>
>>>
>>> >
>>> > Thanks,
>>> > Nuno
>>> >
>>> >
>>> > On Tue, Feb 27, 2018 at 12:10 AM, Luke Diamand <luke@diamand.org> wrote:
>>> >>
>>> >> On 27 February 2018 at 04:16, Nuno Subtil <subtil@gmail.com> wrote:
>>> >> > When useClientSpec=true, stripping of P4 depot paths doesn't happen
>>> >> > correctly on sync. Modifies stripRepoPath to handle this case better.
>>> >>
>>> >> Can you give an example of how this shows up? I could quickly write a
>>> >> test case for this if I knew what was going on.
>>> >>
>>> >> Thanks
>>> >> Luke
>>> >>
>>> >>
>>> >> >
>>> >> > Signed-off-by: Nuno Subtil <subtil@gmail.com>
>>> >> > ---
>>> >> >  git-p4.py | 12 +++++++++---
>>> >> >  1 file changed, 9 insertions(+), 3 deletions(-)
>>> >> >
>>> >> > diff --git a/git-p4.py b/git-p4.py
>>> >> > index 7bb9cadc69738..3df95d0fd1d83 100755
>>> >> > --- a/git-p4.py
>>> >> > +++ b/git-p4.py
>>> >> > @@ -2480,7 +2480,7 @@ def stripRepoPath(self, path, prefixes):
>>> >> >                      if path.startswith(b + "/"):
>>> >> >                          path = path[len(b)+1:]
>>> >> >
>>> >> > -        elif self.keepRepoPath:
>>> >> > +        if self.keepRepoPath:
>>> >> >              # Preserve everything in relative path name except
>>> >> > leading
>>> >> >              # //depot/; just look at first prefix as they all should
>>> >> >              # be in the same depot.
>>> >> > @@ -2490,6 +2490,12 @@ def stripRepoPath(self, path, prefixes):
>>> >> >
>>> >> >          else:
>>> >> >              for p in prefixes:
>>> >> > +               if self.useClientSpec and not self.keepRepoPath:
>>> >> > +                    # when useClientSpec is false, the prefix will
>>> >> > contain the depot name but the path will not
>>> >> > +                    # extract the depot name and add it to the path
>>> >> > so
>>> >> > the match below will do the right thing
>>> >> > +                    depot = re.sub("^(//[^/]+/).*", r'\1', p)
>>> >> > +                    path = depot + path
>>> >> > +
>>> >> >                  if p4PathStartsWith(path, p):
>>> >> >                      path = path[len(p):]
>>> >> >                      break
>>> >> > @@ -2526,8 +2532,8 @@ def splitFilesIntoBranches(self, commit):
>>> >> >              # go in a p4 client
>>> >> >              if self.useClientSpec:
>>> >> >                  relPath = self.clientSpecDirs.map_in_client(path)
>>> >> > -            else:
>>> >> > -                relPath = self.stripRepoPath(path, self.depotPaths)
>>> >> > +
>>> >> > +            relPath = self.stripRepoPath(path, self.depotPaths)
>>> >> >
>>> >> >              for branch in self.knownBranches.keys():
>>> >> >                  # add a trailing slash so that a commit into
>>> >> > qt/4.2foo
>>> >> >
>>> >> > --
>>> >> > https://github.com/git/git/pull/463
>>> >
>>> >
>>
>>