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

[Toon Claes <toon@iotcl.com>]

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

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


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