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

Allow skipping hooks in certain git states: merge and/or rebase #173

Merged
merged 4 commits into from
May 26, 2021

Conversation

DmitryTsepelev
Copy link
Contributor

@DmitryTsepelev DmitryTsepelev commented Apr 28, 2021

skip now allows to skip a certain hook during merge or rebase, configuration example:

pre-commit:
  commands:
    commit-colors:
      run: commit-colors $(git rev-parse HEAD)
  skip:
    - merge
    - rebase

Things to do:

  • make it work on Windows
  • update docs

Things to discuss:

  • should we refactor run and run_windows to avoid duplicating the code?
  • is there a better name than skip_git_states? using old skip option
  • is there a better way to determine that merge or rebase are happening?

Closes #115

@Envek Envek added this to the 0.8 milestone Apr 29, 2021
@Envek
Copy link
Member

Envek commented Apr 29, 2021

Oh, yes, I want it so bad!

This is especially useful for post-checkout hook (as in our Lefthook, Crystalball, and git magic for smooth development experience blog post) as usually you don't want to run such things during git pull --rebase and so on.

And this is a good reason to bump version :-)


is there a better name than skip_git_states?

What about using existing boolean skip directive (allow it to also receive strings or arrays)? Something like this:

pre-push:
  commands:
    packages-audit:
      skip:
       - merge
       - rebase

@DmitryTsepelev
Copy link
Contributor Author

Changes:

  • renamed the option to skip;
  • added docs;
  • moved duplicated functions from run and run_windows to a separate file (not sure what I'm doing).

Announcement: I have never tested it on windows!

@DmitryTsepelev DmitryTsepelev changed the title Draft: allow skipping hooks in certain git stages Allow skipping hooks in certain git stages Apr 30, 2021
@DmitryTsepelev DmitryTsepelev marked this pull request as ready for review April 30, 2021 16:03
@Envek Envek requested a review from charlie-wasp May 4, 2021 11:04
@aminya
Copy link
Contributor

aminya commented May 10, 2021

Also fixes #102

cmd/run_helpers.go Outdated Show resolved Hide resolved
cmd/run_helpers.go Outdated Show resolved Hide resolved
Copy link
Member

@Envek Envek left a 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"

@DmitryTsepelev DmitryTsepelev force-pushed the feature/skip_git_stages branch 2 times, most recently from 5f476f7 to 62436b1 Compare May 20, 2021 14:41
@DmitryTsepelev
Copy link
Contributor Author

Had to make a new branch and copy changes since there were lots of conflicts 🙈

Copy link
Member

@Envek Envek left a 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

Copy link
Member

@Envek Envek left a 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:

  1. For main repo:
    • root url: ~/foo
    • git dir: ~/foo/.git (here will be REBASE_HEAD for main tree)
    • git hooks dir: ~/foo/.git/hooks
  2. For worktree foo-wt:
    • root url: ~/foo-wt
    • git dir: ~/foo/.git/worktrees/foo-wt (here will be REBASE_HEAD for worktree)
    • git hooks dir: ~/foo/.git/hooks (same as for the main repo)

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) {
Copy link
Member

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 🤯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

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
Comment on lines 469 to 470
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) {
Copy link
Member

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.

Copy link
Contributor Author

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"

Copy link
Member

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

@DmitryTsepelev
Copy link
Contributor Author

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 skip_git_stages key) or hack around Viper (because I believe they won't accept a PR allowing to prefer boolean over anything else). Thoughts?

@Envek
Copy link
Member

Envek commented May 24, 2021

For now I created issue in Viper for this (spf13/viper#1142) as I wish that we could reuse the same skip setting.

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?

@DmitryTsepelev
Copy link
Contributor Author

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!

Envek added 2 commits May 24, 2021 22:56
Before that it returned relative path for main working tree and absolute paths for linked working trees
@Envek Envek changed the title Allow skipping hooks in certain git stages Allow skipping hooks in certain git states: merge and/or rebase May 26, 2021
@Envek Envek merged commit 6b4f4ef into evilmartians:master May 26, 2021
@Envek
Copy link
Member

Envek commented May 26, 2021

Thank you, @DmitryTsepelev, amazing work!

@Envek
Copy link
Member

Envek commented Jun 7, 2022

Finally released in 0.8.0

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.

Is there a way to not run pre-commit on merge?
4 participants