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

Fail if no lefthook was found #212

Closed
roman-ih opened this issue Jun 22, 2021 · 14 comments · Fixed by #533
Closed

Fail if no lefthook was found #212

roman-ih opened this issue Jun 22, 2021 · 14 comments · Fixed by #533

Comments

@roman-ih
Copy link

roman-ih commented Jun 22, 2021

Currently, if lefthook was not found, the user will see the message "Can't find lefthook in PATH" but commit still will go through.

IMHO it is not how it should work. The fix can be as simple as adding exit 1 in the else clause.

call_lefthook()
{
  if lefthook -h >/dev/null 2>&1
  then
    eval lefthook $1
  elif test -f "$dir/node_modules/@arkweid/lefthook/bin/lefthook"
  then
    eval "$dir/node_modules/@arkweid/lefthook/bin/lefthook $1"
  elif bundle exec lefthook -h >/dev/null 2>&1
  then
    bundle exec lefthook $1
  elif npx lefthook -h >/dev/null 2>&1
  then
    npx lefthook $1
  elif yarn lefthook -h >/dev/null 2>&1
  then
    yarn lefthook $1
  else
    echo "Can't find lefthook in PATH"
  fi
}



call_lefthook "run pre-commit $@"
@roman-ih roman-ih changed the title Feature request: An option to fail if no lefthook was found Feature request: Fail if no lefthook was found Jun 22, 2021
@roman-ih roman-ih changed the title Feature request: Fail if no lefthook was found Fail if no lefthook was found Jun 22, 2021
@Envek
Copy link
Member

Envek commented Jun 22, 2021

Well, I believe that by default it shouldn't fail if binary wasn't found. It is better to miss something than completely break workflow for some people, IMO.

I don't see a case why we should always abort commit/push/whatever in case we can't perform git hook checks (because hooks are easily skippable by --no-verify option to git commit and git push commands).

If you want to enforce some rules (code linting, commit messages, whatever), you'd better to do this on CI (git hooks just helps to shorten feedback loop).

@skryukov
Copy link
Member

It can break someone with a modified PATH also #178

@roman-ih
Copy link
Author

There are several escape paths already if things goes wrong

  1. git commit --no-verify
  2. LEFTHOOK=0 git commit

And similar as for @skryukov it broke for me twice already.

The use case - I pull latest changes which introduce a new gem in Gemfile, then make some changes myself and commit them without doing bundle install.

@roman-ih
Copy link
Author

People add lefthook for a reason even if it is easy to skip. I think it should break and print a reasonable message what to do if one still wants to proceed.

@roman-ih
Copy link
Author

Opened PR - #213

@bf4
Copy link

bf4 commented Feb 8, 2023

ooh, LEFTHOOK=0 is gold --no-verify isn't working for me on a branch which doesn't yet have lefthook installed. I just get Error: Config File "lefthook" Not Found in... and lose the commit message

@marat-y
Copy link

marat-y commented Mar 16, 2023

I've just installed lefthook using snap and by default git actions fail if no lefthook config file is found. Is there any way to change this behavior without the need to add LEFTHOOK=0 all the time?

@mrexox
Copy link
Member

mrexox commented Mar 16, 2023

Is there any way to change this behavior without the need to add LEFTHOOK=0 all the time?

Could you describe what happened? You have git hooks using lefthook, but no lefthook.yml in your git project, right? And you'd like to see only a warning and not to stop git from creating a commit, right?

@marat-y
Copy link

marat-y commented Mar 16, 2023

@mrexox yes, exactly

@mrexox
Copy link
Member

mrexox commented Mar 16, 2023

I'll try to prepare a fix by next week 👍

@mrexox
Copy link
Member

mrexox commented Mar 20, 2023

Hey @roman-ih! Sorry for the long delay. It feels like changing current behavior is a big change for lefthook. So, I'd like to suggest an option.

What do you think of having a global config option for that, like ensure: true or mandatory: true? Git hooks will have exit 1 if this option is set to true. So, current lefthook users won't have problems when upgrading, but you can make your configuration stricter and require lefthook to be available (through PATH or other supported options).

@hyperupcall
Copy link
Contributor

hyperupcall commented Jul 18, 2023

I'm interested in this feature as well, and such a global configuration sounds very useful. The next-best option if the default cannot be changed for everyone.

@mrexox
Copy link
Member

mrexox commented Jul 18, 2023

@hyperupcall, cool! Do you like mandatory: true or any other naming for this option?

@hyperupcall
Copy link
Contributor

hyperupcall commented Jul 18, 2023

For me mandatory seems a bit vague to me - I like assert_lefthook_exists assert_lefthook_installed, or maybe fail_if_absent or abort_if_not_installed

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 a pull request may close this issue.

7 participants