Re: [PATCH 1/3] git: remove is_bare_repository_cfg global variable

From: shejialuo <shejialuo@gmail.com>
Date: Nov 7, 2024, 16:04 UTC
Message-ID: <ZyzlBZnL-K3S7Env@ArchLinux>
In-Reply-To: <xmqqv7wzsijc.fsf@gitster.g>

On Thu, Nov 07, 2024 at 02:46:15PM +0900, Junio C Hamano wrote:

[snip]

Show 17 quoted lines

> >  int repo_init(struct repository *repo,
> >  	      const char *gitdir,
> > -	      const char *worktree)
> > +	      const char *worktree,
> > +	      int is_bare)
> >  {
> >  	struct repository_format format = REPOSITORY_FORMAT_INIT;
> >  	memset(repo, 0, sizeof(*repo));
> > @@ -283,6 +288,8 @@ int repo_init(struct repository *repo,
> >  	repo_set_compat_hash_algo(repo, format.compat_hash_algo);
> >  	repo_set_ref_storage_format(repo, format.ref_storage_format);
> >  	repo->repository_format_worktree_config = format.worktree_config;
> > +	if (is_bare > 0)
> > +		repo->is_bare_cfg = is_bare;
> 
> When repo_init() is called with anything other than &the_repo, who
> initializes repo->is_bare_cfg?

I also want to ask this question. Actually, I feel quite strange about why we need to add a new parameter `is_bare` to `repo_init` function.

For this call:

    repo_init(the_repository, git_dir, work_tree, -1);

We add a new field "is_bare_cfg" to the "struct repository". So, at now, `the_repository` variable should contain the information about whether the repo is bare(1), is not bare(0) or unknown(-1). However, in this call, we pass "-1" to the parameter `is_bare` for "repo_init" function.

When I first look at this code, I have thought that we will set "repo->is_bare_cfg = -1" to indicate that we cannot tell whether the repo is bare or not. But it just sets the "repo->is_bare_cfg = is_bare" if `bare > 0`. Junio has already commented on this.

This raises a question: why we need to set up `is_bare_cfg` in the `repo_init` function? I guess this is because we need to set up other "struct repository" parameter like the following:

    if (repo_init(&alternate, sb.buf, NULL, the_repository->is_bare_cfg) < 0)

And I think it's better for us to use the following way.

    alternate->is_bare_cfg = the_repository->is_bare_cfg;
    if (repo_init(&alternate, sb.buf, NULL))

And we may create a function called `repo_copy_settings` to set up the common setting inherited from an existing repo:

    repo_copy_settings(alternate, the_repository);
    if (repo_init(&alternate, sb.buf, NULL))

I agree that we could put `is_bare_cfg` to "struct repository *". But I don't agree with the idea that we need to pass `is_bare` to `repo_init`. I think we should know whether the repo is bare or not before calling `repo_init`. And from my understanding, this is what we are doing now.

Also, I think we may add a enum type instead of using (-1, 0, 1). (However, this is not the main point of this patch).

Thanks, Jialuo