Skip to content

pre-commit hook is run at the wrong time #1711

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

Open
pm100 opened this issue Jun 1, 2023 · 4 comments
Open

pre-commit hook is run at the wrong time #1711

pm100 opened this issue Jun 1, 2023 · 4 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@pm100
Copy link
Contributor

pm100 commented Jun 1, 2023

Working on prepare-commit-msg hook (still).

prepare-commit-msg needs to run before commit message dialog is shown, this is doable . But in working on where to place that hook I find that the pre-commit hook is run at the wrong time. It should be run before the user gets to type in the commit message. This is how git cli runs it. It is annoying to type in a commit message for a commit that pre-commit is going to reject anyway

This is not easy to fix though. The way the user gets to choose to skip the validation by the 2 hooks that are allowed to veto a commit is by entering ^f in the commit dialog. Thats too late to bypass the pre-commit check if its run before the UI is shown

solutions

  • dont worry about it. This would run the hooks out of order, prepare-message (which must run before the UI is shown), pre-commit, commit-msg and post-commit. And not mind about the mild user annoyance. There is also the slight possibility that running them out of the specified order creates a problem (if one depends on the other, say)
  • change the mechanism for turning off validate. Maybe a different commit key (upper case C?) or maybe ^f toggle before the 'c' instead of after. This would be a breaking UI change though
@pm100 pm100 added the bug Something isn't working label Jun 1, 2023
@extrawurst
Copy link
Collaborator

Can you make the case by adding links to where these orders are defined?

@pm100
Copy link
Contributor Author

pm100 commented Sep 3, 2023

https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks

"The pre-commit hook is run first, before you even type in a commit message."

@extrawurst
Copy link
Collaborator

since pre-commit is supposed to inspect the snapshot of what would be committed I would run it when the commit msg window pops-up and show the result if the verify fails and force the user to use the no-verify commit command to still execute the commit.

this is low-prio to me right now

@extrawurst
Copy link
Collaborator

extrawurst commented Sep 4, 2023

@pm100 in #1873:

I cant say I like what you propose for that (there would be no way to prevent the pre-commit running for example)

I am happy to leave it as is and run pre-commit once you hit commit (inside the commit msg popup) which puts it in line with other GUI tools like fork

it would be nice to have hook parity with git cli

exact parity is obviously not possible because UX of a cli and a GUI is just different. I am fine to be consistent with other tools (see above).

last but not least remember you compare apples and oranges: running pre-commit after prepare-commit-msg should be independent as one operates on the index state and the other on producing a commit-msg-text

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants