| Message ID | 20180912053519.31085-2-steadmon@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] archive: use packet_reader for communications | expand |
On Tue, Sep 11, 2018 at 10:35 PM Josh Steadmon <steadmon@google.com> wrote: > > Using packet_reader will simplify version detection and capability > handling, which will make implementation of protocol v2 support in > git-archive easier. > > Signed-off-by: Josh Steadmon <steadmon@google.com> Reviewed-by: Stefan Beller <sbeller@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? > 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. > - 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". > + 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 */
Junio C Hamano <gitster@pobox.com> writes: >> - 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? Sorry, this part of my comment is completely backwards. We require they send a flush, not a 0-length data packet of length 4, and otherwise we die, even though we used to treate 0-length data packet of length 4 just like a flush. So this is making the code more strict than the original. As long as all the existing implementations correctly use flush here, there would be no unintended regression, but it bothers me that we have to even worry if these behaviour changes affect the already deployed software negatively. > >> /* Now, start reading from fd[0] and spit it out to stdout */
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")); - 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")) { + 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")); /* Now, start reading from fd[0] and spit it out to stdout */
Using packet_reader will simplify version detection and capability handling, which will make implementation of protocol v2 support in git-archive easier. Signed-off-by: Josh Steadmon <steadmon@google.com> --- builtin/archive.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)