Re: [RFC PATCH v4 03/26] pkt-line: Make packet_read_line easier to debug

[Junio C Hamano <gitster@pobox.com>]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>> "Shawn O. Pearce" <spearce@spearce.org> writes:
>> > diff --git a/pkt-line.c b/pkt-line.c
>> > index bd603f8..893dd3c 100644
>> > --- a/pkt-line.c
>> > +++ b/pkt-line.c
>> > @@ -124,12 +124,14 @@ static int packet_length(const char *linelen)
>> >  int packet_read_line(int fd, char *buffer, unsigned size)
>> >  {
>> >  	int len;
>> > -	char linelen[4];
>> > +	char linelen[5];
>> >  
>> >  	safe_read(fd, linelen, 4);
>> >  	len = packet_length(linelen);
>> > -	if (len < 0)
>> > -		die("protocol error: bad line length character");
>> > +	if (len < 0) {
>> > +		linelen[4] = '\0';
>> > +		die("protocol error: bad line length character: %s", linelen);
>> > +	}
>> 
>> Since this is not called recursively, you can make linelen[] static
>
> Sure.  But it wasn't static before.  It was stack allocated.  Its a
> 5 byte stack allocation.  Its not a lot of data to push onto the
> stack, why does it suddenly matter that we make it static and charge
> everyone for its memory instead of just those who really need it?
>
>> and do
>> without the NUL assignment; safe_read() won't read beyond 4 bytes anyway.
>
> The NUL assignment isn't about safe_read(), its about making the
> block of 4 bytes read a proper C-style string so we can safely pass
> it down into the snprintf that is within die().

I knew and understood all of what you just said.  static linelen[] is not
about stack allocation.  It's about letting the compiler initialize it to
five NULs so that you do not have to assign NUL to its fifth place before
you die.  This removes one added line from your patch.