Re: [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup

Erick Mattos [off-list ref] writes:

-static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
-			 const unsigned char *new_sha1, const char *msg)
+int log_ref_setup(const char *ref_name, char **log_file)
 {
-	int logfd, written, oflags = O_APPEND | O_WRONLY;
-	unsigned maxlen, len;
-	int msglen;
-	char log_file[PATH_MAX];
-	char *logrec;
-	const char *committer;
-
-	if (log_all_ref_updates < 0)
-		log_all_ref_updates = !is_bare_repository();
-
-	git_snpath(log_file, sizeof(log_file), "logs/%s", ref_name);
+	int logfd, oflags = O_APPEND | O_WRONLY;
+	char logfile[PATH_MAX];
+	git_snpath(logfile, sizeof(logfile), "logs/%s", ref_name);
+	*log_file = logfile;
...

I have a slight suspicion that it would have made the patch smaller and
easier to read if you kept the name of the on-stack log_file[] as-is, and
named the retval parameter logfile_p or soemthing.  Also you would need to
make this buffer "static char log_file[]", no?  Otherwise you would be
returning a pointer to a dead buffer to the caller.

+static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
+			 const unsigned char *new_sha1, const char *msg)
+{
+ ...
+	result = log_ref_setup(ref_name, &log_file);
+	if (result)
+		return result;
+
+	logfd = open(log_file, oflags);

Yuck, the caller needs to call "setup" which discards the file descriptor
opened for writing and then open it again itself?