[PATCH 0/5] git-p4: a few assorted fixes for branches, excludes

[PATCH 0/5] git-p4: a few assorted fixes for branches, excludes

[Mazo, Andrey]

This series fixes a couple of corner cases with branch detection
and handling of excludes by git-p4.

Andrey Mazo (5):
  git-p4: detect/prevent infinite loop in gitCommitByP4Change()
  git-p4: match branches case insensitively if configured
  git-p4: don't groom exclude path list on every commit
  git-p4: add failing test for "don't exclude other files with same prefix"
  git-p4: don't exclude other files with same prefix

 git-p4.py                 | 39 +++++++++++++++++++-----------
 t/t9817-git-p4-exclude.sh | 51 +++++++++++++++++++++++++++++++++++----
 2 files changed, 71 insertions(+), 19 deletions(-)


base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
-- 
2.19.2

[PATCH 1/5] git-p4: detect/prevent infinite loop in gitCommitByP4Change()

[Mazo, Andrey]

Under certain circumstances, gitCommitByP4Change() can enter an infinite
loop resulting in `git p4 sync` hanging forever.

The problem is that
`git rev-list --bisect <latest> ^<earliest>` can return `<latest>`,
which would result in reinspecting <latest> and potentially an infinite loop.

This can happen when importing just a subset of P4 repository
and/or with explicit "--changesfile" option.

A real-life example:
"""
    looking in ref refs/remotes/p4/mybranch for change 26894 using bisect...
    Reading pipe: git rev-parse refs/remotes/p4/mybranch
    trying: earliest  latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git cat-file commit 147f5d3292af2e1cc4a56a7b96db845144c68486
    current change 25339
    trying: earliest ^147f5d3292af2e1cc4a56a7b96db845144c68486 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^147f5d3292af2e1cc4a56a7b96db845144c68486
    Reading pipe: git cat-file commit 51db83df9d588010d0bd995641c85aa0408a5bb9
    current change 25420
    trying: earliest ^51db83df9d588010d0bd995641c85aa0408a5bb9 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^51db83df9d588010d0bd995641c85aa0408a5bb9
    Reading pipe: git cat-file commit e8f83909ceb570f5a7e48c2853f3c5d8207cea52
    current change 25448
    trying: earliest ^e8f83909ceb570f5a7e48c2853f3c5d8207cea52 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^e8f83909ceb570f5a7e48c2853f3c5d8207cea52
    Reading pipe: git cat-file commit 09a48eb7acd594dce52e06681be9c366e1844d66
    current change 25521
    trying: earliest ^09a48eb7acd594dce52e06681be9c366e1844d66 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^09a48eb7acd594dce52e06681be9c366e1844d66
    Reading pipe: git cat-file commit 4daff81c520a82678e1ef347f2b5e97258101ae1
    current change 26907
    trying: earliest ^09a48eb7acd594dce52e06681be9c366e1844d66 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^09a48eb7acd594dce52e06681be9c366e1844d66
    Reading pipe: git cat-file commit 4daff81c520a82678e1ef347f2b5e97258101ae1
    current change 26907
    trying: earliest ^09a48eb7acd594dce52e06681be9c366e1844d66 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^09a48eb7acd594dce52e06681be9c366e1844d66
    Reading pipe: git cat-file commit 4daff81c520a82678e1ef347f2b5e97258101ae1
    current change 26907
    ...
"""

The fix is two-fold:
 * detect an infinite loop and die right away
   instead of looping forever;
 * make sure, `git rev-list --bisect` can't return "latestCommit" again
   by excluding it from the rev-list range explicitly.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---

Notes:
    I don't have a simple test-case for this yet,
    and I was able to perform a few complex initial `git p4 sync` runs
    without hitting this problem.
    
    I suspect, I had somehow messed up with branch definitions
    and --changesfile option at some point.

 git-p4.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 5b79920f46..c0a3068b6f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3323,11 +3323,13 @@ def gitCommitByP4Change(self, ref, change):
                 return next
 
             if currentChange < change:
                 earliestCommit = "^%s" % next
             else:
-                latestCommit = "%s" % next
+                if next == latestCommit:
+                    die("Infinite loop while looking in ref %s for change %s. Check your branch mappings" % (ref, change))
+                latestCommit = "%s^@" % next
 
         return ""
 
     def importNewBranch(self, branch, maxChange):
         # make fast-import flush all changes to disk and update the refs using the checkpoint
-- 
2.19.2

[PATCH 2/5] git-p4: match branches case insensitively if configured

[Mazo, Andrey]

git-p4 knows how to handle case insensitivity in file paths
if core.ignorecase is set.
However, when determining a branch for a file,
it still does a case-sensitive prefix match.
This may result in some file changes to be lost on import.

For example, given the following commits
 1. add //depot/main/file1
 2. add //depot/DirA/file2
 3. add //depot/dira/file3
 4. add //depot/DirA/file4
and "branchList = main:DirA" branch mapping,
commit 3 will be lost.

So, do branch search case insensitively if running with core.ignorecase set.
Teach splitFilesIntoBranches() to use the p4PathStartsWith() function
for path prefix matches instead of always case-sensitive match.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index c0a3068b6f..91c610f960 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2721,11 +2721,11 @@ def splitFilesIntoBranches(self, commit):
                 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, e.g.
-                if relPath.startswith(branch + "/"):
+                if p4PathStartsWith(relPath, branch + "/"):
                     if branch not in branches:
                         branches[branch] = []
                     branches[branch].append(file)
                     break
 
-- 
2.19.2

[PATCH 3/5] git-p4: don't groom exclude path list on every commit

[Mazo, Andrey]

Currently, `cloneExclude` array is being groomed (by removing trailing "...")
on every changeset.
(since `extractFilesFromCommit()` is called on every imported changeset)

As a micro-optimization, do it once while parsing arguments.
Also, prepend "/" and remove trailing "..." at the same time.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 git-p4.py | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 91c610f960..a9f53e5b88 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1314,11 +1314,11 @@ class Command:
     def __init__(self):
         self.usage = "usage: %prog [options]"
         self.needsGit = True
         self.verbose = False
 
-    # This is required for the "append" cloneExclude action
+    # This is required for the "append" update_shelve action
     def ensure_value(self, attr, value):
         if not hasattr(self, attr) or getattr(self, attr) is None:
             setattr(self, attr, value)
         return getattr(self, attr)
 
@@ -2528,10 +2528,15 @@ def map_in_client(self, depot_path):
             return self.client_spec_path_cache[depot_path]
 
         die( "Error: %s is not found in client spec path" % depot_path )
         return ""
 
+def cloneExcludeCallback(option, opt_str, value, parser):
+    # prepend "/" because the first "/" was consumed as part of the option itself.
+    # ("-//depot/A/..." becomes "/depot/A/..." after option parsing)
+    parser.values.cloneExclude += ["/" + re.sub(r"\.\.\.$", "", value)]
+
 class P4Sync(Command, P4UserMap):
 
     def __init__(self):
         Command.__init__(self)
         P4UserMap.__init__(self)
@@ -2551,11 +2556,11 @@ def __init__(self):
                 optparse.make_option("--keep-path", dest="keepRepoPath", action='store_true',
                                      help="Keep entire BRANCH/DIR/SUBDIR prefix during import"),
                 optparse.make_option("--use-client-spec", dest="useClientSpec", action='store_true',
                                      help="Only sync files that are included in the Perforce Client Spec"),
                 optparse.make_option("-/", dest="cloneExclude",
-                                     action="append", type="string",
+                                     action="callback", callback=cloneExcludeCallback, type="string",
                                      help="exclude depot path"),
         ]
         self.description = """Imports from Perforce into a git repository.\n
     example:
     //depot/my/project/ -- to import the current head
@@ -2617,12 +2622,10 @@ def checkpoint(self):
         out = self.gitOutput.readline()
         if self.verbose:
             print("checkpoint finished: " + out)
 
     def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
-        self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
-                             for path in self.cloneExclude]
         files = []
         fnum = 0
         while "depotFile%s" % fnum in commit:
             path =  commit["depotFile%s" % fnum]
 
@@ -3888,11 +3891,10 @@ def run(self, args):
 
         if not self.cloneDestination and len(depotPaths) > 1:
             self.cloneDestination = depotPaths[-1]
             depotPaths = depotPaths[:-1]
 
-        self.cloneExclude = ["/"+p for p in self.cloneExclude]
         for p in depotPaths:
             if not p.startswith("//"):
                 sys.stderr.write('Depot paths must start with "//": %s\n' % p)
                 return False
 
-- 
2.19.2

[PATCH 4/5] git-p4: add failing test for "don't exclude other files with same prefix"

[Mazo, Andrey]

In preparation for a fix, add a failing test case to test that
git-p4 doesn't exclude files with the same prefix unintentionally
when exclude paths are specified without a trailing /.
I.e., don't exclude "//depot/file_dont_exclude" if run with "-//depot/file".
or don't exclude "//depot/discard_file_not" if run with "-//depot/discard_file".

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 t/t9817-git-p4-exclude.sh | 51 +++++++++++++++++++++++++++++++++++----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/t/t9817-git-p4-exclude.sh b/t/t9817-git-p4-exclude.sh
index aac568eadf..1c22570797 100755
--- a/t/t9817-git-p4-exclude.sh
+++ b/t/t9817-git-p4-exclude.sh
@@ -20,49 +20,90 @@ test_expect_success 'create exclude repo' '
 	(
 		cd "$cli" &&
 		mkdir -p wanted discard &&
 		echo wanted >wanted/foo &&
 		echo discard >discard/foo &&
-		p4 add wanted/foo discard/foo &&
+		echo discard_file >discard_file &&
+		echo discard_file_not >discard_file_not &&
+		p4 add wanted/foo discard/foo discard_file discard_file_not &&
 		p4 submit -d "initial revision"
 	)
 '
 
 test_expect_success 'check the repo was created correctly' '
 	test_when_finished cleanup_git &&
 	git p4 clone --dest="$git" //depot/...@all &&
 	(
 		cd "$git" &&
 		test_path_is_file wanted/foo &&
-		test_path_is_file discard/foo
+		test_path_is_file discard/foo &&
+		test_path_is_file discard_file &&
+		test_path_is_file discard_file_not
 	)
 '
 
 test_expect_success 'clone, excluding part of repo' '
 	test_when_finished cleanup_git &&
 	git p4 clone -//depot/discard/... --dest="$git" //depot/...@all &&
 	(
 		cd "$git" &&
 		test_path_is_file wanted/foo &&
-		test_path_is_missing discard/foo
+		test_path_is_missing discard/foo &&
+		test_path_is_file discard_file &&
+		test_path_is_file discard_file_not
+	)
+'
+
+test_expect_failure 'clone, excluding single file, no trailing /' '
+	test_when_finished cleanup_git &&
+	git p4 clone -//depot/discard_file --dest="$git" //depot/...@all &&
+	(
+		cd "$git" &&
+		test_path_is_file wanted/foo &&
+		test_path_is_file discard/foo &&
+		test_path_is_missing discard_file &&
+		test_path_is_file discard_file_not
 	)
 '
 
 test_expect_success 'clone, then sync with exclude' '
 	test_when_finished cleanup_git &&
 	git p4 clone -//depot/discard/... --dest="$git" //depot/...@all &&
 	(
 		cd "$cli" &&
-		p4 edit wanted/foo discard/foo &&
+		p4 edit wanted/foo discard/foo discard_file_not &&
 		date >>wanted/foo &&
 		date >>discard/foo &&
+		date >>discard_file_not &&
 		p4 submit -d "updating" &&
 
 		cd "$git" &&
 		git p4 sync -//depot/discard/... &&
 		test_path_is_file wanted/foo &&
-		test_path_is_missing discard/foo
+		test_path_is_missing discard/foo &&
+		test_path_is_file discard_file &&
+		test_path_is_file discard_file_not
+	)
+'
+
+test_expect_failure 'clone, then sync with exclude, no trailing /' '
+	test_when_finished cleanup_git &&
+	git p4 clone -//depot/discard/... -//depot/discard_file --dest="$git" //depot/...@all &&
+	(
+		cd "$cli" &&
+		p4 edit wanted/foo discard/foo discard_file_not &&
+		date >>wanted/foo &&
+		date >>discard/foo &&
+		date >>discard_file_not &&
+		p4 submit -d "updating" &&
+
+		cd "$git" &&
+		git p4 sync -//depot/discard/... -//depot/discard_file &&
+		test_path_is_file wanted/foo &&
+		test_path_is_missing discard/foo &&
+		test_path_is_missing discard_file &&
+		test_path_is_file discard_file_not
 	)
 '
 
 test_expect_success 'kill p4d' '
 	kill_p4d
-- 
2.19.2

[PATCH 5/5] git-p4: don't exclude other files with same prefix

[Mazo, Andrey]

Make sure not to exclude files unintentionally
if exclude paths are specified without a trailing /.
I.e., don't exclude "//depot/file_dont_exclude" if run with "-//depot/file".

Do this by ensuring that paths without a trailing "/" are only matched completely.

Also, abort path search on the first match as a micro-optimization.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 git-p4.py                 | 21 ++++++++++++++-------
 t/t9817-git-p4-exclude.sh |  4 ++--
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index a9f53e5b88..162877aa82 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2621,22 +2621,29 @@ def checkpoint(self):
         self.gitStream.write("progress checkpoint\n\n")
         out = self.gitOutput.readline()
         if self.verbose:
             print("checkpoint finished: " + out)
 
+    def isPathWanted(self, path):
+        for p in self.cloneExclude:
+            if p.endswith("/"):
+                if p4PathStartsWith(path, p):
+                    return False
+            # "-//depot/file1" without a trailing "/" should only exclude "file1", but not "file111" or "file1_dir/file2"
+            elif path.lower() == p.lower():
+                return False
+        for p in self.depotPaths:
+            if p4PathStartsWith(path, p):
+                return True
+        return False
+
     def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
         files = []
         fnum = 0
         while "depotFile%s" % fnum in commit:
             path =  commit["depotFile%s" % fnum]
-
-            if [p for p in self.cloneExclude
-                if p4PathStartsWith(path, p)]:
-                found = False
-            else:
-                found = [p for p in self.depotPaths
-                         if p4PathStartsWith(path, p)]
+            found = self.isPathWanted(path)
             if not found:
                 fnum = fnum + 1
                 continue
 
             file = {}
diff --git a/t/t9817-git-p4-exclude.sh b/t/t9817-git-p4-exclude.sh
index 1c22570797..275dd30425 100755
--- a/t/t9817-git-p4-exclude.sh
+++ b/t/t9817-git-p4-exclude.sh
@@ -51,11 +51,11 @@ test_expect_success 'clone, excluding part of repo' '
 		test_path_is_file discard_file &&
 		test_path_is_file discard_file_not
 	)
 '
 
-test_expect_failure 'clone, excluding single file, no trailing /' '
+test_expect_success 'clone, excluding single file, no trailing /' '
 	test_when_finished cleanup_git &&
 	git p4 clone -//depot/discard_file --dest="$git" //depot/...@all &&
 	(
 		cd "$git" &&
 		test_path_is_file wanted/foo &&
@@ -83,11 +83,11 @@ test_expect_success 'clone, then sync with exclude' '
 		test_path_is_file discard_file &&
 		test_path_is_file discard_file_not
 	)
 '
 
-test_expect_failure 'clone, then sync with exclude, no trailing /' '
+test_expect_success 'clone, then sync with exclude, no trailing /' '
 	test_when_finished cleanup_git &&
 	git p4 clone -//depot/discard/... -//depot/discard_file --dest="$git" //depot/...@all &&
 	(
 		cd "$cli" &&
 		p4 edit wanted/foo discard/foo discard_file_not &&
-- 
2.19.2

[PATCH v2 0/7] git-p4: a few assorted fixes for branches, excludes

[Mazo, Andrey]

This series fixes a few cases with branch detection
and handling of excludes by git-p4.

This is the second iteration of the patch series.
Changes since the v1 [1]:
 * Added new test case for excluded paths when detecting branches;
 * Added a new fix for excluded paths when detecting branches.

[1] https://public-inbox.org/git/cover.1551485349.git.amazo@checkvideo.com

Range-diff vs v1:
1:  3ac39171d4 = 1:  3ac39171d4 git-p4: detect/prevent infinite loop in gitCommitByP4Change()
2:  e644a8ab49 = 2:  e644a8ab49 git-p4: match branches case insensitively if configured
3:  44fed954dc = 3:  44fed954dc git-p4: don't groom exclude path list on every commit
4:  a0d3fa6add = 4:  a0d3fa6add git-p4: add failing test for "don't exclude other files with same prefix"
5:  3330f88a0d = 5:  3330f88a0d git-p4: don't exclude other files with same prefix
-:  ---------- > 6:  6170d45951 git-p4: add failing test for "git-p4: respect excluded paths when detecting branches"
-:  ---------- > 7:  758d8e8486 git-p4: respect excluded paths when detecting branches

Andrey Mazo (7):
  git-p4: detect/prevent infinite loop in gitCommitByP4Change()
  git-p4: match branches case insensitively if configured
  git-p4: don't groom exclude path list on every commit
  git-p4: add failing test for "don't exclude other files with same prefix"
  git-p4: don't exclude other files with same prefix
  git-p4: add failing test for "git-p4: respect excluded paths when detecting branches"
  git-p4: respect excluded paths when detecting branches

 git-p4.py                 | 42 ++++++++++++++++++++------------
 t/t9801-git-p4-branch.sh  | 40 ++++++++++++++++++++++++++++++
 t/t9817-git-p4-exclude.sh | 51 +++++++++++++++++++++++++++++++++++----
 3 files changed, 112 insertions(+), 21 deletions(-)


base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
-- 
2.19.2

[PATCH v2 1/7] git-p4: detect/prevent infinite loop in gitCommitByP4Change()

[Mazo, Andrey]

Under certain circumstances, gitCommitByP4Change() can enter an infinite
loop resulting in `git p4 sync` hanging forever.

The problem is that
`git rev-list --bisect <latest> ^<earliest>` can return `<latest>`,
which would result in reinspecting <latest> and potentially an infinite loop.

This can happen when importing just a subset of P4 repository
and/or with explicit "--changesfile" option.

A real-life example:
"""
    looking in ref refs/remotes/p4/mybranch for change 26894 using bisect...
    Reading pipe: git rev-parse refs/remotes/p4/mybranch
    trying: earliest  latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git cat-file commit 147f5d3292af2e1cc4a56a7b96db845144c68486
    current change 25339
    trying: earliest ^147f5d3292af2e1cc4a56a7b96db845144c68486 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^147f5d3292af2e1cc4a56a7b96db845144c68486
    Reading pipe: git cat-file commit 51db83df9d588010d0bd995641c85aa0408a5bb9
    current change 25420
    trying: earliest ^51db83df9d588010d0bd995641c85aa0408a5bb9 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^51db83df9d588010d0bd995641c85aa0408a5bb9
    Reading pipe: git cat-file commit e8f83909ceb570f5a7e48c2853f3c5d8207cea52
    current change 25448
    trying: earliest ^e8f83909ceb570f5a7e48c2853f3c5d8207cea52 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^e8f83909ceb570f5a7e48c2853f3c5d8207cea52
    Reading pipe: git cat-file commit 09a48eb7acd594dce52e06681be9c366e1844d66
    current change 25521
    trying: earliest ^09a48eb7acd594dce52e06681be9c366e1844d66 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^09a48eb7acd594dce52e06681be9c366e1844d66
    Reading pipe: git cat-file commit 4daff81c520a82678e1ef347f2b5e97258101ae1
    current change 26907
    trying: earliest ^09a48eb7acd594dce52e06681be9c366e1844d66 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^09a48eb7acd594dce52e06681be9c366e1844d66
    Reading pipe: git cat-file commit 4daff81c520a82678e1ef347f2b5e97258101ae1
    current change 26907
    trying: earliest ^09a48eb7acd594dce52e06681be9c366e1844d66 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^09a48eb7acd594dce52e06681be9c366e1844d66
    Reading pipe: git cat-file commit 4daff81c520a82678e1ef347f2b5e97258101ae1
    current change 26907
    ...
"""

The fix is two-fold:
 * detect an infinite loop and die right away
   instead of looping forever;
 * make sure, `git rev-list --bisect` can't return "latestCommit" again
   by excluding it from the rev-list range explicitly.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---

Notes:
    I don't have a simple test-case for this yet,
    and I was able to perform a few complex initial `git p4 sync` runs
    without hitting this problem.
    
    I suspect, I had somehow messed up with branch definitions
    and --changesfile option at some point.

 git-p4.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 5b79920f46..c0a3068b6f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3323,11 +3323,13 @@ def gitCommitByP4Change(self, ref, change):
                 return next
 
             if currentChange < change:
                 earliestCommit = "^%s" % next
             else:
-                latestCommit = "%s" % next
+                if next == latestCommit:
+                    die("Infinite loop while looking in ref %s for change %s. Check your branch mappings" % (ref, change))
+                latestCommit = "%s^@" % next
 
         return ""
 
     def importNewBranch(self, branch, maxChange):
         # make fast-import flush all changes to disk and update the refs using the checkpoint
-- 
2.19.2

[PATCH v2 2/7] git-p4: match branches case insensitively if configured

[Mazo, Andrey]

git-p4 knows how to handle case insensitivity in file paths
if core.ignorecase is set.
However, when determining a branch for a file,
it still does a case-sensitive prefix match.
This may result in some file changes to be lost on import.

For example, given the following commits
 1. add //depot/main/file1
 2. add //depot/DirA/file2
 3. add //depot/dira/file3
 4. add //depot/DirA/file4
and "branchList = main:DirA" branch mapping,
commit 3 will be lost.

So, do branch search case insensitively if running with core.ignorecase set.
Teach splitFilesIntoBranches() to use the p4PathStartsWith() function
for path prefix matches instead of always case-sensitive match.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index c0a3068b6f..91c610f960 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2721,11 +2721,11 @@ def splitFilesIntoBranches(self, commit):
                 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, e.g.
-                if relPath.startswith(branch + "/"):
+                if p4PathStartsWith(relPath, branch + "/"):
                     if branch not in branches:
                         branches[branch] = []
                     branches[branch].append(file)
                     break
 
-- 
2.19.2

Re: [PATCH v2 2/7] git-p4: match branches case insensitively if configured

[Luke Diamand]

On Thu, 21 Mar 2019 at 22:32, Mazo, Andrey <amazo@checkvideo.com> wrote:
>
> git-p4 knows how to handle case insensitivity in file paths
> if core.ignorecase is set.
> However, when determining a branch for a file,
> it still does a case-sensitive prefix match.
> This may result in some file changes to be lost on import.
>
> For example, given the following commits
>  1. add //depot/main/file1
>  2. add //depot/DirA/file2
>  3. add //depot/dira/file3
>  4. add //depot/DirA/file4
> and "branchList = main:DirA" branch mapping,
> commit 3 will be lost.
>
> So, do branch search case insensitively if running with core.ignorecase set.
> Teach splitFilesIntoBranches() to use the p4PathStartsWith() function
> for path prefix matches instead of always case-sensitive match.

I wonder what other code paths break due to this problem!

Looks reasonable but I fear there may be some other holes in there -
quickly looking through the code suggests there are several other
places this problem occurs.

Luke

>
> Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
> ---
>  git-p4.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index c0a3068b6f..91c610f960 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2721,11 +2721,11 @@ def splitFilesIntoBranches(self, commit):
>                  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, e.g.
> -                if relPath.startswith(branch + "/"):
> +                if p4PathStartsWith(relPath, branch + "/"):
>                      if branch not in branches:
>                          branches[branch] = []
>                      branches[branch].append(file)
>                      break
>
> --
> 2.19.2
>

Re: [PATCH v2 2/7] git-p4: match branches case insensitively if configured

[Mazo, Andrey]

From: "Mazo, Andrey" <amazo@checkvideo.com>


23.03.2019, 05:16, "Luke Diamand" <luke@diamand.org>:
> On Thu, 21 Mar 2019 at 22:32, Mazo, Andrey <amazo@checkvideo.com> wrote:
>>  git-p4 knows how to handle case insensitivity in file paths
>>  if core.ignorecase is set.
>>  However, when determining a branch for a file,
>>  it still does a case-sensitive prefix match.
>>  This may result in some file changes to be lost on import.
>>
>>  For example, given the following commits
>>   1. add //depot/main/file1
>>   2. add //depot/DirA/file2
>>   3. add //depot/dira/file3
>>   4. add //depot/DirA/file4
>>  and "branchList = main:DirA" branch mapping,
>>  commit 3 will be lost.
>>
>>  So, do branch search case insensitively if running with core.ignorecase set.
>>  Teach splitFilesIntoBranches() to use the p4PathStartsWith() function
>>  for path prefix matches instead of always case-sensitive match.
>
> I wonder what other code paths break due to this problem!
>
> Looks reasonable but I fear there may be some other holes in there -
> quickly looking through the code suggests there are several other
> places this problem occurs.

From a quick search for .startswith(), I only see that stripRepoPath() might have a similar problem in useclientspec case.
If you see other apparent problematic places, could you, please, point them out?

Or let me try to come up with a test case, and see what other places break.

Thank you,
Andrey.

[PATCH v2 3/7] git-p4: don't groom exclude path list on every commit

[Mazo, Andrey]

Currently, `cloneExclude` array is being groomed (by removing trailing "...")
on every changeset.
(since `extractFilesFromCommit()` is called on every imported changeset)

As a micro-optimization, do it once while parsing arguments.
Also, prepend "/" and remove trailing "..." at the same time.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 git-p4.py | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 91c610f960..a9f53e5b88 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1314,11 +1314,11 @@ class Command:
     def __init__(self):
         self.usage = "usage: %prog [options]"
         self.needsGit = True
         self.verbose = False
 
-    # This is required for the "append" cloneExclude action
+    # This is required for the "append" update_shelve action
     def ensure_value(self, attr, value):
         if not hasattr(self, attr) or getattr(self, attr) is None:
             setattr(self, attr, value)
         return getattr(self, attr)
 
@@ -2528,10 +2528,15 @@ def map_in_client(self, depot_path):
             return self.client_spec_path_cache[depot_path]
 
         die( "Error: %s is not found in client spec path" % depot_path )
         return ""
 
+def cloneExcludeCallback(option, opt_str, value, parser):
+    # prepend "/" because the first "/" was consumed as part of the option itself.
+    # ("-//depot/A/..." becomes "/depot/A/..." after option parsing)
+    parser.values.cloneExclude += ["/" + re.sub(r"\.\.\.$", "", value)]
+
 class P4Sync(Command, P4UserMap):
 
     def __init__(self):
         Command.__init__(self)
         P4UserMap.__init__(self)
@@ -2551,11 +2556,11 @@ def __init__(self):
                 optparse.make_option("--keep-path", dest="keepRepoPath", action='store_true',
                                      help="Keep entire BRANCH/DIR/SUBDIR prefix during import"),
                 optparse.make_option("--use-client-spec", dest="useClientSpec", action='store_true',
                                      help="Only sync files that are included in the Perforce Client Spec"),
                 optparse.make_option("-/", dest="cloneExclude",
-                                     action="append", type="string",
+                                     action="callback", callback=cloneExcludeCallback, type="string",
                                      help="exclude depot path"),
         ]
         self.description = """Imports from Perforce into a git repository.\n
     example:
     //depot/my/project/ -- to import the current head
@@ -2617,12 +2622,10 @@ def checkpoint(self):
         out = self.gitOutput.readline()
         if self.verbose:
             print("checkpoint finished: " + out)
 
     def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
-        self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
-                             for path in self.cloneExclude]
         files = []
         fnum = 0
         while "depotFile%s" % fnum in commit:
             path =  commit["depotFile%s" % fnum]
 
@@ -3888,11 +3891,10 @@ def run(self, args):
 
         if not self.cloneDestination and len(depotPaths) > 1:
             self.cloneDestination = depotPaths[-1]
             depotPaths = depotPaths[:-1]
 
-        self.cloneExclude = ["/"+p for p in self.cloneExclude]
         for p in depotPaths:
             if not p.startswith("//"):
                 sys.stderr.write('Depot paths must start with "//": %s\n' % p)
                 return False
 
-- 
2.19.2

[PATCH v2 4/7] git-p4: add failing test for "don't exclude other files with same prefix"

[Mazo, Andrey]

In preparation for a fix, add a failing test case to test that
git-p4 doesn't exclude files with the same prefix unintentionally
when exclude paths are specified without a trailing /.
I.e., don't exclude "//depot/file_dont_exclude" if run with "-//depot/file".
or don't exclude "//depot/discard_file_not" if run with "-//depot/discard_file".

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 t/t9817-git-p4-exclude.sh | 51 +++++++++++++++++++++++++++++++++++----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/t/t9817-git-p4-exclude.sh b/t/t9817-git-p4-exclude.sh
index aac568eadf..1c22570797 100755
--- a/t/t9817-git-p4-exclude.sh
+++ b/t/t9817-git-p4-exclude.sh
@@ -20,49 +20,90 @@ test_expect_success 'create exclude repo' '
 	(
 		cd "$cli" &&
 		mkdir -p wanted discard &&
 		echo wanted >wanted/foo &&
 		echo discard >discard/foo &&
-		p4 add wanted/foo discard/foo &&
+		echo discard_file >discard_file &&
+		echo discard_file_not >discard_file_not &&
+		p4 add wanted/foo discard/foo discard_file discard_file_not &&
 		p4 submit -d "initial revision"
 	)
 '
 
 test_expect_success 'check the repo was created correctly' '
 	test_when_finished cleanup_git &&
 	git p4 clone --dest="$git" //depot/...@all &&
 	(
 		cd "$git" &&
 		test_path_is_file wanted/foo &&
-		test_path_is_file discard/foo
+		test_path_is_file discard/foo &&
+		test_path_is_file discard_file &&
+		test_path_is_file discard_file_not
 	)
 '
 
 test_expect_success 'clone, excluding part of repo' '
 	test_when_finished cleanup_git &&
 	git p4 clone -//depot/discard/... --dest="$git" //depot/...@all &&
 	(
 		cd "$git" &&
 		test_path_is_file wanted/foo &&
-		test_path_is_missing discard/foo
+		test_path_is_missing discard/foo &&
+		test_path_is_file discard_file &&
+		test_path_is_file discard_file_not
+	)
+'
+
+test_expect_failure 'clone, excluding single file, no trailing /' '
+	test_when_finished cleanup_git &&
+	git p4 clone -//depot/discard_file --dest="$git" //depot/...@all &&
+	(
+		cd "$git" &&
+		test_path_is_file wanted/foo &&
+		test_path_is_file discard/foo &&
+		test_path_is_missing discard_file &&
+		test_path_is_file discard_file_not
 	)
 '
 
 test_expect_success 'clone, then sync with exclude' '
 	test_when_finished cleanup_git &&
 	git p4 clone -//depot/discard/... --dest="$git" //depot/...@all &&
 	(
 		cd "$cli" &&
-		p4 edit wanted/foo discard/foo &&
+		p4 edit wanted/foo discard/foo discard_file_not &&
 		date >>wanted/foo &&
 		date >>discard/foo &&
+		date >>discard_file_not &&
 		p4 submit -d "updating" &&
 
 		cd "$git" &&
 		git p4 sync -//depot/discard/... &&
 		test_path_is_file wanted/foo &&
-		test_path_is_missing discard/foo
+		test_path_is_missing discard/foo &&
+		test_path_is_file discard_file &&
+		test_path_is_file discard_file_not
+	)
+'
+
+test_expect_failure 'clone, then sync with exclude, no trailing /' '
+	test_when_finished cleanup_git &&
+	git p4 clone -//depot/discard/... -//depot/discard_file --dest="$git" //depot/...@all &&
+	(
+		cd "$cli" &&
+		p4 edit wanted/foo discard/foo discard_file_not &&
+		date >>wanted/foo &&
+		date >>discard/foo &&
+		date >>discard_file_not &&
+		p4 submit -d "updating" &&
+
+		cd "$git" &&
+		git p4 sync -//depot/discard/... -//depot/discard_file &&
+		test_path_is_file wanted/foo &&
+		test_path_is_missing discard/foo &&
+		test_path_is_missing discard_file &&
+		test_path_is_file discard_file_not
 	)
 '
 
 test_expect_success 'kill p4d' '
 	kill_p4d
-- 
2.19.2

[PATCH v2 5/7] git-p4: don't exclude other files with same prefix

[Mazo, Andrey]

Make sure not to exclude files unintentionally
if exclude paths are specified without a trailing /.
I.e., don't exclude "//depot/file_dont_exclude" if run with "-//depot/file".

Do this by ensuring that paths without a trailing "/" are only matched completely.

Also, abort path search on the first match as a micro-optimization.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 git-p4.py                 | 21 ++++++++++++++-------
 t/t9817-git-p4-exclude.sh |  4 ++--
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index a9f53e5b88..162877aa82 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2621,22 +2621,29 @@ def checkpoint(self):
         self.gitStream.write("progress checkpoint\n\n")
         out = self.gitOutput.readline()
         if self.verbose:
             print("checkpoint finished: " + out)
 
+    def isPathWanted(self, path):
+        for p in self.cloneExclude:
+            if p.endswith("/"):
+                if p4PathStartsWith(path, p):
+                    return False
+            # "-//depot/file1" without a trailing "/" should only exclude "file1", but not "file111" or "file1_dir/file2"
+            elif path.lower() == p.lower():
+                return False
+        for p in self.depotPaths:
+            if p4PathStartsWith(path, p):
+                return True
+        return False
+
     def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
         files = []
         fnum = 0
         while "depotFile%s" % fnum in commit:
             path =  commit["depotFile%s" % fnum]
-
-            if [p for p in self.cloneExclude
-                if p4PathStartsWith(path, p)]:
-                found = False
-            else:
-                found = [p for p in self.depotPaths
-                         if p4PathStartsWith(path, p)]
+            found = self.isPathWanted(path)
             if not found:
                 fnum = fnum + 1
                 continue
 
             file = {}
diff --git a/t/t9817-git-p4-exclude.sh b/t/t9817-git-p4-exclude.sh
index 1c22570797..275dd30425 100755
--- a/t/t9817-git-p4-exclude.sh
+++ b/t/t9817-git-p4-exclude.sh
@@ -51,11 +51,11 @@ test_expect_success 'clone, excluding part of repo' '
 		test_path_is_file discard_file &&
 		test_path_is_file discard_file_not
 	)
 '
 
-test_expect_failure 'clone, excluding single file, no trailing /' '
+test_expect_success 'clone, excluding single file, no trailing /' '
 	test_when_finished cleanup_git &&
 	git p4 clone -//depot/discard_file --dest="$git" //depot/...@all &&
 	(
 		cd "$git" &&
 		test_path_is_file wanted/foo &&
@@ -83,11 +83,11 @@ test_expect_success 'clone, then sync with exclude' '
 		test_path_is_file discard_file &&
 		test_path_is_file discard_file_not
 	)
 '
 
-test_expect_failure 'clone, then sync with exclude, no trailing /' '
+test_expect_success 'clone, then sync with exclude, no trailing /' '
 	test_when_finished cleanup_git &&
 	git p4 clone -//depot/discard/... -//depot/discard_file --dest="$git" //depot/...@all &&
 	(
 		cd "$cli" &&
 		p4 edit wanted/foo discard/foo discard_file_not &&
-- 
2.19.2

[PATCH v2 6/7] git-p4: add failing test for "git-p4: respect excluded paths when detecting branches"

[Mazo, Andrey]

In preparation for a fix, add a failing test case to test that
git-p4 doesn't exclude files despite being told to
when handling multiple branches.

I.e., it should exclude //depot/branch2/file2 when run with -//depot/branch2/file2,
but doesn't do this right now.

The test is based on 'git p4 clone complex branches' test with the following changes:
 * account for file3 moved from branch3 to branch4 in test 'git p4 submit to two branches in a single changelist';
 * account for branch6 created in test 'git p4 clone file subset branch';
 * file2 is expected to be missing from all branches due to explicit exclude.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 t/t9801-git-p4-branch.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 6a86d6996b..4729f470b2 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -409,10 +409,50 @@ test_expect_failure 'git p4 clone file subset branch' '
 		test_path_is_missing file2 &&
 		test_path_is_missing file3
 	)
 '
 
+# Check that excluded files are omitted during import
+test_expect_failure 'git p4 clone complex branches with excluded files' '
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$git" &&
+		git config git-p4.branchList branch1:branch2 &&
+		git config --add git-p4.branchList branch1:branch3 &&
+		git config --add git-p4.branchList branch1:branch4 &&
+		git config --add git-p4.branchList branch1:branch5 &&
+		git config --add git-p4.branchList branch1:branch6 &&
+		git p4 clone --dest=. --detect-branches -//depot/branch1/file2 -//depot/branch2/file2 -//depot/branch3/file2 -//depot/branch4/file2 -//depot/branch5/file2 -//depot/branch6/file2 //depot@all &&
+		git log --all --graph --decorate --stat &&
+		git reset --hard p4/depot/branch1 &&
+		test_path_is_file file1 &&
+		test_path_is_missing file2 &&
+		test_path_is_file file3 &&
+		git reset --hard p4/depot/branch2 &&
+		test_path_is_file file1 &&
+		test_path_is_missing file2 &&
+		test_path_is_missing file3 &&
+		git reset --hard p4/depot/branch3 &&
+		test_path_is_file file1 &&
+		test_path_is_missing file2 &&
+		test_path_is_missing file3 &&
+		git reset --hard p4/depot/branch4 &&
+		test_path_is_file file1 &&
+		test_path_is_missing file2 &&
+		test_path_is_file file3 &&
+		git reset --hard p4/depot/branch5 &&
+		test_path_is_file file1 &&
+		test_path_is_missing file2 &&
+		test_path_is_file file3 &&
+		git reset --hard p4/depot/branch6 &&
+		test_path_is_file file1 &&
+		test_path_is_missing file2 &&
+		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" &&
-- 
2.19.2

[PATCH v2 7/7] git-p4: respect excluded paths when detecting branches

[Mazo, Andrey]

Currently, excluded paths are only handled in the following cases:
 * no branch detection;
 * branch detection with using clientspec.

However, excluded paths are not respected in case of
branch detection without using clientspec.

Fix this by consulting the list of excluded paths
when splitting files across branches.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 git-p4.py                | 3 +--
 t/t9801-git-p4-branch.sh | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 162877aa82..148ea6f1b0 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2708,12 +2708,11 @@ def splitFilesIntoBranches(self, commit):
 
         branches = {}
         fnum = 0
         while "depotFile%s" % fnum in commit:
             path =  commit["depotFile%s" % fnum]
-            found = [p for p in self.depotPaths
-                     if p4PathStartsWith(path, p)]
+            found = self.isPathWanted(path)
             if not found:
                 fnum = fnum + 1
                 continue
 
             file = {}
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 4729f470b2..62a3929d8e 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -410,11 +410,11 @@ test_expect_failure 'git p4 clone file subset branch' '
 		test_path_is_missing file3
 	)
 '
 
 # Check that excluded files are omitted during import
-test_expect_failure 'git p4 clone complex branches with excluded files' '
+test_expect_success 'git p4 clone complex branches with excluded files' '
 	test_when_finished cleanup_git &&
 	test_create_repo "$git" &&
 	(
 		cd "$git" &&
 		git config git-p4.branchList branch1:branch2 &&
-- 
2.19.2

[RFC PATCH 0/2] git-p4: "alien" branches and load changelist info from file

[Mazo, Andrey]

This patch series introduces two experimental features to git-p4,
which unrelated to each other.
 1. The first patch adds support for so-called "alien" branches.
    The feature lets git-p4 create empty commits
    to make the history or tags more accurate.
    It is particularly useful when splitting a large Perforce depot
    into multiple git repositories.
 2. The second patch adds support for loading changelist information from a file.
    (`p4 -G describe` equivalent)
    The original use case is to be able to migrate a Perforce depot,
    which database got a little bit corrupted, into git.

It would be nice to get some feedback to see
if these features are usable in general and are worth mainlining at all.

The patches don't contain documentation or test changes yet,
because I wanted to get feedback first
if there is interest in mainlining these features in the first place.

This patch series should be applied on top of
"[PATCH v2 0/7] git-p4: a few assorted fixes for branches, excludes" [1]

[1] https://public-inbox.org/git/cover.1551485349.git.amazo@checkvideo.com/t/#m965fb5895d25d6b42638dd8efbb96e9fa9182978

Andrey Mazo (2):
  git-p4: introduce alien branch mappings
  git-p4: support loading changelist descriptions from files

 git-p4.py | 84 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 71 insertions(+), 13 deletions(-)


base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
prerequisite-patch-id: 23e039fec7a1f5c51c98326a14d788adb1ecb5ba
prerequisite-patch-id: 030a0acdce715ff99916fd412832e5a9471225c3
prerequisite-patch-id: 10661f77392f4131d2375976c77a7cd231fdf9ab
prerequisite-patch-id: a55360c904eba1b9e9c934405d3141eb96c5ad30
prerequisite-patch-id: 46357586199c02d956d53d782a12f1ee0c991302
prerequisite-patch-id: c683e7d6017580df9385a1544af409ca615d770c
prerequisite-patch-id: 411dcb5e95aff036e0cb3e850ea75f2424b260a6
-- 
2.19.2

[RFC PATCH 1/2] git-p4: introduce alien branch mappings

[Mazo, Andrey]

Labels in Perforce are not global, but can be placed on a particular view/subdirectory.
This might pose difficulties when importing only parts of Perforce depot into a git repository.
For example:
 1. Depot layout is as follows:
    //depot/metaproject/branch1/subprojectA/...
    //depot/metaproject/branch1/subprojectB/...
    //depot/metaproject/branch2/subprojectA/...
    //depot/metaproject/branch2/subprojectB/...
 2. Labels are placed as follows:
    * label 1A on //depot/metaproject/branch1/subprojectA/...
    * label 1B on //depot/metaproject/branch1/subprojectB/...
    * label 2A on //depot/metaproject/branch2/subprojectA/...
    * label 2B on //depot/metaproject/branch2/subprojectB/...
 3. The goal is to import
    subprojectA into subprojectA.git and
    subprojectB into subprojectB.git
    preserving all the branches and labels.
 4. Importing subprojectA.
    Label 1A is imported fine because it's placed on certain commit on branch1.
    However, label 1B is not imported because it's placed on a commit in another subproject:
    git-p4 says: "importing label 1B: could not find git commit for changelist ..."
    The same is with label 2A, which is imported; and 2B, which is not.

Currently, there is no easy way (that I'm aware of) to tell git-p4 to
import an empty commit into a desired branch,
so that a label placed on that changelist could be imported as well,
It might be possible to get a similar effect by importing both subprojectA and B in a single git repo,
and then running `git filter-branch --subdirectory-filter subprojectA`,
but this might produce way more irrelevant empty commits, than needed for labels.
(although imported changelists can be limited with git-p4 --changesfile option)

So, introduce a concept of an "alien" branch.

In the above example of importing subprojectA,
 * branch1/subprojectB is an alien branch for branch1/subprojectA;
 * branch2/subprojectB is an alien branch for branch2/subprojectA.
Any changelist for branch1/subprojectB will be imported into subprojectA.git branch1
as an empty commit for the sole purpose of being labeled with a tag later
or just to preserve the history of changes across the branches.

This relation between branches is specified in a similar way to branchList:
`git config --add git-p4.alienLabelBranchMap alien_branch:real_branch`

For the example of importing subprojectA above, config parameters are
```
git config --add git-p4.branchList branch1/subprojectA:branch2/subprojectA
git config --add git-p4.alienLabelBranchMap branch1/subprojectB:branch1/subprojectA
git config --add git-p4.alienLabelBranchMap branch2/subprojectB:branch2/subprojectA
```

A similar use case, is when a label is placed on a changelist for an excluded path.
 1. Depot layout is as follows:
    //depot/branch1/...
    //depot/branch1/exclude_me/...
 2. Labels are placed as follows:
    * label 1  on //depot/branch1/...
    * label 1E on //depot/branch1/exclude_me/...
 3. The goal is to import
    //depot/... into depot.git excluding files under
    //depot/branch1/exclude_me/...
    and preserving all the branches and labels.
 4. Importing subprojectA.
    Label 1 is imported fine because it's placed on certain commit on branch1.
    However, label 1E is not imported because it's placed on a commit which is excluded.

For this use case, the config would be
```
git config --add git-p4.alienLabelBranchMap branch1/exclude_me:branch1
```

Note that the current implementation doesn't process alien branches
when a clientspec is used.

Diff best viewed with --ignore-all-space .

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---

Notes:
    Documentation changes and tests are obviously missing,
    but I hoped to get some feedback on the idea overall
    before working on those.
    
    A better name for "alien" branches is very welcome.

 git-p4.py | 59 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 148ea6f1b0..40bc84573b 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2610,10 +2610,12 @@ def __init__(self):
 
         # map from branch depot path to parent branch
         self.knownBranches = {}
         self.initialParents = {}
 
+        self.knownAlienLabelBranches = {}
+
         self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 3600) / 60))
         self.labels = {}
 
     # Force a checkpoint in fast-import and wait for it to finish
     def checkpoint(self):
@@ -2705,14 +2707,37 @@ def splitFilesIntoBranches(self, commit):
         if self.clientSpecDirs:
             files = self.extractFilesFromCommit(commit)
             self.clientSpecDirs.update_client_spec_path_cache(files)
 
         branches = {}
+        alienBranches = {}
         fnum = 0
         while "depotFile%s" % fnum in commit:
             path =  commit["depotFile%s" % fnum]
             found = self.isPathWanted(path)
+
+            # start with the full relative path where this file would
+            # go in a p4 client
+            if self.useClientSpec:
+                # with a clientspec, we won't know where the file goes if it's excluded
+                if found:
+                    relPath = self.clientSpecDirs.map_in_client(path)
+                else:
+                    relPath = None
+            else:
+                # without a clientspec, we can guess a file path even if it's excluded
+                relPath = self.stripRepoPath(path, self.depotPaths)
+
+            # Process alien branches before excludes (i.e. before respecting `found`) --
+            # picking (empty) commits from excluded paths is one of the use-cases for alien branches.
+            # We don't commit any files for alien branches, so don't violate excludes.
+            for (alienPath, ourPath) in self.knownAlienLabelBranches.items():
+                if relPath is not None and p4PathStartsWith(relPath, alienPath + '/') and ourPath in self.knownBranches:
+                    # we don't put any files since they are under the paths, we're not interested in.
+                    # however, we still want the commit message and etc.
+                    alienBranches[ourPath] = [{"alienPath": alienPath}]
+
             if not found:
                 fnum = fnum + 1
                 continue
 
             file = {}
@@ -2720,26 +2745,23 @@ def splitFilesIntoBranches(self, commit):
             file["rev"] = commit["rev%s" % fnum]
             file["action"] = commit["action%s" % fnum]
             file["type"] = commit["type%s" % fnum]
             fnum = fnum + 1
 
-            # 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, e.g.
                 if p4PathStartsWith(relPath, branch + "/"):
                     if branch not in branches:
                         branches[branch] = []
                     branches[branch].append(file)
                     break
 
+        # not on any branch, check out our alien mapping
+        if not branches:
+            branches = alienBranches
+
         return branches
 
     def writeToGitStream(self, gitMode, relPath, contents):
         self.gitStream.write('M %s inline %s\n' % (gitMode, relPath))
         self.gitStream.write('data %d\n' % sum(len(d) for d in contents))
@@ -3283,10 +3305,17 @@ def getBranchMappingFromGitBranches(self):
                 branch = "main"
             else:
                 branch = branch[len(self.projectName):]
             self.knownBranches[branch] = branch
 
+    def getAlienLabelBranchMapping(self):
+        alienLabelBranches = gitConfigList("git-p4.alienLabelBranchMap")
+        for mapping in alienLabelBranches:
+            if mapping:
+                (alien, ours) = mapping.split(":")
+                self.knownAlienLabelBranches[alien] = ours
+
     def updateOptionDict(self, d):
         option_keys = {}
         if self.keepRepoPath:
             option_keys['keepRepoPath'] = 1
 
@@ -3396,17 +3425,22 @@ def importChanges(self, changes, origin_revision=0):
 
             try:
                 if self.detectBranches:
                     branches = self.splitFilesIntoBranches(description)
                     for branch in branches.keys():
-                        ## HACK  --hwn
-                        branchPrefix = self.depotPaths[0] + branch + "/"
-                        self.branchPrefixes = [ branchPrefix ]
+                        # hack branch prefix and file list for alien branches
+                        if branches[branch] and "alienPath" in branches[branch][0]:
+                            branchPrefix = self.depotPaths[0] + branches[branch][0]["alienPath"] + "/"
+                            filesForCommit = []
+                        else:
+                            ## HACK  --hwn
+                            branchPrefix = self.depotPaths[0] + branch + "/"
+                            filesForCommit = branches[branch]
 
-                        parent = ""
+                        self.branchPrefixes = [branchPrefix]
 
-                        filesForCommit = branches[branch]
+                        parent = ""
 
                         if self.verbose:
                             print("branch is %s" % branch)
 
                         self.updatedBranches.add(branch)
@@ -3715,10 +3749,11 @@ def run(self, args):
 
             if self.hasOrigin:
                 self.getBranchMappingFromGitBranches()
             else:
                 self.getBranchMapping()
+                self.getAlienLabelBranchMapping()
             if self.verbose:
                 print("p4-git branches: %s" % self.p4BranchesInGit)
                 print("initial parents: %s" % self.initialParents)
             for b in self.p4BranchesInGit:
                 if b != "master":
-- 
2.19.2

Re: [RFC PATCH 1/2] git-p4: introduce alien branch mappings

[Luke Diamand]

On Fri, 22 Mar 2019 at 19:54, Mazo, Andrey <amazo@checkvideo.com> wrote:
>
> Labels in Perforce are not global, but can be placed on a particular view/subdirectory.
> This might pose difficulties when importing only parts of Perforce depot into a git repository.
> For example:
>  1. Depot layout is as follows:
>     //depot/metaproject/branch1/subprojectA/...
>     //depot/metaproject/branch1/subprojectB/...
>     //depot/metaproject/branch2/subprojectA/...
>     //depot/metaproject/branch2/subprojectB/...
>  2. Labels are placed as follows:
>     * label 1A on //depot/metaproject/branch1/subprojectA/...
>     * label 1B on //depot/metaproject/branch1/subprojectB/...
>     * label 2A on //depot/metaproject/branch2/subprojectA/...
>     * label 2B on //depot/metaproject/branch2/subprojectB/...
>  3. The goal is to import
>     subprojectA into subprojectA.git and
>     subprojectB into subprojectB.git
>     preserving all the branches and labels.
>  4. Importing subprojectA.
>     Label 1A is imported fine because it's placed on certain commit on branch1.
>     However, label 1B is not imported because it's placed on a commit in another subproject:
>     git-p4 says: "importing label 1B: could not find git commit for changelist ..."
>     The same is with label 2A, which is imported; and 2B, which is not.
>
> Currently, there is no easy way (that I'm aware of) to tell git-p4 to
> import an empty commit into a desired branch,
> so that a label placed on that changelist could be imported as well,

So there is a file in subprojectA/foo.c@41.
And label 1B is against //depot/metaproject/branch1/subprojectB/bar.c@42.

And I suppose in Perforce you could still checkout subprojectA at
change 42 and you would get change 41.

But with the way git-p4 works, the label just gets discarded.

You want to be able to checkout the subjectA with a tag called 1B and
get the file contents as of 42.

I wonder if it would be easier to teach the code in importP4Labels to
go searching harder for the next lower changelist number?

Where it currently says "could not find git commit"... could it do
something like "p4 changes -m1 //depot/path/...@LABEL" and use that
instead?

I'm not sure if that would work but it would mean you wouldn't need
any extra configuration to maintain.

But perhaps I have misunderstood what you're trying to do here!
Perhaps a failing test case might help explain it better?

Thanks
Luke



> It might be possible to get a similar effect by importing both subprojectA and B in a single git repo,
> and then running `git filter-branch --subdirectory-filter subprojectA`,
> but this might produce way more irrelevant empty commits, than needed for labels.
> (although imported changelists can be limited with git-p4 --changesfile option)
>
> So, introduce a concept of an "alien" branch.
>
> In the above example of importing subprojectA,
>  * branch1/subprojectB is an alien branch for branch1/subprojectA;
>  * branch2/subprojectB is an alien branch for branch2/subprojectA.
> Any changelist for branch1/subprojectB will be imported into subprojectA.git branch1
> as an empty commit for the sole purpose of being labeled with a tag later
> or just to preserve the history of changes across the branches.
>
> This relation between branches is specified in a similar way to branchList:
> `git config --add git-p4.alienLabelBranchMap alien_branch:real_branch`
>
> For the example of importing subprojectA above, config parameters are
> ```
> git config --add git-p4.branchList branch1/subprojectA:branch2/subprojectA
> git config --add git-p4.alienLabelBranchMap branch1/subprojectB:branch1/subprojectA
> git config --add git-p4.alienLabelBranchMap branch2/subprojectB:branch2/subprojectA
> ```
>
> A similar use case, is when a label is placed on a changelist for an excluded path.
>  1. Depot layout is as follows:
>     //depot/branch1/...
>     //depot/branch1/exclude_me/...
>  2. Labels are placed as follows:
>     * label 1  on //depot/branch1/...
>     * label 1E on //depot/branch1/exclude_me/...
>  3. The goal is to import
>     //depot/... into depot.git excluding files under
>     //depot/branch1/exclude_me/...
>     and preserving all the branches and labels.
>  4. Importing subprojectA.
>     Label 1 is imported fine because it's placed on certain commit on branch1.
>     However, label 1E is not imported because it's placed on a commit which is excluded.
>
> For this use case, the config would be
> ```
> git config --add git-p4.alienLabelBranchMap branch1/exclude_me:branch1
> ```
>
> Note that the current implementation doesn't process alien branches
> when a clientspec is used.
>
> Diff best viewed with --ignore-all-space .
>
> Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
> ---
>
> Notes:
>     Documentation changes and tests are obviously missing,
>     but I hoped to get some feedback on the idea overall
>     before working on those.
>
>     A better name for "alien" branches is very welcome.
>
>  git-p4.py | 59 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 12 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 148ea6f1b0..40bc84573b 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2610,10 +2610,12 @@ def __init__(self):
>
>          # map from branch depot path to parent branch
>          self.knownBranches = {}
>          self.initialParents = {}
>
> +        self.knownAlienLabelBranches = {}
> +
>          self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 3600) / 60))
>          self.labels = {}
>
>      # Force a checkpoint in fast-import and wait for it to finish
>      def checkpoint(self):
> @@ -2705,14 +2707,37 @@ def splitFilesIntoBranches(self, commit):
>          if self.clientSpecDirs:
>              files = self.extractFilesFromCommit(commit)
>              self.clientSpecDirs.update_client_spec_path_cache(files)
>
>          branches = {}
> +        alienBranches = {}
>          fnum = 0
>          while "depotFile%s" % fnum in commit:
>              path =  commit["depotFile%s" % fnum]
>              found = self.isPathWanted(path)
> +
> +            # start with the full relative path where this file would
> +            # go in a p4 client
> +            if self.useClientSpec:
> +                # with a clientspec, we won't know where the file goes if it's excluded
> +                if found:
> +                    relPath = self.clientSpecDirs.map_in_client(path)
> +                else:
> +                    relPath = None
> +            else:
> +                # without a clientspec, we can guess a file path even if it's excluded
> +                relPath = self.stripRepoPath(path, self.depotPaths)
> +
> +            # Process alien branches before excludes (i.e. before respecting `found`) --
> +            # picking (empty) commits from excluded paths is one of the use-cases for alien branches.
> +            # We don't commit any files for alien branches, so don't violate excludes.
> +            for (alienPath, ourPath) in self.knownAlienLabelBranches.items():
> +                if relPath is not None and p4PathStartsWith(relPath, alienPath + '/') and ourPath in self.knownBranches:
> +                    # we don't put any files since they are under the paths, we're not interested in.
> +                    # however, we still want the commit message and etc.
> +                    alienBranches[ourPath] = [{"alienPath": alienPath}]
> +
>              if not found:
>                  fnum = fnum + 1
>                  continue
>
>              file = {}
> @@ -2720,26 +2745,23 @@ def splitFilesIntoBranches(self, commit):
>              file["rev"] = commit["rev%s" % fnum]
>              file["action"] = commit["action%s" % fnum]
>              file["type"] = commit["type%s" % fnum]
>              fnum = fnum + 1
>
> -            # 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, e.g.
>                  if p4PathStartsWith(relPath, branch + "/"):
>                      if branch not in branches:
>                          branches[branch] = []
>                      branches[branch].append(file)
>                      break
>
> +        # not on any branch, check out our alien mapping
> +        if not branches:
> +            branches = alienBranches
> +
>          return branches
>
>      def writeToGitStream(self, gitMode, relPath, contents):
>          self.gitStream.write('M %s inline %s\n' % (gitMode, relPath))
>          self.gitStream.write('data %d\n' % sum(len(d) for d in contents))
> @@ -3283,10 +3305,17 @@ def getBranchMappingFromGitBranches(self):
>                  branch = "main"
>              else:
>                  branch = branch[len(self.projectName):]
>              self.knownBranches[branch] = branch
>
> +    def getAlienLabelBranchMapping(self):
> +        alienLabelBranches = gitConfigList("git-p4.alienLabelBranchMap")
> +        for mapping in alienLabelBranches:
> +            if mapping:
> +                (alien, ours) = mapping.split(":")
> +                self.knownAlienLabelBranches[alien] = ours
> +
>      def updateOptionDict(self, d):
>          option_keys = {}
>          if self.keepRepoPath:
>              option_keys['keepRepoPath'] = 1
>
> @@ -3396,17 +3425,22 @@ def importChanges(self, changes, origin_revision=0):
>
>              try:
>                  if self.detectBranches:
>                      branches = self.splitFilesIntoBranches(description)
>                      for branch in branches.keys():
> -                        ## HACK  --hwn
> -                        branchPrefix = self.depotPaths[0] + branch + "/"
> -                        self.branchPrefixes = [ branchPrefix ]
> +                        # hack branch prefix and file list for alien branches
> +                        if branches[branch] and "alienPath" in branches[branch][0]:
> +                            branchPrefix = self.depotPaths[0] + branches[branch][0]["alienPath"] + "/"
> +                            filesForCommit = []
> +                        else:
> +                            ## HACK  --hwn
> +                            branchPrefix = self.depotPaths[0] + branch + "/"
> +                            filesForCommit = branches[branch]
>
> -                        parent = ""
> +                        self.branchPrefixes = [branchPrefix]
>
> -                        filesForCommit = branches[branch]
> +                        parent = ""
>
>                          if self.verbose:
>                              print("branch is %s" % branch)
>
>                          self.updatedBranches.add(branch)
> @@ -3715,10 +3749,11 @@ def run(self, args):
>
>              if self.hasOrigin:
>                  self.getBranchMappingFromGitBranches()
>              else:
>                  self.getBranchMapping()
> +                self.getAlienLabelBranchMapping()
>              if self.verbose:
>                  print("p4-git branches: %s" % self.p4BranchesInGit)
>                  print("initial parents: %s" % self.initialParents)
>              for b in self.p4BranchesInGit:
>                  if b != "master":
> --
> 2.19.2
>

Re: [RFC PATCH 1/2] git-p4: introduce alien branch mappings

[Mazo, Andrey]

>> Labels in Perforce are not global, but can be placed on a particular view/subdirectory.
>> This might pose difficulties when importing only parts of Perforce depot into a git repository.
>> For example:
>>  1. Depot layout is as follows:
>>     //depot/metaproject/branch1/subprojectA/...
>>     //depot/metaproject/branch1/subprojectB/...
>>     //depot/metaproject/branch2/subprojectA/...
>>     //depot/metaproject/branch2/subprojectB/...
>>  2. Labels are placed as follows:
>>     * label 1A on //depot/metaproject/branch1/subprojectA/...
>>     * label 1B on //depot/metaproject/branch1/subprojectB/...
>>     * label 2A on //depot/metaproject/branch2/subprojectA/...
>>     * label 2B on //depot/metaproject/branch2/subprojectB/...
>>  3. The goal is to import
>>     subprojectA into subprojectA.git and
>>     subprojectB into subprojectB.git
>>     preserving all the branches and labels.
>>  4. Importing subprojectA.
>>     Label 1A is imported fine because it's placed on certain commit on branch1.
>>     However, label 1B is not imported because it's placed on a commit in another subproject:
>>     git-p4 says: "importing label 1B: could not find git commit for changelist ..."
>>     The same is with label 2A, which is imported; and 2B, which is not.
>>
>> Currently, there is no easy way (that I'm aware of) to tell git-p4 to
>> import an empty commit into a desired branch,
>> so that a label placed on that changelist could be imported as well,
> 
> So there is a file in subprojectA/foo.c@41.
> And label 1B is against //depot/metaproject/branch1/subprojectB/bar.c@42.
> 
> And I suppose in Perforce you could still checkout subprojectA at
> change 42 and you would get change 41.
> 
> But with the way git-p4 works, the label just gets discarded.

Yes, exactly.

> You want to be able to checkout the subjectA with a tag called 1B and
> get the file contents as of 42.
> 
> I wonder if it would be easier to teach the code in importP4Labels to
> go searching harder for the next lower changelist number?
> 
> Where it currently says "could not find git commit"... could it do
> something like "p4 changes -m1 //depot/path/...@LABEL" and use that
> instead?
> 
> I'm not sure if that would work but it would mean you wouldn't need
> any extra configuration to maintain.

Yeah, that's a great idea!
I think, it should work pretty well in simpler cases for sure.
Initially, I was thinking, that I needed an explicit configuration option
to choose the proper branch/subproject in a more complicated case,
but let me give it a try to your idea -- hopefully it just works.

Some new option like git-p4.allowInexactLabels to enable this behavior?
Don't think it should be enabled by default unless git-p4.labelImportRegexp is set, right?

> But perhaps I have misunderstood what you're trying to do here!
> Perhaps a failing test case might help explain it better?

No, I think, you got it right!
Thank you for the great suggestion!

Yeah, let me see if I can get a simple but representative test case.

> 
> Thanks
> Luke

[RFC PATCH 1/1] git-p4: inexact label detection

[Mazo, Andrey]

Labels in Perforce are not global, but can be placed on a particular view/subdirectory.
This might pose difficulties when importing only parts of Perforce depot into a git repository.
For example:
 1. Depot layout is as follows:
    //depot/metaproject/branch1/subprojectA/...
    //depot/metaproject/branch1/subprojectB/...
    //depot/metaproject/branch2/subprojectA/...
    //depot/metaproject/branch2/subprojectB/...
 2. Labels are placed as follows:
    * label 1A on //depot/metaproject/branch1/subprojectA/...
    * label 1B on //depot/metaproject/branch1/subprojectB/...
    * label 2A on //depot/metaproject/branch2/subprojectA/...
    * label 2B on //depot/metaproject/branch2/subprojectB/...
 3. The goal is to import
    subprojectA into subprojectA.git and
    subprojectB into subprojectB.git
    preserving all the branches and labels.
 4. Importing subprojectA.
    Label 1A is imported fine because it's placed on certain commit on branch1.
    However, label 1B is not imported because it's placed on a commit in another subproject:
    git-p4 says: "importing label 1B: could not find git commit for changelist ..."
    The same is with label 2A, which is imported; and 2B, which is not.

Currently, there is no easy way (that I'm aware of) to tell git-p4 to
import an empty commit into a desired branch,
so that a label placed on that changelist could be imported as well,
It might be possible to get a similar effect by importing both subprojectA and B in a single git repo,
and then running `git filter-branch --subdirectory-filter subprojectA`,
but this might produce way more irrelevant empty commits, than needed for labels.
(although imported changelists can be limited with git-p4 --changesfile option)
Also, `git filter-branch` is harder to use for incremental imports
or when changes are submitted from git back to Perforce.

As suggested by Luke,
instead of creating an empty commit for the sole purpose of being tagged later,
teach git-p4 to search harder for the next lower changelist,
corresponding to the label in question.

Do this by finding the highest changelist up to the label under all known branches,
(branches are finalized by the time importP4Labels() runs)
and using it instead of a depot-wide changelist corresponding to the label.

This new behavior may not be desired for people,
who want exact label <-> changelist relationship.
So, add a new boolean config parameter git-p4.allowInexactLabels (defaults to false)
to explicitly enable it if needed.
Also, this behavior only appears to be useful in case of multiple branches,
(otherwise, every Perforce changelist should appear in git)
so it's not engaged when running without branch detection.

Detect and report (--verbose) "inexact" tags,
i.e. tags placed on a lower changelist than was in Perforce.
Implement this by comparing a changelist for which a commit was found
with a changelist corresponding to the label on the whole depot.

Note, that the new "inexact" logic works slower
than the original code in case of numerous branches,
because p4 needs to calculate the most recent change for each branch path instead of just one.

This is an alternative solution to "alien" branches concept proposed earlier:
https://public-inbox.org/git/b02df749b9266ac8c73707617a171122156621ab.1553283214.git.amazo@checkvideo.com/

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
Suggested-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py | 45 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index b04f54b3d1..838c1b43d7 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3186,17 +3186,40 @@ def importP4Labels(self, stream, p4Labels):
             if name in ignoredP4Labels:
                 continue
 
             labelDetails = p4CmdList(['label', "-o", name])[0]
 
-            # get the most recent changelist for each file in this label
-            change = p4Cmd(["changes", "-m", "1"] + ["%s...@%s" % (p, name)
+            if self.detectBranches and gitConfigBool("git-p4.allowInexactLabels"):
+                doInexactLabels = True
+            else:
+                doInexactLabels = False
+
+            # get the most recent changelist in this label for the whole depot
+            depot_wide_changelist = p4Cmd(["changes", "-m", "1"] + ["%s...@%s" % (p, name)
                                 for p in self.depotPaths])
+            if 'change' in depot_wide_changelist:
+                depot_wide_changelist = int(depot_wide_changelist['change'])
+            else:
+                depot_wide_changelist = None
+
+            # get the most recent changelist for each file under branches of interest in this label
+            if doInexactLabels:
+                paths = ["%s...@%s" % (self.depotPaths[0] + p + '/', name) for p in self.knownBranches]
+                changes = p4CmdList(["changes", "-m", "1"] + paths)
+                changes = [int(c['change']) for c in changes if 'change' in c]
+
+                # there may be different "most recent" changelists for different paths.
+                # take the newest since some paths were just modified later than others.
+                if changes:
+                    changelist = max(changes)
+                else:
+                    changelist = None
+            else:
+                changelist = depot_wide_changelist
 
-            if 'change' in change:
+            if changelist:
                 # find the corresponding git commit; take the oldest commit
-                changelist = int(change['change'])
                 if changelist in self.committedChanges:
                     gitCommit = ":%d" % changelist       # use a fast-import mark
                     commitFound = True
                 else:
                     gitCommit = read_pipe(["git", "rev-list", "--max-count=1",
@@ -3216,14 +3239,24 @@ def importP4Labels(self, stream, p4Labels):
                         tmwhen = 1
 
                     when = int(time.mktime(tmwhen))
                     self.streamTag(stream, name, labelDetails, gitCommit, when)
                     if verbose:
-                        print("p4 label %s mapped to git commit %s" % (name, gitCommit))
+                        if depot_wide_changelist == changelist:
+                            isExact = ""
+                        else:
+                            isExact = " inexactly"
+                        print("p4 label %s mapped%s to git commit %s" % (name, isExact, gitCommit))
             else:
                 if verbose:
-                    print("Label %s has no changelists - possibly deleted?" % name)
+                    if depot_wide_changelist:
+                        # there is a changelist corresponding to this label,
+                        # but it's not under any branches of interest.
+                        print("Label %s has no changelists under detected branches -- ignoring" % name)
+                    else:
+                        # there is no changelist corresponding to this label in the whole depot
+                        print("Label %s has no changelists - possibly deleted?" % name)
 
             if not commitFound:
                 # We can't import this label; don't try again as it will get very
                 # expensive repeatedly fetching all the files for labels that will
                 # never be imported. If the label is moved in the future, the

base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
-- 
2.19.2

[RFC PATCH 2/2] git-p4: support loading changelist descriptions from files

[Mazo, Andrey]

Our Perforce server experienced some kind of database corruption a few years ago.
While the file data and revision history are mostly intact,
some metadata for several changesets got lost.
For example, inspecting certain changelists produces errors.
"""
$ p4 describe -s 12345
Date 2019/02/26 16:46:17:
Operation: user-describe
Operation 'user-describe' failed.
Change 12345 description missing!
"""

While some metadata (like changeset descriptions) is obviously lost,
most of it can be reconstructed via other commands:
 * `p4 changes -l -t //...@12345,12345` --
   to obtain date+time, author, beginning of changeset description;
 * `p4 files -a //...@12345,12345` --
   to obtain file revisions, file types, file actions;
 * `p4 diff2 -u //...@12344 //...@12345` --
   to get a unified diff of text files in a changeset;
 * `p4 print -o binary.blob@12345 //depot/binary.blob@12345` --
   to get a revision of a binary file.

It might be possible to teach git-p4 to fallback to other methods if `p4 describe` fails,
but it's probably too special-cased (really depends on kind and scale of DB corruption),
so some manual intervention is perhaps acceptable.

So, with some manual work, it's possible to reconstruct `p4 -G describe ...` output manually.
In our case, once git-p4 passes `p4 describe` stage,
it can proceed further just fine.
Thus, it's tempting to feed resurrected metadata to git-p4 when a normal `p4 describe` would fail.

This functionality may be useful to cache changelist information,
or to make some changes to changelist info before feeding it to git-p4.

A new config parameter is introduced to tell git-p4
to load certain changelist descriptions from files instead of from a server.
For simplicity, it's one pickled file per changelist.
```
git config --add git-p4.damagedChangelists 12345.pickled
git config --add git-p4.damagedChangelists 12346.pickled
```

The following trivial script may be used to produce pickled `p4 -G describe`-compatible output.
"""
 #!/usr/bin/python2

 import pickle
 import time

 # recovered commits of interest
 changes = [
     {
         'change':     '12345',
         'status':     'submitted',
         'code':       'stat',
         'user':       'username1',
         'time':       str(int(time.mktime(time.strptime('2019/02/28 16:00:30', '%Y/%m/%d %H:%M:%S')))),
         'client':     'username1_hostname1',
         'desc':       'A bug is fixed.\nDetails are below:<lost>\n',
         'depotFile0': '//depot/branch1/foo.sh',
         'action0':    'edit',
         'rev0':       '28',
         'type0':      'xtext',
         'depotFile1': '//depot/branch1/bar.py',
         'action1':    'edit',
         'rev1':       '43',
         'type1':      'text',
         'depotFile2': '//depot/branch1/baz.doc',
         'action2':    'edit',
         'rev2':       '8',
         'type2':      'binary',
         'depotFile3': '//depot/branch1/qqq.c',
         'action3':    'edit',
         'rev3':       '6',
         'type3':      'ktext',
     },
 ]

 for change in changes:
     pickle.dump(change, open('{0}.pickled'.format(change['change']), 'wb'))
"""

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---

Notes:
    Documentation changes and tests are obviously missing,
    but I hoped to get some feedback on the idea overall
    before working on those.

 git-p4.py | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 40bc84573b..3133419280 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -24,10 +24,11 @@
 import stat
 import zipfile
 import zlib
 import ctypes
 import errno
+import pickle
 
 # support basestring in python3
 try:
     unicode = unicode
 except NameError:
@@ -2615,10 +2616,12 @@ def __init__(self):
         self.knownAlienLabelBranches = {}
 
         self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 3600) / 60))
         self.labels = {}
 
+        self.damagedChangelists = {}
+
     # Force a checkpoint in fast-import and wait for it to finish
     def checkpoint(self):
         self.gitStream.write("checkpoint\n\n")
         self.gitStream.write("progress checkpoint\n\n")
         out = self.gitOutput.readline()
@@ -3312,10 +3315,25 @@ def getAlienLabelBranchMapping(self):
         for mapping in alienLabelBranches:
             if mapping:
                 (alien, ours) = mapping.split(":")
                 self.knownAlienLabelBranches[alien] = ours
 
+    def loadDamagedChangelists(self):
+        damagedChangelists = gitConfigList("git-p4.damagedChangelists")
+        for clPickled in damagedChangelists:
+            if not clPickled:
+                continue
+
+            try:
+                clDesc = pickle.load(open(clPickled, 'rb'))
+                if not ("status" in clDesc and "user" in clDesc and "time" in clDesc and "change" in clDesc):
+                    die("Changelist description read from {0} doesn't have required fields".format(clPickled))
+            except (IOError, TypeError) as e:
+                die("Can't read changelist description dict from {0}: {1}".format(clPickled, str(e)))
+
+            self.damagedChangelists[int(clDesc["change"])] = clDesc
+
     def updateOptionDict(self, d):
         option_keys = {}
         if self.keepRepoPath:
             option_keys['keepRepoPath'] = 1
 
@@ -3413,11 +3431,14 @@ def searchParent(self, parent, branch, target):
             return None
 
     def importChanges(self, changes, origin_revision=0):
         cnt = 1
         for change in changes:
-            description = p4_describe(change)
+            if change in self.damagedChangelists:
+                description = self.damagedChangelists[change]
+            else:
+                description = p4_describe(change)
             self.updateOptionDict(description)
 
             if not self.silent:
                 sys.stdout.write("\rImporting revision %s (%s%%)" % (change, cnt * 100 / len(changes)))
                 sys.stdout.flush()
@@ -3704,10 +3725,12 @@ def run(self, args):
                     bad_changesfile = True
                     break
         if bad_changesfile:
             die("Option --changesfile is incompatible with revision specifiers")
 
+        self.loadDamagedChangelists()
+
         newPaths = []
         for p in self.depotPaths:
             if p.find("@") != -1:
                 atIdx = p.index("@")
                 self.changeRange = p[atIdx:]
-- 
2.19.2

Re: [RFC PATCH 2/2] git-p4: support loading changelist descriptions from files

[Luke Diamand]

On Fri, 22 Mar 2019 at 19:54, Mazo, Andrey <amazo@checkvideo.com> wrote:
>
> Our Perforce server experienced some kind of database corruption a few years ago.
> While the file data and revision history are mostly intact,
> some metadata for several changesets got lost.

I think it's not unheard of for P4 databases to end up being corrupt,
as in your case.

It looks like the RCS files get updated, but the database files (i.e.
the metadata) do not, after which you have a bit of a problem.

So I guess this change could be quite useful, but it really needs some
documentation and tests to support it - git-p4 is already complicated
enough!

Your example script should probably use the same magic that the git-p4
script uses to pick the path to Python.

And perhaps come up with a nicer name than "damaged" - as you say, it
could also be used for other purposes.

> For example, inspecting certain changelists produces errors.
> """
> $ p4 describe -s 12345
> Date 2019/02/26 16:46:17:
> Operation: user-describe
> Operation 'user-describe' failed.
> Change 12345 description missing!
> """
>
> While some metadata (like changeset descriptions) is obviously lost,
> most of it can be reconstructed via other commands:
>  * `p4 changes -l -t //...@12345,12345` --
>    to obtain date+time, author, beginning of changeset description;
>  * `p4 files -a //...@12345,12345` --
>    to obtain file revisions, file types, file actions;
>  * `p4 diff2 -u //...@12344 //...@12345` --
>    to get a unified diff of text files in a changeset;
>  * `p4 print -o binary.blob@12345 //depot/binary.blob@12345` --
>    to get a revision of a binary file.
>
> It might be possible to teach git-p4 to fallback to other methods if `p4 describe` fails,
> but it's probably too special-cased (really depends on kind and scale of DB corruption),
> so some manual intervention is perhaps acceptable.
>
> So, with some manual work, it's possible to reconstruct `p4 -G describe ...` output manually.
> In our case, once git-p4 passes `p4 describe` stage,
> it can proceed further just fine.
> Thus, it's tempting to feed resurrected metadata to git-p4 when a normal `p4 describe` would fail.
>
> This functionality may be useful to cache changelist information,
> or to make some changes to changelist info before feeding it to git-p4.
>
> A new config parameter is introduced to tell git-p4
> to load certain changelist descriptions from files instead of from a server.
> For simplicity, it's one pickled file per changelist.
> ```
> git config --add git-p4.damagedChangelists 12345.pickled
> git config --add git-p4.damagedChangelists 12346.pickled
> ```
>
> The following trivial script may be used to produce pickled `p4 -G describe`-compatible output.
> """
>  #!/usr/bin/python2
>
>  import pickle
>  import time
>
>  # recovered commits of interest
>  changes = [
>      {
>          'change':     '12345',
>          'status':     'submitted',
>          'code':       'stat',
>          'user':       'username1',
>          'time':       str(int(time.mktime(time.strptime('2019/02/28 16:00:30', '%Y/%m/%d %H:%M:%S')))),
>          'client':     'username1_hostname1',
>          'desc':       'A bug is fixed.\nDetails are below:<lost>\n',
>          'depotFile0': '//depot/branch1/foo.sh',
>          'action0':    'edit',
>          'rev0':       '28',
>          'type0':      'xtext',
>          'depotFile1': '//depot/branch1/bar.py',
>          'action1':    'edit',
>          'rev1':       '43',
>          'type1':      'text',
>          'depotFile2': '//depot/branch1/baz.doc',
>          'action2':    'edit',
>          'rev2':       '8',
>          'type2':      'binary',
>          'depotFile3': '//depot/branch1/qqq.c',
>          'action3':    'edit',
>          'rev3':       '6',
>          'type3':      'ktext',
>      },
>  ]
>
>  for change in changes:
>      pickle.dump(change, open('{0}.pickled'.format(change['change']), 'wb'))
> """
>
> Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
> ---
>
> Notes:
>     Documentation changes and tests are obviously missing,
>     but I hoped to get some feedback on the idea overall
>     before working on those.
>
>  git-p4.py | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 40bc84573b..3133419280 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -24,10 +24,11 @@
>  import stat
>  import zipfile
>  import zlib
>  import ctypes
>  import errno
> +import pickle
>
>  # support basestring in python3
>  try:
>      unicode = unicode
>  except NameError:
> @@ -2615,10 +2616,12 @@ def __init__(self):
>          self.knownAlienLabelBranches = {}
>
>          self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 3600) / 60))
>          self.labels = {}
>
> +        self.damagedChangelists = {}
> +
>      # Force a checkpoint in fast-import and wait for it to finish
>      def checkpoint(self):
>          self.gitStream.write("checkpoint\n\n")
>          self.gitStream.write("progress checkpoint\n\n")
>          out = self.gitOutput.readline()
> @@ -3312,10 +3315,25 @@ def getAlienLabelBranchMapping(self):
>          for mapping in alienLabelBranches:
>              if mapping:
>                  (alien, ours) = mapping.split(":")
>                  self.knownAlienLabelBranches[alien] = ours
>
> +    def loadDamagedChangelists(self):
> +        damagedChangelists = gitConfigList("git-p4.damagedChangelists")
> +        for clPickled in damagedChangelists:
> +            if not clPickled:
> +                continue
> +
> +            try:
> +                clDesc = pickle.load(open(clPickled, 'rb'))
> +                if not ("status" in clDesc and "user" in clDesc and "time" in clDesc and "change" in clDesc):
> +                    die("Changelist description read from {0} doesn't have required fields".format(clPickled))
> +            except (IOError, TypeError) as e:
> +                die("Can't read changelist description dict from {0}: {1}".format(clPickled, str(e)))
> +
> +            self.damagedChangelists[int(clDesc["change"])] = clDesc
> +
>      def updateOptionDict(self, d):
>          option_keys = {}
>          if self.keepRepoPath:
>              option_keys['keepRepoPath'] = 1
>
> @@ -3413,11 +3431,14 @@ def searchParent(self, parent, branch, target):
>              return None
>
>      def importChanges(self, changes, origin_revision=0):
>          cnt = 1
>          for change in changes:
> -            description = p4_describe(change)
> +            if change in self.damagedChangelists:
> +                description = self.damagedChangelists[change]
> +            else:
> +                description = p4_describe(change)
>              self.updateOptionDict(description)
>
>              if not self.silent:
>                  sys.stdout.write("\rImporting revision %s (%s%%)" % (change, cnt * 100 / len(changes)))
>                  sys.stdout.flush()
> @@ -3704,10 +3725,12 @@ def run(self, args):
>                      bad_changesfile = True
>                      break
>          if bad_changesfile:
>              die("Option --changesfile is incompatible with revision specifiers")
>
> +        self.loadDamagedChangelists()
> +
>          newPaths = []
>          for p in self.depotPaths:
>              if p.find("@") != -1:
>                  atIdx = p.index("@")
>                  self.changeRange = p[atIdx:]
> --
> 2.19.2
>

Re: [RFC PATCH 2/2] git-p4: support loading changelist descriptions

[Mazo, Andrey]

23.03.2019, 04:44, "Luke Diamand" <luke@diamand.org>:
> On Fri, 22 Mar 2019 at 19:54, Mazo, Andrey <amazo@checkvideo.com> wrote:
>>  Our Perforce server experienced some kind of database corruption a few years ago.
>>  While the file data and revision history are mostly intact,
>>  some metadata for several changesets got lost.
>
> I think it's not unheard of for P4 databases to end up being corrupt,
> as in your case.
>
> It looks like the RCS files get updated, but the database files (i.e.
> the metadata) do not, after which you have a bit of a problem.

Yeah, that's exactly what happened! :/

> So I guess this change could be quite useful, but it really needs some
> documentation and tests to support it - git-p4 is already complicated
> enough!

Great, thanks for positive feedback.
Let me start working on these.
Luckily, the actual git-p4 changes are fairly straightforward.

> Your example script should probably use the same magic that the git-p4
> script uses to pick the path to Python.
Oh, you mean "#!/usr/bin/env python"?
Sure!

Actually, I also have a smarter "example script", which reconstructs `p4 describe` output from `p4 files` and `p4 change` without manual intervention.
It's still short enough to be posted inside commit message, but would it make sense to put it under contrib/ and reference from the docs?

> And perhaps come up with a nicer name than "damaged" - as you say, it
> could also be used for other purposes.
Does anything from the list below sound reasonable?
 1. git-p4.loadChangelistInfo
 2. git-p4.changelistInfoOverride
 3. git-p4.cachedChangelistInfo

>> For simplicity, it's one pickled file per changelist.
(talking to myself)
I realized `p4 -G describe` produces a marshalled output, not pickled one.
Don't know why I decided to use pickle instead of marshal back when I wrote it.
I'll switch to marshal for serialization/deserialization in the next reroll.
It'll be easier for testing as well, since `p4 -G describe` output could be fed directly to damagedChangelists machinery without the need to convert it from marshal to pickle.

Thank you,
Andrey

[PATCH v3 0/8] git-p4: a few assorted fixes for branches, excludes

[Mazo, Andrey]

This series fixes a few cases with branch detection
and handling of excludes by git-p4.

This is the third iteration of the patch series.
Changes since v2 [2]:
 * Added new test cases for case-insensitive branch detection.
Changes since v1 [1]:
 * Added new test case for excluded paths when detecting branches;
 * Added a new fix for excluded paths when detecting branches.

[1] https://public-inbox.org/git/cover.1551485349.git.amazo@checkvideo.com
[2] https://public-inbox.org/git/cover.1553207234.git.amazo@checkvideo.com/

Range-diff vs v2:
1:  3ac39171d4 = 1:  bd009a5ca5 git-p4: detect/prevent infinite loop in gitCommitByP4Change()
2:  e644a8ab49 < -:  ---------- git-p4: match branches case insensitively if configured
-:  ---------- > 2:  68b68ce1e4 git-p4: add failing test for "git-p4: match branches case insensitively if configured"
-:  ---------- > 3:  6eaad2582c git-p4: match branches case insensitively if configured
3:  44fed954dc = 4:  1bd5e170e0 git-p4: don't groom exclude path list on every commit
4:  a0d3fa6add = 5:  b657967154 git-p4: add failing test for "don't exclude other files with same prefix"
5:  3330f88a0d = 6:  035abfff2a git-p4: don't exclude other files with same prefix
6:  6170d45951 = 7:  2bde24b7e4 git-p4: add failing test for "git-p4: respect excluded paths when detecting branches"
7:  758d8e8486 = 8:  6d3ffb98a7 git-p4: respect excluded paths when detecting branches

Andrey Mazo (8):
  git-p4: detect/prevent infinite loop in gitCommitByP4Change()
  git-p4: add failing test for "git-p4: match branches case insensitively if configured"
  git-p4: match branches case insensitively if configured
  git-p4: don't groom exclude path list on every commit
  git-p4: add failing test for "don't exclude other files with same prefix"
  git-p4: don't exclude other files with same prefix
  git-p4: add failing test for "git-p4: respect excluded paths when detecting branches"
  git-p4: respect excluded paths when detecting branches

 git-p4.py                 |  44 ++++++++-----
 t/t9801-git-p4-branch.sh  | 132 ++++++++++++++++++++++++++++++++++++++
 t/t9817-git-p4-exclude.sh |  51 +++++++++++++--
 3 files changed, 205 insertions(+), 22 deletions(-)


base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
-- 
2.19.2

[PATCH v3 1/8] git-p4: detect/prevent infinite loop in gitCommitByP4Change()

[Mazo, Andrey]

Under certain circumstances, gitCommitByP4Change() can enter an infinite
loop resulting in `git p4 sync` hanging forever.

The problem is that
`git rev-list --bisect <latest> ^<earliest>` can return `<latest>`,
which would result in reinspecting <latest> and potentially an infinite loop.

This can happen when importing just a subset of P4 repository
and/or with explicit "--changesfile" option.

A real-life example:
"""
    looking in ref refs/remotes/p4/mybranch for change 26894 using bisect...
    Reading pipe: git rev-parse refs/remotes/p4/mybranch
    trying: earliest  latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git cat-file commit 147f5d3292af2e1cc4a56a7b96db845144c68486
    current change 25339
    trying: earliest ^147f5d3292af2e1cc4a56a7b96db845144c68486 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^147f5d3292af2e1cc4a56a7b96db845144c68486
    Reading pipe: git cat-file commit 51db83df9d588010d0bd995641c85aa0408a5bb9
    current change 25420
    trying: earliest ^51db83df9d588010d0bd995641c85aa0408a5bb9 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^51db83df9d588010d0bd995641c85aa0408a5bb9
    Reading pipe: git cat-file commit e8f83909ceb570f5a7e48c2853f3c5d8207cea52
    current change 25448
    trying: earliest ^e8f83909ceb570f5a7e48c2853f3c5d8207cea52 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^e8f83909ceb570f5a7e48c2853f3c5d8207cea52
    Reading pipe: git cat-file commit 09a48eb7acd594dce52e06681be9c366e1844d66
    current change 25521
    trying: earliest ^09a48eb7acd594dce52e06681be9c366e1844d66 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^09a48eb7acd594dce52e06681be9c366e1844d66
    Reading pipe: git cat-file commit 4daff81c520a82678e1ef347f2b5e97258101ae1
    current change 26907
    trying: earliest ^09a48eb7acd594dce52e06681be9c366e1844d66 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^09a48eb7acd594dce52e06681be9c366e1844d66
    Reading pipe: git cat-file commit 4daff81c520a82678e1ef347f2b5e97258101ae1
    current change 26907
    trying: earliest ^09a48eb7acd594dce52e06681be9c366e1844d66 latest 4daff81c520a82678e1ef347f2b5e97258101ae1
    Reading pipe: git rev-list --bisect 4daff81c520a82678e1ef347f2b5e97258101ae1 ^09a48eb7acd594dce52e06681be9c366e1844d66
    Reading pipe: git cat-file commit 4daff81c520a82678e1ef347f2b5e97258101ae1
    current change 26907
    ...
"""

The fix is two-fold:
 * detect an infinite loop and die right away
   instead of looping forever;
 * make sure, `git rev-list --bisect` can't return "latestCommit" again
   by excluding it from the rev-list range explicitly.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---

Notes:
    I don't have a simple test-case for this yet,
    and I was able to perform a few complex initial `git p4 sync` runs
    without hitting this problem.
    
    I suspect, I had somehow messed up with branch definitions
    and --changesfile option at some point.

 git-p4.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 5b79920f46..c0a3068b6f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3323,11 +3323,13 @@ def gitCommitByP4Change(self, ref, change):
                 return next
 
             if currentChange < change:
                 earliestCommit = "^%s" % next
             else:
-                latestCommit = "%s" % next
+                if next == latestCommit:
+                    die("Infinite loop while looking in ref %s for change %s. Check your branch mappings" % (ref, change))
+                latestCommit = "%s^@" % next
 
         return ""
 
     def importNewBranch(self, branch, maxChange):
         # make fast-import flush all changes to disk and update the refs using the checkpoint
-- 
2.19.2

[PATCH v3 2/8] git-p4: add failing test for "git-p4: match branches case insensitively if configured"

[Mazo, Andrey]

In preparation for a fix, add a failing test case to test that
git-p4 doesn't fold the case in file paths
when doing branch detection case insensitively.
(i.e. when core.ignorecase is set)

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 t/t9801-git-p4-branch.sh | 92 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 6a86d6996b..c48532e12b 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -608,10 +608,102 @@ test_expect_success 'Update a file in git side and submit to P4 using client vie
 		cd branch1 &&
 		grep "client spec" file1
 	)
 '
 
+test_expect_success 'restart p4d (case folding enabled)' '
+	kill_p4d &&
+	start_p4d -C1
+'
+
+#
+# 1: //depot/main/mf1
+# 2: integrate //depot/main/... -> //depot/branch1/...
+# 3: //depot/main/mf2
+# 4: //depot/BRANCH1/B1f3
+# 5: //depot/branch1/b1f4
+#
+test_expect_success !CASE_INSENSITIVE_FS 'basic p4 branches for case folding' '
+	(
+		cd "$cli" &&
+		mkdir -p main &&
+
+		echo mf1 >main/mf1 &&
+		p4 add main/mf1 &&
+		p4 submit -d "main/mf1" &&
+
+		p4 integrate //depot/main/... //depot/branch1/... &&
+		p4 submit -d "integrate main to branch1" &&
+
+		echo mf2 >main/mf2 &&
+		p4 add main/mf2 &&
+		p4 submit -d "main/mf2" &&
+
+		mkdir BRANCH1 &&
+		echo B1f3 >BRANCH1/B1f3 &&
+		p4 add BRANCH1/B1f3 &&
+		p4 submit -d "BRANCH1/B1f3" &&
+
+		echo b1f4 >branch1/b1f4 &&
+		p4 add branch1/b1f4 &&
+		p4 submit -d "branch1/b1f4"
+	)
+'
+
+# Check that files are properly split across branches when ignorecase is set
+test_expect_failure !CASE_INSENSITIVE_FS 'git p4 clone, branchList branch definition, ignorecase' '
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$git" &&
+		git config git-p4.branchList main:branch1 &&
+		git config --type=bool core.ignoreCase true &&
+		git p4 clone --dest=. --detect-branches //depot@all &&
+
+		git log --all --graph --decorate --stat &&
+
+		git reset --hard p4/master &&
+		test_path_is_file mf1 &&
+		test_path_is_file mf2 &&
+		test_path_is_missing B1f3 &&
+		test_path_is_missing b1f4 &&
+
+		git reset --hard p4/depot/branch1 &&
+		test_path_is_file mf1 &&
+		test_path_is_missing mf2 &&
+		test_path_is_file B1f3 &&
+		test_path_is_file b1f4
+	)
+'
+
+# Check that files are properly split across branches when ignorecase is set, use-client-spec case
+test_expect_failure !CASE_INSENSITIVE_FS 'git p4 clone with client-spec, branchList branch definition, ignorecase' '
+	client_view "//depot/... //client/..." &&
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$git" &&
+		git config git-p4.branchList main:branch1 &&
+		git config --type=bool core.ignoreCase true &&
+		git p4 clone --dest=. --use-client-spec --detect-branches //depot@all &&
+
+		git log --all --graph --decorate --stat &&
+
+		git reset --hard p4/master &&
+		test_path_is_file mf1 &&
+		test_path_is_file mf2 &&
+		test_path_is_missing B1f3 &&
+		test_path_is_missing b1f4 &&
+
+		git reset --hard p4/depot/branch1 &&
+		test_path_is_file mf1 &&
+		test_path_is_missing mf2 &&
+		test_path_is_file B1f3 &&
+		test_path_is_file b1f4
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
 
 test_done
-- 
2.19.2

Re: [PATCH v3 2/8] git-p4: add failing test for "git-p4: match branches case insensitively if configured"

[SZEDER Gábor]

Hi Junio and Andrey,

On Mon, Apr 01, 2019 at 06:02:21PM +0000, Mazo, Andrey wrote:
> diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
> index 6a86d6996b..c48532e12b 100755
> --- a/t/t9801-git-p4-branch.sh
> +++ b/t/t9801-git-p4-branch.sh
> @@ -608,10 +608,102 @@ test_expect_success 'Update a file in git side and submit to P4 using client vie
>  		cd branch1 &&
>  		grep "client spec" file1
>  	)
>  '
>  
> +test_expect_success 'restart p4d (case folding enabled)' '
> +	kill_p4d &&
> +	start_p4d -C1
> +'

There is a semantic conflict between this patch and commit 07353d9042
(git p4 test: clean up the p4d cleanup functions, 2019-03-13) in
'sg/test-atexit' currently cooking in 'pu': this patch adds a new
callsite of 'kill_p4d', but that commit renamed that function to
'stop_and_cleanup_p4d'.  Consequently, t9801 on 'pu' now fails with:

  +kill_p4d
  t9801-git-p4-branch.sh: 4: eval: kill_p4d: not found
  error: last command exited with $?=127
  not ok 28 - restart p4d (case folding enabled)

https://travis-ci.org/git/git/jobs/514513463#L5827

I wonder whether it would be worth amending 07353d9042 to keep
'kill_p4d' around as a wrapper around 'stop_and_cleanup_p4d' for the
time being.


https://public-inbox.org/git/20190313122419.2210-9-szeder.dev@gmail.com/

Re: [PATCH v3 2/8] git-p4: add failing test for "git-p4: match branches case insensitively if configured"

[Mazo, Andrey]

> On Mon, Apr 01, 2019 at 06:02:21PM +0000, Mazo, Andrey wrote:
>> diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
>> index 6a86d6996b..c48532e12b 100755
>> --- a/t/t9801-git-p4-branch.sh
>> +++ b/t/t9801-git-p4-branch.sh
>> @@ -608,10 +608,102 @@ test_expect_success 'Update a file in git side and submit to P4 using client vie
>>  		cd branch1 &&
>>  		grep "client spec" file1
>>  	)
>>  '
>>
>> +test_expect_success 'restart p4d (case folding enabled)' '
>> +	kill_p4d &&
>> +	start_p4d -C1
>> +'
> 
> There is a semantic conflict between this patch and commit 07353d9042
> (git p4 test: clean up the p4d cleanup functions, 2019-03-13) in
> 'sg/test-atexit' currently cooking in 'pu': this patch adds a new
> callsite of 'kill_p4d', but that commit renamed that function to
> 'stop_and_cleanup_p4d'.  Consequently, t9801 on 'pu' now fails with:
> 
>   +kill_p4d
>   t9801-git-p4-branch.sh: 4: eval: kill_p4d: not found
>   error: last command exited with $?=127
>   not ok 28 - restart p4d (case folding enabled)
> 
> https://travis-ci.org/git/git/jobs/514513463#L5827
> 
> I wonder whether it would be worth amending 07353d9042 to keep
> 'kill_p4d' around as a wrapper around 'stop_and_cleanup_p4d' for the
> time being.
> 
> 
> https://public-inbox.org/git/20190313122419.2210-9-szeder.dev@gmail.com/

Good catch!

Don't know what's the proper workflow here, but I see 2 more options:
 * Resolve the conflict in t/t9801-git-p4-branch.sh while merging am/p4-branches-excludes
   commit d15068a650 ("git-p4: respect excluded paths when detecting branches", 2019-04-01)
 * I can rebase my git-p4 changes on top of sg/test-atexit branch
   commit 74ec8cf674 ("t9811-git-p4-label-import: fix pipeline negation", 2019-03-13)

In case this might be helpful,
I did a conflict resolution locally,
(by doing `git checkout d15068a650; git merge 74ec8cf674`)
and here's the patch of the merge.

Basically,
 * a newly added "kill_p4d" is replaced with "stop_and_cleanup_p4d"; and
 * "kill_p4d" in the end of the script is removed.

diff --cc t/t9801-git-p4-branch.sh
index 38d6b9043b,9654362052..67ff2711f5
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@@ -610,4 -650,100 +650,96 @@@ test_expect_success 'Update a file in g
  	)
  '
  
+ test_expect_success 'restart p4d (case folding enabled)' '
 -	kill_p4d &&
++	stop_and_cleanup_p4d &&
+ 	start_p4d -C1
+ '
+ 
+ #
+ # 1: //depot/main/mf1
+ # 2: integrate //depot/main/... -> //depot/branch1/...
+ # 3: //depot/main/mf2
+ # 4: //depot/BRANCH1/B1f3
+ # 5: //depot/branch1/b1f4
+ #
+ test_expect_success !CASE_INSENSITIVE_FS 'basic p4 branches for case folding' '
+ 	(
+ 		cd "$cli" &&
+ 		mkdir -p main &&
+ 
+ 		echo mf1 >main/mf1 &&
+ 		p4 add main/mf1 &&
+ 		p4 submit -d "main/mf1" &&
+ 
+ 		p4 integrate //depot/main/... //depot/branch1/... &&
+ 		p4 submit -d "integrate main to branch1" &&
+ 
+ 		echo mf2 >main/mf2 &&
+ 		p4 add main/mf2 &&
+ 		p4 submit -d "main/mf2" &&
+ 
+ 		mkdir BRANCH1 &&
+ 		echo B1f3 >BRANCH1/B1f3 &&
+ 		p4 add BRANCH1/B1f3 &&
+ 		p4 submit -d "BRANCH1/B1f3" &&
+ 
+ 		echo b1f4 >branch1/b1f4 &&
+ 		p4 add branch1/b1f4 &&
+ 		p4 submit -d "branch1/b1f4"
+ 	)
+ '
+ 
+ # Check that files are properly split across branches when ignorecase is set
+ test_expect_success !CASE_INSENSITIVE_FS 'git p4 clone, branchList branch definition, ignorecase' '
+ 	test_when_finished cleanup_git &&
+ 	test_create_repo "$git" &&
+ 	(
+ 		cd "$git" &&
+ 		git config git-p4.branchList main:branch1 &&
+ 		git config --type=bool core.ignoreCase true &&
+ 		git p4 clone --dest=. --detect-branches //depot@all &&
+ 
+ 		git log --all --graph --decorate --stat &&
+ 
+ 		git reset --hard p4/master &&
+ 		test_path_is_file mf1 &&
+ 		test_path_is_file mf2 &&
+ 		test_path_is_missing B1f3 &&
+ 		test_path_is_missing b1f4 &&
+ 
+ 		git reset --hard p4/depot/branch1 &&
+ 		test_path_is_file mf1 &&
+ 		test_path_is_missing mf2 &&
+ 		test_path_is_file B1f3 &&
+ 		test_path_is_file b1f4
+ 	)
+ '
+ 
+ # Check that files are properly split across branches when ignorecase is set, use-client-spec case
+ test_expect_success !CASE_INSENSITIVE_FS 'git p4 clone with client-spec, branchList branch definition, ignorecase' '
+ 	client_view "//depot/... //client/..." &&
+ 	test_when_finished cleanup_git &&
+ 	test_create_repo "$git" &&
+ 	(
+ 		cd "$git" &&
+ 		git config git-p4.branchList main:branch1 &&
+ 		git config --type=bool core.ignoreCase true &&
+ 		git p4 clone --dest=. --use-client-spec --detect-branches //depot@all &&
+ 
+ 		git log --all --graph --decorate --stat &&
+ 
+ 		git reset --hard p4/master &&
+ 		test_path_is_file mf1 &&
+ 		test_path_is_file mf2 &&
+ 		test_path_is_missing B1f3 &&
+ 		test_path_is_missing b1f4 &&
+ 
+ 		git reset --hard p4/depot/branch1 &&
+ 		test_path_is_file mf1 &&
+ 		test_path_is_missing mf2 &&
+ 		test_path_is_file B1f3 &&
+ 		test_path_is_file b1f4
+ 	)
+ '
+ 
 -test_expect_success 'kill p4d' '
 -	kill_p4d
 -'
 -
  test_done

Re: [PATCH v3 2/8] git-p4: add failing test for "git-p4: match branches case insensitively if configured"

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> I wonder whether it would be worth amending 07353d9042 to keep
> 'kill_p4d' around as a wrapper around 'stop_and_cleanup_p4d' for the
> time being.

I think renaming was the right thing to do; "show --cc" shows that
the post-image lives in the new world order where calling kill_p4d
is not usually needed clearly with a hunk that replaces kill_p4d
(which only existed in the old world) with stop_and_cleanup (which
exists only in the new world).

Will redo the merge and teach my rerere database.

Thanks.

[PATCH v3 3/8] git-p4: match branches case insensitively if configured

[Mazo, Andrey]

git-p4 knows how to handle case insensitivity in file paths
if core.ignorecase is set.
However, when determining a branch for a file,
it still does a case-sensitive prefix match.
This may result in some file changes to be lost on import.

For example, given the following commits
 1. add //depot/main/file1
 2. add //depot/DirA/file2
 3. add //depot/dira/file3
 4. add //depot/DirA/file4
and "branchList = main:DirA" branch mapping,
commit 3 will be lost.

So, do branch search case insensitively if running with core.ignorecase set.
Teach splitFilesIntoBranches() to use the p4PathStartsWith() function
for path prefix matches instead of always case-sensitive match.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 git-p4.py                | 4 ++--
 t/t9801-git-p4-branch.sh | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index c0a3068b6f..f3e5ccb7af 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2666,11 +2666,11 @@ def stripRepoPath(self, path, prefixes):
             # 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 + "/"):
+                    if p4PathStartsWith(path, 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
@@ -2721,11 +2721,11 @@ def splitFilesIntoBranches(self, commit):
                 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, e.g.
-                if relPath.startswith(branch + "/"):
+                if p4PathStartsWith(relPath, branch + "/"):
                     if branch not in branches:
                         branches[branch] = []
                     branches[branch].append(file)
                     break
 
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index c48532e12b..4779448b4c 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -648,11 +648,11 @@ test_expect_success !CASE_INSENSITIVE_FS 'basic p4 branches for case folding' '
 		p4 submit -d "branch1/b1f4"
 	)
 '
 
 # Check that files are properly split across branches when ignorecase is set
-test_expect_failure !CASE_INSENSITIVE_FS 'git p4 clone, branchList branch definition, ignorecase' '
+test_expect_success !CASE_INSENSITIVE_FS 'git p4 clone, branchList branch definition, ignorecase' '
 	test_when_finished cleanup_git &&
 	test_create_repo "$git" &&
 	(
 		cd "$git" &&
 		git config git-p4.branchList main:branch1 &&
@@ -674,11 +674,11 @@ test_expect_failure !CASE_INSENSITIVE_FS 'git p4 clone, branchList branch defini
 		test_path_is_file b1f4
 	)
 '
 
 # Check that files are properly split across branches when ignorecase is set, use-client-spec case
-test_expect_failure !CASE_INSENSITIVE_FS 'git p4 clone with client-spec, branchList branch definition, ignorecase' '
+test_expect_success !CASE_INSENSITIVE_FS 'git p4 clone with client-spec, branchList branch definition, ignorecase' '
 	client_view "//depot/... //client/..." &&
 	test_when_finished cleanup_git &&
 	test_create_repo "$git" &&
 	(
 		cd "$git" &&
-- 
2.19.2

[PATCH v3 4/8] git-p4: don't groom exclude path list on every commit

[Mazo, Andrey]

Currently, `cloneExclude` array is being groomed (by removing trailing "...")
on every changeset.
(since `extractFilesFromCommit()` is called on every imported changeset)

As a micro-optimization, do it once while parsing arguments.
Also, prepend "/" and remove trailing "..." at the same time.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 git-p4.py | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index f3e5ccb7af..7edcbad055 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1314,11 +1314,11 @@ class Command:
     def __init__(self):
         self.usage = "usage: %prog [options]"
         self.needsGit = True
         self.verbose = False
 
-    # This is required for the "append" cloneExclude action
+    # This is required for the "append" update_shelve action
     def ensure_value(self, attr, value):
         if not hasattr(self, attr) or getattr(self, attr) is None:
             setattr(self, attr, value)
         return getattr(self, attr)
 
@@ -2528,10 +2528,15 @@ def map_in_client(self, depot_path):
             return self.client_spec_path_cache[depot_path]
 
         die( "Error: %s is not found in client spec path" % depot_path )
         return ""
 
+def cloneExcludeCallback(option, opt_str, value, parser):
+    # prepend "/" because the first "/" was consumed as part of the option itself.
+    # ("-//depot/A/..." becomes "/depot/A/..." after option parsing)
+    parser.values.cloneExclude += ["/" + re.sub(r"\.\.\.$", "", value)]
+
 class P4Sync(Command, P4UserMap):
 
     def __init__(self):
         Command.__init__(self)
         P4UserMap.__init__(self)
@@ -2551,11 +2556,11 @@ def __init__(self):
                 optparse.make_option("--keep-path", dest="keepRepoPath", action='store_true',
                                      help="Keep entire BRANCH/DIR/SUBDIR prefix during import"),
                 optparse.make_option("--use-client-spec", dest="useClientSpec", action='store_true',
                                      help="Only sync files that are included in the Perforce Client Spec"),
                 optparse.make_option("-/", dest="cloneExclude",
-                                     action="append", type="string",
+                                     action="callback", callback=cloneExcludeCallback, type="string",
                                      help="exclude depot path"),
         ]
         self.description = """Imports from Perforce into a git repository.\n
     example:
     //depot/my/project/ -- to import the current head
@@ -2617,12 +2622,10 @@ def checkpoint(self):
         out = self.gitOutput.readline()
         if self.verbose:
             print("checkpoint finished: " + out)
 
     def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
-        self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
-                             for path in self.cloneExclude]
         files = []
         fnum = 0
         while "depotFile%s" % fnum in commit:
             path =  commit["depotFile%s" % fnum]
 
@@ -3888,11 +3891,10 @@ def run(self, args):
 
         if not self.cloneDestination and len(depotPaths) > 1:
             self.cloneDestination = depotPaths[-1]
             depotPaths = depotPaths[:-1]
 
-        self.cloneExclude = ["/"+p for p in self.cloneExclude]
         for p in depotPaths:
             if not p.startswith("//"):
                 sys.stderr.write('Depot paths must start with "//": %s\n' % p)
                 return False
 
-- 
2.19.2

[PATCH v3 5/8] git-p4: add failing test for "don't exclude other files with same prefix"

[Mazo, Andrey]

In preparation for a fix, add a failing test case to test that
git-p4 doesn't exclude files with the same prefix unintentionally
when exclude paths are specified without a trailing /.
I.e., don't exclude "//depot/file_dont_exclude" if run with "-//depot/file".
or don't exclude "//depot/discard_file_not" if run with "-//depot/discard_file".

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 t/t9817-git-p4-exclude.sh | 51 +++++++++++++++++++++++++++++++++++----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/t/t9817-git-p4-exclude.sh b/t/t9817-git-p4-exclude.sh
index aac568eadf..1c22570797 100755
--- a/t/t9817-git-p4-exclude.sh
+++ b/t/t9817-git-p4-exclude.sh
@@ -20,49 +20,90 @@ test_expect_success 'create exclude repo' '
 	(
 		cd "$cli" &&
 		mkdir -p wanted discard &&
 		echo wanted >wanted/foo &&
 		echo discard >discard/foo &&
-		p4 add wanted/foo discard/foo &&
+		echo discard_file >discard_file &&
+		echo discard_file_not >discard_file_not &&
+		p4 add wanted/foo discard/foo discard_file discard_file_not &&
 		p4 submit -d "initial revision"
 	)
 '
 
 test_expect_success 'check the repo was created correctly' '
 	test_when_finished cleanup_git &&
 	git p4 clone --dest="$git" //depot/...@all &&
 	(
 		cd "$git" &&
 		test_path_is_file wanted/foo &&
-		test_path_is_file discard/foo
+		test_path_is_file discard/foo &&
+		test_path_is_file discard_file &&
+		test_path_is_file discard_file_not
 	)
 '
 
 test_expect_success 'clone, excluding part of repo' '
 	test_when_finished cleanup_git &&
 	git p4 clone -//depot/discard/... --dest="$git" //depot/...@all &&
 	(
 		cd "$git" &&
 		test_path_is_file wanted/foo &&
-		test_path_is_missing discard/foo
+		test_path_is_missing discard/foo &&
+		test_path_is_file discard_file &&
+		test_path_is_file discard_file_not
+	)
+'
+
+test_expect_failure 'clone, excluding single file, no trailing /' '
+	test_when_finished cleanup_git &&
+	git p4 clone -//depot/discard_file --dest="$git" //depot/...@all &&
+	(
+		cd "$git" &&
+		test_path_is_file wanted/foo &&
+		test_path_is_file discard/foo &&
+		test_path_is_missing discard_file &&
+		test_path_is_file discard_file_not
 	)
 '
 
 test_expect_success 'clone, then sync with exclude' '
 	test_when_finished cleanup_git &&
 	git p4 clone -//depot/discard/... --dest="$git" //depot/...@all &&
 	(
 		cd "$cli" &&
-		p4 edit wanted/foo discard/foo &&
+		p4 edit wanted/foo discard/foo discard_file_not &&
 		date >>wanted/foo &&
 		date >>discard/foo &&
+		date >>discard_file_not &&
 		p4 submit -d "updating" &&
 
 		cd "$git" &&
 		git p4 sync -//depot/discard/... &&
 		test_path_is_file wanted/foo &&
-		test_path_is_missing discard/foo
+		test_path_is_missing discard/foo &&
+		test_path_is_file discard_file &&
+		test_path_is_file discard_file_not
+	)
+'
+
+test_expect_failure 'clone, then sync with exclude, no trailing /' '
+	test_when_finished cleanup_git &&
+	git p4 clone -//depot/discard/... -//depot/discard_file --dest="$git" //depot/...@all &&
+	(
+		cd "$cli" &&
+		p4 edit wanted/foo discard/foo discard_file_not &&
+		date >>wanted/foo &&
+		date >>discard/foo &&
+		date >>discard_file_not &&
+		p4 submit -d "updating" &&
+
+		cd "$git" &&
+		git p4 sync -//depot/discard/... -//depot/discard_file &&
+		test_path_is_file wanted/foo &&
+		test_path_is_missing discard/foo &&
+		test_path_is_missing discard_file &&
+		test_path_is_file discard_file_not
 	)
 '
 
 test_expect_success 'kill p4d' '
 	kill_p4d
-- 
2.19.2

[PATCH v3 6/8] git-p4: don't exclude other files with same prefix

[Mazo, Andrey]

Make sure not to exclude files unintentionally
if exclude paths are specified without a trailing /.
I.e., don't exclude "//depot/file_dont_exclude" if run with "-//depot/file".

Do this by ensuring that paths without a trailing "/" are only matched completely.

Also, abort path search on the first match as a micro-optimization.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 git-p4.py                 | 21 ++++++++++++++-------
 t/t9817-git-p4-exclude.sh |  4 ++--
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 7edcbad055..c47bd8c4d8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2621,22 +2621,29 @@ def checkpoint(self):
         self.gitStream.write("progress checkpoint\n\n")
         out = self.gitOutput.readline()
         if self.verbose:
             print("checkpoint finished: " + out)
 
+    def isPathWanted(self, path):
+        for p in self.cloneExclude:
+            if p.endswith("/"):
+                if p4PathStartsWith(path, p):
+                    return False
+            # "-//depot/file1" without a trailing "/" should only exclude "file1", but not "file111" or "file1_dir/file2"
+            elif path.lower() == p.lower():
+                return False
+        for p in self.depotPaths:
+            if p4PathStartsWith(path, p):
+                return True
+        return False
+
     def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
         files = []
         fnum = 0
         while "depotFile%s" % fnum in commit:
             path =  commit["depotFile%s" % fnum]
-
-            if [p for p in self.cloneExclude
-                if p4PathStartsWith(path, p)]:
-                found = False
-            else:
-                found = [p for p in self.depotPaths
-                         if p4PathStartsWith(path, p)]
+            found = self.isPathWanted(path)
             if not found:
                 fnum = fnum + 1
                 continue
 
             file = {}
diff --git a/t/t9817-git-p4-exclude.sh b/t/t9817-git-p4-exclude.sh
index 1c22570797..275dd30425 100755
--- a/t/t9817-git-p4-exclude.sh
+++ b/t/t9817-git-p4-exclude.sh
@@ -51,11 +51,11 @@ test_expect_success 'clone, excluding part of repo' '
 		test_path_is_file discard_file &&
 		test_path_is_file discard_file_not
 	)
 '
 
-test_expect_failure 'clone, excluding single file, no trailing /' '
+test_expect_success 'clone, excluding single file, no trailing /' '
 	test_when_finished cleanup_git &&
 	git p4 clone -//depot/discard_file --dest="$git" //depot/...@all &&
 	(
 		cd "$git" &&
 		test_path_is_file wanted/foo &&
@@ -83,11 +83,11 @@ test_expect_success 'clone, then sync with exclude' '
 		test_path_is_file discard_file &&
 		test_path_is_file discard_file_not
 	)
 '
 
-test_expect_failure 'clone, then sync with exclude, no trailing /' '
+test_expect_success 'clone, then sync with exclude, no trailing /' '
 	test_when_finished cleanup_git &&
 	git p4 clone -//depot/discard/... -//depot/discard_file --dest="$git" //depot/...@all &&
 	(
 		cd "$cli" &&
 		p4 edit wanted/foo discard/foo discard_file_not &&
-- 
2.19.2

[PATCH v3 7/8] git-p4: add failing test for "git-p4: respect excluded paths when detecting branches"

[Mazo, Andrey]

In preparation for a fix, add a failing test case to test that
git-p4 doesn't exclude files despite being told to
when handling multiple branches.

I.e., it should exclude //depot/branch2/file2 when run with -//depot/branch2/file2,
but doesn't do this right now.

The test is based on 'git p4 clone complex branches' test with the following changes:
 * account for file3 moved from branch3 to branch4 in test 'git p4 submit to two branches in a single changelist';
 * account for branch6 created in test 'git p4 clone file subset branch';
 * file2 is expected to be missing from all branches due to explicit exclude.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 t/t9801-git-p4-branch.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 4779448b4c..7530d22de2 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -409,10 +409,50 @@ test_expect_failure 'git p4 clone file subset branch' '
 		test_path_is_missing file2 &&
 		test_path_is_missing file3
 	)
 '
 
+# Check that excluded files are omitted during import
+test_expect_failure 'git p4 clone complex branches with excluded files' '
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$git" &&
+		git config git-p4.branchList branch1:branch2 &&
+		git config --add git-p4.branchList branch1:branch3 &&
+		git config --add git-p4.branchList branch1:branch4 &&
+		git config --add git-p4.branchList branch1:branch5 &&
+		git config --add git-p4.branchList branch1:branch6 &&
+		git p4 clone --dest=. --detect-branches -//depot/branch1/file2 -//depot/branch2/file2 -//depot/branch3/file2 -//depot/branch4/file2 -//depot/branch5/file2 -//depot/branch6/file2 //depot@all &&
+		git log --all --graph --decorate --stat &&
+		git reset --hard p4/depot/branch1 &&
+		test_path_is_file file1 &&
+		test_path_is_missing file2 &&
+		test_path_is_file file3 &&
+		git reset --hard p4/depot/branch2 &&
+		test_path_is_file file1 &&
+		test_path_is_missing file2 &&
+		test_path_is_missing file3 &&
+		git reset --hard p4/depot/branch3 &&
+		test_path_is_file file1 &&
+		test_path_is_missing file2 &&
+		test_path_is_missing file3 &&
+		git reset --hard p4/depot/branch4 &&
+		test_path_is_file file1 &&
+		test_path_is_missing file2 &&
+		test_path_is_file file3 &&
+		git reset --hard p4/depot/branch5 &&
+		test_path_is_file file1 &&
+		test_path_is_missing file2 &&
+		test_path_is_file file3 &&
+		git reset --hard p4/depot/branch6 &&
+		test_path_is_file file1 &&
+		test_path_is_missing file2 &&
+		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" &&
-- 
2.19.2

[PATCH v3 8/8] git-p4: respect excluded paths when detecting branches

[Mazo, Andrey]

Currently, excluded paths are only handled in the following cases:
 * no branch detection;
 * branch detection with using clientspec.

However, excluded paths are not respected in case of
branch detection without using clientspec.

Fix this by consulting the list of excluded paths
when splitting files across branches.

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 git-p4.py                | 3 +--
 t/t9801-git-p4-branch.sh | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index c47bd8c4d8..96c4b78dc7 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2708,12 +2708,11 @@ def splitFilesIntoBranches(self, commit):
 
         branches = {}
         fnum = 0
         while "depotFile%s" % fnum in commit:
             path =  commit["depotFile%s" % fnum]
-            found = [p for p in self.depotPaths
-                     if p4PathStartsWith(path, p)]
+            found = self.isPathWanted(path)
             if not found:
                 fnum = fnum + 1
                 continue
 
             file = {}
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 7530d22de2..9654362052 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -410,11 +410,11 @@ test_expect_failure 'git p4 clone file subset branch' '
 		test_path_is_missing file3
 	)
 '
 
 # Check that excluded files are omitted during import
-test_expect_failure 'git p4 clone complex branches with excluded files' '
+test_expect_success 'git p4 clone complex branches with excluded files' '
 	test_when_finished cleanup_git &&
 	test_create_repo "$git" &&
 	(
 		cd "$git" &&
 		git config git-p4.branchList branch1:branch2 &&
-- 
2.19.2

Re: [PATCH v3 0/8] git-p4: a few assorted fixes for branches, excludes

[Mazo, Andrey]

> This series fixes a few cases with branch detection
> and handling of excludes by git-p4.
> 
> This is the third iteration of the patch series.
> Changes since v2 [2]:
>  * Added new test cases for case-insensitive branch detection.

Forgot to add:
 * Added a fix for case-insensitive branch detection when running with useClientSpec enabled.
   (range diff below correctly shows the e644a8ab49 vs 6eaad2582c difference)

> Changes since v1 [1]:
>  * Added new test case for excluded paths when detecting branches;
>  * Added a new fix for excluded paths when detecting branches.
> 
> [1] https://public-inbox.org/git/cover.1551485349.git.amazo@checkvideo.com
> [2] https://public-inbox.org/git/cover.1553207234.git.amazo@checkvideo.com/
> 
> Range-diff vs v2:
> 1:  3ac39171d4 = 1:  bd009a5ca5 git-p4: detect/prevent infinite loop in gitCommitByP4Change()
> 2:  e644a8ab49 < -:  ---------- git-p4: match branches case insensitively if configured
> -:  ---------- > 2:  68b68ce1e4 git-p4: add failing test for "git-p4: match branches case insensitively if configured"
> -:  ---------- > 3:  6eaad2582c git-p4: match branches case insensitively if configured
> 3:  44fed954dc = 4:  1bd5e170e0 git-p4: don't groom exclude path list on every commit
> 4:  a0d3fa6add = 5:  b657967154 git-p4: add failing test for "don't exclude other files with same prefix"
> 5:  3330f88a0d = 6:  035abfff2a git-p4: don't exclude other files with same prefix
> 6:  6170d45951 = 7:  2bde24b7e4 git-p4: add failing test for "git-p4: respect excluded paths when detecting branches"
> 7:  758d8e8486 = 8:  6d3ffb98a7 git-p4: respect excluded paths when detecting branches

--
Andrey

[RFC PATCH v2 0/2] git-p4: inexact labels and load changelist description from file

[Mazo, Andrey]

This patch series introduces two experimental features to git-p4,
unrelated to each other.
 1. The first patch adds support for "inexact" label detection.
    The feature lets git-p4 find a git commit for a Perforce label
    even if there is no git commit with exact same changelist number as in Perforce.
    It is particularly useful when splitting a large Perforce depot
    into multiple git repositories
    or when importing just a subset of a depot into git.
 2. The second patch adds support for loading changelist description from a file.
    (`p4 -G describe` equivalent)
    The original use case is to be able to migrate a Perforce depot,
    which database got a little bit corrupted, into git.

This patch series should be applied on top of
"[PATCH v3 0/8] git-p4: a few assorted fixes for branches, excludes" [1]

This is the second iteration of the patch series.
Changes since the v1 [1]:
 * Dropped "alien" branch feature;
 * Added "inexact" label feature by suggestion from Luke;
 * Added minimal documentation;
 * Changed "damaged"-oriented narrative to more generic one.
   (renamed "git-p4.damagedChangelists" to "git-p4.changelistDescriptionFile",
    functions and variables correspondingly)

Range-diff vs v1:
1:  b02df749b9 < -:  ---------- git-p4: introduce alien branch mappings
-:  ---------- > 1:  54ef897fcf git-p4: inexact label detection
2:  bb3e14a389 ! 2:  83b0034538 git-p4: support loading changelist descriptions from files

[1] https://public-inbox.org/git/cover.1554141338.git.amazo@checkvideo.com/

Andrey Mazo (2):
  git-p4: inexact label detection
  git-p4: support loading changelist descriptions from files

 Documentation/git-p4.txt | 41 +++++++++++++++++++++++
 git-p4.py                | 72 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 106 insertions(+), 7 deletions(-)


base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
prerequisite-patch-id: 23e039fec7a1f5c51c98326a14d788adb1ecb5ba
prerequisite-patch-id: 9840851ffca6f00126c9c91da5a8828c7d0dcaed
prerequisite-patch-id: 32a738b41fb3dccfbbfb4d382a9748e36dcdfa8b
prerequisite-patch-id: 10661f77392f4131d2375976c77a7cd231fdf9ab
prerequisite-patch-id: a55360c904eba1b9e9c934405d3141eb96c5ad30
prerequisite-patch-id: 46357586199c02d956d53d782a12f1ee0c991302
prerequisite-patch-id: c683e7d6017580df9385a1544af409ca615d770c
prerequisite-patch-id: 411dcb5e95aff036e0cb3e850ea75f2424b260a6
-- 
2.19.2

[RFC PATCH v2 1/2] git-p4: inexact label detection

[Mazo, Andrey]

Labels in Perforce are not global, but can be placed on a particular view/subdirectory.
This might pose difficulties when importing only parts of Perforce depot into a git repository.
For example:
 1. Depot layout is as follows:
    //depot/metaproject/branch1/subprojectA/...
    //depot/metaproject/branch1/subprojectB/...
    //depot/metaproject/branch2/subprojectA/...
    //depot/metaproject/branch2/subprojectB/...
 2. Labels are placed as follows:
    * label 1A on //depot/metaproject/branch1/subprojectA/...
    * label 1B on //depot/metaproject/branch1/subprojectB/...
    * label 2A on //depot/metaproject/branch2/subprojectA/...
    * label 2B on //depot/metaproject/branch2/subprojectB/...
 3. The goal is to import
    subprojectA into subprojectA.git and
    subprojectB into subprojectB.git
    preserving all the branches and labels.
 4. Importing subprojectA.
    Label 1A is imported fine because it's placed on certain commit on branch1.
    However, label 1B is not imported because it's placed on a commit in another subproject:
    git-p4 says: "importing label 1B: could not find git commit for changelist ..."
    The same is with label 2A, which is imported; and 2B, which is not.

Currently, there is no easy way (that I'm aware of) to tell git-p4 to
import an empty commit into a desired branch,
so that a label placed on that changelist could be imported as well,
It might be possible to get a similar effect by importing both subprojectA and B in a single git repo,
and then running `git filter-branch --subdirectory-filter subprojectA`,
but this might produce way more irrelevant empty commits, than needed for labels.
(although imported changelists can be limited with git-p4 --changesfile option)
Also, `git filter-branch` is harder to use for incremental imports
or when changes are submitted from git back to Perforce.

As suggested by Luke,
instead of creating an empty commit for the sole purpose of being tagged later,
teach git-p4 to search harder for the next lower changelist,
corresponding to the label in question.

Do this by finding the highest changelist up to the label under all known branches,
(branches are finalized by the time importP4Labels() runs)
and using it instead of a depot-wide changelist corresponding to the label.

This new behavior may not be desired for people,
who want exact label <-> changelist relationship.
So, add a new boolean config parameter git-p4.allowInexactLabels (defaults to false)
to explicitly enable it if needed.
Also, this behavior only appears to be useful in case of multiple branches,
(otherwise, every Perforce changelist should appear in git)
so it's not engaged when running without branch detection.

Detect and report (--verbose) "inexact" tags,
i.e. tags placed on a lower changelist than was in Perforce.
Implement this by comparing a changelist for which a commit was found
with a changelist corresponding to the label on the whole depot.

Note, that the new "inexact" logic works slower
than the original code in case of numerous branches,
because p4 needs to calculate the most recent change for each branch path instead of just one.

This is an alternative solution to "alien" branches concept proposed earlier:
https://public-inbox.org/git/b02df749b9266ac8c73707617a171122156621ab.1553283214.git.amazo@checkvideo.com/

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
Suggested-by: Luke Diamand <luke@diamand.org>
---
 Documentation/git-p4.txt | 14 ++++++++++++
 git-p4.py                | 48 +++++++++++++++++++++++++++++++++++-----
 2 files changed, 56 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 3494a1db3e..ceabab8b86 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -582,10 +582,24 @@ git-p4.importLabels::
 
 git-p4.labelImportRegexp::
 	Only p4 labels matching this regular expression will be imported. The
 	default value is '[a-zA-Z0-9_\-.]+$'.
 
+git-p4.allowInexactLabels::
+	Only has an effect if run with `--detect-branches`.
+	By default, when performing p4 label import,
+	'git p4' finds a changelist number of every label,
+	then finds a git commit corresponding to the found changelist number,
+	and then places an annotated tag on the found git commit.
+	If a git commit is not found, the label is considered unimportable
+	and is added to 'ignoredP4Labels' list.
+	If 'allowInexactLabels' is set to true,
+	'git p4' only considers changelists under branches being imported.
+	This has an effect that a tag in git might be placed on a lower changelist compared to p4.
+	This might be useful when importing just a subset of the depot into git,
+	if a label would be discarded otherwise.
+
 git-p4.useClientSpec::
 	Specify that the p4 client spec should be used to identify p4
 	depot paths of interest.  This is equivalent to specifying the
 	option `--use-client-spec`.  See the "CLIENT SPEC" section above.
 	This variable is a boolean, not the name of a p4 client.
diff --git a/git-p4.py b/git-p4.py
index 96c4b78dc7..98b2b7bbca 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3162,17 +3162,43 @@ def importP4Labels(self, stream, p4Labels):
             if name in ignoredP4Labels:
                 continue
 
             labelDetails = p4CmdList(['label', "-o", name])[0]
 
-            # get the most recent changelist for each file in this label
-            change = p4Cmd(["changes", "-m", "1"] + ["%s...@%s" % (p, name)
+            if self.detectBranches and gitConfigBool("git-p4.allowInexactLabels"):
+                doInexactLabels = True
+            else:
+                doInexactLabels = False
+
+            # get the most recent changelist in this label for the whole depot
+            depot_wide_changelist = p4Cmd(["changes", "-m", "1"] + ["%s...@%s" % (p, name)
                                 for p in self.depotPaths])
+            if 'change' in depot_wide_changelist:
+                depot_wide_changelist = int(depot_wide_changelist['change'])
+            else:
+                depot_wide_changelist = None
 
-            if 'change' in change:
+            # get the most recent changelist for each file under branches of interest in this label
+            if doInexactLabels:
+                if self.useClientSpec:
+                    paths = ["%s...@%s" % (self.clientSpecDirs.client_prefix + p + '/', name) for p in self.knownBranches]
+                else:
+                    paths = ["%s...@%s" % (self.depotPaths[0] + p + '/', name) for p in self.knownBranches]
+                changes = p4CmdList(["changes", "-m", "1"] + paths)
+                changes = [int(c['change']) for c in changes if 'change' in c]
+
+                # there may be different "most recent" changelists for different paths.
+                # take the newest since some paths were just modified later than others.
+                if changes:
+                    changelist = max(changes)
+                else:
+                    changelist = None
+            else:
+                changelist = depot_wide_changelist
+
+            if changelist:
                 # find the corresponding git commit; take the oldest commit
-                changelist = int(change['change'])
                 if changelist in self.committedChanges:
                     gitCommit = ":%d" % changelist       # use a fast-import mark
                     commitFound = True
                 else:
                     gitCommit = read_pipe(["git", "rev-list", "--max-count=1",
@@ -3192,14 +3218,24 @@ def importP4Labels(self, stream, p4Labels):
                         tmwhen = 1
 
                     when = int(time.mktime(tmwhen))
                     self.streamTag(stream, name, labelDetails, gitCommit, when)
                     if verbose:
-                        print("p4 label %s mapped to git commit %s" % (name, gitCommit))
+                        if depot_wide_changelist == changelist:
+                            isExact = ""
+                        else:
+                            isExact = " inexactly"
+                        print("p4 label %s mapped%s to git commit %s" % (name, isExact, gitCommit))
             else:
                 if verbose:
-                    print("Label %s has no changelists - possibly deleted?" % name)
+                    if depot_wide_changelist:
+                        # there is a changelist corresponding to this label,
+                        # but it's not under any branches of interest.
+                        print("Label %s has no changelists under detected branches -- ignoring" % name)
+                    else:
+                        # there is no changelist corresponding to this label in the whole depot
+                        print("Label %s has no changelists - possibly deleted?" % name)
 
             if not commitFound:
                 # We can't import this label; don't try again as it will get very
                 # expensive repeatedly fetching all the files for labels that will
                 # never be imported. If the label is moved in the future, the
-- 
2.19.2

[RFC PATCH v2 2/2] git-p4: support loading changelist descriptions from files

[Mazo, Andrey]

Our Perforce server experienced some kind of database corruption a few years ago.
While the file data and revision history are mostly intact,
some metadata for several changesets got lost.
For example, inspecting certain changelists produces errors.
"""
$ p4 describe -s 12345
Date 2019/02/26 16:46:17:
Operation: user-describe
Operation 'user-describe' failed.
Change 12345 description missing!
"""

While some metadata (like changeset descriptions) is obviously lost,
most of it can be reconstructed via other commands:
 * `p4 changes -l -t //...@12345,12345` --
   to obtain date+time, author, beginning of changeset description;
 * `p4 files -a //...@12345,12345` --
   to obtain file revisions, file types, file actions;
 * `p4 diff2 -u //...@12344 //...@12345` --
   to get a unified diff of text files in a changeset;
 * `p4 print -o binary.blob@12345 //depot/binary.blob@12345` --
   to get a revision of a binary file.

It might be possible to teach git-p4 to fallback to other methods if `p4 describe` fails,
but it's probably too special-cased (really depends on kind and scale of DB corruption),
so some manual intervention is perhaps acceptable.

So, with some manual work, it's possible to reconstruct `p4 -G describe ...` output manually.
In our case, once git-p4 passes `p4 describe` stage,
it can proceed further just fine.
Thus, it's tempting to feed resurrected metadata to git-p4 when a normal `p4 describe` would fail.

This functionality may be useful to cache changelist information,
or to make some changes to changelist info before feeding it to git-p4.

A new config parameter is introduced to tell git-p4
to load certain changelist descriptions from files instead of from a server.
For simplicity, it's one marshalled file per changelist.
```
git config --add git-p4.changelistDescriptionFile 12345.marshal
git config --add git-p4.changelistDescriptionFile 12346.marshal
```

The following trivial script may be used to produce marshalled `p4 -G describe`-compatible output.
"""
 #!/usr/bin/env python

 import marshal
 import time

 # recovered commits of interest
 changes = [
     {
         'change':     '12345',
         'status':     'submitted',
         'code':       'stat',
         'user':       'username1',
         'time':       str(int(time.mktime(time.strptime('2019/02/28 16:00:30', '%Y/%m/%d %H:%M:%S')))),
         'client':     'username1_hostname1',
         'desc':       'A bug is fixed.\nDetails are below:<lost>\n',
         'depotFile0': '//depot/branch1/foo.sh',
         'action0':    'edit',
         'rev0':       '28',
         'type0':      'xtext',
         'depotFile1': '//depot/branch1/bar.py',
         'action1':    'edit',
         'rev1':       '43',
         'type1':      'text',
         'depotFile2': '//depot/branch1/baz.doc',
         'action2':    'edit',
         'rev2':       '8',
         'type2':      'binary',
         'depotFile3': '//depot/branch1/qqq.c',
         'action3':    'edit',
         'rev3':       '6',
         'type3':      'ktext',
     },
 ]

 for change in changes:
     marshal.dump(change, open('{0}.marshal'.format(change['change']), 'wb'))
"""

Or, the following script may be used to produce marshalled `p4 -G describe`-compatible output
for our particular database corruption.
"""
 #!/usr/bin/env python

 import itertools
 import marshal
 import subprocess
 import tempfile

 def p4_unmarshal(fileobj):
     result = []
     while True:
         try:
             result += [marshal.load(fileobj)]
         except EOFError:
             break

     return result

 def p4_describe_fallback(cl):
     with tempfile.TemporaryFile() as p4_changes_output:
         with tempfile.TemporaryFile() as p4_files_output:
             subprocess.check_call(['p4', '-G', 'changes', '-l', '-t', '//...@{0},{0}'.format(cl)], stdout=p4_changes_output)
             subprocess.check_call(['p4', '-G', 'files', '-a', '//...@{0},{0}'.format(cl)], stdout=p4_files_output)

             p4_changes_output.seek(0)
             p4_files_output.seek(0)

             p4_changes_unmarshalled = p4_unmarshal(p4_changes_output)
             p4_files_unmarshalled = p4_unmarshal(p4_files_output)

             described_cl = p4_changes_unmarshalled[0] # there is usually only one entry
             described_cl['desc'] += '<lost>\n'

             assert described_cl['change'] == str(cl)

             for (file_info, i) in itertools.izip(p4_files_unmarshalled, itertools.count()):
                 for f in ('depotFile', 'action', 'rev', 'type'):
                     described_cl['{}{}'.format(f, i)] = file_info[f]

                 assert file_info['change'] == described_cl['change']

             return described_cl

 cls_wanted = ( 12345, 12346 )

 for cl in cls_wanted:
     with open('{0}.marshal'.format(cl), 'wb') as f:
         cl_info = p4_describe_fallback(cl)
         marshal.dump(cl_info, f)
"""

Signed-off-by: Andrey Mazo <amazo@checkvideo.com>
---
 Documentation/git-p4.txt | 27 +++++++++++++++++++++++++++
 git-p4.py                | 24 +++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index ceabab8b86..f751ae729f 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -654,10 +654,37 @@ git config --add git-p4.mapUser "p4user = First Last <mail@address.com>"
 -------------
 +
 A mapping will override any user information from P4. Mappings for
 multiple P4 user can be defined.
 
+git-p4.changelistDescriptionFile::
+	This config variable points 'git p4' to a file,
+	containing a serialized (marshalled) changelist description.
+	'git p4' loads a description from such a file
+	instead of asking Perforce server about it.
+	The file format is the same as produced by 'p4 -G describe -s <changelist>'.
+	This option can be specified multiple times
+	to feed multiple changelist descriptions to 'git p4'.
+	The path is relative to git work tree,
+	file names or extensions don't matter.
+	This example loads 2 changelist descriptions:
++
+-------------
+git config --add git-p4.changelistDescriptionFile cl-12345.marshal
+git config --add git-p4.changelistDescriptionFile cl-12347.marshal
+-------------
++
+Under some circumstances (for example, Perforce database corruption)
+this option is useful to supply changelist description to 'git p4' bypassing 'p4'.
+Also, it can be used for caching of changelist descriptions
+to reduce load on the Perforce server in case of successive imports
+(say, when splitting the depot into multiple Git repositories)
+or for overriding some changelist information.
+This config variable is generally not needed after the initial import,
+so it can be removed from the config file
+together with corresponding description files after the import.
+
 Submit variables
 ~~~~~~~~~~~~~~~~
 git-p4.detectRenames::
 	Detect renames.  See linkgit:git-diff[1].  This can be true,
 	false, or a score as expected by 'git diff -M'.
diff --git a/git-p4.py b/git-p4.py
index 98b2b7bbca..e65df92d75 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2613,10 +2613,12 @@ def __init__(self):
         self.initialParents = {}
 
         self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 3600) / 60))
         self.labels = {}
 
+        self.loadedChangelistDescriptions = {}
+
     # Force a checkpoint in fast-import and wait for it to finish
     def checkpoint(self):
         self.gitStream.write("checkpoint\n\n")
         self.gitStream.write("progress checkpoint\n\n")
         out = self.gitOutput.readline()
@@ -3319,10 +3321,25 @@ def getBranchMappingFromGitBranches(self):
                 branch = "main"
             else:
                 branch = branch[len(self.projectName):]
             self.knownBranches[branch] = branch
 
+    def loadChangelistDescFromFile(self):
+        changelistDescriptionFiles = gitConfigList("git-p4.changelistDescriptionFile")
+        for clMarshalledDescFile in changelistDescriptionFiles:
+            if not clMarshalledDescFile:
+                continue
+
+            try:
+                with open(clMarshalledDescFile, 'rb') as clFileObj:
+                    clDesc = marshal.load(clFileObj)
+                if not ("status" in clDesc and "user" in clDesc and "time" in clDesc and "change" in clDesc):
+                    die("Changelist description read from {0} doesn't have required fields".format(clMarshalledDescFile))
+                self.loadedChangelistDescriptions[int(clDesc["change"])] = clDesc
+            except (IOError, TypeError, ValueError, EOFError) as e:
+                die("Can't read changelist description from {0}: {1}".format(clMarshalledDescFile, str(e)))
+
     def updateOptionDict(self, d):
         option_keys = {}
         if self.keepRepoPath:
             option_keys['keepRepoPath'] = 1
 
@@ -3420,11 +3437,14 @@ def searchParent(self, parent, branch, target):
             return None
 
     def importChanges(self, changes, origin_revision=0):
         cnt = 1
         for change in changes:
-            description = p4_describe(change)
+            if change in self.loadedChangelistDescriptions:
+                description = self.loadedChangelistDescriptions[change]
+            else:
+                description = p4_describe(change)
             self.updateOptionDict(description)
 
             if not self.silent:
                 sys.stdout.write("\rImporting revision %s (%s%%)" % (change, cnt * 100 / len(changes)))
                 sys.stdout.flush()
@@ -3706,10 +3726,12 @@ def run(self, args):
                     bad_changesfile = True
                     break
         if bad_changesfile:
             die("Option --changesfile is incompatible with revision specifiers")
 
+        self.loadChangelistDescFromFile()
+
         newPaths = []
         for p in self.depotPaths:
             if p.find("@") != -1:
                 atIdx = p.index("@")
                 self.changeRange = p[atIdx:]
-- 
2.19.2