Skip to content
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.

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

respect core.hooksPath, falling back to .git/hooks #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mephinet
Copy link

@mephinet mephinet commented Jun 2, 2017

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

@@ -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]}]} {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a get_config function that uses a cached config (you need to use lower-case keys for that, though, IIRC).

In any case, this is duplicating the logic in Git's config.c, therefore a bit fragile, besides, if Git GUI is used with Git <v2.9.0 it would be inconsistent with Git's idea of the hooks path.

How about something like this instead (completely untested, you likely have to fix a few bits here and there):

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

I offered the same patch on the Git mailing list, together with a suggestion for an improved commit message.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about making it so by amending your patch and force-pushing, just in case that Pat returns from his absence and wants to merge the open PRs?

@dscho
Copy link
Contributor

dscho commented Jun 14, 2017

Hi @mephinet. I offered an alternative, also on the Git mailing list.

Since this repository has become very, very quiet, I would understand if you went back to trying to get this patch in via the Git mailing list, side-stepping @patthoyts.

If you want to, I would be willing to carry the patch (after the discussion settles) in Git for Windows (not sure what OS you use, so I don't know whether that would help you).

@dscho
Copy link
Contributor

dscho commented Jul 9, 2018

@mephinet hi?

@luismbo
Copy link

luismbo commented Jul 10, 2018

This fixes git-for-windows/git#1755.

@dscho
Copy link
Contributor

dscho commented Jul 10, 2018

@luismbo could I bother you to test git-for-windows/git#1757, as this here PR has been stalled from two sides already?

@mephinet
Copy link
Author

@dscho you're very welcome to forward the patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants