Skip to content
New issue

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

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

Already on GitHub? # 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.

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

Successfully merging this pull request may close these issues.

3 participants