Re: [PATCH 1/3] archive: use packet_reader for communications
Junio C Hamano writes:
>> -if (packet_read_line(fd[0], NULL))
>> +status = packet_reader_read();
>> +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 */
Re: [PATCH 1/3] archive: use packet_reader for communications
Josh Steadmon 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
> ---
> 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(, 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();
> +
> + 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();
> + 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 */
Re: [PATCH 1/3] archive: use packet_reader for communications
On Tue, Sep 11, 2018 at 10:35 PM Josh Steadmon 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 Reviewed-by: Stefan Beller
[PATCH 1/3] archive: use packet_reader for communications
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
---
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(, 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();
+
+ 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();
+ if (status != PACKET_READ_FLUSH)
die(_("git archive: expected a flush"));
/* Now, start reading from fd[0] and spit it out to stdout */
--
2.19.0.397.gdd90340f6a-goog
