Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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,
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html