On Thu, Nov 07, 2024 at 02:46:15PM +0900, Junio C Hamano wrote:
[snip]
> > 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
