Re: [PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf

> >> @@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
> >>  static int run_rewrite_hook(const unsigned char *oldsha1,
> >>  			    const unsigned char *newsha1)
> >>  {
> >> -	/* oldsha1 SP newsha1 LF NUL */
> >> -	static char buf[2*40 + 3];
> >> +	char *buf;
> >>  	struct child_process proc = CHILD_PROCESS_INIT;
> >>  	const char *argv[3];
> >>  	int code;
> >> -	size_t n;
> >>  
> >>  	argv[0] = find_hook("post-rewrite");
> >>  	if (!argv[0])
> >> @@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
> >>  	code = start_command(&proc);
> >>  	if (code)
> >>  		return code;
> >> -	n = snprintf(buf, sizeof(buf), "%s %s\n",
> >> -		     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> >> +	buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> >>  	sigchain_push(SIGPIPE, SIG_IGN);
> >> -	write_in_full(proc.in, buf, n);
> >> +	write_in_full(proc.in, buf, strlen(buf));
> >>  	close(proc.in);
> >> +	free(buf);
> >
> > Any time you care about the length of the result, I'd generally use an
> > actual strbuf instead of xstrfmt. The extra strlen isn't a big deal
> > here, but it somehow seems simpler to me. It probably doesn't matter
> > much either way, though.
> 
> Your justification for this extra allocation was that it is a
> heavy-weight operation.  While I agree that the runtime cost of
> allocation and deallocation does not matter, I would be a bit
> worried about extra cognitive burden to programmers.  They did not
> have to worry about leaking because they are writing a fixed length
> string.  Now they do, whether they use xstrfmt() or struct strbuf.
> When they need to update what they write, they do have to remember
> to adjust the size of the "fixed string", and the original is not
> free from the "programmers' cognitive cost" point of view, of
> course.  Probably use of strbuf/xstrfmt is an overall win.