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, 17:57 UTC
Message-ID: <20091030175741.GC18583@coredump.intra.peff.net>
In-Reply-To: <20091029215829.GD10505@spearce.org>

On Thu, Oct 29, 2009 at 02:58:29PM -0700, Shawn O. Pearce wrote:

Show 11 quoted lines

> Junio C Hamano <gitster@pobox.com> wrote:
> > > 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.
> 
> Thanks, I get it now.  Patch amended.

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

If you really just want to initialize to zero, using

  char linelen[5] = { 0 };

should be sufficient (I think all of the compilers we care about support such incomplete array assignments, right?).

I think it would be even more clear to leave it as

  char linelen[4];

which declares how the data is _actually_ expected to be used, and then do:

  die("protocol error: bad line length character: %.4s", linelen);

-Peff

PS All of the proposals also suffer from the fact that die will truncate broken output at a NUL sent by the remote. Probably OK, since we are expecting ascii hex or some variant, and if we don't correctly print what the remote said in a totally broken NUL-filled case, it is not that big a deal.