Re: [PATCH v2 1/2] Use packet_reader instead of packet_read_line

[Masaya Suzuki <masayasuzuki@google.com>]

On Mon, Jan 7, 2019 at 2:33 PM Josh Steadmon <steadmon@google.com> 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@google.com>
> > ---
> >  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@google.com/

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.