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

From: Junio C Hamano <gitster@pobox.com>
Date: May 26, 2010, 05:07 UTC
Message-ID: <7v632bs13c.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1274488119-6989-3-git-send-email-erick.mattos@gmail.com>

Erick Mattos <erick.mattos@gmail.com> writes:

Show 20 quoted lines

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

Show 9 quoted lines

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