From: Jonathan Tan <jonathantanmy@google.com>
To: masayasuzuki@google.com
Cc: git@vger.kernel.org, peff@peff.net, steadmon@google.com,
Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v2 2/2] pack-protocol.txt: accept error packets in any context
Date: Mon, 7 Jan 2019 11:41:50 -0800 [thread overview]
Message-ID: <20190107194150.58700-1-jonathantanmy@google.com> (raw)
In-Reply-To: <20181229211915.161686-3-masayasuzuki@google.com>
First of all, there was some discussion [1] about its relation with
js/smart-http-detect-remote-error. I noticed that
js/smart-http-detect-remote-error has all tests passing even if I remove
Masaya's patch, so I'm reviewing these patches independently, directly on
master.
[1] https://public-inbox.org/git/CAJB1erU0utjKGtv3LBFT6SEEKCfFRuxGvDtpkKeS3GSC1S89JA@mail.gmail.com/
> In the Git pack protocol definition, an error packet may appear only in
> a certain context. However, servers can face a runtime error (e.g. I/O
> error) at an arbitrary timing. This patch changes the protocol to allow
> an error packet to be sent instead of any packet.
>
> Without this protocol spec change, when a server cannot process a
> request, there's no way to tell that to a client. Since the server
> cannot produce a valid response, it would be forced to cut a connection
> without telling why. With this protocol spec change, the server can be
> more gentle in this situation. An old client may see these error packets
> as an unexpected packet, but this is not worse than having an unexpected
> EOF.
The other thing that happens is that servers send "ERR" anyway, even
though it is not allowed by the protocol.
This overall looks like a good direction - this makes explicit something
that is already being done.
My remaining concern is if "ERR " could be a non-error packet mistaken
for an error one. I glanced through pack-protocol.txt and as far as I
can tell, I don't see anything non-error sent by the server that could
be prefixed with "ERR". (There are push-option and gpg-signature-lines,
but those are sent by the client.) Packfiles can contain anything, of
course, but as far as I can tell, they are either sent un-PKT-ed or
preceded by a sideband (\1), so they are fine.
> diff --git a/serve.c b/serve.c
> index bda085f09..317256c1a 100644
> --- a/serve.c
> +++ b/serve.c
> @@ -167,7 +167,8 @@ static int process_request(void)
>
> packet_reader_init(&reader, 0, NULL, 0,
> PACKET_READ_CHOMP_NEWLINE |
> - PACKET_READ_GENTLE_ON_EOF);
> + PACKET_READ_GENTLE_ON_EOF |
> + PACKET_READ_DIE_ON_ERR_PACKET);
>
> /*
> * Check to see if the client closed their end before sending another
> @@ -175,7 +176,7 @@ static int process_request(void)
> */
> if (packet_reader_peek(&reader) == PACKET_READ_EOF)
> return 1;
> - reader.options = PACKET_READ_CHOMP_NEWLINE;
> + reader.options &= ~PACKET_READ_GENTLE_ON_EOF;
Here, the old line is meant to remove PACKET_READ_GENTLE_ON_EOF - the
new line is both necessary and clearer.
> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> index 3f58f05cb..d2a9d0c12 100755
> --- a/t/t5703-upload-pack-ref-in-want.sh
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -208,7 +208,7 @@ test_expect_success 'server is initially ahead - no ref in want' '
> cp -r "$LOCAL_PRISTINE" local &&
> inconsistency master 1234567890123456789012345678901234567890 &&
> test_must_fail git -C local fetch 2>err &&
> - grep "ERR upload-pack: not our ref" err
> + grep "fatal: remote error: upload-pack: not our ref" err
> '
>
> test_expect_success 'server is initially ahead - ref in want' '
> @@ -254,7 +254,7 @@ test_expect_success 'server loses a ref - ref in want' '
> echo "s/master/raster/" >"$HTTPD_ROOT_PATH/one-time-sed" &&
> test_must_fail git -C local fetch 2>err &&
>
> - grep "ERR unknown ref refs/heads/raster" err
> + grep "fatal: remote error: unknown ref refs/heads/raster" err
> '
And this shows that we have tests that exercise the new code.
The rest of the diff is just the addition of the new
PACKET_READ_DIE_ON_ERR_PACKET, and looks correct.
next prev parent reply other threads:[~2019-01-07 19:41 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <reply-to=20181127045301.103807-1-masayasuzuki@google.com>
2018-12-29 21:19 ` [PATCH v2 0/2] Accept error packets in any context Masaya Suzuki
2018-12-29 21:19 ` [PATCH v2 1/2] Use packet_reader instead of packet_read_line Masaya Suzuki
2019-01-07 19:01 ` Jonathan Tan
2019-01-07 22:33 ` Josh Steadmon
2019-01-08 23:27 ` Masaya Suzuki
2018-12-29 21:19 ` [PATCH v2 2/2] pack-protocol.txt: accept error packets in any context Masaya Suzuki
2019-01-07 19:41 ` Jonathan Tan [this message]
2019-01-07 22:53 ` Josh Steadmon
2019-01-03 23:05 ` [PATCH v2 0/2] Accept " Junio C Hamano
2019-01-04 0:20 ` Masaya Suzuki
2019-01-15 1:43 ` Jonathan Nieder
2019-01-15 1:49 ` Jonathan Nieder
2019-01-15 18:44 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190107194150.58700-1-jonathantanmy@google.com \
--to=jonathantanmy@google.com \
--cc=git@vger.kernel.org \
--cc=masayasuzuki@google.com \
--cc=peff@peff.net \
--cc=steadmon@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).