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

From: Jeff King <peff@peff.net>
Date: Oct 30, 2009, 19:30 UTC
Message-ID: <20091030193030.GA10985@coredump.intra.peff.net>
In-Reply-To: <20091030191239.GF10505@spearce.org>

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

Show 7 quoted lines

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

Show 8 quoted lines

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

Show 8 quoted lines

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

Show 7 quoted lines

> 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