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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Tue, 1 Oct 2019, Pratyush Yadav wrote:

> On 30/09/19 11:42AM, Johannes Schindelin wrote:
> > On Fri, 27 Sep 2019, Pratyush Yadav wrote:
> > > On 27/09/19 08:10AM, Bert Wesarg wrote:
> > > > On Fri, Sep 27, 2019 at 12:40 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > > > > gitdir is used in a lot of places, and I think all those would
> > > > > also
> > > > > benefit from using --git-path. So I think it is a better idea to move
> > > > > this to the procedure gitdir. It would have to be refactored to take any
> > > > > number of arguments, instead of the two it takes here.
> >
> > The `gitdir` function is called 13 times during startup alone, and who
> > knows how many more times later.
> >
> > So I am quite convinced that the original intention was to save on
> > spawning processes left and right.
> >
> > But since you are the Git GUI maintainer, and this was your suggestion,
> > I made it so.
>
> Yes, I am the maintainer, but I am not an all-knowing, all-seeing
> entity. Your, and every other contributors, suggestions are very
> valuable. And my suggestions aren't gospel. I would hate to see someone
> send in a patch they weren't sure was the best thing to do just because
> I suggested it. Please feel free to object my suggestions.
>
> In this case, I didn't expect gitdir to be called this many times.
>
> While I don't notice much of a performance difference on my system
> (Linux), a quick measurement tells me that the time spent in gitdir is
> about 16 ms. In contrast, the same measurement without the v2 patch
> gives out 0 ms (IOW, very fast). 16 ms sounds a bit much for something
> so simple. It might not be the same for everyone else. AFAIK, spawning a
> process is much slower on Windows.
>
> So now I'm not so sure my suggestion was a good one. My original aim was
> to be sure everything was correct, and no incorrect directories were
> being used. But the current solution comes at a performance hit.
>
> > > > We could either maintain a blacklist, for what we cache the result
> > > > too, or always call "git rev-parse --git-dir".
> > > >
> > > > This blacklist would need to be in sync with the one in Git's
> > > > path.c::adjust_git_path() than.
>
> Bert's suggestion seems like a decent compromise. We run `git rev-parse
> --git-path` for the paths in the blacklist, and for the rest we use the
> cached value. This does run the risk of getting out of sync with
> git.git's list, but it might be better than spawing a process every
> time, and is very likely better than just doing it for hooks.

But what about this part of that function?

-- snip --
else if (repo->different_commondir)
	update_common_dir(buf, git_dir_len, repo->commondir);
-- snap --

It might well turn out that this blacklist is neither easy to implement
nor will it help much.

So let's look at all the call sites:

-- snip --
$ git grep -w gitdir | sed -ne 's|\].*||' -e 's|.*\[gitdir ||p' | sort | uniq
$file
$name
CHERRY_PICK_HEAD
FETCH_HEAD
GITGUI_BCK
GITGUI_EDITMSG
GITGUI_MSG
HEAD
hooks $hook_name
index.lock
info exclude
logs $name
MERGE_HEAD
MERGE_MSG
MERGE_RR
objects 4\[0-[expr {$ndirs-1}
objects info
objects info alternates
objects pack
packed-refs
PREPARE_COMMIT_MSG
rebase-merge head-name
remotes
remotes $r
rr-cache
rr-cache MERGE_RR
SQUASH_MSG
-- snap --

The `$file` call looks for messages (probably commit, merge, tag
messages and the likes), the `$name` one looks for refs.

Some of those arguments strike me as very good candidates to require the
common directory while others require the real gitdir (remember,
commondir != gitdir in worktrees other than the main worktree).

What _could_ be done (but we're certainly threatening to enter the realm
of the ridiculous here) is to call `git rev-parse --git-dir --git-path
CHERRY_PICK_HEAD --git-path FETCH_HEAD [...]`, which will output one
path per line, and then store the result in an associative array
(https://tcl.tk/man/tcl8.5/tutorial/Tcl22.html), and use that to look up
paths based on their first component, caching as we go.

Something like this:

-- snipsnap --
diff --git a/git-gui.sh b/git-gui.sh
index fd476b6..9295c75 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -158,6 +158,7 @@ if {[tk windowingsystem] eq "aqua"} {

 set _appname {Git Gui}
 set _gitdir {}
+array set _gitdir_cached {}
 set _gitworktree {}
 set _isbare {}
 set _gitexec {}
@@ -197,12 +198,50 @@ proc appname {} {
 	return $_appname
 }

+proc init_gitdir_cached {} {
+	global _gitdir _gitdir_cached
+
+	set gitdir_keys [list \
+		CHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \
+		GITGUI_MSG HEAD hooks index.lock info logs MERGE_HEAD \
+		MERGE_MSG MERGE_RR objects packed-refs PREPARE_COMMIT_MSG \
+		rebase-merge head-name remotes rr-cache SQUASH_MSG \
+		]
+
+	set gitdir_cmd [list git rev-parse --git-dir]
+	foreach key $gitdir_keys {
+		lappend gitdir_cmd --git-path $key
+	}
+
+	set i -1
+	foreach path [split [eval $gitdir_cmd] "\n"] {
+		if {$i eq -1} {
+			set _gitdir $path
+		} else {
+			set _gitdir_cached([lindex $gitdir_keys $i]) $path
+		}
+		incr i
+	}
+}
+
 proc gitdir {args} {
-	global _gitdir
+	global _gitdir _gitdir_cached
+
 	if {$args eq {}} {
 		return $_gitdir
 	}
-	return [eval [list file join $_gitdir] $args]
+
+	set arg0 [lindex $args 0]
+	set args [lrange $args 1 end]
+	if {![info exists _gitdir_cached($arg0)]} {
+		if {[package vcompare $::_git_version 2.5.0] >= 0} {
+			set _gitdir_cached($arg0) [git rev-parse --git-path $arg0]
+		} else {
+			set _gitdir_cached($arg0) [file join $_gitdir $arg0]
+		}
+	}
+
+	return [eval [concat [list file join $_gitdir_cached($arg0)] $args]]
 }

 proc gitexec {args} {
@@ -1242,7 +1281,7 @@ if {[catch {
 	&& [catch {
 		# beware that from the .git dir this sets _gitdir to .
 		# and _prefix to the empty string
-		set _gitdir [git rev-parse --git-dir]
+		init_gitdir_cached
 		set _prefix [git rev-parse --show-prefix]
 	} err]} {
 	load_config 1