Re: [PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation

From: Jeff King <peff@peff.net>
Date: Feb 18, 2013, 10:48 UTC
Message-ID: <20130218104804.GB16408@sigill.intra.peff.net>
In-Reply-To: <20130218104350.GF7049@elie.Belkin>

On Mon, Feb 18, 2013 at 02:43:50AM -0800, Jonathan Nieder wrote:

Show 6 quoted lines

> Jeff King wrote:
> 
> > The packet_read function reads from a descriptor.
> 
> Ah, so this introduces a new analagous helper that reads from
> a strbuf, to avoid the copy-from-async-procedure hack?

Not from a strbuf, but basically, yes.

Show 6 quoted lines

> > +		ret = read_in_full(fd, dst, size);
> > +		if (ret < 0)
> > +			die_errno("read error");
> 
> This is noisy about upstream pipe gone missing, which makes sense
> since this is transport-related.  Maybe that deserves a comment.

That is not new code; it is just reindented from the original safe_read. But is it noisy about a missing pipe? We do not get EPIPE for reading. We should just get a short read or EOF, both of which is handled later.

Show 5 quoted lines

> > +		len = packet_read_from_buf(line, sizeof(line), &last->buf, &last->len);
> > +		if (len && line[len - 1] == '\n')
> > +			len--;
> 
> Was anything guaranteeing that buffer.len < 1000 before this change?

No. That's discussed in point (3) of the "implications" in the commit message.

-Peff