From: Junio C Hamano <gitster@pobox.com>
To: "Fahad Alrashed via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Karthik Nayak <karthik.188@gmail.com>,
Fahad Alrashed <fahad@keylock.net>
Subject: Re: [PATCH v2] Bug fix: ensure P4 "err" is displayed when exception is raised.
Date: Wed, 08 May 2024 09:53:02 -0700 [thread overview]
Message-ID: <xmqqo79gxn2p.fsf@gitster.g> (raw)
In-Reply-To: <pull.1668.v2.git.git.1715168796873.gitgitgadget@gmail.com> (Fahad Alrashed via GitGitGadget's message of "Wed, 08 May 2024 11:46:36 +0000")
"Fahad Alrashed via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Fahad Alrashed <fahad@keylock.net>
> Subject: Re: [PATCH v2] Bug fix: ensure P4 "err" is displayed when exception is raised.
Our convention is to use the name of the area as the prefix, e.g.,
git-p4: show Perforce error to the user
in the title. Documentation/SubmittingPatches has more guidance on
the title, like omitting the full-stop at the end, and on the
proposed log message in general.
> During "git p4 clone" if p4 process returns an error from the server,
> it will store the message in the 'err' variable. Then it will send a
> text command "die-now" to git-fast-import. However, git-fast-import
> raises an exception: "fatal: Unsupported command: die-now" and err is
> never displayed.
Nicely explained.
> This patch ensures that err is dispayed using
> "finally:".
Instead, we write it more like
Ensure that err is shown to the end user.
The implementation (i.e., catching an error with try/finally) can be
seen from the patch text. What is more important to capture in the
proposed log message is what _effect_ the commit wanted to make,
which is a good way to show _why_ we are making the change.
The intent of the original code being to always die at this point.
Even though it is necessary to enclose the earlier part up to
.wait() inside a try block to catch an exception, the two calls to
die() do not have to be inside finally block (in other words,
finally block could be just a no-op, and the die() calls can be done
after the whole try/finally block that kills the fast-import by
sending die-now and makes sure .wait() sees the process go), which
may have made the intent of the fix even more clear, which is "we
use the try/finally construct only to work around an exception
thrown by killing the fast-import process".
The patch posted as-is is not wrong per-se, though.
Thanks.
> diff --git a/git-p4.py b/git-p4.py
> index 28ab12c72b6..f1ab31d5403 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -3253,17 +3253,19 @@ def streamP4FilesCb(self, marshalled):
> if self.stream_have_file_info:
> if "depotFile" in self.stream_file:
> f = self.stream_file["depotFile"]
> - # force a failure in fast-import, else an empty
> - # commit will be made
> - self.gitStream.write("\n")
> - self.gitStream.write("die-now\n")
> - self.gitStream.close()
> - # ignore errors, but make sure it exits first
> - self.importProcess.wait()
> - if f:
> - die("Error from p4 print for %s: %s" % (f, err))
> - else:
> - die("Error from p4 print: %s" % err)
> + try:
> + # force a failure in fast-import, else an empty
> + # commit will be made
> + self.gitStream.write("\n")
> + self.gitStream.write("die-now\n")
> + self.gitStream.close()
> + # ignore errors, but make sure it exits first
> + self.importProcess.wait()
> + finally:
> + if f:
> + die("Error from p4 print for %s: %s" % (f, err))
> + else:
> + die("Error from p4 print: %s" % err)
>
> if 'depotFile' in marshalled and self.stream_have_file_info:
> # start of a new file - output the old one first
>
> base-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821
next prev parent reply other threads:[~2024-05-08 16:53 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-04 5:57 [PATCH] Bug fix: ensure P4 "err" is displayed when exception is raised Fahad Alrashed via GitGitGadget
2024-05-06 12:01 ` Karthik Nayak
2024-05-08 11:46 ` [PATCH v2] " Fahad Alrashed via GitGitGadget
2024-05-08 16:53 ` Junio C Hamano [this message]
2024-05-08 22:11 ` [PATCH v3] git-p4: show Perforce error to the user Fahad Alrashed via GitGitGadget
2024-05-09 15:30 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqo79gxn2p.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=fahad@keylock.net \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=karthik.188@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.