Merge branch 'pw/p4-view-updates'

* pw/p4-view-updates:
  git-p4: view spec documentation
  git-p4: rewrite view handling
  git-p4: support single file p4 client view maps
  git-p4: sort client views by reverse View number
  git-p4: fix test for unsupported P4 Client Views
  git-p4: test client view handling

Documentation/git-p4.txt

@@ -230,12 +230,7 @@ git repository:
 
 --use-client-spec::
 	Use a client spec to find the list of interesting files in p4.
-	The client spec is discovered using 'p4 client -o' which checks
+	See the "CLIENT SPEC" section below.
-	the 'P4CLIENT' environment variable and returns a mapping of
-	depot files to workspace files.  Note that a depot path is
-	still required, but files found in the path that match in
-	the client spec view will be laid out according to the client
-	spec.
 
 Clone options
 ~~~~~~~~~~~~~
@@ -304,6 +299,27 @@ p4 revision specifier on the end:
 See 'p4 help revisions' for the full syntax of p4 revision specifiers.
 
 
+CLIENT SPEC
+-----------
+The p4 client specification is maintained with the 'p4 client' command
+and contains among other fields, a View that specifies how the depot
+is mapped into the client repository.  Git-p4 can consult the client
+spec when given the '--use-client-spec' option or useClientSpec
+variable.
+
+The full syntax for a p4 view is documented in 'p4 help views'.  Git-p4
+knows only a subset of the view syntax.  It understands multi-line
+mappings, overlays with '+', exclusions with '-' and double-quotes
+around whitespace.  Of the possible wildcards, git-p4 only handles
+'...', and only when it is at the end of the path.  Git-p4 will complain
+if it encounters an unhandled wildcard.
+
+The name of the client can be given to git-p4 in multiple ways.  The
+variable 'git-p4.client' takes precedence if it exists.  Otherwise,
+normal p4 mechanisms of determining the client are used:  environment
+variable P4CLIENT, a file referenced by P4CONFIG, or the local host name.
+
+
 BRANCH DETECTION
 ----------------
 P4 does not have the same concept of a branch as git.  Instead,
@@ -387,9 +403,7 @@ git-p4.host::
 
 git-p4.client::
 	Client specified as an option to all p4 commands, with
-	'-c <client>'.  This can also be used as a way to find
+	'-c <client>', including the client spec.
-	the client spec for the 'useClientSpec' option.
-	The environment variable 'P4CLIENT' can be used instead.
 
 Clone and sync variables
 ~~~~~~~~~~~~~~~~~~~~~~~~
@@ -417,10 +431,10 @@ git config --add git-p4.branchList main:branchB
 -------------
 
 git-p4.useClientSpec::
-	Specify that the p4 client spec to be used to identify p4 depot
+	Specify that the p4 client spec should be used to identify p4
-	paths of interest.  This is equivalent to specifying the option
+	depot paths of interest.  This is equivalent to specifying the
-	'--use-client-spec'.  The variable 'git-p4.client' can be used
+	option '--use-client-spec'.  See the "CLIENT SPEC" section above.
-	to specify the name of the client.
+	This variable is a boolean, not the name of a p4 client.
 
 Submit variables
 ~~~~~~~~~~~~~~~~

contrib/fast-import/git-p4

@@ -1169,6 +1169,218 @@ class P4Submit(Command, P4UserMap):
 
         return True
 
+class View(object):
+    """Represent a p4 view ("p4 help views"), and map files in a
+       repo according to the view."""
+
+    class Path(object):
+        """A depot or client path, possibly containing wildcards.
+           The only one supported is ... at the end, currently.
+           Initialize with the full path, with //depot or //client."""
+
+        def __init__(self, path, is_depot):
+            self.path = path
+            self.is_depot = is_depot
+            self.find_wildcards()
+            # remember the prefix bit, useful for relative mappings
+            m = re.match("(//[^/]+/)", self.path)
+            if not m:
+                die("Path %s does not start with //prefix/" % self.path)
+            prefix = m.group(1)
+            if not self.is_depot:
+                # strip //client/ on client paths
+                self.path = self.path[len(prefix):]
+
+        def find_wildcards(self):
+            """Make sure wildcards are valid, and set up internal
+               variables."""
+
+            self.ends_triple_dot = False
+            # There are three wildcards allowed in p4 views
+            # (see "p4 help views").  This code knows how to
+            # handle "..." (only at the end), but cannot deal with
+            # "%%n" or "*".  Only check the depot_side, as p4 should
+            # validate that the client_side matches too.
+            if re.search(r'%%[1-9]', self.path):
+                die("Can't handle %%n wildcards in view: %s" % self.path)
+            if self.path.find("*") >= 0:
+                die("Can't handle * wildcards in view: %s" % self.path)
+            triple_dot_index = self.path.find("...")
+            if triple_dot_index >= 0:
+                if not self.path.endswith("..."):
+                    die("Can handle ... wildcard only at end of path: %s" %
+                        self.path)
+                self.ends_triple_dot = True
+
+        def ensure_compatible(self, other_path):
+            """Make sure the wildcards agree."""
+            if self.ends_triple_dot != other_path.ends_triple_dot:
+                 die("Both paths must end with ... if either does;\n" +
+                     "paths: %s %s" % (self.path, other_path.path))
+
+        def match_wildcards(self, test_path):
+            """See if this test_path matches us, and fill in the value
+               of the wildcards if so.  Returns a tuple of
+               (True|False, wildcards[]).  For now, only the ... at end
+               is supported, so at most one wildcard."""
+            if self.ends_triple_dot:
+                dotless = self.path[:-3]
+                if test_path.startswith(dotless):
+                    wildcard = test_path[len(dotless):]
+                    return (True, [ wildcard ])
+            else:
+                if test_path == self.path:
+                    return (True, [])
+            return (False, [])
+
+        def match(self, test_path):
+            """Just return if it matches; don't bother with the wildcards."""
+            b, _ = self.match_wildcards(test_path)
+            return b
+
+        def fill_in_wildcards(self, wildcards):
+            """Return the relative path, with the wildcards filled in
+               if there are any."""
+            if self.ends_triple_dot:
+                return self.path[:-3] + wildcards[0]
+            else:
+                return self.path
+
+    class Mapping(object):
+        def __init__(self, depot_side, client_side, overlay, exclude):
+            # depot_side is without the trailing /... if it had one
+            self.depot_side = View.Path(depot_side, is_depot=True)
+            self.client_side = View.Path(client_side, is_depot=False)
+            self.overlay = overlay  # started with "+"
+            self.exclude = exclude  # started with "-"
+            assert not (self.overlay and self.exclude)
+            self.depot_side.ensure_compatible(self.client_side)
+
+        def __str__(self):
+            c = " "
+            if self.overlay:
+                c = "+"
+            if self.exclude:
+                c = "-"
+            return "View.Mapping: %s%s -> %s" % \
+                   (c, self.depot_side, self.client_side)
+
+        def map_depot_to_client(self, depot_path):
+            """Calculate the client path if using this mapping on the
+               given depot path; does not consider the effect of other
+               mappings in a view.  Even excluded mappings are returned."""
+            matches, wildcards = self.depot_side.match_wildcards(depot_path)
+            if not matches:
+                return ""
+            client_path = self.client_side.fill_in_wildcards(wildcards)
+            return client_path
+
+    #
+    # View methods
+    #
+    def __init__(self):
+        self.mappings = []
+
+    def append(self, view_line):
+        """Parse a view line, splitting it into depot and client
+           sides.  Append to self.mappings, preserving order."""
+
+        # Split the view line into exactly two words.  P4 enforces
+        # structure on these lines that simplifies this quite a bit.
+        #
+        # Either or both words may be double-quoted.
+        # Single quotes do not matter.
+        # Double-quote marks cannot occur inside the words.
+        # A + or - prefix is also inside the quotes.
+        # There are no quotes unless they contain a space.
+        # The line is already white-space stripped.
+        # The two words are separated by a single space.
+        #
+        if view_line[0] == '"':
+            # First word is double quoted.  Find its end.
+            close_quote_index = view_line.find('"', 1)
+            if close_quote_index <= 0:
+                die("No first-word closing quote found: %s" % view_line)
+            depot_side = view_line[1:close_quote_index]
+            # skip closing quote and space
+            rhs_index = close_quote_index + 1 + 1
+        else:
+            space_index = view_line.find(" ")
+            if space_index <= 0:
+                die("No word-splitting space found: %s" % view_line)
+            depot_side = view_line[0:space_index]
+            rhs_index = space_index + 1
+
+        if view_line[rhs_index] == '"':
+            # Second word is double quoted.  Make sure there is a
+            # double quote at the end too.
+            if not view_line.endswith('"'):
+                die("View line with rhs quote should end with one: %s" %
+                    view_line)
+            # skip the quotes
+            client_side = view_line[rhs_index+1:-1]
+        else:
+            client_side = view_line[rhs_index:]
+
+        # prefix + means overlay on previous mapping
+        overlay = False
+        if depot_side.startswith("+"):
+            overlay = True
+            depot_side = depot_side[1:]
+
+        # prefix - means exclude this path
+        exclude = False
+        if depot_side.startswith("-"):
+            exclude = True
+            depot_side = depot_side[1:]
+
+        m = View.Mapping(depot_side, client_side, overlay, exclude)
+        self.mappings.append(m)
+
+    def map_in_client(self, depot_path):
+        """Return the relative location in the client where this
+           depot file should live.  Returns "" if the file should
+           not be mapped in the client."""
+
+        paths_filled = []
+        client_path = ""
+
+        # look at later entries first
+        for m in self.mappings[::-1]:
+
+            # see where will this path end up in the client
+            p = m.map_depot_to_client(depot_path)
+
+            if p == "":
+                # Depot path does not belong in client.  Must remember
+                # this, as previous items should not cause files to
+                # exist in this path either.  Remember that the list is
+                # being walked from the end, which has higher precedence.
+                # Overlap mappings do not exclude previous mappings.
+                if not m.overlay:
+                    paths_filled.append(m.client_side)
+
+            else:
+                # This mapping matched; no need to search any further.
+                # But, the mapping could be rejected if the client path
+                # has already been claimed by an earlier mapping.
+                already_mapped_in_client = False
+                for f in paths_filled:
+                    # this is View.Path.match
+                    if f.match(p):
+                        already_mapped_in_client = True
+                        break
+                if not already_mapped_in_client:
+                    # Include this file, unless it is from a line that
+                    # explicitly said to exclude it.
+                    if not m.exclude:
+                        client_path = p
+
+                # a match, even if rejected, always stops the search
+                break
+
+        return client_path
+
 class P4Sync(Command, P4UserMap):
     delete_actions = ( "delete", "move/delete", "purge" )
 
@@ -1216,7 +1428,7 @@ class P4Sync(Command, P4UserMap):
         self.p4BranchesInGit = []
         self.cloneExclude = []
         self.useClientSpec = False
-        self.clientSpecDirs = []
+        self.clientSpecDirs = None
 
         if gitConfig("git-p4.syncFromOrigin") == "false":
             self.syncWithOrigin = False
@@ -1267,20 +1479,7 @@ class P4Sync(Command, P4UserMap):
 
     def stripRepoPath(self, path, prefixes):
         if self.useClientSpec:
-
+            return self.clientSpecDirs.map_in_client(path)
-            # 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])]
@@ -1430,14 +1629,12 @@ class P4Sync(Command, P4UserMap):
         filesToDelete = []
 
         for f in files:
-            includeFile = True
+            # if using a client spec, only add the files that have
-            for val in self.clientSpecDirs:
+            # a path in the client
-                if f['path'].startswith(val[0]):
+            if self.clientSpecDirs:
-                    if val[1][0] <= 0:
+                if self.clientSpecDirs.map_in_client(f['path']) == "":
-                        includeFile = False
+                    continue
-                    break
 
-            if includeFile:
             filesForCommit.append(f)
             if f['action'] in self.delete_actions:
                 filesToDelete.append(f)
@@ -1882,49 +2079,30 @@ class P4Sync(Command, P4UserMap):
 
     def getClientSpec(self):
         specList = p4CmdList("client -o")
-        temp = {}
+        if len(specList) != 1:
-        for entry in specList:
+            die('Output from "client -o" is %d lines, expecting 1' %
-            for k,v in entry.iteritems():
+                len(specList))
-                if k.startswith("View"):
 
-                    # p4 has these %%1 to %%9 arguments in specs to
+        # dictionary of all client parameters
-                    # reorder paths; which we can't handle (yet :)
+        entry = specList[0]
-                    if re.match('%%\d', v) != None:
-                        print "Sorry, can't handle %%n arguments in client specs"
-                        sys.exit(1)
 
-                    if v.startswith('"'):
+        # just the keys that start with "View"
-                        start = 1
+        view_keys = [ k for k in entry.keys() if k.startswith("View") ]
-                    else:
-                        start = 0
-                    index = v.find("...")
 
-                    # save the "client view"; i.e the RHS of the view
+        # hold this new View
-                    # line that tells the client where to put the
+        view = View()
-                    # files for this view.
-                    cv = v[index+3:].strip() # +3 to remove previous '...'
 
-                    # if the client view doesn't end with a
+        # append the lines, in order, to the view
-                    # ... wildcard, then we're going to mess up the
+        for view_num in range(len(view_keys)):
-                    # output directory, so fail gracefully.
+            k = "View%d" % view_num
-                    if not cv.endswith('...'):
+            if k not in view_keys:
-                        print 'Sorry, client view in "%s" needs to end with wildcard' % (k)
+                die("Expected view key %s missing" % k)
-                        sys.exit(1)
+            view.append(entry[k])
-                    cv=cv[:-3]
 
-                    # now save the view; +index means included, -index
+        self.clientSpecDirs = view
-                    # means it should be filtered out.
+        if self.verbose:
-                    v = v[start:index]
+            for i, m in enumerate(self.clientSpecDirs.mappings):
-                    if v.startswith("-"):
+                    print "clientSpecDirs %d: %s" % (i, str(m))
-                        v = v[1:]
-                        include = -len(v)
-                    else:
-                        include = len(v)
-
-                    temp[v] = (include, cv)
-
-        self.clientSpecDirs = temp.items()
-        self.clientSpecDirs.sort( lambda x, y: abs( y[1][0] ) - abs( x[1][0] ) )
 
     def run(self, args):
         self.depotPaths = []

t/t9809-git-p4-client-view.sh

@@ -0,0 +1,290 @@
+#!/bin/sh
+
+test_description='git-p4 client view'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+#
+# Construct a client with this list of View lines
+#
+client_view() {
+	(
+		cat <<-EOF &&
+		Client: client
+		Description: client
+		Root: $cli
+		View:
+		EOF
+		for arg ; do
+			printf "\t$arg\n"
+		done
+	) | p4 client -i
+}
+
+#
+# Verify these files exist, exactly.  Caller creates
+# a list of files in file "files".
+#
+check_files_exist() {
+	ok=0 &&
+	num=${#@} &&
+	for arg ; do
+		test_path_is_file "$arg" &&
+		ok=$(($ok + 1))
+	done &&
+	test $ok -eq $num &&
+	test_line_count = $num files
+}
+
+#
+# Sync up the p4 client, make sure the given files (and only
+# those) exist.
+#
+client_verify() {
+	(
+		cd "$cli" &&
+		p4 sync &&
+		find . -type f ! -name files >files &&
+		check_files_exist "$@"
+	)
+}
+
+#
+# Make sure the named files, exactly, exist.
+#
+git_verify() {
+	(
+		cd "$git" &&
+		git ls-files >files &&
+		check_files_exist "$@"
+	)
+}
+
+# //depot
+#   - dir1
+#     - file11
+#     - file12
+#   - dir2
+#     - file21
+#     - file22
+test_expect_success 'init depot' '
+	(
+		cd "$cli" &&
+		for d in 1 2 ; do
+			mkdir -p dir$d &&
+			for f in 1 2 ; do
+				echo dir$d/file$d$f >dir$d/file$d$f &&
+				p4 add dir$d/file$d$f &&
+				p4 submit -d "dir$d/file$d$f"
+			done
+		done &&
+		find . -type f ! -name files >files &&
+		check_files_exist dir1/file11 dir1/file12 \
+				  dir2/file21 dir2/file22
+	)
+'
+
+# double % for printf
+test_expect_success 'unsupported view wildcard %%n' '
+	client_view "//depot/%%%%1/sub/... //client/sub/%%%%1/..." &&
+	test_when_finished cleanup_git &&
+	test_must_fail "$GITP4" clone --use-client-spec --dest="$git" //depot
+'
+
+test_expect_success 'unsupported view wildcard *' '
+	client_view "//depot/*/bar/... //client/*/bar/..." &&
+	test_when_finished cleanup_git &&
+	test_must_fail "$GITP4" clone --use-client-spec --dest="$git" //depot
+'
+
+test_expect_success 'wildcard ... only supported at end of spec' '
+	client_view "//depot/.../file11 //client/.../file11" &&
+	test_when_finished cleanup_git &&
+	test_must_fail "$GITP4" clone --use-client-spec --dest="$git" //depot
+'
+
+test_expect_success 'basic map' '
+	client_view "//depot/dir1/... //client/cli1/..." &&
+	files="cli1/file11 cli1/file12" &&
+	client_verify $files &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify $files
+'
+
+test_expect_success 'client view with no mappings' '
+	client_view &&
+	client_verify &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify
+'
+
+test_expect_success 'single file map' '
+	client_view "//depot/dir1/file11 //client/file11" &&
+	files="file11" &&
+	client_verify $files &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify $files
+'
+
+test_expect_success 'later mapping takes precedence (entire repo)' '
+	client_view "//depot/dir1/... //client/cli1/..." \
+		    "//depot/... //client/cli2/..." &&
+	files="cli2/dir1/file11 cli2/dir1/file12
+	       cli2/dir2/file21 cli2/dir2/file22" &&
+	client_verify $files &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify $files
+'
+
+test_expect_success 'later mapping takes precedence (partial repo)' '
+	client_view "//depot/dir1/... //client/..." \
+		    "//depot/dir2/... //client/..." &&
+	files="file21 file22" &&
+	client_verify $files &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify $files
+'
+
+# Reading the view backwards,
+#   dir2 goes to cli12
+#   dir1 cannot go to cli12 since it was filled by dir2
+#   dir1 also does not go to cli3, since the second rule
+#     noticed that it matched, but was already filled
+test_expect_success 'depot path matching rejected client path' '
+	client_view "//depot/dir1/... //client/cli3/..." \
+		    "//depot/dir1/... //client/cli12/..." \
+		    "//depot/dir2/... //client/cli12/..." &&
+	files="cli12/file21 cli12/file22" &&
+	client_verify $files &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify $files
+'
+
+# since both have the same //client/..., the exclusion
+# rule keeps everything out
+test_expect_success 'exclusion wildcard, client rhs same (odd)' '
+	client_view "//depot/... //client/..." \
+		    "-//depot/dir2/... //client/..." &&
+	client_verify &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify
+'
+
+test_expect_success 'exclusion wildcard, client rhs different (normal)' '
+	client_view "//depot/... //client/..." \
+		    "-//depot/dir2/... //client/dir2/..." &&
+	files="dir1/file11 dir1/file12" &&
+	client_verify $files &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify $files
+'
+
+test_expect_success 'exclusion single file' '
+	client_view "//depot/... //client/..." \
+		    "-//depot/dir2/file22 //client/file22" &&
+	files="dir1/file11 dir1/file12 dir2/file21" &&
+	client_verify $files &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify $files
+'
+
+test_expect_success 'overlay wildcard' '
+	client_view "//depot/dir1/... //client/cli/..." \
+		    "+//depot/dir2/... //client/cli/...\n" &&
+	files="cli/file11 cli/file12 cli/file21 cli/file22" &&
+	client_verify $files &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify $files
+'
+
+test_expect_success 'overlay single file' '
+	client_view "//depot/dir1/... //client/cli/..." \
+		    "+//depot/dir2/file21 //client/cli/file21" &&
+	files="cli/file11 cli/file12 cli/file21" &&
+	client_verify $files &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify $files
+'
+
+test_expect_success 'exclusion with later inclusion' '
+	client_view "//depot/... //client/..." \
+		    "-//depot/dir2/... //client/dir2/..." \
+		    "//depot/dir2/... //client/dir2incl/..." &&
+	files="dir1/file11 dir1/file12 dir2incl/file21 dir2incl/file22" &&
+	client_verify $files &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify $files
+'
+
+test_expect_success 'quotes on rhs only' '
+	client_view "//depot/dir1/... \"//client/cdir 1/...\"" &&
+	client_verify "cdir 1/file11" "cdir 1/file12" &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify "cdir 1/file11" "cdir 1/file12"
+'
+
+#
+# Rename directories to test quoting in depot-side mappings
+# //depot
+#    - "dir 1"
+#       - file11
+#       - file12
+#    - "dir 2"
+#       - file21
+#       - file22
+#
+test_expect_success 'rename files to introduce spaces' '
+	client_view "//depot/... //client/..." &&
+	client_verify dir1/file11 dir1/file12 \
+		      dir2/file21 dir2/file22 &&
+	(
+		cd "$cli" &&
+		p4 open dir1/... &&
+		p4 move dir1/... "dir 1"/... &&
+		p4 open dir2/... &&
+		p4 move dir2/... "dir 2"/... &&
+		p4 submit -d "rename with spaces"
+	) &&
+	client_verify "dir 1/file11" "dir 1/file12" \
+		      "dir 2/file21" "dir 2/file22"
+'
+
+test_expect_success 'quotes on lhs only' '
+	client_view "\"//depot/dir 1/...\" //client/cdir1/..." &&
+	files="cdir1/file11 cdir1/file12" &&
+	client_verify $files &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	client_verify $files
+'
+
+test_expect_success 'quotes on both sides' '
+	client_view "\"//depot/dir 1/...\" \"//client/cdir 1/...\"" &&
+	client_verify "cdir 1/file11" "cdir 1/file12" &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --use-client-spec --dest="$git" //depot &&
+	git_verify "cdir 1/file11" "cdir 1/file12"
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done