Re: [PATCH v1 10/11] git-gui: improve worktree discovery

[Mark Levedahl <mlevedahl@gmail.com>]

On 5/16/26 4:16 AM, Johannes Sixt wrote:
> Am 14.05.26 um 16:33 schrieb Mark Levedahl:
>> git gui's worktree discovery needs update based upon prior work in this
>> series. In the normal case, all information we need comes directly from
>> git rev-parse (--show-toplevel, and --show-prefix). Should this work, we
>> have a valid worktree and all git gui commands can run.
>>
>> If not, we need to consider:
>> - if GIT_DIR or GIT_WORK_TREE are in the environment, just stop as we
>>   the input configuration was wrong, the user must fix that.
>> - if we have a browser or blame subcommand, no worktree is needed so
>>   git-gui can run without.
>> - using the git repository's parent is a valid worktree (if possible),
>>   restoring prior behavior.
>>
>> The current directory should be either the root of the worktree, if one
>> is found, or the top-level of the git repository.
> I disagree in the case where no working tree is found. Then there is no
> point in changing the current directory.

git-gui always changes to root of the worktree if that is found.
Failing to cd to the root of the repository when operating with no worktree opens the
possibility th
>
>> Make it so. Also, make worktree discover directly follow repository
>> discovery, reducing the locations that might need error trapping to
>> catch configuration issues.
> Good!
>
>> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> BTW, please make the subject line more descriptive. The word "improve"
> does not convey anything of importance.
>
>> ---
>>  git-gui.sh | 56 ++++++++++++++++++++++--------------------------------
>>  1 file changed, 23 insertions(+), 33 deletions(-)
>>
>> diff --git a/git-gui.sh b/git-gui.sh
>> index e326401..3a83dd5 100755
>> --- a/git-gui.sh
>> +++ b/git-gui.sh
>> @@ -1173,6 +1173,28 @@ if {[catch {
>>  	}
>>  }
>>  
>> +# find worktree, continue without if not required
>> +if {[catch {
>> +	set _gitworktree [git rev-parse --show-toplevel]
>> +	set _prefix [git rev-parse --show-prefix]
>> +	cd $_gitworktree
>> +} err]} {
> We have three commands, each with their own possible failure sources.
> One of the outcomes of an error is that we proceed anyway. I think that
> this is incorrect if the 'cd' fails: we must not proceed if it fails.
> Therefore, we must handle its failure separately.
>
>> +	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.

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.
    

>> +	} 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.

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

I will clarify this explicitly in the commit message.

>> @@ -1189,37 +1211,8 @@ if {$hashalgorithm eq "sha1"} {
>>  load_config 0
>>  apply_config
>>  
>> -set _gitworktree [git rev-parse --show-toplevel]
>>  
>> -if {$_prefix ne {}} {
>> -	if {$_gitworktree eq {}} {
>> -		regsub -all {[^/]+/} $_prefix ../ cdup
>> -	} else {
>> -		set cdup $_gitworktree
>> -	}
>> -	if {[catch {cd $cdup} err]} {
>> -		catch {wm withdraw .}
>> -		error_popup [strcat [mc "Cannot move to top of working directory:"] "\n\n$err"]
>> -		exit 1
>> -	}
>> -	set _gitworktree [pwd]
>> -	unset cdup
>> -} elseif {![is_enabled bare]} {
>> -	if {[is_bare]} {
>> -		catch {wm withdraw .}
>> -		error_popup [strcat [mc "Cannot use bare repository:"] "\n\n$_gitdir"]
>> -		exit 1
>> -	}
>> -	if {$_gitworktree eq {}} {
>> -		set _gitworktree [file dirname $_gitdir]
>> -	}
>> -	if {[catch {cd $_gitworktree} err]} {
>> -		catch {wm withdraw .}
>> -		error_popup [strcat [mc "No working directory"] " $_gitworktree:\n\n$err"]
>> -		exit 1
>> -	}
>> -	set _gitworktree [pwd]
>> -}
>> +# Derive a human-readable repository name
>>  set _reponame [file split [file normalize $_gitdir]]
>>  if {[lindex $_reponame end] eq {.git}} {
>>  	set _reponame [lindex $_reponame end-1]
>> @@ -1227,9 +1220,6 @@ if {[lindex $_reponame end] eq {.git}} {
>>  	set _reponame [lindex $_reponame end]
>>  }
>>  
>> -# Export the final paths
>> -set_gitdir_vars
>> -
>>  ######################################################################
>>  ##
>>  ## global init
> -- Hannes
>