git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 1/1] Fix gitdir e.g. to respect core.hooksPath

From
Pratyush Yadav <me@yadavpratyush.com>
Date
Oct 8, 2019, 00:29 UTC
Message-ID
<20191008002902.n4b3bhlvwzsxodsc@yadavpratyush.com>
In-Reply-To
<65c2fa33e1aeec03930921ee0ef562d3f9dc5ccf.1570225311.git.gitgitgadget@gmail.com>
Hi Johannes,

Could you please change the commit subject to more clearly state that we are caching all paths. This is not something just related to hooks any more.

On 04/10/19 02:41PM, Johannes Schindelin via GitGitGadget wrote:
Show 20 quoted lines
> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> 
> Since v2.9.0, Git knows about the config variable core.hookspath
> that allows overriding the path to the directory containing the
> Git hooks.
> 
> Since v2.10.0, the `--git-path` option respects that config
> variable, too, so we may just as well use that command.
> 
> For Git versions older than v2.5.0 (which was the first version to
> support the `--git-path` option for the `rev-parse` command), we
> simply fall back to the previous code.
> 
> An original patch handled only the hooksPath setting, however, during
> the code submission it was deemed better to fix all call to the `gitdir`
> function.
> 
> To avoid spawning a gazillion `git rev-parse --git-path` instances, we
> cache the returned paths, priming the cache upon startup in a single
> `git rev-parse invocation` with the known entries.
I think it would also be a good idea to mention that we are fixing 
worktree paths not being correct.
 
Show 31 quoted lines
> This fixes https://github.com/git-for-windows/git/issues/1755
> 
> Initial-patch-by: Philipp Gortan <philipp@gortan.org>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  git-gui.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index fd476b6999..8b72e59cd0 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -158,6 +158,7 @@ if {[tk windowingsystem] eq "aqua"} {
>  
>  set _appname {Git Gui}
>  set _gitdir {}
> +array set _gitdir_cache {}
>  set _gitworktree {}
>  set _isbare {}
>  set _gitexec {}
> @@ -197,12 +198,57 @@ proc appname {} {
>  	return $_appname
>  }
>  
> +proc prime_gitdir_cache {} {
> +	global _gitdir _gitdir_cache
> +
> +	set gitdir_cmd [list git rev-parse --git-dir]
> +
> +	# `--git-path` is only supported since Git v2.5.0
> +	if {[package vcompare $::_git_version 2.5.0] >= 0} {

I think we should mention the source of the list of "magic" keys. A comment mentioning this list came from looking at the common calls to `gitdir` in the rest of the git-gui code would explain this function to a future reader better.

Show 18 quoted lines
> +		set gitdir_keys [list \
> +			CHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \
> +			GITGUI_MSG HEAD hooks hooks/prepare-commit-msg \
> +			index.lock info info/exclude logs MERGE_HEAD MERGE_MSG \
> +			MERGE_RR objects "objects/4\[0-1\]/*" \
> +			"objects/4\[0-3\]/*" objects/info \
> +			objects/info/alternates objects/pack packed-refs \
> +			PREPARE_COMMIT_MSG rebase-merge/head-name remotes \
> +			rr-cache rr-cache/MERGE_RR SQUASH_MSG \
> +		]
> +
> +		foreach key $gitdir_keys {
> +			lappend gitdir_cmd --git-path $key
> +		}
> +	}
> +
> +	set i -1
> +	foreach path [split [eval $gitdir_cmd] "\n"] {

A call to the procedure 'git' is wrapped in a 'catch' in a lot of places. But it is also not wrapped in 'catch' in a lot of other places.

I'm not sure how something like this would fail, so I'm not sure if wrapping this call in a catch is a good idea. But it is something to consider.

Show 40 quoted lines
> +		if {$i eq -1} {
> +			set _gitdir $path
> +		} else {
> +			set _gitdir_cache([lindex $gitdir_keys $i]) $path
> +		}
> +		incr i
> +	}
> +}
> +
>  proc gitdir {args} {
> -	global _gitdir
> +	global _gitdir _gitdir_cache
> +
>  	if {$args eq {}} {
>  		return $_gitdir
>  	}
> -	return [eval [list file join $_gitdir] $args]
> +
> +	set args [eval [list file join] $args]
> +	if {![info exists _gitdir_cache($args)]} {
> +		if {[package vcompare $::_git_version 2.5.0] >= 0} {
> +			set _gitdir_cache($args) [git rev-parse --git-path $args]
> +		} else {
> +			set _gitdir_cache($args) [file join $_gitdir $args]
> +		}
> +	}
> +
> +	return $_gitdir_cache($args)
>  }
>  
>  proc gitexec {args} {
> @@ -1242,7 +1288,7 @@ if {[catch {
>  	&& [catch {
>  		# beware that from the .git dir this sets _gitdir to .
>  		# and _prefix to the empty string
> -		set _gitdir [git rev-parse --git-dir]
> +		prime_gitdir_cache
>  		set _prefix [git rev-parse --show-prefix]
>  	} err]} {
>  	load_config 1

Can these paths we cache change while git-gui is running, say by a command run by the user in the terminal? In that case, we should refresh the list when the user rescans.

Other than some minor comments, looks good. Thanks.
-- 
Regards,
Pratyush Yadav
Previous: Johannes Schindelin via GitGitGadgetNext: Johannes Schindelin
Message 15 of 24 in “git-gui: respect core.hooksPath, falling back to .git/hooks”
  1. 0/1 git-gui: respect core.hooksPath, falling back to .git/hooksJohannes Schindelin via GitGitGadget, Sep 26, 2019
  2. 1/1 respect core.hooksPath, falling back to .git/hooksJohannes Schindelin via GitGitGadget, Sep 26, 2019
  3. Pratyush YadavSep 26, 2019
  4. Bert WesargSep 27, 2019
  5. Pratyush YadavSep 27, 2019
  6. Johannes SchindelinSep 30, 2019
  7. Pratyush YadavOct 1, 2019
  8. Johannes SchindelinOct 1, 2019
  9. Pratyush YadavOct 4, 2019
  10. Johannes SchindelinOct 4, 2019
  11. 0/1 git-gui: respect core.hooksPath, falling back to .git/hooksJohannes Schindelin via GitGitGadget, Sep 30, 2019
  12. 1/1 respect core.hooksPath, falling back to .git/hooksJohannes Schindelin via GitGitGadget, Sep 30, 2019
  13. 0/1 git-gui: respect core.hooksPath, falling back to .git/hooksJohannes Schindelin via GitGitGadget, Oct 4, 2019
  14. 1/1 Fix gitdir e.g. to respect core.hooksPathJohannes Schindelin via GitGitGadget, Oct 4, 2019
  15. Pratyush YadavOct 8, 2019
  16. Johannes SchindelinOct 8, 2019
  17. 0/1 git-gui: respect core.hooksPath, falling back to .git/hooksJohannes Schindelin via GitGitGadget, Oct 8, 2019
  18. 1/1 Make gitdir work with worktrees, respect core.hooksPath, etcJohannes Schindelin via GitGitGadget, Oct 8, 2019
  19. Pratyush YadavOct 11, 2019
  20. Johannes SchindelinOct 12, 2019
  21. Pratyush YadavOct 13, 2019
  22. Johannes SchindelinOct 13, 2019
  23. Pratyush YadavOct 17, 2019
  24. Johannes SchindelinOct 14, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.