git-gui ignores core.hooksPath

Hi git devs,

First off, thanks for your awesome work!

I've been unhappy for quite a while that I had to configure the hooks
manually for each of my repos - until I found out recently that there is
the core.hooksPath config variable that (when set globally) allows me to
specify a hooks directory to be used for all my repositories.

Now I was happy - for a few minutes, until I tested this feature in
git-gui, and realized that it doesn't work there.

This seems to be caused by "proc githook_read", which says "set pchook
[gitdir hooks $hook_name]" instead of querying "git config
core.hooksPath" first - cf
https://github.com/git/git/blob/2cc2e70264e0fcba04f9ef791d144bbc8b501206/git-gui/git-gui.sh#L627

Would be great if this could get fixed...

Thanks, Philipp

On Fri, Jun 2, 2017 at 9:41 AM, Philipp Gortan [off-list ref] wrote:

Hi git devs,

First off, thanks for your awesome work!

I've been unhappy for quite a while that I had to configure the hooks
manually for each of my repos - until I found out recently that there is
the core.hooksPath config variable that (when set globally) allows me to
specify a hooks directory to be used for all my repositories.

OT but you may also want to look into using Git templates.

Now I was happy - for a few minutes, until I tested this feature in
git-gui, and realized that it doesn't work there.

This seems to be caused by "proc githook_read", which says "set pchook
[gitdir hooks $hook_name]" instead of querying "git config
core.hooksPath" first - cf
https://github.com/git/git/blob/2cc2e70264e0fcba04f9ef791d144bbc8b501206/git-gui/git-gui.sh#L627

Would be great if this could get fixed...

Thanks, Philipp

On Fri, Jun 2, 2017 at 3:41 PM, Philipp Gortan [off-list ref] wrote:

Hi git devs,

First off, thanks for your awesome work!

I've been unhappy for quite a while that I had to configure the hooks
manually for each of my repos - until I found out recently that there is
the core.hooksPath config variable that (when set globally) allows me to
specify a hooks directory to be used for all my repositories.

Now I was happy - for a few minutes, until I tested this feature in
git-gui, and realized that it doesn't work there.

This seems to be caused by "proc githook_read", which says "set pchook
[gitdir hooks $hook_name]" instead of querying "git config
core.hooksPath" first - cf
https://github.com/git/git/blob/2cc2e70264e0fcba04f9ef791d144bbc8b501206/git-gui/git-gui.sh#L627

Would be great if this could get fixed...

Hi. I added core.hooksPath, glad to see it's useful to other people.

This indeed is something that should be fixed, but git-gui development
is managed outside of git.git, it's just occasionally pulled in. I'm
not what the best place to contact is, but I've CC'd
Philip Oakley who's been making recent commits to git-gui.git at
http://repo.or.cz/git-gui.git/

Signed-off-by: Philipp Gortan <redacted>
---

The following patch tries to fix git-gui to respect the core.hooksPath config
variable, falling back to the old behavior.

git-gui.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/git-gui.sh b/git-gui.sh
index 5bc21b8..a5335b1 100755
--- a/git-gui.sh
+++ b/git-gui.sh

@@ -624,7 +624,10 @@ proc git_write {args} {
 }
 
 proc githook_read {hook_name args} {
-	set pchook [gitdir hooks $hook_name]
+	if {[catch {set hooksdir [git config core.hooksPath]}]} {
+		set hooksdir [gitdir hooks]
+	}
+	set pchook [file join $hooksdir $hook_name]
 	lappend args 2>@1
 
 	# On Windows [file executable] might lie so we need to ask

-- 
2.13.0

Dear Philip,

the previous mail contains a patch against the master of
http://repo.or.cz/git-gui.git

Could you please review it? I am not a TCL developer, so please take
extra care!

Thanks,
Philipp

From: "Ævar Arnfjörð Bjarmason" <redacted>

On Fri, Jun 2, 2017 at 3:41 PM, Philipp Gortan [off-list ref] wrote:

quoted

Hi git devs,

First off, thanks for your awesome work!

I've been unhappy for quite a while that I had to configure the hooks
manually for each of my repos - until I found out recently that there is
the core.hooksPath config variable that (when set globally) allows me to
specify a hooks directory to be used for all my repositories.

Now I was happy - for a few minutes, until I tested this feature in
git-gui, and realized that it doesn't work there.

This seems to be caused by "proc githook_read", which says "set pchook
[gitdir hooks $hook_name]" instead of querying "git config
core.hooksPath" first - cf
https://github.com/git/git/blob/2cc2e70264e0fcba04f9ef791d144bbc8b501206/git-gui/git-gui.sh#L627

Would be great if this could get fixed...

Hi. I added core.hooksPath, glad to see it's useful to other people.

This indeed is something that should be fixed, but git-gui development
is managed outside of git.git, it's just occasionally pulled in. I'm
not what the best place to contact is, but I've CC'd
Philip Oakley who's been making recent commits to git-gui.git at
http://repo.or.cz/git-gui.git/

The proper maintainer for the git-gui is Pat Thoyts (cc'd) who now has a 
repo at https://github.com/patthoyts/git-gui where Pull requests can be 
made, with patches posted here initially for wider review.

I'm just another contributor, but I have managed (with a bit of help from 
google and formative years using FORTH;-) to get a few git-gui patches 
tested and incorporated. tcl/tk isn't that hard, its just interpreted 
code.., the main part, for me, was working out how to ru the code in the 
Git-for-Windows SDK environment (with help).
--
Philip 

Thanks Philip,

I've created a pull request there -
https://github.com/patthoyts/git-gui/pull/12

Hi,

On Fri, 2 Jun 2017, Ævar Arnfjörð Bjarmason wrote:

On Fri, Jun 2, 2017 at 3:41 PM, Philipp Gortan [off-list ref] wrote:

quoted

I've been unhappy for quite a while that I had to configure the hooks
manually for each of my repos - until I found out recently that there
is the core.hooksPath config variable that (when set globally) allows
me to specify a hooks directory to be used for all my repositories.

Now I was happy - for a few minutes, until I tested this feature in
git-gui, and realized that it doesn't work there.

This seems to be caused by "proc githook_read", which says "set pchook
[gitdir hooks $hook_name]" instead of querying "git config
core.hooksPath" first - cf
https://github.com/git/git/blob/2cc2e70264e0fcba04f9ef791d144bbc8b501206/git-gui/git-gui.sh#L627

Would be great if this could get fixed...

Indeed. Why don't you give it a try?

This indeed is something that should be fixed, but git-gui development
is managed outside of git.git, it's just occasionally pulled in. I'm
not what the best place to contact is, but I've CC'd
Philip Oakley who's been making recent commits to git-gui.git at
http://repo.or.cz/git-gui.git/

Philip is in the same spot as I am: we both worked on Git GUI to improve
it for Git for Windows users, but Pat has been silent for over half a year
on all of our PRs.

In the meantime, I managed to get a couple of changes into git.git via
Junio, but the situation is far from ideal.

So what I settled on is to carry a couple of Git GUI patches in Git for
Windows' fork, until the time when the patches finally get accepted into
https://github.com/patthoyts/git-gui.

In this particular instance, the only question is whether to use `git
rev-parse --git-path hooks` or re-roll the core.hookspath logic in
git-gui.

Both approaches have their downsides:

- rev-parse --git-path was broken in subdirectories for a *really* long
  time. Since Git GUI is supposed to be relatively independent from the
  version of the installed git executable, that would imply a couple of
  ugly extra code just to make sure that it works correctly.

- duplicating the core.hookspath logic is prone to become stale over time,
  as Git may change the behavior (as it did with the core.hookspath
  setting).

The subdirectory problem of --git-path is actually not that bad, as Git
GUI cd's to the top-level directory anyway. So that bug does not affect
us. The only caveat is that --git-path was only introduced into v2.5.0,
and Git GUI has conditional code to even support pre-1.6.3 versions.

Happily, pre-1.6.3 versions are not supposed to handle core.hookspath in
the way v2.9.0 and later handle it.

So something like this *may* work:

-- snip --

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 37c1c5d227b..3067a3b000a 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh

@@ -624,7 +624,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]
+	}
 	lappend args 2>@1
 
 	# On Windows [file executable] might lie so we need to ask
-- snap --

Philipp, this is as far as I will go with this. If you truly desire this
to be fixed, please take it from here (read: test, fix and submit with a
good commit message).

Ciao,
Johannes

Hi Philipp,

On Fri, 2 Jun 2017, Philipp Gortan wrote:

Signed-off-by: Philipp Gortan <redacted>

I just saw this. I made sure that the thread to which I just replied did
not have any news from you, but you simply started a new thread ;-)

This commit message needs a little bit of love. Something along the lines:

	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 assumes that you'll go with the approach I outlined in the other
thread, comparing the Git version to 2.5.0 and going with --git-path if
available.)

---

The following patch tries to fix git-gui to respect the core.hooksPath config
variable, falling back to the old behavior.

That would also have been a decent commit message, if a bit short. But you
need to put this text before the `---` line, even before the
`Signed-off-by:` footer.

Did you not mean [get_config core.hookspath] here, i.e. get_config and the
key all lower-case?

+		set hooksdir [gitdir hooks]
+	}
+	set pchook [file join $hooksdir $hook_name]
 	lappend args 2>@1
 

The problem I see with that is, as I mentioned in the other thread, that
it duplicates the logic in config.c that may change at any stage.

Even worse: it is inconsistent with the way Git handles core.hooksPath, if
the installed `git` executable predates v2.9.0. Git GUI explicitly allows
for being used with a large range of Git versions.

In short: I think it would be better to go with the approach I outlined in
the other thread. I'll reproduce the patch (completely untested) here:

-- snipsnap --

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 37c1c5d227b..3067a3b000a 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh

@@ -624,7 +624,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]
+	}
 	lappend args 2>@1
 
 	# On Windows [file executable] might lie so we need to ask

Dear Johannes,

thanks for following up,

Indeed. Why don't you give it a try?

Actually, I already did: https://github.com/patthoyts/git-gui/pull/12

You might want to post your analysis and patch there as well...

cu, Philipp

Hello,

using git 2.16 the same problem is still present. I see that the pull 
request https://github.com/patthoyts/git-gui/pull/12 (along with 15 
other pull requests) are lying around since about one year without any 
sign of life from patthoyts.

Is there any hope from here that anyone will pick up this / these 
changes? Will anyone else be assigned the main responsible for this 
git-gui repository?

Just hoping to revive the discussion here, since the 
https://github.com/patthoyts/git-gui/ repository seems quite dead.

sincerely,

Chris Maes.

-- 
Macq nv
Luchtschipstraat, 2 - 1140 Brussel - België
T +32 (0) 2 610 15 57
chris.maes@macq.eu - www.macq.eu

Hi Chris,

On Tue, 10 Apr 2018, Chris Maes wrote:

using git 2.16 the same problem is still present.

And probably 2.17, too.

I see that the pull request https://github.com/patthoyts/git-gui/pull/12
(along with 15 other pull requests) are lying around since about one
year without any sign of life from patthoyts.

Yes, this is very sad. I hope he is alive and doing well.

As to Git GUI: if you know your way around Tcl/Tk reasonably well, how
about stepping up and reviewing those PRs? Even if the PRs are not merged,
a review would do those PRs pretty good and we could then take things from
there.

Is there any hope from here that anyone will pick up this / these
changes?  Will anyone else be assigned the main responsible for this
git-gui repository?

There is no "assigning" here, not really. What is missing is a volunteer
who earned the trust of the Git developers. Reviewing those PRs would go a
long way to earn that trust.

Just hoping to revive the discussion here, since the
https://github.com/patthoyts/git-gui/ repository seems quite dead.

Thank you for doing this.

I also hope that somebody with reasonable understanding of Tcl/Tk and a
vested interest in Git GUI takes up the responsibility of maintaining it.
Judging by the rate the PRs trickled into
https://github.com/patthoyts/git-gui, I think it would be a minor time
commitment.

Ciao,
Johannes

Hi Phillip,

On Wed, 14 Jun 2017, Philipp Gortan wrote:

thanks for following up,

quoted

Indeed. Why don't you give it a try?

Actually, I already did: https://github.com/patthoyts/git-gui/pull/12

You might want to post your analysis and patch there as well...

I wonder what good posting my analysis did, if nothing changed as a
consequence.

FWIW I opened this PR with Git for Windows to fix it properly:

	https://github.com/git-for-windows/git/pull/1757

I plan on consolidating all of the PRs at
https://github.com/patthoyts/git-gui, too, and to try to get them into
git.git. I guess that means that I just volunteered as interim maintainer
of the git-gui repository. However, I will really act as maintainer, not
as "cleaner upper".

Ciao,
Johannes

From: "Johannes Schindelin" <redacted>

Hi Phillip,

On Wed, 14 Jun 2017, Philipp Gortan wrote:

quoted

thanks for following up,

quoted

Indeed. Why don't you give it a try?

Actually, I already did: https://github.com/patthoyts/git-gui/pull/12

You might want to post your analysis and patch there as well...

I wonder what good posting my analysis did, if nothing changed as a
consequence.

FWIW I opened this PR with Git for Windows to fix it properly:

https://github.com/git-for-windows/git/pull/1757

I plan on consolidating all of the PRs at
https://github.com/patthoyts/git-gui, too, and to try to get them into
git.git.

       I guess that means that I just volunteered as interim maintainer
of the git-gui repository. However, I will really act as maintainer, not
as "cleaner upper".

"Curator" is a useful intermediate level concept between active maintenance 
and passive benign neglect, if that term is a help...

--
Philip