Re: [PATCH 1/3] remote-curl: refactor smart-http discovery

From: Jeff King <peff@peff.net>
To: Josh Steadmon <steadmon@google.com>,
	Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 1/3] remote-curl: refactor smart-http discovery
Date: Wed, 6 Feb 2019 16:14:14 -0500	[thread overview]
Message-ID: <20190206211414.GA12737@sigill.intra.peff.net> (raw)
In-Reply-To: <20190206192903.GE72177@google.com>

On Wed, Feb 06, 2019 at 11:29:03AM -0800, Josh Steadmon wrote:

> > +	packet_reader_init(&reader, -1, d->buf, d->len,
> > +			   PACKET_READ_CHOMP_NEWLINE |
> > +			   PACKET_READ_DIE_ON_ERR_PACKET);
> > +	if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
> > +		die("invalid server response; expected service, got flush packet");
> 
> This can also trigger on an EOF or a delim packet, should we clarify the
> error message?

Maybe, though I'd prefer to do it as a patch on top; these lines were
moved straight from the existing code.

> > +	if (skip_prefix(reader.line, "# service=", &p) && !strcmp(p, service)) {
> > +		/*
> > +		 * 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.
> > +		 */
> > +		for (;;) {
> > +			packet_reader_read(&reader);
> > +			if (reader.pktlen <= 0) {
> > +				break;
> > +			}
> > +		}
> 
> Could we make this loop cleaner as:
> 
> while (packet_reader_read(&reader) != PACKET_READ_NORMAL)
>   ;

Likewise here.

-Peff