{"thread":{"id":"55648","subject":"[PATCH] git-p4: fix \"git p4 sync\" after ignored changelist","startedAt":"2021-05-08T16:12:38Z","lastAt":"2021-05-10T17:36:57Z","messageCount":3,"participants":["Evan McLain via GitGitGadget","Junio C Hamano","Andrew Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"423935","messageId":"pull.941.git.1620490353758.gitgitgadget@gmail.com","threadId":"55648","inReplyTo":null,"subject":"[PATCH] git-p4: fix \"git p4 sync\" after ignored changelist","fromName":"Evan McLain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-08T16:12:33Z","receivedAt":"2021-05-08T16:12:38Z","isPatch":true,"sender":{"key":"name:Evan McLain","avatar":null},"body":"From: Evan McLain <git.commit@none-of-yer.biz>\n\nAfter a sync/clone, if the very next p4 change is outside the client\nview (and therefore \"empty\" as far as git-p4 is concerned), a\nsubsequent sync will fail with a message like:\n\n     \"fast-import failed: warning: Not updating refs/remotes/p4/master\n      (new tip xxxxxx does not contain yyyyyy)\"\n\nThe bug is caused by discarding the parent commit information\nunconditionally after processing a p4 change, whether the change was\ncommitted or not.  This leaves the next non-empty commit disconnected\nfrom its parent, causing fast-import to fail.  This only occurs when\ngit-p4.keepEmptyCommits is false, but that is the default.\n\nRename P4Sync.commit() to maybeCommit() and return True if the change\nis committed, or False if ignored. Clear P4Sync.initialParent only if\nmaybeCommit() returns True.\n\nDiagnosed by IanH at https://stackoverflow.com/a/39876288/2033415\n\nSigned-off-by: Evan McLain <git.commit@none-of-yer.biz>\n---\n    git-p4: fix \"git p4 sync\" after ignored changelist\n    \n    After a sync/clone, if the very next p4 change is outside the client\n    view (and therefore \"empty\" as far as git-p4 is concerned), a subsequent\n    sync will fail with a message like:\n    \n     \"fast-import failed: warning: Not updating refs/remotes/p4/master\n      (new tip xxxxxx does not contain yyyyyy)\"\n    \n    \n    The bug is caused by discarding the parent commit information\n    unconditionally after processing a p4 change, whether the change was\n    committed or not. This leaves the next non-empty commit disconnected\n    from its parent, causing fast-import to fail. This only occurs when\n    git-p4.keepEmptyCommits is false, but that is the default.\n    \n    Rename P4Sync.commit() to maybeCommit() and return True if the change is\n    committed, or False if ignored. Clear P4Sync.initialParent only if\n    maybeCommit() returns True.\n    \n    Diagnosed by IanH at https://stackoverflow.com/a/39876288/2033415\n    \n    Signed-off-by: Evan McLain git.commit@none-of-yer.biz\n    \n    ===\n    \n    There may be some other latent bugs here that I haven't fixed. In\n    particular, there seems to be a similar flow when detecting branches\n    with del self.initialParents[branch]. I wasn't sure how to set up a\n    repro case to expose that error, so I just fixed the bug I understood.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-941%2Femclain%2Fem%2Ffix-p4-sync-after-ignored-change-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-941/emclain/em/fix-p4-sync-after-ignored-change-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/941\n\n git-p4.py                     | 29 +++++++++++++++------------\n t/t9809-git-p4-client-view.sh | 37 +++++++++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 13 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 09c9e93ac401..f15818e1a842 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -3251,7 +3251,9 @@ def findShadowedFiles(self, files, change):\n                     'rev': record['headRev'],\n                     'type': record['headType']})\n \n-    def commit(self, details, files, branch, parent = \"\", allow_empty=False):\n+    # Commit a p4 change to git, unless it should be ignored.\n+    # Returns True if the change was committed to git, or False if it was ignored.\n+    def maybeCommit(self, details, files, branch, parent = \"\", allow_empty=False):\n         epoch = details[\"time\"]\n         author = details[\"user\"]\n         jobs = self.extractJobsFromCommit(details)\n@@ -3274,7 +3276,7 @@ def commit(self, details, files, branch, parent = \"\", allow_empty=False):\n         if not files and not allow_empty:\n             print('Ignoring revision {0} as it would produce an empty commit.'\n                 .format(details['change']))\n-            return\n+            return False\n \n         self.gitStream.write(\"commit %s\\n\" % branch)\n         self.gitStream.write(\"mark :%s\\n\" % details[\"change\"])\n@@ -3340,6 +3342,7 @@ def commit(self, details, files, branch, parent = \"\", allow_empty=False):\n                 if not self.silent:\n                     print(\"Tag %s does not match with change %s: file count is different.\"\n                            % (labelDetails[\"label\"], change))\n+        return True\n \n     # Build a dictionary of changelists and labels, for \"detect-labels\" option.\n     def getLabels(self):\n@@ -3676,22 +3679,22 @@ def importChanges(self, changes, origin_revision=0):\n                             tempBranch = \"%s/%d\" % (self.tempBranchLocation, change)\n                             if self.verbose:\n                                 print(\"Creating temporary branch: \" + tempBranch)\n-                            self.commit(description, filesForCommit, tempBranch)\n+                            self.maybeCommit(description, filesForCommit, tempBranch)\n                             self.tempBranches.append(tempBranch)\n                             self.checkpoint()\n                             blob = self.searchParent(parent, branch, tempBranch)\n                         if blob:\n-                            self.commit(description, filesForCommit, branch, blob)\n+                            self.maybeCommit(description, filesForCommit, branch, blob)\n                         else:\n                             if self.verbose:\n                                 print(\"Parent of %s not found. Committing into head of %s\" % (branch, parent))\n-                            self.commit(description, filesForCommit, branch, parent)\n+                            self.maybeCommit(description, filesForCommit, branch, parent)\n                 else:\n                     files = self.extractFilesFromCommit(description)\n-                    self.commit(description, files, self.branch,\n-                                self.initialParent)\n-                    # only needed once, to connect to the previous commit\n-                    self.initialParent = \"\"\n+                    if self.maybeCommit(description, files, self.branch,\n+                                        self.initialParent):\n+                        # only needed once, to connect to the previous commit\n+                        self.initialParent = \"\"\n             except IOError:\n                 print(self.gitError.read())\n                 sys.exit(1)\n@@ -3755,7 +3758,7 @@ def importHeadRevision(self, revision):\n \n         self.updateOptionDict(details)\n         try:\n-            self.commit(details, self.extractFilesFromCommit(details), self.branch)\n+            self.maybeCommit(details, self.extractFilesFromCommit(details), self.branch)\n         except IOError as err:\n             print(\"IO error with git fast-import. Is your git version recent enough?\")\n             print(\"IO error details: {}\".format(err))\n@@ -4265,8 +4268,8 @@ def createShelveParent(self, change, branch_name, sync, origin):\n \n             parent_files.append(f)\n \n-        sync.commit(parent_description, parent_files, branch_name,\n-                parent=origin, allow_empty=True)\n+        sync.maybeCommit(parent_description, parent_files, branch_name,\n+                         parent=origin, allow_empty=True)\n         print(\"created parent commit for {0} based on {1} in {2}\".format(\n             change, self.origin, branch_name))\n \n@@ -4307,7 +4310,7 @@ def run(self, args):\n         description = p4_describe(change, True)\n         files = sync.extractFilesFromCommit(description, True, change)\n \n-        sync.commit(description, files, branch_name, \"\")\n+        sync.maybeCommit(description, files, branch_name, \"\")\n         sync.closeStreams()\n \n         print(\"unshelved changelist {0} into {1}\".format(change, branch_name))\ndiff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh\nindex 9c9710d8c7b8..4daf0305474c 100755\n--- a/t/t9809-git-p4-client-view.sh\n+++ b/t/t9809-git-p4-client-view.sh\n@@ -147,6 +147,43 @@ test_expect_success 'later mapping takes precedence (partial repo)' '\n \tgit_verify $files\n '\n \n+# after a sync/clone of a partial client view, if the very next p4 change is outside\n+# the view and thus ignored, the a subsequent sync will fail to connect to its parent:\n+#     \"fast-import failed: warning: Not updating refs/remotes/p4/master\n+#      (new tip x does not contain y)\"\n+test_expect_success 'partial sync bug with ignored change' '\n+\tclient_view \"//depot/dir1/... //client/...\" &&\n+\tfiles=\"file11 file12\" &&\n+\tclient_verify $files &&\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --use-client-spec --dest=\"$git\" //depot &&\n+\tcli2=\"$TRASH_DIRECTORY/cli2\" &&\n+\tmkdir -p \"$cli2\" &&\n+\ttest_when_finished \"p4 client -f -d client2 && rm -rf \\\"$cli2\\\"\" &&\n+\t(\n+\t\tcd \"$cli2\" &&\n+\t\tP4CLIENT=client2 &&\n+\t\tcli=\"$cli2\" &&\n+\t\tclient_view \"//depot/... //client2/...\" &&\n+\t\tp4 sync &&\n+\t\tp4 open dir2/file21 &&\n+\t\techo dir2/file21 update >dir2/file21 &&\n+\t\tp4 submit -d \"update dir2/file21\"\n+\t) &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tp4 sync &&\n+\t\tp4 open file11 &&\n+\t\techo file11 update >file11 &&\n+\t\tp4 submit -d \"update file11\"\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit p4 sync --use-client-spec\n+\t) &&\n+\tgit_verify $files\n+'\n+\n # Reading the view backwards,\n #   dir2 goes to cli12\n #   dir1 cannot go to cli12 since it was filled by dir2\n\nbase-commit: 48bf2fa8bad054d66bd79c6ba903c89c704201f7\n-- \ngitgitgadget\n"},{"id":"423990","messageId":"xmqqsg2vrzyk.fsf@gitster.g","threadId":"55648","inReplyTo":"pull.941.git.1620490353758.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-p4: fix \"git p4 sync\" after ignored changelist","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-10T04:10:27Z","receivedAt":"2021-05-10T04:10:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Even to somebody like me who does not know P4 all that much, the log\nmessage does indicate that it is written by somebody who knows what\ns/he is talking about, but asking help evaluating, or better yet,\ngiving Acks, from those who had contributions in git-p4 in the past\n12 months.\n\nThanks.\n\n\"Evan McLain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Evan McLain <git.commit@none-of-yer.biz>\n>\n> After a sync/clone, if the very next p4 change is outside the client\n> view (and therefore \"empty\" as far as git-p4 is concerned), a\n> subsequent sync will fail with a message like:\n>\n>      \"fast-import failed: warning: Not updating refs/remotes/p4/master\n>       (new tip xxxxxx does not contain yyyyyy)\"\n>\n> The bug is caused by discarding the parent commit information\n> unconditionally after processing a p4 change, whether the change was\n> committed or not.  This leaves the next non-empty commit disconnected\n> from its parent, causing fast-import to fail.  This only occurs when\n> git-p4.keepEmptyCommits is false, but that is the default.\n>\n> Rename P4Sync.commit() to maybeCommit() and return True if the change\n> is committed, or False if ignored. Clear P4Sync.initialParent only if\n> maybeCommit() returns True.\n>\n> Diagnosed by IanH at https://stackoverflow.com/a/39876288/2033415\n>\n> Signed-off-by: Evan McLain <git.commit@none-of-yer.biz>\n> ---\n>     git-p4: fix \"git p4 sync\" after ignored changelist\n>     \n>     After a sync/clone, if the very next p4 change is outside the client\n>     view (and therefore \"empty\" as far as git-p4 is concerned), a subsequent\n>     sync will fail with a message like:\n>     \n>      \"fast-import failed: warning: Not updating refs/remotes/p4/master\n>       (new tip xxxxxx does not contain yyyyyy)\"\n>     \n>     \n>     The bug is caused by discarding the parent commit information\n>     unconditionally after processing a p4 change, whether the change was\n>     committed or not. This leaves the next non-empty commit disconnected\n>     from its parent, causing fast-import to fail. This only occurs when\n>     git-p4.keepEmptyCommits is false, but that is the default.\n>     \n>     Rename P4Sync.commit() to maybeCommit() and return True if the change is\n>     committed, or False if ignored. Clear P4Sync.initialParent only if\n>     maybeCommit() returns True.\n>     \n>     Diagnosed by IanH at https://stackoverflow.com/a/39876288/2033415\n>     \n>     Signed-off-by: Evan McLain git.commit@none-of-yer.biz\n>     \n>     ===\n>     \n>     There may be some other latent bugs here that I haven't fixed. In\n>     particular, there seems to be a similar flow when detecting branches\n>     with del self.initialParents[branch]. I wasn't sure how to set up a\n>     repro case to expose that error, so I just fixed the bug I understood.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-941%2Femclain%2Fem%2Ffix-p4-sync-after-ignored-change-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-941/emclain/em/fix-p4-sync-after-ignored-change-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/941\n>\n>  git-p4.py                     | 29 +++++++++++++++------------\n>  t/t9809-git-p4-client-view.sh | 37 +++++++++++++++++++++++++++++++++++\n>  2 files changed, 53 insertions(+), 13 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 09c9e93ac401..f15818e1a842 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -3251,7 +3251,9 @@ def findShadowedFiles(self, files, change):\n>                      'rev': record['headRev'],\n>                      'type': record['headType']})\n>  \n> -    def commit(self, details, files, branch, parent = \"\", allow_empty=False):\n> +    # Commit a p4 change to git, unless it should be ignored.\n> +    # Returns True if the change was committed to git, or False if it was ignored.\n> +    def maybeCommit(self, details, files, branch, parent = \"\", allow_empty=False):\n>          epoch = details[\"time\"]\n>          author = details[\"user\"]\n>          jobs = self.extractJobsFromCommit(details)\n> @@ -3274,7 +3276,7 @@ def commit(self, details, files, branch, parent = \"\", allow_empty=False):\n>          if not files and not allow_empty:\n>              print('Ignoring revision {0} as it would produce an empty commit.'\n>                  .format(details['change']))\n> -            return\n> +            return False\n>  \n>          self.gitStream.write(\"commit %s\\n\" % branch)\n>          self.gitStream.write(\"mark :%s\\n\" % details[\"change\"])\n> @@ -3340,6 +3342,7 @@ def commit(self, details, files, branch, parent = \"\", allow_empty=False):\n>                  if not self.silent:\n>                      print(\"Tag %s does not match with change %s: file count is different.\"\n>                             % (labelDetails[\"label\"], change))\n> +        return True\n>  \n>      # Build a dictionary of changelists and labels, for \"detect-labels\" option.\n>      def getLabels(self):\n> @@ -3676,22 +3679,22 @@ def importChanges(self, changes, origin_revision=0):\n>                              tempBranch = \"%s/%d\" % (self.tempBranchLocation, change)\n>                              if self.verbose:\n>                                  print(\"Creating temporary branch: \" + tempBranch)\n> -                            self.commit(description, filesForCommit, tempBranch)\n> +                            self.maybeCommit(description, filesForCommit, tempBranch)\n>                              self.tempBranches.append(tempBranch)\n>                              self.checkpoint()\n>                              blob = self.searchParent(parent, branch, tempBranch)\n>                          if blob:\n> -                            self.commit(description, filesForCommit, branch, blob)\n> +                            self.maybeCommit(description, filesForCommit, branch, blob)\n>                          else:\n>                              if self.verbose:\n>                                  print(\"Parent of %s not found. Committing into head of %s\" % (branch, parent))\n> -                            self.commit(description, filesForCommit, branch, parent)\n> +                            self.maybeCommit(description, filesForCommit, branch, parent)\n>                  else:\n>                      files = self.extractFilesFromCommit(description)\n> -                    self.commit(description, files, self.branch,\n> -                                self.initialParent)\n> -                    # only needed once, to connect to the previous commit\n> -                    self.initialParent = \"\"\n> +                    if self.maybeCommit(description, files, self.branch,\n> +                                        self.initialParent):\n> +                        # only needed once, to connect to the previous commit\n> +                        self.initialParent = \"\"\n>              except IOError:\n>                  print(self.gitError.read())\n>                  sys.exit(1)\n> @@ -3755,7 +3758,7 @@ def importHeadRevision(self, revision):\n>  \n>          self.updateOptionDict(details)\n>          try:\n> -            self.commit(details, self.extractFilesFromCommit(details), self.branch)\n> +            self.maybeCommit(details, self.extractFilesFromCommit(details), self.branch)\n>          except IOError as err:\n>              print(\"IO error with git fast-import. Is your git version recent enough?\")\n>              print(\"IO error details: {}\".format(err))\n> @@ -4265,8 +4268,8 @@ def createShelveParent(self, change, branch_name, sync, origin):\n>  \n>              parent_files.append(f)\n>  \n> -        sync.commit(parent_description, parent_files, branch_name,\n> -                parent=origin, allow_empty=True)\n> +        sync.maybeCommit(parent_description, parent_files, branch_name,\n> +                         parent=origin, allow_empty=True)\n>          print(\"created parent commit for {0} based on {1} in {2}\".format(\n>              change, self.origin, branch_name))\n>  \n> @@ -4307,7 +4310,7 @@ def run(self, args):\n>          description = p4_describe(change, True)\n>          files = sync.extractFilesFromCommit(description, True, change)\n>  \n> -        sync.commit(description, files, branch_name, \"\")\n> +        sync.maybeCommit(description, files, branch_name, \"\")\n>          sync.closeStreams()\n>  \n>          print(\"unshelved changelist {0} into {1}\".format(change, branch_name))\n> diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh\n> index 9c9710d8c7b8..4daf0305474c 100755\n> --- a/t/t9809-git-p4-client-view.sh\n> +++ b/t/t9809-git-p4-client-view.sh\n> @@ -147,6 +147,43 @@ test_expect_success 'later mapping takes precedence (partial repo)' '\n>  \tgit_verify $files\n>  '\n>  \n> +# after a sync/clone of a partial client view, if the very next p4 change is outside\n> +# the view and thus ignored, the a subsequent sync will fail to connect to its parent:\n> +#     \"fast-import failed: warning: Not updating refs/remotes/p4/master\n> +#      (new tip x does not contain y)\"\n> +test_expect_success 'partial sync bug with ignored change' '\n> +\tclient_view \"//depot/dir1/... //client/...\" &&\n> +\tfiles=\"file11 file12\" &&\n> +\tclient_verify $files &&\n> +\ttest_when_finished cleanup_git &&\n> +\tgit p4 clone --use-client-spec --dest=\"$git\" //depot &&\n> +\tcli2=\"$TRASH_DIRECTORY/cli2\" &&\n> +\tmkdir -p \"$cli2\" &&\n> +\ttest_when_finished \"p4 client -f -d client2 && rm -rf \\\"$cli2\\\"\" &&\n> +\t(\n> +\t\tcd \"$cli2\" &&\n> +\t\tP4CLIENT=client2 &&\n> +\t\tcli=\"$cli2\" &&\n> +\t\tclient_view \"//depot/... //client2/...\" &&\n> +\t\tp4 sync &&\n> +\t\tp4 open dir2/file21 &&\n> +\t\techo dir2/file21 update >dir2/file21 &&\n> +\t\tp4 submit -d \"update dir2/file21\"\n> +\t) &&\n> +\t(\n> +\t\tcd \"$cli\" &&\n> +\t\tp4 sync &&\n> +\t\tp4 open file11 &&\n> +\t\techo file11 update >file11 &&\n> +\t\tp4 submit -d \"update file11\"\n> +\t) &&\n> +\t(\n> +\t\tcd \"$git\" &&\n> +\t\tgit p4 sync --use-client-spec\n> +\t) &&\n> +\tgit_verify $files\n> +'\n> +\n>  # Reading the view backwards,\n>  #   dir2 goes to cli12\n>  #   dir1 cannot go to cli12 since it was filled by dir2\n>\n> base-commit: 48bf2fa8bad054d66bd79c6ba903c89c704201f7\n"},{"id":"424061","messageId":"20210510183638.156a6b1d@ado-tr","threadId":"55648","inReplyTo":"pull.941.git.1620490353758.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-p4: fix \"git p4 sync\" after ignored changelist","fromName":"Andrew Oakley","fromEmail":"andrew@adoakley.name","sentAt":"2021-05-10T17:36:38Z","receivedAt":"2021-05-10T17:36:57Z","isPatch":true,"sender":{"key":"andrew@adoakley.name","avatar":"https://avatars.githubusercontent.com/u/1107440?v=4"},"body":"On Sat, 08 May 2021 16:12:33 +0000\n\"Evan McLain via GitGitGadget\" <gitgitgadget@gmail.com> wrote:\n>     Rename P4Sync.commit() to maybeCommit() and return True if the\n> change is committed, or False if ignored. Clear P4Sync.initialParent\n> only if maybeCommit() returns True.\n\nI'm not sure I'd bother doing the rename - it makes the diff more noisy\nthan it needs to be.\n\n>     There may be some other latent bugs here that I haven't fixed. In\n>     particular, there seems to be a similar flow when detecting\n> branches with del self.initialParents[branch]. I wasn't sure how to\n> set up a repro case to expose that error, so I just fixed the bug I\n> understood.\n\nI don't think this issue can happen when using --detect-branches.  The\nlist of changed files get split up in splitFilesIntoBranches.  Only the\nbranches with modified files get processed, so the initialParents entry\ndoes not get removed on branches with no files to commit.\n\nThis change looks good to me.\n"}]}