From: Johannes Sixt <j6t@kdbg.org>
Date: Fri, 22 May 2026 08:46:49 GMT
Subject: Re: [PATCH v2 04/11] git-gui: use rev-parse exclusively to find a repository
Message-ID: <8d1488ec-c4de-4ddd-b3cd-e1e8b4a343bf@kdbg.org>
In-Reply-To: <20260520202411.108764-5-mlevedahl@gmail.com>

Am 20.05.26 um 22:24 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.
> 
> Let's just invoke rev-parse so all error checking is done.
> 
> While here, let's cleanup the error handling.
> 
> Stop if an error occurs and the user set GIT_DIR or GIT_WORK_TREE.
> Use of either or both of those variables is supported by git, but their
> use also means the user has taken responsibility that they are correct,
> so a failure is something the user must address.

Very much so!

> 
> Otherwise on error, continue the existing behavior and show the
> repository picker. But, let's move the possible invocation of
> repository_chooser::pick to a separate code block. This permits adding
> separate conditions on using pick indepent of repository discovery, and
> will be exploited later in the series.  Note that the picker always
> returns with the current directory in the root of a worktree with the
> git repository is in the .git subdirectory.  The variable "picked" is
> used by git-gui to automatically execute the "Explore Working Copy" menu
> item after the repository picker is run.  This is controlled by config
> variable gui.autoexplore, and happens after all discovery is complete.
> 
> Remove a later check on whether _gitdir is a directory: that code
> cannot be reached without rev-parse already validating the repository.
> 
> _prefix should not be set before worktree discovery: the prefix is only
> known after the worktree is found, and at this point we have only
> discovered the repository.

Sorry, but I cannot agree with "prefix is only known after the worktree
is found". The prefix is a property that can be known even if we haven't
asked where the top-level of the working tree is. See more below.

> This is true even when running the repository
> picker: that option provides a list of prior selections, and does no
> validation on the list beyond checking that the directories exist.  For
> now, just initialize _prefix along with other global variables.
> 
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-gui.sh | 48 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 233c975786..c61a6cbd8f 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@@}
>  
> @@ -1122,6 +1123,24 @@ unset argv0dir
>  ##
>  ## repository setup
>  
> +proc is_gitvars_error {err} {
> +	set havevars 0
> +	set GIT_DIR {}
> +	set GIT_WORK_TREE {}
> +	catch {set GIT_DIR $::env(GIT_DIR); set havevars 1}
> +	catch {set GIT_WORK_TREE $::env(GIT_WORK_TREE) ; set havevars 1}
> +
> +	if {$havevars} {
> +		catch {wm withdraw .}
> +		error_popup [strcat [mc "Invalid configuration:"] \
> +		   "\n" "GIT_DIR: " $GIT_DIR \
> +		   "\n" "GIT_WORK_TREE: " $GIT_WORK_TREE \
> +			"\n\n$err"]
> +		return 1
> +	}
> +	return 0
> +}
> +
>  proc set_gitdir_vars {} {
>  	global _gitdir _gitworktree env
>  	if {$_gitdir ne {}} {
> @@ -1138,17 +1157,22 @@ proc unset_gitdir_vars {} {
>  	catch {unset env(GIT_WORK_TREE)}
>  }
>  
> -set picked 0
> -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
> +# find repository.
> +set _gitdir {}
> +if {$_gitdir eq {}} {
> +	if {[catch {
>  		set _gitdir [git rev-parse --absolute-git-dir]
> -		set _prefix [git rev-parse --show-prefix]

You cannot leave the _prefix empty, because it breaks `git gui browser
master dir` when invoked from a subdirectory of the working tree.

This line must remain. I see that you add it back in later patch. There
may be some motivation to move prefix discovery, but there is no
motivation to remove it at this point.

>  	} err]} {
> +		if {[is_gitvars_error $err]} {
> +			exit 1
> +		}
> +		set _gitdir {}

BTW, this line would only be needed if the 'set _prefix' line above stays.

> +	}
> +}
> +
> +set picked 0
> +if {$_gitdir eq {}} {
> +	unset_gitdir_vars
>  	load_config 1
>  	apply_config
>  	if {![choose_repository::pick]} {
> @@ -1160,7 +1184,6 @@ if {[catch {
>  		catch {wm withdraw .}
>  		error_popup [strcat [mc "Unusable repo/worktree:"] " [pwd] "\n\n$err"]
>  	}
> -	set _prefix {}
>  	set picked 1
>  }
>  
> @@ -1175,11 +1198,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

-- Hannes