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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html