# [PATCH 1/1] git-p4: recover from inconsistent perforce history 5 messages from 2019-02-06 to 2019-02-11. Participants: andrew@adoakley.name, Luke Diamand, Andrew Oakley. Thread: https://gitlist.dev/t/50417 ## andrew@adoakley.name, 2019-02-06 19:42 Subject: [PATCH 1/1] git-p4: recover from inconsistent perforce history Message-ID: <20190206194219.15507-1-andrew@adoakley.name> URL: https://gitlist.dev/e/20190206194219.15507-1-andrew%40adoakley.name ``` From: Andrew Oakley 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 --- 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, 2019-02-07 08:04 Subject: Re: [PATCH 1/1] git-p4: recover from inconsistent perforce history Message-ID: <20190207080420.395c521be017dafb6561784b@diamand.org> URL: https://gitlist.dev/e/20190207080420.395c521be017dafb6561784b%40diamand.org In-Reply-To: <20190206194219.15507-1-andrew@adoakley.name> ``` On Wed, 6 Feb 2019 19:42:19 +0000 andrew@adoakley.name wrote: > From: Andrew Oakley > > 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 > --- > 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 ``` ## Andrew Oakley, 2019-02-08 09:07 Subject: Re: [PATCH 1/1] git-p4: recover from inconsistent perforce history Message-ID: <20190208090704.68154885@ado-amd-gentoo.home.adoakley.name> URL: https://gitlist.dev/e/20190208090704.68154885%40ado-amd-gentoo.home.adoakley.name In-Reply-To: <20190207080420.395c521be017dafb6561784b@diamand.org> ``` On Thu, 7 Feb 2019 08:04:20 +0000 Luke Diamand wrote: > On Wed, 6 Feb 2019 19:42:19 +0000 > andrew@adoakley.name wrote: > > > From: Andrew Oakley > > > > 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 ``` ## andrew@adoakley.name, 2019-02-08 09:10 Subject: [PATCH v2] git-p4: recover from inconsistent perforce history Message-ID: <20190208091033.12617-1-andrew@adoakley.name> URL: https://gitlist.dev/e/20190208091033.12617-1-andrew%40adoakley.name In-Reply-To: <20190208090704.68154885@ado-amd-gentoo.home.adoakley.name> ``` From: Andrew Oakley 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 --- 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 ``` ## Andrew Oakley, 2019-02-11 08:54 Subject: Re: [PATCH v2] git-p4: recover from inconsistent perforce history Message-ID: <20190211085405.51d752b7@ado-amd-gentoo.home.adoakley.name> URL: https://gitlist.dev/e/20190211085405.51d752b7%40ado-amd-gentoo.home.adoakley.name In-Reply-To: <20190208091033.12617-1-andrew@adoakley.name> ``` On Fri, 8 Feb 2019 09:10:33 +0000 andrew@adoakley.name wrote: > From: Andrew Oakley > > 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 ```