{"thread":{"id":"60545","subject":"[PATCH] git-p4: fix fast import when empty commit is first","startedAt":"2023-11-22T07:56:07Z","lastAt":"2023-11-24T06:55:32Z","messageCount":4,"participants":["Alisha Kim via GitGitGadget","Junio C Hamano","Alisha Kim"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"485047","messageId":"pull.1609.git.git.1700639764041.gitgitgadget@gmail.com","threadId":"60545","inReplyTo":null,"subject":"[PATCH] git-p4: fix fast import when empty commit is first","fromName":"Alisha Kim via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-11-22T07:56:03Z","receivedAt":"2023-11-22T07:56:07Z","isPatch":true,"sender":{"key":"admin@pril.cc","avatar":"https://gravatar.com/avatar/974cb2fe46bc221b3ae915262ea0275c3edd03cbab792a2657e70cc232274bf5?d=mp&s=160"},"body":"From: Alisha Kim <pril@pril.cc>\n\nWhen executing p4 sync by specifying an excluded path, an empty commit\nwill be created if there is only a change in the excluded path in\nrevision.\nIf git-p4.keepEmptyCommits is turned off and an empty commit is the\nfirst, fast-import will fail.\n\nSigned-off-by: Alisha Kim <pril@pril.cc>\n---\n    git-p4: fix fast import when empty commit is first\n    \n    When executing p4 sync by specifying an excluded path, an empty commit\n    will be created if there is only a change in the excluded path in\n    revision. If git-p4.keepEmptyCommits is turned off and an empty commit\n    is the first, fast-import will fail.\n    \n    The error log is as follows Ignoring revision 14035 as it would produce\n    an empty commit. fast-import failed: warning: Not updating\n    refs/heads/p4/master (new tip new commit hash does not contain parent\n    commit hash) fast-import statistics: ...\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1609%2Fdaebo01%2Fgit-p4-pr-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1609/daebo01/git-p4-pr-v1\nPull-Request: https://github.com/git/git/pull/1609\n\n git-p4.py | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 0eb3bb4c47d..a61200e29e4 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -3466,7 +3466,7 @@ class P4Sync(Command, P4UserMap):\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@@ -3533,6 +3533,8 @@ class P4Sync(Command, P4UserMap):\n                     print(\"Tag %s does not match with change %s: file count is different.\"\n                            % (labelDetails[\"label\"], change))\n \n+        return True\n+\n     def getLabels(self):\n         \"\"\"Build a dictionary of changelists and labels, for \"detect-labels\"\n            option.\n@@ -3876,10 +3878,12 @@ class P4Sync(Command, P4UserMap):\n                             self.commit(description, filesForCommit, branch, parent)\n                 else:\n                     files = self.extractFilesFromCommit(description)\n-                    self.commit(description, files, self.branch,\n+                    isCommitted = self.commit(description, files, self.branch,\n                                 self.initialParent)\n                     # only needed once, to connect to the previous commit\n-                    self.initialParent = \"\"\n+                    if isCommitted:\n+                        self.initialParent = \"\"\n+\n             except IOError:\n                 print(self.gitError.read())\n                 sys.exit(1)\n\nbase-commit: cfb8a6e9a93adbe81efca66e6110c9b4d2e57169\n-- \ngitgitgadget\n"},{"id":"485057","messageId":"xmqq1qchcjtq.fsf@gitster.g","threadId":"60545","inReplyTo":"pull.1609.git.git.1700639764041.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-p4: fix fast import when empty commit is first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-23T01:57:53Z","receivedAt":"2023-11-23T01:57:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alisha Kim via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Alisha Kim <pril@pril.cc>\n>\n> When executing p4 sync by specifying an excluded path, an empty commit\n> will be created if there is only a change in the excluded path in\n> revision.\n> If git-p4.keepEmptyCommits is turned off and an empty commit is the\n> first, fast-import will fail.\n\nThe above describe under what condition a failure gets triggered,\nbut there is no description on what approach the proposed solution\ntakes.  You could teach \"fast-import\" to deal with an empty commit\nproperly, you could ignore empty commits and not produce input for\nthe fast-import command, you could probably turn these initial empty\ncommits into non-empty commits by adding dummy contents, etc.  We\nwant to see in our proposed log messages what solution was taken and\nhow the solution was designed to satisfy what requirements.  This is\nto help future developers who will have to change the code that is\ngiven by this patch, so that their updates can still adhere to what\never design criteria you had in working on this change [*].\n\n    Side note: Your solution might be to ignore empty commits\n    despite keepEmptyCommits option is set (as I said, you did not\n    describe it at all in the above, so this is a hypothetical\n    example).  If the reason behind choosing that design were \"I\n    just do not want it to fail---I do not care if the resulting\n    history coming out of fast-import is crappy (I lose the p4 CL\n    descriptions for these commits, even though the user wants to\n    keep them)\", then future developers can safely \"fix\" your fix\n    here by turning the initial empty commits into non-empty ones by\n    adding fake contents.\n\n> @@ -3876,10 +3878,12 @@ class P4Sync(Command, P4UserMap):\n>                              self.commit(description, filesForCommit, branch, parent)\n>                  else:\n>                      files = self.extractFilesFromCommit(description)\n> -                    self.commit(description, files, self.branch,\n> +                    isCommitted = self.commit(description, files, self.branch,\n>                                  self.initialParent)\n>                      # only needed once, to connect to the previous commit\n> -                    self.initialParent = \"\"\n> +                    if isCommitted:\n> +                        self.initialParent = \"\"\n\n\"is\" does not sound grammatically correct.  \"didCommit\" (i.e., \"we\nmade a commit\"), \"haveCommitted\" (i.e., \"we have made a commit\")\nmight be more understandable.\n\n>              except IOError:\n>                  print(self.gitError.read())\n>                  sys.exit(1)\n>\n> base-commit: cfb8a6e9a93adbe81efca66e6110c9b4d2e57169\n"},{"id":"485098","messageId":"pull.1609.v2.git.git.1700806464802.gitgitgadget@gmail.com","threadId":"60545","inReplyTo":"pull.1609.git.git.1700639764041.gitgitgadget@gmail.com","subject":"[PATCH v2] git-p4: fix fast import when empty commit is first","fromName":"Alisha Kim via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-11-24T06:14:24Z","receivedAt":"2023-11-24T06:14:28Z","isPatch":true,"sender":{"key":"admin@pril.cc","avatar":"https://gravatar.com/avatar/974cb2fe46bc221b3ae915262ea0275c3edd03cbab792a2657e70cc232274bf5?d=mp&s=160"},"body":"From: Alisha Kim <pril@pril.cc>\n\nWhen executing p4 sync by specifying an excluded path, an empty commit\nwill be created if there is only a change in the excluded path in\nrevision.\nIf git-p4.keepEmptyCommits is turned off and an empty commit is the\nfirst, fast-import will fail. Change the return type of the commit\nfunction from void to bool and return whether a commit has been\ncreated. This failure was prevented by modifying initialParent\nto be initialized only when a commit was actually created.\n\nSigned-off-by: Alisha Kim <pril@pril.cc>\n---\n    git-p4: fix fast import when empty commit is first\n    \n    When executing p4 sync by specifying an excluded path, an empty commit\n    will be created if there is only a change in the excluded path in\n    revision. If git-p4.keepEmptyCommits is turned off and an empty commit\n    is the first, fast-import will fail. Change the return type of the\n    commit function from void to bool and return whether a commit has been\n    created. This failure was prevented by modifying initialParent to be\n    initialized only when a commit was actually created.\n    \n    The error log is as follows Ignoring revision 14035 as it would produce\n    an empty commit. fast-import failed: warning: Not updating\n    refs/heads/p4/master (new tip new commit hash does not contain parent\n    commit hash) fast-import statistics: ...\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1609%2Fdaebo01%2Fgit-p4-pr-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1609/daebo01/git-p4-pr-v2\nPull-Request: https://github.com/git/git/pull/1609\n\nRange-diff vs v1:\n\n 1:  f7c4fa18c4c ! 1:  1de9ac6dbf8 git-p4: fix fast import when empty commit is first\n     @@ Commit message\n          will be created if there is only a change in the excluded path in\n          revision.\n          If git-p4.keepEmptyCommits is turned off and an empty commit is the\n     -    first, fast-import will fail.\n     +    first, fast-import will fail. Change the return type of the commit\n     +    function from void to bool and return whether a commit has been\n     +    created. This failure was prevented by modifying initialParent\n     +    to be initialized only when a commit was actually created.\n      \n          Signed-off-by: Alisha Kim <pril@pril.cc>\n      \n     @@ git-p4.py: class P4Sync(Command, P4UserMap):\n                       else:\n                           files = self.extractFilesFromCommit(description)\n      -                    self.commit(description, files, self.branch,\n     -+                    isCommitted = self.commit(description, files, self.branch,\n     ++                    haveCommitted = self.commit(description, files, self.branch,\n                                       self.initialParent)\n                           # only needed once, to connect to the previous commit\n      -                    self.initialParent = \"\"\n     -+                    if isCommitted:\n     ++                    if haveCommitted:\n      +                        self.initialParent = \"\"\n      +\n                   except IOError:\n\n\n git-p4.py | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 0eb3bb4c47d..1e3c0e815f0 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -3466,7 +3466,7 @@ class P4Sync(Command, P4UserMap):\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@@ -3533,6 +3533,8 @@ class P4Sync(Command, P4UserMap):\n                     print(\"Tag %s does not match with change %s: file count is different.\"\n                            % (labelDetails[\"label\"], change))\n \n+        return True\n+\n     def getLabels(self):\n         \"\"\"Build a dictionary of changelists and labels, for \"detect-labels\"\n            option.\n@@ -3876,10 +3878,12 @@ class P4Sync(Command, P4UserMap):\n                             self.commit(description, filesForCommit, branch, parent)\n                 else:\n                     files = self.extractFilesFromCommit(description)\n-                    self.commit(description, files, self.branch,\n+                    haveCommitted = self.commit(description, files, self.branch,\n                                 self.initialParent)\n                     # only needed once, to connect to the previous commit\n-                    self.initialParent = \"\"\n+                    if haveCommitted:\n+                        self.initialParent = \"\"\n+\n             except IOError:\n                 print(self.gitError.read())\n                 sys.exit(1)\n\nbase-commit: cfb8a6e9a93adbe81efca66e6110c9b4d2e57169\n-- \ngitgitgadget\n"},{"id":"485099","messageId":"CAB5P87v-S6ODcog118t7ONkW6Y8ZUDOnNZ4C5UiH6d5Fq=3UCQ@mail.gmail.com","threadId":"60545","inReplyTo":"xmqq1qchcjtq.fsf@gitster.g","subject":"Re: [PATCH] git-p4: fix fast import when empty commit is first","fromName":"Alisha Kim","fromEmail":"admin@pril.cc","sentAt":"2023-11-24T06:55:21Z","receivedAt":"2023-11-24T06:55:32Z","isPatch":true,"sender":{"key":"admin@pril.cc","avatar":"https://gravatar.com/avatar/974cb2fe46bc221b3ae915262ea0275c3edd03cbab792a2657e70cc232274bf5?d=mp&s=160"},"body":"On Thu, Nov 23, 2023 at 10:57 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Alisha Kim via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Alisha Kim <pril@pril.cc>\n> >\n> > When executing p4 sync by specifying an excluded path, an empty commit\n> > will be created if there is only a change in the excluded path in\n> > revision.\n> > If git-p4.keepEmptyCommits is turned off and an empty commit is the\n> > first, fast-import will fail.\n>\n> The above describe under what condition a failure gets triggered,\n> but there is no description on what approach the proposed solution\n> takes.  You could teach \"fast-import\" to deal with an empty commit\n> properly, you could ignore empty commits and not produce input for\n> the fast-import command, you could probably turn these initial empty\n> commits into non-empty commits by adding dummy contents, etc.  We\n> want to see in our proposed log messages what solution was taken and\n> how the solution was designed to satisfy what requirements.  This is\n> to help future developers who will have to change the code that is\n> given by this patch, so that their updates can still adhere to what\n> ever design criteria you had in working on this change [*].\n>\n>     Side note: Your solution might be to ignore empty commits\n>     despite keepEmptyCommits option is set (as I said, you did not\n>     describe it at all in the above, so this is a hypothetical\n>     example).  If the reason behind choosing that design were \"I\n>     just do not want it to fail---I do not care if the resulting\n>     history coming out of fast-import is crappy (I lose the p4 CL\n>     descriptions for these commits, even though the user wants to\n>     keep them)\", then future developers can safely \"fix\" your fix\n>     here by turning the initial empty commits into non-empty ones by\n>     adding fake contents.\n\n\nI've added explanations for the changes I suggested.\n\n>\n>\n> > @@ -3876,10 +3878,12 @@ class P4Sync(Command, P4UserMap):\n> >                              self.commit(description, filesForCommit, branch, parent)\n> >                  else:\n> >                      files = self.extractFilesFromCommit(description)\n> > -                    self.commit(description, files, self.branch,\n> > +                    isCommitted = self.commit(description, files, self.branch,\n> >                                  self.initialParent)\n> >                      # only needed once, to connect to the previous commit\n> > -                    self.initialParent = \"\"\n> > +                    if isCommitted:\n> > +                        self.initialParent = \"\"\n>\n> \"is\" does not sound grammatically correct.  \"didCommit\" (i.e., \"we\n> made a commit\"), \"haveCommitted\" (i.e., \"we have made a commit\")\n> might be more understandable.\n\n\nThe problem with correct grammar that you mentioned has also been\nchanged. Thank you for your good comments.\n\n\n>\n>\n> >              except IOError:\n> >                  print(self.gitError.read())\n> >                  sys.exit(1)\n> >\n> > base-commit: cfb8a6e9a93adbe81efca66e6110c9b4d2e57169\n"}]}