| Message ID | 20181229211915.161686-1-masayasuzuki@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Accept error packets in any context | expand |
Masaya Suzuki <masayasuzuki@google.com> writes: > This makes it possible for servers to send an error message back to clients in > an arbitrary situation. > > The first patch was originally sent in [1]. This version includes some fix. > > The second patch was originally sent in [2]. Later, this was cherry-picked in > [3]. In the discussion in [3], we agreed that this error packet handling should > be done only against the Git pack protocol handling code. With this agreement, > the patch series sent in [3] is abandoned (according to [4]). This is a patch > series based on that agreement on limiting the error packet handling. In short, are you shooting js/smart-http-detect-remote-error topic down and replacing it with this one? As that topic is not yet in 'next', I am perfectly fine doing that. I just want to make sure that is what you meant, as my reading of [4] was a bit fuzzy. Thanks. > > [1]: https://public-inbox.org/git/20181227065210.60817-1-masayasuzuki@google.com/ > [2]: https://public-inbox.org/git/20181127045301.103807-1-masayasuzuki@google.com/ > [3]: https://public-inbox.org/git/df7d3659ae5f11d163f1e992f3b9403be709ddb7.1544572142.git.steadmon@google.com/ > [4]: https://public-inbox.org/git/20181213221826.GE37614@google.com/ > > Masaya Suzuki (2): > Use packet_reader instead of packet_read_line > pack-protocol.txt: accept error packets in any context
On Thu, Jan 3, 2019 at 3:05 PM Junio C Hamano <gitster@pobox.com> wrote: > > Masaya Suzuki <masayasuzuki@google.com> writes: > > > This makes it possible for servers to send an error message back to clients in > > an arbitrary situation. > > > > The first patch was originally sent in [1]. This version includes some fix. > > > > The second patch was originally sent in [2]. Later, this was cherry-picked in > > [3]. In the discussion in [3], we agreed that this error packet handling should > > be done only against the Git pack protocol handling code. With this agreement, > > the patch series sent in [3] is abandoned (according to [4]). This is a patch > > series based on that agreement on limiting the error packet handling. > > In short, are you shooting js/smart-http-detect-remote-error topic > down and replacing it with this one? > > As that topic is not yet in 'next', I am perfectly fine doing that. > I just want to make sure that is what you meant, as my reading of > [4] was a bit fuzzy. Sorry, I think I referenced a wrong email. [4] was meant to be https://public-inbox.org/git/20181219233005.GI37614@google.com/. I think he wants to have https://public-inbox.org/git/20181116084725.GA31603@sigill.intra.peff.net/, https://public-inbox.org/git/20181116084838.GB31603@sigill.intra.peff.net/, and https://public-inbox.org/git/20181116084951.GC31603@sigill.intra.peff.net/ for js/smart-http-detect-remote-error.
Hi, Junio C Hamano wrote: > Masaya Suzuki <masayasuzuki@google.com> writes: >> This makes it possible for servers to send an error message back to clients in >> an arbitrary situation. Yay! Yes, this should simplify server implementations and user support. [...] > In short, are you shooting js/smart-http-detect-remote-error topic > down and replacing it with this one? > > As that topic is not yet in 'next', I am perfectly fine doing that. > I just want to make sure that is what you meant, as my reading of > [4] was a bit fuzzy. Josh, looking at that branch, I see: remote-curl: die on server-side errors Includes a test illustrating error handling in the ref advertisement. Should that be revived as a standalone patch, without the remote-curl.c change? remote-curl: tighten "version 2" check for smart-http A bug fix. We had an analogous bug in the .googlesource.com servers and it was problematic when experimenting with protocol changes using placeholder version numbers YYYYmmdd (since that starts with a "2"). remote-curl: refactor smart-http discovery A nice cleanup. Thanks, Jonathan
Jonathan Nieder wrote: > Junio C Hamano wrote: >> In short, are you shooting js/smart-http-detect-remote-error topic >> down and replacing it with this one? >> >> As that topic is not yet in 'next', I am perfectly fine doing that. >> I just want to make sure that is what you meant, as my reading of >> [4] was a bit fuzzy. > > Josh, looking at that branch, I see: > > remote-curl: die on server-side errors > > Includes a test illustrating error handling in the ref > advertisement. Should that be revived as a standalone patch, > without the remote-curl.c change? In fact, it appears I have that locally: commit 7fe6abcd775dbffd919891d5838f8d125e41f160 Author: Josh Steadmon <steadmon@google.com> Date: Tue Dec 11 16:25:18 2018 -0800 lib-httpd, t5551: check server-side HTTP errors Add an HTTP path to the test server config that returns an ERR pkt-line unconditionally. Verify in t5551 that remote-curl properly detects this ERR message and aborts. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> It's handled fine by the merge 7be333a6362882e8ffceef3de830dbbfafe99995 (Merge branch 'js/smart-http-detect-remote-error' into pu, 2019-01-11), though. So I think what is in "pu" is okay, without shooting any series down. (Alternatively we can combine them into a single five-patch series, if the maintainer prefers.) Thanks, Jonathan
Jonathan Nieder <jrnieder@gmail.com> writes: > It's handled fine by the merge 7be333a6362882e8ffceef3de830dbbfafe99995 > (Merge branch 'js/smart-http-detect-remote-error' into pu, 2019-01-11), > though. So I think what is in "pu" is okay, without shooting any series > down. Yeah, I think I wiggled that topic branch after getting a clarification from Masaya on what is going on, so what's on 'pu' should be in a good shape.