> 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.
I see that packet_read() and packet_read_line_buf() invocations are also
removed, so it might be better to use "Use packet_reader instead of
packet_read.*" as the commit title.
The code looks correct to me - most of the changes are removals of
packet_read_line(), replaced with a packet_reader that has
PACKET_READ_CHOMP_NEWLINE. One instance is packet_read(), for which the
corresponding reader does not have PACKET_READ_CHOMP_NEWLINE (noted
below); and another instance is packet_read_line_buf(), for which the
corresponding reader is instantiated accordingly with the buffer (also
noted below).
> - if (!strcmp(line, "push-cert")) {
> + if (!strcmp(reader->line, "push-cert")) {
> int true_flush = 0;
> - char certbuf[1024];
> + int saved_options = reader->options;
> + reader->options &= ~PACKET_READ_CHOMP_NEWLINE;
>
> for (;;) {
> - len = packet_read(0, NULL, NULL,
> - certbuf, sizeof(certbuf), 0);
> - if (!len) {
> + packet_reader_read(reader);
> + if (reader->status == PACKET_READ_FLUSH) {
> true_flush = 1;
> break;
> }
> - if (!strcmp(certbuf, "push-cert-end\n"))
> + if (reader->status != PACKET_READ_NORMAL) {
> + die("protocol error: got an unexpected packet");
> + }
> + if (!strcmp(reader->line, "push-cert-end\n"))
> break; /* end of cert */
> - strbuf_addstr(&push_cert, certbuf);
> + strbuf_addstr(&push_cert, reader->line);
> }
> + reader->options = saved_options;
Here, packet_read() is used, so we shouldn't chomp the newline, so this
is correct.
> - char *line;
> + struct packet_reader reader;
> + packet_reader_init(&reader, -1, last->buf, last->len,
> + PACKET_READ_CHOMP_NEWLINE);
>
> /*
> * smart HTTP response; validate that the service
> * pkt-line matches our request.
> */
> - line = packet_read_line_buf(&last->buf, &last->len, NULL);
> - if (!line)
> + if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
> die("invalid server response; expected service, got flush packet");
>
> strbuf_reset(&exp);
> strbuf_addf(&exp, "# service=%s", service);
> - if (strcmp(line, exp.buf))
> - die("invalid server response; got '%s'", line);
> + if (strcmp(reader.line, exp.buf))
> + die("invalid server response; got '%s'", reader.line);
> strbuf_release(&exp);
>
> /* The header can include additional metadata lines, up
> * until a packet flush marker. Ignore these now, but
> * in the future we might start to scan them.
> */
> - while (packet_read_line_buf(&last->buf, &last->len, NULL))
> - ;
> + for (;;) {
> + packet_reader_read(&reader);
> + if (reader.pktlen <= 0) {
> + break;
> + }
> + }
> +
> + last->buf = reader.src_buffer;
> + last->len = reader.src_len;
And here, packet_reader_init() correctly initializes the packet_reader
with the buffer, and we need to know where in the buffer to continue
after parsing the additional metadata lines and the packet flush, so we
assign the state of the reader to last->buf and last->len.
(Incidentally, with this change, there is no longer any usage of
packet_read_line_buf(), but we can remove that in a subsequent patch.)
In summary, this looks like a good change. Configuration of reader
metadata (file descriptors, input buffers, and flags) is now more
centralized, and there are fewer file descriptor constants and variables
(which all look like ints) strewn around.
