Re: [PATCH v1 07/11] git-gui: use rev-parse exclusively to find a repository

[Johannes Sixt <j6t@kdbg.org>]

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.

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

(Stopping the review here for today.)

-- Hannes