On Fri, Jan 31, 2025 at 12:26 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > "Olga Pilipenco via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> >
> >> +/*
> >> +* When in a secondary worktree, and when extensions.worktreeConfig
> >> +* is true, only $commondir/config and $commondir/worktrees/<id>/
> >> +* config.worktree are consulted, hence any core.bare=true setting in
> >> +* $commondir/config.worktree gets overlooked. Thus, check it manually
> >> +* to determine if the repository is bare.
> >> +*/
> >> +static int is_main_worktree_bare(struct repository *repo)
> >> +{
> >> + int bare = 0;
> >> + struct config_set cs = {0};
> >> + char *worktree_config = xstrfmt("%s/config.worktree", repo_get_common_dir(repo));
> >> +
> >> + git_configset_init(&cs);
> >> + git_configset_add_file(&cs, worktree_config);
> >> + git_configset_get_bool(&cs, "core.bare", &bare);
> >> +
> >> + git_configset_clear(&cs);
> >> + free(worktree_config);
> >> + return bare;
> >> +}
> >
> > That is nicely described.
> >
> >> /**
> >> * get the main worktree
> >> */
> >> @@ -79,16 +101,11 @@ static struct worktree *get_main_worktree(int skip_reading_head)
> >> CALLOC_ARRAY(worktree, 1);
> >> worktree->repo = the_repository;
> >> worktree->path = strbuf_detach(&worktree_path, NULL);
> >> - /*
> >> - * NEEDSWORK: If this function is called from a secondary worktree and
> >> - * config.worktree is present, is_bare_repository_cfg will reflect the
> >> - * contents of config.worktree, not the contents of the main worktree.
> >> - * This means that worktree->is_bare may be set to 0 even if the main
> >> - * worktree is configured to be bare.
> >> - */
> >> - worktree->is_bare = (is_bare_repository_cfg == 1) ||
> >> - is_bare_repository();
> >> worktree->is_current = is_current_worktree(worktree);
> >> + worktree->is_bare = (is_bare_repository_cfg == 1) ||
> >> + is_bare_repository() ||
> >> + (!worktree->is_current && is_main_worktree_bare(the_repository));
> >
> > Is "this worktree does not have is_current bit set" equivalent to
> > "this worktree is the main one, so is_main_worktree_bare() needs to
> > be consulted"? That linkage between "the is_current bit unset" and
> > "is the main worktree" is not obvious to me.
>
> Does the thinking behind it go like this?
>
> We grabbed the "main" worktree object and stored it in worktree;
> it is either our current worktree (in which case is_current is
> true), or it is not (in which case, is_current is false). We
> know that the old logic failed when asking the "is it bare"
> question from a secondary worktree. !worktree->is_current tells
> us that we _are_ asking the question from a secondary worktree,
> so we need to make the extra call to check config.worktree file
> as well in that case.
>
> Perhaps the logic is clear to those who diagnosed the problem, wrote
> the patch, and reviewed it, in which case there is no reason to
> reroll. Perhaps it was just me to whom it was not obvious that
> the purpose of "is_current" check was not about "are we looking at
> the main worktree" but was about "if we are not in the main worktree,
> we need this extra check".
>
> Thanks.
You did a great job figuring it out and I agree it's confusing at
first, but we tried
our best to make it less confusing.
`is_current` check is actually not necessary there, but having it there saves
extra unnecessary calculations, also describes & fixes the exact scenario
that didn't work (not being able to see main worktree as bare from a
secondary worktree).
Thanks for looking into that.
