-
Notifications
You must be signed in to change notification settings - Fork 220
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
Allow skipping hooks in certain git states: merge and/or rebase #173
Allow skipping hooks in certain git states: merge and/or rebase #173
Conversation
Oh, yes, I want it so bad! This is especially useful for And this is a good reason to bump version :-)
What about using existing boolean pre-push:
commands:
packages-audit:
skip:
- merge
- rebase |
308f922
to
99d1c97
Compare
Changes:
Announcement: I have never tested it on windows! |
Also fixes #102 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build with goreleaser build --skip-validate --rm-dist
isn't passing with following error:
⨯ build failed after 1.67s error=failed to build for windows_amd64: # github.com/evilmartians/lefthook/cmd
cmd/run_windows.go:14:2: imported and not used: "sort"
cmd/run_windows.go:22:2: imported and not used: "github.com/gobwas/glob"
5f476f7
to
62436b1
Compare
62436b1
to
0ce9374
Compare
Had to make a new branch and copy changes since there were lots of conflicts 🙈 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like there is some validation of config types exists that prevents it from working.
Given following lefthook.yml
:
post-checkout:
piped: true
scripts:
01-bundle-checkinstall:
skip: true
tags: backend
02-db-migrate:
skip: true
tags: backend
03-crystalball-update:
skip: true
tags: rspec backend
And following lefthook-local.yml
:
post-checkout:
piped: true
scripts:
01-bundle-checkinstall:
skip: false
02-db-migrate:
skip: rebase
03-crystalball-update:
skip:
- rebase
- merge
Checkout to another branch (without merge or rebase in progress) produces following output:
$ git checkout master
ERROR 2021/05/22 12:16:28 svType != tvType; key=skip, st=string, tt=bool, sv=rebase, tv=true
ERROR 2021/05/22 12:16:28 svType != tvType; key=skip, st=[]interface {}, tt=bool, sv=[rebase merge], tv=true
Lefthook v0.7.5
RUNNING HOOKS GROUP: post-checkout
EXECUTE > 01-bundle-checkinstall
The Gemfile's dependencies are satisfied
02-db-migrate (SKIP BY SETTINGS)
03-crystalball-update (SKIP BY SETTINGS)
SUMMARY: (done in 0.36 seconds)
Expected result: all steps are run
Actual result: steps 2 and 3 are skipped, type errors are printed to console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing is git worktree support.
As every worktree can do rebases and merges independently from main worktree, every worktree has its own git directory (which can be retrieved with git rev-parse --git-dir
command) where we need to search for merge and rebase files (however, hooks are global for all worktrees and are only stored in global worktree).
For example, if we have repository ~/foo
with worktree ~/foo-wt
and both are during rebase, directory structure will be as follows:
- For main repo:
- root url:
~/foo
- git dir:
~/foo/.git
(here will beREBASE_HEAD
for main tree) - git hooks dir:
~/foo/.git/hooks
- root url:
- For worktree
foo-wt
:- root url:
~/foo-wt
- git dir:
~/foo/.git/worktrees/foo-wt
(here will beREBASE_HEAD
for worktree) - git hooks dir:
~/foo/.git/hooks
(same as for the main repo)
- root url:
These are output of git rev-parse --show-toplevel
, git rev-parse --git-dir
, and git rev-parse --git-path hooks
commands respectively.
Looks like that we need one more helper like getGitDir
that will return path from git rev-parse --git-dir
command (see #177 for reference).
cmd/run.go
Outdated
} | ||
|
||
func isMergeInProgress() bool { | ||
if _, err := os.Stat(filepath.Join(getRootPath(), ".git", "MERGE_HEAD")); os.IsNotExist(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inside git worktrees, getRootPath(), ".git"
will be a file, not directory! Also every worktree has its own git directory placed inside .git
directory of the main worktree 🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And fixed one more time for worktrees (pushed commit right into your pull request, sorry!)
cmd/run.go
Outdated
if _, mergeErr := os.Stat(filepath.Join(getRootPath(), ".git", "rebase-merge")); os.IsNotExist(mergeErr) { | ||
if _, applyErr := os.Stat(filepath.Join(getRootPath(), ".git", "rebase-apply")); os.IsNotExist(applyErr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, whether can we check for single REBASE_HEAD
file instead of this two checks? 🤔
Also here we need to use output of git rev-parse --git-dir
command to get .git
directory for worktrees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like REBASE_HEAD exists only when it's an "interactive rebase or when rebase is stopped because of conflicts"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, official git prompts also checks for these directories, so let's keep it as it is: https://github.com/git/git/blob/de88ac70f3a801262eb3aa087e5d9a712be0a54a/contrib/completion/git-prompt.sh#L446-L462
Talkin about merging configs: looks like Viper does not support merging values of different types. Looks like we have to either change our API (e.g., bring back a separate |
For now I created issue in Viper for this (spf13/viper#1142) as I wish that we could reuse the same However, it is probably quite a rare use case (when hooks are always disabled in main config, and only during merge/rebase in local config), so may be we can safely ignore it for now? |
Yeah, we can add a separate issue for that and see if it's a big deal. Would be really nice for Viper to handle it for us! |
Before that it returned relative path for main working tree and absolute paths for linked working trees
Thank you, @DmitryTsepelev, amazing work! |
Finally released in 0.8.0 |
skip
now allows to skip a certain hook during merge or rebase, configuration example:Things to do:
Things to discuss:
is there a better name thanusing oldskip_git_states
?skip
optionCloses #115