Re: [PATCH 1/3] archive: use packet_reader for communications
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 13, 2018, 14:58 UTC
- Message-ID
- <xmqqr2hxigw2.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180912053519.31085-2-steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> Using packet_reader will simplify version detection and capability > handling, which will make implementation of protocol v2 support in > git-archive easier.
Is this meant as a change in implementation without any change in behaviour?
Show 41 quoted lines
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
> builtin/archive.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/archive.c b/builtin/archive.c
> index e74f67539..e54fc39ad 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -27,10 +27,11 @@ static int run_remote_archiver(int argc, const char **argv,
> const char *remote, const char *exec,
> const char *name_hint)
> {
> - char *buf;
> int fd[2], i, rv;
> struct transport *transport;
> struct remote *_remote;
> + struct packet_reader reader;
> + enum packet_read_status status;
>
> _remote = remote_get(remote);
> if (!_remote->url[0])
> @@ -38,6 +39,8 @@ static int run_remote_archiver(int argc, const char **argv,
> transport = transport_get(_remote, _remote->url[0]);
> transport_connect(transport, "git-upload-archive", exec, fd);
>
> + packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);
> +
> /*
> * Inject a fake --format field at the beginning of the
> * arguments, with the format inferred from our output
> @@ -53,18 +56,20 @@ static int run_remote_archiver(int argc, const char **argv,
> packet_write_fmt(fd[1], "argument %s\n", argv[i]);
> packet_flush(fd[1]);
>
> - buf = packet_read_line(fd[0], NULL);
> - if (!buf)
> + status = packet_reader_read(&reader);
> +
> + if (status == PACKET_READ_FLUSH)
> die(_("git archive: expected ACK/NAK, got a flush packet"));It is true that packet_read_line() returns a NULL on flush, but the function also returns NULL on other conditions for which underlying packet_read() returns 0 (or negative) length. EOF, normal data with zero length (i.e. packet length itself is 4), and DELIM packets would all have led to this die() in the original code. We fail to notice that we got something unexpected when we were expecting to get a normal packet with ACK or NACK on it.
Show 6 quoted lines
> - if (strcmp(buf, "ACK")) {
> - if (starts_with(buf, "NACK "))
> - die(_("git archive: NACK %s"), buf + 5);
> - if (starts_with(buf, "ERR "))
> - die(_("remote error: %s"), buf + 4);
> + if (strcmp(reader.buffer, "ACK")) {The way I read packet_reader_read()'s implementation and existing callers (like the ones in fetch-pack.c) tells me that consumers should not be looking at "reader.buffer"; they should instead be reading from "reader.line".
Show 11 quoted lines
> + if (starts_with(reader.buffer, "NACK "))
> + die(_("git archive: NACK %s"), reader.buffer + 5);
> + if (starts_with(reader.buffer, "ERR "))
> + die(_("remote error: %s"), reader.buffer + 4);
> die(_("git archive: protocol error"));
> }
>
> - if (packet_read_line(fd[0], NULL))
> + status = packet_reader_read(&reader);
> + if (status != PACKET_READ_FLUSH)
> die(_("git archive: expected a flush"));This makes me wonder what happens if we got an EOF instead. We fail to notice protocol error here, but do the code after this part correctly handle the situation?
> /* Now, start reading from fd[0] and spit it out to stdout */