From: Johannes Sixt <j6t@kdbg.org>
Date: Tue, 19 May 2026 08:16:11 GMT
Subject: Re: [PATCH v1 10/11] git-gui: improve worktree discovery
Message-ID: <5081fcc5-19b5-49aa-a33c-2c13aba7edb1@kdbg.org>
In-Reply-To: <f654eab3-2157-4591-9ae0-440efb052e8e@gmail.com>

Am 16.05.26 um 17:28 schrieb Mark Levedahl:
> On 5/16/26 4:16 AM, Johannes Sixt wrote:
>> Am 14.05.26 um 16:33 schrieb Mark Levedahl:
>>> +	if {[is_gitvars_error $err]} {
>>> +		exit 1
>>> +	}
>>> +	set _gitworktree {}
>>> +	set _prefix {}
>>> +	if {[is_enabled bare]} {
>>> +		cd $_gitdir
>> Why change the directory here? If we run `git gui browser master dir` we
>> do not want to change the directory in an uncontrolled manner. The
>> argument parser will want to check for the existence of files, and then
>> we do not want to operate from a random directory.
>>
>> Also, I think that the check must be for [is_bare] and not [is_enabled
>> bare].
> 
> [is_enabled_bare] is correct. This code handles the case: 
>     - neither the startup directory nor GIT_WORK_TREE are useable worktrees, so [is_bare]
> is currently true.
>     - the command given is browser or blame so a worktree is not needed. We can proceed.

But in the case where the command is browser or blame, the argument
parser must later check for the existence of files, provided that a
worktree is present. But this conditional would change directory to
somewhere that is not a worktree at all even though a worktree is
available. So, I am still convinced that [is_bare] is correct.

> The bigger question is whether to change directory at all: git-gui should never touch
> files that are neither in the worktree nor in the repository. Leaving the current
> directory as neither of those could be troublesome. I have no strong feeling here though,
> will delete this.

OK. We need the conditional, but not change the directory.

>>> +	} elseif {![is_parent_worktree]} {
>>> +		catch {wm withdraw .}
>>> +		error_popup [strcat [mc "Cannot use bare repository:"] "\n\n" $_gitdir]
>>> +		exit 1
>>> +	}
>>> +}
>>> +
>>> +# repository and worktree config are complete, export them
>>> +set_gitdir_vars
>>>  
>>>  # Use object format as hash algorithm (either "sha1" or "sha256")
>>>  set hashalgorithm [git rev-parse --show-object-format]
>> This moves code around. In particular, we see load_config and
>> apply_config in the context below, which now happens only after these
>> calls. How certain are we that these have no effect on the code that
>> runs now earlier?
> We need to load the system and user global config before running the repository picker. We
> (re-) load the full config including the repository after we have a repository. I think
> this is correct: git-config explicitly lists worktree dependent includeif statements,
> meaning the config can be worktree dependent, and we must not load the final config until
> repository and worktree discovery are complete.

Good point.

> 
> git rev-parse, etc., perform discovery and config file loading each time they are invoked,
> those are unaffected by git-gui's internal config.

Also very true. Only calls to proc get_config would be affected by a
different setup, but there are none in the code that has swapped places
with working tree discovery. Only --show-object-format would be affected
by the discovered environment, and for that case it is more correct to
operate in the final setup.

-- Hannes