* [PATCH] Support different branch layouts in git-p4 @ 2011-02-01 22:59 Ian Wienand 2011-02-05 0:37 ` Tor Arvid Lund 0 siblings, 1 reply; 7+ messages in thread From: Ian Wienand @ 2011-02-01 22:59 UTC (permalink / raw) To: git Hi, I think the addition to the git-p4.txt in the diff explains the reasoning behind the patch best. In short, we have a repository layout //depot/foo/branch //depot/moo/branch where we require projects 'foo' and 'moo' to be alongside each other. We can do this with p4 views, but currently have to have 'foo' and 'moo' in separate git repos. This just munges the incoming paths to either put the branch as the top level directory, or just remove it entirely if you don't need it. I've tested it locally, but I don't really have a wide variety of p4 environments to expose it too. -i Signed-off-by: Ian Wienand <ianw@vmware.com> --- contrib/fast-import/git-p4 | 35 +++++++++++++++++++++++- contrib/fast-import/git-p4.txt | 58 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletions(-) diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4 index 04ce7e3..4bd40f8 100755 --- a/contrib/fast-import/git-p4 +++ b/contrib/fast-import/git-p4 @@ -848,6 +848,10 @@ class P4Sync(Command): optparse.make_option("--max-changes", dest="maxChanges"), optparse.make_option("--keep-path", dest="keepRepoPath", action='store_true', help="Keep entire BRANCH/DIR/SUBDIR prefix during import"), + optparse.make_option("--branch-path", dest="branchPath", type='choice', + choices=('none', 'first'), + default=None, + help="Remove the branch dir (none) or move it above project dir (first)"), optparse.make_option("--use-client-spec", dest="useClientSpec", action='store_true', help="Only sync files that are included in the Perforce Client Spec") ] @@ -917,6 +921,20 @@ class P4Sync(Command): if path.startswith(p): path = path[len(p):] + # reorg to move/remove branch from the output filename -- kind + # of like how you can set your view in your p4 client + if self.keepRepoPath and self.branchPath == 'first': + # move the second element first, so what was was + # "//depot/proj/branch/file" becomes "branch/proj/file". + path = re.sub("^([^/]+/)([^/]+/)", r'\2\1', path) + elif self.keepRepoPath and self.branchPath == 'none': + # remove the second element, so what was + # "//depot/proj/branch/file" becomes "proj/file" + path = re.sub("^([^/]+/)([^/]+/)", r'\2', path) + elif self.branchPath: + sys.stderr.write("branchPath without keepRepoPath?") + sys.exit(1) + return path def splitFilesIntoBranches(self, commit): @@ -940,7 +958,6 @@ class P4Sync(Command): relPath = self.stripRepoPath(path, self.depotPaths) for branch in self.knownBranches.keys(): - # add a trailing slash so that a commit into qt/4.2foo doesn't end up in qt/4.2 if relPath.startswith(branch + "/"): if branch not in branches: @@ -1283,12 +1300,24 @@ class P4Sync(Command): if self.keepRepoPath: option_keys['keepRepoPath'] = 1 + # since we're just saving the dict keys, append the branchPath + # option to the key + if self.branchPath: + option_keys['branchPath_%s' % self.branchPath] = 1 + d["options"] = ' '.join(sorted(option_keys.keys())) def readOptions(self, d): self.keepRepoPath = (d.has_key('options') and ('keepRepoPath' in d['options'])) + # restore the branchpath option; is one of "none" and "first" + if (d.has_key('options')): + if ('branchPath_none' in d['options']): + self.branchPath = 'none' + elif ('branchPath_first' in d['options']): + self.branchPath = 'first' + def gitRefForBranch(self, branch): if branch == "main": return self.refPrefix + "master" @@ -1775,6 +1804,10 @@ class P4Clone(P4Sync): sys.stderr.write("Must specify destination for --keep-path\n") sys.exit(1) + if self.branchPath and not self.keepRepoPath: + sys.stderr.write("Must specify --keep-path for --branch-path\n") + sys.exit(1) + depotPaths = args if not self.cloneDestination and len(depotPaths) > 1: diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt index 49b3359..669c63c 100644 --- a/contrib/fast-import/git-p4.txt +++ b/contrib/fast-import/git-p4.txt @@ -191,6 +191,64 @@ git-p4.useclientspec git config [--global] git-p4.useclientspec false +Dealing with different repository layouts +========================================= + +Perforce clients can map views of projects and branches in different +ways which your build system may rely on. Say your code is organised +as two projects "foo" and "moo" which have a common branch + +//depot/foo/branch/... +//depot/moo/branch/... + +and you require both "foo" and "moo" projects in your git repository, +there are several options. + +Firstly, you could simply clone each project as a completely separate +git tree. However, if the two projects are dependent on each other +this can be annoying for both sync -- you must remember to sync both +"foo" and "moo" to keep everything consistent -- and submit -- a +change that should logically be a single changeset across "foo" and +"moo" will have to be broken up (breaking bisection too). + +Another option is to simply specify multiple depots + + git p4 sync //depot/foo/branch //depot/moo/branch + +which will import "foo" and "moo" into the same directory. + +To keep the projects separate, the --keep-path option used as + + git p4 sync --keep-path --destination /tmp/boo/ //depot/foo/branch //depot/moo/branch + +will create a layout of + + /tmp/boo/foo/branch/... + /tmp/boo/moo/branch/... + +However, some build systems may rely on p4's ability to specify +destinations for views in your client. The --branch-path flag, which +requires the --keep-path flag, allows two additional layout options. + + git p4 sync --keep-path --destination /tmp/boo --branch-path=none //depot/foo/branch //depot/moo/branch + +will remove the branch name entirely, leaving you with a directory +that looks like + + /tmp/boo/foo/... + /tmp/boo/moo/... + +and + + git p4 sync --keep-path --destination /tmp/boo --branch-path=first //depot/foo/branch //depot/moo/branch + +will give you each of the projects under a directory named for their +common branch + + /tmp/boo/branch/foo/... + /tmp/boo/branch/moo/... + + Implementation Details... ========================= -- 1.7.2.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Support different branch layouts in git-p4 2011-02-01 22:59 [PATCH] Support different branch layouts in git-p4 Ian Wienand @ 2011-02-05 0:37 ` Tor Arvid Lund 2011-02-07 0:05 ` Ian Wienand 0 siblings, 1 reply; 7+ messages in thread From: Tor Arvid Lund @ 2011-02-05 0:37 UTC (permalink / raw) To: Ian Wienand; +Cc: git On Tue, Feb 1, 2011 at 11:59 PM, Ian Wienand <ianw@vmware.com> wrote: > Hi, > > I think the addition to the git-p4.txt in the diff explains the > reasoning behind the patch best. In short, we have a repository > layout > > //depot/foo/branch > //depot/moo/branch > > where we require projects 'foo' and 'moo' to be alongside each other. > We can do this with p4 views, but currently have to have 'foo' and > 'moo' in separate git repos. > > This just munges the incoming paths to either put the branch as the > top level directory, or just remove it entirely if you don't need it. Hi, Ian! We haven't met, but thank you for the patch, and for trying to help make git better. Now, I have to say that I don't particularly like it, and here's why I will vote against this patch: For starters, I don't think that I like git-p4 being taught to solve problems that seem to be caused by a poor/unfortunate perforce layout. Especially since *your* type of poor perforce layout will probably be poor in a very different way from the next guy with a poor layout :) For instance, you have hard-coded that you replace the first and second directory name... It is very easy to imagine people having deeper trees than that... But then I started thinking about it a bit more... It was me who added the --use-client-spec option back in the day. The support for that stuff really should be better than what I made at the time. The client-spec format contains lines like //depot/... //local-root/... -//depot/dontcare/... //local-root/dontcare/... //depot/foo/branch/... //local-root/branch/foo/... //depot/moo/branch/... //local-root/branch/moo/... Please observe that the two last lines look like what I think *your* client-spec should look like. This would map the foo/branch and moo/branch in the perforce depot to branch/foo and branch/moo on the client side. Of course, today this will not work with git-p4 clone. The --use-client-spec option, as I implemented it, simply filters out all that stuff that matches the pattern lines that starts with "-". So the names of all files will match the patterns on the left-hand side in the client-spec. A solution which I think would work well for everyone, is if files would be placed according to the right-hand patterns in the client-spec. That should be a much more elegant and generic solution. Whatcha think? If you want to take a whack at hacking that into place, I will help guide the way if needed (if others are not opposed to such an idea) :) -- Tor Arvid > I've tested it locally, but I don't really have a wide variety of p4 > environments to expose it too. > > -i > > Signed-off-by: Ian Wienand <ianw@vmware.com> > --- > contrib/fast-import/git-p4 | 35 +++++++++++++++++++++++- > contrib/fast-import/git-p4.txt | 58 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 92 insertions(+), 1 deletions(-) > > diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4 > index 04ce7e3..4bd40f8 100755 > --- a/contrib/fast-import/git-p4 > +++ b/contrib/fast-import/git-p4 > @@ -848,6 +848,10 @@ class P4Sync(Command): > optparse.make_option("--max-changes", dest="maxChanges"), > optparse.make_option("--keep-path", dest="keepRepoPath", action='store_true', > help="Keep entire BRANCH/DIR/SUBDIR prefix during import"), > + optparse.make_option("--branch-path", dest="branchPath", type='choice', > + choices=('none', 'first'), > + default=None, > + help="Remove the branch dir (none) or move it above project dir (first)"), > optparse.make_option("--use-client-spec", dest="useClientSpec", action='store_true', > help="Only sync files that are included in the Perforce Client Spec") > ] > @@ -917,6 +921,20 @@ class P4Sync(Command): > if path.startswith(p): > path = path[len(p):] > > + # reorg to move/remove branch from the output filename -- kind > + # of like how you can set your view in your p4 client > + if self.keepRepoPath and self.branchPath == 'first': > + # move the second element first, so what was was > + # "//depot/proj/branch/file" becomes "branch/proj/file". > + path = re.sub("^([^/]+/)([^/]+/)", r'\2\1', path) > + elif self.keepRepoPath and self.branchPath == 'none': > + # remove the second element, so what was > + # "//depot/proj/branch/file" becomes "proj/file" > + path = re.sub("^([^/]+/)([^/]+/)", r'\2', path) > + elif self.branchPath: > + sys.stderr.write("branchPath without keepRepoPath?") > + sys.exit(1) > + > return path > > def splitFilesIntoBranches(self, commit): > @@ -940,7 +958,6 @@ class P4Sync(Command): > relPath = self.stripRepoPath(path, self.depotPaths) > > for branch in self.knownBranches.keys(): > - > # add a trailing slash so that a commit into qt/4.2foo doesn't end up in qt/4.2 > if relPath.startswith(branch + "/"): > if branch not in branches: > @@ -1283,12 +1300,24 @@ class P4Sync(Command): > if self.keepRepoPath: > option_keys['keepRepoPath'] = 1 > > + # since we're just saving the dict keys, append the branchPath > + # option to the key > + if self.branchPath: > + option_keys['branchPath_%s' % self.branchPath] = 1 > + > d["options"] = ' '.join(sorted(option_keys.keys())) > > def readOptions(self, d): > self.keepRepoPath = (d.has_key('options') > and ('keepRepoPath' in d['options'])) > > + # restore the branchpath option; is one of "none" and "first" > + if (d.has_key('options')): > + if ('branchPath_none' in d['options']): > + self.branchPath = 'none' > + elif ('branchPath_first' in d['options']): > + self.branchPath = 'first' > + > def gitRefForBranch(self, branch): > if branch == "main": > return self.refPrefix + "master" > @@ -1775,6 +1804,10 @@ class P4Clone(P4Sync): > sys.stderr.write("Must specify destination for --keep-path\n") > sys.exit(1) > > + if self.branchPath and not self.keepRepoPath: > + sys.stderr.write("Must specify --keep-path for --branch-path\n") > + sys.exit(1) > + > depotPaths = args > > if not self.cloneDestination and len(depotPaths) > 1: > diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt > index 49b3359..669c63c 100644 > --- a/contrib/fast-import/git-p4.txt > +++ b/contrib/fast-import/git-p4.txt > @@ -191,6 +191,64 @@ git-p4.useclientspec > > git config [--global] git-p4.useclientspec false > > +Dealing with different repository layouts > +========================================= > + > +Perforce clients can map views of projects and branches in different > +ways which your build system may rely on. Say your code is organised > +as two projects "foo" and "moo" which have a common branch > + > +//depot/foo/branch/... > +//depot/moo/branch/... > + > +and you require both "foo" and "moo" projects in your git repository, > +there are several options. > + > +Firstly, you could simply clone each project as a completely separate > +git tree. However, if the two projects are dependent on each other > +this can be annoying for both sync -- you must remember to sync both > +"foo" and "moo" to keep everything consistent -- and submit -- a > +change that should logically be a single changeset across "foo" and > +"moo" will have to be broken up (breaking bisection too). > + > +Another option is to simply specify multiple depots > + > + git p4 sync //depot/foo/branch //depot/moo/branch > + > +which will import "foo" and "moo" into the same directory. > + > +To keep the projects separate, the --keep-path option used as > + > + git p4 sync --keep-path --destination /tmp/boo/ //depot/foo/branch //depot/moo/branch > + > +will create a layout of > + > + /tmp/boo/foo/branch/... > + /tmp/boo/moo/branch/... > + > +However, some build systems may rely on p4's ability to specify > +destinations for views in your client. The --branch-path flag, which > +requires the --keep-path flag, allows two additional layout options. > + > + git p4 sync --keep-path --destination /tmp/boo --branch-path=none //depot/foo/branch //depot/moo/branch > + > +will remove the branch name entirely, leaving you with a directory > +that looks like > + > + /tmp/boo/foo/... > + /tmp/boo/moo/... > + > +and > + > + git p4 sync --keep-path --destination /tmp/boo --branch-path=first //depot/foo/branch //depot/moo/branch > + > +will give you each of the projects under a directory named for their > +common branch > + > + /tmp/boo/branch/foo/... > + /tmp/boo/branch/moo/... > + > + > Implementation Details... > ========================= > > -- > 1.7.2.3 > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Support different branch layouts in git-p4 2011-02-05 0:37 ` Tor Arvid Lund @ 2011-02-07 0:05 ` Ian Wienand 2011-02-07 23:27 ` Tor Arvid Lund 2011-02-08 1:22 ` Pete Wyckoff 0 siblings, 2 replies; 7+ messages in thread From: Ian Wienand @ 2011-02-07 0:05 UTC (permalink / raw) To: Tor Arvid Lund; +Cc: git Thanks for taking a look On 04/02/11 16:37, Tor Arvid Lund wrote: > For starters, I don't think that I like git-p4 being taught to solve > problems that seem to be caused by a poor/unfortunate perforce layout. I do think this //depot/project/branch type layout is pretty typical, although I admit I don't have a lot of experience with alternative p4 setups. > A solution which I think would work well for everyone, is if files > would be placed according to the right-hand patterns in the > client-spec. I did consider this at first. My only issue is that it is a bit confusing to use the client spec for filtering (and in this case re-writing), but not for actually selecting the depots to clone, which I still need to replicate on the command line. However that is a much larger change. What do you think of this one? In this case, my client view is //depot/project/branch/... //client/branch/project/... //depot/project2/branch/... //client/branch/project2/... and my git directory layout ends up as branch/project/... branch/project2/... -i --- diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4 index 04ce7e3..eb9620c 100755 --- a/contrib/fast-import/git-p4 +++ b/contrib/fast-import/git-p4 @@ -878,6 +878,7 @@ class P4Sync(Command): self.cloneExclude = [] self.useClientSpec = False self.clientSpecDirs = [] + self.clientName = None if gitConfig("git-p4.syncFromOrigin") == "false": self.syncWithOrigin = False @@ -910,6 +911,22 @@ class P4Sync(Command): return files def stripRepoPath(self, path, prefixes): + if self.useClientSpec: + + # if using the client spec, we use the output directory + # specified in the client. For example, a view + # //depot/foo/branch/... //client/branch/foo/... + # will end up putting all foo/branch files into + # branch/foo/ + for val in self.clientSpecDirs: + if path.startswith(val[0]): + # replace the depot path with the client path + path = path.replace(val[0], val[1][1]) + # now strip out the client (//client/...) + path = re.sub("^(//[^/]+/)", '', path) + # the rest is all path + return path + if self.keepRepoPath: prefixes = [re.sub("^(//[^/]+/).*", r'\1', prefixes[0])] @@ -1032,7 +1049,7 @@ class P4Sync(Command): includeFile = True for val in self.clientSpecDirs: if f['path'].startswith(val[0]): - if val[1] <= 0: + if val[1][0] <= 0: includeFile = False break @@ -1474,20 +1491,36 @@ class P4Sync(Command): temp = {} for entry in specList: for k,v in entry.iteritems(): + if k.startswith("Client"): + self.clientName = v + if k.startswith("View"): if v.startswith('"'): start = 1 else: start = 0 index = v.find("...") + + # save the "client view"; i.e the RHS of the view + # line that tells the client where to put the + # files for this view. + cv = v[index+4:] # +4 to remove previous '... ' + cv_index = cv.find("...") + cv=cv[:cv_index] + + # now save the view; +index means included, -index + # means it should be filtered out. v = v[start:index] if v.startswith("-"): v = v[1:] - temp[v] = -len(v) + include = -len(v) else: - temp[v] = len(v) + include = len(v) + + temp[v] = (include, cv) + self.clientSpecDirs = temp.items() - self.clientSpecDirs.sort( lambda x, y: abs( y[1] ) - abs( x[1] ) ) + self.clientSpecDirs.sort( lambda x, y: abs( y[1][0] ) - abs( x[1][0] ) ) def run(self, args): self.depotPaths = [] diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt index 49b3359..e09da44 100644 --- a/contrib/fast-import/git-p4.txt +++ b/contrib/fast-import/git-p4.txt @@ -191,6 +191,11 @@ git-p4.useclientspec git config [--global] git-p4.useclientspec false +The P4CLIENT environment variable should be correctly set for p4 to be +able to find the relevant client. This client spec will be used to +both filter the files cloned by git and set the directory layout as +specified in the client (this implies --keep-path style semantics). + Implementation Details... ========================= ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Support different branch layouts in git-p4 2011-02-07 0:05 ` Ian Wienand @ 2011-02-07 23:27 ` Tor Arvid Lund 2011-02-09 3:46 ` Ian Wienand 2011-02-08 1:22 ` Pete Wyckoff 1 sibling, 1 reply; 7+ messages in thread From: Tor Arvid Lund @ 2011-02-07 23:27 UTC (permalink / raw) To: Ian Wienand; +Cc: git On Mon, Feb 7, 2011 at 1:05 AM, Ian Wienand <ianw@vmware.com> wrote: > On 04/02/11 16:37, Tor Arvid Lund wrote: >> For starters, I don't think that I like git-p4 being taught to solve >> problems that seem to be caused by a poor/unfortunate perforce layout. > > I do think this //depot/project/branch type layout is pretty typical, > although I admit I don't have a lot of experience with alternative p4 > setups. You may be right, although I suspect that //depot/department/project/branch may be equally typical. At my $dayjob, we have gone through several "reorganizations" of the perforce layouts. I'm the guy that never likes any of them ;) >> A solution which I think would work well for everyone, is if files >> would be placed according to the right-hand patterns in the >> client-spec. > > I did consider this at first. My only issue is that it is a bit > confusing to use the client spec for filtering (and in this case > re-writing), but not for actually selecting the depots to clone, which > I still need to replicate on the command line. However that is a much > larger change. > > What do you think of this one? In general, me thinks me likes it :-) (and it turned out much smaller than I would have originally guessed) I should probably mention that I haven't tested your patch at all. I will have a pretty rough week at work, so it would be great if anyone else feels like chiming in on this one... But I have some quick observations below: > --- > > diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4 > index 04ce7e3..eb9620c 100755 > --- a/contrib/fast-import/git-p4 > +++ b/contrib/fast-import/git-p4 > @@ -878,6 +878,7 @@ class P4Sync(Command): > self.cloneExclude = [] > self.useClientSpec = False > self.clientSpecDirs = [] > + self.clientName = None > > if gitConfig("git-p4.syncFromOrigin") == "false": > self.syncWithOrigin = False > @@ -910,6 +911,22 @@ class P4Sync(Command): > return files > > def stripRepoPath(self, path, prefixes): > + if self.useClientSpec: > + > + # if using the client spec, we use the output directory > + # specified in the client. For example, a view > + # //depot/foo/branch/... //client/branch/foo/... > + # will end up putting all foo/branch files into > + # branch/foo/ > + for val in self.clientSpecDirs: > + if path.startswith(val[0]): > + # replace the depot path with the client path > + path = path.replace(val[0], val[1][1]) > + # now strip out the client (//client/...) > + path = re.sub("^(//[^/]+/)", '', path) > + # the rest is all path > + return path > + > if self.keepRepoPath: > prefixes = [re.sub("^(//[^/]+/).*", r'\1', prefixes[0])] > > @@ -1032,7 +1049,7 @@ class P4Sync(Command): > includeFile = True > for val in self.clientSpecDirs: > if f['path'].startswith(val[0]): > - if val[1] <= 0: > + if val[1][0] <= 0: > includeFile = False > break > > @@ -1474,20 +1491,36 @@ class P4Sync(Command): > temp = {} > for entry in specList: > for k,v in entry.iteritems(): > + if k.startswith("Client"): > + self.clientName = v > + > if k.startswith("View"): > if v.startswith('"'): > start = 1 > else: > start = 0 > index = v.find("...") > + > + # save the "client view"; i.e the RHS of the view > + # line that tells the client where to put the > + # files for this view. > + cv = v[index+4:] # +4 to remove previous '... ' This feels less robust than what we might want. Isn't the format of a client-spec line either: -?//depot/path[/...]\s+//client/path[/...]\n or -?"//depot/path with spaces/path[/...]"\s+"//client/path with spaces/path[/...]" .. where -? means an optional '-' char, and \s+ is 'whatever-length-and-kind-of-whitespace'. I'm just guessing from memory regarding these patterns, but assuming that the section separator is exactly the string '... ' seems risky, no? :) > + cv_index = cv.find("...") > + cv=cv[:cv_index] What if a line doesn't end with "..." ? Maybe add an "if cv_index >= 0" > + > + # now save the view; +index means included, -index > + # means it should be filtered out. > v = v[start:index] > if v.startswith("-"): > v = v[1:] > - temp[v] = -len(v) > + include = -len(v) > else: > - temp[v] = len(v) > + include = len(v) > + > + temp[v] = (include, cv) > + > self.clientSpecDirs = temp.items() > - self.clientSpecDirs.sort( lambda x, y: abs( y[1] ) - abs( x[1] ) ) > + self.clientSpecDirs.sort( lambda x, y: abs( y[1][0] ) - abs( x[1][0] ) ) > > def run(self, args): > self.depotPaths = [] > diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt > index 49b3359..e09da44 100644 > --- a/contrib/fast-import/git-p4.txt > +++ b/contrib/fast-import/git-p4.txt > @@ -191,6 +191,11 @@ git-p4.useclientspec > > git config [--global] git-p4.useclientspec false > > +The P4CLIENT environment variable should be correctly set for p4 to be > +able to find the relevant client. This client spec will be used to > +both filter the files cloned by git and set the directory layout as > +specified in the client (this implies --keep-path style semantics). > + > Implementation Details... > ========================= > > Aight. I need some sleep now. Nice work so far, Ian! :) -- Tor Arvid ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Support different branch layouts in git-p4 2011-02-07 23:27 ` Tor Arvid Lund @ 2011-02-09 3:46 ` Ian Wienand 2011-02-10 13:43 ` Pete Wyckoff 0 siblings, 1 reply; 7+ messages in thread From: Ian Wienand @ 2011-02-09 3:46 UTC (permalink / raw) To: Tor Arvid Lund, Pete Wyckoff; +Cc: git Thanks for the review On 07/02/11 15:27, Tor Arvid Lund wrote: > I'm just guessing from memory regarding these patterns, but assuming > that the section separator is exactly the string '... ' seems risky, > no? :) Good point. If we go past the end of the depot ...'s, then the rest of the line strip()ed should just be the client I guess. > What if a line doesn't end with "..." ? Maybe add an "if cv_index>= 0" I think it would mess us up. I put in a failure case for this. On 07/02/11 17:22, Pete Wyckoff wrote: > If you look at "p4 help views", they have lots of oddities > that in theory should be accounted for here. It doesn't > even mention the thing about quotes, but obviously that is > supported. Wildcards ... and * can appear multiple > times. And %%[1-9] can be used to reorder the path. Also the > order of lines matters, and "+" can be used to merge entries. > Whew. Those %%'s would also mess us up, I put in an escape hatch for that too. I'm pretty sure this covers the majority of cases; if people have really weird clients I guess they're going to have to do some more work to get git-p4 to recognise it properly. Personally, I struggle to see why it is a feature to have every user re-organsing their views of the tree -- it seems to move a lot of uncaptured state to the client side. anyway... So here's another version; I agree some testing would be good as I've only run it locally on //depot/proj/branch clients -i --- diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4 index 04ce7e3..3304f36 100755 --- a/contrib/fast-import/git-p4 +++ b/contrib/fast-import/git-p4 @@ -910,6 +910,22 @@ class P4Sync(Command): return files def stripRepoPath(self, path, prefixes): + if self.useClientSpec: + + # if using the client spec, we use the output directory + # specified in the client. For example, a view + # //depot/foo/branch/... //client/branch/foo/... + # will end up putting all foo/branch files into + # branch/foo/ + for val in self.clientSpecDirs: + if path.startswith(val[0]): + # replace the depot path with the client path + path = path.replace(val[0], val[1][1]) + # now strip out the client (//client/...) + path = re.sub("^(//[^/]+/)", '', path) + # the rest is all path + return path + if self.keepRepoPath: prefixes = [re.sub("^(//[^/]+/).*", r'\1', prefixes[0])] @@ -1032,7 +1048,7 @@ class P4Sync(Command): includeFile = True for val in self.clientSpecDirs: if f['path'].startswith(val[0]): - if val[1] <= 0: + if val[1][0] <= 0: includeFile = False break @@ -1475,19 +1491,45 @@ class P4Sync(Command): for entry in specList: for k,v in entry.iteritems(): if k.startswith("View"): + + # p4 has these %%1 to %%9 arguments in specs to + # reorder paths; which we can't handle (yet :) + if re.match('\%\%d', v) != None: + print "Sorry, can't handle %%n arguments in client specs" + sys.exit(1) + if v.startswith('"'): start = 1 else: start = 0 index = v.find("...") + + # save the "client view"; i.e the RHS of the view + # line that tells the client where to put the + # files for this view. + cv = v[index+3:].strip() # +3 to remove previous '...' + + # if the client view doesn't end with a + # ... wildcard, then we're going to mess up the + # output directory, so fail gracefully. + if not cv.endswith('...'): + print 'Sorry, client view in "%s" needs to end with wildcard' % (k) + sys.exit(1) + cv=cv[:-3] + + # now save the view; +index means included, -index + # means it should be filtered out. v = v[start:index] if v.startswith("-"): v = v[1:] - temp[v] = -len(v) + include = -len(v) else: - temp[v] = len(v) + include = len(v) + + temp[v] = (include, cv) + self.clientSpecDirs = temp.items() - self.clientSpecDirs.sort( lambda x, y: abs( y[1] ) - abs( x[1] ) ) + self.clientSpecDirs.sort( lambda x, y: abs( y[1][0] ) - abs( x[1][0] ) ) def run(self, args): self.depotPaths = [] diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt index 49b3359..e09da44 100644 --- a/contrib/fast-import/git-p4.txt +++ b/contrib/fast-import/git-p4.txt @@ -191,6 +191,11 @@ git-p4.useclientspec git config [--global] git-p4.useclientspec false +The P4CLIENT environment variable should be correctly set for p4 to be +able to find the relevant client. This client spec will be used to +both filter the files cloned by git and set the directory layout as +specified in the client (this implies --keep-path style semantics). + Implementation Details... ========================= ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Support different branch layouts in git-p4 2011-02-09 3:46 ` Ian Wienand @ 2011-02-10 13:43 ` Pete Wyckoff 0 siblings, 0 replies; 7+ messages in thread From: Pete Wyckoff @ 2011-02-10 13:43 UTC (permalink / raw) To: Ian Wienand; +Cc: Tor Arvid Lund, git ianw@vmware.com wrote on Tue, 08 Feb 2011 19:46 -0800: > So here's another version; I agree some testing would be good as I've > only run it locally on //depot/proj/branch clients This is good. Thanks for fixing it up. One last pedantic whine from me. Fix the regex for the error case: arf$ python >>> import re >>> re.match('\%\%d', "%%3") >>> re.match(r'%%\d', "%%3") >>> <_sre.SRE_Match object at 0x1ec4168> -- Pete > --- > > diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4 > index 04ce7e3..3304f36 100755 > --- a/contrib/fast-import/git-p4 > +++ b/contrib/fast-import/git-p4 > @@ -910,6 +910,22 @@ class P4Sync(Command): > return files > > def stripRepoPath(self, path, prefixes): > + if self.useClientSpec: > + > + # if using the client spec, we use the output directory > + # specified in the client. For example, a view > + # //depot/foo/branch/... //client/branch/foo/... > + # will end up putting all foo/branch files into > + # branch/foo/ > + for val in self.clientSpecDirs: > + if path.startswith(val[0]): > + # replace the depot path with the client path > + path = path.replace(val[0], val[1][1]) > + # now strip out the client (//client/...) > + path = re.sub("^(//[^/]+/)", '', path) > + # the rest is all path > + return path > + > if self.keepRepoPath: > prefixes = [re.sub("^(//[^/]+/).*", r'\1', prefixes[0])] > > @@ -1032,7 +1048,7 @@ class P4Sync(Command): > includeFile = True > for val in self.clientSpecDirs: > if f['path'].startswith(val[0]): > - if val[1] <= 0: > + if val[1][0] <= 0: > includeFile = False > break > > @@ -1475,19 +1491,45 @@ class P4Sync(Command): > for entry in specList: > for k,v in entry.iteritems(): > if k.startswith("View"): > + > + # p4 has these %%1 to %%9 arguments in specs to > + # reorder paths; which we can't handle (yet :) > + if re.match('\%\%d', v) != None: > + print "Sorry, can't handle %%n arguments in client specs" > + sys.exit(1) > + > if v.startswith('"'): > start = 1 > else: > start = 0 > index = v.find("...") > + > + # save the "client view"; i.e the RHS of the view > + # line that tells the client where to put the > + # files for this view. > + cv = v[index+3:].strip() # +3 to remove previous '...' > + > + # if the client view doesn't end with a > + # ... wildcard, then we're going to mess up the > + # output directory, so fail gracefully. > + if not cv.endswith('...'): > + print 'Sorry, client view in "%s" needs to end with wildcard' % (k) > + sys.exit(1) > + cv=cv[:-3] > + > + # now save the view; +index means included, -index > + # means it should be filtered out. > v = v[start:index] > if v.startswith("-"): > v = v[1:] > - temp[v] = -len(v) > + include = -len(v) > else: > - temp[v] = len(v) > + include = len(v) > + > + temp[v] = (include, cv) > + > self.clientSpecDirs = temp.items() > - self.clientSpecDirs.sort( lambda x, y: abs( y[1] ) - abs( x[1] ) ) > + self.clientSpecDirs.sort( lambda x, y: abs( y[1][0] ) - abs( x[1][0] ) ) > > def run(self, args): > self.depotPaths = [] > diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt > index 49b3359..e09da44 100644 > --- a/contrib/fast-import/git-p4.txt > +++ b/contrib/fast-import/git-p4.txt > @@ -191,6 +191,11 @@ git-p4.useclientspec > > git config [--global] git-p4.useclientspec false > > +The P4CLIENT environment variable should be correctly set for p4 to be > +able to find the relevant client. This client spec will be used to > +both filter the files cloned by git and set the directory layout as > +specified in the client (this implies --keep-path style semantics). > + > Implementation Details... > ========================= > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Support different branch layouts in git-p4 2011-02-07 0:05 ` Ian Wienand 2011-02-07 23:27 ` Tor Arvid Lund @ 2011-02-08 1:22 ` Pete Wyckoff 1 sibling, 0 replies; 7+ messages in thread From: Pete Wyckoff @ 2011-02-08 1:22 UTC (permalink / raw) To: Ian Wienand; +Cc: Tor Arvid Lund, git ianw@vmware.com wrote on Sun, 06 Feb 2011 16:05 -0800: > I did consider this at first. My only issue is that it is a bit > confusing to use the client spec for filtering (and in this case > re-writing), but not for actually selecting the depots to clone, which > I still need to replicate on the command line. However that is a much > larger change. > > What do you think of this one? > > In this case, my client view is > > //depot/project/branch/... //client/branch/project/... > //depot/project2/branch/... //client/branch/project2/... > > and my git directory layout ends up as > > branch/project/... > branch/project2/... We had such terrible p4 mappings too, before the last rearrangement put us into a single-line view spec. I think it would help others to include such support, though. Back then, I hacked together similar code to deal with the annoyance. My code was not pretty and not complete, either. If you look at "p4 help views", they have lots of oddities that in theory should be accounted for here. It doesn't even mention the thing about quotes, but obviously that is supported. Wildcards ... and * can appear multiple times. And %%[1-9] can be used to reorder the path. Also the order of lines matters, and "+" can be used to merge entries. Whew. In practice, I think you get most everything we care about. A few comments below, beyond the bits that Tor Arvid caught. -- Pete > diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4 > index 04ce7e3..eb9620c 100755 > --- a/contrib/fast-import/git-p4 > +++ b/contrib/fast-import/git-p4 > @@ -878,6 +878,7 @@ class P4Sync(Command): > self.cloneExclude = [] > self.useClientSpec = False > self.clientSpecDirs = [] > + self.clientName = None Unused. > if gitConfig("git-p4.syncFromOrigin") == "false": > self.syncWithOrigin = False > @@ -910,6 +911,22 @@ class P4Sync(Command): > return files > > def stripRepoPath(self, path, prefixes): > + if self.useClientSpec: > + > + # if using the client spec, we use the output directory > + # specified in the client. For example, a view > + # //depot/foo/branch/... //client/branch/foo/... > + # will end up putting all foo/branch files into > + # branch/foo/ > + for val in self.clientSpecDirs: > + if path.startswith(val[0]): > + # replace the depot path with the client path > + path = path.replace(val[0], val[1][1]) > + # now strip out the client (//client/...) > + path = re.sub("^(//[^/]+/)", '', path) > + # the rest is all path > + return path That's clever. Better than having to remember Client: and build //client/ out of it. You could do this down in getClientSpec() so that val[1] starts with the git-relative path. > if self.keepRepoPath: > prefixes = [re.sub("^(//[^/]+/).*", r'\1', prefixes[0])] > > @@ -1032,7 +1049,7 @@ class P4Sync(Command): > includeFile = True > for val in self.clientSpecDirs: > if f['path'].startswith(val[0]): > - if val[1] <= 0: > + if val[1][0] <= 0: > includeFile = False > break > > @@ -1474,20 +1491,36 @@ class P4Sync(Command): > temp = {} > for entry in specList: > for k,v in entry.iteritems(): > + if k.startswith("Client"): > + self.clientName = v Oh maybe here is where you thought you would need client, but don't. > if k.startswith("View"): > if v.startswith('"'): > start = 1 > else: > start = 0 > index = v.find("...") > + > + # save the "client view"; i.e the RHS of the view > + # line that tells the client where to put the > + # files for this view. > + cv = v[index+4:] # +4 to remove previous '... ' > + cv_index = cv.find("...") > + cv=cv[:cv_index] > + > + # now save the view; +index means included, -index > + # means it should be filtered out. > v = v[start:index] > if v.startswith("-"): > v = v[1:] > - temp[v] = -len(v) > + include = -len(v) > else: > - temp[v] = len(v) > + include = len(v) > + > + temp[v] = (include, cv) > + > self.clientSpecDirs = temp.items() > - self.clientSpecDirs.sort( lambda x, y: abs( y[1] ) - abs( x[1] ) ) > + self.clientSpecDirs.sort( lambda x, y: abs( y[1][0] ) - abs( x[1][0] ) ) ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-02-10 13:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-02-01 22:59 [PATCH] Support different branch layouts in git-p4 Ian Wienand 2011-02-05 0:37 ` Tor Arvid Lund 2011-02-07 0:05 ` Ian Wienand 2011-02-07 23:27 ` Tor Arvid Lund 2011-02-09 3:46 ` Ian Wienand 2011-02-10 13:43 ` Pete Wyckoff 2011-02-08 1:22 ` Pete Wyckoff
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.