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

issue a warning if a mutable ref is used (HEAD / stable / master / etc.) #974

Closed
asottile opened this issue Mar 18, 2019 · 4 comments · Fixed by #1715
Closed

issue a warning if a mutable ref is used (HEAD / stable / master / etc.) #974

asottile opened this issue Mar 18, 2019 · 4 comments · Fixed by #1715

Comments

@asottile
Copy link
Member

Perhaps nudging towards https://pre-commit.com/#using-the-latest-version-for-a-repository or pre-commit autoupdate

@giannello
Copy link

We use pre-commit to maintain a shared set of hooks for our 50+ repositories. Using a mutable ref is at the moment the solution that requires less effort from our developers, as there's no need to update the .pre-commit-config.yaml file and add unnecessary commits whenever the hooks are updated.
We're fully aware that this way hooks are not stable, but that's a drawback we are willing to accept in order to keep a faster pace when developing both our software and the hooks themselves.
What's the rationale behind enforcing a specific revision of the hooks? Ultimately, it should be a user's informed decision whether to stick with a specific revision or with a mutable one.

@asottile
Copy link
Member Author

this bit goes into a little detail about the rationale

The design enforces this idea for a couple reasons

  • The first is that global hooks cause mass breaking upon improvement. From experience, having a system-managed hook which upgrades without a commit often causes a large percentage of repositories to go from green-to-red. The situation is even worse when that update requires a lock-step upgrade (old hook fails with new-hook's expected output, new-hook fails with old-hook's expected output).
  • Repeatability. You want to be able to check out any version of your code and have it build in a repeatable fashion. This is especially true for your most basic checks (syntax / lint / etc.). If linters are non-determinstic, you're much more likely to develop fatigue or turn them off.
  • Cargo cult. Since pre-commit caches based on the rev value, you'll immediately start drifting across all of your machines. From practice, it's not uncommon that a pre-commit cache lives for many months or even years! This causes a whole class of "works on my machine" and "oh yeah that thing sucks just run pre-commit clean" which is both a headache and a huge waste of precious developer time.
  • It's trivially easy to upgrade. pre-commit autoupdate was purpose built to make it easy to upgrade to the latest version. Coupled with pre-commit run --all-files, an upgrade and reformat is a snap.
  • Even with many repositories (we manage ~550 and growing with pre-commit at lyft) you can use tools to distribute these updates periodically -- for example all-repos has an autofixer which does this example PR

@ssbarnea
Copy link

ssbarnea commented Nov 7, 2019

While I do not use branches anywhere, and I am not against this feature, I would to mention few aspects:

  • tags can mutate too, just like branches
  • what makes a tag/branch mutable, its name? are we going to harcode warning for branches like master or devel?
  • if we implement it, lets add a config item that can be used to disable this warning, so those adding it will confirm that they know what they are doing.
  • I do see a perfectly fine use-case where someone could want to use a stable-2.3 branch and avoid the need to update hundreds of repos just because they had a hotfix on it.
  • there is a bug in pre-commit autoupdate which removes a revision and replaces it with last tag, even if the revision may be newer than the tag. I faced it myself where I was forced to used an unreleased version of ansible-lint which contained critical bugfixes.

@asottile
Copy link
Member Author

asottile commented Nov 7, 2019

  • I'm not going to make this configurable
  • that's not a bug and completely off topic, you want --bleeding-edge

This was referenced Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants