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

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.

[...]

[...]

[...]

Since 'result' isn't used here, this could be simplified to

	if (log_ref_write_fd(...)) {
		...
	}

Thanks and hope that helps,
Jonathan