{"thread":{"id":"51931","subject":"[PATCH 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks","startedAt":"2019-09-26T21:17:13Z","lastAt":"2019-10-17T18:34:43Z","messageCount":24,"participants":["Johannes Schindelin via GitGitGadget","Pratyush Yadav","Bert Wesarg","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"383033","messageId":"pull.361.git.gitgitgadget@gmail.com","threadId":"51931","inReplyTo":null,"subject":"[PATCH 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-09-26T21:17:09Z","receivedAt":"2019-09-26T21:17:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is yet another patch from Git for Windows.\n\nJohannes Schindelin (1):\n  respect core.hooksPath, falling back to .git/hooks\n\n git-gui.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\n\nbase-commit: 60c60b627e81bf84e1cb01729d2ae882178f079d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-361%2Fdscho%2Fgit-gui-hooks-path-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-361/dscho/git-gui-hooks-path-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/361\n-- \ngitgitgadget\n"},{"id":"383034","messageId":"eca193f91b85fc4ffea453bc3adb64bc5c8831a8.1569532628.git.gitgitgadget@gmail.com","threadId":"51931","inReplyTo":"pull.361.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] respect core.hooksPath, falling back to .git/hooks","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-09-26T21:17:10Z","receivedAt":"2019-09-26T21:17:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n\nSince v2.9.0, Git knows about the config variable core.hookspath\nthat allows overriding the path to the directory containing the\nGit hooks.\n\nSince v2.10.0, the `--git-path` option respects that config\nvariable, too, so we may just as well use that command.\n\nFor Git versions older than v2.5.0 (which was the first version to\nsupport the `--git-path` option for the `rev-parse` command), we\nsimply fall back to the previous code.\n\nThis fixes https://github.com/git-for-windows/git/issues/1755\n\nInitial-patch-by: Philipp Gortan <philipp@gortan.org>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n git-gui.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex fd476b6999..b2c6e7a1db 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -623,7 +623,11 @@ proc git_write {args} {\n }\n \n proc githook_read {hook_name args} {\n-\tset pchook [gitdir hooks $hook_name]\n+\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n+\t\tset pchook [git rev-parse --git-path \"hooks/$hook_name\"]\n+\t} else {\n+\t\tset pchook [gitdir hooks $hook_name]\n+\t}\n \tlappend args 2>@1\n \n \t# On Windows [file executable] might lie so we need to ask\n-- \ngitgitgadget\n"},{"id":"383042","messageId":"20190926223638.6tk2qhc4e62hs2wt@yadavpratyush.com","threadId":"51931","inReplyTo":"eca193f91b85fc4ffea453bc3adb64bc5c8831a8.1569532628.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-09-26T22:36:38Z","receivedAt":"2019-09-26T22:36:45Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"Hi,\n\nOn 26/09/19 02:17PM, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> \n> Since v2.9.0, Git knows about the config variable core.hookspath\n> that allows overriding the path to the directory containing the\n> Git hooks.\n> \n> Since v2.10.0, the `--git-path` option respects that config\n> variable, too, so we may just as well use that command.\n> \n> For Git versions older than v2.5.0 (which was the first version to\n> support the `--git-path` option for the `rev-parse` command), we\n> simply fall back to the previous code.\n> \n> This fixes https://github.com/git-for-windows/git/issues/1755\n> \n> Initial-patch-by: Philipp Gortan <philipp@gortan.org>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  git-gui.sh | 6 +++++-\n>  1 file changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git a/git-gui.sh b/git-gui.sh\n> index fd476b6999..b2c6e7a1db 100755\n> --- a/git-gui.sh\n> +++ b/git-gui.sh\n> @@ -623,7 +623,11 @@ proc git_write {args} {\n>  }\n>  \n>  proc githook_read {hook_name args} {\n> -\tset pchook [gitdir hooks $hook_name]\n> +\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n> +\t\tset pchook [git rev-parse --git-path \"hooks/$hook_name\"]\n> +\t} else {\n> +\t\tset pchook [gitdir hooks $hook_name]\n> +\t}\n\ngitdir is used in a lot of places, and I think all those would also \nbenefit from using --git-path. So I think it is a better idea to move \nthis to the procedure gitdir. It would have to be refactored to take any \nnumber of arguments, instead of the two it takes here.\n\nOther than that, looks good. Thanks.\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"383054","messageId":"CAKPyHN1P713bTb2TYXFuXcM5Dg=7vXBVgchwvJUrNsQ6EcP5Rg@mail.gmail.com","threadId":"51931","inReplyTo":"20190926223638.6tk2qhc4e62hs2wt@yadavpratyush.com","subject":"Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2019-09-27T06:10:45Z","receivedAt":"2019-09-27T06:10:59Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote:\n>\n> Hi,\n>\n> On 26/09/19 02:17PM, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> >\n> > Since v2.9.0, Git knows about the config variable core.hookspath\n> > that allows overriding the path to the directory containing the\n> > Git hooks.\n> >\n> > Since v2.10.0, the `--git-path` option respects that config\n> > variable, too, so we may just as well use that command.\n> >\n> > For Git versions older than v2.5.0 (which was the first version to\n> > support the `--git-path` option for the `rev-parse` command), we\n> > simply fall back to the previous code.\n> >\n> > This fixes https://github.com/git-for-windows/git/issues/1755\n> >\n> > Initial-patch-by: Philipp Gortan <philipp@gortan.org>\n> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > ---\n> >  git-gui.sh | 6 +++++-\n> >  1 file changed, 5 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/git-gui.sh b/git-gui.sh\n> > index fd476b6999..b2c6e7a1db 100755\n> > --- a/git-gui.sh\n> > +++ b/git-gui.sh\n> > @@ -623,7 +623,11 @@ proc git_write {args} {\n> >  }\n> >\n> >  proc githook_read {hook_name args} {\n> > -     set pchook [gitdir hooks $hook_name]\n> > +     if {[package vcompare $::_git_version 2.5.0] >= 0} {\n> > +             set pchook [git rev-parse --git-path \"hooks/$hook_name\"]\n> > +     } else {\n> > +             set pchook [gitdir hooks $hook_name]\n> > +     }\n>\n> gitdir is used in a lot of places, and I think all those would also\n> benefit from using --git-path. So I think it is a better idea to move\n> this to the procedure gitdir. It would have to be refactored to take any\n> number of arguments, instead of the two it takes here.\n\ngitdir already takes an arbitrary number of arguments and joins them\nto a path. The more imminent challenge is, that gitdir caches the\nGIT_DIR, thus it tries to avoid calling \"git rev-parse\". Which works\nfor most, but not for hooks.\n\nWe could either maintain a blacklist, for what we cache the result\ntoo, or always call \"git rev-parse --git-dir\".\n\nThis blacklist would need to be in sync with the one in Git's\npath.c::adjust_git_path() than.\n\nBert\n\n>\n> Other than that, looks good. Thanks.\n>\n> --\n> Regards,\n> Pratyush Yadav\n"},{"id":"383064","messageId":"20190927130539.52oir56byuedqmm3@yadavpratyush.com","threadId":"51931","inReplyTo":"CAKPyHN1P713bTb2TYXFuXcM5Dg=7vXBVgchwvJUrNsQ6EcP5Rg@mail.gmail.com","subject":"Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-09-27T13:05:39Z","receivedAt":"2019-09-27T13:05:45Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 27/09/19 08:10AM, Bert Wesarg wrote:\n> On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote:\n> >\n> > Hi,\n> >\n> > On 26/09/19 02:17PM, Johannes Schindelin via GitGitGadget wrote:\n> > > From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> > >\n> > > Since v2.9.0, Git knows about the config variable core.hookspath\n> > > that allows overriding the path to the directory containing the\n> > > Git hooks.\n> > >\n> > > Since v2.10.0, the `--git-path` option respects that config\n> > > variable, too, so we may just as well use that command.\n> > >\n> > > For Git versions older than v2.5.0 (which was the first version to\n> > > support the `--git-path` option for the `rev-parse` command), we\n> > > simply fall back to the previous code.\n> > >\n> > > This fixes https://github.com/git-for-windows/git/issues/1755\n> > >\n> > > Initial-patch-by: Philipp Gortan <philipp@gortan.org>\n> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > > ---\n> > >  git-gui.sh | 6 +++++-\n> > >  1 file changed, 5 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/git-gui.sh b/git-gui.sh\n> > > index fd476b6999..b2c6e7a1db 100755\n> > > --- a/git-gui.sh\n> > > +++ b/git-gui.sh\n> > > @@ -623,7 +623,11 @@ proc git_write {args} {\n> > >  }\n> > >\n> > >  proc githook_read {hook_name args} {\n> > > -     set pchook [gitdir hooks $hook_name]\n> > > +     if {[package vcompare $::_git_version 2.5.0] >= 0} {\n> > > +             set pchook [git rev-parse --git-path \"hooks/$hook_name\"]\n> > > +     } else {\n> > > +             set pchook [gitdir hooks $hook_name]\n> > > +     }\n> >\n> > gitdir is used in a lot of places, and I think all those would also\n> > benefit from using --git-path. So I think it is a better idea to move\n> > this to the procedure gitdir. It would have to be refactored to take any\n> > number of arguments, instead of the two it takes here.\n> \n> gitdir already takes an arbitrary number of arguments and joins them\n> to a path. The more imminent challenge is, that gitdir caches the\n> GIT_DIR, thus it tries to avoid calling \"git rev-parse\". Which works\n> for most, but not for hooks.\n\nWhat I was thinking of was something like this:\n\n  - If no args are passed, then just directly return $_gitdir. This is \n    already being done. I assume the GIT_DIR relocation is already \n    handled by `git rev-parse --git-dir`, so this would point to the \n    correct location.\n  - If args are passed, then we want a subdirectory of GIT_DIR In this \n    case, it is possible that this subdirectory has also been relocated \n    (hooks/ being one of those subdirectories). So in this case, use \n    `git rev-parse --git-path` instead.\n\nSo the gitdir procedure would look something like:\n\n  proc gitdir {args} {\n  \tglobal $_gitdir\n  \tif {$args eq {}} {\n  \t\t# Return the cached GIT_DIR\n  \t\treturn $_gitdir\n  \t}\n  \n  \t# Use `git rev-parse --git-path` to get the path instead of \n  \t# using the cached value.\n  }\n\nAm I missing something? Or does this fix the issue you describe?\n \n> We could either maintain a blacklist, for what we cache the result\n> too, or always call \"git rev-parse --git-dir\".\n> \n> This blacklist would need to be in sync with the one in Git's\n> path.c::adjust_git_path() than.\n\nIs caching GIT_DIR that important in terms of performance? Otherwise, \nI'd say calling `git rev-parse --git-path` for _every_ subdirectory of \nGIT_DIR is a much simpler solution.\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"383131","messageId":"nycvar.QRO.7.76.6.1909301139460.46@tvgsbejvaqbjf.bet","threadId":"51931","inReplyTo":"20190927130539.52oir56byuedqmm3@yadavpratyush.com","subject":"Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-09-30T09:42:30Z","receivedAt":"2019-09-30T09:42:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 27 Sep 2019, Pratyush Yadav wrote:\n\n> On 27/09/19 08:10AM, Bert Wesarg wrote:\n> > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote:\n> > >\n> > > On 26/09/19 02:17PM, Johannes Schindelin via GitGitGadget wrote:\n> > > > From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> > > >\n> > > > Since v2.9.0, Git knows about the config variable core.hookspath\n> > > > that allows overriding the path to the directory containing the\n> > > > Git hooks.\n> > > >\n> > > > Since v2.10.0, the `--git-path` option respects that config\n> > > > variable, too, so we may just as well use that command.\n> > > >\n> > > > For Git versions older than v2.5.0 (which was the first version to\n> > > > support the `--git-path` option for the `rev-parse` command), we\n> > > > simply fall back to the previous code.\n> > > >\n> > > > This fixes https://github.com/git-for-windows/git/issues/1755\n> > > >\n> > > > Initial-patch-by: Philipp Gortan <philipp@gortan.org>\n> > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > > > ---\n> > > >  git-gui.sh | 6 +++++-\n> > > >  1 file changed, 5 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/git-gui.sh b/git-gui.sh\n> > > > index fd476b6999..b2c6e7a1db 100755\n> > > > --- a/git-gui.sh\n> > > > +++ b/git-gui.sh\n> > > > @@ -623,7 +623,11 @@ proc git_write {args} {\n> > > >  }\n> > > >\n> > > >  proc githook_read {hook_name args} {\n> > > > -     set pchook [gitdir hooks $hook_name]\n> > > > +     if {[package vcompare $::_git_version 2.5.0] >= 0} {\n> > > > +             set pchook [git rev-parse --git-path \"hooks/$hook_name\"]\n> > > > +     } else {\n> > > > +             set pchook [gitdir hooks $hook_name]\n> > > > +     }\n> > >\n> > > gitdir is used in a lot of places, and I think all those would also\n> > > benefit from using --git-path. So I think it is a better idea to move\n> > > this to the procedure gitdir. It would have to be refactored to take any\n> > > number of arguments, instead of the two it takes here.\n> >\n> > gitdir already takes an arbitrary number of arguments and joins them\n> > to a path. The more imminent challenge is, that gitdir caches the\n> > GIT_DIR, thus it tries to avoid calling \"git rev-parse\". Which works\n> > for most, but not for hooks.\n>\n> What I was thinking of was something like this:\n>\n>   - If no args are passed, then just directly return $_gitdir. This is\n>     already being done. I assume the GIT_DIR relocation is already\n>     handled by `git rev-parse --git-dir`, so this would point to the\n>     correct location.\n>   - If args are passed, then we want a subdirectory of GIT_DIR In this\n>     case, it is possible that this subdirectory has also been relocated\n>     (hooks/ being one of those subdirectories). So in this case, use\n>     `git rev-parse --git-path` instead.\n>\n> So the gitdir procedure would look something like:\n>\n>   proc gitdir {args} {\n>   \tglobal $_gitdir\n>   \tif {$args eq {}} {\n>   \t\t# Return the cached GIT_DIR\n>   \t\treturn $_gitdir\n>   \t}\n>\n>   \t# Use `git rev-parse --git-path` to get the path instead of\n>   \t# using the cached value.\n>   }\n>\n> Am I missing something? Or does this fix the issue you describe?\n\nThe `gitdir` function is called 13 times during startup alone, and who\nknows how many more times later.\n\nSo I am quite convinced that the original intention was to save on\nspawning processes left and right.\n\nBut since you are the Git GUI maintainer, and this was your suggestion,\nI made it so.\n\nCiao,\nJohannes\n\n> > We could either maintain a blacklist, for what we cache the result\n> > too, or always call \"git rev-parse --git-dir\".\n> >\n> > This blacklist would need to be in sync with the one in Git's\n> > path.c::adjust_git_path() than.\n>\n> Is caching GIT_DIR that important in terms of performance? Otherwise,\n> I'd say calling `git rev-parse --git-path` for _every_ subdirectory of\n> GIT_DIR is a much simpler solution.\n>\n> --\n> Regards,\n> Pratyush Yadav\n>\n"},{"id":"383132","messageId":"pull.361.v2.git.gitgitgadget@gmail.com","threadId":"51931","inReplyTo":"pull.361.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-09-30T09:45:04Z","receivedAt":"2019-09-30T09:45:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is yet another patch from Git for Windows.\n\nChanges since v1:\n\n * Rather than a fine-grained override of gitdir just for the hooks path, we\n   now spawn git rev-parse --git-path [...] every time gitdir is called with\n   arguments. This makes the code safer, although at the cost of potentially\n   many spawned processes.\n\nJohannes Schindelin (1):\n  respect core.hooksPath, falling back to .git/hooks\n\n git-gui.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\n\nbase-commit: 60c60b627e81bf84e1cb01729d2ae882178f079d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-361%2Fdscho%2Fgit-gui-hooks-path-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-361/dscho/git-gui-hooks-path-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/361\n\nRange-diff vs v1:\n\n 1:  eca193f91b < -:  ---------- respect core.hooksPath, falling back to .git/hooks\n -:  ---------- > 1:  c101422936 respect core.hooksPath, falling back to .git/hooks\n\n-- \ngitgitgadget\n"},{"id":"383133","messageId":"c101422936d03e26833a82ebeb9ff6f7a199753b.1569836703.git.gitgitgadget@gmail.com","threadId":"51931","inReplyTo":"pull.361.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] respect core.hooksPath, falling back to .git/hooks","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-09-30T09:45:05Z","receivedAt":"2019-09-30T09:45:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n\nSince v2.9.0, Git knows about the config variable core.hookspath\nthat allows overriding the path to the directory containing the\nGit hooks.\n\nSince v2.10.0, the `--git-path` option respects that config\nvariable, too, so we may just as well use that command.\n\nFor Git versions older than v2.5.0 (which was the first version to\nsupport the `--git-path` option for the `rev-parse` command), we\nsimply fall back to the previous code.\n\nAn original patch handled only the hooksPath setting (as the title of\nthis commit message suggests), however, during the code submission it\nwas deemed better to fix all call to the `gitdir` function.\n\nWith this change, we spawn `git rev-parse --git-path [...]` 13 times\nduring Git GUI's startup.\n\nThis fixes https://github.com/git-for-windows/git/issues/1755\n\nInitial-patch-by: Philipp Gortan <philipp@gortan.org>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n git-gui.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex fd476b6999..b2f0e78077 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -202,7 +202,11 @@ proc gitdir {args} {\n \tif {$args eq {}} {\n \t\treturn $_gitdir\n \t}\n-\treturn [eval [list file join $_gitdir] $args]\n+\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n+\t\treturn [git rev-parse --git-path [eval [list file join] $args]]\n+\t} else {\n+\t\treturn [eval [list file join $_gitdir] $args]\n+\t}\n }\n \n proc gitexec {args} {\n-- \ngitgitgadget\n"},{"id":"383205","messageId":"20191001133112.3qh4aeoctkoz2wxy@yadavpratyush.com","threadId":"51931","inReplyTo":"nycvar.QRO.7.76.6.1909301139460.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-10-01T13:31:12Z","receivedAt":"2019-10-01T13:31:18Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 30/09/19 11:42AM, Johannes Schindelin wrote:\n> On Fri, 27 Sep 2019, Pratyush Yadav wrote:\n> > On 27/09/19 08:10AM, Bert Wesarg wrote:\n> > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote:\n> > > > gitdir is used in a lot of places, and I think all those would \n> > > > also\n> > > > benefit from using --git-path. So I think it is a better idea to move\n> > > > this to the procedure gitdir. It would have to be refactored to take any\n> > > > number of arguments, instead of the two it takes here.\n> \n> The `gitdir` function is called 13 times during startup alone, and who\n> knows how many more times later.\n> \n> So I am quite convinced that the original intention was to save on\n> spawning processes left and right.\n> \n> But since you are the Git GUI maintainer, and this was your suggestion,\n> I made it so.\n\nYes, I am the maintainer, but I am not an all-knowing, all-seeing \nentity. Your, and every other contributors, suggestions are very \nvaluable. And my suggestions aren't gospel. I would hate to see someone \nsend in a patch they weren't sure was the best thing to do just because \nI suggested it. Please feel free to object my suggestions.\n\nIn this case, I didn't expect gitdir to be called this many times.\n\nWhile I don't notice much of a performance difference on my system \n(Linux), a quick measurement tells me that the time spent in gitdir is \nabout 16 ms. In contrast, the same measurement without the v2 patch \ngives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something \nso simple. It might not be the same for everyone else. AFAIK, spawning a \nprocess is much slower on Windows.\n\nSo now I'm not so sure my suggestion was a good one. My original aim was \nto be sure everything was correct, and no incorrect directories were \nbeing used. But the current solution comes at a performance hit.\n\n> > > We could either maintain a blacklist, for what we cache the result\n> > > too, or always call \"git rev-parse --git-dir\".\n> > >\n> > > This blacklist would need to be in sync with the one in Git's\n> > > path.c::adjust_git_path() than.\n\nBert's suggestion seems like a decent compromise. We run `git rev-parse \n--git-path` for the paths in the blacklist, and for the rest we use the \ncached value. This does run the risk of getting out of sync with \ngit.git's list, but it might be better than spawing a process every \ntime, and is very likely better than just doing it for hooks.\n\nThoughts?\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"383221","messageId":"nycvar.QRO.7.76.6.1910011638580.46@tvgsbejvaqbjf.bet","threadId":"51931","inReplyTo":"20191001133112.3qh4aeoctkoz2wxy@yadavpratyush.com","subject":"Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-01T17:38:36Z","receivedAt":"2019-10-01T17:38:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 1 Oct 2019, Pratyush Yadav wrote:\n\n> On 30/09/19 11:42AM, Johannes Schindelin wrote:\n> > On Fri, 27 Sep 2019, Pratyush Yadav wrote:\n> > > On 27/09/19 08:10AM, Bert Wesarg wrote:\n> > > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote:\n> > > > > gitdir is used in a lot of places, and I think all those would\n> > > > > also\n> > > > > benefit from using --git-path. So I think it is a better idea to move\n> > > > > this to the procedure gitdir. It would have to be refactored to take any\n> > > > > number of arguments, instead of the two it takes here.\n> >\n> > The `gitdir` function is called 13 times during startup alone, and who\n> > knows how many more times later.\n> >\n> > So I am quite convinced that the original intention was to save on\n> > spawning processes left and right.\n> >\n> > But since you are the Git GUI maintainer, and this was your suggestion,\n> > I made it so.\n>\n> Yes, I am the maintainer, but I am not an all-knowing, all-seeing\n> entity. Your, and every other contributors, suggestions are very\n> valuable. And my suggestions aren't gospel. I would hate to see someone\n> send in a patch they weren't sure was the best thing to do just because\n> I suggested it. Please feel free to object my suggestions.\n>\n> In this case, I didn't expect gitdir to be called this many times.\n>\n> While I don't notice much of a performance difference on my system\n> (Linux), a quick measurement tells me that the time spent in gitdir is\n> about 16 ms. In contrast, the same measurement without the v2 patch\n> gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something\n> so simple. It might not be the same for everyone else. AFAIK, spawning a\n> process is much slower on Windows.\n>\n> So now I'm not so sure my suggestion was a good one. My original aim was\n> to be sure everything was correct, and no incorrect directories were\n> being used. But the current solution comes at a performance hit.\n>\n> > > > We could either maintain a blacklist, for what we cache the result\n> > > > too, or always call \"git rev-parse --git-dir\".\n> > > >\n> > > > This blacklist would need to be in sync with the one in Git's\n> > > > path.c::adjust_git_path() than.\n>\n> Bert's suggestion seems like a decent compromise. We run `git rev-parse\n> --git-path` for the paths in the blacklist, and for the rest we use the\n> cached value. This does run the risk of getting out of sync with\n> git.git's list, but it might be better than spawing a process every\n> time, and is very likely better than just doing it for hooks.\n\nBut what about this part of that function?\n\n-- snip --\nelse if (repo->different_commondir)\n\tupdate_common_dir(buf, git_dir_len, repo->commondir);\n-- snap --\n\nIt might well turn out that this blacklist is neither easy to implement\nnor will it help much.\n\nSo let's look at all the call sites:\n\n-- snip --\n$ git grep -w gitdir | sed -ne 's|\\].*||' -e 's|.*\\[gitdir ||p' | sort | uniq\n$file\n$name\nCHERRY_PICK_HEAD\nFETCH_HEAD\nGITGUI_BCK\nGITGUI_EDITMSG\nGITGUI_MSG\nHEAD\nhooks $hook_name\nindex.lock\ninfo exclude\nlogs $name\nMERGE_HEAD\nMERGE_MSG\nMERGE_RR\nobjects 4\\[0-[expr {$ndirs-1}\nobjects info\nobjects info alternates\nobjects pack\npacked-refs\nPREPARE_COMMIT_MSG\nrebase-merge head-name\nremotes\nremotes $r\nrr-cache\nrr-cache MERGE_RR\nSQUASH_MSG\n-- snap --\n\nThe `$file` call looks for messages (probably commit, merge, tag\nmessages and the likes), the `$name` one looks for refs.\n\nSome of those arguments strike me as very good candidates to require the\ncommon directory while others require the real gitdir (remember,\ncommondir != gitdir in worktrees other than the main worktree).\n\nWhat _could_ be done (but we're certainly threatening to enter the realm\nof the ridiculous here) is to call `git rev-parse --git-dir --git-path\nCHERRY_PICK_HEAD --git-path FETCH_HEAD [...]`, which will output one\npath per line, and then store the result in an associative array\n(https://tcl.tk/man/tcl8.5/tutorial/Tcl22.html), and use that to look up\npaths based on their first component, caching as we go.\n\nSomething like this:\n\n-- snipsnap --\ndiff --git a/git-gui.sh b/git-gui.sh\nindex fd476b6..9295c75 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -158,6 +158,7 @@ if {[tk windowingsystem] eq \"aqua\"} {\n\n set _appname {Git Gui}\n set _gitdir {}\n+array set _gitdir_cached {}\n set _gitworktree {}\n set _isbare {}\n set _gitexec {}\n@@ -197,12 +198,50 @@ proc appname {} {\n \treturn $_appname\n }\n\n+proc init_gitdir_cached {} {\n+\tglobal _gitdir _gitdir_cached\n+\n+\tset gitdir_keys [list \\\n+\t\tCHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \\\n+\t\tGITGUI_MSG HEAD hooks index.lock info logs MERGE_HEAD \\\n+\t\tMERGE_MSG MERGE_RR objects packed-refs PREPARE_COMMIT_MSG \\\n+\t\trebase-merge head-name remotes rr-cache SQUASH_MSG \\\n+\t\t]\n+\n+\tset gitdir_cmd [list git rev-parse --git-dir]\n+\tforeach key $gitdir_keys {\n+\t\tlappend gitdir_cmd --git-path $key\n+\t}\n+\n+\tset i -1\n+\tforeach path [split [eval $gitdir_cmd] \"\\n\"] {\n+\t\tif {$i eq -1} {\n+\t\t\tset _gitdir $path\n+\t\t} else {\n+\t\t\tset _gitdir_cached([lindex $gitdir_keys $i]) $path\n+\t\t}\n+\t\tincr i\n+\t}\n+}\n+\n proc gitdir {args} {\n-\tglobal _gitdir\n+\tglobal _gitdir _gitdir_cached\n+\n \tif {$args eq {}} {\n \t\treturn $_gitdir\n \t}\n-\treturn [eval [list file join $_gitdir] $args]\n+\n+\tset arg0 [lindex $args 0]\n+\tset args [lrange $args 1 end]\n+\tif {![info exists _gitdir_cached($arg0)]} {\n+\t\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n+\t\t\tset _gitdir_cached($arg0) [git rev-parse --git-path $arg0]\n+\t\t} else {\n+\t\t\tset _gitdir_cached($arg0) [file join $_gitdir $arg0]\n+\t\t}\n+\t}\n+\n+\treturn [eval [concat [list file join $_gitdir_cached($arg0)] $args]]\n }\n\n proc gitexec {args} {\n@@ -1242,7 +1281,7 @@ if {[catch {\n \t&& [catch {\n \t\t# beware that from the .git dir this sets _gitdir to .\n \t\t# and _prefix to the empty string\n-\t\tset _gitdir [git rev-parse --git-dir]\n+\t\tinit_gitdir_cached\n \t\tset _prefix [git rev-parse --show-prefix]\n \t} err]} {\n \tload_config 1\n"},{"id":"383436","messageId":"20191004164809.txdiwf7fandxsbmq@yadavpratyush.com","threadId":"51931","inReplyTo":"nycvar.QRO.7.76.6.1910011638580.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-10-04T16:48:09Z","receivedAt":"2019-10-04T16:48:16Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 01/10/19 07:38PM, Johannes Schindelin wrote:\n> Hi,\n> \n> On Tue, 1 Oct 2019, Pratyush Yadav wrote:\n> \n> > On 30/09/19 11:42AM, Johannes Schindelin wrote:\n> > > On Fri, 27 Sep 2019, Pratyush Yadav wrote:\n> > > > On 27/09/19 08:10AM, Bert Wesarg wrote:\n> > > > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote:\n> > > > > > gitdir is used in a lot of places, and I think all those would\n> > > > > > also\n> > > > > > benefit from using --git-path. So I think it is a better idea to move\n> > > > > > this to the procedure gitdir. It would have to be refactored to take any\n> > > > > > number of arguments, instead of the two it takes here.\n> > >\n> > > The `gitdir` function is called 13 times during startup alone, and who\n> > > knows how many more times later.\n> > >\n> > > So I am quite convinced that the original intention was to save on\n> > > spawning processes left and right.\n> > >\n> > > But since you are the Git GUI maintainer, and this was your suggestion,\n> > > I made it so.\n> >\n> > Yes, I am the maintainer, but I am not an all-knowing, all-seeing\n> > entity. Your, and every other contributors, suggestions are very\n> > valuable. And my suggestions aren't gospel. I would hate to see someone\n> > send in a patch they weren't sure was the best thing to do just because\n> > I suggested it. Please feel free to object my suggestions.\n> >\n> > In this case, I didn't expect gitdir to be called this many times.\n> >\n> > While I don't notice much of a performance difference on my system\n> > (Linux), a quick measurement tells me that the time spent in gitdir is\n> > about 16 ms. In contrast, the same measurement without the v2 patch\n> > gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something\n> > so simple. It might not be the same for everyone else. AFAIK, spawning a\n> > process is much slower on Windows.\n> >\n> > So now I'm not so sure my suggestion was a good one. My original aim was\n> > to be sure everything was correct, and no incorrect directories were\n> > being used. But the current solution comes at a performance hit.\n> >\n> > > > > We could either maintain a blacklist, for what we cache the result\n> > > > > too, or always call \"git rev-parse --git-dir\".\n> > > > >\n> > > > > This blacklist would need to be in sync with the one in Git's\n> > > > > path.c::adjust_git_path() than.\n> >\n> > Bert's suggestion seems like a decent compromise. We run `git rev-parse\n> > --git-path` for the paths in the blacklist, and for the rest we use the\n> > cached value. This does run the risk of getting out of sync with\n> > git.git's list, but it might be better than spawing a process every\n> > time, and is very likely better than just doing it for hooks.\n> \n> But what about this part of that function?\n> \n> -- snip --\n> else if (repo->different_commondir)\n> \tupdate_common_dir(buf, git_dir_len, repo->commondir);\n> -- snap --\n\nI'm afraid I'm a bit out of my depth on this. I have no idea what a \n\"common directory\" is, and how is it different from the \"git directory\". \nI can't find anything useful on Google about it. My guess is that it is \nsomething related to separate worktrees.\n \n> It might well turn out that this blacklist is neither easy to implement\n> nor will it help much.\n\nAm I correct in assuming that for other cases like \"info\", \"grafts\", \n\"index\", \"objects\", and \"hooks\" the blacklist would be simple to \nimplement, and it is the \"common directory\" case that is problematic?\n \n> So let's look at all the call sites:\n> \n> -- snip --\n> $ git grep -w gitdir | sed -ne 's|\\].*||' -e 's|.*\\[gitdir ||p' | sort | uniq\n> $file\n> $name\n> CHERRY_PICK_HEAD\n> FETCH_HEAD\n> GITGUI_BCK\n> GITGUI_EDITMSG\n> GITGUI_MSG\n> HEAD\n> hooks $hook_name\n> index.lock\n> info exclude\n> logs $name\n> MERGE_HEAD\n> MERGE_MSG\n> MERGE_RR\n> objects 4\\[0-[expr {$ndirs-1}\n> objects info\n> objects info alternates\n> objects pack\n> packed-refs\n> PREPARE_COMMIT_MSG\n> rebase-merge head-name\n> remotes\n> remotes $r\n> rr-cache\n> rr-cache MERGE_RR\n> SQUASH_MSG\n> -- snap --\n> \n> The `$file` call looks for messages (probably commit, merge, tag\n> messages and the likes), the `$name` one looks for refs.\n\nSo they should always be inside the '.git' or GIT_DIR, correct?\n \n> Some of those arguments strike me as very good candidates to require the\n> common directory while others require the real gitdir (remember,\n> commondir != gitdir in worktrees other than the main worktree).\n> \n> What _could_ be done (but we're certainly threatening to enter the realm\n> of the ridiculous here) is to call `git rev-parse --git-dir --git-path\n> CHERRY_PICK_HEAD --git-path FETCH_HEAD [...]`, which will output one\n> path per line, and then store the result in an associative array\n> (https://tcl.tk/man/tcl8.5/tutorial/Tcl22.html), and use that to look up\n> paths based on their first component, caching as we go.\n\nAh yes! That is certainly threatening to enter the realm of ridiculous. \nI'm not sure what benefit this will have. Right now, I don't think \ngit-gui handles these cases. Have people complained? Is this a common \nproblem?\n\nI want to evaluate how much benefit we get doing something like this has \nover just using your original patch that works with hooks only.\n \n> Something like this:\n> \n> -- snipsnap --\n> diff --git a/git-gui.sh b/git-gui.sh\n> index fd476b6..9295c75 100755\n> --- a/git-gui.sh\n> +++ b/git-gui.sh\n> @@ -158,6 +158,7 @@ if {[tk windowingsystem] eq \"aqua\"} {\n> \n>  set _appname {Git Gui}\n>  set _gitdir {}\n> +array set _gitdir_cached {}\n>  set _gitworktree {}\n>  set _isbare {}\n>  set _gitexec {}\n> @@ -197,12 +198,50 @@ proc appname {} {\n>  \treturn $_appname\n>  }\n> \n> +proc init_gitdir_cached {} {\n> +\tglobal _gitdir _gitdir_cached\n> +\n> +\tset gitdir_keys [list \\\n> +\t\tCHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \\\n> +\t\tGITGUI_MSG HEAD hooks index.lock info logs MERGE_HEAD \\\n> +\t\tMERGE_MSG MERGE_RR objects packed-refs PREPARE_COMMIT_MSG \\\n> +\t\trebase-merge head-name remotes rr-cache SQUASH_MSG \\\n> +\t\t]\n> +\n> +\tset gitdir_cmd [list git rev-parse --git-dir]\n> +\tforeach key $gitdir_keys {\n> +\t\tlappend gitdir_cmd --git-path $key\n> +\t}\n> +\n> +\tset i -1\n> +\tforeach path [split [eval $gitdir_cmd] \"\\n\"] {\n> +\t\tif {$i eq -1} {\n> +\t\t\tset _gitdir $path\n> +\t\t} else {\n> +\t\t\tset _gitdir_cached([lindex $gitdir_keys $i]) $path\n> +\t\t}\n> +\t\tincr i\n> +\t}\n> +}\n> +\n>  proc gitdir {args} {\n> -\tglobal _gitdir\n> +\tglobal _gitdir _gitdir_cached\n> +\n>  \tif {$args eq {}} {\n>  \t\treturn $_gitdir\n>  \t}\n> -\treturn [eval [list file join $_gitdir] $args]\n> +\n> +\tset arg0 [lindex $args 0]\n> +\tset args [lrange $args 1 end]\n> +\tif {![info exists _gitdir_cached($arg0)]} {\n> +\t\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n> +\t\t\tset _gitdir_cached($arg0) [git rev-parse --git-path $arg0]\n> +\t\t} else {\n> +\t\t\tset _gitdir_cached($arg0) [file join $_gitdir $arg0]\n> +\t\t}\n> +\t}\n> +\n> +\treturn [eval [concat [list file join $_gitdir_cached($arg0)] $args]]\n>  }\n> \n>  proc gitexec {args} {\n> @@ -1242,7 +1281,7 @@ if {[catch {\n>  \t&& [catch {\n>  \t\t# beware that from the .git dir this sets _gitdir to .\n>  \t\t# and _prefix to the empty string\n> -\t\tset _gitdir [git rev-parse --git-dir]\n> +\t\tinit_gitdir_cached\n>  \t\tset _prefix [git rev-parse --show-prefix]\n>  \t} err]} {\n>  \tload_config 1\n\nA nice way of tackling this problem overall considering the challenges, \nbut I'm worried about whether all this is _actually_ needed for real use \ncases, and what breaks if we don't.\n\nHonestly, I'm not too sure how to tackle this problem. That is also the \nreason I took so long in writing this response. What would your \nsuggestion be?\n\nAlso, if some other people interested in git-gui could chime in, it \nwould be great.\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"383441","messageId":"nycvar.QRO.7.76.6.1910042141430.46@tvgsbejvaqbjf.bet","threadId":"51931","inReplyTo":"20191004164809.txdiwf7fandxsbmq@yadavpratyush.com","subject":"Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-04T19:56:47Z","receivedAt":"2019-10-04T19:57:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Pratyush,\n\nOn Fri, 4 Oct 2019, Pratyush Yadav wrote:\n\n> On 01/10/19 07:38PM, Johannes Schindelin wrote:\n> >\n> > On Tue, 1 Oct 2019, Pratyush Yadav wrote:\n> >\n> > > On 30/09/19 11:42AM, Johannes Schindelin wrote:\n> > > > On Fri, 27 Sep 2019, Pratyush Yadav wrote:\n> > > > > On 27/09/19 08:10AM, Bert Wesarg wrote:\n> > > > > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote:\n> > > > > > > gitdir is used in a lot of places, and I think all those would\n> > > > > > > also\n> > > > > > > benefit from using --git-path. So I think it is a better idea to move\n> > > > > > > this to the procedure gitdir. It would have to be refactored to take any\n> > > > > > > number of arguments, instead of the two it takes here.\n> > > >\n> > > > The `gitdir` function is called 13 times during startup alone, and who\n> > > > knows how many more times later.\n> > > >\n> > > > So I am quite convinced that the original intention was to save on\n> > > > spawning processes left and right.\n> > > >\n> > > > But since you are the Git GUI maintainer, and this was your suggestion,\n> > > > I made it so.\n> > >\n> > > Yes, I am the maintainer, but I am not an all-knowing, all-seeing\n> > > entity. Your, and every other contributors, suggestions are very\n> > > valuable. And my suggestions aren't gospel. I would hate to see someone\n> > > send in a patch they weren't sure was the best thing to do just because\n> > > I suggested it. Please feel free to object my suggestions.\n> > >\n> > > In this case, I didn't expect gitdir to be called this many times.\n> > >\n> > > While I don't notice much of a performance difference on my system\n> > > (Linux), a quick measurement tells me that the time spent in gitdir is\n> > > about 16 ms. In contrast, the same measurement without the v2 patch\n> > > gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something\n> > > so simple. It might not be the same for everyone else. AFAIK, spawning a\n> > > process is much slower on Windows.\n> > >\n> > > So now I'm not so sure my suggestion was a good one. My original aim was\n> > > to be sure everything was correct, and no incorrect directories were\n> > > being used. But the current solution comes at a performance hit.\n> > >\n> > > > > > We could either maintain a blacklist, for what we cache the result\n> > > > > > too, or always call \"git rev-parse --git-dir\".\n> > > > > >\n> > > > > > This blacklist would need to be in sync with the one in Git's\n> > > > > > path.c::adjust_git_path() than.\n> > >\n> > > Bert's suggestion seems like a decent compromise. We run `git rev-parse\n> > > --git-path` for the paths in the blacklist, and for the rest we use the\n> > > cached value. This does run the risk of getting out of sync with\n> > > git.git's list, but it might be better than spawing a process every\n> > > time, and is very likely better than just doing it for hooks.\n> >\n> > But what about this part of that function?\n> >\n> > -- snip --\n> > else if (repo->different_commondir)\n> > \tupdate_common_dir(buf, git_dir_len, repo->commondir);\n> > -- snap --\n>\n> I'm afraid I'm a bit out of my depth on this. I have no idea what a\n> \"common directory\" is, and how is it different from the \"git directory\".\n> I can't find anything useful on Google about it. My guess is that it is\n> something related to separate worktrees.\n\nIt is indeed related to worktrees. If you create a secondary worktree\nvia `git worktree add [...]`, that work tree will get its own git\ndirectory under `.git/worktrees/<name>` in the main worktree. That git\ndirectory will not, however, contain all contents of a regular git\ndirectory. Most refs, for example, are stored in the main worktree's git\ndirectory. That is what the \"common dir\" is.\n\n> > It might well turn out that this blacklist is neither easy to implement\n> > nor will it help much.\n>\n> Am I correct in assuming that for other cases like \"info\", \"grafts\",\n> \"index\", \"objects\", and \"hooks\" the blacklist would be simple to\n> implement, and it is the \"common directory\" case that is problematic?\n\nIndeed, for the other, simple cases, the list would be unproblematic to\nimplement. Problematic to maintain, though, especially given that Git\nGUI is _supposed_ to support even very old Git versions.\n\nAnd those simple cases don't include _all_ interesting cases. Take\n`logs/` for example. The git directory will contain the reflogs for\n`HEAD`, but unless you're on an unnamed branch (AKA \"detached HEAD\"),\nthe reflogs for the current branch are _in the commondir_.\n\n> > So let's look at all the call sites:\n> >\n> > -- snip --\n> > $ git grep -w gitdir | sed -ne 's|\\].*||' -e 's|.*\\[gitdir ||p' | sort | uniq\n> > $file\n> > $name\n> > CHERRY_PICK_HEAD\n> > FETCH_HEAD\n> > GITGUI_BCK\n> > GITGUI_EDITMSG\n> > GITGUI_MSG\n> > HEAD\n> > hooks $hook_name\n> > index.lock\n> > info exclude\n> > logs $name\n> > MERGE_HEAD\n> > MERGE_MSG\n> > MERGE_RR\n> > objects 4\\[0-[expr {$ndirs-1}\n> > objects info\n> > objects info alternates\n> > objects pack\n> > packed-refs\n> > PREPARE_COMMIT_MSG\n> > rebase-merge head-name\n> > remotes\n> > remotes $r\n> > rr-cache\n> > rr-cache MERGE_RR\n> > SQUASH_MSG\n> > -- snap --\n> >\n> > The `$file` call looks for messages (probably commit, merge, tag\n> > messages and the likes), the `$name` one looks for refs.\n>\n> So they should always be inside the '.git' or GIT_DIR, correct?\n\nThey should be inside the git directory. Note that `.git` in worktrees\nis just a file that contains `gitdir: <path>`. The indicated path is the\nactual git directory. Inside that git directory, the file `commondir`\ncontains the path to the main worktree's git directory.\n\n> > Some of those arguments strike me as very good candidates to require the\n> > common directory while others require the real gitdir (remember,\n> > commondir != gitdir in worktrees other than the main worktree).\n> >\n> > What _could_ be done (but we're certainly threatening to enter the realm\n> > of the ridiculous here) is to call `git rev-parse --git-dir --git-path\n> > CHERRY_PICK_HEAD --git-path FETCH_HEAD [...]`, which will output one\n> > path per line, and then store the result in an associative array\n> > (https://tcl.tk/man/tcl8.5/tutorial/Tcl22.html), and use that to look up\n> > paths based on their first component, caching as we go.\n>\n> Ah yes! That is certainly threatening to enter the realm of ridiculous.\n> I'm not sure what benefit this will have. Right now, I don't think\n> git-gui handles these cases. Have people complained? Is this a common\n> problem?\n\nWell, we know that people complained about the hooks directory. And that\ndid not even involve worktrees.\n\nI see that e.g. `packed-refs` is queried by Git GUI. And that file lives\nin the main worktree's git directory, i.e. in the commondir.\n\nSo either users don't use Git GUI in secondary worktrees, or they did\nnot even notice the bug.\n\n> I want to evaluate how much benefit we get doing something like this\n> has over just using your original patch that works with hooks only.\n\nSince I already have that ridiculous approach essentially implemented,\nand since it fixes very real bugs in Git GUI ever since `git worktree`\nwas introduced, I'd say that you'd be better off taking the ridiculous\npatch than not.\n\n> > Something like this:\n> >\n> > -- snipsnap --\n> > diff --git a/git-gui.sh b/git-gui.sh\n> > index fd476b6..9295c75 100755\n> > --- a/git-gui.sh\n> > +++ b/git-gui.sh\n> > @@ -158,6 +158,7 @@ if {[tk windowingsystem] eq \"aqua\"} {\n> >\n> >  set _appname {Git Gui}\n> >  set _gitdir {}\n> > +array set _gitdir_cached {}\n> >  set _gitworktree {}\n> >  set _isbare {}\n> >  set _gitexec {}\n> > @@ -197,12 +198,50 @@ proc appname {} {\n> >  \treturn $_appname\n> >  }\n> >\n> > +proc init_gitdir_cached {} {\n> > +\tglobal _gitdir _gitdir_cached\n> > +\n> > +\tset gitdir_keys [list \\\n> > +\t\tCHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \\\n> > +\t\tGITGUI_MSG HEAD hooks index.lock info logs MERGE_HEAD \\\n> > +\t\tMERGE_MSG MERGE_RR objects packed-refs PREPARE_COMMIT_MSG \\\n> > +\t\trebase-merge head-name remotes rr-cache SQUASH_MSG \\\n> > +\t\t]\n> > +\n> > +\tset gitdir_cmd [list git rev-parse --git-dir]\n> > +\tforeach key $gitdir_keys {\n> > +\t\tlappend gitdir_cmd --git-path $key\n> > +\t}\n> > +\n> > +\tset i -1\n> > +\tforeach path [split [eval $gitdir_cmd] \"\\n\"] {\n> > +\t\tif {$i eq -1} {\n> > +\t\t\tset _gitdir $path\n> > +\t\t} else {\n> > +\t\t\tset _gitdir_cached([lindex $gitdir_keys $i]) $path\n> > +\t\t}\n> > +\t\tincr i\n> > +\t}\n> > +}\n> > +\n> >  proc gitdir {args} {\n> > -\tglobal _gitdir\n> > +\tglobal _gitdir _gitdir_cached\n> > +\n> >  \tif {$args eq {}} {\n> >  \t\treturn $_gitdir\n> >  \t}\n> > -\treturn [eval [list file join $_gitdir] $args]\n> > +\n> > +\tset arg0 [lindex $args 0]\n> > +\tset args [lrange $args 1 end]\n> > +\tif {![info exists _gitdir_cached($arg0)]} {\n> > +\t\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n> > +\t\t\tset _gitdir_cached($arg0) [git rev-parse --git-path $arg0]\n> > +\t\t} else {\n> > +\t\t\tset _gitdir_cached($arg0) [file join $_gitdir $arg0]\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn [eval [concat [list file join $_gitdir_cached($arg0)] $args]]\n> >  }\n> >\n> >  proc gitexec {args} {\n> > @@ -1242,7 +1281,7 @@ if {[catch {\n> >  \t&& [catch {\n> >  \t\t# beware that from the .git dir this sets _gitdir to .\n> >  \t\t# and _prefix to the empty string\n> > -\t\tset _gitdir [git rev-parse --git-dir]\n> > +\t\tinit_gitdir_cached\n> >  \t\tset _prefix [git rev-parse --show-prefix]\n> >  \t} err]} {\n> >  \tload_config 1\n>\n> A nice way of tackling this problem overall considering the challenges,\n> but I'm worried about whether all this is _actually_ needed for real use\n> cases, and what breaks if we don't.\n\nWhy don't you try using Git GUI in a worktree for a while? I am sure\nyou will encounter the issues sooner or later.\n\n> Honestly, I'm not too sure how to tackle this problem. That is also the\n> reason I took so long in writing this response. What would your\n> suggestion be?\n\nI would actually go for the ridiculous patch, as it provides the safest\nbet we have on fixing the `gitdir`-related bugs.\n\n> Also, if some other people interested in git-gui could chime in, it\n> would be great.\n\nSure.\n\nCiao,\nJohannes\n"},{"id":"383444","messageId":"65c2fa33e1aeec03930921ee0ef562d3f9dc5ccf.1570225311.git.gitgitgadget@gmail.com","threadId":"51931","inReplyTo":"pull.361.v3.git.gitgitgadget@gmail.com","subject":"[PATCH v3 1/1] Fix gitdir e.g. to respect core.hooksPath","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-04T21:41:52Z","receivedAt":"2019-10-04T21:41:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n\nSince v2.9.0, Git knows about the config variable core.hookspath\nthat allows overriding the path to the directory containing the\nGit hooks.\n\nSince v2.10.0, the `--git-path` option respects that config\nvariable, too, so we may just as well use that command.\n\nFor Git versions older than v2.5.0 (which was the first version to\nsupport the `--git-path` option for the `rev-parse` command), we\nsimply fall back to the previous code.\n\nAn original patch handled only the hooksPath setting, however, during\nthe code submission it was deemed better to fix all call to the `gitdir`\nfunction.\n\nTo avoid spawning a gazillion `git rev-parse --git-path` instances, we\ncache the returned paths, priming the cache upon startup in a single\n`git rev-parse invocation` with the known entries.\n\nThis fixes https://github.com/git-for-windows/git/issues/1755\n\nInitial-patch-by: Philipp Gortan <philipp@gortan.org>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n git-gui.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 49 insertions(+), 3 deletions(-)\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex fd476b6999..8b72e59cd0 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -158,6 +158,7 @@ if {[tk windowingsystem] eq \"aqua\"} {\n \n set _appname {Git Gui}\n set _gitdir {}\n+array set _gitdir_cache {}\n set _gitworktree {}\n set _isbare {}\n set _gitexec {}\n@@ -197,12 +198,57 @@ proc appname {} {\n \treturn $_appname\n }\n \n+proc prime_gitdir_cache {} {\n+\tglobal _gitdir _gitdir_cache\n+\n+\tset gitdir_cmd [list git rev-parse --git-dir]\n+\n+\t# `--git-path` is only supported since Git v2.5.0\n+\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n+\t\tset gitdir_keys [list \\\n+\t\t\tCHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \\\n+\t\t\tGITGUI_MSG HEAD hooks hooks/prepare-commit-msg \\\n+\t\t\tindex.lock info info/exclude logs MERGE_HEAD MERGE_MSG \\\n+\t\t\tMERGE_RR objects \"objects/4\\[0-1\\]/*\" \\\n+\t\t\t\"objects/4\\[0-3\\]/*\" objects/info \\\n+\t\t\tobjects/info/alternates objects/pack packed-refs \\\n+\t\t\tPREPARE_COMMIT_MSG rebase-merge/head-name remotes \\\n+\t\t\trr-cache rr-cache/MERGE_RR SQUASH_MSG \\\n+\t\t]\n+\n+\t\tforeach key $gitdir_keys {\n+\t\t\tlappend gitdir_cmd --git-path $key\n+\t\t}\n+\t}\n+\n+\tset i -1\n+\tforeach path [split [eval $gitdir_cmd] \"\\n\"] {\n+\t\tif {$i eq -1} {\n+\t\t\tset _gitdir $path\n+\t\t} else {\n+\t\t\tset _gitdir_cache([lindex $gitdir_keys $i]) $path\n+\t\t}\n+\t\tincr i\n+\t}\n+}\n+\n proc gitdir {args} {\n-\tglobal _gitdir\n+\tglobal _gitdir _gitdir_cache\n+\n \tif {$args eq {}} {\n \t\treturn $_gitdir\n \t}\n-\treturn [eval [list file join $_gitdir] $args]\n+\n+\tset args [eval [list file join] $args]\n+\tif {![info exists _gitdir_cache($args)]} {\n+\t\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n+\t\t\tset _gitdir_cache($args) [git rev-parse --git-path $args]\n+\t\t} else {\n+\t\t\tset _gitdir_cache($args) [file join $_gitdir $args]\n+\t\t}\n+\t}\n+\n+\treturn $_gitdir_cache($args)\n }\n \n proc gitexec {args} {\n@@ -1242,7 +1288,7 @@ if {[catch {\n \t&& [catch {\n \t\t# beware that from the .git dir this sets _gitdir to .\n \t\t# and _prefix to the empty string\n-\t\tset _gitdir [git rev-parse --git-dir]\n+\t\tprime_gitdir_cache\n \t\tset _prefix [git rev-parse --show-prefix]\n \t} err]} {\n \tload_config 1\n-- \ngitgitgadget\n"},{"id":"383445","messageId":"pull.361.v3.git.gitgitgadget@gmail.com","threadId":"51931","inReplyTo":"pull.361.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v3 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-04T21:41:51Z","receivedAt":"2019-10-04T21:41:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is yet another patch from Git for Windows.\n\nChanges since v2:\n\n * The paths returned by git rev-parse --git-path are now cached, and the\n   cache is primed with the most common paths.\n\nChanges since v1:\n\n * Rather than a fine-grained override of gitdir just for the hooks path, we\n   now spawn git rev-parse --git-path [...] every time gitdir is called with\n   arguments. This makes the code safer, although at the cost of potentially\n   many spawned processes.\n\nJohannes Schindelin (1):\n  Fix gitdir e.g. to respect core.hooksPath\n\n git-gui.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 49 insertions(+), 3 deletions(-)\n\n\nbase-commit: 60c60b627e81bf84e1cb01729d2ae882178f079d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-361%2Fdscho%2Fgit-gui-hooks-path-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-361/dscho/git-gui-hooks-path-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/361\n\nRange-diff vs v2:\n\n 1:  c101422936 < -:  ---------- respect core.hooksPath, falling back to .git/hooks\n -:  ---------- > 1:  65c2fa33e1 Fix gitdir e.g. to respect core.hooksPath\n\n-- \ngitgitgadget\n"},{"id":"383639","messageId":"20191008002902.n4b3bhlvwzsxodsc@yadavpratyush.com","threadId":"51931","inReplyTo":"65c2fa33e1aeec03930921ee0ef562d3f9dc5ccf.1570225311.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/1] Fix gitdir e.g. to respect core.hooksPath","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-10-08T00:29:02Z","receivedAt":"2019-10-08T00:29:09Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"Hi Johannes,\n\nCould you please change the commit subject to more clearly state that we \nare caching all paths. This is not something just related to hooks any \nmore.\n\nOn 04/10/19 02:41PM, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> \n> Since v2.9.0, Git knows about the config variable core.hookspath\n> that allows overriding the path to the directory containing the\n> Git hooks.\n> \n> Since v2.10.0, the `--git-path` option respects that config\n> variable, too, so we may just as well use that command.\n> \n> For Git versions older than v2.5.0 (which was the first version to\n> support the `--git-path` option for the `rev-parse` command), we\n> simply fall back to the previous code.\n> \n> An original patch handled only the hooksPath setting, however, during\n> the code submission it was deemed better to fix all call to the `gitdir`\n> function.\n> \n> To avoid spawning a gazillion `git rev-parse --git-path` instances, we\n> cache the returned paths, priming the cache upon startup in a single\n> `git rev-parse invocation` with the known entries.\n\nI think it would also be a good idea to mention that we are fixing \nworktree paths not being correct.\n \n> This fixes https://github.com/git-for-windows/git/issues/1755\n> \n> Initial-patch-by: Philipp Gortan <philipp@gortan.org>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  git-gui.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---\n>  1 file changed, 49 insertions(+), 3 deletions(-)\n> \n> diff --git a/git-gui.sh b/git-gui.sh\n> index fd476b6999..8b72e59cd0 100755\n> --- a/git-gui.sh\n> +++ b/git-gui.sh\n> @@ -158,6 +158,7 @@ if {[tk windowingsystem] eq \"aqua\"} {\n>  \n>  set _appname {Git Gui}\n>  set _gitdir {}\n> +array set _gitdir_cache {}\n>  set _gitworktree {}\n>  set _isbare {}\n>  set _gitexec {}\n> @@ -197,12 +198,57 @@ proc appname {} {\n>  \treturn $_appname\n>  }\n>  \n> +proc prime_gitdir_cache {} {\n> +\tglobal _gitdir _gitdir_cache\n> +\n> +\tset gitdir_cmd [list git rev-parse --git-dir]\n> +\n> +\t# `--git-path` is only supported since Git v2.5.0\n> +\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n\nI think we should mention the source of the list of \"magic\" keys. A \ncomment mentioning this list came from looking at the common calls to \n`gitdir` in the rest of the git-gui code would explain this function to \na future reader better.\n\n> +\t\tset gitdir_keys [list \\\n> +\t\t\tCHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \\\n> +\t\t\tGITGUI_MSG HEAD hooks hooks/prepare-commit-msg \\\n> +\t\t\tindex.lock info info/exclude logs MERGE_HEAD MERGE_MSG \\\n> +\t\t\tMERGE_RR objects \"objects/4\\[0-1\\]/*\" \\\n> +\t\t\t\"objects/4\\[0-3\\]/*\" objects/info \\\n> +\t\t\tobjects/info/alternates objects/pack packed-refs \\\n> +\t\t\tPREPARE_COMMIT_MSG rebase-merge/head-name remotes \\\n> +\t\t\trr-cache rr-cache/MERGE_RR SQUASH_MSG \\\n> +\t\t]\n> +\n> +\t\tforeach key $gitdir_keys {\n> +\t\t\tlappend gitdir_cmd --git-path $key\n> +\t\t}\n> +\t}\n> +\n> +\tset i -1\n> +\tforeach path [split [eval $gitdir_cmd] \"\\n\"] {\n\nA call to the procedure 'git' is wrapped in a 'catch' in a lot of \nplaces. But it is also not wrapped in 'catch' in a lot of other places.\n\nI'm not sure how something like this would fail, so I'm not sure if \nwrapping this call in a catch is a good idea. But it is something to \nconsider.\n\n> +\t\tif {$i eq -1} {\n> +\t\t\tset _gitdir $path\n> +\t\t} else {\n> +\t\t\tset _gitdir_cache([lindex $gitdir_keys $i]) $path\n> +\t\t}\n> +\t\tincr i\n> +\t}\n> +}\n> +\n>  proc gitdir {args} {\n> -\tglobal _gitdir\n> +\tglobal _gitdir _gitdir_cache\n> +\n>  \tif {$args eq {}} {\n>  \t\treturn $_gitdir\n>  \t}\n> -\treturn [eval [list file join $_gitdir] $args]\n> +\n> +\tset args [eval [list file join] $args]\n> +\tif {![info exists _gitdir_cache($args)]} {\n> +\t\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n> +\t\t\tset _gitdir_cache($args) [git rev-parse --git-path $args]\n> +\t\t} else {\n> +\t\t\tset _gitdir_cache($args) [file join $_gitdir $args]\n> +\t\t}\n> +\t}\n> +\n> +\treturn $_gitdir_cache($args)\n>  }\n>  \n>  proc gitexec {args} {\n> @@ -1242,7 +1288,7 @@ if {[catch {\n>  \t&& [catch {\n>  \t\t# beware that from the .git dir this sets _gitdir to .\n>  \t\t# and _prefix to the empty string\n> -\t\tset _gitdir [git rev-parse --git-dir]\n> +\t\tprime_gitdir_cache\n>  \t\tset _prefix [git rev-parse --show-prefix]\n>  \t} err]} {\n>  \tload_config 1\n\nCan these paths we cache change while git-gui is running, say by a \ncommand run by the user in the terminal? In that case, we should refresh \nthe list when the user rescans.\n\nOther than some minor comments, looks good. Thanks.\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"383669","messageId":"nycvar.QRO.7.76.6.1910081303360.46@tvgsbejvaqbjf.bet","threadId":"51931","inReplyTo":"20191008002902.n4b3bhlvwzsxodsc@yadavpratyush.com","subject":"Re: [PATCH v3 1/1] Fix gitdir e.g. to respect core.hooksPath","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-08T11:30:42Z","receivedAt":"2019-10-08T11:30:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Pratyush,\n\nOn Tue, 8 Oct 2019, Pratyush Yadav wrote:\n\n> Could you please change the commit subject to more clearly state that we\n> are caching all paths. This is not something just related to hooks any\n> more.\n\nIt is related, but not exclusively so. Changed accordingly.\n\n> On 04/10/19 02:41PM, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> >\n> > Since v2.9.0, Git knows about the config variable core.hookspath\n> > that allows overriding the path to the directory containing the\n> > Git hooks.\n> >\n> > Since v2.10.0, the `--git-path` option respects that config\n> > variable, too, so we may just as well use that command.\n> >\n> > For Git versions older than v2.5.0 (which was the first version to\n> > support the `--git-path` option for the `rev-parse` command), we\n> > simply fall back to the previous code.\n> >\n> > An original patch handled only the hooksPath setting, however, during\n> > the code submission it was deemed better to fix all call to the `gitdir`\n> > function.\n> >\n> > To avoid spawning a gazillion `git rev-parse --git-path` instances, we\n> > cache the returned paths, priming the cache upon startup in a single\n> > `git rev-parse invocation` with the known entries.\n>\n> I think it would also be a good idea to mention that we are fixing\n> worktree paths not being correct.\n\nDone.\n\n> > This fixes https://github.com/git-for-windows/git/issues/1755\n> >\n> > Initial-patch-by: Philipp Gortan <philipp@gortan.org>\n> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > ---\n> >  git-gui.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---\n> >  1 file changed, 49 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/git-gui.sh b/git-gui.sh\n> > index fd476b6999..8b72e59cd0 100755\n> > --- a/git-gui.sh\n> > +++ b/git-gui.sh\n> > @@ -158,6 +158,7 @@ if {[tk windowingsystem] eq \"aqua\"} {\n> >\n> >  set _appname {Git Gui}\n> >  set _gitdir {}\n> > +array set _gitdir_cache {}\n> >  set _gitworktree {}\n> >  set _isbare {}\n> >  set _gitexec {}\n> > @@ -197,12 +198,57 @@ proc appname {} {\n> >  \treturn $_appname\n> >  }\n> >\n> > +proc prime_gitdir_cache {} {\n> > +\tglobal _gitdir _gitdir_cache\n> > +\n> > +\tset gitdir_cmd [list git rev-parse --git-dir]\n> > +\n> > +\t# `--git-path` is only supported since Git v2.5.0\n> > +\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n>\n> I think we should mention the source of the list of \"magic\" keys. A\n> comment mentioning this list came from looking at the common calls to\n> `gitdir` in the rest of the git-gui code would explain this function to\n> a future reader better.\n\nMakes sense.\n\n> > +\t\tset gitdir_keys [list \\\n> > +\t\t\tCHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \\\n> > +\t\t\tGITGUI_MSG HEAD hooks hooks/prepare-commit-msg \\\n> > +\t\t\tindex.lock info info/exclude logs MERGE_HEAD MERGE_MSG \\\n> > +\t\t\tMERGE_RR objects \"objects/4\\[0-1\\]/*\" \\\n> > +\t\t\t\"objects/4\\[0-3\\]/*\" objects/info \\\n> > +\t\t\tobjects/info/alternates objects/pack packed-refs \\\n> > +\t\t\tPREPARE_COMMIT_MSG rebase-merge/head-name remotes \\\n> > +\t\t\trr-cache rr-cache/MERGE_RR SQUASH_MSG \\\n> > +\t\t]\n> > +\n> > +\t\tforeach key $gitdir_keys {\n> > +\t\t\tlappend gitdir_cmd --git-path $key\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tset i -1\n> > +\tforeach path [split [eval $gitdir_cmd] \"\\n\"] {\n>\n> A call to the procedure 'git' is wrapped in a 'catch' in a lot of\n> places. But it is also not wrapped in 'catch' in a lot of other places.\n>\n> I'm not sure how something like this would fail, so I'm not sure if\n> wrapping this call in a catch is a good idea. But it is something to\n> consider.\n\nIf that call fails, we lack a `gitdir`, which is a rather fundamental\nprerequisite to running Git GUI. I'd rather show an (ugly, but also\nhighly unexpected) exception.\n\nIn other words, I think the patch should stay \"catch-less\".\n\n> > +\t\tif {$i eq -1} {\n> > +\t\t\tset _gitdir $path\n> > +\t\t} else {\n> > +\t\t\tset _gitdir_cache([lindex $gitdir_keys $i]) $path\n> > +\t\t}\n> > +\t\tincr i\n> > +\t}\n> > +}\n> > +\n> >  proc gitdir {args} {\n> > -\tglobal _gitdir\n> > +\tglobal _gitdir _gitdir_cache\n> > +\n> >  \tif {$args eq {}} {\n> >  \t\treturn $_gitdir\n> >  \t}\n> > -\treturn [eval [list file join $_gitdir] $args]\n> > +\n> > +\tset args [eval [list file join] $args]\n> > +\tif {![info exists _gitdir_cache($args)]} {\n> > +\t\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n> > +\t\t\tset _gitdir_cache($args) [git rev-parse --git-path $args]\n> > +\t\t} else {\n> > +\t\t\tset _gitdir_cache($args) [file join $_gitdir $args]\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn $_gitdir_cache($args)\n> >  }\n> >\n> >  proc gitexec {args} {\n> > @@ -1242,7 +1288,7 @@ if {[catch {\n> >  \t&& [catch {\n> >  \t\t# beware that from the .git dir this sets _gitdir to .\n> >  \t\t# and _prefix to the empty string\n> > -\t\tset _gitdir [git rev-parse --git-dir]\n> > +\t\tprime_gitdir_cache\n> >  \t\tset _prefix [git rev-parse --show-prefix]\n> >  \t} err]} {\n> >  \tload_config 1\n>\n> Can these paths we cache change while git-gui is running, say by a\n> command run by the user in the terminal? In that case, we should refresh\n> the list when the user rescans.\n\nI guess it can. A user could configure `core.hooksPath` while Git GUI is\nopen, after all. Or rewrite the `.git` file of a worktree.\n\nI won't touch the `.git` file part, but I changed the `rescan` code to\nre-prime the gitdir cache.\n\nThanks for the review,\nJohannes\n\n>\n> Other than some minor comments, looks good. Thanks.\n>\n> --\n> Regards,\n> Pratyush Yadav\n>\n"},{"id":"383670","messageId":"pull.361.v4.git.gitgitgadget@gmail.com","threadId":"51931","inReplyTo":"pull.361.v3.git.gitgitgadget@gmail.com","subject":"[PATCH v4 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-08T11:33:25Z","receivedAt":"2019-10-08T11:33:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is yet another patch from Git for Windows.\n\nChanges since v3:\n\n * Adjusted the commit message to reflect that this is no longer only about\n   the hooks directory.\n * Added a code comment to indicate how the list of keys was determined that\n   are used for the gitdir priming.\n * The gitdir cache is now re-primed upon F5.\n\nChanges since v2:\n\n * The paths returned by git rev-parse --git-path are now cached, and the\n   cache is primed with the most common paths.\n\nChanges since v1:\n\n * Rather than a fine-grained override of gitdir just for the hooks path, we\n   now spawn git rev-parse --git-path [...] every time gitdir is called with\n   arguments. This makes the code safer, although at the cost of potentially\n   many spawned processes.\n\nJohannes Schindelin (1):\n  Make gitdir work with worktrees, respect core.hooksPath, etc\n\n git-gui.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 58 insertions(+), 4 deletions(-)\n\n\nbase-commit: 60c60b627e81bf84e1cb01729d2ae882178f079d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-361%2Fdscho%2Fgit-gui-hooks-path-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-361/dscho/git-gui-hooks-path-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/361\n\nRange-diff vs v3:\n\n 1:  65c2fa33e1 ! 1:  2f55d6fb2a Fix gitdir e.g. to respect core.hooksPath\n     @@ -1,17 +1,21 @@\n      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n      \n     -    Fix gitdir e.g. to respect core.hooksPath\n     +    Make gitdir work with worktrees, respect core.hooksPath, etc\n      \n     -    Since v2.9.0, Git knows about the config variable core.hookspath\n     -    that allows overriding the path to the directory containing the\n     -    Git hooks.\n     +    Since v2.9.0, Git knows about the config variable core.hookspath that\n     +    allows overriding the path to the directory containing the Git hooks.\n      \n     -    Since v2.10.0, the `--git-path` option respects that config\n     -    variable, too, so we may just as well use that command.\n     +    Since v2.10.0, the `--git-path` option respects that config variable,\n     +    too, so we may just as well use that command.\n     +\n     +    Other paths inside `.git` are equally subject to differ from\n     +    `<gitdir>/<path>`, i.e. inside worktrees, where _some_ paths live in the\n     +    \"gitdir\" and some live in the \"commondir\" (i.e. the \"gitdir\" of the main\n     +    worktree).\n      \n          For Git versions older than v2.5.0 (which was the first version to\n     -    support the `--git-path` option for the `rev-parse` command), we\n     -    simply fall back to the previous code.\n     +    support the `--git-path` option for the `rev-parse` command), we simply\n     +    fall back to the previous code.\n      \n          An original patch handled only the hooksPath setting, however, during\n          the code submission it was deemed better to fix all call to the `gitdir`\n     @@ -19,7 +23,9 @@\n      \n          To avoid spawning a gazillion `git rev-parse --git-path` instances, we\n          cache the returned paths, priming the cache upon startup in a single\n     -    `git rev-parse invocation` with the known entries.\n     +    `git rev-parse invocation` with some paths (that have been\n     +    determined via a typical startup and via grepping the source code for\n     +    calls to the `gitdir` function).\n      \n          This fixes https://github.com/git-for-windows/git/issues/1755\n      \n     @@ -48,6 +54,8 @@\n      +\n      +\t# `--git-path` is only supported since Git v2.5.0\n      +\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n     ++\t\t# This list was generated from a typical startup as well as from\n     ++\t\t# grepping through Git GUI's source code.\n      +\t\tset gitdir_keys [list \\\n      +\t\t\tCHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \\\n      +\t\t\tGITGUI_MSG HEAD hooks hooks/prepare-commit-msg \\\n     @@ -106,3 +114,21 @@\n       \t\tset _prefix [git rev-parse --show-prefix]\n       \t} err]} {\n       \tload_config 1\n     +@@\n     + \tglobal HEAD PARENT MERGE_HEAD commit_type\n     + \tglobal ui_index ui_workdir ui_comm\n     + \tglobal rescan_active file_states\n     +-\tglobal repo_config\n     ++\tglobal repo_config _gitdir_cache\n     + \n     + \tif {$rescan_active > 0 || ![lock_index read]} return\n     + \n     ++\t# Only re-prime gitdir cache on a full rescan\n     ++\tif {$after ne \"ui_ready\"} {\n     ++\t\tarray unset _gitdir_cache\n     ++\t\tprime_gitdir_cache\n     ++\t}\n     ++\n     + \trepository_state newType newHEAD newMERGE_HEAD\n     + \tif {[string match amend* $commit_type]\n     + \t\t&& $newType eq {normal}\n\n-- \ngitgitgadget\n"},{"id":"383671","messageId":"2f55d6fb2a158c5b26b93ddb9c144ce1af5d9c32.1570534405.git.gitgitgadget@gmail.com","threadId":"51931","inReplyTo":"pull.361.v4.git.gitgitgadget@gmail.com","subject":"[PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-08T11:33:26Z","receivedAt":"2019-10-08T11:33:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n\nSince v2.9.0, Git knows about the config variable core.hookspath that\nallows overriding the path to the directory containing the Git hooks.\n\nSince v2.10.0, the `--git-path` option respects that config variable,\ntoo, so we may just as well use that command.\n\nOther paths inside `.git` are equally subject to differ from\n`<gitdir>/<path>`, i.e. inside worktrees, where _some_ paths live in the\n\"gitdir\" and some live in the \"commondir\" (i.e. the \"gitdir\" of the main\nworktree).\n\nFor Git versions older than v2.5.0 (which was the first version to\nsupport the `--git-path` option for the `rev-parse` command), we simply\nfall back to the previous code.\n\nAn original patch handled only the hooksPath setting, however, during\nthe code submission it was deemed better to fix all call to the `gitdir`\nfunction.\n\nTo avoid spawning a gazillion `git rev-parse --git-path` instances, we\ncache the returned paths, priming the cache upon startup in a single\n`git rev-parse invocation` with some paths (that have been\ndetermined via a typical startup and via grepping the source code for\ncalls to the `gitdir` function).\n\nThis fixes https://github.com/git-for-windows/git/issues/1755\n\nInitial-patch-by: Philipp Gortan <philipp@gortan.org>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n git-gui.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 58 insertions(+), 4 deletions(-)\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex fd476b6999..c684dc7ae1 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -158,6 +158,7 @@ if {[tk windowingsystem] eq \"aqua\"} {\n \n set _appname {Git Gui}\n set _gitdir {}\n+array set _gitdir_cache {}\n set _gitworktree {}\n set _isbare {}\n set _gitexec {}\n@@ -197,12 +198,59 @@ proc appname {} {\n \treturn $_appname\n }\n \n+proc prime_gitdir_cache {} {\n+\tglobal _gitdir _gitdir_cache\n+\n+\tset gitdir_cmd [list git rev-parse --git-dir]\n+\n+\t# `--git-path` is only supported since Git v2.5.0\n+\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n+\t\t# This list was generated from a typical startup as well as from\n+\t\t# grepping through Git GUI's source code.\n+\t\tset gitdir_keys [list \\\n+\t\t\tCHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \\\n+\t\t\tGITGUI_MSG HEAD hooks hooks/prepare-commit-msg \\\n+\t\t\tindex.lock info info/exclude logs MERGE_HEAD MERGE_MSG \\\n+\t\t\tMERGE_RR objects \"objects/4\\[0-1\\]/*\" \\\n+\t\t\t\"objects/4\\[0-3\\]/*\" objects/info \\\n+\t\t\tobjects/info/alternates objects/pack packed-refs \\\n+\t\t\tPREPARE_COMMIT_MSG rebase-merge/head-name remotes \\\n+\t\t\trr-cache rr-cache/MERGE_RR SQUASH_MSG \\\n+\t\t]\n+\n+\t\tforeach key $gitdir_keys {\n+\t\t\tlappend gitdir_cmd --git-path $key\n+\t\t}\n+\t}\n+\n+\tset i -1\n+\tforeach path [split [eval $gitdir_cmd] \"\\n\"] {\n+\t\tif {$i eq -1} {\n+\t\t\tset _gitdir $path\n+\t\t} else {\n+\t\t\tset _gitdir_cache([lindex $gitdir_keys $i]) $path\n+\t\t}\n+\t\tincr i\n+\t}\n+}\n+\n proc gitdir {args} {\n-\tglobal _gitdir\n+\tglobal _gitdir _gitdir_cache\n+\n \tif {$args eq {}} {\n \t\treturn $_gitdir\n \t}\n-\treturn [eval [list file join $_gitdir] $args]\n+\n+\tset args [eval [list file join] $args]\n+\tif {![info exists _gitdir_cache($args)]} {\n+\t\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n+\t\t\tset _gitdir_cache($args) [git rev-parse --git-path $args]\n+\t\t} else {\n+\t\t\tset _gitdir_cache($args) [file join $_gitdir $args]\n+\t\t}\n+\t}\n+\n+\treturn $_gitdir_cache($args)\n }\n \n proc gitexec {args} {\n@@ -1242,7 +1290,7 @@ if {[catch {\n \t&& [catch {\n \t\t# beware that from the .git dir this sets _gitdir to .\n \t\t# and _prefix to the empty string\n-\t\tset _gitdir [git rev-parse --git-dir]\n+\t\tprime_gitdir_cache\n \t\tset _prefix [git rev-parse --show-prefix]\n \t} err]} {\n \tload_config 1\n@@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {\n \tglobal HEAD PARENT MERGE_HEAD commit_type\n \tglobal ui_index ui_workdir ui_comm\n \tglobal rescan_active file_states\n-\tglobal repo_config\n+\tglobal repo_config _gitdir_cache\n \n \tif {$rescan_active > 0 || ![lock_index read]} return\n \n+\t# Only re-prime gitdir cache on a full rescan\n+\tif {$after ne \"ui_ready\"} {\n+\t\tarray unset _gitdir_cache\n+\t\tprime_gitdir_cache\n+\t}\n+\n \trepository_state newType newHEAD newMERGE_HEAD\n \tif {[string match amend* $commit_type]\n \t\t&& $newType eq {normal}\n-- \ngitgitgadget\n"},{"id":"383944","messageId":"20191011222611.le5lyf6mr5lmvbbd@yadavpratyush.com","threadId":"51931","inReplyTo":"2f55d6fb2a158c5b26b93ddb9c144ce1af5d9c32.1570534405.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-10-11T22:26:12Z","receivedAt":"2019-10-11T22:26:19Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"Hi Johannes,\n\nThanks for the re-roll. Some comments below...\n\nOn 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> \n> Since v2.9.0, Git knows about the config variable core.hookspath that\n> allows overriding the path to the directory containing the Git hooks.\n> \n> Since v2.10.0, the `--git-path` option respects that config variable,\n> too, so we may just as well use that command.\n> \n> Other paths inside `.git` are equally subject to differ from\n> `<gitdir>/<path>`, i.e. inside worktrees, where _some_ paths live in the\n> \"gitdir\" and some live in the \"commondir\" (i.e. the \"gitdir\" of the main\n> worktree).\n> \n> For Git versions older than v2.5.0 (which was the first version to\n> support the `--git-path` option for the `rev-parse` command), we simply\n> fall back to the previous code.\n> \n> An original patch handled only the hooksPath setting, however, during\n> the code submission it was deemed better to fix all call to the `gitdir`\n> function.\n> \n> To avoid spawning a gazillion `git rev-parse --git-path` instances, we\n> cache the returned paths, priming the cache upon startup in a single\n> `git rev-parse invocation` with some paths (that have been\n> determined via a typical startup and via grepping the source code for\n> calls to the `gitdir` function).\n> \n> This fixes https://github.com/git-for-windows/git/issues/1755\n> \n> Initial-patch-by: Philipp Gortan <philipp@gortan.org>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  git-gui.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++----\n>  1 file changed, 58 insertions(+), 4 deletions(-)\n> \n> diff --git a/git-gui.sh b/git-gui.sh\n> index fd476b6999..c684dc7ae1 100755\n> --- a/git-gui.sh\n> +++ b/git-gui.sh\n> @@ -158,6 +158,7 @@ if {[tk windowingsystem] eq \"aqua\"} {\n>  \n>  set _appname {Git Gui}\n>  set _gitdir {}\n> +array set _gitdir_cache {}\n>  set _gitworktree {}\n>  set _isbare {}\n>  set _gitexec {}\n> @@ -197,12 +198,59 @@ proc appname {} {\n>  \treturn $_appname\n>  }\n>  \n> +proc prime_gitdir_cache {} {\n> +\tglobal _gitdir _gitdir_cache\n> +\n> +\tset gitdir_cmd [list git rev-parse --git-dir]\n> +\n> +\t# `--git-path` is only supported since Git v2.5.0\n> +\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n> +\t\t# This list was generated from a typical startup as well as from\n> +\t\t# grepping through Git GUI's source code.\n> +\t\tset gitdir_keys [list \\\n> +\t\t\tCHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \\\n> +\t\t\tGITGUI_MSG HEAD hooks hooks/prepare-commit-msg \\\n> +\t\t\tindex.lock info info/exclude logs MERGE_HEAD MERGE_MSG \\\n> +\t\t\tMERGE_RR objects \"objects/4\\[0-1\\]/*\" \\\n> +\t\t\t\"objects/4\\[0-3\\]/*\" objects/info \\\n> +\t\t\tobjects/info/alternates objects/pack packed-refs \\\n> +\t\t\tPREPARE_COMMIT_MSG rebase-merge/head-name remotes \\\n> +\t\t\trr-cache rr-cache/MERGE_RR SQUASH_MSG \\\n> +\t\t]\n> +\n> +\t\tforeach key $gitdir_keys {\n> +\t\t\tlappend gitdir_cmd --git-path $key\n> +\t\t}\n> +\t}\n> +\n> +\tset i -1\n> +\tforeach path [split [eval $gitdir_cmd] \"\\n\"] {\n> +\t\tif {$i eq -1} {\n> +\t\t\tset _gitdir $path\n> +\t\t} else {\n> +\t\t\tset _gitdir_cache([lindex $gitdir_keys $i]) $path\n> +\t\t}\n> +\t\tincr i\n> +\t}\n> +}\n> +\n>  proc gitdir {args} {\n> -\tglobal _gitdir\n> +\tglobal _gitdir _gitdir_cache\n> +\n>  \tif {$args eq {}} {\n>  \t\treturn $_gitdir\n>  \t}\n> -\treturn [eval [list file join $_gitdir] $args]\n> +\n> +\tset args [eval [list file join] $args]\n> +\tif {![info exists _gitdir_cache($args)]} {\n> +\t\tif {[package vcompare $::_git_version 2.5.0] >= 0} {\n> +\t\t\tset _gitdir_cache($args) [git rev-parse --git-path $args]\n> +\t\t} else {\n> +\t\t\tset _gitdir_cache($args) [file join $_gitdir $args]\n> +\t\t}\n> +\t}\n> +\n> +\treturn $_gitdir_cache($args)\n>  }\n>  \n>  proc gitexec {args} {\n> @@ -1242,7 +1290,7 @@ if {[catch {\n>  \t&& [catch {\n>  \t\t# beware that from the .git dir this sets _gitdir to .\n>  \t\t# and _prefix to the empty string\n> -\t\tset _gitdir [git rev-parse --git-dir]\n> +\t\tprime_gitdir_cache\n>  \t\tset _prefix [git rev-parse --show-prefix]\n>  \t} err]} {\n>  \tload_config 1\n\nLooks good till here.\n\n> @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {\n>  \tglobal HEAD PARENT MERGE_HEAD commit_type\n>  \tglobal ui_index ui_workdir ui_comm\n>  \tglobal rescan_active file_states\n> -\tglobal repo_config\n> +\tglobal repo_config _gitdir_cache\n>  \n>  \tif {$rescan_active > 0 || ![lock_index read]} return\n>  \n> +\t# Only re-prime gitdir cache on a full rescan\n> +\tif {$after ne \"ui_ready\"} {\n\nWhat do you mean by a \"full rescan\"? I assume you use it as the \ndifferentiator between `ui_do_rescan` (called when you hit F5 or choose \nrescan from the menu) and `do_rescan` (called when you revert a line or \nhunk), and a \"full rescan\" refers to `ui_do_rescan`.\n\nWell in that case, this check is incorrect. `do_rescan` passes only \n\"ui_ready\" and `ui_do_rescan` passes \"force_first_diff ui_ready\".\n\nBut either way, I'm not a big fan of this. This check makes assumptions \nabout the behaviour of its callers based on what they pass to $after. \nThe way I see it, $after should be a black box to `rescan`, and it \nshould make absolutely no assumptions about it.\n\nDoing it this way is really brittle, and would break as soon as someone \nchanges the behaviour of `ui_do_rescan`. If someone in the future passes \na different value in $after, this would stop working as intended and \nwould not refresh the cached list on a rescan.\n\nSo, I think a better place for this if statement would be in \n`ui_do_rescan`. This would mean adding a new function that does this. \nBut if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not \nto), we can get away with just something like:\n\n  proc ui_do_rescan {} {\n  \trescan {prime_gitdir_cache; ui_ready}\n  }\n\nThough since `prime_gitdir_cache` does not really depend on the rescan \nbeing finished, something like this would also work fine:\n\n  proc ui_do_rescan {} {\n  \trescan ui_ready\n  \tprime_gitdir_cache\n  }\n\nThis would allow us to do these two things in parallel since `rescan` is \nasynchronous. But that would also mean it is possible that the status \nbar would show \"Ready\" while `prime_gitdir_cache` is still executing.\n\nI can't really make up my mind on what is better. I'm inclining on using \nthe latter way, effectively trading a bit of UI inconsistency for \nperformance (at least in theory).\n\nThoughts?\n\n> +\t\tarray unset _gitdir_cache\n> +\t\tprime_gitdir_cache\n> +\t}\n> +\n>  \trepository_state newType newHEAD newMERGE_HEAD\n>  \tif {[string match amend* $commit_type]\n>  \t\t&& $newType eq {normal}\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"383976","messageId":"nycvar.QRO.7.76.6.1910122321211.3272@tvgsbejvaqbjf.bet","threadId":"51931","inReplyTo":"20191011222611.le5lyf6mr5lmvbbd@yadavpratyush.com","subject":"Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-12T21:24:12Z","receivedAt":"2019-10-12T21:24:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Pratyush,\n\nOn Sat, 12 Oct 2019, Pratyush Yadav wrote:\n\n> On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:\n>\n> > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {\n> >  \tglobal HEAD PARENT MERGE_HEAD commit_type\n> >  \tglobal ui_index ui_workdir ui_comm\n> >  \tglobal rescan_active file_states\n> > -\tglobal repo_config\n> > +\tglobal repo_config _gitdir_cache\n> >\n> >  \tif {$rescan_active > 0 || ![lock_index read]} return\n> >\n> > +\t# Only re-prime gitdir cache on a full rescan\n> > +\tif {$after ne \"ui_ready\"} {\n>\n> What do you mean by a \"full rescan\"? I assume you use it as the\n> differentiator between `ui_do_rescan` (called when you hit F5 or choose\n> rescan from the menu) and `do_rescan` (called when you revert a line or\n> hunk), and a \"full rescan\" refers to `ui_do_rescan`.\n>\n> Well in that case, this check is incorrect. `do_rescan` passes only\n> \"ui_ready\" and `ui_do_rescan` passes \"force_first_diff ui_ready\".\n>\n> But either way, I'm not a big fan of this. This check makes assumptions\n> about the behaviour of its callers based on what they pass to $after.\n> The way I see it, $after should be a black box to `rescan`, and it\n> should make absolutely no assumptions about it.\n>\n> Doing it this way is really brittle, and would break as soon as someone\n> changes the behaviour of `ui_do_rescan`. If someone in the future passes\n> a different value in $after, this would stop working as intended and\n> would not refresh the cached list on a rescan.\n>\n> So, I think a better place for this if statement would be in\n> `ui_do_rescan`. This would mean adding a new function that does this.\n> But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not\n> to), we can get away with just something like:\n>\n>   proc ui_do_rescan {} {\n>   \trescan {prime_gitdir_cache; ui_ready}\n>   }\n>\n> Though since `prime_gitdir_cache` does not really depend on the rescan\n> being finished, something like this would also work fine:\n>\n>   proc ui_do_rescan {} {\n>   \trescan ui_ready\n>   \tprime_gitdir_cache\n>   }\n\nThat was my first attempt. However, there is a very important piece of\ncode that is even still quoted above: that `if {$rescan_active > 0 ||\n![lock_index read]} return` part.\n\nI do _not_ want to interfere with an actively-going-on rescan. If there\nis an active one, I don't want to re-prime the `_gitdir` cache.\n\nThat was the reason why I put the additional code into `rescan` rather\nthan into `ui_do_rescan()`.\n\nCiao,\nJohannes\n\n>\n> This would allow us to do these two things in parallel since `rescan` is\n> asynchronous. But that would also mean it is possible that the status\n> bar would show \"Ready\" while `prime_gitdir_cache` is still executing.\n>\n> I can't really make up my mind on what is better. I'm inclining on using\n> the latter way, effectively trading a bit of UI inconsistency for\n> performance (at least in theory).\n>\n> Thoughts?\n>\n> > +\t\tarray unset _gitdir_cache\n> > +\t\tprime_gitdir_cache\n> > +\t}\n> > +\n> >  \trepository_state newType newHEAD newMERGE_HEAD\n> >  \tif {[string match amend* $commit_type]\n> >  \t\t&& $newType eq {normal}\n>\n> --\n> Regards,\n> Pratyush Yadav\n>\n"},{"id":"383996","messageId":"20191013185540.fwyn4ox5dbyxgvnh@yadavpratyush.com","threadId":"51931","inReplyTo":"nycvar.QRO.7.76.6.1910122321211.3272@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-10-13T18:55:40Z","receivedAt":"2019-10-13T18:55:46Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 12/10/19 11:24PM, Johannes Schindelin wrote:\n> Hi Pratyush,\n> \n> On Sat, 12 Oct 2019, Pratyush Yadav wrote:\n> \n> > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:\n> >\n> > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {\n> > >  \tglobal HEAD PARENT MERGE_HEAD commit_type\n> > >  \tglobal ui_index ui_workdir ui_comm\n> > >  \tglobal rescan_active file_states\n> > > -\tglobal repo_config\n> > > +\tglobal repo_config _gitdir_cache\n> > >\n> > >  \tif {$rescan_active > 0 || ![lock_index read]} return\n> > >\n> > > +\t# Only re-prime gitdir cache on a full rescan\n> > > +\tif {$after ne \"ui_ready\"} {\n> >\n> > What do you mean by a \"full rescan\"? I assume you use it as the\n> > differentiator between `ui_do_rescan` (called when you hit F5 or choose\n> > rescan from the menu) and `do_rescan` (called when you revert a line or\n> > hunk), and a \"full rescan\" refers to `ui_do_rescan`.\n> >\n> > Well in that case, this check is incorrect. `do_rescan` passes only\n> > \"ui_ready\" and `ui_do_rescan` passes \"force_first_diff ui_ready\".\n> >\n> > But either way, I'm not a big fan of this. This check makes assumptions\n> > about the behaviour of its callers based on what they pass to $after.\n> > The way I see it, $after should be a black box to `rescan`, and it\n> > should make absolutely no assumptions about it.\n> >\n> > Doing it this way is really brittle, and would break as soon as someone\n> > changes the behaviour of `ui_do_rescan`. If someone in the future passes\n> > a different value in $after, this would stop working as intended and\n> > would not refresh the cached list on a rescan.\n> >\n> > So, I think a better place for this if statement would be in\n> > `ui_do_rescan`. This would mean adding a new function that does this.\n> > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not\n> > to), we can get away with just something like:\n> >\n> >   proc ui_do_rescan {} {\n> >   \trescan {prime_gitdir_cache; ui_ready}\n> >   }\n> >\n> > Though since `prime_gitdir_cache` does not really depend on the rescan\n> > being finished, something like this would also work fine:\n> >\n> >   proc ui_do_rescan {} {\n> >   \trescan ui_ready\n> >   \tprime_gitdir_cache\n> >   }\n> \n> That was my first attempt. However, there is a very important piece of\n> code that is even still quoted above: that `if {$rescan_active > 0 ||\n> ![lock_index read]} return` part.\n> \n> I do _not_ want to interfere with an actively-going-on rescan. If there\n> is an active one, I don't want to re-prime the `_gitdir` cache.\n\nGood catch! In that case I suppose refreshing the cache in $after would \nbe the way to go (IOW, the former of my two suggestions). Anything \n$after won't get executed if we return early from that check.\n \n> That was the reason why I put the additional code into `rescan` rather\n> than into `ui_do_rescan()`.\n> \n> Ciao,\n> Johannes\n> \n> >\n> > This would allow us to do these two things in parallel since `rescan` is\n> > asynchronous. But that would also mean it is possible that the status\n> > bar would show \"Ready\" while `prime_gitdir_cache` is still executing.\n> >\n> > I can't really make up my mind on what is better. I'm inclining on using\n> > the latter way, effectively trading a bit of UI inconsistency for\n> > performance (at least in theory).\n> >\n> > Thoughts?\n> >\n> > > +\t\tarray unset _gitdir_cache\n> > > +\t\tprime_gitdir_cache\n> > > +\t}\n> > > +\n> > >  \trepository_state newType newHEAD newMERGE_HEAD\n> > >  \tif {[string match amend* $commit_type]\n> > >  \t\t&& $newType eq {normal}\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"384000","messageId":"nycvar.QRO.7.76.6.1910140015500.46@tvgsbejvaqbjf.bet","threadId":"51931","inReplyTo":"20191013185540.fwyn4ox5dbyxgvnh@yadavpratyush.com","subject":"Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-13T22:18:51Z","receivedAt":"2019-10-13T22:19:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Pratyush,\n\nOn Mon, 14 Oct 2019, Pratyush Yadav wrote:\n\n> On 12/10/19 11:24PM, Johannes Schindelin wrote:\n> > Hi Pratyush,\n> >\n> > On Sat, 12 Oct 2019, Pratyush Yadav wrote:\n> >\n> > > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:\n> > >\n> > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {\n> > > >  \tglobal HEAD PARENT MERGE_HEAD commit_type\n> > > >  \tglobal ui_index ui_workdir ui_comm\n> > > >  \tglobal rescan_active file_states\n> > > > -\tglobal repo_config\n> > > > +\tglobal repo_config _gitdir_cache\n> > > >\n> > > >  \tif {$rescan_active > 0 || ![lock_index read]} return\n> > > >\n> > > > +\t# Only re-prime gitdir cache on a full rescan\n> > > > +\tif {$after ne \"ui_ready\"} {\n> > >\n> > > What do you mean by a \"full rescan\"? I assume you use it as the\n> > > differentiator between `ui_do_rescan` (called when you hit F5 or choose\n> > > rescan from the menu) and `do_rescan` (called when you revert a line or\n> > > hunk), and a \"full rescan\" refers to `ui_do_rescan`.\n> > >\n> > > Well in that case, this check is incorrect. `do_rescan` passes only\n> > > \"ui_ready\" and `ui_do_rescan` passes \"force_first_diff ui_ready\".\n> > >\n> > > But either way, I'm not a big fan of this. This check makes assumptions\n> > > about the behaviour of its callers based on what they pass to $after.\n> > > The way I see it, $after should be a black box to `rescan`, and it\n> > > should make absolutely no assumptions about it.\n> > >\n> > > Doing it this way is really brittle, and would break as soon as someone\n> > > changes the behaviour of `ui_do_rescan`. If someone in the future passes\n> > > a different value in $after, this would stop working as intended and\n> > > would not refresh the cached list on a rescan.\n> > >\n> > > So, I think a better place for this if statement would be in\n> > > `ui_do_rescan`. This would mean adding a new function that does this.\n> > > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not\n> > > to), we can get away with just something like:\n> > >\n> > >   proc ui_do_rescan {} {\n> > >   \trescan {prime_gitdir_cache; ui_ready}\n> > >   }\n> > >\n> > > Though since `prime_gitdir_cache` does not really depend on the rescan\n> > > being finished, something like this would also work fine:\n> > >\n> > >   proc ui_do_rescan {} {\n> > >   \trescan ui_ready\n> > >   \tprime_gitdir_cache\n> > >   }\n> >\n> > That was my first attempt. However, there is a very important piece of\n> > code that is even still quoted above: that `if {$rescan_active > 0 ||\n> > ![lock_index read]} return` part.\n> >\n> > I do _not_ want to interfere with an actively-going-on rescan. If there\n> > is an active one, I don't want to re-prime the `_gitdir` cache.\n>\n> Good catch! In that case I suppose refreshing the cache in $after would\n> be the way to go (IOW, the former of my two suggestions). Anything\n> $after won't get executed if we return early from that check.\n\nThe obvious problem with that solution is that the `_gitdir` is reset\n_after_ the rescan. In my solution, it is reset _before_, as I have no\nidea how often the `_gitdir` values are used during a rescan (and if\nnone of they were, I would like to be very cautious to prepare for a\nfuture where any of those `_gitdir` values _are_ used during a rescan).\n\nSo I am afraid that I find way too serious problems with both of your\nproposed alternatives.\n\nCiao,\nJohannes\n\n>\n> > That was the reason why I put the additional code into `rescan` rather\n> > than into `ui_do_rescan()`.\n> >\n> > Ciao,\n> > Johannes\n> >\n> > >\n> > > This would allow us to do these two things in parallel since `rescan` is\n> > > asynchronous. But that would also mean it is possible that the status\n> > > bar would show \"Ready\" while `prime_gitdir_cache` is still executing.\n> > >\n> > > I can't really make up my mind on what is better. I'm inclining on using\n> > > the latter way, effectively trading a bit of UI inconsistency for\n> > > performance (at least in theory).\n> > >\n> > > Thoughts?\n> > >\n> > > > +\t\tarray unset _gitdir_cache\n> > > > +\t\tprime_gitdir_cache\n> > > > +\t}\n> > > > +\n> > > >  \trepository_state newType newHEAD newMERGE_HEAD\n> > > >  \tif {[string match amend* $commit_type]\n> > > >  \t\t&& $newType eq {normal}\n>\n> --\n> Regards,\n> Pratyush Yadav\n>\n>\n"},{"id":"384006","messageId":"nycvar.QRO.7.76.6.1910141013360.46@tvgsbejvaqbjf.bet","threadId":"51931","inReplyTo":"20191013185540.fwyn4ox5dbyxgvnh@yadavpratyush.com","subject":"Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-14T08:14:24Z","receivedAt":"2019-10-14T08:14:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Pratyush,\n\nOn Mon, 14 Oct 2019, Pratyush Yadav wrote:\n\n> On 12/10/19 11:24PM, Johannes Schindelin wrote:\n> >\n> > On Sat, 12 Oct 2019, Pratyush Yadav wrote:\n> >\n> > > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:\n> > >\n> > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {\n> > > >  \tglobal HEAD PARENT MERGE_HEAD commit_type\n> > > >  \tglobal ui_index ui_workdir ui_comm\n> > > >  \tglobal rescan_active file_states\n> > > > -\tglobal repo_config\n> > > > +\tglobal repo_config _gitdir_cache\n> > > >\n> > > >  \tif {$rescan_active > 0 || ![lock_index read]} return\n> > > >\n> > > > +\t# Only re-prime gitdir cache on a full rescan\n> > > > +\tif {$after ne \"ui_ready\"} {\n> > >\n> > > What do you mean by a \"full rescan\"? I assume you use it as the\n> > > differentiator between `ui_do_rescan` (called when you hit F5 or choose\n> > > rescan from the menu) and `do_rescan` (called when you revert a line or\n> > > hunk), and a \"full rescan\" refers to `ui_do_rescan`.\n> > >\n> > > Well in that case, this check is incorrect. `do_rescan` passes only\n> > > \"ui_ready\" and `ui_do_rescan` passes \"force_first_diff ui_ready\".\n> > >\n> > > But either way, I'm not a big fan of this. This check makes assumptions\n> > > about the behaviour of its callers based on what they pass to $after.\n> > > The way I see it, $after should be a black box to `rescan`, and it\n> > > should make absolutely no assumptions about it.\n> > >\n> > > Doing it this way is really brittle, and would break as soon as someone\n> > > changes the behaviour of `ui_do_rescan`. If someone in the future passes\n> > > a different value in $after, this would stop working as intended and\n> > > would not refresh the cached list on a rescan.\n> > >\n> > > So, I think a better place for this if statement would be in\n> > > `ui_do_rescan`. This would mean adding a new function that does this.\n> > > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not\n> > > to), we can get away with just something like:\n> > >\n> > >   proc ui_do_rescan {} {\n> > >   \trescan {prime_gitdir_cache; ui_ready}\n> > >   }\n> > >\n> > > Though since `prime_gitdir_cache` does not really depend on the rescan\n> > > being finished, something like this would also work fine:\n> > >\n> > >   proc ui_do_rescan {} {\n> > >   \trescan ui_ready\n> > >   \tprime_gitdir_cache\n> > >   }\n> >\n> > That was my first attempt. However, there is a very important piece of\n> > code that is even still quoted above: that `if {$rescan_active > 0 ||\n> > ![lock_index read]} return` part.\n> >\n> > I do _not_ want to interfere with an actively-going-on rescan. If there\n> > is an active one, I don't want to re-prime the `_gitdir` cache.\n>\n> Good catch! In that case I suppose refreshing the cache in $after would\n> be the way to go (IOW, the former of my two suggestions). Anything\n> $after won't get executed if we return early from that check.\n\nBut it also won't get executed before the actual rescan. Which is\nprecisely what I tried to ensure.\n\nCiao,\nJohannes\n\n>\n> > That was the reason why I put the additional code into `rescan` rather\n> > than into `ui_do_rescan()`.\n> >\n> > Ciao,\n> > Johannes\n> >\n> > >\n> > > This would allow us to do these two things in parallel since `rescan` is\n> > > asynchronous. But that would also mean it is possible that the status\n> > > bar would show \"Ready\" while `prime_gitdir_cache` is still executing.\n> > >\n> > > I can't really make up my mind on what is better. I'm inclining on using\n> > > the latter way, effectively trading a bit of UI inconsistency for\n> > > performance (at least in theory).\n> > >\n> > > Thoughts?\n> > >\n> > > > +\t\tarray unset _gitdir_cache\n> > > > +\t\tprime_gitdir_cache\n> > > > +\t}\n> > > > +\n> > > >  \trepository_state newType newHEAD newMERGE_HEAD\n> > > >  \tif {[string match amend* $commit_type]\n> > > >  \t\t&& $newType eq {normal}\n>\n> --\n> Regards,\n> Pratyush Yadav\n>\n"},{"id":"384285","messageId":"20191017183437.pc6kpin2zvdtwmqu@yadavpratyush.com","threadId":"51931","inReplyTo":"nycvar.QRO.7.76.6.1910140015500.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-10-17T18:34:37Z","receivedAt":"2019-10-17T18:34:43Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 14/10/19 12:18AM, Johannes Schindelin wrote:\n> Hi Pratyush,\n> \n> On Mon, 14 Oct 2019, Pratyush Yadav wrote:\n> \n> > On 12/10/19 11:24PM, Johannes Schindelin wrote:\n> > > Hi Pratyush,\n> > >\n> > > On Sat, 12 Oct 2019, Pratyush Yadav wrote:\n> > >\n> > > > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:\n> > > >\n> > > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {\n> > > > >  \tglobal HEAD PARENT MERGE_HEAD commit_type\n> > > > >  \tglobal ui_index ui_workdir ui_comm\n> > > > >  \tglobal rescan_active file_states\n> > > > > -\tglobal repo_config\n> > > > > +\tglobal repo_config _gitdir_cache\n> > > > >\n> > > > >  \tif {$rescan_active > 0 || ![lock_index read]} return\n> > > > >\n> > > > > +\t# Only re-prime gitdir cache on a full rescan\n> > > > > +\tif {$after ne \"ui_ready\"} {\n> > > >\n> > > > What do you mean by a \"full rescan\"? I assume you use it as the\n> > > > differentiator between `ui_do_rescan` (called when you hit F5 or choose\n> > > > rescan from the menu) and `do_rescan` (called when you revert a line or\n> > > > hunk), and a \"full rescan\" refers to `ui_do_rescan`.\n> > > >\n> > > > Well in that case, this check is incorrect. `do_rescan` passes only\n> > > > \"ui_ready\" and `ui_do_rescan` passes \"force_first_diff ui_ready\".\n> > > >\n> > > > But either way, I'm not a big fan of this. This check makes assumptions\n> > > > about the behaviour of its callers based on what they pass to $after.\n> > > > The way I see it, $after should be a black box to `rescan`, and it\n> > > > should make absolutely no assumptions about it.\n> > > >\n> > > > Doing it this way is really brittle, and would break as soon as someone\n> > > > changes the behaviour of `ui_do_rescan`. If someone in the future passes\n> > > > a different value in $after, this would stop working as intended and\n> > > > would not refresh the cached list on a rescan.\n> > > >\n> > > > So, I think a better place for this if statement would be in\n> > > > `ui_do_rescan`. This would mean adding a new function that does this.\n> > > > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not\n> > > > to), we can get away with just something like:\n> > > >\n> > > >   proc ui_do_rescan {} {\n> > > >   \trescan {prime_gitdir_cache; ui_ready}\n> > > >   }\n> > > >\n> > > > Though since `prime_gitdir_cache` does not really depend on the rescan\n> > > > being finished, something like this would also work fine:\n> > > >\n> > > >   proc ui_do_rescan {} {\n> > > >   \trescan ui_ready\n> > > >   \tprime_gitdir_cache\n> > > >   }\n> > >\n> > > That was my first attempt. However, there is a very important piece of\n> > > code that is even still quoted above: that `if {$rescan_active > 0 ||\n> > > ![lock_index read]} return` part.\n> > >\n> > > I do _not_ want to interfere with an actively-going-on rescan. If there\n> > > is an active one, I don't want to re-prime the `_gitdir` cache.\n> >\n> > Good catch! In that case I suppose refreshing the cache in $after would\n> > be the way to go (IOW, the former of my two suggestions). Anything\n> > $after won't get executed if we return early from that check.\n> \n> The obvious problem with that solution is that the `_gitdir` is reset\n> _after_ the rescan. In my solution, it is reset _before_, as I have no\n> idea how often the `_gitdir` values are used during a rescan (and if\n> none of they were, I would like to be very cautious to prepare for a\n> future where any of those `_gitdir` values _are_ used during a rescan).\n\n_gitdir values are used quite often during a rescan, so yes, this won't \nwork.\n \n> So I am afraid that I find way too serious problems with both of your\n> proposed alternatives.\n\nOne alternative I can see right now is adding another optional parameter \nto `rescan` that controls whether we refresh the gitdir cache or not. \nThat parameter would default to 0/false. I'm not the biggest fan of \nsomething like this, but it might be the easiest way to do it given the \nconstraints.\n\nI also thought about trying to acquire the index lock in \n`prime_gitdir_cache`, but that could create a race on the lock between \n`prime_gitdir_cache` and `rescan`.\n\nIf you have any better ideas, I'm all ears, but otherwise, I maybe our \nbest bet is to just go with adding an optional parameter to `rescan`.\n\n-- \nRegards,\nPratyush Yadav\n"}]}