Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks

[Pratyush Yadav <me@yadavpratyush.com>]

On 30/09/19 11:42AM, Johannes Schindelin wrote:
> On Fri, 27 Sep 2019, Pratyush Yadav wrote:
> > On 27/09/19 08:10AM, Bert Wesarg wrote:
> > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > > > gitdir is used in a lot of places, and I think all those would 
> > > > also
> > > > benefit from using --git-path. So I think it is a better idea to move
> > > > this to the procedure gitdir. It would have to be refactored to take any
> > > > number of arguments, instead of the two it takes here.
> 
> The `gitdir` function is called 13 times during startup alone, and who
> knows how many more times later.
> 
> So I am quite convinced that the original intention was to save on
> spawning processes left and right.
> 
> But since you are the Git GUI maintainer, and this was your suggestion,
> I made it so.

Yes, I am the maintainer, but I am not an all-knowing, all-seeing 
entity. Your, and every other contributors, suggestions are very 
valuable. And my suggestions aren't gospel. I would hate to see someone 
send in a patch they weren't sure was the best thing to do just because 
I suggested it. Please feel free to object my suggestions.

In this case, I didn't expect gitdir to be called this many times.

While I don't notice much of a performance difference on my system 
(Linux), a quick measurement tells me that the time spent in gitdir is 
about 16 ms. In contrast, the same measurement without the v2 patch 
gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something 
so simple. It might not be the same for everyone else. AFAIK, spawning a 
process is much slower on Windows.

So now I'm not so sure my suggestion was a good one. My original aim was 
to be sure everything was correct, and no incorrect directories were 
being used. But the current solution comes at a performance hit.

> > > We could either maintain a blacklist, for what we cache the result
> > > too, or always call "git rev-parse --git-dir".
> > >
> > > This blacklist would need to be in sync with the one in Git's
> > > path.c::adjust_git_path() than.

Bert's suggestion seems like a decent compromise. We run `git rev-parse 
--git-path` for the paths in the blacklist, and for the rest we use the 
cached value. This does run the risk of getting out of sync with 
git.git's list, but it might be better than spawing a process every 
time, and is very likely better than just doing it for hooks.

Thoughts?

-- 
Regards,
Pratyush Yadav