git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Pratyush Yadav <me@yadavpratyush.com>
Cc: Bert Wesarg <bert.wesarg@googlemail.com>,
	Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 1/1] respect core.hooksPath, falling back to .git/hooks
Date: Tue, 1 Oct 2019 19:38:36 +0200 (CEST)
Message-ID: <nycvar.QRO.7.76.6.1910011638580.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20191001133112.3qh4aeoctkoz2wxy@yadavpratyush.com>

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

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-26 21:17 [PATCH 0/1] git-gui: " Johannes Schindelin via GitGitGadget
2019-09-26 21:17 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget
2019-09-26 22:36   ` Pratyush Yadav
2019-09-27  6:10     ` Bert Wesarg
2019-09-27 13:05       ` Pratyush Yadav
2019-09-30  9:42         ` Johannes Schindelin
2019-10-01 13:31           ` Pratyush Yadav
2019-10-01 17:38             ` Johannes Schindelin [this message]
2019-10-04 16:48               ` Pratyush Yadav
2019-10-04 19:56                 ` Johannes Schindelin
2019-09-30  9:45 ` [PATCH v2 0/1] git-gui: " Johannes Schindelin via GitGitGadget
2019-09-30  9:45   ` [PATCH v2 1/1] " Johannes Schindelin via GitGitGadget
2019-10-04 21:41   ` [PATCH v3 0/1] git-gui: " Johannes Schindelin via GitGitGadget
2019-10-04 21:41     ` [PATCH v3 1/1] Fix gitdir e.g. to respect core.hooksPath Johannes Schindelin via GitGitGadget
2019-10-08  0:29       ` Pratyush Yadav
2019-10-08 11:30         ` Johannes Schindelin
2019-10-08 11:33     ` [PATCH v4 0/1] git-gui: respect core.hooksPath, falling back to .git/hooks Johannes Schindelin via GitGitGadget
2019-10-08 11:33       ` [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc Johannes Schindelin via GitGitGadget
2019-10-11 22:26         ` Pratyush Yadav
2019-10-12 21:24           ` Johannes Schindelin
2019-10-13 18:55             ` Pratyush Yadav
2019-10-13 22:18               ` Johannes Schindelin
2019-10-17 18:34                 ` Pratyush Yadav
2019-10-14  8:14               ` Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.1910011638580.46@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=bert.wesarg@googlemail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=me@yadavpratyush.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git