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

["Shawn O. Pearce" <spearce@spearce.org>]

Jeff King <peff@peff.net> wrote:
> On Thu, Oct 29, 2009 at 02:58:29PM -0700, Shawn O. Pearce wrote:
> > Junio C Hamano <gitster@pobox.com> wrote:
> > > 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.
> 
> I am just a bystander, so maybe my opinion is not worth anything, but
> personally I think you are obfuscating the code to save a single line.

Yup, me too.  But I'm also willing to do what I need to get my
patches included in git.git.  Smart HTTP is something a lot of people
have been waiting for.  If the maintainer wants the bikeshed to be
a particular shade of red, I'll paint it that way.

> When I see a static variable, I assume it is because the value should be
> saved from invocation to invocation, and now I will spend time wondering
> why that would be the case here.

Me too.  I also can't grok the code I just wrote, for the same
reason, it just reads wrong.  But who am I to argue with the guy
who is most likely going to be the one who needs to deal with it
in the future?
 
> If you really just want to initialize to zero, using
> 
>   char linelen[5] = { 0 };

Bleh, I find that has hard to grok as what we have now.  Perhaps my
understanding of the relevant standards is incomplete, but I'd read
that as linelen[0] = 0, but the other 4 positions are undefined
and may be not be initialized.
 
> I think it would be even more clear to leave it as
> 
>   die("protocol error: bad line length character: %.4s", linelen);

I actually considered this one, but again I wasn't clear what would
happen in the standard C library when we fed a string that wasn't
actually NUL terminated.  Is the library permitted to call strlen()
before formatting?  If so strlen() could SIGSEGV if we are unlucky
and no NUL is present between our string and the end of the assigned
memory region.

To me, my original version was the most clear, to me and anyone
else who could ever possibly come by to read it.  The "one extra
line of code" is also only in an error condition which never occurs
(but did once due to a bug in the HTTP code, which is why I added
this patch to my series, to help debug it).  Its not like this is
a performance sensitive section of git that Linus is going to come
back and overhaul.

-- 
Shawn.