Re: [PATCH v2 05/20] path: stop relying on `the_repository` when reporting garbage

On Wed, Aug 14, 2024 at 06:28:50PM +0000, Calvin Wan wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

We access `the_repository` in `report_linked_checkout_garbage()` both
directly and indirectly via `get_git_dir()`. Remove this dependency by
instead passing a `struct repository` as parameter.

Signed-off-by: Patrick Steinhardt <redacted>
---
 builtin/count-objects.c | 2 +-
 path.c                  | 6 +++---
 path.h                  | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/path.c b/path.c
index 069db6ff8f..97a07fafc7 100644
--- a/path.c
+++ b/path.c

@@ -365,15 +365,15 @@ static void update_common_dir(struct strbuf *buf, int git_dir_len,
 		strbuf_addstr(buf, LOCK_SUFFIX);
 }
 
-void report_linked_checkout_garbage(void)
+void report_linked_checkout_garbage(struct repository *r)
 {
 	struct strbuf sb = STRBUF_INIT;
 	const struct common_dir *p;
 	int len;
 
-	if (!the_repository->different_commondir)
+	if (!r->different_commondir)
 		return;
-	strbuf_addf(&sb, "%s/", get_git_dir());
+	strbuf_addf(&sb, "%s/", r->gitdir);
 	len = sb.len;
 	for (p = common_list; p->path; p++) {
 		const char *path = p->path;

Callers have two options here for accessing the gitdir: one is including
environment.h and calling get_git_dir(). The other is passing in `struct
repository *r` and accessing r->gitdir. It's not entirely clear which
should be used in what scenario. Sure with the second option the user
has the option of passing something in that isn't `the_repository` but
practically speaking that's not happening here and also in the large
majority of other scenarios.

I'm OK with this patch for the purposes of the series, but do you think
in the future we should introduce get_git_dir(struct repository *r) and
change get_git_dir() into get_env_git_dir() that simply calls
get_git_dir(the_repository)?

Good question. `get_git_dir()` is implemented like this:

        const char *get_git_dir(void)
        {
            if (!the_repository->gitdir)
                BUG("git environment hasn't been setup");
            return the_repository->gitdir;
        }

We could of course introduce something like `repo_get_git_dir()` that
takes an arbitrary repository as input, like this:

        const char *repo_get_git_dir(struct repository *r)
        {
            if (!r->gitdir)
                BUG("git environment hasn't been setup");
            return r->gitdir;
        }

The only benefit that this has is that we double check whether
`r->gitdir` has actually been set in the first place. I don't really
think that check is all that useful though, because I expect that the
caller either has a repository, and given that a repository always has a
`gitdir` I'd always expect it to be populated. Or they don't have a
gitdir, but in that case they cannot call `repo_get_git_dir()` in the
first place because we'd already segfault before hitting the BUG when
trying to dereference a NULL pointer.

So I'm not entirely sure of the benefit of such a function over just
accessing `r->gitdir` directly.

Patrick