Skip to content

git-gui: respect core.hooksPath, falling back to .git/hooks #361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: git-gui/master
Choose a base branch
from
Open

git-gui: respect core.hooksPath, falling back to .git/hooks #361

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 58 additions & 4 deletions git-gui.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ if {[tk windowingsystem] eq "aqua"} {

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Pratyush Yadav wrote (reply to this):

Hi Johannes,

Thanks for the re-roll. Some comments below...

On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> 
> 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.
> 
> Other paths inside `.git` are equally subject to differ from
> `<gitdir>/<path>`, i.e. inside worktrees, where _some_ paths live in the
> "gitdir" and some live in the "commondir" (i.e. the "gitdir" of the main
> worktree).
> 
> 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.
> 
> An original patch handled only the hooksPath setting, however, during
> the code submission it was deemed better to fix all call to the `gitdir`
> function.
> 
> To avoid spawning a gazillion `git rev-parse --git-path` instances, we
> cache the returned paths, priming the cache upon startup in a single
> `git rev-parse invocation` with some paths (that have been
> determined via a typical startup and via grepping the source code for
> calls to the `gitdir` function).
> 
> This fixes https://github.com/git-for-windows/git/issues/1755
> 
> Initial-patch-by: Philipp Gortan <philipp@gortan.org>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  git-gui.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index fd476b6999..c684dc7ae1 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_cache {}
>  set _gitworktree {}
>  set _isbare {}
>  set _gitexec {}
> @@ -197,12 +198,59 @@ proc appname {} {
>  	return $_appname
>  }
>  
> +proc prime_gitdir_cache {} {
> +	global _gitdir _gitdir_cache
> +
> +	set gitdir_cmd [list git rev-parse --git-dir]
> +
> +	# `--git-path` is only supported since Git v2.5.0
> +	if {[package vcompare $::_git_version 2.5.0] >= 0} {
> +		# This list was generated from a typical startup as well as from
> +		# grepping through Git GUI's source code.
> +		set gitdir_keys [list \
> +			CHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \
> +			GITGUI_MSG HEAD hooks hooks/prepare-commit-msg \
> +			index.lock info info/exclude logs MERGE_HEAD MERGE_MSG \
> +			MERGE_RR objects "objects/4\[0-1\]/*" \
> +			"objects/4\[0-3\]/*" objects/info \
> +			objects/info/alternates objects/pack packed-refs \
> +			PREPARE_COMMIT_MSG rebase-merge/head-name remotes \
> +			rr-cache rr-cache/MERGE_RR SQUASH_MSG \
> +		]
> +
> +		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_cache([lindex $gitdir_keys $i]) $path
> +		}
> +		incr i
> +	}
> +}
> +
>  proc gitdir {args} {
> -	global _gitdir
> +	global _gitdir _gitdir_cache
> +
>  	if {$args eq {}} {
>  		return $_gitdir
>  	}
> -	return [eval [list file join $_gitdir] $args]
> +
> +	set args [eval [list file join] $args]
> +	if {![info exists _gitdir_cache($args)]} {
> +		if {[package vcompare $::_git_version 2.5.0] >= 0} {
> +			set _gitdir_cache($args) [git rev-parse --git-path $args]
> +		} else {
> +			set _gitdir_cache($args) [file join $_gitdir $args]
> +		}
> +	}
> +
> +	return $_gitdir_cache($args)
>  }
>  
>  proc gitexec {args} {
> @@ -1242,7 +1290,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]
> +		prime_gitdir_cache
>  		set _prefix [git rev-parse --show-prefix]
>  	} err]} {
>  	load_config 1

Looks good till here.

> @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {
>  	global HEAD PARENT MERGE_HEAD commit_type
>  	global ui_index ui_workdir ui_comm
>  	global rescan_active file_states
> -	global repo_config
> +	global repo_config _gitdir_cache
>  
>  	if {$rescan_active > 0 || ![lock_index read]} return
>  
> +	# Only re-prime gitdir cache on a full rescan
> +	if {$after ne "ui_ready"} {

What do you mean by a "full rescan"? I assume you use it as the 
differentiator between `ui_do_rescan` (called when you hit F5 or choose 
rescan from the menu) and `do_rescan` (called when you revert a line or 
hunk), and a "full rescan" refers to `ui_do_rescan`.

Well in that case, this check is incorrect. `do_rescan` passes only 
"ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready".

But either way, I'm not a big fan of this. This check makes assumptions 
about the behaviour of its callers based on what they pass to $after. 
The way I see it, $after should be a black box to `rescan`, and it 
should make absolutely no assumptions about it.

Doing it this way is really brittle, and would break as soon as someone 
changes the behaviour of `ui_do_rescan`. If someone in the future passes 
a different value in $after, this would stop working as intended and 
would not refresh the cached list on a rescan.

So, I think a better place for this if statement would be in 
`ui_do_rescan`. This would mean adding a new function that does this. 
But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not 
to), we can get away with just something like:

  proc ui_do_rescan {} {
  	rescan {prime_gitdir_cache; ui_ready}
  }

Though since `prime_gitdir_cache` does not really depend on the rescan 
being finished, something like this would also work fine:

  proc ui_do_rescan {} {
  	rescan ui_ready
  	prime_gitdir_cache
  }

This would allow us to do these two things in parallel since `rescan` is 
asynchronous. But that would also mean it is possible that the status 
bar would show "Ready" while `prime_gitdir_cache` is still executing.

I can't really make up my mind on what is better. I'm inclining on using 
the latter way, effectively trading a bit of UI inconsistency for 
performance (at least in theory).

Thoughts?

> +		array unset _gitdir_cache
> +		prime_gitdir_cache
> +	}
> +
>  	repository_state newType newHEAD newMERGE_HEAD
>  	if {[string match amend* $commit_type]
>  		&& $newType eq {normal}

-- 
Regards,
Pratyush Yadav

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Pratyush,

On Sat, 12 Oct 2019, Pratyush Yadav wrote:

> On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:
>
> > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {
> >  	global HEAD PARENT MERGE_HEAD commit_type
> >  	global ui_index ui_workdir ui_comm
> >  	global rescan_active file_states
> > -	global repo_config
> > +	global repo_config _gitdir_cache
> >
> >  	if {$rescan_active > 0 || ![lock_index read]} return
> >
> > +	# Only re-prime gitdir cache on a full rescan
> > +	if {$after ne "ui_ready"} {
>
> What do you mean by a "full rescan"? I assume you use it as the
> differentiator between `ui_do_rescan` (called when you hit F5 or choose
> rescan from the menu) and `do_rescan` (called when you revert a line or
> hunk), and a "full rescan" refers to `ui_do_rescan`.
>
> Well in that case, this check is incorrect. `do_rescan` passes only
> "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready".
>
> But either way, I'm not a big fan of this. This check makes assumptions
> about the behaviour of its callers based on what they pass to $after.
> The way I see it, $after should be a black box to `rescan`, and it
> should make absolutely no assumptions about it.
>
> Doing it this way is really brittle, and would break as soon as someone
> changes the behaviour of `ui_do_rescan`. If someone in the future passes
> a different value in $after, this would stop working as intended and
> would not refresh the cached list on a rescan.
>
> So, I think a better place for this if statement would be in
> `ui_do_rescan`. This would mean adding a new function that does this.
> But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not
> to), we can get away with just something like:
>
>   proc ui_do_rescan {} {
>   	rescan {prime_gitdir_cache; ui_ready}
>   }
>
> Though since `prime_gitdir_cache` does not really depend on the rescan
> being finished, something like this would also work fine:
>
>   proc ui_do_rescan {} {
>   	rescan ui_ready
>   	prime_gitdir_cache
>   }

That was my first attempt. However, there is a very important piece of
code that is even still quoted above: that `if {$rescan_active > 0 ||
![lock_index read]} return` part.

I do _not_ want to interfere with an actively-going-on rescan. If there
is an active one, I don't want to re-prime the `_gitdir` cache.

That was the reason why I put the additional code into `rescan` rather
than into `ui_do_rescan()`.

Ciao,
Johannes

>
> This would allow us to do these two things in parallel since `rescan` is
> asynchronous. But that would also mean it is possible that the status
> bar would show "Ready" while `prime_gitdir_cache` is still executing.
>
> I can't really make up my mind on what is better. I'm inclining on using
> the latter way, effectively trading a bit of UI inconsistency for
> performance (at least in theory).
>
> Thoughts?
>
> > +		array unset _gitdir_cache
> > +		prime_gitdir_cache
> > +	}
> > +
> >  	repository_state newType newHEAD newMERGE_HEAD
> >  	if {[string match amend* $commit_type]
> >  		&& $newType eq {normal}
>
> --
> Regards,
> Pratyush Yadav
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Pratyush Yadav wrote (reply to this):

On 12/10/19 11:24PM, Johannes Schindelin wrote:
> Hi Pratyush,
> 
> On Sat, 12 Oct 2019, Pratyush Yadav wrote:
> 
> > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:
> >
> > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {
> > >  	global HEAD PARENT MERGE_HEAD commit_type
> > >  	global ui_index ui_workdir ui_comm
> > >  	global rescan_active file_states
> > > -	global repo_config
> > > +	global repo_config _gitdir_cache
> > >
> > >  	if {$rescan_active > 0 || ![lock_index read]} return
> > >
> > > +	# Only re-prime gitdir cache on a full rescan
> > > +	if {$after ne "ui_ready"} {
> >
> > What do you mean by a "full rescan"? I assume you use it as the
> > differentiator between `ui_do_rescan` (called when you hit F5 or choose
> > rescan from the menu) and `do_rescan` (called when you revert a line or
> > hunk), and a "full rescan" refers to `ui_do_rescan`.
> >
> > Well in that case, this check is incorrect. `do_rescan` passes only
> > "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready".
> >
> > But either way, I'm not a big fan of this. This check makes assumptions
> > about the behaviour of its callers based on what they pass to $after.
> > The way I see it, $after should be a black box to `rescan`, and it
> > should make absolutely no assumptions about it.
> >
> > Doing it this way is really brittle, and would break as soon as someone
> > changes the behaviour of `ui_do_rescan`. If someone in the future passes
> > a different value in $after, this would stop working as intended and
> > would not refresh the cached list on a rescan.
> >
> > So, I think a better place for this if statement would be in
> > `ui_do_rescan`. This would mean adding a new function that does this.
> > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not
> > to), we can get away with just something like:
> >
> >   proc ui_do_rescan {} {
> >   	rescan {prime_gitdir_cache; ui_ready}
> >   }
> >
> > Though since `prime_gitdir_cache` does not really depend on the rescan
> > being finished, something like this would also work fine:
> >
> >   proc ui_do_rescan {} {
> >   	rescan ui_ready
> >   	prime_gitdir_cache
> >   }
> 
> That was my first attempt. However, there is a very important piece of
> code that is even still quoted above: that `if {$rescan_active > 0 ||
> ![lock_index read]} return` part.
> 
> I do _not_ want to interfere with an actively-going-on rescan. If there
> is an active one, I don't want to re-prime the `_gitdir` cache.

Good catch! In that case I suppose refreshing the cache in $after would 
be the way to go (IOW, the former of my two suggestions). Anything 
$after won't get executed if we return early from that check.
 
> That was the reason why I put the additional code into `rescan` rather
> than into `ui_do_rescan()`.
> 
> Ciao,
> Johannes
> 
> >
> > This would allow us to do these two things in parallel since `rescan` is
> > asynchronous. But that would also mean it is possible that the status
> > bar would show "Ready" while `prime_gitdir_cache` is still executing.
> >
> > I can't really make up my mind on what is better. I'm inclining on using
> > the latter way, effectively trading a bit of UI inconsistency for
> > performance (at least in theory).
> >
> > Thoughts?
> >
> > > +		array unset _gitdir_cache
> > > +		prime_gitdir_cache
> > > +	}
> > > +
> > >  	repository_state newType newHEAD newMERGE_HEAD
> > >  	if {[string match amend* $commit_type]
> > >  		&& $newType eq {normal}

-- 
Regards,
Pratyush Yadav

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Pratyush,

On Mon, 14 Oct 2019, Pratyush Yadav wrote:

> On 12/10/19 11:24PM, Johannes Schindelin wrote:
> > Hi Pratyush,
> >
> > On Sat, 12 Oct 2019, Pratyush Yadav wrote:
> >
> > > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:
> > >
> > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {
> > > >  	global HEAD PARENT MERGE_HEAD commit_type
> > > >  	global ui_index ui_workdir ui_comm
> > > >  	global rescan_active file_states
> > > > -	global repo_config
> > > > +	global repo_config _gitdir_cache
> > > >
> > > >  	if {$rescan_active > 0 || ![lock_index read]} return
> > > >
> > > > +	# Only re-prime gitdir cache on a full rescan
> > > > +	if {$after ne "ui_ready"} {
> > >
> > > What do you mean by a "full rescan"? I assume you use it as the
> > > differentiator between `ui_do_rescan` (called when you hit F5 or cho=
ose
> > > rescan from the menu) and `do_rescan` (called when you revert a line=
 or
> > > hunk), and a "full rescan" refers to `ui_do_rescan`.
> > >
> > > Well in that case, this check is incorrect. `do_rescan` passes only
> > > "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready".
> > >
> > > But either way, I'm not a big fan of this. This check makes assumpti=
ons
> > > about the behaviour of its callers based on what they pass to $after=
.
> > > The way I see it, $after should be a black box to `rescan`, and it
> > > should make absolutely no assumptions about it.
> > >
> > > Doing it this way is really brittle, and would break as soon as some=
one
> > > changes the behaviour of `ui_do_rescan`. If someone in the future pa=
sses
> > > a different value in $after, this would stop working as intended and
> > > would not refresh the cached list on a rescan.
> > >
> > > So, I think a better place for this if statement would be in
> > > `ui_do_rescan`. This would mean adding a new function that does this=
.
> > > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason=
 not
> > > to), we can get away with just something like:
> > >
> > >   proc ui_do_rescan {} {
> > >   	rescan {prime_gitdir_cache; ui_ready}
> > >   }
> > >
> > > Though since `prime_gitdir_cache` does not really depend on the resc=
an
> > > being finished, something like this would also work fine:
> > >
> > >   proc ui_do_rescan {} {
> > >   	rescan ui_ready
> > >   	prime_gitdir_cache
> > >   }
> >
> > That was my first attempt. However, there is a very important piece of
> > code that is even still quoted above: that `if {$rescan_active > 0 ||
> > ![lock_index read]} return` part.
> >
> > I do _not_ want to interfere with an actively-going-on rescan. If ther=
e
> > is an active one, I don't want to re-prime the `_gitdir` cache.
>
> Good catch! In that case I suppose refreshing the cache in $after would
> be the way to go (IOW, the former of my two suggestions). Anything
> $after won't get executed if we return early from that check.

The obvious problem with that solution is that the `_gitdir` is reset
_after_ the rescan. In my solution, it is reset _before_, as I have no
idea how often the `_gitdir` values are used during a rescan (and if
none of they were, I would like to be very cautious to prepare for a
future where any of those `_gitdir` values _are_ used during a rescan).

So I am afraid that I find way too serious problems with both of your
proposed alternatives.

Ciao,
Johannes

>
> > That was the reason why I put the additional code into `rescan` rather
> > than into `ui_do_rescan()`.
> >
> > Ciao,
> > Johannes
> >
> > >
> > > This would allow us to do these two things in parallel since `rescan=
` is
> > > asynchronous. But that would also mean it is possible that the statu=
s
> > > bar would show "Ready" while `prime_gitdir_cache` is still executing=
.
> > >
> > > I can't really make up my mind on what is better. I'm inclining on u=
sing
> > > the latter way, effectively trading a bit of UI inconsistency for
> > > performance (at least in theory).
> > >
> > > Thoughts?
> > >
> > > > +		array unset _gitdir_cache
> > > > +		prime_gitdir_cache
> > > > +	}
> > > > +
> > > >  	repository_state newType newHEAD newMERGE_HEAD
> > > >  	if {[string match amend* $commit_type]
> > > >  		&& $newType eq {normal}
>
> --
> Regards,
> Pratyush Yadav
>
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Pratyush,

On Mon, 14 Oct 2019, Pratyush Yadav wrote:

> On 12/10/19 11:24PM, Johannes Schindelin wrote:
> >
> > On Sat, 12 Oct 2019, Pratyush Yadav wrote:
> >
> > > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:
> > >
> > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {
> > > >  	global HEAD PARENT MERGE_HEAD commit_type
> > > >  	global ui_index ui_workdir ui_comm
> > > >  	global rescan_active file_states
> > > > -	global repo_config
> > > > +	global repo_config _gitdir_cache
> > > >
> > > >  	if {$rescan_active > 0 || ![lock_index read]} return
> > > >
> > > > +	# Only re-prime gitdir cache on a full rescan
> > > > +	if {$after ne "ui_ready"} {
> > >
> > > What do you mean by a "full rescan"? I assume you use it as the
> > > differentiator between `ui_do_rescan` (called when you hit F5 or cho=
ose
> > > rescan from the menu) and `do_rescan` (called when you revert a line=
 or
> > > hunk), and a "full rescan" refers to `ui_do_rescan`.
> > >
> > > Well in that case, this check is incorrect. `do_rescan` passes only
> > > "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready".
> > >
> > > But either way, I'm not a big fan of this. This check makes assumpti=
ons
> > > about the behaviour of its callers based on what they pass to $after=
.
> > > The way I see it, $after should be a black box to `rescan`, and it
> > > should make absolutely no assumptions about it.
> > >
> > > Doing it this way is really brittle, and would break as soon as some=
one
> > > changes the behaviour of `ui_do_rescan`. If someone in the future pa=
sses
> > > a different value in $after, this would stop working as intended and
> > > would not refresh the cached list on a rescan.
> > >
> > > So, I think a better place for this if statement would be in
> > > `ui_do_rescan`. This would mean adding a new function that does this=
.
> > > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason=
 not
> > > to), we can get away with just something like:
> > >
> > >   proc ui_do_rescan {} {
> > >   	rescan {prime_gitdir_cache; ui_ready}
> > >   }
> > >
> > > Though since `prime_gitdir_cache` does not really depend on the resc=
an
> > > being finished, something like this would also work fine:
> > >
> > >   proc ui_do_rescan {} {
> > >   	rescan ui_ready
> > >   	prime_gitdir_cache
> > >   }
> >
> > That was my first attempt. However, there is a very important piece of
> > code that is even still quoted above: that `if {$rescan_active > 0 ||
> > ![lock_index read]} return` part.
> >
> > I do _not_ want to interfere with an actively-going-on rescan. If ther=
e
> > is an active one, I don't want to re-prime the `_gitdir` cache.
>
> Good catch! In that case I suppose refreshing the cache in $after would
> be the way to go (IOW, the former of my two suggestions). Anything
> $after won't get executed if we return early from that check.

But it also won't get executed before the actual rescan. Which is
precisely what I tried to ensure.

Ciao,
Johannes

>
> > That was the reason why I put the additional code into `rescan` rather
> > than into `ui_do_rescan()`.
> >
> > Ciao,
> > Johannes
> >
> > >
> > > This would allow us to do these two things in parallel since `rescan=
` is
> > > asynchronous. But that would also mean it is possible that the statu=
s
> > > bar would show "Ready" while `prime_gitdir_cache` is still executing=
.
> > >
> > > I can't really make up my mind on what is better. I'm inclining on u=
sing
> > > the latter way, effectively trading a bit of UI inconsistency for
> > > performance (at least in theory).
> > >
> > > Thoughts?
> > >
> > > > +		array unset _gitdir_cache
> > > > +		prime_gitdir_cache
> > > > +	}
> > > > +
> > > >  	repository_state newType newHEAD newMERGE_HEAD
> > > >  	if {[string match amend* $commit_type]
> > > >  		&& $newType eq {normal}
>
> --
> Regards,
> Pratyush Yadav
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Pratyush Yadav wrote (reply to this):

On 14/10/19 12:18AM, Johannes Schindelin wrote:
> Hi Pratyush,
> 
> On Mon, 14 Oct 2019, Pratyush Yadav wrote:
> 
> > On 12/10/19 11:24PM, Johannes Schindelin wrote:
> > > Hi Pratyush,
> > >
> > > On Sat, 12 Oct 2019, Pratyush Yadav wrote:
> > >
> > > > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:
> > > >
> > > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {
> > > > >  	global HEAD PARENT MERGE_HEAD commit_type
> > > > >  	global ui_index ui_workdir ui_comm
> > > > >  	global rescan_active file_states
> > > > > -	global repo_config
> > > > > +	global repo_config _gitdir_cache
> > > > >
> > > > >  	if {$rescan_active > 0 || ![lock_index read]} return
> > > > >
> > > > > +	# Only re-prime gitdir cache on a full rescan
> > > > > +	if {$after ne "ui_ready"} {
> > > >
> > > > What do you mean by a "full rescan"? I assume you use it as the
> > > > differentiator between `ui_do_rescan` (called when you hit F5 or choose
> > > > rescan from the menu) and `do_rescan` (called when you revert a line or
> > > > hunk), and a "full rescan" refers to `ui_do_rescan`.
> > > >
> > > > Well in that case, this check is incorrect. `do_rescan` passes only
> > > > "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready".
> > > >
> > > > But either way, I'm not a big fan of this. This check makes assumptions
> > > > about the behaviour of its callers based on what they pass to $after.
> > > > The way I see it, $after should be a black box to `rescan`, and it
> > > > should make absolutely no assumptions about it.
> > > >
> > > > Doing it this way is really brittle, and would break as soon as someone
> > > > changes the behaviour of `ui_do_rescan`. If someone in the future passes
> > > > a different value in $after, this would stop working as intended and
> > > > would not refresh the cached list on a rescan.
> > > >
> > > > So, I think a better place for this if statement would be in
> > > > `ui_do_rescan`. This would mean adding a new function that does this.
> > > > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not
> > > > to), we can get away with just something like:
> > > >
> > > >   proc ui_do_rescan {} {
> > > >   	rescan {prime_gitdir_cache; ui_ready}
> > > >   }
> > > >
> > > > Though since `prime_gitdir_cache` does not really depend on the rescan
> > > > being finished, something like this would also work fine:
> > > >
> > > >   proc ui_do_rescan {} {
> > > >   	rescan ui_ready
> > > >   	prime_gitdir_cache
> > > >   }
> > >
> > > That was my first attempt. However, there is a very important piece of
> > > code that is even still quoted above: that `if {$rescan_active > 0 ||
> > > ![lock_index read]} return` part.
> > >
> > > I do _not_ want to interfere with an actively-going-on rescan. If there
> > > is an active one, I don't want to re-prime the `_gitdir` cache.
> >
> > Good catch! In that case I suppose refreshing the cache in $after would
> > be the way to go (IOW, the former of my two suggestions). Anything
> > $after won't get executed if we return early from that check.
> 
> The obvious problem with that solution is that the `_gitdir` is reset
> _after_ the rescan. In my solution, it is reset _before_, as I have no
> idea how often the `_gitdir` values are used during a rescan (and if
> none of they were, I would like to be very cautious to prepare for a
> future where any of those `_gitdir` values _are_ used during a rescan).

_gitdir values are used quite often during a rescan, so yes, this won't 
work.
 
> So I am afraid that I find way too serious problems with both of your
> proposed alternatives.

One alternative I can see right now is adding another optional parameter 
to `rescan` that controls whether we refresh the gitdir cache or not. 
That parameter would default to 0/false. I'm not the biggest fan of 
something like this, but it might be the easiest way to do it given the 
constraints.

I also thought about trying to acquire the index lock in 
`prime_gitdir_cache`, but that could create a race on the lock between 
`prime_gitdir_cache` and `rescan`.

If you have any better ideas, I'm all ears, but otherwise, I maybe our 
best bet is to just go with adding an optional parameter to `rescan`.

-- 
Regards,
Pratyush Yadav

set _appname {Git Gui}
set _gitdir {}
array set _gitdir_cache {}
set _gitworktree {}
set _isbare {}
set _gitexec {}
Expand Down Expand Up @@ -197,12 +198,59 @@ proc appname {} {
return $_appname
}

proc prime_gitdir_cache {} {
global _gitdir _gitdir_cache

set gitdir_cmd [list git rev-parse --git-dir]

# `--git-path` is only supported since Git v2.5.0
if {[package vcompare $::_git_version 2.5.0] >= 0} {
# This list was generated from a typical startup as well as from
# grepping through Git GUI's source code.
set gitdir_keys [list \
CHERRY_PICK_HEAD FETCH_HEAD GITGUI_BCK GITGUI_EDITMSG \
GITGUI_MSG HEAD hooks hooks/prepare-commit-msg \
index.lock info info/exclude logs MERGE_HEAD MERGE_MSG \
MERGE_RR objects "objects/4\[0-1\]/*" \
"objects/4\[0-3\]/*" objects/info \
objects/info/alternates objects/pack packed-refs \
PREPARE_COMMIT_MSG rebase-merge/head-name remotes \
rr-cache rr-cache/MERGE_RR SQUASH_MSG \
]

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_cache([lindex $gitdir_keys $i]) $path
}
incr i
}
}

proc gitdir {args} {
global _gitdir
global _gitdir _gitdir_cache

if {$args eq {}} {
return $_gitdir
}
return [eval [list file join $_gitdir] $args]

set args [eval [list file join] $args]
if {![info exists _gitdir_cache($args)]} {
if {[package vcompare $::_git_version 2.5.0] >= 0} {
set _gitdir_cache($args) [git rev-parse --git-path $args]
} else {
set _gitdir_cache($args) [file join $_gitdir $args]
}
}

return $_gitdir_cache($args)
}

proc gitexec {args} {
Expand Down Expand Up @@ -1242,7 +1290,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]
prime_gitdir_cache
set _prefix [git rev-parse --show-prefix]
} err]} {
load_config 1
Expand Down Expand Up @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {
global HEAD PARENT MERGE_HEAD commit_type
global ui_index ui_workdir ui_comm
global rescan_active file_states
global repo_config
global repo_config _gitdir_cache

if {$rescan_active > 0 || ![lock_index read]} return

# Only re-prime gitdir cache on a full rescan
if {$after ne "ui_ready"} {
array unset _gitdir_cache
prime_gitdir_cache
}

repository_state newType newHEAD newMERGE_HEAD
if {[string match amend* $commit_type]
&& $newType eq {normal}
Expand Down