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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 20, 2014, 02:36 UTC
Message-ID: <20141120023655.GG6527@google.com>
In-Reply-To: <1416449134-12281-1-git-send-email-sbeller@google.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,

[...]

Show 6 quoted lines

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