[PATCH 1/1] git-p4: recover from inconsistent perforce history

[PATCH 1/1] git-p4: recover from inconsistent perforce history

[andrew]

From: Andrew Oakley <andrew@adoakley.name>

Perforce allows you commit files and directories with the same name, so
you could have files //depot/foo and //depot/foo/bar both checked in.  A
p4 sync of a repository in this state fails.  Deleting one of the files
recovers the repository.

When this happens we want git-p4 to recover in the same way as perforce.

Signed-off-by: Andrew Oakley <andrew@adoakley.name>
---
 git-p4.py                      | 41 ++++++++++++++++++++++--
 t/t9834-git-p4-file-dir-bug.sh | 58 ++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 2 deletions(-)
 create mode 100755 t/t9834-git-p4-file-dir-bug.sh

diff --git a/git-p4.py b/git-p4.py
index 3e12774f96..6bf2bbbcec 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3012,6 +3012,43 @@ def hasBranchPrefix(self, path):
             print('Ignoring file outside of prefix: {0}'.format(path))
         return hasPrefix
 
+    def isIncluded(self, path):
+        return self.inClientSpec(path) and self.hasBranchPrefix(path)
+
+    def findShadowedFiles(self, files, change):
+        # Perforce allows you commit files and directories with the same name,
+        # so you could have files //depot/foo and //depot/foo/bar both checked
+        # in.  A p4 sync of a repository in this state fails.  Deleting one of
+        # the files recovers the repository.
+        #
+        # Git will not allow the broken state to exist and only the most recent
+        # of the conflicting names is left in the repository.  When one of the
+        # conflicting files is deleted we need to re-add the other one to make
+        # sure the git repository recovers in the same way as perforce.
+        deleted = [f for f in files if f['action'] in self.delete_actions]
+        to_check = set()
+        for f in deleted:
+            path = f['path']
+            to_check.add(path + '/...')
+            while True:
+                path = path.rsplit("/", 1)[0]
+                if path == "/" or path in to_check:
+                    break
+                to_check.add(path)
+        to_check = [p for p in to_check if self.isIncluded(p)]
+        if to_check:
+            stat_result = p4CmdList(
+                ["fstat", "-T", "depotFile,headRev,headType"] +
+                    ["%s@%s" % (p, change) for p in to_check])
+            for record in stat_result:
+                if record['code'] != 'stat':
+                    continue
+                files.append({
+                    'action': 'add',
+                    'path': record['depotFile'],
+                    'rev': record['headRev'],
+                    'type': record['headType']})
+
     def commit(self, details, files, branch, parent = "", allow_empty=False):
         epoch = details["time"]
         author = details["user"]
@@ -3023,8 +3060,8 @@ def commit(self, details, files, branch, parent = "", allow_empty=False):
         if self.clientSpecDirs:
             self.clientSpecDirs.update_client_spec_path_cache(files)
 
-        files = [f for f in files
-            if self.inClientSpec(f['path']) and self.hasBranchPrefix(f['path'])]
+        files = [f for f in files if self.isIncluded(f['path'])]
+        self.findShadowedFiles(files, details["change"])
 
         if gitConfigBool('git-p4.keepEmptyCommits'):
             allow_empty = True
diff --git a/t/t9834-git-p4-file-dir-bug.sh b/t/t9834-git-p4-file-dir-bug.sh
new file mode 100755
index 0000000000..9839a3d2bb
--- /dev/null
+++ b/t/t9834-git-p4-file-dir-bug.sh
@@ -0,0 +1,58 @@
+#!/bin/sh
+
+test_description='git p4 directory/file bug handling
+
+This test creates files and directories with the same name in perforce and
+checks that git-p4 recovers from the error at the same time as the perforce
+repository.'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'init depot' '
+	(
+		cd "$cli" &&
+
+		touch add_file_add_dir_del_file add_file_add_dir_del_dir &&
+		p4 add add_file_add_dir_del_file add_file_add_dir_del_dir &&
+		mkdir add_dir_add_file_del_file add_dir_add_file_del_dir &&
+		touch add_dir_add_file_del_file/file add_dir_add_file_del_dir/file &&
+		p4 add add_dir_add_file_del_file/file add_dir_add_file_del_dir/file &&
+		p4 submit -d "add initial" &&
+
+		rm -f add_file_add_dir_del_file add_file_add_dir_del_dir &&
+		mkdir add_file_add_dir_del_file add_file_add_dir_del_dir &&
+		touch add_file_add_dir_del_file/file add_file_add_dir_del_dir/file &&
+		p4 add add_file_add_dir_del_file/file add_file_add_dir_del_dir/file &&
+		rm -rf add_dir_add_file_del_file add_dir_add_file_del_dir &&
+		touch add_dir_add_file_del_file add_dir_add_file_del_dir &&
+		p4 add add_dir_add_file_del_file add_dir_add_file_del_dir &&
+		p4 submit -d "add conflicting" &&
+
+		p4 delete -k add_file_add_dir_del_file &&
+		p4 delete -k add_file_add_dir_del_dir/file &&
+		p4 delete -k add_dir_add_file_del_file &&
+		p4 delete -k add_dir_add_file_del_dir/file &&
+		p4 submit -d "delete conflicting"
+	)
+'
+
+test_expect_success 'clone with git-p4' '
+	git p4 clone --dest="$git" //depot/@all
+'
+
+test_expect_success 'check final contents' '
+	test_path_is_dir "$git/add_file_add_dir_del_file" &&
+	test_path_is_file "$git/add_file_add_dir_del_dir" &&
+	test_path_is_dir "$git/add_dir_add_file_del_file" &&
+	test_path_is_file "$git/add_dir_add_file_del_dir"
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
2.19.2

Re: [PATCH 1/1] git-p4: recover from inconsistent perforce history

[Luke Diamand]

On Wed,  6 Feb 2019 19:42:19 +0000
andrew@adoakley.name wrote:

> From: Andrew Oakley <andrew@adoakley.name>
> 
> Perforce allows you commit files and directories with the same name, so
> you could have files //depot/foo and //depot/foo/bar both checked in.  A
> p4 sync of a repository in this state fails.  Deleting one of the files
> recovers the repository.
> 
> When this happens we want git-p4 to recover in the same way as perforce.

I'm finding the test fails for me on a clean git repo, although I can't see any obvious reason why.

Having the ability to detect when Perforce users submit a change which creates a file-inside-a-file will be really very useful.

Luke


> 
> Signed-off-by: Andrew Oakley <andrew@adoakley.name>
> ---
>  git-p4.py                      | 41 ++++++++++++++++++++++--
>  t/t9834-git-p4-file-dir-bug.sh | 58 ++++++++++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+), 2 deletions(-)
>  create mode 100755 t/t9834-git-p4-file-dir-bug.sh
> 
> diff --git a/git-p4.py b/git-p4.py
> index 3e12774f96..6bf2bbbcec 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -3012,6 +3012,43 @@ def hasBranchPrefix(self, path):
>              print('Ignoring file outside of prefix: {0}'.format(path))
>          return hasPrefix
>  
> +    def isIncluded(self, path):
> +        return self.inClientSpec(path) and self.hasBranchPrefix(path)
> +
> +    def findShadowedFiles(self, files, change):
> +        # Perforce allows you commit files and directories with the same name,
> +        # so you could have files //depot/foo and //depot/foo/bar both checked
> +        # in.  A p4 sync of a repository in this state fails.  Deleting one of
> +        # the files recovers the repository.
> +        #
> +        # Git will not allow the broken state to exist and only the most recent
> +        # of the conflicting names is left in the repository.  When one of the
> +        # conflicting files is deleted we need to re-add the other one to make
> +        # sure the git repository recovers in the same way as perforce.
> +        deleted = [f for f in files if f['action'] in self.delete_actions]
> +        to_check = set()
> +        for f in deleted:
> +            path = f['path']
> +            to_check.add(path + '/...')
> +            while True:
> +                path = path.rsplit("/", 1)[0]
> +                if path == "/" or path in to_check:
> +                    break
> +                to_check.add(path)
> +        to_check = [p for p in to_check if self.isIncluded(p)]
> +        if to_check:
> +            stat_result = p4CmdList(
> +                ["fstat", "-T", "depotFile,headRev,headType"] +
> +                    ["%s@%s" % (p, change) for p in to_check])
> +            for record in stat_result:
> +                if record['code'] != 'stat':
> +                    continue
> +                files.append({
> +                    'action': 'add',
> +                    'path': record['depotFile'],
> +                    'rev': record['headRev'],
> +                    'type': record['headType']})
> +
>      def commit(self, details, files, branch, parent = "", allow_empty=False):
>          epoch = details["time"]
>          author = details["user"]
> @@ -3023,8 +3060,8 @@ def commit(self, details, files, branch, parent = "", allow_empty=False):
>          if self.clientSpecDirs:
>              self.clientSpecDirs.update_client_spec_path_cache(files)
>  
> -        files = [f for f in files
> -            if self.inClientSpec(f['path']) and self.hasBranchPrefix(f['path'])]
> +        files = [f for f in files if self.isIncluded(f['path'])]
> +        self.findShadowedFiles(files, details["change"])
>  
>          if gitConfigBool('git-p4.keepEmptyCommits'):
>              allow_empty = True
> diff --git a/t/t9834-git-p4-file-dir-bug.sh b/t/t9834-git-p4-file-dir-bug.sh
> new file mode 100755
> index 0000000000..9839a3d2bb
> --- /dev/null
> +++ b/t/t9834-git-p4-file-dir-bug.sh
> @@ -0,0 +1,58 @@
> +#!/bin/sh
> +
> +test_description='git p4 directory/file bug handling
> +
> +This test creates files and directories with the same name in perforce and
> +checks that git-p4 recovers from the error at the same time as the perforce
> +repository.'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> +	start_p4d
> +'
> +
> +test_expect_success 'init depot' '
> +	(
> +		cd "$cli" &&
> +
> +		touch add_file_add_dir_del_file add_file_add_dir_del_dir &&
> +		p4 add add_file_add_dir_del_file add_file_add_dir_del_dir &&
> +		mkdir add_dir_add_file_del_file add_dir_add_file_del_dir &&
> +		touch add_dir_add_file_del_file/file add_dir_add_file_del_dir/file &&
> +		p4 add add_dir_add_file_del_file/file add_dir_add_file_del_dir/file &&
> +		p4 submit -d "add initial" &&
> +
> +		rm -f add_file_add_dir_del_file add_file_add_dir_del_dir &&
> +		mkdir add_file_add_dir_del_file add_file_add_dir_del_dir &&
> +		touch add_file_add_dir_del_file/file add_file_add_dir_del_dir/file &&
> +		p4 add add_file_add_dir_del_file/file add_file_add_dir_del_dir/file &&
> +		rm -rf add_dir_add_file_del_file add_dir_add_file_del_dir &&
> +		touch add_dir_add_file_del_file add_dir_add_file_del_dir &&
> +		p4 add add_dir_add_file_del_file add_dir_add_file_del_dir &&
> +		p4 submit -d "add conflicting" &&
> +
> +		p4 delete -k add_file_add_dir_del_file &&
> +		p4 delete -k add_file_add_dir_del_dir/file &&
> +		p4 delete -k add_dir_add_file_del_file &&
> +		p4 delete -k add_dir_add_file_del_dir/file &&
> +		p4 submit -d "delete conflicting"
> +	)
> +'
> +
> +test_expect_success 'clone with git-p4' '
> +	git p4 clone --dest="$git" //depot/@all
> +'
> +
> +test_expect_success 'check final contents' '
> +	test_path_is_dir "$git/add_file_add_dir_del_file" &&
> +	test_path_is_file "$git/add_file_add_dir_del_dir" &&
> +	test_path_is_dir "$git/add_dir_add_file_del_file" &&
> +	test_path_is_file "$git/add_dir_add_file_del_dir"
> +'
> +
> +test_expect_success 'kill p4d' '
> +	kill_p4d
> +'
> +
> +test_done
> -- 
> 2.19.2
> 


-- 
Luke Diamand <luke@diamand.org>

Re: [PATCH 1/1] git-p4: recover from inconsistent perforce history

[Andrew Oakley]

On Thu, 7 Feb 2019 08:04:20 +0000
Luke Diamand <luke@diamand.org> wrote:

> On Wed,  6 Feb 2019 19:42:19 +0000
> andrew@adoakley.name wrote:
> 
> > From: Andrew Oakley <andrew@adoakley.name>
> > 
> > Perforce allows you commit files and directories with the same
> > name, so you could have files //depot/foo and //depot/foo/bar both
> > checked in.  A p4 sync of a repository in this state fails.
> > Deleting one of the files recovers the repository.
> > 
> > When this happens we want git-p4 to recover in the same way as
> > perforce.  
> 
> I'm finding the test fails for me on a clean git repo, although I
> can't see any obvious reason why.

This introduced a failure when client specs are being used.  I wasn't
populating the cache (update_client_spec_path_cache) before checking if
a file should be included (inClientSpec).  I can rearrange the code to
fix this.

Thanks

-- 
Andrew Oakley

[PATCH v2] git-p4: recover from inconsistent perforce history

[andrew]

From: Andrew Oakley <andrew@adoakley.name>

Perforce allows you commit files and directories with the same name, so
you could have files //depot/foo and //depot/foo/bar both checked in.  A
p4 sync of a repository in this state fails.  Deleting one of the files
recovers the repository.

When this happens we want git-p4 to recover in the same way as perforce.

Signed-off-by: Andrew Oakley <andrew@adoakley.name>
---
 git-p4.py                      | 41 ++++++++++++++++++++++--
 t/t9834-git-p4-file-dir-bug.sh | 58 ++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 3 deletions(-)
 create mode 100755 t/t9834-git-p4-file-dir-bug.sh

diff --git a/git-p4.py b/git-p4.py
index 3e12774f96..42f6805641 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3012,6 +3012,40 @@ def hasBranchPrefix(self, path):
             print('Ignoring file outside of prefix: {0}'.format(path))
         return hasPrefix
 
+    def findShadowedFiles(self, files, change):
+        # Perforce allows you commit files and directories with the same name,
+        # so you could have files //depot/foo and //depot/foo/bar both checked
+        # in.  A p4 sync of a repository in this state fails.  Deleting one of
+        # the files recovers the repository.
+        #
+        # Git will not allow the broken state to exist and only the most recent
+        # of the conflicting names is left in the repository.  When one of the
+        # conflicting files is deleted we need to re-add the other one to make
+        # sure the git repository recovers in the same way as perforce.
+        deleted = [f for f in files if f['action'] in self.delete_actions]
+        to_check = set()
+        for f in deleted:
+            path = f['path']
+            to_check.add(path + '/...')
+            while True:
+                path = path.rsplit("/", 1)[0]
+                if path == "/" or path in to_check:
+                    break
+                to_check.add(path)
+        to_check = [p for p in to_check if self.hasBranchPrefix(p)]
+        if to_check:
+            stat_result = p4CmdList(
+                ["fstat", "-T", "depotFile,headRev,headType"] +
+                    ["%s@%s" % (p, change) for p in to_check])
+            for record in stat_result:
+                if record['code'] != 'stat':
+                    continue
+                files.append({
+                    'action': 'add',
+                    'path': record['depotFile'],
+                    'rev': record['headRev'],
+                    'type': record['headType']})
+
     def commit(self, details, files, branch, parent = "", allow_empty=False):
         epoch = details["time"]
         author = details["user"]
@@ -3020,11 +3054,12 @@ def commit(self, details, files, branch, parent = "", allow_empty=False):
         if self.verbose:
             print('commit into {0}'.format(branch))
 
+        files = [f for f in files if self.hasBranchPrefix(f['path'])]
+        self.findShadowedFiles(files, details["change"])
+
         if self.clientSpecDirs:
             self.clientSpecDirs.update_client_spec_path_cache(files)
-
-        files = [f for f in files
-            if self.inClientSpec(f['path']) and self.hasBranchPrefix(f['path'])]
+            files = [f for f in files if self.inClientSpec(f['path'])]
 
         if gitConfigBool('git-p4.keepEmptyCommits'):
             allow_empty = True
diff --git a/t/t9834-git-p4-file-dir-bug.sh b/t/t9834-git-p4-file-dir-bug.sh
new file mode 100755
index 0000000000..9839a3d2bb
--- /dev/null
+++ b/t/t9834-git-p4-file-dir-bug.sh
@@ -0,0 +1,58 @@
+#!/bin/sh
+
+test_description='git p4 directory/file bug handling
+
+This test creates files and directories with the same name in perforce and
+checks that git-p4 recovers from the error at the same time as the perforce
+repository.'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'init depot' '
+	(
+		cd "$cli" &&
+
+		touch add_file_add_dir_del_file add_file_add_dir_del_dir &&
+		p4 add add_file_add_dir_del_file add_file_add_dir_del_dir &&
+		mkdir add_dir_add_file_del_file add_dir_add_file_del_dir &&
+		touch add_dir_add_file_del_file/file add_dir_add_file_del_dir/file &&
+		p4 add add_dir_add_file_del_file/file add_dir_add_file_del_dir/file &&
+		p4 submit -d "add initial" &&
+
+		rm -f add_file_add_dir_del_file add_file_add_dir_del_dir &&
+		mkdir add_file_add_dir_del_file add_file_add_dir_del_dir &&
+		touch add_file_add_dir_del_file/file add_file_add_dir_del_dir/file &&
+		p4 add add_file_add_dir_del_file/file add_file_add_dir_del_dir/file &&
+		rm -rf add_dir_add_file_del_file add_dir_add_file_del_dir &&
+		touch add_dir_add_file_del_file add_dir_add_file_del_dir &&
+		p4 add add_dir_add_file_del_file add_dir_add_file_del_dir &&
+		p4 submit -d "add conflicting" &&
+
+		p4 delete -k add_file_add_dir_del_file &&
+		p4 delete -k add_file_add_dir_del_dir/file &&
+		p4 delete -k add_dir_add_file_del_file &&
+		p4 delete -k add_dir_add_file_del_dir/file &&
+		p4 submit -d "delete conflicting"
+	)
+'
+
+test_expect_success 'clone with git-p4' '
+	git p4 clone --dest="$git" //depot/@all
+'
+
+test_expect_success 'check final contents' '
+	test_path_is_dir "$git/add_file_add_dir_del_file" &&
+	test_path_is_file "$git/add_file_add_dir_del_dir" &&
+	test_path_is_dir "$git/add_dir_add_file_del_file" &&
+	test_path_is_file "$git/add_dir_add_file_del_dir"
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
2.19.2

Re: [PATCH v2] git-p4: recover from inconsistent perforce history

[Andrew Oakley]

On Fri,  8 Feb 2019 09:10:33 +0000
andrew@adoakley.name wrote:

> From: Andrew Oakley <andrew@adoakley.name>
> 
> Perforce allows you commit files and directories with the same name,
> so you could have files //depot/foo and //depot/foo/bar both checked
> in.  A p4 sync of a repository in this state fails.  Deleting one of
> the files recovers the repository.
> 
> When this happens we want git-p4 to recover in the same way as
> perforce.

Unfortunately this still has a few issues:
- newer p4d versions don't allow this by default so the tests fail
- deleted files get recreated in some cases
- command line too long when there are lots of deletions

These are all easily fixable, but I'll test more on a real repository
with before sharing another patch.

-- 
Andrew Oakley