Re: [PATCH v1 03/11] git-gui: guard set/unset of GIT_DIR and GIT_WORK_TREE

[Johannes Sixt <j6t@kdbg.org>]

Am 14.05.26 um 16:33 schrieb Mark Levedahl:
> git-gui unconditionally exports GIT_DIR and GIT_WORK_TREE to the
> environment, and furthmore unconditionally unsets these in many places.
> But, GIT_WORK_TREE should be set only if it is not {} as the empty
> value, really meaning no work-tree is found, causes git to throw fatal
> errors (git-gui gets the error from branch --show-current).  Fixing this
> is required to allow blame and browser to operate from a repository
> without a worktree.
> 
> Establish a pair of functions to remove GIT_DIR and GIT_WORK_TREE from
> the environment, avoiding any error if they do not exist. Also, add a
> function to export these, but export GIT_WORK_TREE only if not empty.

Good. But as I said in a parallel thread, I actually concur with your
assessment in the coverletter of this patch series that GIT_WORK_TREE
should be not set at all. At least in the modes that require a working tree.

> 
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-gui.sh | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index a951fcd..387cad6 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1122,6 +1122,22 @@ unset argv0dir
>  ##
>  ## repository setup
>  
> +proc set_gitdir_vars {} {
> +	global _gitdir _gitworktree env
> +	if {$_gitdir ne {}} {
> +		set env(GIT_DIR) $_gitdir
> +	}
> +	if {$_gitworktree ne {}} {
> +		set env(GIT_WORK_TREE) $_gitworktree
> +	}
> +}
> +
> +proc unset_gitdir_vars {} {
> +	global env
> +	catch {unset env(GIT_DIR)}
> +	catch {unset env(GIT_WORK_TREE)}
> +}
> +
>  set picked 0
>  if {[catch {
>  		set _gitdir $env(GIT_DIR)
> @@ -1207,8 +1223,8 @@ if {[lindex $_reponame end] eq {.git}} {
>  	set _reponame [lindex $_reponame end]
>  }
>  
> -set env(GIT_DIR) $_gitdir
> -set env(GIT_WORK_TREE) $_gitworktree
> +# Export the final paths
> +set_gitdir_vars
>  
>  ######################################################################
>  ##
> @@ -2050,13 +2066,11 @@ proc do_gitk {revs {is_submodule false}} {
>  			# TODO we could make life easier (start up faster?) for gitk
>  			# by setting these to the appropriate values to allow gitk
>  			# to skip the heuristics to find their proper value
> -			unset env(GIT_DIR)
> -			unset env(GIT_WORK_TREE)
> +			unset_gitdir_vars
>  		}
>  		safe_exec_bg [concat $cmd $revs "--" "--"]
>  
> -		set env(GIT_DIR) $_gitdir
> -		set env(GIT_WORK_TREE) $_gitworktree
> +		set_gitdir_vars
>  		cd $pwd
>  
>  		if {[info exists main_status]} {
> @@ -2084,16 +2098,14 @@ proc do_git_gui {} {
>  
>  		# see note in do_gitk about unsetting these vars when
>  		# running tools in a submodule
> -		unset env(GIT_DIR)
> -		unset env(GIT_WORK_TREE)
> +		unset_gitdir_vars
>  
>  		set pwd [pwd]
>  		cd $current_diff_path
>  
>  		safe_exec_bg [concat $exe gui]
>  
> -		set env(GIT_DIR) $_gitdir
> -		set env(GIT_WORK_TREE) $_gitworktree
> +		set_gitdir_vars
>  		cd $pwd
>  
>  		set status_operation [$::main_status \

After these changes, a 'global env' probably becomes stale and could be
removed.

-- Hannes