On Wed, Aug 14, 2024 at 06:28:50PM +0000, Calvin Wan wrote:
> Patrick Steinhardt <ps@xxxxxx> 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@xxxxxx>
> > ---
> > 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
