On Mon, Jan 7, 2019 at 2:33 PM Josh Steadmon <steadmon@xxxxxxxxxx> wrote:
>
> On 2018.12.29 13:19, Masaya Suzuki wrote:
> > By using and sharing a packet_reader while handling a Git pack protocol
> > request, the same reader option is used throughout the code. This makes
> > it easy to set a reader option to the request parsing code.
> >
> > Signed-off-by: Masaya Suzuki <masayasuzuki@xxxxxxxxxx>
> > ---
> > builtin/archive.c | 19 ++++++-------
> > builtin/receive-pack.c | 60 +++++++++++++++++++++--------------------
> > fetch-pack.c | 61 +++++++++++++++++++++++-------------------
> > remote-curl.c | 22 ++++++++++-----
> > send-pack.c | 37 ++++++++++++-------------
> > upload-pack.c | 38 +++++++++++++-------------
> > 6 files changed, 129 insertions(+), 108 deletions(-)
> >
> > diff --git a/builtin/archive.c b/builtin/archive.c
> > index d2455237c..2fe1f05ca 100644
> > --- a/builtin/archive.c
> > +++ b/builtin/archive.c
> > @@ -27,10 +27,10 @@ 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;
> >
> > _remote = remote_get(remote);
> > if (!_remote->url[0])
> > @@ -53,18 +53,19 @@ 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)
> > + packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);
> > +
> > + if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
>
> packet_read_line() can also return NULL if the packet is zero-length, so
> you may want to add a "|| reader.pktlen <= 0" to the condition here (and
> in other places where we were checking that packet_read_line() != NULL)
> to make sure the behavior doesn't change. See discussion on my previous
> attempt[1] to refactor this in builtin/archive.c.
>
> [1]: https://public-inbox.org/git/20180912053519.31085-1-steadmon@xxxxxxxxxx/
That is interesting. In Documentation/technical/protocol-common.txt,
it says "Implementations SHOULD NOT send an empty pkt-line ("0004").".
The existing code won't distinguish "0000" and "0004", while "0004" is
actually not a valid pkt-line. I'll make this patch with no behavior
change, but I think we can make that behavior change to stop accepting
0004 as 0000, and remove the pktlen checks.
