| Message ID | 65c2fa33e1aeec03930921ee0ef562d3f9dc5ccf.1570225311.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-gui: respect core.hooksPath, falling back to .git/hooks | expand |
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: > 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. > 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. > + 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. > + 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.
Hi Pratyush, On Tue, 8 Oct 2019, Pratyush Yadav wrote: > 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. It is related, but not exclusively so. Changed accordingly. > On 04/10/19 02:41PM, Johannes Schindelin via GitGitGadget wrote: > > 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. Done. > > 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. Makes sense. > > + 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. If that call fails, we lack a `gitdir`, which is a rather fundamental prerequisite to running Git GUI. I'd rather show an (ugly, but also highly unexpected) exception. In other words, I think the patch should stay "catch-less". > > + 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. I guess it can. A user could configure `core.hooksPath` while Git GUI is open, after all. Or rewrite the `.git` file of a worktree. I won't touch the `.git` file part, but I changed the `rescan` code to re-prime the gitdir cache. Thanks for the review, Johannes > > Other than some minor comments, looks good. Thanks. > > -- > Regards, > Pratyush Yadav >
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} { + 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"] { + 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