Re: [PATCH v3] worktree: detect from secondary worktree if main worktree is bare

>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Olga Pilipenco via GitGitGadget" <gitgitgadget@gmail.com> 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.