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

[Jeff King <peff@peff.net>]

On Fri, Oct 30, 2009 at 12:12:40PM -0700, Shawn O. Pearce wrote:

> > 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.

Yes, I too want it merged ASAP, so please Junio if you disagree with my
comments, ignore them. But I did want to point out that the proposed
"improvement" was making things worse, IMHO. So I feel like I was at
least bikeshedding his bikeshed, instead of starting my own. ;)

> > 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.

Your understanding is incomplete. :) C99, 6.7.8, paragraph 19:

  ... all subobjects that are not initialized explicitly shall be
  initialized implicitly the same as objects that have static storage
  duration.

I don't recall offhand whether that was the case in C89 or not.

But that being said, it was an attempt to make the meaning clear. If
it's not to you (who I consider at least reasonably competent ;) ), then
it failed.

> >   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.

No, using a precision specifier for a string with no NUL is explicitly
allowed by the standard (and we use it elsewhere in git, IIRC).

> 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.

Heh. You never know what performance item Linus will complain about. ;)
But yes, I am fine with your initial one, too. The drawback to it (and
my "%.4s") is not that one line of code is so unbearable, but that it is
one extra thing anyone using it as a string must do, and if they forget,
we get a horrible segfault.

Anyway, my complaint has been lodged. Junio is aware of the alternatives
and can pick whichever he wants.

-Peff