Re: [PATCH v3 04/16] files-backend: replace *git_path*() with files_path()

Nguyễn Thái Ngọc Duy  [off-list ref] 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 <redacted>
---
 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.

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.

So is this one; lock_ref_sha1_basic() is protected with assert-main-repo.

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.

This is safe, as repack_without_refs() asserts that it is main-repo
only.

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.