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

[Jonathan Nieder <jrnieder@gmail.com>]

Stefan Beller wrote:

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

Does this supersede the other patch with the same subject?

Please keep adding v<num> in the subject --- when it's there, it makes
reading much easier.

>

Missing 'From:' line naming the original patch author.

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

Is the idea something like "Split out a function that [explanation
of when a caller would use this function goes here]?"

>                   For now this is only used from log_ref_write,
> but later on we will call this function from reflog transactions too,

Useful to know.  I'd end the sentence here, since it seems to run
on with a different thought.

> which means that we will end up with only a single place,
> where we write a reflog entry to a file instead of the current two
> places (log_ref_write and builtin/reflog.c).

Ah, so builtin/reflog.c is doing something similar and will later
be changed to use this code, too?

Overall it sounds like a very good change.

[...]
> Here comes a resend without changing code, but just making it a new function,
> so we come forwards with the patch.

\o/

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

[...]
> +++ b/refs.c
[...]
> @@ -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(...)) {
		...
	}

Thanks and hope that helps,
Jonathan