Re: [PATCH v5 03/28] Convert git_snpath() to strbuf_git_path()

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> In the previous patch, git_snpath() is modified to allocate a new
> strbuf buffer because vsnpath() needs that. But that makes it awkward
> because git_snpath() receives a pre-allocated buffer from outside and
> has to copy data back. Rename it to strbuf_git_path() and make it
> receive strbuf directly.
>
> The conversion from git_snpath() to git_path() in
> update_refs_for_switch() is safe because that function does not keep
> any pointer to the round-robin buffer pool allocated by
> get_pathname().

s/that function does not/that function and all of its callers do not/

is the guarantee we need to say it is safe.  And I think that holds
true.

Thanks.

>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/checkout.c | 25 +++++++++--------
>  cache.h            |  4 +--
>  path.c             | 11 ++------
>  refs.c             | 78 +++++++++++++++++++++++++++++++++---------------------
>  refs.h             |  2 +-
>  5 files changed, 65 insertions(+), 55 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 64c2aca..efb5e2f 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -585,18 +585,21 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>  		if (opts->new_orphan_branch) {
>  			if (opts->new_branch_log && !log_all_ref_updates) {
>  				int temp;
> -				char log_file[PATH_MAX];
> -				const char *ref_name = mkpath("refs/heads/%s", opts->new_orphan_branch);
> +				struct strbuf log_file = STRBUF_INIT;
> +				int ret;
> +				const char *ref_name;
>  
> +				ref_name = mkpath("refs/heads/%s", opts->new_orphan_branch);
>  				temp = log_all_ref_updates;
>  				log_all_ref_updates = 1;
> -				if (log_ref_setup(ref_name, log_file, sizeof(log_file))) {
> +				ret = log_ref_setup(ref_name, &log_file);
> +				log_all_ref_updates = temp;
> +				strbuf_release(&log_file);
> +				if (ret) {
>  					fprintf(stderr, _("Can not do reflog for '%s'\n"),
>  					    opts->new_orphan_branch);
> -					log_all_ref_updates = temp;
>  					return;
>  				}
> -				log_all_ref_updates = temp;
>  			}
>  		}
>  		else
> @@ -651,14 +654,10 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>  					new->name);
>  			}
>  		}
> -		if (old->path && old->name) {
> -			char log_file[PATH_MAX], ref_file[PATH_MAX];
> -
> -			git_snpath(log_file, sizeof(log_file), "logs/%s", old->path);
> -			git_snpath(ref_file, sizeof(ref_file), "%s", old->path);
> -			if (!file_exists(ref_file) && file_exists(log_file))
> -				remove_path(log_file);
> -		}
> +		if (old->path && old->name &&
> +		    !file_exists(git_path("%s", old->path)) &&
> +		     file_exists(git_path("logs/%s", old->path)))
> +			remove_path(git_path("logs/%s", old->path));
>  	}
>  	remove_branch_state();
>  	strbuf_release(&msg);
> diff --git a/cache.h b/cache.h
> index a344a5f..0fae730 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -646,8 +646,8 @@ extern int check_repository_format(void);
>  
>  extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)
>  	__attribute__((format (printf, 3, 4)));
> -extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
> -	__attribute__((format (printf, 3, 4)));
> +extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
> +	__attribute__((format (printf, 2, 3)));
>  extern char *git_pathdup(const char *fmt, ...)
>  	__attribute__((format (printf, 1, 2)));
>  extern char *mkpathdup(const char *fmt, ...)
> diff --git a/path.c b/path.c
> index 36d461e..417df76 100644
> --- a/path.c
> +++ b/path.c
> @@ -70,19 +70,12 @@ static void vsnpath(struct strbuf *buf, const char *fmt, va_list args)
>  	strbuf_cleanup_path(buf);
>  }
>  
> -char *git_snpath(char *buf, size_t n, const char *fmt, ...)
> +void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
>  {
> -	struct strbuf sb = STRBUF_INIT;
>  	va_list args;
>  	va_start(args, fmt);
> -	vsnpath(&sb, fmt, args);
> +	vsnpath(sb, fmt, args);
>  	va_end(args);
> -	if (sb.len >= n)
> -		strlcpy(buf, bad_path, n);
> -	else
> -		memcpy(buf, sb.buf, sb.len + 1);
> -	strbuf_release(&sb);
> -	return buf;
>  }
>  
>  char *git_pathdup(const char *fmt, ...)
> diff --git a/refs.c b/refs.c
> index f846f2f..c5613b0 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1325,10 +1325,12 @@ static const char *handle_missing_loose_ref(const char *refname,
>  
>  const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)
>  {
> +	struct strbuf sb_path = STRBUF_INIT;
>  	int depth = MAXDEPTH;
>  	ssize_t len;
>  	char buffer[256];
>  	static char refname_buffer[256];
> +	const char *ret;
>  
>  	if (flag)
>  		*flag = 0;
> @@ -1337,15 +1339,17 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
>  		return NULL;
>  
>  	for (;;) {
> -		char path[PATH_MAX];
> +		const char *path;
>  		struct stat st;
>  		char *buf;
>  		int fd;
>  
>  		if (--depth < 0)
> -			return NULL;
> +			goto fail;
>  
> -		git_snpath(path, sizeof(path), "%s", refname);
> +		strbuf_reset(&sb_path);
> +		strbuf_git_path(&sb_path, "%s", refname);
> +		path = sb_path.buf;
>  
>  		/*
>  		 * We might have to loop back here to avoid a race
> @@ -1359,10 +1363,11 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
>  	stat_ref:
>  		if (lstat(path, &st) < 0) {
>  			if (errno == ENOENT)
> -				return handle_missing_loose_ref(refname, sha1,
> -								reading, flag);
> +				ret = handle_missing_loose_ref(refname, sha1,
> +							       reading, flag);
>  			else
> -				return NULL;
> +				ret = NULL;
> +			goto done;
>  		}
>  
>  		/* Follow "normalized" - ie "refs/.." symlinks by hand */
> @@ -1373,7 +1378,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
>  					/* inconsistent with lstat; retry */
>  					goto stat_ref;
>  				else
> -					return NULL;
> +					goto fail;
>  			}
>  			buffer[len] = 0;
>  			if (starts_with(buffer, "refs/") &&
> @@ -1389,7 +1394,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
>  		/* Is it a directory? */
>  		if (S_ISDIR(st.st_mode)) {
>  			errno = EISDIR;
> -			return NULL;
> +			goto fail;
>  		}
>  
>  		/*
> @@ -1402,12 +1407,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
>  				/* inconsistent with lstat; retry */
>  				goto stat_ref;
>  			else
> -				return NULL;
> +				goto fail;
>  		}
> +
>  		len = read_in_full(fd, buffer, sizeof(buffer)-1);
>  		close(fd);
>  		if (len < 0)
> -			return NULL;
> +			goto fail;
>  		while (len && isspace(buffer[len-1]))
>  			len--;
>  		buffer[len] = '\0';
> @@ -1424,9 +1430,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
>  			    (buffer[40] != '\0' && !isspace(buffer[40]))) {
>  				if (flag)
>  					*flag |= REF_ISBROKEN;
> -				return NULL;
> +				goto fail;
>  			}
> -			return refname;
> +			ret = refname;
> +			goto done;
>  		}
>  		if (flag)
>  			*flag |= REF_ISSYMREF;
> @@ -1436,10 +1443,15 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
>  		if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
>  			if (flag)
>  				*flag |= REF_ISBROKEN;
> -			return NULL;
> +			goto fail;
>  		}
>  		refname = strcpy(refname_buffer, buf);
>  	}
> +fail:
> +	ret = NULL;
> +done:
> +	strbuf_release(&sb_path);
> +	return ret;
>  }
>  
>  char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)
> @@ -2717,41 +2729,41 @@ static int copy_msg(char *buf, const char *msg)
>  	return cp - buf;
>  }
>  
> -int log_ref_setup(const char *refname, char *logfile, int bufsize)
> +int log_ref_setup(const char *refname, struct strbuf *logfile)
>  {
>  	int logfd, oflags = O_APPEND | O_WRONLY;
>  
> -	git_snpath(logfile, bufsize, "logs/%s", refname);
> +	strbuf_git_path(logfile, "logs/%s", refname);
>  	if (log_all_ref_updates &&
>  	    (starts_with(refname, "refs/heads/") ||
>  	     starts_with(refname, "refs/remotes/") ||
>  	     starts_with(refname, "refs/notes/") ||
>  	     !strcmp(refname, "HEAD"))) {
> -		if (safe_create_leading_directories(logfile) < 0)
> +		if (safe_create_leading_directories(logfile->buf) < 0)
>  			return error("unable to create directory for %s",
> -				     logfile);
> +				     logfile->buf);
>  		oflags |= O_CREAT;
>  	}
>  
> -	logfd = open(logfile, oflags, 0666);
> +	logfd = open(logfile->buf, oflags, 0666);
>  	if (logfd < 0) {
>  		if (!(oflags & O_CREAT) && errno == ENOENT)
>  			return 0;
>  
>  		if ((oflags & O_CREAT) && errno == EISDIR) {
> -			if (remove_empty_directories(logfile)) {
> +			if (remove_empty_directories(logfile->buf)) {
>  				return error("There are still logs under '%s'",
> -					     logfile);
> +					     logfile->buf);
>  			}
> -			logfd = open(logfile, oflags, 0666);
> +			logfd = open(logfile->buf, oflags, 0666);
>  		}
>  
>  		if (logfd < 0)
>  			return error("Unable to append to %s: %s",
> -				     logfile, strerror(errno));
> +				     logfile->buf, strerror(errno));
>  	}
>  
> -	adjust_shared_perm(logfile);
> +	adjust_shared_perm(logfile->buf);
>  	close(logfd);
>  	return 0;
>  }
> @@ -2762,20 +2774,22 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
>  	int logfd, result, written, oflags = O_APPEND | O_WRONLY;
>  	unsigned maxlen, len;
>  	int msglen;
> -	char log_file[PATH_MAX];
> +	struct strbuf sb_log_file = STRBUF_INIT;
> +	const char *log_file;
>  	char *logrec;
>  	const char *committer;
>  
>  	if (log_all_ref_updates < 0)
>  		log_all_ref_updates = !is_bare_repository();
>  
> -	result = log_ref_setup(refname, log_file, sizeof(log_file));
> +	result = log_ref_setup(refname, &sb_log_file);
>  	if (result)
> -		return result;
> +		goto done;
> +	log_file = sb_log_file.buf;
>  
>  	logfd = open(log_file, oflags);
>  	if (logfd < 0)
> -		return 0;
> +		goto done;
>  	msglen = msg ? strlen(msg) : 0;
>  	committer = git_committer_info(0);
>  	maxlen = strlen(committer) + msglen + 100;
> @@ -2788,9 +2802,13 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
>  		len += copy_msg(logrec + len - 1, msg) - 1;
>  	written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
>  	free(logrec);
> -	if (close(logfd) != 0 || written != len)
> -		return error("Unable to append to %s", log_file);
> -	return 0;
> +	if (close(logfd) != 0 || written != len) {
> +		error("Unable to append to %s", log_file);
> +		result = -1;
> +	}
> +done:
> +	strbuf_release(&sb_log_file);
> +	return result;
>  }
>  
>  static int is_branch(const char *refname)
> diff --git a/refs.h b/refs.h
> index 87a1a79..783033a 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -166,7 +166,7 @@ extern void unlock_ref(struct ref_lock *lock);
>  extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
>  
>  /** Setup reflog before using. **/
> -int log_ref_setup(const char *ref_name, char *logfile, int bufsize);
> +int log_ref_setup(const char *ref_name, struct strbuf *logfile);
>  
>  /** Reads log for the value of ref during at_time. **/
>  extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,