Re: [PATCH 1/3] archive: use packet_reader for communications

[Junio C Hamano <gitster@pobox.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 */