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

[Nuno Subtil <subtil@gmail.com>]

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
>>> >
>>> >
>>
>>