Re: [PATCH v5 04/24] files-backend: convert git_path() to strbuf_git_path()

On Wed, Mar 1, 2017 at 12:06 AM, Michael Haggerty [off-list ref] wrote:

quoted

@@ -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...

quoted

+     strbuf_release(&sb_newref);
+     strbuf_release(&sb_oldref);
+     strbuf_release(&tmp_renamed_log);

...and change this to `return ret`?

If that's ok with you, will do. I kept the patch dumb so the reader
does not have to keep track of many things when they check if there
are any behavior changes.

quoted

 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.)

Yeah you're right. I guess this code was written when
safe_create_dir() didn't do that. That certainly shortens this patch.
-- 
Duy