Re: [PATCH 2/2] bundle-uri: do not abort on invalid packet line
On 25/09/17 10:40AM, Toon Claes wrote:
Show 12 quoted lines
> Justin Tobler <jltobler@gmail.com> writes:
>
> > 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.
From my perspective, a server-side fix is the most important as it addresses the root of the problem. That is, a remote Git server with misconfigured bundle-uri is communicating with a client in an invalid manner.
We could address this by updating the client to simply ignore the miscommunicating server. I'm hesitant though because this feels like a slightly different class of issue here being that, from my understanding, the server is not correctly negotiating the capability with the client. Maybe it is fine though for clients to ignore errors in this process?
I think I would be more inclined to agree with a client-side fix if the issue was more pronounced in practice. From my understanding, for this to occur:
1. A remote Git server must be advertising bundle-uri.
2. The bundle-uri must be misconfigured on the server-side.
3. The client must request the bundle-uri from the server via
transfer.bundleURI.
A user can workaround this problem simply by disabling bundle-uri. I suspect there are not many users in this situation though.
Show 30 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.Another benefit of addressing this on the server-side is that it should transparently fix the issue for clients. I would like to think server providers using the bundle-uri feature would be more inclined than clients to update quickly and address the problem. Regardless, I still think the issue is niche enough not to be a huge problem for clients.
IMO, I think a server-side fix should come first. If this step isn't enough, then it may make sense to also address it on the client side.
-Justin
Toon Claes
