On Thu, Oct 29, 2009 at 02:58:29PM -0700, Shawn O. Pearce wrote:
> Junio C Hamano <gitster@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html