Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks
Hi Pratyush,
On Fri, 4 Oct 2019, Pratyush Yadav wrote:
> On 01/10/19 07:38PM, Johannes Schindelin wrote:
> >
> > On Tue, 1 Oct 2019, Pratyush Yadav wrote:
> >
> > > On 30/09/19 11:42AM, Johannes Schindelin wrote:
> > > > On Fri, 27 Sep 2019, Pratyush Yadav wrote:
> > > > > On 27/09/19 08:10AM, Bert Wesarg wrote:
> > > > > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav
> > > > > > wrote:
> > > > > > > gitdir is used in a lot of places, and I think all those would
> > > > > > > also
> > > > > > > benefit from using --git-path. So I think it is a better idea to
> > > > > > > move
> > > > > > > this to the procedure gitdir. It would have to be refactored to
> > > > > > > take any
> > > > > > > number of arguments, instead of the two it takes here.
> > > >
> > > > The `gitdir` function is called 13 times during startup alone, and who
> > > > knows how many more times later.
> > > >
> > > > So I am quite convinced that the original intention was to save on
> > > > spawning processes left and right.
> > > >
> > > > But since you are the Git GUI maintainer, and this was your suggestion,
> > > > I made it so.
> > >
> > > Yes, I am the maintainer, but I am not an all-knowing, all-seeing
> > > entity. Your, and every other contributors, suggestions are very
> > > valuable. And my suggestions aren't gospel. I would hate to see someone
> > > send in a patch they weren't sure was the best thing to do just because
> > > I suggested it. Please feel free to object my suggestions.
> > >
> > > In this case, I didn't expect gitdir to be called this many times.
> > >
> > > While I don't notice much of a performance difference on my system
> > > (Linux), a quick measurement tells me that the time spent in gitdir is
> > > about 16 ms. In contrast, the same measurement without the v2 patch
> > > gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something
> > > so simple. It might not be the same for everyone else. AFAIK, spawning a
> > > process is much slower on Windows.
> > >
> > > So now I'm not so sure my suggestion was a good one. My original aim was
> > > to be sure everything was correct, and no incorrect directories were
> > > being used. But the current solution comes at a performance hit.
> > >
> > > > > > We could either maintain a blacklist, for what we cache the result
> > > > > > too, or always call "git rev-parse --git-dir".
> > > > > >
> > > > > > This blacklist would need to be in sync with the one in Git's
> > > > > > path.c::adjust_git_path() than.
> > >
> > > Bert's suggestion seems like a decent compromise. We run `git rev-parse
> > > --git-path` for the paths in the blacklist, and for the rest we use the
> > > cached value. This does run the risk of getting out of sync with
> > > git.git's list, but it might be better than spawing a process every
> > > time, and is very likely better than just doing it for hooks.
> >
> > But what about this part of that function?
> >
> > -- snip --
> > else if (repo->different_commondir)
> > update_common_dir(buf, git_dir_len, repo->commondir);
> > -- snap --
>
> I'm afraid I'm a bit out of my depth on this. I have no idea what a
> "common directory" is, and how is it different from the "git directory".
> I can't find anything useful on Google about it. My guess is that it is
> something related to separate worktrees.
It is indeed related to worktrees. If you create a secondary worktree
via `git worktree add [...]`, that work tree will get its own git
directory under `.git/worktrees/` in the main worktree. That git
directory will not, however, contain all contents of a regular git
directory. Most refs, for example, are stored in the main worktree's git
directory. That is what the "common dir" is.
> > It might well turn out that this blacklist is neither easy to implement
> > nor will it help much.
>
> Am I correct in assuming that for other cases like "info", "grafts",
> "index", "objects", and "hooks" the blacklist would be simple to
> implement, and it is the "common directory" case that is problematic?
Indeed, for the other, simple cases, the list would be unproblematic to
implement. Problematic to maintain, though, especially given that Git
GUI is _supposed_ to support even very old Git versions.
And those simple cases don't include _all_ interesting cases. Take
`logs/` for example. The git directory will contain the reflogs for
`HEAD`, but unless you're on an unnamed branch (AKA "detached HEAD"),
the reflogs for the current branch are _in the commondir_.
> > So let's look at all the call sites:
> >
> > -- snip --
> > $ git grep -w gitdir | sed -ne 's|\].*||' -e 's|.*\[gitdir ||p' | sort |
> > uniq
> > $file
> > $name
> > CHERRY_PICK_HEAD
> > FETCH_HEAD
> > GITGUI_BCK
> > GITGUI_EDITMSG
> > GITGUI_MSG
> > HEAD
> > hooks $hook_name
> > index.lock
> > info exclude
> > logs $name
> > MERGE_HEAD
> > MERGE_MSG
> > MERGE_RR
> > objects 4\[0-[expr {$ndirs-1}
> > objects info
> > objects info alternates
> > objects pack
> > packed-refs
> > PREPARE_C
Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks
On 01/10/19 07:38PM, Johannes Schindelin wrote:
> Hi,
>
> On Tue, 1 Oct 2019, Pratyush Yadav wrote:
>
> > On 30/09/19 11:42AM, Johannes Schindelin wrote:
> > > On Fri, 27 Sep 2019, Pratyush Yadav wrote:
> > > > On 27/09/19 08:10AM, Bert Wesarg wrote:
> > > > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav
> > > > > wrote:
> > > > > > gitdir is used in a lot of places, and I think all those would
> > > > > > also
> > > > > > benefit from using --git-path. So I think it is a better idea to
> > > > > > move
> > > > > > this to the procedure gitdir. It would have to be refactored to
> > > > > > take any
> > > > > > number of arguments, instead of the two it takes here.
> > >
> > > The `gitdir` function is called 13 times during startup alone, and who
> > > knows how many more times later.
> > >
> > > So I am quite convinced that the original intention was to save on
> > > spawning processes left and right.
> > >
> > > But since you are the Git GUI maintainer, and this was your suggestion,
> > > I made it so.
> >
> > Yes, I am the maintainer, but I am not an all-knowing, all-seeing
> > entity. Your, and every other contributors, suggestions are very
> > valuable. And my suggestions aren't gospel. I would hate to see someone
> > send in a patch they weren't sure was the best thing to do just because
> > I suggested it. Please feel free to object my suggestions.
> >
> > In this case, I didn't expect gitdir to be called this many times.
> >
> > While I don't notice much of a performance difference on my system
> > (Linux), a quick measurement tells me that the time spent in gitdir is
> > about 16 ms. In contrast, the same measurement without the v2 patch
> > gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something
> > so simple. It might not be the same for everyone else. AFAIK, spawning a
> > process is much slower on Windows.
> >
> > So now I'm not so sure my suggestion was a good one. My original aim was
> > to be sure everything was correct, and no incorrect directories were
> > being used. But the current solution comes at a performance hit.
> >
> > > > > We could either maintain a blacklist, for what we cache the result
> > > > > too, or always call "git rev-parse --git-dir".
> > > > >
> > > > > This blacklist would need to be in sync with the one in Git's
> > > > > path.c::adjust_git_path() than.
> >
> > Bert's suggestion seems like a decent compromise. We run `git rev-parse
> > --git-path` for the paths in the blacklist, and for the rest we use the
> > cached value. This does run the risk of getting out of sync with
> > git.git's list, but it might be better than spawing a process every
> > time, and is very likely better than just doing it for hooks.
>
> But what about this part of that function?
>
> -- snip --
> else if (repo->different_commondir)
> update_common_dir(buf, git_dir_len, repo->commondir);
> -- snap --
I'm afraid I'm a bit out of my depth on this. I have no idea what a
"common directory" is, and how is it different from the "git directory".
I can't find anything useful on Google about it. My guess is that it is
something related to separate worktrees.
> It might well turn out that this blacklist is neither easy to implement
> nor will it help much.
Am I correct in assuming that for other cases like "info", "grafts",
"index", "objects", and "hooks" the blacklist would be simple to
implement, and it is the "common directory" case that is problematic?
> So let's look at all the call sites:
>
> -- snip --
> $ git grep -w gitdir | sed -ne 's|\].*||' -e 's|.*\[gitdir ||p' | sort | uniq
> $file
> $name
> CHERRY_PICK_HEAD
> FETCH_HEAD
> GITGUI_BCK
> GITGUI_EDITMSG
> GITGUI_MSG
> HEAD
> hooks $hook_name
> index.lock
> info exclude
> logs $name
> MERGE_HEAD
> MERGE_MSG
> MERGE_RR
> objects 4\[0-[expr {$ndirs-1}
> objects info
> objects info alternates
> objects pack
> packed-refs
> PREPARE_COMMIT_MSG
> rebase-merge head-name
> remotes
> remotes $r
> rr-cache
> rr-cache MERGE_RR
> SQUASH_MSG
> -- snap --
>
> The `$file` call looks for messages (probably commit, merge, tag
> messages and the likes), the `$name` one looks for refs.
So they should always be inside the '.git' or GIT_DIR, correct?
> Some of those arguments strike me as very good candidates to require the
> common directory while others require the real gitdir (remember,
> commondir != gitdir in worktrees other than the main worktree).
>
> What _could_ be done (but we're certainly threatening to enter the realm
> of the ridiculous here) is to call `git rev-parse --git-dir --git-path
> CHERRY_PICK_HEAD --git-path FETCH_HEAD [...]`, which will output one
> path per line, and then store the result in an associative array
> (https://tcl.tk/man/tcl8.5/tutorial/Tcl22.html), and use that to look up
> paths based on their first component, caching as we go.
Ah yes! That is certainly threatening to enter the realm of ridiculous.
I'm not sure what benefit this will have. Right now, I don't think
git-gui
Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks
Hi,
On Tue, 1 Oct 2019, Pratyush Yadav wrote:
> On 30/09/19 11:42AM, Johannes Schindelin wrote:
> > On Fri, 27 Sep 2019, Pratyush Yadav wrote:
> > > On 27/09/19 08:10AM, Bert Wesarg wrote:
> > > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav
> > > > wrote:
> > > > > gitdir is used in a lot of places, and I think all those would
> > > > > also
> > > > > benefit from using --git-path. So I think it is a better idea to move
> > > > > this to the procedure gitdir. It would have to be refactored to take
> > > > > any
> > > > > number of arguments, instead of the two it takes here.
> >
> > The `gitdir` function is called 13 times during startup alone, and who
> > knows how many more times later.
> >
> > So I am quite convinced that the original intention was to save on
> > spawning processes left and right.
> >
> > But since you are the Git GUI maintainer, and this was your suggestion,
> > I made it so.
>
> Yes, I am the maintainer, but I am not an all-knowing, all-seeing
> entity. Your, and every other contributors, suggestions are very
> valuable. And my suggestions aren't gospel. I would hate to see someone
> send in a patch they weren't sure was the best thing to do just because
> I suggested it. Please feel free to object my suggestions.
>
> In this case, I didn't expect gitdir to be called this many times.
>
> While I don't notice much of a performance difference on my system
> (Linux), a quick measurement tells me that the time spent in gitdir is
> about 16 ms. In contrast, the same measurement without the v2 patch
> gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something
> so simple. It might not be the same for everyone else. AFAIK, spawning a
> process is much slower on Windows.
>
> So now I'm not so sure my suggestion was a good one. My original aim was
> to be sure everything was correct, and no incorrect directories were
> being used. But the current solution comes at a performance hit.
>
> > > > We could either maintain a blacklist, for what we cache the result
> > > > too, or always call "git rev-parse --git-dir".
> > > >
> > > > This blacklist would need to be in sync with the one in Git's
> > > > path.c::adjust_git_path() than.
>
> Bert's suggestion seems like a decent compromise. We run `git rev-parse
> --git-path` for the paths in the blacklist, and for the rest we use the
> cached value. This does run the risk of getting out of sync with
> git.git's list, but it might be better than spawing a process every
> time, and is very likely better than just doing it for hooks.
But what about this part of that function?
-- snip --
else if (repo->different_commondir)
update_common_dir(buf, git_dir_len, repo->commondir);
-- snap --
It might well turn out that this blacklist is neither easy to implement
nor will it help much.
So let's look at all the call sites:
-- snip --
$ git grep -w gitdir | sed -ne 's|\].*||' -e 's|.*\[gitdir ||p' | sort | uniq
$file
$name
CHERRY_PICK_HEAD
FETCH_HEAD
GITGUI_BCK
GITGUI_EDITMSG
GITGUI_MSG
HEAD
hooks $hook_name
index.lock
info exclude
logs $name
MERGE_HEAD
MERGE_MSG
MERGE_RR
objects 4\[0-[expr {$ndirs-1}
objects info
objects info alternates
objects pack
packed-refs
PREPARE_COMMIT_MSG
rebase-merge head-name
remotes
remotes $r
rr-cache
rr-cache MERGE_RR
SQUASH_MSG
-- snap --
The `$file` call looks for messages (probably commit, merge, tag
messages and the likes), the `$name` one looks for refs.
Some of those arguments strike me as very good candidates to require the
common directory while others require the real gitdir (remember,
commondir != gitdir in worktrees other than the main worktree).
What _could_ be done (but we're certainly threatening to enter the realm
of the ridiculous here) is to call `git rev-parse --git-dir --git-path
CHERRY_PICK_HEAD --git-path FETCH_HEAD [...]`, which will output one
path per line, and then store the result in an associative array
(https://tcl.tk/man/tcl8.5/tutorial/Tcl22.html), and use that to look up
paths based on their first component, caching as we go.
Something like this:
-- snipsnap --
diff --git a/git-gui.sh b/git-gui.sh
index fd476b6..9295c75 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -158,6 +158,7 @@ if {[tk windowingsystem] eq "aqua"} {
set _appname {Git Gui}
set _gitdir {}
+array set _gitdir_cached {}
set _gitworktree {}
set _isbare {}
set _gitexec {}
@@ -197,12 +198,50 @@ proc appname {} {
return $_appname
}
+proc init_gitdir_cached {} {
+ global _gitdir _gitdir_cached
+
+ set gitdir_keys [list \
+ CHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \
+ GITGUI_MSG HEAD hooks index.lock info logs MERGE_HEAD \
+ MERGE_MSG MERGE_RR objects packed-refs PREPARE_COMMIT_MSG \
+ rebase-merge head-name remotes rr-cache SQUASH_MSG \
+ ]
+
+ set gitdir_cmd [list git rev-parse --git-dir]
+ foreach key $gitdir_keys {
+ lappend gitdir_cmd --git-path $key
+ }
+
+
Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks
On 30/09/19 11:42AM, Johannes Schindelin wrote: > On Fri, 27 Sep 2019, Pratyush Yadav wrote: > > On 27/09/19 08:10AM, Bert Wesarg wrote: > > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav > > > wrote: > > > > gitdir is used in a lot of places, and I think all those would > > > > also > > > > benefit from using --git-path. So I think it is a better idea to move > > > > this to the procedure gitdir. It would have to be refactored to take any > > > > number of arguments, instead of the two it takes here. > > The `gitdir` function is called 13 times during startup alone, and who > knows how many more times later. > > So I am quite convinced that the original intention was to save on > spawning processes left and right. > > But since you are the Git GUI maintainer, and this was your suggestion, > I made it so. Yes, I am the maintainer, but I am not an all-knowing, all-seeing entity. Your, and every other contributors, suggestions are very valuable. And my suggestions aren't gospel. I would hate to see someone send in a patch they weren't sure was the best thing to do just because I suggested it. Please feel free to object my suggestions. In this case, I didn't expect gitdir to be called this many times. While I don't notice much of a performance difference on my system (Linux), a quick measurement tells me that the time spent in gitdir is about 16 ms. In contrast, the same measurement without the v2 patch gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something so simple. It might not be the same for everyone else. AFAIK, spawning a process is much slower on Windows. So now I'm not so sure my suggestion was a good one. My original aim was to be sure everything was correct, and no incorrect directories were being used. But the current solution comes at a performance hit. > > > We could either maintain a blacklist, for what we cache the result > > > too, or always call "git rev-parse --git-dir". > > > > > > This blacklist would need to be in sync with the one in Git's > > > path.c::adjust_git_path() than. Bert's suggestion seems like a decent compromise. We run `git rev-parse --git-path` for the paths in the blacklist, and for the rest we use the cached value. This does run the risk of getting out of sync with git.git's list, but it might be better than spawing a process every time, and is very likely better than just doing it for hooks. Thoughts? -- Regards, Pratyush Yadav
Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks
Hi,
On Fri, 27 Sep 2019, Pratyush Yadav wrote:
> On 27/09/19 08:10AM, Bert Wesarg wrote:
> > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav
> > wrote:
> > >
> > > On 26/09/19 02:17PM, Johannes Schindelin via GitGitGadget wrote:
> > > > From: Johannes Schindelin
> > > >
> > > > Since v2.9.0, Git knows about the config variable core.hookspath
> > > > that allows overriding the path to the directory containing the
> > > > Git hooks.
> > > >
> > > > Since v2.10.0, the `--git-path` option respects that config
> > > > variable, too, so we may just as well use that command.
> > > >
> > > > For Git versions older than v2.5.0 (which was the first version to
> > > > support the `--git-path` option for the `rev-parse` command), we
> > > > simply fall back to the previous code.
> > > >
> > > > This fixes https://github.com/git-for-windows/git/issues/1755
> > > >
> > > > Initial-patch-by: Philipp Gortan
> > > > Signed-off-by: Johannes Schindelin
> > > > ---
> > > > git-gui.sh | 6 +-
> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/git-gui.sh b/git-gui.sh
> > > > index fd476b6999..b2c6e7a1db 100755
> > > > --- a/git-gui.sh
> > > > +++ b/git-gui.sh
> > > > @@ -623,7 +623,11 @@ proc git_write {args} {
> > > > }
> > > >
> > > > proc githook_read {hook_name args} {
> > > > - set pchook [gitdir hooks $hook_name]
> > > > + if {[package vcompare $::_git_version 2.5.0] >= 0} {
> > > > + set pchook [git rev-parse --git-path "hooks/$hook_name"]
> > > > + } else {
> > > > + set pchook [gitdir hooks $hook_name]
> > > > + }
> > >
> > > gitdir is used in a lot of places, and I think all those would also
> > > benefit from using --git-path. So I think it is a better idea to move
> > > this to the procedure gitdir. It would have to be refactored to take any
> > > number of arguments, instead of the two it takes here.
> >
> > gitdir already takes an arbitrary number of arguments and joins them
> > to a path. The more imminent challenge is, that gitdir caches the
> > GIT_DIR, thus it tries to avoid calling "git rev-parse". Which works
> > for most, but not for hooks.
>
> What I was thinking of was something like this:
>
> - If no args are passed, then just directly return $_gitdir. This is
> already being done. I assume the GIT_DIR relocation is already
> handled by `git rev-parse --git-dir`, so this would point to the
> correct location.
> - If args are passed, then we want a subdirectory of GIT_DIR In this
> case, it is possible that this subdirectory has also been relocated
> (hooks/ being one of those subdirectories). So in this case, use
> `git rev-parse --git-path` instead.
>
> So the gitdir procedure would look something like:
>
> proc gitdir {args} {
> global $_gitdir
> if {$args eq {}} {
> # Return the cached GIT_DIR
> return $_gitdir
> }
>
> # Use `git rev-parse --git-path` to get the path instead of
> # using the cached value.
> }
>
> Am I missing something? Or does this fix the issue you describe?
The `gitdir` function is called 13 times during startup alone, and who
knows how many more times later.
So I am quite convinced that the original intention was to save on
spawning processes left and right.
But since you are the Git GUI maintainer, and this was your suggestion,
I made it so.
Ciao,
Johannes
> > We could either maintain a blacklist, for what we cache the result
> > too, or always call "git rev-parse --git-dir".
> >
> > This blacklist would need to be in sync with the one in Git's
> > path.c::adjust_git_path() than.
>
> Is caching GIT_DIR that important in terms of performance? Otherwise,
> I'd say calling `git rev-parse --git-path` for _every_ subdirectory of
> GIT_DIR is a much simpler solution.
>
> --
> Regards,
> Pratyush Yadav
>
Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks
On 27/09/19 08:10AM, Bert Wesarg wrote:
> On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav
> wrote:
> >
> > Hi,
> >
> > On 26/09/19 02:17PM, Johannes Schindelin via GitGitGadget wrote:
> > > From: Johannes Schindelin
> > >
> > > Since v2.9.0, Git knows about the config variable core.hookspath
> > > that allows overriding the path to the directory containing the
> > > Git hooks.
> > >
> > > Since v2.10.0, the `--git-path` option respects that config
> > > variable, too, so we may just as well use that command.
> > >
> > > For Git versions older than v2.5.0 (which was the first version to
> > > support the `--git-path` option for the `rev-parse` command), we
> > > simply fall back to the previous code.
> > >
> > > This fixes https://github.com/git-for-windows/git/issues/1755
> > >
> > > Initial-patch-by: Philipp Gortan
> > > Signed-off-by: Johannes Schindelin
> > > ---
> > > git-gui.sh | 6 +-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/git-gui.sh b/git-gui.sh
> > > index fd476b6999..b2c6e7a1db 100755
> > > --- a/git-gui.sh
> > > +++ b/git-gui.sh
> > > @@ -623,7 +623,11 @@ proc git_write {args} {
> > > }
> > >
> > > proc githook_read {hook_name args} {
> > > - set pchook [gitdir hooks $hook_name]
> > > + if {[package vcompare $::_git_version 2.5.0] >= 0} {
> > > + set pchook [git rev-parse --git-path "hooks/$hook_name"]
> > > + } else {
> > > + set pchook [gitdir hooks $hook_name]
> > > + }
> >
> > gitdir is used in a lot of places, and I think all those would also
> > benefit from using --git-path. So I think it is a better idea to move
> > this to the procedure gitdir. It would have to be refactored to take any
> > number of arguments, instead of the two it takes here.
>
> gitdir already takes an arbitrary number of arguments and joins them
> to a path. The more imminent challenge is, that gitdir caches the
> GIT_DIR, thus it tries to avoid calling "git rev-parse". Which works
> for most, but not for hooks.
What I was thinking of was something like this:
- If no args are passed, then just directly return $_gitdir. This is
already being done. I assume the GIT_DIR relocation is already
handled by `git rev-parse --git-dir`, so this would point to the
correct location.
- If args are passed, then we want a subdirectory of GIT_DIR In this
case, it is possible that this subdirectory has also been relocated
(hooks/ being one of those subdirectories). So in this case, use
`git rev-parse --git-path` instead.
So the gitdir procedure would look something like:
proc gitdir {args} {
global $_gitdir
if {$args eq {}} {
# Return the cached GIT_DIR
return $_gitdir
}
# Use `git rev-parse --git-path` to get the path instead of
# using the cached value.
}
Am I missing something? Or does this fix the issue you describe?
> We could either maintain a blacklist, for what we cache the result
> too, or always call "git rev-parse --git-dir".
>
> This blacklist would need to be in sync with the one in Git's
> path.c::adjust_git_path() than.
Is caching GIT_DIR that important in terms of performance? Otherwise,
I'd say calling `git rev-parse --git-path` for _every_ subdirectory of
GIT_DIR is a much simpler solution.
--
Regards,
Pratyush Yadav
Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks
On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav wrote:
>
> Hi,
>
> On 26/09/19 02:17PM, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin
> >
> > Since v2.9.0, Git knows about the config variable core.hookspath
> > that allows overriding the path to the directory containing the
> > Git hooks.
> >
> > Since v2.10.0, the `--git-path` option respects that config
> > variable, too, so we may just as well use that command.
> >
> > For Git versions older than v2.5.0 (which was the first version to
> > support the `--git-path` option for the `rev-parse` command), we
> > simply fall back to the previous code.
> >
> > This fixes https://github.com/git-for-windows/git/issues/1755
> >
> > Initial-patch-by: Philipp Gortan
> > Signed-off-by: Johannes Schindelin
> > ---
> > git-gui.sh | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/git-gui.sh b/git-gui.sh
> > index fd476b6999..b2c6e7a1db 100755
> > --- a/git-gui.sh
> > +++ b/git-gui.sh
> > @@ -623,7 +623,11 @@ proc git_write {args} {
> > }
> >
> > proc githook_read {hook_name args} {
> > - set pchook [gitdir hooks $hook_name]
> > + if {[package vcompare $::_git_version 2.5.0] >= 0} {
> > + set pchook [git rev-parse --git-path "hooks/$hook_name"]
> > + } else {
> > + set pchook [gitdir hooks $hook_name]
> > + }
>
> gitdir is used in a lot of places, and I think all those would also
> benefit from using --git-path. So I think it is a better idea to move
> this to the procedure gitdir. It would have to be refactored to take any
> number of arguments, instead of the two it takes here.
gitdir already takes an arbitrary number of arguments and joins them
to a path. The more imminent challenge is, that gitdir caches the
GIT_DIR, thus it tries to avoid calling "git rev-parse". Which works
for most, but not for hooks.
We could either maintain a blacklist, for what we cache the result
too, or always call "git rev-parse --git-dir".
This blacklist would need to be in sync with the one in Git's
path.c::adjust_git_path() than.
Bert
>
> Other than that, looks good. Thanks.
>
> --
> Regards,
> Pratyush Yadav
Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks
Hi,
On 26/09/19 02:17PM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin
>
> Since v2.9.0, Git knows about the config variable core.hookspath
> that allows overriding the path to the directory containing the
> Git hooks.
>
> Since v2.10.0, the `--git-path` option respects that config
> variable, too, so we may just as well use that command.
>
> For Git versions older than v2.5.0 (which was the first version to
> support the `--git-path` option for the `rev-parse` command), we
> simply fall back to the previous code.
>
> This fixes https://github.com/git-for-windows/git/issues/1755
>
> Initial-patch-by: Philipp Gortan
> Signed-off-by: Johannes Schindelin
> ---
> git-gui.sh | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index fd476b6999..b2c6e7a1db 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -623,7 +623,11 @@ proc git_write {args} {
> }
>
> proc githook_read {hook_name args} {
> - set pchook [gitdir hooks $hook_name]
> + if {[package vcompare $::_git_version 2.5.0] >= 0} {
> + set pchook [git rev-parse --git-path "hooks/$hook_name"]
> + } else {
> + set pchook [gitdir hooks $hook_name]
> + }
gitdir is used in a lot of places, and I think all those would also
benefit from using --git-path. So I think it is a better idea to move
this to the procedure gitdir. It would have to be refactored to take any
number of arguments, instead of the two it takes here.
Other than that, looks good. Thanks.
--
Regards,
Pratyush Yadav
