Re: [PATCH 2/2] bundle-uri: do not abort on invalid packet line

From: Toon Claes <toon@iotcl.com>
Date: Sep 17, 2025, 08:40 UTC
Message-ID: <878qido4is.fsf@iotcl.com>
In-Reply-To: <yqyn5w6oq47lhrcbuziip5tajzrpylirswr5kyfyu35n3k7vgj@jn4rc7cwqwow>

Justin Tobler <jltobler@gmail.com> writes:

> Is there any reason that the server should be expected to invalid data
> to the client?

You mean "should be sending"? Well, at $DAYJOB we've had some unintentional misconfiguration that led to a similar failure[1]. I wasn't able to reproduce the exact same failure in test, because it requires to pass different configuration to the `GET info/refs` and `POST upload-pack` handlers, but the effect is the same.

[1]: https://gitlab.com/gitlab-org/git/-/issues/564

Show 5 quoted lines

> If the server is misconfigured, I wonder if it should
> instead handle this issue by not sending the invalid bundle-uri in the
> first place and printing a warning message on the server-side. From
> client perspective, if it's a server-side issue there may not be much
> they can do about the error and it could cause some confusion.

I can include a server-side fix in this series as well, but that doesn't hold back there might be servers out there in the field that don't have that fix and still serve invalid data. I think ideally we should fix it on both sides.

Show 22 quoted lines

>> Signed-off-by: Toon Claes <toon@iotcl.com>
>> ---
>>  connect.c                   |  4 ++--
>>  t/t5558-clone-bundle-uri.sh | 25 +++++++++++++++++++++++++
>>  2 files changed, 27 insertions(+), 2 deletions(-)
>> 
>> diff --git a/connect.c b/connect.c
>> index 8352b71faf..d2e2bd8cce 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -536,8 +536,8 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,
>>  		if (!bundle_uri_parse_line(bundles, line))
>>  			continue;
>>  
>> -		return error(_("error on bundle-uri response line %d: %s"),
>> -			     line_nr, line);
>> +		warning(_("ignore invalid bundle-uri response line %d: %s"),
>> +			    line_nr, line);
>
> If I'm understanding correctly, an error here indicates some sort of
> issue between the client and remote Git server while figuring out the
> bundle-uri capability.

> I think it is reasonable for the client to always
> expect the server to communicate in a way it understands and IMO should
> probably be handled by fixing the server-side instead.

Ideally yes, but broken servers exist now. Having clients deal with it properly is anyway advised in my opinion.

-- 
Cheers,
Toon