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

[Junio C Hamano <gitster@pobox.com>]

Erick Mattos <erick.mattos@gmail.com> 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?