Re: [PATCH] builtin: replace the_repository parameter in is_bare_repository()
- From
Hardik Kumar <hardikxk@gmail.com>
- Date
- Aug 27, 2026, 20:09 UTC
- Message-ID
- <DKZZYSTLY6TX.2TDQEBBOG5IAV@gmail.com>
- In-Reply-To
- <xmqqh5kf8hqc.fsf@gitster.g>
On Fri Aug 28, 2026 at 1:21 AM IST, Junio C Hamano wrote:
Show 34 quoted lines
> I guess this was a bit too short, so let me explain in a bit more
> detail.
>
>>> diff --git a/builtin/blame.c b/builtin/blame.c
>>> index 48d5251c6d..dbf4b4ffc7 100644
>>> --- a/builtin/blame.c
>>> +++ b/builtin/blame.c
>>> @@ -957,7 +957,7 @@ static void build_ignorelist(struct blame_scoreboard *sb,
>>> int cmd_blame(int argc,
>>> const char **argv,
>>> const char *prefix,
>>> - struct repository *repo UNUSED)
>>> + struct repository *repo)
>>> {
>>> struct rev_info revs;
>>> char *path = NULL;
>>> @@ -1187,7 +1187,7 @@ int cmd_blame(int argc,
>>>
>>> revs.disable_stdin = 1;
>>> setup_revisions(argc, argv, &revs, NULL);
>>> - if (!revs.pending.nr && is_bare_repository(the_repository)) {
>>> + if (!revs.pending.nr && is_bare_repository(repo)) {
>>> struct commit *head_commit;
>>> struct object_id head_oid;
>
> There are a handful of uses of the_repository before the execution
> reaches here. But you left them unmodified.
>
> The original code used to consistently used the_repository. Here
> you changed it to use "repo". In practice, they are most likely the
> same when "repo" is not NULL, so in that sense, this may not be
> breaking anything, but you must ask yourself what the point is,
> unless you convert all uses of the_repository with "repo". It does
> not help libification effort at all.My main goal here was to start small with by removing the dependence from `the_repository`. The other instances are obivous left out which makes the patch seem inconsistent but I suppose those functions are the ones that require execution before the flow ever checks for the value of `repo` and so they rely on the global one instead.
Show 5 quoted lines
> In general, builtin/foo.c::cmd_foo() are concrete programs that work > on specific repository (i.e., the_repository), and there is not much > reason to rewrite the use of the_repository to use "repo" given by > the caller which is git potty. You'd also need to deal with the > case where "repo" is NULL (hint: "cd / && git foo -h").
Right, but would safety check be required for single instance or better to find and work on only the specific ones which could lead to an exception.
> > They are quite different from other parts of the system, things > outside builtin/, many of which are general utility/helper routines, > many of which should be designed to work with given repository.