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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html