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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 6, 2019, 20:42 UTC
Message-ID: <xmqqzhr8ps2m.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190206192903.GE72177@google.com>

Josh Steadmon <steadmon@google.com> writes:

Show 8 quoted lines

>> +	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?

You mean that it may not be "flush packet"? I guess we can remove ", got X" part and that would probably be an improvement.

Show 18 quoted lines

>> +
>> +	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)
>   ;

The only case as far as I can tell that would make the difference between the original and your simplified one would be if a packet had a single newline and nothing else in it, in which case pktlen would be zero (since we pass CHOMP_NEWLINE) and the return value would be READ_NORMAL. The original would break, while yours will spin one more time.

Thanks.