From: Hardik Kumar <hardikxk@gmail.com>
Date: Thu, 27 Aug 2026 20:09:43 GMT
Subject: Re: [PATCH] builtin: replace the_repository parameter in is_bare_repository()
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:

> 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.

> 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.