On Wed, Mar 1, 2017 at 12:06 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
>> @@ -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`?
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.
>> 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