From: Jeff King <peff@peff.net>
Date: Fri, 03 Jun 2016 17:10:01 GMT
Subject: Re: [PATCH 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf
Message-ID: <20160603171001.GB3858@sigill.intra.peff.net>
In-Reply-To: <5751A319.1030806@ramsayjones.plus.com>

On Fri, Jun 03, 2016 at 04:32:41PM +0100, Ramsay Jones wrote:

> >>  		char buf[48];
> >> -		int len = snprintf(buf, sizeof(buf), "%s\t%s\n",
> >> +		int len = xsnprintf(buf, sizeof(buf), "%s\t%s\n",
> >>  				   report, sha1_to_hex(sha1));
> >>  		write_or_die(1, buf, len);
> > 
> > So it's pretty unclear here whether that 48 is big enough (it is, if you
> > read the whole function, because "report" is always a 4-char string).
> > Yuck. At least there should be a comment explaining why 48 is big
> > enough.
> 
> Agreed, again I would use something like:
> 
> 		char buf[GIT_SHA1_HEXSZ + 7]; /* 40 (sha1) + 4 (report) + 3 (\t\n\0) */

Yes, that's much better, I think.

> (and yes yuck - is report ever likely to increase? "bitmap" perhaps?)

It shouldn't. It's easy to think that's a filetype, but it really is
just "did you tell me --keep". TBH, I am not really sure that switching
it accomplishes anything.

-Peff