| Message ID | bbaa85ebad458c60c59784b690be8be2ddbe76fc.1723540226.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 78f2210b3c8605144d62a90b58e888312f64efc8 |
| Headers | show |
| Series | Stop using `the_repository` in "config.c" | expand |
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)?
On Wed, Aug 14, 2024 at 06:28:50PM +0000, Calvin Wan wrote: > 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
diff --git a/builtin/count-objects.c b/builtin/count-objects.c index 2d4bb5e8d0..ec6098a149 100644 --- a/builtin/count-objects.c +++ b/builtin/count-objects.c @@ -113,7 +113,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix) usage_with_options(count_objects_usage, opts); if (verbose) { report_garbage = real_report_garbage; - report_linked_checkout_garbage(); + report_linked_checkout_garbage(the_repository); } for_each_loose_file_in_objdir(get_object_directory(), 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; diff --git a/path.h b/path.h index 05aff5f4c3..9a4a4a8fb3 100644 --- a/path.h +++ b/path.h @@ -158,7 +158,7 @@ int strbuf_git_path_submodule(struct strbuf *sb, const char *path, const char *fmt, ...) __attribute__((format (printf, 3, 4))); -void report_linked_checkout_garbage(void); +void report_linked_checkout_garbage(struct repository *r); /* * You can define a static memoized git path like:
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(-)