* [PATCH] git-p4: Fix depot path stripping @ 2018-02-27 4:16 Nuno Subtil 2018-02-27 8:10 ` Luke Diamand 0 siblings, 1 reply; 5+ messages in thread From: Nuno Subtil @ 2018-02-27 4:16 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] git-p4: Fix depot path stripping 2018-02-27 4:16 [PATCH] git-p4: Fix depot path stripping Nuno Subtil @ 2018-02-27 8:10 ` Luke Diamand [not found] ` <CALdXDfcU0gApmfjRbxecGwEwDN1rP4PUqGhPmgbzTuhc3SiWBg@mail.gmail.com> 0 siblings, 1 reply; 5+ messages in thread From: Luke Diamand @ 2018-02-27 8:10 UTC (permalink / raw) To: Nuno Subtil; +Cc: Git Users 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <CALdXDfcU0gApmfjRbxecGwEwDN1rP4PUqGhPmgbzTuhc3SiWBg@mail.gmail.com>]
* Re: [PATCH] git-p4: Fix depot path stripping [not found] ` <CALdXDfcU0gApmfjRbxecGwEwDN1rP4PUqGhPmgbzTuhc3SiWBg@mail.gmail.com> @ 2018-02-27 11:12 ` Luke Diamand [not found] ` <CALdXDfcdj+zqd=R2PXsZ0GhE=H+Q8UAr+8w8397_M8xH+5td+g@mail.gmail.com> 0 siblings, 1 reply; 5+ messages in thread From: Luke Diamand @ 2018-02-27 11:12 UTC (permalink / raw) To: Nuno Subtil; +Cc: Git Users, Pete Wyckoff 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <CALdXDfcdj+zqd=R2PXsZ0GhE=H+Q8UAr+8w8397_M8xH+5td+g@mail.gmail.com>]
* Re: [PATCH] git-p4: Fix depot path stripping [not found] ` <CALdXDfcdj+zqd=R2PXsZ0GhE=H+Q8UAr+8w8397_M8xH+5td+g@mail.gmail.com> @ 2018-03-02 10:09 ` Luke Diamand 2018-03-02 20:12 ` Nuno Subtil 0 siblings, 1 reply; 5+ messages in thread From: Luke Diamand @ 2018-03-02 10:09 UTC (permalink / raw) To: Nuno Subtil; +Cc: Git Users, Pete Wyckoff 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 >> > >> > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] git-p4: Fix depot path stripping 2018-03-02 10:09 ` Luke Diamand @ 2018-03-02 20:12 ` Nuno Subtil 0 siblings, 0 replies; 5+ messages in thread From: Nuno Subtil @ 2018-03-02 20:12 UTC (permalink / raw) To: Luke Diamand; +Cc: Git Users, Pete Wyckoff 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 >>> > >>> > >> >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-03-02 20:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-27 4:16 [PATCH] git-p4: Fix depot path stripping Nuno Subtil
2018-02-27 8:10 ` Luke Diamand
[not found] ` <CALdXDfcU0gApmfjRbxecGwEwDN1rP4PUqGhPmgbzTuhc3SiWBg@mail.gmail.com>
2018-02-27 11:12 ` Luke Diamand
[not found] ` <CALdXDfcdj+zqd=R2PXsZ0GhE=H+Q8UAr+8w8397_M8xH+5td+g@mail.gmail.com>
2018-03-02 10:09 ` Luke Diamand
2018-03-02 20:12 ` Nuno Subtil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).