Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> This centralizes all path rewriting of files-backend.c in one place so
> we have easier time removing the path rewriting later. There could be
> some hidden indirect git_path() though, I didn't audit the code to the
> bottom.
>
> Side note: set_worktree_head_symref() is a bad boy and should not be in
> files-backend.c (probably should not exist in the first place). But
> we'll leave it there until we have better multi-worktree support in refs
> before we update it.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
> refs/files-backend.c | 185 ++++++++++++++++++++++++++-------------------------
> 1 file changed, 94 insertions(+), 91 deletions(-)
In this step, files_path() is still "if refs->submodule field is
there, then use that to call strbuf_git_path_submodule() and
otherwise call strbuf_git_path()." That is a very sensible
refactoring for things like packed-refs-file in this hunk:
> static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *refs)
> {
> char *packed_refs_file;
> + struct strbuf sb = STRBUF_INIT;
>
> - if (refs->submodule)
> - packed_refs_file = git_pathdup_submodule(refs->submodule,
> - "packed-refs");
> - else
> - packed_refs_file = git_pathdup("packed-refs");
> + files_path(refs, &sb, "packed-refs");
> + packed_refs_file = strbuf_detach(&sb, NULL);
But the original code of some other changes do not follow that
pattern, e.g.
> @@ -1585,7 +1578,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
> *lock_p = lock = xcalloc(1, sizeof(*lock));
>
> lock->ref_name = xstrdup(refname);
> - strbuf_git_path(&ref_file, "%s", refname);
> + files_path(refs, &ref_file, "%s", refname);
Is it the right way to review these changes to make sure that a
conversion from the original that is an unconditional
strbuf_git_path() to files_path() happens only if the function is
"files-assert-main-repository" clean? lock_raw_ref() certainly is
one of those functions where the caller should not have a non-empty
submodule field in refs.
> @@ -2052,7 +2045,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
> if (flags & REF_DELETING)
> resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
>
> - strbuf_git_path(&ref_file, "%s", refname);
> + files_path(refs, &ref_file, "%s", refname);
So is this one; lock_ref_sha1_basic() is protected with assert-main-repo.
> @@ -2343,7 +2336,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
> * Remove empty parents, but spare refs/ and immediate subdirs.
> * Note: munges *name.
> */
> -static void try_remove_empty_parents(char *name)
> +static void try_remove_empty_parents(struct files_ref_store *refs, char *name)
> {
> char *p, *q;
> int i;
> @@ -2368,7 +2361,7 @@ static void try_remove_empty_parents(char *name)
> if (q == p)
> break;
> *q = '\0';
> - strbuf_git_path(&sb, "%s", name);
> + files_path(refs, &sb, "%s", name);
But here it gets iffy. try_remove_empty_parents() itself does not
assert, and its sole caller prune_ref() does not, either. The sole
caller of prune_ref() which is prune_refs() does not. As we climb
the call chain up, we reach files_pack_refs(). Am I confused to
doubt that the method is inherently main-repo only?
... ah, OK, files_downcast() at the beginning of pack_refs
forbids submodule. So this is safe.
> @@ -2462,7 +2455,7 @@ static int repack_without_refs(struct files_ref_store *refs,
> if (lock_packed_refs(refs, 0)) {
> struct strbuf sb = STRBUF_INIT;
>
> - strbuf_git_path(&sb, "packed-refs");
> + files_path(refs, &sb, "packed-refs");
This is safe, as repack_without_refs() asserts that it is main-repo
only.
> @@ -2558,17 +2551,17 @@ static int files_delete_refs(struct ref_store *ref_store,
> */
> #define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"
>
> -static int rename_tmp_log(const char *newrefname)
> +static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname)
> {
The sole caller files_rename_ref() is main-repo only and that is
guaranteed when downcast is done.
> -static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
> +static int log_ref_setup(struct files_ref_store *refs, const char *refname,
> + struct strbuf *logfile, struct strbuf *err,
> + int force_create)
> {
> int logfd, oflags = O_APPEND | O_WRONLY;
>
> - strbuf_git_path(logfile, "logs/%s", refname);
> + files_path(refs, logfile, "logs/%s", refname);
This and friends of log_ref_write() eventually rolls up to
commit_ref_update() that has the main-repo only assertion, so they
should be safe.
Another entry point files_create_symref() via create_symref_locked()
also reaches log_ref_write() and friends but the safety is guaranteed
via the downcast that asserts.
OK, overall I really like the loss of "Check the validity but we do
not need the result" with this step. The same checks are still done
but the code looks much less hacky.