{"thread":{"id":"53441","subject":"[PATCH v3] git-p4: recover from inconsistent perforce history","startedAt":"2020-05-10T10:42:40Z","lastAt":"2020-05-10T17:02:05Z","messageCount":4,"participants":["Andrew Oakley","Luke Diamand","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"397491","messageId":"20200510101650.50583-1-andrew@adoakley.name","threadId":"53441","inReplyTo":null,"subject":"[PATCH v3] git-p4: recover from inconsistent perforce history","fromName":"Andrew Oakley","fromEmail":"andrew@adoakley.name","sentAt":"2020-05-10T10:16:50Z","receivedAt":"2020-05-10T10:42:40Z","isPatch":true,"sender":{"key":"andrew@adoakley.name","avatar":"https://avatars.githubusercontent.com/u/1107440?v=4"},"body":"Perforce allows you commit files and directories with the same name, so\nyou could have files //depot/foo and //depot/foo/bar both checked in.  A\np4 sync of a repository in this state fails.  Deleting one of the files\nrecovers the repository.\n\nWhen this happens we want git-p4 to recover in the same way as perforce.\n\nSigned-off-by: Andrew Oakley <andrew@adoakley.name>\n---\n git-p4.py                      | 43 ++++++++++++++++++++-\n t/t9834-git-p4-file-dir-bug.sh | 70 ++++++++++++++++++++++++++++++++++\n 2 files changed, 111 insertions(+), 2 deletions(-)\n create mode 100755 t/t9834-git-p4-file-dir-bug.sh\n\ndiff --git a/git-p4.py b/git-p4.py\nindex b8b2a1679e..d551efb0dd 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -3214,6 +3214,42 @@ def hasBranchPrefix(self, path):\n             print('Ignoring file outside of prefix: {0}'.format(path))\n         return hasPrefix\n \n+    def findShadowedFiles(self, files, change):\n+        # Perforce allows you commit files and directories with the same name,\n+        # so you could have files //depot/foo and //depot/foo/bar both checked\n+        # in.  A p4 sync of a repository in this state fails.  Deleting one of\n+        # the files recovers the repository.\n+        #\n+        # Git will not allow the broken state to exist and only the most recent\n+        # of the conflicting names is left in the repository.  When one of the\n+        # conflicting files is deleted we need to re-add the other one to make\n+        # sure the git repository recovers in the same way as perforce.\n+        deleted = [f for f in files if f['action'] in self.delete_actions]\n+        to_check = set()\n+        for f in deleted:\n+            path = decode_path(f['path'])\n+            to_check.add(path + '/...')\n+            while True:\n+                path = path.rsplit(\"/\", 1)[0]\n+                if path == \"/\" or path in to_check:\n+                    break\n+                to_check.add(path)\n+        to_check = ['%s@%s' % (wildcard_encode(p), change) for p in to_check\n+            if self.hasBranchPrefix(p)]\n+        if to_check:\n+            stat_result = p4CmdList([\"-x\", \"-\", \"fstat\", \"-T\",\n+                \"depotFile,headAction,headRev,headType\"], stdin=to_check)\n+            for record in stat_result:\n+                if record['code'] != 'stat':\n+                    continue\n+                if record['headAction'] in self.delete_actions:\n+                    continue\n+                files.append({\n+                    'action': 'add',\n+                    'path': record['depotFile'],\n+                    'rev': record['headRev'],\n+                    'type': record['headType']})\n+\n     def commit(self, details, files, branch, parent = \"\", allow_empty=False):\n         epoch = details[\"time\"]\n         author = details[\"user\"]\n@@ -3222,11 +3258,14 @@ def commit(self, details, files, branch, parent = \"\", allow_empty=False):\n         if self.verbose:\n             print('commit into {0}'.format(branch))\n \n+        files = [f for f in files\n+            if self.hasBranchPrefix(decode_path(f['path']))]\n+        self.findShadowedFiles(files, details['change'])\n+\n         if self.clientSpecDirs:\n             self.clientSpecDirs.update_client_spec_path_cache(files)\n \n-        files = [f for (f, path) in ((f, decode_path(f['path'])) for f in files)\n-            if self.inClientSpec(path) and self.hasBranchPrefix(path)]\n+        files = [f for f in files if self.inClientSpec(decode_path(f['path']))]\n \n         if gitConfigBool('git-p4.keepEmptyCommits'):\n             allow_empty = True\ndiff --git a/t/t9834-git-p4-file-dir-bug.sh b/t/t9834-git-p4-file-dir-bug.sh\nnew file mode 100755\nindex 0000000000..031e1f8668\n--- /dev/null\n+++ b/t/t9834-git-p4-file-dir-bug.sh\n@@ -0,0 +1,70 @@\n+#!/bin/sh\n+\n+test_description='git p4 directory/file bug handling\n+\n+This test creates files and directories with the same name in perforce and\n+checks that git-p4 recovers from the error at the same time as the perforce\n+repository.'\n+\n+. ./lib-git-p4.sh\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d &&\n+\ttest_might_fail p4 configure set submit.collision.check=0\n+'\n+\n+test_expect_success 'init depot' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\ttouch add_file_add_dir_del_file add_file_add_dir_del_dir &&\n+\t\tp4 add add_file_add_dir_del_file add_file_add_dir_del_dir &&\n+\t\tmkdir add_dir_add_file_del_file add_dir_add_file_del_dir &&\n+\t\ttouch add_dir_add_file_del_file/file add_dir_add_file_del_dir/file &&\n+\t\tp4 add add_dir_add_file_del_file/file add_dir_add_file_del_dir/file &&\n+\t\tp4 submit -d \"add initial\" &&\n+\n+\t\trm -f add_file_add_dir_del_file add_file_add_dir_del_dir &&\n+\t\tmkdir add_file_add_dir_del_file add_file_add_dir_del_dir &&\n+\t\ttouch add_file_add_dir_del_file/file add_file_add_dir_del_dir/file &&\n+\t\tp4 add add_file_add_dir_del_file/file add_file_add_dir_del_dir/file &&\n+\t\trm -rf add_dir_add_file_del_file add_dir_add_file_del_dir &&\n+\t\ttouch add_dir_add_file_del_file add_dir_add_file_del_dir &&\n+\t\tp4 add add_dir_add_file_del_file add_dir_add_file_del_dir &&\n+\t\tp4 submit -d \"add conflicting\" &&\n+\n+\t\tp4 delete -k add_file_add_dir_del_file &&\n+\t\tp4 delete -k add_file_add_dir_del_dir/file &&\n+\t\tp4 delete -k add_dir_add_file_del_file &&\n+\t\tp4 delete -k add_dir_add_file_del_dir/file &&\n+\t\tp4 submit -d \"delete conflicting\" &&\n+\n+\t\tp4 delete -k \"add_file_add_dir_del_file/file\" &&\n+\t\tp4 delete -k \"add_file_add_dir_del_dir\" &&\n+\t\tp4 delete -k \"add_dir_add_file_del_file/file\" &&\n+\t\tp4 delete -k \"add_dir_add_file_del_dir\" &&\n+\t\tp4 submit -d \"delete remaining\"\n+\t)\n+'\n+\n+test_expect_success 'clone with git-p4' '\n+\tgit p4 clone --dest=\"$git\" //depot/@1,3\n+'\n+\n+test_expect_success 'check contents' '\n+\ttest_path_is_dir \"$git/add_file_add_dir_del_file\" &&\n+\ttest_path_is_file \"$git/add_file_add_dir_del_dir\" &&\n+\ttest_path_is_dir \"$git/add_dir_add_file_del_file\" &&\n+\ttest_path_is_file \"$git/add_dir_add_file_del_dir\"\n+'\n+\n+test_expect_success 'rebase and check empty' '\n+\tgit -C \"$git\" p4 rebase &&\n+\n+\ttest_path_is_missing \"$git/add_file_add_dir_del_file\" &&\n+\ttest_path_is_missing \"$git/add_file_add_dir_del_dir\" &&\n+\ttest_path_is_missing \"$git/add_dir_add_file_del_file\" &&\n+\ttest_path_is_missing \"$git/add_dir_add_file_del_dir\"\n+'\n+\n+test_done\n-- \n2.24.1\n\n"},{"id":"397495","messageId":"CAE5ih793qKyOSE-hkOw7+nFmM3XTRxxrXv0FD2+WWXjGbVHkoQ@mail.gmail.com","threadId":"53441","inReplyTo":"20200510101650.50583-1-andrew@adoakley.name","subject":"Re: [PATCH v3] git-p4: recover from inconsistent perforce history","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-05-10T12:03:11Z","receivedAt":"2020-05-10T12:03:25Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Sun, 10 May 2020 at 11:17, Andrew Oakley <andrew@adoakley.name> wrote:\n>\n> Perforce allows you commit files and directories with the same name, so\n> you could have files //depot/foo and //depot/foo/bar both checked in.  A\n> p4 sync of a repository in this state fails.  Deleting one of the files\n> recovers the repository.\n>\n> When this happens we want git-p4 to recover in the same way as perforce.\n\nLooks good to me.\n\nPerforce changed their server to reject this kind of thing in the\n2017.1 version:\n\n    Bugs fixed in 2017.1\n    #1489051 (Job #2170) **\n       Submitting a file with the same name as an existing depot\n       directory path (or vice versa) will now be rejected.\n\n(Of course people will still have damaged repos even today).\n\nI tried your test with both the 2015.1 and the 2020.1 versions, and it\nworked in both cases - shouldn't it be impossible to get into the\nstate that git-p4 now recovers from with a newer p4d?\n\nLuke\n\n\n>\n>\n> Signed-off-by: Andrew Oakley <andrew@adoakley.name>\n> ---\n>  git-p4.py                      | 43 ++++++++++++++++++++-\n>  t/t9834-git-p4-file-dir-bug.sh | 70 ++++++++++++++++++++++++++++++++++\n>  2 files changed, 111 insertions(+), 2 deletions(-)\n>  create mode 100755 t/t9834-git-p4-file-dir-bug.sh\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index b8b2a1679e..d551efb0dd 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -3214,6 +3214,42 @@ def hasBranchPrefix(self, path):\n>              print('Ignoring file outside of prefix: {0}'.format(path))\n>          return hasPrefix\n>\n> +    def findShadowedFiles(self, files, change):\n> +        # Perforce allows you commit files and directories with the same name,\n> +        # so you could have files //depot/foo and //depot/foo/bar both checked\n> +        # in.  A p4 sync of a repository in this state fails.  Deleting one of\n> +        # the files recovers the repository.\n> +        #\n> +        # Git will not allow the broken state to exist and only the most recent\n> +        # of the conflicting names is left in the repository.  When one of the\n> +        # conflicting files is deleted we need to re-add the other one to make\n> +        # sure the git repository recovers in the same way as perforce.\n> +        deleted = [f for f in files if f['action'] in self.delete_actions]\n> +        to_check = set()\n> +        for f in deleted:\n> +            path = decode_path(f['path'])\n> +            to_check.add(path + '/...')\n> +            while True:\n> +                path = path.rsplit(\"/\", 1)[0]\n> +                if path == \"/\" or path in to_check:\n> +                    break\n> +                to_check.add(path)\n> +        to_check = ['%s@%s' % (wildcard_encode(p), change) for p in to_check\n> +            if self.hasBranchPrefix(p)]\n> +        if to_check:\n> +            stat_result = p4CmdList([\"-x\", \"-\", \"fstat\", \"-T\",\n> +                \"depotFile,headAction,headRev,headType\"], stdin=to_check)\n> +            for record in stat_result:\n> +                if record['code'] != 'stat':\n> +                    continue\n> +                if record['headAction'] in self.delete_actions:\n> +                    continue\n> +                files.append({\n> +                    'action': 'add',\n> +                    'path': record['depotFile'],\n> +                    'rev': record['headRev'],\n> +                    'type': record['headType']})\n> +\n>      def commit(self, details, files, branch, parent = \"\", allow_empty=False):\n>          epoch = details[\"time\"]\n>          author = details[\"user\"]\n> @@ -3222,11 +3258,14 @@ def commit(self, details, files, branch, parent = \"\", allow_empty=False):\n>          if self.verbose:\n>              print('commit into {0}'.format(branch))\n>\n> +        files = [f for f in files\n> +            if self.hasBranchPrefix(decode_path(f['path']))]\n> +        self.findShadowedFiles(files, details['change'])\n> +\n>          if self.clientSpecDirs:\n>              self.clientSpecDirs.update_client_spec_path_cache(files)\n>\n> -        files = [f for (f, path) in ((f, decode_path(f['path'])) for f in files)\n> -            if self.inClientSpec(path) and self.hasBranchPrefix(path)]\n> +        files = [f for f in files if self.inClientSpec(decode_path(f['path']))]\n>\n>          if gitConfigBool('git-p4.keepEmptyCommits'):\n>              allow_empty = True\n> diff --git a/t/t9834-git-p4-file-dir-bug.sh b/t/t9834-git-p4-file-dir-bug.sh\n> new file mode 100755\n> index 0000000000..031e1f8668\n> --- /dev/null\n> +++ b/t/t9834-git-p4-file-dir-bug.sh\n> @@ -0,0 +1,70 @@\n> +#!/bin/sh\n> +\n> +test_description='git p4 directory/file bug handling\n> +\n> +This test creates files and directories with the same name in perforce and\n> +checks that git-p4 recovers from the error at the same time as the perforce\n> +repository.'\n> +\n> +. ./lib-git-p4.sh\n> +\n> +test_expect_success 'start p4d' '\n> +       start_p4d &&\n> +       test_might_fail p4 configure set submit.collision.check=0\n> +'\n> +\n> +test_expect_success 'init depot' '\n> +       (\n> +               cd \"$cli\" &&\n> +\n> +               touch add_file_add_dir_del_file add_file_add_dir_del_dir &&\n> +               p4 add add_file_add_dir_del_file add_file_add_dir_del_dir &&\n> +               mkdir add_dir_add_file_del_file add_dir_add_file_del_dir &&\n> +               touch add_dir_add_file_del_file/file add_dir_add_file_del_dir/file &&\n> +               p4 add add_dir_add_file_del_file/file add_dir_add_file_del_dir/file &&\n> +               p4 submit -d \"add initial\" &&\n> +\n> +               rm -f add_file_add_dir_del_file add_file_add_dir_del_dir &&\n> +               mkdir add_file_add_dir_del_file add_file_add_dir_del_dir &&\n> +               touch add_file_add_dir_del_file/file add_file_add_dir_del_dir/file &&\n> +               p4 add add_file_add_dir_del_file/file add_file_add_dir_del_dir/file &&\n> +               rm -rf add_dir_add_file_del_file add_dir_add_file_del_dir &&\n> +               touch add_dir_add_file_del_file add_dir_add_file_del_dir &&\n> +               p4 add add_dir_add_file_del_file add_dir_add_file_del_dir &&\n> +               p4 submit -d \"add conflicting\" &&\n> +\n> +               p4 delete -k add_file_add_dir_del_file &&\n> +               p4 delete -k add_file_add_dir_del_dir/file &&\n> +               p4 delete -k add_dir_add_file_del_file &&\n> +               p4 delete -k add_dir_add_file_del_dir/file &&\n> +               p4 submit -d \"delete conflicting\" &&\n> +\n> +               p4 delete -k \"add_file_add_dir_del_file/file\" &&\n> +               p4 delete -k \"add_file_add_dir_del_dir\" &&\n> +               p4 delete -k \"add_dir_add_file_del_file/file\" &&\n> +               p4 delete -k \"add_dir_add_file_del_dir\" &&\n> +               p4 submit -d \"delete remaining\"\n> +       )\n> +'\n> +\n> +test_expect_success 'clone with git-p4' '\n> +       git p4 clone --dest=\"$git\" //depot/@1,3\n> +'\n> +\n> +test_expect_success 'check contents' '\n> +       test_path_is_dir \"$git/add_file_add_dir_del_file\" &&\n> +       test_path_is_file \"$git/add_file_add_dir_del_dir\" &&\n> +       test_path_is_dir \"$git/add_dir_add_file_del_file\" &&\n> +       test_path_is_file \"$git/add_dir_add_file_del_dir\"\n> +'\n> +\n> +test_expect_success 'rebase and check empty' '\n> +       git -C \"$git\" p4 rebase &&\n> +\n> +       test_path_is_missing \"$git/add_file_add_dir_del_file\" &&\n> +       test_path_is_missing \"$git/add_file_add_dir_del_dir\" &&\n> +       test_path_is_missing \"$git/add_dir_add_file_del_file\" &&\n> +       test_path_is_missing \"$git/add_dir_add_file_del_dir\"\n> +'\n> +\n> +test_done\n> --\n> 2.24.1\n>\n"},{"id":"397496","messageId":"20200510151354.16ce2ab1@ado-tr.home.arpa","threadId":"53441","inReplyTo":"CAE5ih793qKyOSE-hkOw7+nFmM3XTRxxrXv0FD2+WWXjGbVHkoQ@mail.gmail.com","subject":"Re: [PATCH v3] git-p4: recover from inconsistent perforce history","fromName":"Andrew Oakley","fromEmail":"andrew@adoakley.name","sentAt":"2020-05-10T14:13:54Z","receivedAt":"2020-05-10T14:14:07Z","isPatch":true,"sender":{"key":"andrew@adoakley.name","avatar":"https://avatars.githubusercontent.com/u/1107440?v=4"},"body":"On Sun, 10 May 2020 13:03:11 +0100\nLuke Diamand <luke@diamand.org> wrote:\n> Perforce changed their server to reject this kind of thing in the\n> 2017.1 version:\n> \n>     Bugs fixed in 2017.1\n>     #1489051 (Job #2170) **\n>        Submitting a file with the same name as an existing depot\n>        directory path (or vice versa) will now be rejected.\n> \n> (Of course people will still have damaged repos even today).\n> \n> I tried your test with both the 2015.1 and the 2020.1 versions, and it\n> worked in both cases - shouldn't it be impossible to get into the\n> state that git-p4 now recovers from with a newer p4d?\n\nYes, there is an option in perforce (submit.collision.check) that stops\nnew changelists from introducing this problem, and it is turned on by\ndefault.  Unfortunately this option does *exactly* what the description\nsays, so you can't delete a directory and replace it with a file in the\nsame changelist - the delete has to happen first.  It's not clear which\nbehaviour is least bad.\n\nThe test case tries to turn the perforce option off so it works on both\nold and new server versions.\n\nThanks\n"},{"id":"397506","messageId":"xmqqsgg7u3js.fsf@gitster.c.googlers.com","threadId":"53441","inReplyTo":"CAE5ih793qKyOSE-hkOw7+nFmM3XTRxxrXv0FD2+WWXjGbVHkoQ@mail.gmail.com","subject":"Re: [PATCH v3] git-p4: recover from inconsistent perforce history","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-10T17:01:59Z","receivedAt":"2020-05-10T17:02:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand <luke@diamand.org> writes:\n\n> On Sun, 10 May 2020 at 11:17, Andrew Oakley <andrew@adoakley.name> wrote:\n>>\n>> Perforce allows you commit files and directories with the same name, so\n>> you could have files //depot/foo and //depot/foo/bar both checked in.  A\n>> p4 sync of a repository in this state fails.  Deleting one of the files\n>> recovers the repository.\n>>\n>> When this happens we want git-p4 to recover in the same way as perforce.\n>\n> Looks good to me.\n>\n> Perforce changed their server to reject this kind of thing in the\n> 2017.1 version:\n>\n>     Bugs fixed in 2017.1\n>     #1489051 (Job #2170) **\n>        Submitting a file with the same name as an existing depot\n>        directory path (or vice versa) will now be rejected.\n>\n> (Of course people will still have damaged repos even today).\n\nPerhaps it is worth describing the above in the log message?  E.g.\n\n    Perforce allows you commit files and directories with the same name,\n    so you could have files //depot/foo and //depot/foo/bar both checked\n    in.  A p4 sync of a repository in this state fails.  Deleting one of\n    the files recovers the repository.\n\n    When this happens we want git-p4 to recover in the same way as\n    perforce.\n\n    Note that Perforce has this change in their 2017.1 version:\n\n         Bugs fixed in 2017.1\n         #1489051 (Job #2170) **\n            Submitting a file with the same name as an existing depot\n            directory path (or vice versa) will now be rejected.\n\n    so people hopefully will not creating damaged Perforce repos\n    anymore, but \"git p4\" needs to be able to interact with already\n    corrupt ones.\n\n    Signed-off-by: Andrew Oakley <andrew@adoakley.name>\n    Reviewed-by: Luke Diamand <luke@diamand.org>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nThanks.\n"}]}