On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> git_path() and friends are going to be killed in files-backend.c in near
> future. And because there's a risk with overwriting buffer in
> git_path(), let's convert them all to strbuf_git_path(). We'll have
> easier time killing/converting strbuf_git_path() then because we won't
> have to worry about memory management again.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
> refs/files-backend.c | 139 +++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 106 insertions(+), 33 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 4676525de..435db1293 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> [...]
> @@ -2586,9 +2603,15 @@ static int files_rename_ref(struct ref_store *ref_store,
> int flag = 0, logmoved = 0;
> struct ref_lock *lock;
> struct stat loginfo;
> - int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
> + struct strbuf sb_oldref = STRBUF_INIT;
> + struct strbuf sb_newref = STRBUF_INIT;
> + struct strbuf tmp_renamed_log = STRBUF_INIT;
> + int log, ret;
> struct strbuf err = STRBUF_INIT;
>
> + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
> + log = !lstat(sb_oldref.buf, &loginfo);
> + strbuf_release(&sb_oldref);
> if (log && S_ISLNK(loginfo.st_mode))
> return error("reflog for %s is a symlink", oldrefname);
>
> @@ -2602,7 +2625,12 @@ static int files_rename_ref(struct ref_store *ref_store,
> if (!rename_ref_available(oldrefname, newrefname))
> return 1;
>
> - if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG)))
> + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
> + strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
> + ret = log && rename(sb_oldref.buf, tmp_renamed_log.buf);
> + strbuf_release(&sb_oldref);
> + strbuf_release(&tmp_renamed_log);
> + if (ret)
> return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
> oldrefname, strerror(errno));
>
> @@ -2681,13 +2709,19 @@ static int files_rename_ref(struct ref_store *ref_store,
> log_all_ref_updates = flag;
>
> rollbacklog:
> - if (logmoved && rename(git_path("logs/%s", newrefname), git_path("logs/%s", oldrefname)))
> + strbuf_git_path(&sb_newref, "logs/%s", newrefname);
> + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
> + if (logmoved && rename(sb_newref.buf, sb_oldref.buf))
> error("unable to restore logfile %s from %s: %s",
> oldrefname, newrefname, strerror(errno));
> + strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
> if (!logmoved && log &&
> - rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", oldrefname)))
> + rename(tmp_renamed_log.buf, sb_oldref.buf))
> error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
> oldrefname, strerror(errno));
It feels like you're writing, releasing, re-writing these strbufs more
than necessary. Maybe it would be clearer to set them once, and on
errors set `ret = error()` then jump to a label here...
> + strbuf_release(&sb_newref);
> + strbuf_release(&sb_oldref);
> + strbuf_release(&tmp_renamed_log);
>
...and change this to `return ret`?
> return 1;
> }
> [...]
> @@ -4108,18 +4171,28 @@ static int files_reflog_expire(struct ref_store *ref_store,
>
> static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
> {
> + struct strbuf sb = STRBUF_INIT;
> +
> /* Check validity (but we don't need the result): */
> files_downcast(ref_store, 0, "init_db");
>
> /*
> * Create .git/refs/{heads,tags}
> */
> - safe_create_dir(git_path("refs/heads"), 1);
> - safe_create_dir(git_path("refs/tags"), 1);
> + strbuf_git_path(&sb, "refs/heads");
> + safe_create_dir(sb.buf, 1);
> + strbuf_reset(&sb);
> + strbuf_git_path(&sb, "refs/tags");
> + safe_create_dir(sb.buf, 1);
> + strbuf_reset(&sb);
> if (get_shared_repository()) {
> - adjust_shared_perm(git_path("refs/heads"));
> - adjust_shared_perm(git_path("refs/tags"));
> + strbuf_git_path(&sb, "refs/heads");
> + adjust_shared_perm(sb.buf);
> + strbuf_reset(&sb);
> + strbuf_git_path(&sb, "refs/tags");
> + adjust_shared_perm(sb.buf);
> }
> + strbuf_release(&sb);
> return 0;
> }
It looks to me like `safe_create_dir()` already has the ability to
`adjust_shared_perm()`, or am I missing something? (I realize that this
is preexisting code.)
Michael