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

[Junio C Hamano <gitster@pobox.com>]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

>> 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 ;-).

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