On Sun, Sep 14, 2025 at 10:23:01PM -0400, Jeff King wrote:
> - could upload-pack install a die() handler that prints the message in
> an ERR packet? I worry a little that older versions of Git would not
> handle this great, as I don't think they were always prepared to see
> an ERR packet at any point. OTOH, it is probably better than sending
> nothing, which is what we do now.
Just for fun, I tried the patch below on v2.39.5:
diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index f446ff04f6..ad40143beb 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -13,6 +13,21 @@ static const char * const upload_pack_usage[] = {
NULL
};
+NORETURN
+static void send_err_pkt_on_die(const char *fmt, va_list ap)
+{
+ struct strbuf buf = STRBUF_INIT;
+
+ /* format into a buf since interfaces below do not handle va_list */
+ strbuf_vaddf(&buf, fmt, ap);
+
+ /* write our ERR packet */
+ packet_write_fmt_gently(1, "ERR %s", buf.buf);
+
+ /* and then do the usual die to stderr */
+ exit(die_message("%s", buf.buf));
+}
+
int cmd_upload_pack(int argc, const char **argv, const char *prefix)
{
const char *dir;
@@ -38,6 +53,8 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
/* TODO: This should use NO_LAZY_FETCH_ENVIRONMENT */
xsetenv("GIT_NO_LAZY_FETCH", "1", 0);
+ set_die_routine(send_err_pkt_on_die);
+
argc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0);
if (argc != 1)
The results are...not great. You get every message twice, of course,
because we still print it to stderr. Though that could easily be fixed.
But for the multi-line message in question, the "remote error" part is
hard to see amidst the other lines:
Cloning into 'foo'...
fatal: detected dubious ownership in repository at '/tmp/foo.git'
To add an exception for this directory, call:
git config --global --add safe.directory /tmp/foo.git
fatal: remote error: detected dubious ownership in repository at '/tmp/foo.git'
To add an exception for this directory, call:
git config --global --add safe.directory /tmp/foo.git
Probably it would help to look for newlines and prefix every line with
"remote: or similar. But an even bigger problem is that we die
immediately on seeing the remote ERR packet, so we miss out on any local
error messages. An obvious one is trying to clone something that doesn't
exist at all. We used to say:
Cloning into 'does-not-exist'...
fatal: '/tmp/does-not-exist.git' does not appear to be a git repository
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
Noting that we saw an error from the remote side and giving some hints.
And the stderr from the other side is enough to give us the more
specific message (though again, over http the user would not get that
stderr message; however, if we get a 404 we do show a useful message).
But with the patch above we just relay what the other side says:
Cloning into 'does-not-exist'...
fatal: '/tmp/does-not-exist.git' does not appear to be a git repository
fatal: remote error: '/tmp/does-not-exist.git' does not appear to be a git repository
which seems worse to me.
So probably not a very productive direction. Oh well.
-Peff
