Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines
From: Toon Claes <hidden>
Date: 2026-06-24 07:49:16
Hi, My apologies for digging up this old thread, but it was suddenly brought back to my attention. Anyhow: Justin Tobler [off-list ref] writes:
I think it is questionable for a Git server to be sending clients malformed bundle-uri configuration.
I will not argue about that.
Do other Git implementations on the server-side exhibit this same behavior? If so, or we reasonably think they could and just want to be safe, then I agree that adjusting clients first to ignore invalid bundle-uri configuration from the server is reasonable. Generally, I'm of the mindset that when a server is sending malformed/garbage data that the client doesn't expect, the client should should be more strict and error out. In this case though, since there are known affected Git versions and bundle-uri is an optional feature to begin with, it probably doesn't hurt to be more permissive.
Yeah, that's the point I was trying to make. The use of bundle-uri is optional, and clone can continue without it. The code was intentionally written to continue when something goes wrong with bundle-uris. But because of some underlaying issue I was trying to fix with this patch, the process does not continue.
On 26/04/10 08:31AM, Patrick Steinhardt wrote:
quoted
That being said, I also think that we should fix the server side. Whether that needs to be part of this patch series though is a different question. Based on the proposed patch you posted it seems to be trivial enough though, so maybe it's worth it to just add that in as a second patch.Ya, my main concern was that a client-side fix would mask its root cause. As long as it gets addressed though it's fine. I think it would be worth adding to this series, but if not I'm happy to send a follow up patch to fix it too.
I do not fully agree. My fix doesn't make the issue go away silently, the user gets a warning message. I think this would cause (at least some) users to complain to the owner of the server (especially because bundle-URI is an opt-in feature). But I realize now, this warning isn't checked in the tests, adding that would have made that more clear. I do agree though a server-side fix would be advised. But I have no idea how to best address this. In a previous mail you wrote:
quoted hunk ↗ jump to hunk
Naively, I would assume the easiest way to fix the issue on the server-side would be the following:--- >8 --- diff --git a/bundle-uri.c b/bundle-uri.c index 3b2e347288..96d38bb80f 100644 --- a/bundle-uri.c +++ b/bundle-uri.c@@ -946,7 +946,7 @@ static int config_to_packet_line(const char *key, const char *value, { struct packet_reader *writer = data; - if (starts_with(key, "bundle.")) + if (starts_with(key, "bundle.") && value && *value) packet_write_fmt(writer->fd, "%s=%s", key, value); return 0; ---- >8 ---A quick check using the tests provided in this patch seems to show them passing with the above. If we want, we could also have the server print a warning on its end regarding the missing value too.
I don't like this fix, because it papers over the issue, silently. But then again, what is the best way to inform the server admin there's something wrong? Adding one line to the log files is easily to be missed. -- Cheers, Toon