Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines

[Toon Claes <toon@iotcl.com>]

Hi,

My apologies for digging up this old thread, but it was suddenly brought
back to my attention. Anyhow:

Justin Tobler <jltobler@gmail.com> 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:

>> 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:

> 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