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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 31, 2025, 19:26 UTC
Message-ID: <xmqq8qqq943u.fsf@gitster.g>
In-Reply-To: <xmqqcyg294ft.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 51 quoted lines

> "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.