From: Jeff King <peff@peff.net>
Date: Fri, 15 May 2020 20:57:17 GMT
Subject: Re: [PATCH 4/6] pkt-line: extern packet_length()
Message-ID: <20200515205717.GC115445@coredump.intra.peff.net>
In-Reply-To: <20200515205632.GB115445@coredump.intra.peff.net>

On Fri, May 15, 2020 at 04:56:32PM -0400, Jeff King wrote:

> On Wed, May 13, 2020 at 02:04:56PM -0400, Denton Liu wrote:
> 
> > +/*
> > + * Reads a packetized line and returns the length header of the packet.
> > + */
> > +int packet_length(const char *linelen);
> 
> If this is becoming public, I think we need to talk a bit more about:
> 
>   - what is linelen; it's not a NUL-terminated string, which I would
>     have expected from the prototype. It must be at least 4 chars, and
>     doesn't need terminated.
> 
>   - what happens on error; it looks like we return -1
> 
> I think the prototype would be more clear to me as:
> 
>   int packet_length(const char *line, size_t len)
>   {
> 	if (len < 4)
> 		return -1;
>   }
> 
> which makes it clear that this is a sized buffer. But since nobody
> should ever be passing anything except "4", it may be overkill. I'd be
> happy enough with a comment.

Oh, and obviously:

  int packet_length(const char linelen[4]);

would be descriptive, but I think falls afoul of C pointer/array
weirdness.

-Peff