On 13/02/17 15:20, 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 <[email protected]>
> ---
> refs/files-backend.c | 114
> ++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 90 insertions(+), 24 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 75565c3aa..6582c9b2d 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2169,6 +2169,8 @@ static int lock_packed_refs(struct files_ref_store
> *refs, int flags)
> static int timeout_configured = 0;
> static int timeout_value = 1000;
> struct packed_ref_cache *packed_ref_cache;
> + struct strbuf sb = STRBUF_INIT;
> + int ret;
>
> files_assert_main_repository(refs, "lock_packed_refs");
>
> @@ -2177,10 +2179,13 @@ static int lock_packed_refs(struct files_ref_store
> *refs, int flags)
> timeout_configured = 1;
> }
>
> - if (hold_lock_file_for_update_timeout(
> - &packlock, git_path("packed-refs"),
> - flags, timeout_value) < 0)
> + strbuf_git_path(&sb, "packed-refs");
> + ret = hold_lock_file_for_update_timeout(&packlock, sb.buf,
> + flags, timeout_value);
> + strbuf_release(&sb);
> + if (ret < 0)
> return -1;
> +
> /*
> * Get the current packed-refs while holding the lock. If the
> * packed-refs file has been modified since we last read it,
> @@ -2335,6 +2340,9 @@ static void try_remove_empty_parents(char *name)
> for (q = p; *q; q++)
> ;
> while (1) {
> + struct strbuf sb = STRBUF_INIT;
> + int ret;
> +
> while (q > p && *q != '/')
> q--;
> while (q > p && *(q-1) == '/')
> @@ -2342,7 +2350,10 @@ static void try_remove_empty_parents(char *name)
> if (q == p)
> break;
> *q = '\0';
> - if (rmdir(git_path("%s", name)))
> + strbuf_git_path(&sb, "%s", name);
> + ret = rmdir(sb.buf);
> + strbuf_release(&sb);
> + if (ret)
> break;
> }
> }
> @@ -2431,7 +2442,11 @@ static int repack_without_refs(struct files_ref_store
> *refs,
> return 0; /* no refname exists in packed refs */
>
> if (lock_packed_refs(refs, 0)) {
> - unable_to_lock_message(git_path("packed-refs"), errno, err);
> + struct strbuf sb = STRBUF_INIT;
> +
> + strbuf_git_path(&sb, "packed-refs");
> + unable_to_lock_message(sb.buf, errno, err);
> + strbuf_release(&sb);
> return -1;
> }
> packed = get_packed_refs(refs);
> @@ -2529,11 +2544,12 @@ static int rename_tmp_log(const char *newrefname)
> {
> int attempts_remaining = 4;
> struct strbuf path = STRBUF_INIT;
> + struct strbuf tmp_renamed_log = STRBUF_INIT;
> int ret = -1;
>
> - retry:
> - strbuf_reset(&path);
> strbuf_git_path(&path, "logs/%s", newrefname);
> + strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
> + retry:
> switch (safe_create_leading_directories_const(path.buf)) {
> case SCLD_OK:
> break; /* success */
> @@ -2546,7 +2562,7 @@ static int rename_tmp_log(const char *newrefname)
> goto out;
> }
>
> - if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
> + if (rename(tmp_renamed_log.buf, path.buf)) {
> if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining >
> 0) {
> /*
> * rename(a, b) when b is an existing
> @@ -2574,6 +2590,7 @@ static int rename_tmp_log(const char *newrefname)
> ret = 0;
> out:
> strbuf_release(&path);
> + strbuf_release(&tmp_renamed_log);
> return ret;
> }
>
> @@ -2614,9 +2631,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);
>
> @@ -2630,7 +2653,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);
It probably won't make too much difference, but the two code
sequences above are not similar in terms of side-effects when
'log' is false. In that case, the two calls to git_path() and
the call to rename() are not made in the original code. In the
new sequence, the two calls to strbuf_git_path() are always made
(but rename() is not).
> + 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));
>
> @@ -2709,13 +2737,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))
ditto
> 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))
similar
> error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
> oldrefname, strerror(errno));
> + strbuf_release(&sb_newref);
> + strbuf_release(&sb_oldref);
> + strbuf_release(&tmp_renamed_log);
>
> return 1;
> }
ATB,
Ramsay Jones
