Re: [PATCH v2 02/12] builtin/verify-tag: stop using `the_repository`

From: Usman Akinyemi <usmanakinyemi202@gmail.com>
Date: Feb 27, 2025, 17:56 UTC
Message-ID: <CAPSxiM-bOcqEbVNAYxh0oPXKceN7YSrCU_37e7mXAbxwpq3QNQ@mail.gmail.com>
In-Reply-To: <xmqq34g8ei5x.fsf@gitster.g>

On Thu, Feb 20, 2025 at 9:13 PM Junio C Hamano <gitster@pobox.com> wrote:

>
> Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
>

Hi Junio.

Show 23 quoted lines

> > @@ -23,7 +22,7 @@ static const char * const verify_tag_usage[] = {
> >  int cmd_verify_tag(int argc,
> >                  const char **argv,
> >                  const char *prefix,
> > -                struct repository *repo UNUSED)
> > +                struct repository *repo)
> >  {
> >       int i = 1, verbose = 0, had_error = 0;
> >       unsigned flags = 0;
> > @@ -50,13 +49,13 @@ int cmd_verify_tag(int argc,
> >               flags |= GPG_VERIFY_OMIT_STATUS;
> >       }
> >
> > -     git_config(git_default_config, NULL);
> > +     repo_config(repo, git_default_config, NULL);
>
> I seriously think that it is a horrible idea (but the previous step
> of this series is hardly the first one that commits the same sin) to
> move git_config() down only to deal with "repo might be NULL if run
> outside a repository".  We should stop making such changes, and we
> should revert the changes we already made along that line, to solve
> it differently.
>

Yeah, I agree with this after going through your comment on the other patch. We should look for a better solution.

Show 10 quoted lines

> Wouldn't it work much better if we teach repo_config() to allow repo
> to be NULL to signal that we are outside any repository, and behave
> the same way the current git_config() works when called outside a
> repository?  Even though the function is called repo_config(), it is
> *NOT* limited to read from $GIT_DIR/config but does read from the
> usual "repository configuration trumps per-user configuration which
> trumps system-side configuration" cascade, so it is natural to skip
> the repository configuration when called outside any repository but
> read the other configuration sources, which should be what happens
> when git_config() is called from outside the repository, no?

Yeah, I was studying the config.c and config.h files to understand better how all these functions work.

The git_config() when called outside the repository, uses the global the_repository variable basically called the repo_config(). It does not necessarily handle any situation when the repo was NULL. It always uses the global the_repository variable. I do not think we want to handle the repo_config() the same as the point of all these are to reduce/remove the use of the_repository global variable.

While going through the config.c I saw the read_very_early_config() which read the config from the system and global settings and does not require any repo variable. I think, to teach the repo_config() to allow NULL value, we could call the read_very_early_config() whenever the repo is NULL as we know, this happens outside the repository. I sent a rfc patch for this and it can viewed here :- https://public-inbox.org/git/20250227175456.1129840-1-usmanakinyemi202@gmail.com/T/#u

Another approach which I was thinking about is having a local repository variable inside the repo_config() whenever the repo variable passed to it is NULL.

What do you think ?

Thanks.