{"thread":{"id":"61419","subject":"[PATCH] Bug fix: ensure P4 \"err\" is displayed when exception is raised.","startedAt":"2024-05-04T05:57:04Z","lastAt":"2024-05-09T15:30:42Z","messageCount":6,"participants":["Fahad Alrashed via GitGitGadget","Karthik Nayak","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"494074","messageId":"pull.1668.git.git.1714802221671.gitgitgadget@gmail.com","threadId":"61419","inReplyTo":null,"subject":"[PATCH] Bug fix: ensure P4 \"err\" is displayed when exception is raised.","fromName":"Fahad Alrashed via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-04T05:57:01Z","receivedAt":"2024-05-04T05:57:04Z","isPatch":true,"sender":{"key":"name:Fahad Alrashed","avatar":null},"body":"From: Fahad Alrashed <fahad@keylock.net>\n\nBug fix: During \"git p4 clone\" if p4 process returns\nan error from the server, it will store it in variable\n\"err\". The it will send a text command \"die-now\" to\ngit-fast-import. However, git-fast-import raises an\nexception: \"fatal: Unsupported command: die-now\"\nand err is never displayed. This patch ensures that\nerr is dispayed using \"finally:\"\n\nSigned-off-by: Fahad Alrashed <fahad@keylock.net>\n---\n    In git p4, git fast-import fails from die-now command and err (from\n    Perforce) is not shown\n    \n    When importing from Perforce using git p4 clone <depot location>,\n    cloning works fine until Perforce command p4 raises an error. This error\n    message is stored in err variable then git-fast-import is sent a die-now\n    command to kill it. An exception is raised fatal: Unsupported command:\n    die-now.\n    \n    This patch forces python to call die() with the err message returned\n    from Perforce.\n    \n    This commit fixes the root cause of a bug that took me hours to find.\n    I'm sure many faced the cryptic error and declared that git-p4 is not\n    working for them.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1668%2Falrashedf%2Fmaster-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1668/alrashedf/master-v1\nPull-Request: https://github.com/git/git/pull/1668\n\n git-p4.py | 24 +++++++++++++-----------\n 1 file changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 28ab12c72b6..f1ab31d5403 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -3253,17 +3253,19 @@ def streamP4FilesCb(self, marshalled):\n             if self.stream_have_file_info:\n                 if \"depotFile\" in self.stream_file:\n                     f = self.stream_file[\"depotFile\"]\n-            # force a failure in fast-import, else an empty\n-            # commit will be made\n-            self.gitStream.write(\"\\n\")\n-            self.gitStream.write(\"die-now\\n\")\n-            self.gitStream.close()\n-            # ignore errors, but make sure it exits first\n-            self.importProcess.wait()\n-            if f:\n-                die(\"Error from p4 print for %s: %s\" % (f, err))\n-            else:\n-                die(\"Error from p4 print: %s\" % err)\n+            try:\n+                # force a failure in fast-import, else an empty\n+                # commit will be made\n+                self.gitStream.write(\"\\n\")\n+                self.gitStream.write(\"die-now\\n\")\n+                self.gitStream.close()\n+                # ignore errors, but make sure it exits first\n+                self.importProcess.wait()\n+            finally:\n+                if f:\n+                    die(\"Error from p4 print for %s: %s\" % (f, err))\n+                else:\n+                    die(\"Error from p4 print: %s\" % err)\n \n         if 'depotFile' in marshalled and self.stream_have_file_info:\n             # start of a new file - output the old one first\n\nbase-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821\n-- \ngitgitgadget\n"},{"id":"494154","messageId":"CAOLa=ZQUEHO0oCcccgHQqzvgXkK-gMOqhpCQ2fGTLLMBPB+Pag@mail.gmail.com","threadId":"61419","inReplyTo":"pull.1668.git.git.1714802221671.gitgitgadget@gmail.com","subject":"Re: [PATCH] Bug fix: ensure P4 \"err\" is displayed when exception is raised.","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-05-06T12:01:45Z","receivedAt":"2024-05-06T12:01:47Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Hello Fahad,\n\n\"Fahad Alrashed via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Fahad Alrashed <fahad@keylock.net>\n>\n> Bug fix: During \"git p4 clone\" if p4 process returns\n\nNit: I haven't seen us use a prefix for the commit message. Also we use\na max line length of 72 characters (see .editorconfig). The commit\nmessage seems to be wrapped at a much lower threshold.\n\n> an error from the server, it will store it in variable\n\nperhaps `in the 'err' variable` or `in a variable 'err'`.\n\n> \"err\". The it will send a text command \"die-now\" to\n\ns/The/Then\n\n> git-fast-import. However, git-fast-import raises an\n> exception: \"fatal: Unsupported command: die-now\"\n> and err is never displayed. This patch ensures that\n> err is dispayed using \"finally:\"\n>\n> Signed-off-by: Fahad Alrashed <fahad@keylock.net>\n> ---\n>     In git p4, git fast-import fails from die-now command and err (from\n>     Perforce) is not shown\n>\n>     When importing from Perforce using git p4 clone <depot location>,\n>     cloning works fine until Perforce command p4 raises an error. This error\n>     message is stored in err variable then git-fast-import is sent a die-now\n>     command to kill it. An exception is raised fatal: Unsupported command:\n>     die-now.\n>\n>     This patch forces python to call die() with the err message returned\n>     from Perforce.\n>\n>     This commit fixes the root cause of a bug that took me hours to find.\n>     I'm sure many faced the cryptic error and declared that git-p4 is not\n>     working for them.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1668%2Falrashedf%2Fmaster-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1668/alrashedf/master-v1\n> Pull-Request: https://github.com/git/git/pull/1668\n>\n>  git-p4.py | 24 +++++++++++++-----------\n>  1 file changed, 13 insertions(+), 11 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 28ab12c72b6..f1ab31d5403 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -3253,17 +3253,19 @@ def streamP4FilesCb(self, marshalled):\n>              if self.stream_have_file_info:\n>                  if \"depotFile\" in self.stream_file:\n>                      f = self.stream_file[\"depotFile\"]\n> -            # force a failure in fast-import, else an empty\n> -            # commit will be made\n> -            self.gitStream.write(\"\\n\")\n> -            self.gitStream.write(\"die-now\\n\")\n> -            self.gitStream.close()\n> -            # ignore errors, but make sure it exits first\n> -            self.importProcess.wait()\n> -            if f:\n> -                die(\"Error from p4 print for %s: %s\" % (f, err))\n> -            else:\n> -                die(\"Error from p4 print: %s\" % err)\n> +            try:\n> +                # force a failure in fast-import, else an empty\n> +                # commit will be made\n> +                self.gitStream.write(\"\\n\")\n> +                self.gitStream.write(\"die-now\\n\")\n> +                self.gitStream.close()\n> +                # ignore errors, but make sure it exits first\n> +                self.importProcess.wait()\n> +            finally:\n> +                if f:\n> +                    die(\"Error from p4 print for %s: %s\" % (f, err))\n> +                else:\n> +                    die(\"Error from p4 print: %s\" % err)\n>\n\nThis part looks good.\n\n>          if 'depotFile' in marshalled and self.stream_have_file_info:\n>              # start of a new file - output the old one first\n>\n> base-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821\n> --\n> gitgitgadget\n\nThanks,\nKarthik\n"},{"id":"494312","messageId":"pull.1668.v2.git.git.1715168796873.gitgitgadget@gmail.com","threadId":"61419","inReplyTo":"pull.1668.git.git.1714802221671.gitgitgadget@gmail.com","subject":"[PATCH v2] Bug fix: ensure P4 \"err\" is displayed when exception is raised.","fromName":"Fahad Alrashed via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-08T11:46:36Z","receivedAt":"2024-05-08T11:46:41Z","isPatch":true,"sender":{"key":"name:Fahad Alrashed","avatar":null},"body":"From: Fahad Alrashed <fahad@keylock.net>\n\nDuring \"git p4 clone\" if p4 process returns an error from the server,\nit will store the message in the 'err' variable. Then it will send a\ntext command \"die-now\" to git-fast-import. However, git-fast-import\nraises an exception: \"fatal: Unsupported command: die-now\" and err is\nnever displayed. This patch ensures that err is dispayed using\n\"finally:\".\n\nSigned-off-by: Fahad Alrashed <fahad@keylock.net>\n---\n    In git p4, git fast-import fails from die-now command and err (from\n    Perforce) is not shown\n    \n    When importing from Perforce using git p4 clone <depot location>,\n    cloning works fine until Perforce command p4 raises an error. This error\n    message is stored in err variable then git-fast-import is sent a die-now\n    command to kill it. An exception is raised fatal: Unsupported command:\n    die-now.\n    \n    This patch forces python to call die() with the err message returned\n    from Perforce.\n    \n    This commit fixes the root cause of a bug that took me hours to find.\n    I'm sure many faced the cryptic error and declared that git-p4 is not\n    working for them.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1668%2Falrashedf%2Fmaster-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1668/alrashedf/master-v2\nPull-Request: https://github.com/git/git/pull/1668\n\nRange-diff vs v1:\n\n 1:  8dd2c730128 ! 1:  8d5b982bd08 Bug fix: ensure P4 \"err\" is displayed when exception is raised.\n     @@ Metadata\n       ## Commit message ##\n          Bug fix: ensure P4 \"err\" is displayed when exception is raised.\n      \n     -    Bug fix: During \"git p4 clone\" if p4 process returns\n     -    an error from the server, it will store it in variable\n     -    \"err\". The it will send a text command \"die-now\" to\n     -    git-fast-import. However, git-fast-import raises an\n     -    exception: \"fatal: Unsupported command: die-now\"\n     -    and err is never displayed. This patch ensures that\n     -    err is dispayed using \"finally:\"\n     +    During \"git p4 clone\" if p4 process returns an error from the server,\n     +    it will store the message in the 'err' variable. Then it will send a\n     +    text command \"die-now\" to git-fast-import. However, git-fast-import\n     +    raises an exception: \"fatal: Unsupported command: die-now\" and err is\n     +    never displayed. This patch ensures that err is dispayed using\n     +    \"finally:\".\n      \n          Signed-off-by: Fahad Alrashed <fahad@keylock.net>\n      \n\n\n git-p4.py | 24 +++++++++++++-----------\n 1 file changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 28ab12c72b6..f1ab31d5403 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -3253,17 +3253,19 @@ def streamP4FilesCb(self, marshalled):\n             if self.stream_have_file_info:\n                 if \"depotFile\" in self.stream_file:\n                     f = self.stream_file[\"depotFile\"]\n-            # force a failure in fast-import, else an empty\n-            # commit will be made\n-            self.gitStream.write(\"\\n\")\n-            self.gitStream.write(\"die-now\\n\")\n-            self.gitStream.close()\n-            # ignore errors, but make sure it exits first\n-            self.importProcess.wait()\n-            if f:\n-                die(\"Error from p4 print for %s: %s\" % (f, err))\n-            else:\n-                die(\"Error from p4 print: %s\" % err)\n+            try:\n+                # force a failure in fast-import, else an empty\n+                # commit will be made\n+                self.gitStream.write(\"\\n\")\n+                self.gitStream.write(\"die-now\\n\")\n+                self.gitStream.close()\n+                # ignore errors, but make sure it exits first\n+                self.importProcess.wait()\n+            finally:\n+                if f:\n+                    die(\"Error from p4 print for %s: %s\" % (f, err))\n+                else:\n+                    die(\"Error from p4 print: %s\" % err)\n \n         if 'depotFile' in marshalled and self.stream_have_file_info:\n             # start of a new file - output the old one first\n\nbase-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821\n-- \ngitgitgadget\n"},{"id":"494327","messageId":"xmqqo79gxn2p.fsf@gitster.g","threadId":"61419","inReplyTo":"pull.1668.v2.git.git.1715168796873.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] Bug fix: ensure P4 \"err\" is displayed when exception is raised.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-08T16:53:02Z","receivedAt":"2024-05-08T16:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Fahad Alrashed via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Fahad Alrashed <fahad@keylock.net>\n> Subject: Re: [PATCH v2] Bug fix: ensure P4 \"err\" is displayed when exception is raised.\n\nOur convention is to use the name of the area as the prefix, e.g.,\n\n\tgit-p4: show Perforce error to the user\n\nin the title.  Documentation/SubmittingPatches has more guidance on\nthe title, like omitting the full-stop at the end, and on the\nproposed log message in general.\n\n> During \"git p4 clone\" if p4 process returns an error from the server,\n> it will store the message in the 'err' variable. Then it will send a\n> text command \"die-now\" to git-fast-import. However, git-fast-import\n> raises an exception: \"fatal: Unsupported command: die-now\" and err is\n> never displayed.\n\nNicely explained.\n\n> This patch ensures that err is dispayed using\n> \"finally:\".\n\nInstead, we write it more like\n\n\tEnsure that err is shown to the end user.\n\nThe implementation (i.e., catching an error with try/finally) can be\nseen from the patch text.  What is more important to capture in the\nproposed log message is what _effect_ the commit wanted to make,\nwhich is a good way to show _why_ we are making the change.\n\nThe intent of the original code being to always die at this point.\nEven though it is necessary to enclose the earlier part up to\n.wait() inside a try block to catch an exception, the two calls to\ndie() do not have to be inside finally block (in other words,\nfinally block could be just a no-op, and the die() calls can be done\nafter the whole try/finally block that kills the fast-import by\nsending die-now and makes sure .wait() sees the process go), which\nmay have made the intent of the fix even more clear, which is \"we\nuse the try/finally construct only to work around an exception\nthrown by killing the fast-import process\".\n\nThe patch posted as-is is not wrong per-se, though.\n\nThanks.\n\n> diff --git a/git-p4.py b/git-p4.py\n> index 28ab12c72b6..f1ab31d5403 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -3253,17 +3253,19 @@ def streamP4FilesCb(self, marshalled):\n>              if self.stream_have_file_info:\n>                  if \"depotFile\" in self.stream_file:\n>                      f = self.stream_file[\"depotFile\"]\n> -            # force a failure in fast-import, else an empty\n> -            # commit will be made\n> -            self.gitStream.write(\"\\n\")\n> -            self.gitStream.write(\"die-now\\n\")\n> -            self.gitStream.close()\n> -            # ignore errors, but make sure it exits first\n> -            self.importProcess.wait()\n> -            if f:\n> -                die(\"Error from p4 print for %s: %s\" % (f, err))\n> -            else:\n> -                die(\"Error from p4 print: %s\" % err)\n> +            try:\n> +                # force a failure in fast-import, else an empty\n> +                # commit will be made\n> +                self.gitStream.write(\"\\n\")\n> +                self.gitStream.write(\"die-now\\n\")\n> +                self.gitStream.close()\n> +                # ignore errors, but make sure it exits first\n> +                self.importProcess.wait()\n> +            finally:\n> +                if f:\n> +                    die(\"Error from p4 print for %s: %s\" % (f, err))\n> +                else:\n> +                    die(\"Error from p4 print: %s\" % err)\n>  \n>          if 'depotFile' in marshalled and self.stream_have_file_info:\n>              # start of a new file - output the old one first\n>\n> base-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821\n"},{"id":"494333","messageId":"pull.1668.v3.git.git.1715206266214.gitgitgadget@gmail.com","threadId":"61419","inReplyTo":"pull.1668.v2.git.git.1715168796873.gitgitgadget@gmail.com","subject":"[PATCH v3] git-p4: show Perforce error to the user","fromName":"Fahad Alrashed via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-08T22:11:05Z","receivedAt":"2024-05-08T22:11:10Z","isPatch":true,"sender":{"key":"name:Fahad Alrashed","avatar":null},"body":"From: Fahad Alrashed <fahad@keylock.net>\n\nDuring \"git p4 clone\" if p4 process returns an error from the server,\nit will store the message in the 'err' variable. Then it will send a\ntext command \"die-now\" to git-fast-import. However, git-fast-import\nraises an exception: \"fatal: Unsupported command: die-now\" and err is\nnever displayed. This patch ensures that err is shown to the end user.\n\nSigned-off-by: Fahad Alrashed <fahad@keylock.net>\n---\n    In git p4, git fast-import fails from die-now command and err (from\n    Perforce) is not shown\n    \n    When importing from Perforce using git p4 clone <depot location>,\n    cloning works fine until Perforce command p4 raises an error. This error\n    message is stored in err variable then git-fast-import is sent a die-now\n    command to kill it. An exception is raised fatal: Unsupported command:\n    die-now.\n    \n    This patch forces python to call die() with the err message returned\n    from Perforce.\n    \n    This commit fixes the root cause of a bug that took me hours to find.\n    I'm sure many faced the cryptic error and declared that git-p4 is not\n    working for them.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1668%2Falrashedf%2Fmaster-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1668/alrashedf/master-v3\nPull-Request: https://github.com/git/git/pull/1668\n\nRange-diff vs v2:\n\n 1:  8d5b982bd08 ! 1:  16a0693b932 Bug fix: ensure P4 \"err\" is displayed when exception is raised.\n     @@ Metadata\n      Author: Fahad Alrashed <fahad@keylock.net>\n      \n       ## Commit message ##\n     -    Bug fix: ensure P4 \"err\" is displayed when exception is raised.\n     +    git-p4: show Perforce error to the user\n      \n          During \"git p4 clone\" if p4 process returns an error from the server,\n          it will store the message in the 'err' variable. Then it will send a\n          text command \"die-now\" to git-fast-import. However, git-fast-import\n          raises an exception: \"fatal: Unsupported command: die-now\" and err is\n     -    never displayed. This patch ensures that err is dispayed using\n     -    \"finally:\".\n     +    never displayed. This patch ensures that err is shown to the end user.\n      \n          Signed-off-by: Fahad Alrashed <fahad@keylock.net>\n      \n\n\n git-p4.py | 24 +++++++++++++-----------\n 1 file changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 28ab12c72b6..f1ab31d5403 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -3253,17 +3253,19 @@ def streamP4FilesCb(self, marshalled):\n             if self.stream_have_file_info:\n                 if \"depotFile\" in self.stream_file:\n                     f = self.stream_file[\"depotFile\"]\n-            # force a failure in fast-import, else an empty\n-            # commit will be made\n-            self.gitStream.write(\"\\n\")\n-            self.gitStream.write(\"die-now\\n\")\n-            self.gitStream.close()\n-            # ignore errors, but make sure it exits first\n-            self.importProcess.wait()\n-            if f:\n-                die(\"Error from p4 print for %s: %s\" % (f, err))\n-            else:\n-                die(\"Error from p4 print: %s\" % err)\n+            try:\n+                # force a failure in fast-import, else an empty\n+                # commit will be made\n+                self.gitStream.write(\"\\n\")\n+                self.gitStream.write(\"die-now\\n\")\n+                self.gitStream.close()\n+                # ignore errors, but make sure it exits first\n+                self.importProcess.wait()\n+            finally:\n+                if f:\n+                    die(\"Error from p4 print for %s: %s\" % (f, err))\n+                else:\n+                    die(\"Error from p4 print: %s\" % err)\n \n         if 'depotFile' in marshalled and self.stream_have_file_info:\n             # start of a new file - output the old one first\n\nbase-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821\n-- \ngitgitgadget\n"},{"id":"494351","messageId":"xmqqcypvvw80.fsf@gitster.g","threadId":"61419","inReplyTo":"pull.1668.v3.git.git.1715206266214.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] git-p4: show Perforce error to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-09T15:30:39Z","receivedAt":"2024-05-09T15:30:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Fahad Alrashed via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Fahad Alrashed <fahad@keylock.net>\n>\n> During \"git p4 clone\" if p4 process returns an error from the server,\n> it will store the message in the 'err' variable. Then it will send a\n> text command \"die-now\" to git-fast-import. However, git-fast-import\n> raises an exception: \"fatal: Unsupported command: die-now\" and err is\n> never displayed. This patch ensures that err is shown to the end user.\n>\n> Signed-off-by: Fahad Alrashed <fahad@keylock.net>\n> ---\n\nLooking good.  Queued.  Thanks.\n"}]}