From: Mark Levedahl <mlevedahl@gmail.com>
Date: Sat, 16 May 2026 14:38:48 GMT
Subject: Re: [PATCH v1 07/11] git-gui: use rev-parse exclusively to find a repository
Message-ID: <62ed0001-5769-47f7-b31d-4948d498555c@gmail.com>
In-Reply-To: <d8844726-0b08-4035-946e-c5ada0759f32@kdbg.org>



On 5/15/26 12:06 PM, Johannes Sixt wrote:
> Am 14.05.26 um 16:33 schrieb Mark Levedahl:
>> git-gui attempts to use env(GIT_DIR) directly as the git repository,
>> accepting GIT_DIR if it is a directory. Only if that fails is git
>> rev-parse used to discover the repository.  But, this avoids all of
>> git-core's validity checking on a repository, thus possibly deferring an
>> error to a later step, possibly unexpected. Repository validation should
>> be part of initial setup so that later processing does not need error
>> trapping for configuration errors.
> OK. If the user gave us GIT_DIR with our without GIT_WORK_TREE, then
> that combination better be workable.
>
>> Let's just invoke rev-parse so all error checking is done. Stop here if
>> the user set GIT_DIR or GIT_WORK_TREE. Otherwise, continue the existing
>> behavior and show the repository picker.
> OK. But the paragraph is confusing, because a big "If an error occurs"
> is missing after the first sentence.
will fix.
>> Also, remove a later check on whether _gitdir is a directory: that code
>> cannot be reached without rev-parse having validating the repository.
> Good.
>
>> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
>> ---
>>  git-gui.sh | 24 +++++++++---------------
>>  1 file changed, 9 insertions(+), 15 deletions(-)
>>
>> diff --git a/git-gui.sh b/git-gui.sh
>> index 2e2ddc0..81789dd 100755
>> --- a/git-gui.sh
>> +++ b/git-gui.sh
>> @@ -374,6 +374,7 @@ set _gitdir {}
>>  set _gitworktree {}
>>  set _isbare {}
>>  set _githtmldir {}
>> +set _prefix {}
>>  set _reponame {}
>>  set _shellpath {@@SHELL_PATH@@}
>>  
>> @@ -1167,19 +1168,18 @@ proc pick_repo {} {
>>  	set picked 1
>>  }
>>  
>> +# find repository.
>>  if {[catch {
>> -		set _gitdir $env(GIT_DIR)
>> -		set _prefix {}
>> -		}]
>> -	&& [catch {
>> -		# beware that from the .git dir this sets _gitdir to .
>> -		# and _prefix to the empty string
>> -		set _gitdir [git rev-parse --absolute-git-dir]
>> -		set _prefix [git rev-parse --show-prefix]
>> -	} err]} {
>> +	set _gitdir [git rev-parse --absolute-git-dir]
> Please do also set _prefix. It should fix the bug that the file chooser
> uses an empty prefix after
>
> cd lib
> GIT_DIR=$PWD/../.git GIT_WORK_TREE=$PWD/.. ../git-gui.sh browser master .
>
> (this is an old bug.)
>
> Please keep the additional indentation of the catch body.
>
>> +} err]} {
>> +	if {[is_gitvars_error $err]} {
>> +		exit 1
>> +	} else {
>>  		pick_repo
>> +	}
> Treat the 'if' as an early exist without an else, and we don't need the
> previously strange indentation of 'pick_repo'.
>
>>  }
>>  
>> +
>>  # Use object format as hash algorithm (either "sha1" or "sha256")
>>  set hashalgorithm [git rev-parse --show-object-format]
>>  if {$hashalgorithm eq "sha1"} {
>> @@ -1191,12 +1191,6 @@ if {$hashalgorithm eq "sha1"} {
>>  	exit 1
>>  }
>>  
>> -if {![file isdirectory $_gitdir]} {
>> -	catch {wm withdraw .}
>> -	error_popup [strcat [mc "Git directory not found:"] "\n\n$_gitdir"]
>> -	exit 1
>> -}
>> -
>>  # _gitdir exists, so try loading the config
>>  load_config 0
>>  apply_config

will fix all.

Mark