On Mon, Feb 7, 2011 at 1:05 AM, Ian Wienand <ianw@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html