Re: [PATCH] refs.c: add a function to append a reflog entry to a fd

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2014, 16:57 UTC
Message-ID: <xmqqmw7l4xoj.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20141120023655.GG6527@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 5 quoted lines

>> Break out the code to create the string and writing it to the file
>> descriptor from log_ref_write and add it into a dedicated function
>> log_ref_write_fd.
>
> (grammar) I'm having trouble parsing the above.

Yeah, I can see what it wants to say, but still...

Show 7 quoted lines

>> Let's discuss the change and decide if I shouldsend a follow up patch to change
>> it into snprintf.
>
> Both sprintf and snprintf are error-prone functions.  It would be
> lovely in a followup to use strbuf_addf or xstrfmt in this code path.
> strbufs are how git deals with bookkeeping for string sizes --- they
> are very pleasant.

Yeah, that solves both sides of not stomping on other peoples' data (which may lead to replaced return address and such) and not giving broken result (which may cause trouble to the callers) equally well without raising a stink about "security!!!", which is good ;-).

Show 15 quoted lines

>> @@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname,
>> const unsigned char *old_sha1,
> [...]
>> +	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
>> +				  git_committer_info(0), msg);
>> +	if (result) {
>>  		int save_errno = errno;
>>  		close(logfd);
>>  		error("Unable to append to %s", log_file);
>
> Since 'result' isn't used here, this could be simplified to
>
> 	if (log_ref_write_fd(...)) {
> 		...
> 	}

Yeah that is a good simplification.