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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 13, 2016, 19:03 UTC
Message-ID: <xmqqy3zj3b3a.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161213135514.z7eituxgxsvybwgz@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 12 quoted lines

>> +		argv_array_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
>> +		if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
>>  			fprintf(stderr,
>>  			_("Please supply the message using either -m or -F option.\n"));
>> +			argv_array_clear(&env);
>>  			exit(1);
>>  		}
>> +		argv_array_clear(&env);
>
> I'd generally skip the clear() right before exiting, though I saw
> somebody disagree with me recently on that.  I wonder if we should
> decide as a project on whether it is worth doing or not.

I'd say it is a responsibility of the person who turns exit(1) to return -1 to ensure the code does not leak.

Show 31 quoted lines

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

And of course, I think strbuf is more appropriate if you have to do strlen().