* [PATCH 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks @ 2019-09-26 21:17 Johannes Schindelin via GitGitGadget 2019-09-26 21:17 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget 2019-09-30 9:45 ` [PATCH v2 0/1] git-gui: " Johannes Schindelin via GitGitGadget 0 siblings, 2 replies; 24+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2019-09-26 21:17 UTC (permalink / raw) To: git; +Cc: Pratyush Yadav, Johannes Schindelin, Junio C Hamano This is yet another patch from Git for Windows. Johannes Schindelin (1): respect core.hooksPath, falling back to .git/hooks git-gui.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) base-commit: 60c60b627e81bf84e1cb01729d2ae882178f079d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-361%2Fdscho%2Fgit-gui-hooks-path-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-361/dscho/git-gui-hooks-path-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/361 -- gitgitgadget ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks 2019-09-26 21:17 [PATCH 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks Johannes Schindelin via GitGitGadget @ 2019-09-26 21:17 ` Johannes Schindelin via GitGitGadget 2019-09-26 22:36 ` Pratyush Yadav 2019-09-30 9:45 ` [PATCH v2 0/1] git-gui: " Johannes Schindelin via GitGitGadget 1 sibling, 1 reply; 24+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2019-09-26 21:17 UTC (permalink / raw) To: git Cc: Pratyush Yadav, Johannes Schindelin, Junio C Hamano, Johannes Schindelin 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. 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 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/git-gui.sh b/git-gui.sh index fd476b6999..b2c6e7a1db 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -623,7 +623,11 @@ proc git_write {args} { } proc githook_read {hook_name args} { - set pchook [gitdir hooks $hook_name] + if {[package vcompare $::_git_version 2.5.0] >= 0} { + set pchook [git rev-parse --git-path "hooks/$hook_name"] + } else { + set pchook [gitdir hooks $hook_name] + } lappend args 2>@1 # On Windows [file executable] might lie so we need to ask -- gitgitgadget ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks 2019-09-26 21:17 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget @ 2019-09-26 22:36 ` Pratyush Yadav 2019-09-27 6:10 ` Bert Wesarg 0 siblings, 1 reply; 24+ messages in thread From: Pratyush Yadav @ 2019-09-26 22:36 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Johannes Schindelin, Junio C Hamano Hi, On 26/09/19 02:17PM, 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. > > 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 | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/git-gui.sh b/git-gui.sh > index fd476b6999..b2c6e7a1db 100755 > --- a/git-gui.sh > +++ b/git-gui.sh > @@ -623,7 +623,11 @@ proc git_write {args} { > } > > proc githook_read {hook_name args} { > - set pchook [gitdir hooks $hook_name] > + if {[package vcompare $::_git_version 2.5.0] >= 0} { > + set pchook [git rev-parse --git-path "hooks/$hook_name"] > + } else { > + set pchook [gitdir hooks $hook_name] > + } gitdir is used in a lot of places, and I think all those would also benefit from using --git-path. So I think it is a better idea to move this to the procedure gitdir. It would have to be refactored to take any number of arguments, instead of the two it takes here. Other than that, looks good. Thanks. -- Regards, Pratyush Yadav ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks 2019-09-26 22:36 ` Pratyush Yadav @ 2019-09-27 6:10 ` Bert Wesarg 2019-09-27 13:05 ` Pratyush Yadav 0 siblings, 1 reply; 24+ messages in thread From: Bert Wesarg @ 2019-09-27 6:10 UTC (permalink / raw) To: Pratyush Yadav Cc: Johannes Schindelin via GitGitGadget, Git Mailing List, Johannes Schindelin, Junio C Hamano On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote: > > Hi, > > On 26/09/19 02:17PM, 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. > > > > 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 | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/git-gui.sh b/git-gui.sh > > index fd476b6999..b2c6e7a1db 100755 > > --- a/git-gui.sh > > +++ b/git-gui.sh > > @@ -623,7 +623,11 @@ proc git_write {args} { > > } > > > > proc githook_read {hook_name args} { > > - set pchook [gitdir hooks $hook_name] > > + if {[package vcompare $::_git_version 2.5.0] >= 0} { > > + set pchook [git rev-parse --git-path "hooks/$hook_name"] > > + } else { > > + set pchook [gitdir hooks $hook_name] > > + } > > gitdir is used in a lot of places, and I think all those would also > benefit from using --git-path. So I think it is a better idea to move > this to the procedure gitdir. It would have to be refactored to take any > number of arguments, instead of the two it takes here. gitdir already takes an arbitrary number of arguments and joins them to a path. The more imminent challenge is, that gitdir caches the GIT_DIR, thus it tries to avoid calling "git rev-parse". Which works for most, but not for hooks. We could either maintain a blacklist, for what we cache the result too, or always call "git rev-parse --git-dir". This blacklist would need to be in sync with the one in Git's path.c::adjust_git_path() than. Bert > > Other than that, looks good. Thanks. > > -- > Regards, > Pratyush Yadav ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks 2019-09-27 6:10 ` Bert Wesarg @ 2019-09-27 13:05 ` Pratyush Yadav 2019-09-30 9:42 ` Johannes Schindelin 0 siblings, 1 reply; 24+ messages in thread From: Pratyush Yadav @ 2019-09-27 13:05 UTC (permalink / raw) To: Bert Wesarg Cc: Johannes Schindelin via GitGitGadget, Git Mailing List, Johannes Schindelin, Junio C Hamano On 27/09/19 08:10AM, Bert Wesarg wrote: > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote: > > > > Hi, > > > > On 26/09/19 02:17PM, 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. > > > > > > 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 | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/git-gui.sh b/git-gui.sh > > > index fd476b6999..b2c6e7a1db 100755 > > > --- a/git-gui.sh > > > +++ b/git-gui.sh > > > @@ -623,7 +623,11 @@ proc git_write {args} { > > > } > > > > > > proc githook_read {hook_name args} { > > > - set pchook [gitdir hooks $hook_name] > > > + if {[package vcompare $::_git_version 2.5.0] >= 0} { > > > + set pchook [git rev-parse --git-path "hooks/$hook_name"] > > > + } else { > > > + set pchook [gitdir hooks $hook_name] > > > + } > > > > gitdir is used in a lot of places, and I think all those would also > > benefit from using --git-path. So I think it is a better idea to move > > this to the procedure gitdir. It would have to be refactored to take any > > number of arguments, instead of the two it takes here. > > gitdir already takes an arbitrary number of arguments and joins them > to a path. The more imminent challenge is, that gitdir caches the > GIT_DIR, thus it tries to avoid calling "git rev-parse". Which works > for most, but not for hooks. What I was thinking of was something like this: - If no args are passed, then just directly return $_gitdir. This is already being done. I assume the GIT_DIR relocation is already handled by `git rev-parse --git-dir`, so this would point to the correct location. - If args are passed, then we want a subdirectory of GIT_DIR In this case, it is possible that this subdirectory has also been relocated (hooks/ being one of those subdirectories). So in this case, use `git rev-parse --git-path` instead. So the gitdir procedure would look something like: proc gitdir {args} { global $_gitdir if {$args eq {}} { # Return the cached GIT_DIR return $_gitdir } # Use `git rev-parse --git-path` to get the path instead of # using the cached value. } Am I missing something? Or does this fix the issue you describe? > We could either maintain a blacklist, for what we cache the result > too, or always call "git rev-parse --git-dir". > > This blacklist would need to be in sync with the one in Git's > path.c::adjust_git_path() than. Is caching GIT_DIR that important in terms of performance? Otherwise, I'd say calling `git rev-parse --git-path` for _every_ subdirectory of GIT_DIR is a much simpler solution. -- Regards, Pratyush Yadav ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks 2019-09-27 13:05 ` Pratyush Yadav @ 2019-09-30 9:42 ` Johannes Schindelin 2019-10-01 13:31 ` Pratyush Yadav 0 siblings, 1 reply; 24+ messages in thread From: Johannes Schindelin @ 2019-09-30 9:42 UTC (permalink / raw) To: Pratyush Yadav Cc: Bert Wesarg, Johannes Schindelin via GitGitGadget, Git Mailing List, Junio C Hamano Hi, On Fri, 27 Sep 2019, Pratyush Yadav wrote: > On 27/09/19 08:10AM, Bert Wesarg wrote: > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote: > > > > > > On 26/09/19 02:17PM, 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. > > > > > > > > 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 | 6 +++++- > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/git-gui.sh b/git-gui.sh > > > > index fd476b6999..b2c6e7a1db 100755 > > > > --- a/git-gui.sh > > > > +++ b/git-gui.sh > > > > @@ -623,7 +623,11 @@ proc git_write {args} { > > > > } > > > > > > > > proc githook_read {hook_name args} { > > > > - set pchook [gitdir hooks $hook_name] > > > > + if {[package vcompare $::_git_version 2.5.0] >= 0} { > > > > + set pchook [git rev-parse --git-path "hooks/$hook_name"] > > > > + } else { > > > > + set pchook [gitdir hooks $hook_name] > > > > + } > > > > > > gitdir is used in a lot of places, and I think all those would also > > > benefit from using --git-path. So I think it is a better idea to move > > > this to the procedure gitdir. It would have to be refactored to take any > > > number of arguments, instead of the two it takes here. > > > > gitdir already takes an arbitrary number of arguments and joins them > > to a path. The more imminent challenge is, that gitdir caches the > > GIT_DIR, thus it tries to avoid calling "git rev-parse". Which works > > for most, but not for hooks. > > What I was thinking of was something like this: > > - If no args are passed, then just directly return $_gitdir. This is > already being done. I assume the GIT_DIR relocation is already > handled by `git rev-parse --git-dir`, so this would point to the > correct location. > - If args are passed, then we want a subdirectory of GIT_DIR In this > case, it is possible that this subdirectory has also been relocated > (hooks/ being one of those subdirectories). So in this case, use > `git rev-parse --git-path` instead. > > So the gitdir procedure would look something like: > > proc gitdir {args} { > global $_gitdir > if {$args eq {}} { > # Return the cached GIT_DIR > return $_gitdir > } > > # Use `git rev-parse --git-path` to get the path instead of > # using the cached value. > } > > Am I missing something? Or does this fix the issue you describe? The `gitdir` function is called 13 times during startup alone, and who knows how many more times later. So I am quite convinced that the original intention was to save on spawning processes left and right. But since you are the Git GUI maintainer, and this was your suggestion, I made it so. Ciao, Johannes > > We could either maintain a blacklist, for what we cache the result > > too, or always call "git rev-parse --git-dir". > > > > This blacklist would need to be in sync with the one in Git's > > path.c::adjust_git_path() than. > > Is caching GIT_DIR that important in terms of performance? Otherwise, > I'd say calling `git rev-parse --git-path` for _every_ subdirectory of > GIT_DIR is a much simpler solution. > > -- > Regards, > Pratyush Yadav > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks 2019-09-30 9:42 ` Johannes Schindelin @ 2019-10-01 13:31 ` Pratyush Yadav 2019-10-01 17:38 ` Johannes Schindelin 0 siblings, 1 reply; 24+ messages in thread From: Pratyush Yadav @ 2019-10-01 13:31 UTC (permalink / raw) To: Johannes Schindelin Cc: Bert Wesarg, Johannes Schindelin via GitGitGadget, Git Mailing List, Junio C Hamano On 30/09/19 11:42AM, Johannes Schindelin wrote: > On Fri, 27 Sep 2019, Pratyush Yadav wrote: > > On 27/09/19 08:10AM, Bert Wesarg wrote: > > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote: > > > > gitdir is used in a lot of places, and I think all those would > > > > also > > > > benefit from using --git-path. So I think it is a better idea to move > > > > this to the procedure gitdir. It would have to be refactored to take any > > > > number of arguments, instead of the two it takes here. > > The `gitdir` function is called 13 times during startup alone, and who > knows how many more times later. > > So I am quite convinced that the original intention was to save on > spawning processes left and right. > > But since you are the Git GUI maintainer, and this was your suggestion, > I made it so. Yes, I am the maintainer, but I am not an all-knowing, all-seeing entity. Your, and every other contributors, suggestions are very valuable. And my suggestions aren't gospel. I would hate to see someone send in a patch they weren't sure was the best thing to do just because I suggested it. Please feel free to object my suggestions. In this case, I didn't expect gitdir to be called this many times. While I don't notice much of a performance difference on my system (Linux), a quick measurement tells me that the time spent in gitdir is about 16 ms. In contrast, the same measurement without the v2 patch gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something so simple. It might not be the same for everyone else. AFAIK, spawning a process is much slower on Windows. So now I'm not so sure my suggestion was a good one. My original aim was to be sure everything was correct, and no incorrect directories were being used. But the current solution comes at a performance hit. > > > We could either maintain a blacklist, for what we cache the result > > > too, or always call "git rev-parse --git-dir". > > > > > > This blacklist would need to be in sync with the one in Git's > > > path.c::adjust_git_path() than. Bert's suggestion seems like a decent compromise. We run `git rev-parse --git-path` for the paths in the blacklist, and for the rest we use the cached value. This does run the risk of getting out of sync with git.git's list, but it might be better than spawing a process every time, and is very likely better than just doing it for hooks. Thoughts? -- Regards, Pratyush Yadav ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks 2019-10-01 13:31 ` Pratyush Yadav @ 2019-10-01 17:38 ` Johannes Schindelin 2019-10-04 16:48 ` Pratyush Yadav 0 siblings, 1 reply; 24+ messages in thread From: Johannes Schindelin @ 2019-10-01 17:38 UTC (permalink / raw) To: Pratyush Yadav Cc: Bert Wesarg, Johannes Schindelin via GitGitGadget, Git Mailing List, Junio C Hamano Hi, On Tue, 1 Oct 2019, Pratyush Yadav wrote: > On 30/09/19 11:42AM, Johannes Schindelin wrote: > > On Fri, 27 Sep 2019, Pratyush Yadav wrote: > > > On 27/09/19 08:10AM, Bert Wesarg wrote: > > > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote: > > > > > gitdir is used in a lot of places, and I think all those would > > > > > also > > > > > benefit from using --git-path. So I think it is a better idea to move > > > > > this to the procedure gitdir. It would have to be refactored to take any > > > > > number of arguments, instead of the two it takes here. > > > > The `gitdir` function is called 13 times during startup alone, and who > > knows how many more times later. > > > > So I am quite convinced that the original intention was to save on > > spawning processes left and right. > > > > But since you are the Git GUI maintainer, and this was your suggestion, > > I made it so. > > Yes, I am the maintainer, but I am not an all-knowing, all-seeing > entity. Your, and every other contributors, suggestions are very > valuable. And my suggestions aren't gospel. I would hate to see someone > send in a patch they weren't sure was the best thing to do just because > I suggested it. Please feel free to object my suggestions. > > In this case, I didn't expect gitdir to be called this many times. > > While I don't notice much of a performance difference on my system > (Linux), a quick measurement tells me that the time spent in gitdir is > about 16 ms. In contrast, the same measurement without the v2 patch > gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something > so simple. It might not be the same for everyone else. AFAIK, spawning a > process is much slower on Windows. > > So now I'm not so sure my suggestion was a good one. My original aim was > to be sure everything was correct, and no incorrect directories were > being used. But the current solution comes at a performance hit. > > > > > We could either maintain a blacklist, for what we cache the result > > > > too, or always call "git rev-parse --git-dir". > > > > > > > > This blacklist would need to be in sync with the one in Git's > > > > path.c::adjust_git_path() than. > > Bert's suggestion seems like a decent compromise. We run `git rev-parse > --git-path` for the paths in the blacklist, and for the rest we use the > cached value. This does run the risk of getting out of sync with > git.git's list, but it might be better than spawing a process every > time, and is very likely better than just doing it for hooks. But what about this part of that function? -- snip -- else if (repo->different_commondir) update_common_dir(buf, git_dir_len, repo->commondir); -- snap -- It might well turn out that this blacklist is neither easy to implement nor will it help much. So let's look at all the call sites: -- snip -- $ git grep -w gitdir | sed -ne 's|\].*||' -e 's|.*\[gitdir ||p' | sort | uniq $file $name CHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG GITGUI_MSG HEAD hooks $hook_name index.lock info exclude logs $name MERGE_HEAD MERGE_MSG MERGE_RR objects 4\[0-[expr {$ndirs-1} objects info objects info alternates objects pack packed-refs PREPARE_COMMIT_MSG rebase-merge head-name remotes remotes $r rr-cache rr-cache MERGE_RR SQUASH_MSG -- snap -- The `$file` call looks for messages (probably commit, merge, tag messages and the likes), the `$name` one looks for refs. Some of those arguments strike me as very good candidates to require the common directory while others require the real gitdir (remember, commondir != gitdir in worktrees other than the main worktree). What _could_ be done (but we're certainly threatening to enter the realm of the ridiculous here) is to call `git rev-parse --git-dir --git-path CHERRY_PICK_HEAD --git-path FETCH_HEAD [...]`, which will output one path per line, and then store the result in an associative array (https://tcl.tk/man/tcl8.5/tutorial/Tcl22.html), and use that to look up paths based on their first component, caching as we go. Something like this: -- snipsnap -- diff --git a/git-gui.sh b/git-gui.sh index fd476b6..9295c75 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_cached {} set _gitworktree {} set _isbare {} set _gitexec {} @@ -197,12 +198,50 @@ proc appname {} { return $_appname } +proc init_gitdir_cached {} { + global _gitdir _gitdir_cached + + set gitdir_keys [list \ + CHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \ + GITGUI_MSG HEAD hooks index.lock info logs MERGE_HEAD \ + MERGE_MSG MERGE_RR objects packed-refs PREPARE_COMMIT_MSG \ + rebase-merge head-name remotes rr-cache SQUASH_MSG \ + ] + + set gitdir_cmd [list git rev-parse --git-dir] + 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_cached([lindex $gitdir_keys $i]) $path + } + incr i + } +} + proc gitdir {args} { - global _gitdir + global _gitdir _gitdir_cached + if {$args eq {}} { return $_gitdir } - return [eval [list file join $_gitdir] $args] + + set arg0 [lindex $args 0] + set args [lrange $args 1 end] + if {![info exists _gitdir_cached($arg0)]} { + if {[package vcompare $::_git_version 2.5.0] >= 0} { + set _gitdir_cached($arg0) [git rev-parse --git-path $arg0] + } else { + set _gitdir_cached($arg0) [file join $_gitdir $arg0] + } + } + + return [eval [concat [list file join $_gitdir_cached($arg0)] $args]] } proc gitexec {args} { @@ -1242,7 +1281,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] + init_gitdir_cached set _prefix [git rev-parse --show-prefix] } err]} { load_config 1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks 2019-10-01 17:38 ` Johannes Schindelin @ 2019-10-04 16:48 ` Pratyush Yadav 2019-10-04 19:56 ` Johannes Schindelin 0 siblings, 1 reply; 24+ messages in thread From: Pratyush Yadav @ 2019-10-04 16:48 UTC (permalink / raw) To: Johannes Schindelin Cc: Bert Wesarg, Johannes Schindelin via GitGitGadget, Git Mailing List, Junio C Hamano On 01/10/19 07:38PM, Johannes Schindelin wrote: > Hi, > > On Tue, 1 Oct 2019, Pratyush Yadav wrote: > > > On 30/09/19 11:42AM, Johannes Schindelin wrote: > > > On Fri, 27 Sep 2019, Pratyush Yadav wrote: > > > > On 27/09/19 08:10AM, Bert Wesarg wrote: > > > > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote: > > > > > > gitdir is used in a lot of places, and I think all those would > > > > > > also > > > > > > benefit from using --git-path. So I think it is a better idea to move > > > > > > this to the procedure gitdir. It would have to be refactored to take any > > > > > > number of arguments, instead of the two it takes here. > > > > > > The `gitdir` function is called 13 times during startup alone, and who > > > knows how many more times later. > > > > > > So I am quite convinced that the original intention was to save on > > > spawning processes left and right. > > > > > > But since you are the Git GUI maintainer, and this was your suggestion, > > > I made it so. > > > > Yes, I am the maintainer, but I am not an all-knowing, all-seeing > > entity. Your, and every other contributors, suggestions are very > > valuable. And my suggestions aren't gospel. I would hate to see someone > > send in a patch they weren't sure was the best thing to do just because > > I suggested it. Please feel free to object my suggestions. > > > > In this case, I didn't expect gitdir to be called this many times. > > > > While I don't notice much of a performance difference on my system > > (Linux), a quick measurement tells me that the time spent in gitdir is > > about 16 ms. In contrast, the same measurement without the v2 patch > > gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something > > so simple. It might not be the same for everyone else. AFAIK, spawning a > > process is much slower on Windows. > > > > So now I'm not so sure my suggestion was a good one. My original aim was > > to be sure everything was correct, and no incorrect directories were > > being used. But the current solution comes at a performance hit. > > > > > > > We could either maintain a blacklist, for what we cache the result > > > > > too, or always call "git rev-parse --git-dir". > > > > > > > > > > This blacklist would need to be in sync with the one in Git's > > > > > path.c::adjust_git_path() than. > > > > Bert's suggestion seems like a decent compromise. We run `git rev-parse > > --git-path` for the paths in the blacklist, and for the rest we use the > > cached value. This does run the risk of getting out of sync with > > git.git's list, but it might be better than spawing a process every > > time, and is very likely better than just doing it for hooks. > > But what about this part of that function? > > -- snip -- > else if (repo->different_commondir) > update_common_dir(buf, git_dir_len, repo->commondir); > -- snap -- I'm afraid I'm a bit out of my depth on this. I have no idea what a "common directory" is, and how is it different from the "git directory". I can't find anything useful on Google about it. My guess is that it is something related to separate worktrees. > It might well turn out that this blacklist is neither easy to implement > nor will it help much. Am I correct in assuming that for other cases like "info", "grafts", "index", "objects", and "hooks" the blacklist would be simple to implement, and it is the "common directory" case that is problematic? > So let's look at all the call sites: > > -- snip -- > $ git grep -w gitdir | sed -ne 's|\].*||' -e 's|.*\[gitdir ||p' | sort | uniq > $file > $name > CHERRY_PICK_HEAD > FETCH_HEAD > GITGUI_BCK > GITGUI_EDITMSG > GITGUI_MSG > HEAD > hooks $hook_name > index.lock > info exclude > logs $name > MERGE_HEAD > MERGE_MSG > MERGE_RR > objects 4\[0-[expr {$ndirs-1} > objects info > objects info alternates > objects pack > packed-refs > PREPARE_COMMIT_MSG > rebase-merge head-name > remotes > remotes $r > rr-cache > rr-cache MERGE_RR > SQUASH_MSG > -- snap -- > > The `$file` call looks for messages (probably commit, merge, tag > messages and the likes), the `$name` one looks for refs. So they should always be inside the '.git' or GIT_DIR, correct? > Some of those arguments strike me as very good candidates to require the > common directory while others require the real gitdir (remember, > commondir != gitdir in worktrees other than the main worktree). > > What _could_ be done (but we're certainly threatening to enter the realm > of the ridiculous here) is to call `git rev-parse --git-dir --git-path > CHERRY_PICK_HEAD --git-path FETCH_HEAD [...]`, which will output one > path per line, and then store the result in an associative array > (https://tcl.tk/man/tcl8.5/tutorial/Tcl22.html), and use that to look up > paths based on their first component, caching as we go. Ah yes! That is certainly threatening to enter the realm of ridiculous. I'm not sure what benefit this will have. Right now, I don't think git-gui handles these cases. Have people complained? Is this a common problem? I want to evaluate how much benefit we get doing something like this has over just using your original patch that works with hooks only. > Something like this: > > -- snipsnap -- > diff --git a/git-gui.sh b/git-gui.sh > index fd476b6..9295c75 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_cached {} > set _gitworktree {} > set _isbare {} > set _gitexec {} > @@ -197,12 +198,50 @@ proc appname {} { > return $_appname > } > > +proc init_gitdir_cached {} { > + global _gitdir _gitdir_cached > + > + set gitdir_keys [list \ > + CHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \ > + GITGUI_MSG HEAD hooks index.lock info logs MERGE_HEAD \ > + MERGE_MSG MERGE_RR objects packed-refs PREPARE_COMMIT_MSG \ > + rebase-merge head-name remotes rr-cache SQUASH_MSG \ > + ] > + > + set gitdir_cmd [list git rev-parse --git-dir] > + 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_cached([lindex $gitdir_keys $i]) $path > + } > + incr i > + } > +} > + > proc gitdir {args} { > - global _gitdir > + global _gitdir _gitdir_cached > + > if {$args eq {}} { > return $_gitdir > } > - return [eval [list file join $_gitdir] $args] > + > + set arg0 [lindex $args 0] > + set args [lrange $args 1 end] > + if {![info exists _gitdir_cached($arg0)]} { > + if {[package vcompare $::_git_version 2.5.0] >= 0} { > + set _gitdir_cached($arg0) [git rev-parse --git-path $arg0] > + } else { > + set _gitdir_cached($arg0) [file join $_gitdir $arg0] > + } > + } > + > + return [eval [concat [list file join $_gitdir_cached($arg0)] $args]] > } > > proc gitexec {args} { > @@ -1242,7 +1281,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] > + init_gitdir_cached > set _prefix [git rev-parse --show-prefix] > } err]} { > load_config 1 A nice way of tackling this problem overall considering the challenges, but I'm worried about whether all this is _actually_ needed for real use cases, and what breaks if we don't. Honestly, I'm not too sure how to tackle this problem. That is also the reason I took so long in writing this response. What would your suggestion be? Also, if some other people interested in git-gui could chime in, it would be great. -- Regards, Pratyush Yadav ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks 2019-10-04 16:48 ` Pratyush Yadav @ 2019-10-04 19:56 ` Johannes Schindelin 0 siblings, 0 replies; 24+ messages in thread From: Johannes Schindelin @ 2019-10-04 19:56 UTC (permalink / raw) To: Pratyush Yadav Cc: Bert Wesarg, Johannes Schindelin via GitGitGadget, Git Mailing List, Junio C Hamano Hi Pratyush, On Fri, 4 Oct 2019, Pratyush Yadav wrote: > On 01/10/19 07:38PM, Johannes Schindelin wrote: > > > > On Tue, 1 Oct 2019, Pratyush Yadav wrote: > > > > > On 30/09/19 11:42AM, Johannes Schindelin wrote: > > > > On Fri, 27 Sep 2019, Pratyush Yadav wrote: > > > > > On 27/09/19 08:10AM, Bert Wesarg wrote: > > > > > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote: > > > > > > > gitdir is used in a lot of places, and I think all those would > > > > > > > also > > > > > > > benefit from using --git-path. So I think it is a better idea to move > > > > > > > this to the procedure gitdir. It would have to be refactored to take any > > > > > > > number of arguments, instead of the two it takes here. > > > > > > > > The `gitdir` function is called 13 times during startup alone, and who > > > > knows how many more times later. > > > > > > > > So I am quite convinced that the original intention was to save on > > > > spawning processes left and right. > > > > > > > > But since you are the Git GUI maintainer, and this was your suggestion, > > > > I made it so. > > > > > > Yes, I am the maintainer, but I am not an all-knowing, all-seeing > > > entity. Your, and every other contributors, suggestions are very > > > valuable. And my suggestions aren't gospel. I would hate to see someone > > > send in a patch they weren't sure was the best thing to do just because > > > I suggested it. Please feel free to object my suggestions. > > > > > > In this case, I didn't expect gitdir to be called this many times. > > > > > > While I don't notice much of a performance difference on my system > > > (Linux), a quick measurement tells me that the time spent in gitdir is > > > about 16 ms. In contrast, the same measurement without the v2 patch > > > gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something > > > so simple. It might not be the same for everyone else. AFAIK, spawning a > > > process is much slower on Windows. > > > > > > So now I'm not so sure my suggestion was a good one. My original aim was > > > to be sure everything was correct, and no incorrect directories were > > > being used. But the current solution comes at a performance hit. > > > > > > > > > We could either maintain a blacklist, for what we cache the result > > > > > > too, or always call "git rev-parse --git-dir". > > > > > > > > > > > > This blacklist would need to be in sync with the one in Git's > > > > > > path.c::adjust_git_path() than. > > > > > > Bert's suggestion seems like a decent compromise. We run `git rev-parse > > > --git-path` for the paths in the blacklist, and for the rest we use the > > > cached value. This does run the risk of getting out of sync with > > > git.git's list, but it might be better than spawing a process every > > > time, and is very likely better than just doing it for hooks. > > > > But what about this part of that function? > > > > -- snip -- > > else if (repo->different_commondir) > > update_common_dir(buf, git_dir_len, repo->commondir); > > -- snap -- > > I'm afraid I'm a bit out of my depth on this. I have no idea what a > "common directory" is, and how is it different from the "git directory". > I can't find anything useful on Google about it. My guess is that it is > something related to separate worktrees. It is indeed related to worktrees. If you create a secondary worktree via `git worktree add [...]`, that work tree will get its own git directory under `.git/worktrees/<name>` in the main worktree. That git directory will not, however, contain all contents of a regular git directory. Most refs, for example, are stored in the main worktree's git directory. That is what the "common dir" is. > > It might well turn out that this blacklist is neither easy to implement > > nor will it help much. > > Am I correct in assuming that for other cases like "info", "grafts", > "index", "objects", and "hooks" the blacklist would be simple to > implement, and it is the "common directory" case that is problematic? Indeed, for the other, simple cases, the list would be unproblematic to implement. Problematic to maintain, though, especially given that Git GUI is _supposed_ to support even very old Git versions. And those simple cases don't include _all_ interesting cases. Take `logs/` for example. The git directory will contain the reflogs for `HEAD`, but unless you're on an unnamed branch (AKA "detached HEAD"), the reflogs for the current branch are _in the commondir_. > > So let's look at all the call sites: > > > > -- snip -- > > $ git grep -w gitdir | sed -ne 's|\].*||' -e 's|.*\[gitdir ||p' | sort | uniq > > $file > > $name > > CHERRY_PICK_HEAD > > FETCH_HEAD > > GITGUI_BCK > > GITGUI_EDITMSG > > GITGUI_MSG > > HEAD > > hooks $hook_name > > index.lock > > info exclude > > logs $name > > MERGE_HEAD > > MERGE_MSG > > MERGE_RR > > objects 4\[0-[expr {$ndirs-1} > > objects info > > objects info alternates > > objects pack > > packed-refs > > PREPARE_COMMIT_MSG > > rebase-merge head-name > > remotes > > remotes $r > > rr-cache > > rr-cache MERGE_RR > > SQUASH_MSG > > -- snap -- > > > > The `$file` call looks for messages (probably commit, merge, tag > > messages and the likes), the `$name` one looks for refs. > > So they should always be inside the '.git' or GIT_DIR, correct? They should be inside the git directory. Note that `.git` in worktrees is just a file that contains `gitdir: <path>`. The indicated path is the actual git directory. Inside that git directory, the file `commondir` contains the path to the main worktree's git directory. > > Some of those arguments strike me as very good candidates to require the > > common directory while others require the real gitdir (remember, > > commondir != gitdir in worktrees other than the main worktree). > > > > What _could_ be done (but we're certainly threatening to enter the realm > > of the ridiculous here) is to call `git rev-parse --git-dir --git-path > > CHERRY_PICK_HEAD --git-path FETCH_HEAD [...]`, which will output one > > path per line, and then store the result in an associative array > > (https://tcl.tk/man/tcl8.5/tutorial/Tcl22.html), and use that to look up > > paths based on their first component, caching as we go. > > Ah yes! That is certainly threatening to enter the realm of ridiculous. > I'm not sure what benefit this will have. Right now, I don't think > git-gui handles these cases. Have people complained? Is this a common > problem? Well, we know that people complained about the hooks directory. And that did not even involve worktrees. I see that e.g. `packed-refs` is queried by Git GUI. And that file lives in the main worktree's git directory, i.e. in the commondir. So either users don't use Git GUI in secondary worktrees, or they did not even notice the bug. > I want to evaluate how much benefit we get doing something like this > has over just using your original patch that works with hooks only. Since I already have that ridiculous approach essentially implemented, and since it fixes very real bugs in Git GUI ever since `git worktree` was introduced, I'd say that you'd be better off taking the ridiculous patch than not. > > Something like this: > > > > -- snipsnap -- > > diff --git a/git-gui.sh b/git-gui.sh > > index fd476b6..9295c75 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_cached {} > > set _gitworktree {} > > set _isbare {} > > set _gitexec {} > > @@ -197,12 +198,50 @@ proc appname {} { > > return $_appname > > } > > > > +proc init_gitdir_cached {} { > > + global _gitdir _gitdir_cached > > + > > + set gitdir_keys [list \ > > + CHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \ > > + GITGUI_MSG HEAD hooks index.lock info logs MERGE_HEAD \ > > + MERGE_MSG MERGE_RR objects packed-refs PREPARE_COMMIT_MSG \ > > + rebase-merge head-name remotes rr-cache SQUASH_MSG \ > > + ] > > + > > + set gitdir_cmd [list git rev-parse --git-dir] > > + 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_cached([lindex $gitdir_keys $i]) $path > > + } > > + incr i > > + } > > +} > > + > > proc gitdir {args} { > > - global _gitdir > > + global _gitdir _gitdir_cached > > + > > if {$args eq {}} { > > return $_gitdir > > } > > - return [eval [list file join $_gitdir] $args] > > + > > + set arg0 [lindex $args 0] > > + set args [lrange $args 1 end] > > + if {![info exists _gitdir_cached($arg0)]} { > > + if {[package vcompare $::_git_version 2.5.0] >= 0} { > > + set _gitdir_cached($arg0) [git rev-parse --git-path $arg0] > > + } else { > > + set _gitdir_cached($arg0) [file join $_gitdir $arg0] > > + } > > + } > > + > > + return [eval [concat [list file join $_gitdir_cached($arg0)] $args]] > > } > > > > proc gitexec {args} { > > @@ -1242,7 +1281,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] > > + init_gitdir_cached > > set _prefix [git rev-parse --show-prefix] > > } err]} { > > load_config 1 > > A nice way of tackling this problem overall considering the challenges, > but I'm worried about whether all this is _actually_ needed for real use > cases, and what breaks if we don't. Why don't you try using Git GUI in a worktree for a while? I am sure you will encounter the issues sooner or later. > Honestly, I'm not too sure how to tackle this problem. That is also the > reason I took so long in writing this response. What would your > suggestion be? I would actually go for the ridiculous patch, as it provides the safest bet we have on fixing the `gitdir`-related bugs. > Also, if some other people interested in git-gui could chime in, it > would be great. Sure. Ciao, Johannes ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks 2019-09-26 21:17 [PATCH 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks Johannes Schindelin via GitGitGadget 2019-09-26 21:17 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget @ 2019-09-30 9:45 ` Johannes Schindelin via GitGitGadget 2019-09-30 9:45 ` [PATCH v2 1/1] " Johannes Schindelin via GitGitGadget 2019-10-04 21:41 ` [PATCH v3 0/1] git-gui: " Johannes Schindelin via GitGitGadget 1 sibling, 2 replies; 24+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2019-09-30 9:45 UTC (permalink / raw) To: git; +Cc: Pratyush Yadav, Bert Wesarg, Johannes Schindelin, Junio C Hamano This is yet another patch from Git for Windows. Changes since v1: * Rather than a fine-grained override of gitdir just for the hooks path, we now spawn git rev-parse --git-path [...] every time gitdir is called with arguments. This makes the code safer, although at the cost of potentially many spawned processes. Johannes Schindelin (1): respect core.hooksPath, falling back to .git/hooks git-gui.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) base-commit: 60c60b627e81bf84e1cb01729d2ae882178f079d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-361%2Fdscho%2Fgit-gui-hooks-path-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-361/dscho/git-gui-hooks-path-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/361 Range-diff vs v1: 1: eca193f91b < -: ---------- respect core.hooksPath, falling back to .git/hooks -: ---------- > 1: c101422936 respect core.hooksPath, falling back to .git/hooks -- gitgitgadget ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/1] respect core.hooksPath, falling back to .git/hooks 2019-09-30 9:45 ` [PATCH v2 0/1] git-gui: " Johannes Schindelin via GitGitGadget @ 2019-09-30 9:45 ` Johannes Schindelin via GitGitGadget 2019-10-04 21:41 ` [PATCH v3 0/1] git-gui: " Johannes Schindelin via GitGitGadget 1 sibling, 0 replies; 24+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2019-09-30 9:45 UTC (permalink / raw) To: git Cc: Pratyush Yadav, Bert Wesarg, Johannes Schindelin, Junio C Hamano, Johannes Schindelin 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 (as the title of this commit message suggests), however, during the code submission it was deemed better to fix all call to the `gitdir` function. With this change, we spawn `git rev-parse --git-path [...]` 13 times during Git GUI's startup. 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 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/git-gui.sh b/git-gui.sh index fd476b6999..b2f0e78077 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -202,7 +202,11 @@ proc gitdir {args} { if {$args eq {}} { return $_gitdir } - return [eval [list file join $_gitdir] $args] + if {[package vcompare $::_git_version 2.5.0] >= 0} { + return [git rev-parse --git-path [eval [list file join] $args]] + } else { + return [eval [list file join $_gitdir] $args] + } } proc gitexec {args} { -- gitgitgadget ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks 2019-09-30 9:45 ` [PATCH v2 0/1] git-gui: " Johannes Schindelin via GitGitGadget 2019-09-30 9:45 ` [PATCH v2 1/1] " Johannes Schindelin via GitGitGadget @ 2019-10-04 21:41 ` Johannes Schindelin via GitGitGadget 2019-10-04 21:41 ` [PATCH v3 1/1] Fix gitdir e.g. to respect core.hooksPath Johannes Schindelin via GitGitGadget 2019-10-08 11:33 ` [PATCH v4 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks Johannes Schindelin via GitGitGadget 1 sibling, 2 replies; 24+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2019-10-04 21:41 UTC (permalink / raw) To: git; +Cc: Pratyush Yadav, Bert Wesarg, Johannes Schindelin, Junio C Hamano This is yet another patch from Git for Windows. Changes since v2: * The paths returned by git rev-parse --git-path are now cached, and the cache is primed with the most common paths. Changes since v1: * Rather than a fine-grained override of gitdir just for the hooks path, we now spawn git rev-parse --git-path [...] every time gitdir is called with arguments. This makes the code safer, although at the cost of potentially many spawned processes. Johannes Schindelin (1): Fix gitdir e.g. to respect core.hooksPath git-gui.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 3 deletions(-) base-commit: 60c60b627e81bf84e1cb01729d2ae882178f079d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-361%2Fdscho%2Fgit-gui-hooks-path-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-361/dscho/git-gui-hooks-path-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/361 Range-diff vs v2: 1: c101422936 < -: ---------- respect core.hooksPath, falling back to .git/hooks -: ---------- > 1: 65c2fa33e1 Fix gitdir e.g. to respect core.hooksPath -- gitgitgadget ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/1] Fix gitdir e.g. to respect core.hooksPath 2019-10-04 21:41 ` [PATCH v3 0/1] git-gui: " Johannes Schindelin via GitGitGadget @ 2019-10-04 21:41 ` Johannes Schindelin via GitGitGadget 2019-10-08 0:29 ` Pratyush Yadav 2019-10-08 11:33 ` [PATCH v4 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks Johannes Schindelin via GitGitGadget 1 sibling, 1 reply; 24+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2019-10-04 21:41 UTC (permalink / raw) To: git Cc: Pratyush Yadav, Bert Wesarg, Johannes Schindelin, Junio C Hamano, Johannes Schindelin 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. 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} { + 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 -- gitgitgadget ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/1] Fix gitdir e.g. to respect core.hooksPath 2019-10-04 21:41 ` [PATCH v3 1/1] Fix gitdir e.g. to respect core.hooksPath Johannes Schindelin via GitGitGadget @ 2019-10-08 0:29 ` Pratyush Yadav 2019-10-08 11:30 ` Johannes Schindelin 0 siblings, 1 reply; 24+ messages in thread From: Pratyush Yadav @ 2019-10-08 0:29 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Bert Wesarg, Johannes Schindelin, Junio C Hamano 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. -- Regards, Pratyush Yadav ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/1] Fix gitdir e.g. to respect core.hooksPath 2019-10-08 0:29 ` Pratyush Yadav @ 2019-10-08 11:30 ` Johannes Schindelin 0 siblings, 0 replies; 24+ messages in thread From: Johannes Schindelin @ 2019-10-08 11:30 UTC (permalink / raw) To: Pratyush Yadav Cc: Johannes Schindelin via GitGitGadget, git, Bert Wesarg, Junio C Hamano 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 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks 2019-10-04 21:41 ` [PATCH v3 0/1] git-gui: " Johannes Schindelin via GitGitGadget 2019-10-04 21:41 ` [PATCH v3 1/1] Fix gitdir e.g. to respect core.hooksPath Johannes Schindelin via GitGitGadget @ 2019-10-08 11:33 ` Johannes Schindelin via GitGitGadget 2019-10-08 11:33 ` [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc Johannes Schindelin via GitGitGadget 1 sibling, 1 reply; 24+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2019-10-08 11:33 UTC (permalink / raw) To: git; +Cc: Pratyush Yadav, Bert Wesarg, Johannes Schindelin, Pratyush Yadav This is yet another patch from Git for Windows. Changes since v3: * Adjusted the commit message to reflect that this is no longer only about the hooks directory. * Added a code comment to indicate how the list of keys was determined that are used for the gitdir priming. * The gitdir cache is now re-primed upon F5. Changes since v2: * The paths returned by git rev-parse --git-path are now cached, and the cache is primed with the most common paths. Changes since v1: * Rather than a fine-grained override of gitdir just for the hooks path, we now spawn git rev-parse --git-path [...] every time gitdir is called with arguments. This makes the code safer, although at the cost of potentially many spawned processes. Johannes Schindelin (1): Make gitdir work with worktrees, respect core.hooksPath, etc git-gui.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 4 deletions(-) base-commit: 60c60b627e81bf84e1cb01729d2ae882178f079d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-361%2Fdscho%2Fgit-gui-hooks-path-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-361/dscho/git-gui-hooks-path-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/361 Range-diff vs v3: 1: 65c2fa33e1 ! 1: 2f55d6fb2a Fix gitdir e.g. to respect core.hooksPath @@ -1,17 +1,21 @@ Author: Johannes Schindelin <Johannes.Schindelin@gmx.de> - Fix gitdir e.g. to respect core.hooksPath + Make gitdir work with worktrees, respect core.hooksPath, etc - 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.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. + Since v2.10.0, the `--git-path` option respects that config variable, + too, so we may just as well use that command. + + Other paths inside `.git` are equally subject to differ from + `<gitdir>/<path>`, i.e. inside worktrees, where _some_ paths live in the + "gitdir" and some live in the "commondir" (i.e. the "gitdir" of the main + worktree). 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. + 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` @@ -19,7 +23,9 @@ 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. + `git rev-parse invocation` with some paths (that have been + determined via a typical startup and via grepping the source code for + calls to the `gitdir` function). This fixes https://github.com/git-for-windows/git/issues/1755 @@ -48,6 +54,8 @@ + + # `--git-path` is only supported since Git v2.5.0 + if {[package vcompare $::_git_version 2.5.0] >= 0} { ++ # This list was generated from a typical startup as well as from ++ # grepping through Git GUI's source code. + set gitdir_keys [list \ + CHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \ + GITGUI_MSG HEAD hooks hooks/prepare-commit-msg \ @@ -106,3 +114,21 @@ set _prefix [git rev-parse --show-prefix] } err]} { load_config 1 +@@ + global HEAD PARENT MERGE_HEAD commit_type + global ui_index ui_workdir ui_comm + global rescan_active file_states +- global repo_config ++ global repo_config _gitdir_cache + + if {$rescan_active > 0 || ![lock_index read]} return + ++ # Only re-prime gitdir cache on a full rescan ++ if {$after ne "ui_ready"} { ++ array unset _gitdir_cache ++ prime_gitdir_cache ++ } ++ + repository_state newType newHEAD newMERGE_HEAD + if {[string match amend* $commit_type] + && $newType eq {normal} -- gitgitgadget ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc 2019-10-08 11:33 ` [PATCH v4 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks Johannes Schindelin via GitGitGadget @ 2019-10-08 11:33 ` Johannes Schindelin via GitGitGadget 2019-10-11 22:26 ` Pratyush Yadav 0 siblings, 1 reply; 24+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2019-10-08 11:33 UTC (permalink / raw) To: git Cc: Pratyush Yadav, Bert Wesarg, Johannes Schindelin, Pratyush Yadav, Johannes Schindelin 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. Other paths inside `.git` are equally subject to differ from `<gitdir>/<path>`, i.e. inside worktrees, where _some_ paths live in the "gitdir" and some live in the "commondir" (i.e. the "gitdir" of the main worktree). 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 some paths (that have been determined via a typical startup and via grepping the source code for calls to the `gitdir` function). 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 | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 4 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index fd476b6999..c684dc7ae1 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,59 @@ 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} { + # This list was generated from a typical startup as well as from + # grepping through Git GUI's source code. + 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 +1290,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 @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} { global HEAD PARENT MERGE_HEAD commit_type global ui_index ui_workdir ui_comm global rescan_active file_states - global repo_config + global repo_config _gitdir_cache if {$rescan_active > 0 || ![lock_index read]} return + # Only re-prime gitdir cache on a full rescan + if {$after ne "ui_ready"} { + array unset _gitdir_cache + prime_gitdir_cache + } + repository_state newType newHEAD newMERGE_HEAD if {[string match amend* $commit_type] && $newType eq {normal} -- gitgitgadget ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc 2019-10-08 11:33 ` [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc Johannes Schindelin via GitGitGadget @ 2019-10-11 22:26 ` Pratyush Yadav 2019-10-12 21:24 ` Johannes Schindelin 0 siblings, 1 reply; 24+ messages in thread From: Pratyush Yadav @ 2019-10-11 22:26 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Bert Wesarg, Johannes Schindelin Hi Johannes, Thanks for the re-roll. Some comments below... On 08/10/19 04:33AM, 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. > > Other paths inside `.git` are equally subject to differ from > `<gitdir>/<path>`, i.e. inside worktrees, where _some_ paths live in the > "gitdir" and some live in the "commondir" (i.e. the "gitdir" of the main > worktree). > > 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 some paths (that have been > determined via a typical startup and via grepping the source code for > calls to the `gitdir` function). > > 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 | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 58 insertions(+), 4 deletions(-) > > diff --git a/git-gui.sh b/git-gui.sh > index fd476b6999..c684dc7ae1 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,59 @@ 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} { > + # This list was generated from a typical startup as well as from > + # grepping through Git GUI's source code. > + 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 +1290,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 Looks good till here. > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} { > global HEAD PARENT MERGE_HEAD commit_type > global ui_index ui_workdir ui_comm > global rescan_active file_states > - global repo_config > + global repo_config _gitdir_cache > > if {$rescan_active > 0 || ![lock_index read]} return > > + # Only re-prime gitdir cache on a full rescan > + if {$after ne "ui_ready"} { What do you mean by a "full rescan"? I assume you use it as the differentiator between `ui_do_rescan` (called when you hit F5 or choose rescan from the menu) and `do_rescan` (called when you revert a line or hunk), and a "full rescan" refers to `ui_do_rescan`. Well in that case, this check is incorrect. `do_rescan` passes only "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready". But either way, I'm not a big fan of this. This check makes assumptions about the behaviour of its callers based on what they pass to $after. The way I see it, $after should be a black box to `rescan`, and it should make absolutely no assumptions about it. Doing it this way is really brittle, and would break as soon as someone changes the behaviour of `ui_do_rescan`. If someone in the future passes a different value in $after, this would stop working as intended and would not refresh the cached list on a rescan. So, I think a better place for this if statement would be in `ui_do_rescan`. This would mean adding a new function that does this. But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not to), we can get away with just something like: proc ui_do_rescan {} { rescan {prime_gitdir_cache; ui_ready} } Though since `prime_gitdir_cache` does not really depend on the rescan being finished, something like this would also work fine: proc ui_do_rescan {} { rescan ui_ready prime_gitdir_cache } This would allow us to do these two things in parallel since `rescan` is asynchronous. But that would also mean it is possible that the status bar would show "Ready" while `prime_gitdir_cache` is still executing. I can't really make up my mind on what is better. I'm inclining on using the latter way, effectively trading a bit of UI inconsistency for performance (at least in theory). Thoughts? > + array unset _gitdir_cache > + prime_gitdir_cache > + } > + > repository_state newType newHEAD newMERGE_HEAD > if {[string match amend* $commit_type] > && $newType eq {normal} -- Regards, Pratyush Yadav ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc 2019-10-11 22:26 ` Pratyush Yadav @ 2019-10-12 21:24 ` Johannes Schindelin 2019-10-13 18:55 ` Pratyush Yadav 0 siblings, 1 reply; 24+ messages in thread From: Johannes Schindelin @ 2019-10-12 21:24 UTC (permalink / raw) To: Pratyush Yadav; +Cc: Johannes Schindelin via GitGitGadget, git, Bert Wesarg Hi Pratyush, On Sat, 12 Oct 2019, Pratyush Yadav wrote: > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote: > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} { > > global HEAD PARENT MERGE_HEAD commit_type > > global ui_index ui_workdir ui_comm > > global rescan_active file_states > > - global repo_config > > + global repo_config _gitdir_cache > > > > if {$rescan_active > 0 || ![lock_index read]} return > > > > + # Only re-prime gitdir cache on a full rescan > > + if {$after ne "ui_ready"} { > > What do you mean by a "full rescan"? I assume you use it as the > differentiator between `ui_do_rescan` (called when you hit F5 or choose > rescan from the menu) and `do_rescan` (called when you revert a line or > hunk), and a "full rescan" refers to `ui_do_rescan`. > > Well in that case, this check is incorrect. `do_rescan` passes only > "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready". > > But either way, I'm not a big fan of this. This check makes assumptions > about the behaviour of its callers based on what they pass to $after. > The way I see it, $after should be a black box to `rescan`, and it > should make absolutely no assumptions about it. > > Doing it this way is really brittle, and would break as soon as someone > changes the behaviour of `ui_do_rescan`. If someone in the future passes > a different value in $after, this would stop working as intended and > would not refresh the cached list on a rescan. > > So, I think a better place for this if statement would be in > `ui_do_rescan`. This would mean adding a new function that does this. > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not > to), we can get away with just something like: > > proc ui_do_rescan {} { > rescan {prime_gitdir_cache; ui_ready} > } > > Though since `prime_gitdir_cache` does not really depend on the rescan > being finished, something like this would also work fine: > > proc ui_do_rescan {} { > rescan ui_ready > prime_gitdir_cache > } That was my first attempt. However, there is a very important piece of code that is even still quoted above: that `if {$rescan_active > 0 || ![lock_index read]} return` part. I do _not_ want to interfere with an actively-going-on rescan. If there is an active one, I don't want to re-prime the `_gitdir` cache. That was the reason why I put the additional code into `rescan` rather than into `ui_do_rescan()`. Ciao, Johannes > > This would allow us to do these two things in parallel since `rescan` is > asynchronous. But that would also mean it is possible that the status > bar would show "Ready" while `prime_gitdir_cache` is still executing. > > I can't really make up my mind on what is better. I'm inclining on using > the latter way, effectively trading a bit of UI inconsistency for > performance (at least in theory). > > Thoughts? > > > + array unset _gitdir_cache > > + prime_gitdir_cache > > + } > > + > > repository_state newType newHEAD newMERGE_HEAD > > if {[string match amend* $commit_type] > > && $newType eq {normal} > > -- > Regards, > Pratyush Yadav > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc 2019-10-12 21:24 ` Johannes Schindelin @ 2019-10-13 18:55 ` Pratyush Yadav 2019-10-13 22:18 ` Johannes Schindelin 2019-10-14 8:14 ` Johannes Schindelin 0 siblings, 2 replies; 24+ messages in thread From: Pratyush Yadav @ 2019-10-13 18:55 UTC (permalink / raw) To: Johannes Schindelin Cc: Johannes Schindelin via GitGitGadget, git, Bert Wesarg On 12/10/19 11:24PM, Johannes Schindelin wrote: > Hi Pratyush, > > On Sat, 12 Oct 2019, Pratyush Yadav wrote: > > > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote: > > > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} { > > > global HEAD PARENT MERGE_HEAD commit_type > > > global ui_index ui_workdir ui_comm > > > global rescan_active file_states > > > - global repo_config > > > + global repo_config _gitdir_cache > > > > > > if {$rescan_active > 0 || ![lock_index read]} return > > > > > > + # Only re-prime gitdir cache on a full rescan > > > + if {$after ne "ui_ready"} { > > > > What do you mean by a "full rescan"? I assume you use it as the > > differentiator between `ui_do_rescan` (called when you hit F5 or choose > > rescan from the menu) and `do_rescan` (called when you revert a line or > > hunk), and a "full rescan" refers to `ui_do_rescan`. > > > > Well in that case, this check is incorrect. `do_rescan` passes only > > "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready". > > > > But either way, I'm not a big fan of this. This check makes assumptions > > about the behaviour of its callers based on what they pass to $after. > > The way I see it, $after should be a black box to `rescan`, and it > > should make absolutely no assumptions about it. > > > > Doing it this way is really brittle, and would break as soon as someone > > changes the behaviour of `ui_do_rescan`. If someone in the future passes > > a different value in $after, this would stop working as intended and > > would not refresh the cached list on a rescan. > > > > So, I think a better place for this if statement would be in > > `ui_do_rescan`. This would mean adding a new function that does this. > > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not > > to), we can get away with just something like: > > > > proc ui_do_rescan {} { > > rescan {prime_gitdir_cache; ui_ready} > > } > > > > Though since `prime_gitdir_cache` does not really depend on the rescan > > being finished, something like this would also work fine: > > > > proc ui_do_rescan {} { > > rescan ui_ready > > prime_gitdir_cache > > } > > That was my first attempt. However, there is a very important piece of > code that is even still quoted above: that `if {$rescan_active > 0 || > ![lock_index read]} return` part. > > I do _not_ want to interfere with an actively-going-on rescan. If there > is an active one, I don't want to re-prime the `_gitdir` cache. Good catch! In that case I suppose refreshing the cache in $after would be the way to go (IOW, the former of my two suggestions). Anything $after won't get executed if we return early from that check. > That was the reason why I put the additional code into `rescan` rather > than into `ui_do_rescan()`. > > Ciao, > Johannes > > > > > This would allow us to do these two things in parallel since `rescan` is > > asynchronous. But that would also mean it is possible that the status > > bar would show "Ready" while `prime_gitdir_cache` is still executing. > > > > I can't really make up my mind on what is better. I'm inclining on using > > the latter way, effectively trading a bit of UI inconsistency for > > performance (at least in theory). > > > > Thoughts? > > > > > + array unset _gitdir_cache > > > + prime_gitdir_cache > > > + } > > > + > > > repository_state newType newHEAD newMERGE_HEAD > > > if {[string match amend* $commit_type] > > > && $newType eq {normal} -- Regards, Pratyush Yadav ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc 2019-10-13 18:55 ` Pratyush Yadav @ 2019-10-13 22:18 ` Johannes Schindelin 2019-10-17 18:34 ` Pratyush Yadav 2019-10-14 8:14 ` Johannes Schindelin 1 sibling, 1 reply; 24+ messages in thread From: Johannes Schindelin @ 2019-10-13 22:18 UTC (permalink / raw) To: Pratyush Yadav; +Cc: Johannes Schindelin via GitGitGadget, git, Bert Wesarg Hi Pratyush, On Mon, 14 Oct 2019, Pratyush Yadav wrote: > On 12/10/19 11:24PM, Johannes Schindelin wrote: > > Hi Pratyush, > > > > On Sat, 12 Oct 2019, Pratyush Yadav wrote: > > > > > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote: > > > > > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} { > > > > global HEAD PARENT MERGE_HEAD commit_type > > > > global ui_index ui_workdir ui_comm > > > > global rescan_active file_states > > > > - global repo_config > > > > + global repo_config _gitdir_cache > > > > > > > > if {$rescan_active > 0 || ![lock_index read]} return > > > > > > > > + # Only re-prime gitdir cache on a full rescan > > > > + if {$after ne "ui_ready"} { > > > > > > What do you mean by a "full rescan"? I assume you use it as the > > > differentiator between `ui_do_rescan` (called when you hit F5 or choose > > > rescan from the menu) and `do_rescan` (called when you revert a line or > > > hunk), and a "full rescan" refers to `ui_do_rescan`. > > > > > > Well in that case, this check is incorrect. `do_rescan` passes only > > > "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready". > > > > > > But either way, I'm not a big fan of this. This check makes assumptions > > > about the behaviour of its callers based on what they pass to $after. > > > The way I see it, $after should be a black box to `rescan`, and it > > > should make absolutely no assumptions about it. > > > > > > Doing it this way is really brittle, and would break as soon as someone > > > changes the behaviour of `ui_do_rescan`. If someone in the future passes > > > a different value in $after, this would stop working as intended and > > > would not refresh the cached list on a rescan. > > > > > > So, I think a better place for this if statement would be in > > > `ui_do_rescan`. This would mean adding a new function that does this. > > > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not > > > to), we can get away with just something like: > > > > > > proc ui_do_rescan {} { > > > rescan {prime_gitdir_cache; ui_ready} > > > } > > > > > > Though since `prime_gitdir_cache` does not really depend on the rescan > > > being finished, something like this would also work fine: > > > > > > proc ui_do_rescan {} { > > > rescan ui_ready > > > prime_gitdir_cache > > > } > > > > That was my first attempt. However, there is a very important piece of > > code that is even still quoted above: that `if {$rescan_active > 0 || > > ![lock_index read]} return` part. > > > > I do _not_ want to interfere with an actively-going-on rescan. If there > > is an active one, I don't want to re-prime the `_gitdir` cache. > > Good catch! In that case I suppose refreshing the cache in $after would > be the way to go (IOW, the former of my two suggestions). Anything > $after won't get executed if we return early from that check. The obvious problem with that solution is that the `_gitdir` is reset _after_ the rescan. In my solution, it is reset _before_, as I have no idea how often the `_gitdir` values are used during a rescan (and if none of they were, I would like to be very cautious to prepare for a future where any of those `_gitdir` values _are_ used during a rescan). So I am afraid that I find way too serious problems with both of your proposed alternatives. Ciao, Johannes > > > That was the reason why I put the additional code into `rescan` rather > > than into `ui_do_rescan()`. > > > > Ciao, > > Johannes > > > > > > > > This would allow us to do these two things in parallel since `rescan` is > > > asynchronous. But that would also mean it is possible that the status > > > bar would show "Ready" while `prime_gitdir_cache` is still executing. > > > > > > I can't really make up my mind on what is better. I'm inclining on using > > > the latter way, effectively trading a bit of UI inconsistency for > > > performance (at least in theory). > > > > > > Thoughts? > > > > > > > + array unset _gitdir_cache > > > > + prime_gitdir_cache > > > > + } > > > > + > > > > repository_state newType newHEAD newMERGE_HEAD > > > > if {[string match amend* $commit_type] > > > > && $newType eq {normal} > > -- > Regards, > Pratyush Yadav > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc 2019-10-13 22:18 ` Johannes Schindelin @ 2019-10-17 18:34 ` Pratyush Yadav 0 siblings, 0 replies; 24+ messages in thread From: Pratyush Yadav @ 2019-10-17 18:34 UTC (permalink / raw) To: Johannes Schindelin Cc: Johannes Schindelin via GitGitGadget, git, Bert Wesarg On 14/10/19 12:18AM, Johannes Schindelin wrote: > Hi Pratyush, > > On Mon, 14 Oct 2019, Pratyush Yadav wrote: > > > On 12/10/19 11:24PM, Johannes Schindelin wrote: > > > Hi Pratyush, > > > > > > On Sat, 12 Oct 2019, Pratyush Yadav wrote: > > > > > > > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote: > > > > > > > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} { > > > > > global HEAD PARENT MERGE_HEAD commit_type > > > > > global ui_index ui_workdir ui_comm > > > > > global rescan_active file_states > > > > > - global repo_config > > > > > + global repo_config _gitdir_cache > > > > > > > > > > if {$rescan_active > 0 || ![lock_index read]} return > > > > > > > > > > + # Only re-prime gitdir cache on a full rescan > > > > > + if {$after ne "ui_ready"} { > > > > > > > > What do you mean by a "full rescan"? I assume you use it as the > > > > differentiator between `ui_do_rescan` (called when you hit F5 or choose > > > > rescan from the menu) and `do_rescan` (called when you revert a line or > > > > hunk), and a "full rescan" refers to `ui_do_rescan`. > > > > > > > > Well in that case, this check is incorrect. `do_rescan` passes only > > > > "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready". > > > > > > > > But either way, I'm not a big fan of this. This check makes assumptions > > > > about the behaviour of its callers based on what they pass to $after. > > > > The way I see it, $after should be a black box to `rescan`, and it > > > > should make absolutely no assumptions about it. > > > > > > > > Doing it this way is really brittle, and would break as soon as someone > > > > changes the behaviour of `ui_do_rescan`. If someone in the future passes > > > > a different value in $after, this would stop working as intended and > > > > would not refresh the cached list on a rescan. > > > > > > > > So, I think a better place for this if statement would be in > > > > `ui_do_rescan`. This would mean adding a new function that does this. > > > > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not > > > > to), we can get away with just something like: > > > > > > > > proc ui_do_rescan {} { > > > > rescan {prime_gitdir_cache; ui_ready} > > > > } > > > > > > > > Though since `prime_gitdir_cache` does not really depend on the rescan > > > > being finished, something like this would also work fine: > > > > > > > > proc ui_do_rescan {} { > > > > rescan ui_ready > > > > prime_gitdir_cache > > > > } > > > > > > That was my first attempt. However, there is a very important piece of > > > code that is even still quoted above: that `if {$rescan_active > 0 || > > > ![lock_index read]} return` part. > > > > > > I do _not_ want to interfere with an actively-going-on rescan. If there > > > is an active one, I don't want to re-prime the `_gitdir` cache. > > > > Good catch! In that case I suppose refreshing the cache in $after would > > be the way to go (IOW, the former of my two suggestions). Anything > > $after won't get executed if we return early from that check. > > The obvious problem with that solution is that the `_gitdir` is reset > _after_ the rescan. In my solution, it is reset _before_, as I have no > idea how often the `_gitdir` values are used during a rescan (and if > none of they were, I would like to be very cautious to prepare for a > future where any of those `_gitdir` values _are_ used during a rescan). _gitdir values are used quite often during a rescan, so yes, this won't work. > So I am afraid that I find way too serious problems with both of your > proposed alternatives. One alternative I can see right now is adding another optional parameter to `rescan` that controls whether we refresh the gitdir cache or not. That parameter would default to 0/false. I'm not the biggest fan of something like this, but it might be the easiest way to do it given the constraints. I also thought about trying to acquire the index lock in `prime_gitdir_cache`, but that could create a race on the lock between `prime_gitdir_cache` and `rescan`. If you have any better ideas, I'm all ears, but otherwise, I maybe our best bet is to just go with adding an optional parameter to `rescan`. -- Regards, Pratyush Yadav ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc 2019-10-13 18:55 ` Pratyush Yadav 2019-10-13 22:18 ` Johannes Schindelin @ 2019-10-14 8:14 ` Johannes Schindelin 1 sibling, 0 replies; 24+ messages in thread From: Johannes Schindelin @ 2019-10-14 8:14 UTC (permalink / raw) To: Pratyush Yadav; +Cc: Johannes Schindelin via GitGitGadget, git, Bert Wesarg Hi Pratyush, On Mon, 14 Oct 2019, Pratyush Yadav wrote: > On 12/10/19 11:24PM, Johannes Schindelin wrote: > > > > On Sat, 12 Oct 2019, Pratyush Yadav wrote: > > > > > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote: > > > > > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} { > > > > global HEAD PARENT MERGE_HEAD commit_type > > > > global ui_index ui_workdir ui_comm > > > > global rescan_active file_states > > > > - global repo_config > > > > + global repo_config _gitdir_cache > > > > > > > > if {$rescan_active > 0 || ![lock_index read]} return > > > > > > > > + # Only re-prime gitdir cache on a full rescan > > > > + if {$after ne "ui_ready"} { > > > > > > What do you mean by a "full rescan"? I assume you use it as the > > > differentiator between `ui_do_rescan` (called when you hit F5 or choose > > > rescan from the menu) and `do_rescan` (called when you revert a line or > > > hunk), and a "full rescan" refers to `ui_do_rescan`. > > > > > > Well in that case, this check is incorrect. `do_rescan` passes only > > > "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready". > > > > > > But either way, I'm not a big fan of this. This check makes assumptions > > > about the behaviour of its callers based on what they pass to $after. > > > The way I see it, $after should be a black box to `rescan`, and it > > > should make absolutely no assumptions about it. > > > > > > Doing it this way is really brittle, and would break as soon as someone > > > changes the behaviour of `ui_do_rescan`. If someone in the future passes > > > a different value in $after, this would stop working as intended and > > > would not refresh the cached list on a rescan. > > > > > > So, I think a better place for this if statement would be in > > > `ui_do_rescan`. This would mean adding a new function that does this. > > > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not > > > to), we can get away with just something like: > > > > > > proc ui_do_rescan {} { > > > rescan {prime_gitdir_cache; ui_ready} > > > } > > > > > > Though since `prime_gitdir_cache` does not really depend on the rescan > > > being finished, something like this would also work fine: > > > > > > proc ui_do_rescan {} { > > > rescan ui_ready > > > prime_gitdir_cache > > > } > > > > That was my first attempt. However, there is a very important piece of > > code that is even still quoted above: that `if {$rescan_active > 0 || > > ![lock_index read]} return` part. > > > > I do _not_ want to interfere with an actively-going-on rescan. If there > > is an active one, I don't want to re-prime the `_gitdir` cache. > > Good catch! In that case I suppose refreshing the cache in $after would > be the way to go (IOW, the former of my two suggestions). Anything > $after won't get executed if we return early from that check. But it also won't get executed before the actual rescan. Which is precisely what I tried to ensure. Ciao, Johannes > > > That was the reason why I put the additional code into `rescan` rather > > than into `ui_do_rescan()`. > > > > Ciao, > > Johannes > > > > > > > > This would allow us to do these two things in parallel since `rescan` is > > > asynchronous. But that would also mean it is possible that the status > > > bar would show "Ready" while `prime_gitdir_cache` is still executing. > > > > > > I can't really make up my mind on what is better. I'm inclining on using > > > the latter way, effectively trading a bit of UI inconsistency for > > > performance (at least in theory). > > > > > > Thoughts? > > > > > > > + array unset _gitdir_cache > > > > + prime_gitdir_cache > > > > + } > > > > + > > > > repository_state newType newHEAD newMERGE_HEAD > > > > if {[string match amend* $commit_type] > > > > && $newType eq {normal} > > -- > Regards, > Pratyush Yadav > ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2019-10-17 18:34 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-26 21:17 [PATCH 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks Johannes Schindelin via GitGitGadget 2019-09-26 21:17 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget 2019-09-26 22:36 ` Pratyush Yadav 2019-09-27 6:10 ` Bert Wesarg 2019-09-27 13:05 ` Pratyush Yadav 2019-09-30 9:42 ` Johannes Schindelin 2019-10-01 13:31 ` Pratyush Yadav 2019-10-01 17:38 ` Johannes Schindelin 2019-10-04 16:48 ` Pratyush Yadav 2019-10-04 19:56 ` Johannes Schindelin 2019-09-30 9:45 ` [PATCH v2 0/1] git-gui: " Johannes Schindelin via GitGitGadget 2019-09-30 9:45 ` [PATCH v2 1/1] " Johannes Schindelin via GitGitGadget 2019-10-04 21:41 ` [PATCH v3 0/1] git-gui: " Johannes Schindelin via GitGitGadget 2019-10-04 21:41 ` [PATCH v3 1/1] Fix gitdir e.g. to respect core.hooksPath Johannes Schindelin via GitGitGadget 2019-10-08 0:29 ` Pratyush Yadav 2019-10-08 11:30 ` Johannes Schindelin 2019-10-08 11:33 ` [PATCH v4 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks Johannes Schindelin via GitGitGadget 2019-10-08 11:33 ` [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc Johannes Schindelin via GitGitGadget 2019-10-11 22:26 ` Pratyush Yadav 2019-10-12 21:24 ` Johannes Schindelin 2019-10-13 18:55 ` Pratyush Yadav 2019-10-13 22:18 ` Johannes Schindelin 2019-10-17 18:34 ` Pratyush Yadav 2019-10-14 8:14 ` Johannes Schindelin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).