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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 15, 2024, 05:26 UTC
Message-ID: <Zr2RaQCbi3Tpstz7@tanuki>
In-Reply-To: <20240814182850.1273188-1-calvinwan@google.com>

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

Show 48 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> > 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 <ps@pks.im>
> > ---
> >  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