| Message ID | 1604029402-19593-1-git-send-email-dan@mutual.io (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | upload-pack: allow stateless client EOF just prior to haves | expand |
Daniel Duvall <dan@mutual.io> writes: > diff --git a/upload-pack.c b/upload-pack.c > index 3b858eb..2b128e4 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -1344,7 +1344,18 @@ void upload_pack(struct upload_pack_options *options) > PACKET_READ_DIE_ON_ERR_PACKET); > > receive_needs(&data, &reader); > - if (data.want_obj.nr) { > + > + /* > + * An EOF at this exact point in negotiation should be > + * acceptable from stateless clients as they will consume the > + * shallow list before doing subsequent rpc with haves/etc. > + */ > + if (data.stateless_rpc) > + reader.options |= PACKET_READ_GENTLE_ON_EOF; > + > + if (data.want_obj.nr && > + packet_reader_peek(&reader) != PACKET_READ_EOF) { > + reader.options ^= PACKET_READ_GENTLE_ON_EOF; I am a bit puzzled why it is sensible to (1) unconditionally (2) toggle the GENTLE bit. > get_common_commits(&data, &reader); > create_pack_file(&data, NULL); > } If we are not doing stateless_rpc, we call get_common_commits() with a reader that is gentle on eof, which is not an intended behaviour change, no? I would have understood if this were more like if (data.stateless_rpc) reader.options |= PACKET_READ_GENTLE_ON_EOF; if (data.want_obj.nr && packet_reader_peek(&reader) != PACKET_READ_EOF) { if (data.stateless_rpc) reader.options &= ~PACKET_READ_GENTLE_ON_EOF; i.e. only when we know we set the bit when the bit was originally clear, revert to the original state. Note. initially I thought we may need to check the original value of the bit in reader.options before flipping it on, but this packet_reader has freshly been initialized in the inner block we see here, so we know that nobody other than this new code would have set the bit. Or for that matter, just unconditionally turn it off, e.g. if (data.want_obj.nr && packet_reader_peek(&reader) != PACKET_READ_EOF) { reader.options &= ~PACKET_READ_GENTLE_ON_EOF; Puzzled...
On Fri, Oct 30, 2020 at 11:42 AM Junio C Hamano <gitster@pobox.com> wrote: > I am a bit puzzled why it is sensible to > > (1) unconditionally > (2) toggle > > the GENTLE bit. That was pure carelessness on my part in flipping it instead of using `&= ~`. Fixed in v2
diff --git a/t/t9904-upload-pack-stateless-timely-eof.sh b/t/t9904-upload-pack-stateless-timely-eof.sh new file mode 100755 index 0000000..9a116c6 --- /dev/null +++ b/t/t9904-upload-pack-stateless-timely-eof.sh @@ -0,0 +1,22 @@ +#!/bin/sh + +test_description='stateless upload-pack gently handles EOF just after want/shallow/depth/flush' + +. ./test-lib.sh + +test_expect_success 'upload-pack outputs flush and exits ok' ' + test_commit initial && + head=$(git rev-parse HEAD) && + hexsz=$(test_oid hexsz) && + + printf "%04xwant %s\n%04xshallow %s\n000ddeepen 1\n0000" \ + $(($hexsz + 10)) $head $(($hexsz + 13)) $head >request && + + git upload-pack --stateless-rpc "$(pwd)" <request >actual && + + printf "0000" >expect && + + test_cmp expect actual +' + +test_done diff --git a/upload-pack.c b/upload-pack.c index 3b858eb..2b128e4 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1344,7 +1344,18 @@ void upload_pack(struct upload_pack_options *options) PACKET_READ_DIE_ON_ERR_PACKET); receive_needs(&data, &reader); - if (data.want_obj.nr) { + + /* + * An EOF at this exact point in negotiation should be + * acceptable from stateless clients as they will consume the + * shallow list before doing subsequent rpc with haves/etc. + */ + if (data.stateless_rpc) + reader.options |= PACKET_READ_GENTLE_ON_EOF; + + if (data.want_obj.nr && + packet_reader_peek(&reader) != PACKET_READ_EOF) { + reader.options ^= PACKET_READ_GENTLE_ON_EOF; get_common_commits(&data, &reader); create_pack_file(&data, NULL); }
During stateless packfile negotiation, it is normal behavior for stateless RPC clients (e.g. git-remote-curl) to send multiple upload-pack requests with the first containing only the wants/shallows/deepens/filters followed by a flush. When run in stateless mode, continuing on without first checking that the client request has reached EOF can result in a bad file descriptor during get_common_commits. Instead, upload-pack should gently peek for an EOF between the sending of shallow/unshallow lines (followed by flush) and the reading of client haves. If the client has hung up at this point, exit normally. Signed-off-by: Daniel Duvall <dan@mutual.io> --- t/t9904-upload-pack-stateless-timely-eof.sh | 22 ++++++++++++++++++++++ upload-pack.c | 13 ++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) create mode 100755 t/t9904-upload-pack-stateless-timely-eof.sh