rycus86 / githooks

Githooks: per-repo and global Git hooks with version control
MIT License
378 stars 19 forks source link

Global hooks from local directory? #82

Open matthijskooijman opened 4 years ago

matthijskooijman commented 4 years ago

What I'd like to do with githooks, is have a global set of hooks that run for all repositories on my system. I'd like to use githooks, rather than just putting the hooks in the global core.hooksPath directory, in order to allow also running .githooks in repositories, and to allow cleanly separating various stuff in multiple files.

It seems there is some support for this through shared hooks, which can be pulled in by a specfic repository using a .githooks/.shared file that lists repository urls to pull hooks from. Also, the githooks.shared config variable can also add repositories to this list for all repositories on my system.

The latter is is almost what I want, except that AFAIU this:

matthijskooijman commented 4 years ago

Hm, it seems I might have misunderstood the documentation somewhat. I thought the shared hooks would be cloned into the repository .githooks directory, but the documentation quite clearly says "hese repositories will be checked out into the /.githooks/shared folder (~/.githooks/shared by default)," (so not sure where my confusion came from).

So, (global) shared hooks are only checked out once, which solves half of my problem. I'd still like to have just a locally managed directory, rather than a git checkout, but that might be easy to add, or workaround :-)

matthijskooijman commented 4 years ago

I ended up just doing this:

--- a/base-template.sh
+++ b/base-template.sh
@@ -30,6 +30,7 @@ process_git_hook() {
     execute_global_shared_hooks "$@" || return 1
     execute_local_shared_hooks "$@" || return 1
     execute_all_hooks_in "$(pwd)/.githooks" "$@" || return 1
+    execute_all_hooks_in "$HOOK_FOLDER/../global-hooks" "$@" || return 1
 }

 #####################################################

This just runs all hooks from a global directory of my choice. Here it's relative to the hook folder since I'm running from a git checkout, see #83, but in general it would probably be more useful if the path would be loaded from a git config variable instead.

gabyx commented 4 years ago

Maybe we could support such a thing easily: Lest se what @rycus86 thinks. I think we can definitely for local/global shared hooks implement an opt-in solution, that also normal folders are accepted instead of git repos.

That should be easy :-)

rycus86 commented 4 years ago

I think this might just be https://github.com/rycus86/githooks/blob/master/docs/command-line-tool.md#git-hooks-shared ? Would git hooks shared add --global <local-dir> do what you described @matthijskooijman ?

matthijskooijman commented 4 years ago

@rycus86 I think the shared hooks always need a git repo, right? Which gets cloned into ~/.githooks? I guess <local-dir> could be a local git repo, but then I think it gets cloned into ~/.githooks (and automatically updated/pulled for some hooks), but that seems rather pointless (having two copies of this local git repo) as well as leaves room for using old versions of my local dir (when I change something in <local-dir> but then do not trigger one of the hooks that triggers a git pull on the shared repos).

So it could be made to work, but I'm not so thrilled about how it would work excactly.

What @gabyx suggests sounds perfect to me, just keep the existing settings but support local directories. One caveat: I think this local directory support should not be applied to the lines from the .githooks/.shared directories inside repositories, as content inside a repo can typically not assume anything about the filesystem layout of the systems it is cloned on (unlike local and global git settings, which are expected to be set by the local user).

rycus86 commented 4 years ago

Sure, it could get some conditional logic, but I'd rather just treat everything the same, so it's easy to support them. What's your reservation about the double-checkout? Only space concerns?

The update delay problem is there, it's a tradeoff for not slowing down all your git operations, only the ones that are likely to be slow anyway, like pull IIRC. There's also a cli command to force the shared repo updates that you can use if you absolutely need up-to-date local hooks.

matthijskooijman commented 4 years ago

Sure, it could get some conditional logic, but I'd rather just treat everything the same, so it's easy to support them. What's your reservation about the double-checkout? Only space concerns?

Space is not so much of an issue, it's more the extra complexity of the setup. I know how this stuff works now, but when I come back to this in a few months and don't remember the details, end up modifying the wrong copy and have my changes overwritten, or end up modifying the right copy but the changes not applied immediately, etc.

The update delay problem is there, it's a tradeoff for not slowing down all your git operations, only the ones that are likely to be slow anyway, like pull IIRC. There's also a cli command to force the shared repo updates that you can use if you absolutely need up-to-date local hooks.

Yeah, for shared hooks from a remote repository, this makes total sense. But for a local directory, there is no need for all this cloning/pulling overhead, so it would be nicer to skip all this overhead then.

rycus86 commented 4 years ago

Yeah, you're making good points. We could try to just use the hooks from a local shared repo as-is, without the extra clone, then see how that changes the code. If it's not too bad, we can go with it.

Do you know of a reliable way of telling apart local/remote git repo URLs? (using only POSIX tools)

matthijskooijman commented 4 years ago

Do you know of a reliable way of telling apart local/remote git repo URLs? (using only POSIX tools)

I think local git urls can be file://, but probably also plain paths.

I would suggest just allowing a local absolute path instead of a git url, then you can just see if it starts with / and use the [ -d ...] or something to see if it actually points to a local directory. Just the -d option might also work, but that might end up allowing relative paths as well (which can be ok if you make guarantees about the current directory, but that might also complicate things). If you need to support Windows, the starts with / check might need to be a bit more involved (not sure if there's tools for that).

Also, I think such local paths do not need to be a git repository at all, they might just be plain directories with hooks (which is another argument for not trying to clone these but just using them as-is).

rycus86 commented 4 years ago

Good points, a [ -d "$item" ] || echo "$item" | grep -qE '^file://' of some sorts could probably work for most cases.

matthijskooijman commented 4 years ago

I would maybe not treat file:// specially, because:

rycus86 commented 4 years ago

Heh, true, getting more complicated by the minute. 😅

gabyx commented 4 years ago

Huch: ok I see, what I would suggest ist the following:

  1. all urls in local and global shared hooks which do not start with <protocol>:// are treated as non clonable paths, otherwise they get cloned and the default logic happens. For non-clonable paths
    • Relative paths are relative to the repository, absolut are absolut
    • we dont clone, but directy use these paths (no changes are made to these path in any circumstance, since we are not under control
    • updates can only be done manually by the user by updating these paths them self if they happen to be git repos.
    • The file:// option is still there to explicitly allow paths locally which get cloned

Windows is not a concern, since everything is wrapped through bash.exe anyways and it works :-)! Paths should be with forward slashes (and anything otherwise we define as unsupported, it might work however...), we do it as git does it...

That should be done fairely easily

matthijskooijman commented 4 years ago

Sounds good. One addition: I think git urls are typically e.g. git@github.com:user/repo.git, so without a protocol:// prefix (I think you can also do ssh:// but that is not commonly done).

This is why I suggested looking at a leading /, but that is again problematic on Windows (since absolute paths start with e.g. c:/).

You can probably just take any url, check it with [ -d and use as-is if it turns out to be a directory. There might be a small chance that an url also ends up being valid as a directory, but that seems unlikely enough (and nothing will be run without a prompt anyway, so there should not be any unsafety here).

Alternatively, you could just introduce an extra config directive, e.g. shared-dir or something to be explicit about what is a dir and what is a repo.

rycus86 commented 4 years ago

Yeah, perhaps a simple [ -d .. ] test is fine, then we'll just add some docs to use a local path as an argument if you want it to work like this.

gabyx commented 4 years ago

May be an extra config directive isnt that bad, mixing all different things together makes it hard for further improvements if we need to keep things apart...

rycus86 commented 4 years ago

Yeah, I'd just better optimize for end-user experience/convenience if we can, in cases where it's a matter of perhaps the feature not being as powerful, but does what you want always/most of the time. Introducing more options might get a bit confusing, but then again, that ship might have sailed already. 🤷‍♂️

gabyx commented 4 years ago

True :)

Von meinem iPhone gesendet

Am 23.04.2020 um 11:36 schrieb Viktor Adam notifications@github.com:

Yeah, I'd just better optimize for end-user experience/convenience if we can, in cases where it's a matter of perhaps the feature not being as powerful, but does what you want always/most of the time. Introducing more options might get a bit confusing, but then again, that ship might have sailed already. 🤷‍♂️

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

gabyx commented 3 years ago

Huch: ok I see, what I would suggest ist the following:

  1. all urls in local and global shared hooks which do not start with <protocol>:// are treated as non clonable paths, otherwise they get cloned and the default logic happens. For non-clonable paths

    • Relative paths are relative to the repository, absolut are absolut
    • we dont clone, but directy use these paths (no changes are made to these path in any circumstance, since we are not under control
    • updates can only be done manually by the user by updating these paths them self if they happen to be git repos.
    • The file:// option is still there to explicitly allow paths locally which get cloned

Windows is not a concern, since everything is wrapped through bash.exe anyways and it works :-)! Paths should be with forward slashes (and anything otherwise we define as unsupported, it might work however...), we do it as git does it...

That should be done fairely easily

Maybe we should amke some progress here ;)

gabyx commented 3 years ago

Maybe we can also just add <protocol>://<path>[@tag|commit|branch] with a specific branch or tag to checkout.

gabyx commented 3 years ago

See #119 underway :-)