git-p4 migrates perforce “main” branch into git branches as subdirectories (doubled code in git branches)

git-p4 migrates perforce “main” branch into git branches as subdirectories (doubled code in git branches)

[Matthew Korich]

Using git p4 on git version 1.7.12.rc2 has path issues. Standard
clone/sync ops apparently place detected master and branches on
independent and parallel directory structures instead of git branches.
See http://stackoverflow.com/q/11893688/1588831 for a full demo of the problem.
Thanks,
-Matt Korich

[PATCH 0/5] git p4: fix branch detection with --use-client-spec

[Pete Wyckoff]

matthew@korich.net wrote on Fri, 10 Aug 2012 12:14 -0700:
> Using git p4 on git version 1.7.12.rc2 has path issues. Standard
> clone/sync ops apparently place detected master and branches on
> independent and parallel directory structures instead of git branches.
> See http://stackoverflow.com/q/11893688/1588831 for a full demo of the problem.

Thank you for the detailed report.  It is a bug in 1.7.12-rc2.
This series fixes it, on top of origin/master.

The crux of the matter is that files are mapped into the wrong
locations in git when both --use-client-spec and --branch-detection
are enabled.

Pete Wyckoff (5):
  git p4 test: move client_view() function to library
  git p4 test: add broken --use-client-spec --detect-branches tests
  git p4: set self.branchPrefixes in initialization
  git p4: do wildcard decoding in stripRepoPath
  git p4: make branch detection work with --use-client-spec

 git-p4.py                     | 75 +++++++++++++++++++++++++++--------------
 t/lib-git-p4.sh               | 18 ++++++++++
 t/t9801-git-p4-branch.sh      | 77 +++++++++++++++++++++++++++++++++++++++++++
 t/t9809-git-p4-client-view.sh | 17 ----------
 4 files changed, 146 insertions(+), 41 deletions(-)

-- 
1.7.12.rc2.24.gc304662

[PATCH 1/5] git p4 test: move client_view() function to library

[Pete Wyckoff]

This code will be useful in --detect-branches --use-client-spec tests.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/lib-git-p4.sh               | 18 ++++++++++++++++++
 t/t9809-git-p4-client-view.sh | 17 -----------------
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 2d753ab..5c952d6 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -115,3 +115,21 @@ marshal_dump() {
 	EOF
 	"$PYTHON_PATH" "$TRASH_DIRECTORY/marshal-dump.py"
 }
+
+#
+# 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
+}
+
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index 7d993ef..281be29 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -9,23 +9,6 @@ test_expect_success '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".
 #
-- 
1.7.12.rc2.24.gc304662

[PATCH 2/5] git p4 test: add broken --use-client-spec --detect-branches tests

[Pete Wyckoff]

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9801-git-p4-branch.sh | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 99fe16b..ca3a7f9 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -410,6 +410,83 @@ test_expect_failure 'git p4 clone file subset branch' '
 		test_path_is_missing file3
 	)
 '
+
+# From a report in http://stackoverflow.com/questions/11893688
+# where --use-client-spec caused branch prefixes not to be removed;
+# every file in git appeared into a subdirectory of the branch name.
+test_expect_success 'use-client-spec detect-branches setup' '
+	rm -rf "$cli" &&
+	mkdir "$cli" &&
+	(
+		cd "$cli" &&
+		client_view "//depot/usecs/... //client/..." &&
+		mkdir b1 &&
+		echo b1/b1-file1 >b1/b1-file1 &&
+		p4 add b1/b1-file1 &&
+		p4 submit -d "b1/b1-file1" &&
+
+		p4 integrate //depot/usecs/b1/... //depot/usecs/b2/... &&
+		p4 submit -d "b1 -> b2" &&
+		p4 branch -i <<-EOF &&
+		Branch: b2
+		View: //depot/usecs/b1/... //depot/usecs/b2/...
+		EOF
+
+		echo b2/b2-file2 >b2/b2-file2 &&
+		p4 add b2/b2-file2 &&
+		p4 submit -d "b2/b2-file2"
+	)
+'
+
+test_expect_failure 'use-client-spec detect-branches files in top-level' '
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$git" &&
+		git p4 sync --detect-branches --use-client-spec //depot/usecs@all &&
+		git checkout -b master p4/usecs/b1 &&
+		test_path_is_file b1-file1 &&
+		test_path_is_missing b2-file2 &&
+		test_path_is_missing b1 &&
+		test_path_is_missing b2 &&
+
+		git checkout -b b2 p4/usecs/b2 &&
+		test_path_is_file b1-file1 &&
+		test_path_is_file b2-file2 &&
+		test_path_is_missing b1 &&
+		test_path_is_missing b2
+	)
+'
+
+test_expect_success 'use-client-spec detect-branches skips branches setup' '
+	(
+		cd "$cli" &&
+
+		p4 integrate //depot/usecs/b1/... //depot/usecs/b3/... &&
+		p4 submit -d "b1 -> b3" &&
+		p4 branch -i <<-EOF &&
+		Branch: b3
+		View: //depot/usecs/b1/... //depot/usecs/b3/...
+		EOF
+
+		echo b3/b3-file3 >b3/b3-file3 &&
+		p4 add b3/b3-file3 &&
+		p4 submit -d "b3/b3-file3"
+	)
+'
+
+test_expect_success 'use-client-spec detect-branches skips branches' '
+	client_view "//depot/usecs/... //client/..." \
+	            "-//depot/usecs/b3/... //client/b3/..." &&
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$git" &&
+		git p4 sync --detect-branches --use-client-spec //depot/usecs@all &&
+		test_must_fail git rev-parse refs/remotes/p4/usecs/b3
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.7.12.rc2.24.gc304662

[PATCH 3/5] git p4: set self.branchPrefixes in initialization

[Pete Wyckoff]

This instance variable is needed during commit() to map
files from p4 to their relative locations in git.  Set
it when initializing P4Sync to avoid passing it to every
commit() call.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index e67d37d..6d07115 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2041,10 +2041,9 @@ class P4Sync(Command, P4UserMap):
         gitStream.write(description)
         gitStream.write("\n")
 
-    def commit(self, details, files, branch, branchPrefixes, parent = ""):
+    def commit(self, details, files, branch, parent = ""):
         epoch = details["time"]
         author = details["user"]
-        self.branchPrefixes = branchPrefixes
 
         if self.verbose:
             print "commit into %s" % branch
@@ -2053,7 +2052,7 @@ class P4Sync(Command, P4UserMap):
         # create a commit.
         new_files = []
         for f in files:
-            if [p for p in branchPrefixes if p4PathStartsWith(f['path'], p)]:
+            if [p for p in self.branchPrefixes if p4PathStartsWith(f['path'], p)]:
                 new_files.append (f)
             else:
                 sys.stderr.write("Ignoring file outside of prefix: %s\n" % f['path'])
@@ -2070,8 +2069,8 @@ class P4Sync(Command, P4UserMap):
 
         self.gitStream.write("data <<EOT\n")
         self.gitStream.write(details["desc"])
-        self.gitStream.write("\n[git-p4: depot-paths = \"%s\": change = %s"
-                             % (','.join (branchPrefixes), details["change"]))
+        self.gitStream.write("\n[git-p4: depot-paths = \"%s\": change = %s" %
+                             (','.join(self.branchPrefixes), details["change"]))
         if len(details['options']) > 0:
             self.gitStream.write(": options = %s" % details['options'])
         self.gitStream.write("]\nEOT\n\n")
@@ -2094,7 +2093,7 @@ class P4Sync(Command, P4UserMap):
                 print "Change %s is labelled %s" % (change, labelDetails)
 
             files = p4CmdList(["files"] + ["%s...@%s" % (p, change)
-                                                    for p in branchPrefixes])
+                                                for p in self.branchPrefixes])
 
             if len(files) == len(labelRevisions):
 
@@ -2405,6 +2404,7 @@ class P4Sync(Command, P4UserMap):
                     for branch in branches.keys():
                         ## HACK  --hwn
                         branchPrefix = self.depotPaths[0] + branch + "/"
+                        self.branchPrefixes = [ branchPrefix ]
 
                         parent = ""
 
@@ -2449,19 +2449,19 @@ class P4Sync(Command, P4UserMap):
                             tempBranch = os.path.join(self.tempBranchLocation, "%d" % (change))
                             if self.verbose:
                                 print "Creating temporary branch: " + tempBranch
-                            self.commit(description, filesForCommit, tempBranch, [branchPrefix])
+                            self.commit(description, filesForCommit, tempBranch)
                             self.tempBranches.append(tempBranch)
                             self.checkpoint()
                             blob = self.searchParent(parent, branch, tempBranch)
                         if blob:
-                            self.commit(description, filesForCommit, branch, [branchPrefix], blob)
+                            self.commit(description, filesForCommit, branch, blob)
                         else:
                             if self.verbose:
                                 print "Parent of %s not found. Committing into head of %s" % (branch, parent)
-                            self.commit(description, filesForCommit, branch, [branchPrefix], parent)
+                            self.commit(description, filesForCommit, branch, parent)
                 else:
                     files = self.extractFilesFromCommit(description)
-                    self.commit(description, files, self.branch, self.depotPaths,
+                    self.commit(description, files, self.branch,
                                 self.initialParent)
                     self.initialParent = ""
             except IOError:
@@ -2525,7 +2525,7 @@ class P4Sync(Command, P4UserMap):
 
         self.updateOptionDict(details)
         try:
-            self.commit(details, self.extractFilesFromCommit(details), self.branch, self.depotPaths)
+            self.commit(details, self.extractFilesFromCommit(details), self.branch)
         except IOError:
             print "IO error with git fast-import. Is your git version recent enough?"
             print self.gitError.read()
@@ -2683,6 +2683,9 @@ class P4Sync(Command, P4UserMap):
 
         self.depotPaths = newPaths
 
+        # --detect-branches may change this for each branch
+        self.branchPrefixes = self.depotPaths
+
         self.loadUserMapFromCache()
         self.labels = {}
         if self.detectLabels:
-- 
1.7.12.rc2.24.gc304662

[PATCH 4/5] git p4: do wildcard decoding in stripRepoPath

[Pete Wyckoff]

Instead of having to remember to do it after each call to
stripRepoPath, make it part of that function.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 6d07115..e20ff5d 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1819,15 +1819,17 @@ class P4Sync(Command, P4UserMap):
 
     def stripRepoPath(self, path, prefixes):
         if self.useClientSpec:
-            return self.clientSpecDirs.map_in_client(path)
+            path = self.clientSpecDirs.map_in_client(path)
 
-        if self.keepRepoPath:
-            prefixes = [re.sub("^(//[^/]+/).*", r'\1', prefixes[0])]
+        else:
+            if self.keepRepoPath:
+                prefixes = [re.sub("^(//[^/]+/).*", r'\1', prefixes[0])]
 
-        for p in prefixes:
-            if p4PathStartsWith(path, p):
-                path = path[len(p):]
+            for p in prefixes:
+                if p4PathStartsWith(path, p):
+                    path = path[len(p):]
 
+        path = wildcard_decode(path)
         return path
 
     def splitFilesIntoBranches(self, commit):
@@ -1849,7 +1851,6 @@ class P4Sync(Command, P4UserMap):
             fnum = fnum + 1
 
             relPath = self.stripRepoPath(path, self.depotPaths)
-            relPath = wildcard_decode(relPath)
 
             for branch in self.knownBranches.keys():
 
@@ -1867,7 +1868,6 @@ class P4Sync(Command, P4UserMap):
 
     def streamOneP4File(self, file, contents):
         relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
-        relPath = wildcard_decode(relPath)
         if verbose:
             sys.stderr.write("%s\n" % relPath)
 
@@ -1936,7 +1936,6 @@ class P4Sync(Command, P4UserMap):
 
     def streamOneP4Deletion(self, file):
         relPath = self.stripRepoPath(file['path'], self.branchPrefixes)
-        relPath = wildcard_decode(relPath)
         if verbose:
             sys.stderr.write("delete %s\n" % relPath)
         self.gitStream.write("D %s\n" % relPath)
-- 
1.7.12.rc2.24.gc304662

[PATCH 5/5] git p4: make branch detection work with --use-client-spec

[Pete Wyckoff]

The bug report in http://stackoverflow.com/questions/11893688
observes that files are mapped into the wrong locations in
git when both --use-client-spec and --branch-detection are enabled.

Fix this by changing the relative path prefix to match discovered
branches when using a client spec.

The problem was likely introduced with ecb7cf9 (git-p4: rewrite view
handling, 2012-01-02).

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py                | 37 +++++++++++++++++++++++++++++++------
 t/t9801-git-p4-branch.sh |  2 +-
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index e20ff5d..aed1a2d 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1818,21 +1818,41 @@ class P4Sync(Command, P4UserMap):
         return files
 
     def stripRepoPath(self, path, prefixes):
+        """When streaming files, this is called to map a p4 depot path
+           to where it should go in git.  The prefixes are either
+           self.depotPaths, or self.branchPrefixes in the case of
+           branch detection."""
+
         if self.useClientSpec:
+            # branch detection moves files up a level (the branch name)
+            # from what client spec interpretation gives
             path = self.clientSpecDirs.map_in_client(path)
+            if self.detectBranches:
+                for b in self.knownBranches:
+                    if path.startswith(b + "/"):
+                        path = path[len(b)+1:]
+
+        elif 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.
+            depot = re.sub("^(//[^/]+/).*", r'\1', prefixes[0])
+            if p4PathStartsWith(path, depot):
+                path = path[len(depot):]
 
         else:
-            if self.keepRepoPath:
-                prefixes = [re.sub("^(//[^/]+/).*", r'\1', prefixes[0])]
-
             for p in prefixes:
                 if p4PathStartsWith(path, p):
                     path = path[len(p):]
+                    break
 
         path = wildcard_decode(path)
         return path
 
     def splitFilesIntoBranches(self, commit):
+        """Look at each depotFile in the commit to figure out to what
+           branch it belongs."""
+
         branches = {}
         fnum = 0
         while commit.has_key("depotFile%s" % fnum):
@@ -1850,11 +1870,16 @@ class P4Sync(Command, P4UserMap):
             file["type"] = commit["type%s" % fnum]
             fnum = fnum + 1
 
-            relPath = self.stripRepoPath(path, self.depotPaths)
+            # start with the full relative path where this file would
+            # go in a p4 client
+            if self.useClientSpec:
+                relPath = self.clientSpecDirs.map_in_client(path)
+            else:
+                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
+                # add a trailing slash so that a commit into qt/4.2foo
+                # doesn't end up in qt/4.2, e.g.
                 if relPath.startswith(branch + "/"):
                     if branch not in branches:
                         branches[branch] = []
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index ca3a7f9..c5f0977 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -438,7 +438,7 @@ test_expect_success 'use-client-spec detect-branches setup' '
 	)
 '
 
-test_expect_failure 'use-client-spec detect-branches files in top-level' '
+test_expect_success 'use-client-spec detect-branches files in top-level' '
 	test_when_finished cleanup_git &&
 	test_create_repo "$git" &&
 	(
-- 
1.7.12.rc2.24.gc304662

Re: [PATCH 0/5] git p4: fix branch detection with --use-client-spec

[Junio C Hamano]

Pete Wyckoff <pw@padd.com> writes:

> matthew@korich.net wrote on Fri, 10 Aug 2012 12:14 -0700:
>> Using git p4 on git version 1.7.12.rc2 has path issues. Standard
>> clone/sync ops apparently place detected master and branches on
>> independent and parallel directory structures instead of git branches.
>> See http://stackoverflow.com/q/11893688/1588831 for a full demo of the problem.
>
> Thank you for the detailed report.  It is a bug in 1.7.12-rc2.

Do you mean "a feature that was present in 1.7.11 without this bug
was broken when used with 1.7.12-rc2"?  Or do you mean "this bug
exists in 1.7.12-rc2 (older versions may or may not have it, but I
am stressing that it is not fixed)"?

The description for [PATCH 5/5] blames v1.7.9-rc0~4^2~1, which tells
me it is the latter.  And if that were the case, and if this were in
the area of the system I oversee, I wouldn't push it to the upcoming
release at this late in the cycle, when I do not know what other
things it might break while fixing this bug (in other words, a fix
to an old bug is not an execuse to introduce a regression).

But git-p4 is not in my area, so if you meant this should go in the
upcoming 1.7.12 release, I'll queue them directly on 'master'.

Please tell me what your preference is.

Thanks.

> This series fixes it, on top of origin/master.
>
> The crux of the matter is that files are mapped into the wrong
> locations in git when both --use-client-spec and --branch-detection
> are enabled.
>
> Pete Wyckoff (5):
>   git p4 test: move client_view() function to library
>   git p4 test: add broken --use-client-spec --detect-branches tests
>   git p4: set self.branchPrefixes in initialization
>   git p4: do wildcard decoding in stripRepoPath
>   git p4: make branch detection work with --use-client-spec
>
>  git-p4.py                     | 75 +++++++++++++++++++++++++++--------------
>  t/lib-git-p4.sh               | 18 ++++++++++
>  t/t9801-git-p4-branch.sh      | 77 +++++++++++++++++++++++++++++++++++++++++++
>  t/t9809-git-p4-client-view.sh | 17 ----------
>  4 files changed, 146 insertions(+), 41 deletions(-)

Re: [PATCH 0/5] git p4: fix branch detection with --use-client-spec

[Pete Wyckoff]

gitster@pobox.com wrote on Sat, 11 Aug 2012 21:41 -0700:
> Pete Wyckoff <pw@padd.com> writes:
> 
> > matthew@korich.net wrote on Fri, 10 Aug 2012 12:14 -0700:
> >> Using git p4 on git version 1.7.12.rc2 has path issues. Standard
> >> clone/sync ops apparently place detected master and branches on
> >> independent and parallel directory structures instead of git branches.
> >> See http://stackoverflow.com/q/11893688/1588831 for a full demo of the problem.
> >
> > Thank you for the detailed report.  It is a bug in 1.7.12-rc2.
> 
> Do you mean "a feature that was present in 1.7.11 without this bug
> was broken when used with 1.7.12-rc2"?  Or do you mean "this bug
> exists in 1.7.12-rc2 (older versions may or may not have it, but I
> am stressing that it is not fixed)"?
> 
> The description for [PATCH 5/5] blames v1.7.9-rc0~4^2~1, which tells
> me it is the latter.  And if that were the case, and if this were in
> the area of the system I oversee, I wouldn't push it to the upcoming
> release at this late in the cycle, when I do not know what other
> things it might break while fixing this bug (in other words, a fix
> to an old bug is not an execuse to introduce a regression).
> 
> But git-p4 is not in my area, so if you meant this should go in the
> upcoming 1.7.12 release, I'll queue them directly on 'master'.
> 
> Please tell me what your preference is.

Good point about "already released bugs".  I confirmed it was
broken in 1.7.11 too, so there's no reason to rush this fix into
1.7.12.  If you could queue it up in pu, that would be great.
Otherwise I'll resubmit after the upcoming release.

Thanks,

		-- Pete

Re: [PATCH 0/5] git p4: fix branch detection with --use-client-spec

[Junio C Hamano]

Pete Wyckoff <pw@padd.com> writes:

>> The description for [PATCH 5/5] blames v1.7.9-rc0~4^2~1, which tells
>> me it is the latter.  And if that were the case, and if this were in
>> the area of the system I oversee, I wouldn't push it to the upcoming
>> release at this late in the cycle, when I do not know what other
>> things it might break while fixing this bug (in other words, a fix
>> to an old bug is not an execuse to introduce a regression).
>> 
>> But git-p4 is not in my area, so if you meant this should go in the
>> upcoming 1.7.12 release, I'll queue them directly on 'master'.
>> 
>> Please tell me what your preference is.
>
> Good point about "already released bugs".  I confirmed it was
> broken in 1.7.11 too, so there's no reason to rush this fix into
> 1.7.12.  If you could queue it up in pu, that would be great.

OK, I've done so already last night when I wrote my message but
haven't pushed the result out yet.

Note (primarily to other people) that the above exchange does not
mean "a fix to an old bug is automatically disqualified during the
rc freeze period".  If the fix is so focused and obvious that there
is no way the change inadvertently and negatively affects other code
and introduce a new bug, it is perfectly fine to apply the fix any
time.  I don't know git-p4 well enough to tell if this five patch
series was in that "obviously safe" category myself, so I asked Pete
to decide it for me.

Thanks.