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

forbid-new-submodules marked not applicable on repo w/o submodules #707

Closed
ChiefGokhlayeh opened this issue Dec 27, 2021 · 4 comments
Closed

Comments

@ChiefGokhlayeh
Copy link

ChiefGokhlayeh commented Dec 27, 2021

This might be intended behavior, but I'd like to get confirmation.

Since 4.1.0, meta-hook check-hooks-apply marks forbid-new-submodules as "does not apply", if the repo does not contain any submodules.

The hook forbid-new-submodules itself correctly warns when adding submodules to a previously empty repo, so it technically does apply.

This might have to do with the changes introduced in #619.

Steps to reproduce:

  1. Create new repo
    $ git init issue-707 && cd issue-707
  2. Create .pre-commit-config.yaml with following content:
    repos:
      - repo: https://github.com/pre-commit/pre-commit-hooks
        rev: v4.1.0
        hooks:
          - id: forbid-new-submodules
      - repo: meta
        hooks:
          - id: check-hooks-apply
  3. Commit .pre-commit-config.yaml
    $ git add .pre-commit-config.yaml && git commit -m "Add pre-commit config"
  4. Run the following:
    $ pre-commit run --all
    forbid new submodules................................(no files to check)Skipped
    Check hooks apply to the repository......................................Failed
    - hook id: check-hooks-apply
    - exit code: 1
    
    forbid-new-submodules does not apply to this repository
  5. Add submodule and force-commit
    $ git submodule add https://github.com/pre-commit/pre-commit-hooks.git
    $ git commit -m "Add some submodule" -n
  6. Run the following:
    $ pre-commit run --all
    forbid new submodules....................................................Passed
    Check hooks apply to the repository......................................Passed
@asottile
Copy link
Member

indeed this is a bit of a tricky case -- admittedly the original forbid-new-submodules was designed for a migration where submodules were slowly removed over time (and wasn't necessary afterwards)

#619 changed how the hook matches files which causes it to no longer match on a repo without submodules

@ChiefGokhlayeh
Copy link
Author

I admit, the chances of someone accidentally adding a submodule are very slim. I simply added it to all my configs as a nice to have.

If this is a "won't fix", feel free to close the issue. I can simply remove the hook where no longer applicable.

@asottile
Copy link
Member

you can maybe restore the old behaviour by overriding types: [] but then it probably does more work than it needs to

JJMC89 added a commit to JJMC89/pywikibot-extensions that referenced this issue Dec 27, 2021
JJMC89 pushed a commit to JJMC89/pywikibot-extensions that referenced this issue Dec 27, 2021
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.0.1 → v4.1.0](pre-commit/pre-commit-hooks@v4.0.1...v4.1.0)
- [github.com/pre-commit/mirrors-mypy: v0.920 → v0.930](pre-commit/mirrors-mypy@v0.920...v0.930)

remove forbid-new-submodules pre-commit/pre-commit-hooks#707
JJMC89 added a commit to jjmc89-bot/nfcbot that referenced this issue Dec 27, 2021
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.0.1 → v4.1.0](pre-commit/pre-commit-hooks@v4.0.1...v4.1.0)
- [github.com/pre-commit/mirrors-mypy: v0.920 → v0.930](pre-commit/mirrors-mypy@v0.920...v0.930)
- [github.com/shellcheck-py/shellcheck-py: v0.8.0.2 → v0.8.0.3](shellcheck-py/shellcheck-py@v0.8.0.2...v0.8.0.3)

remove forbid-new-submodules pre-commit/pre-commit-hooks#707
JJMC89 pushed a commit to jjmc89-bot/nfcbot that referenced this issue Dec 27, 2021
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.0.1 → v4.1.0](pre-commit/pre-commit-hooks@v4.0.1...v4.1.0)
- [github.com/pre-commit/mirrors-mypy: v0.920 → v0.930](pre-commit/mirrors-mypy@v0.920...v0.930)
- [github.com/shellcheck-py/shellcheck-py: v0.8.0.2 → v0.8.0.3](shellcheck-py/shellcheck-py@v0.8.0.2...v0.8.0.3)

remove forbid-new-submodules pre-commit/pre-commit-hooks#707
JJMC89 added a commit to jjmc89-bot/jjmc89-bot-scripts that referenced this issue Dec 28, 2021
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.0.1 → v4.1.0](pre-commit/pre-commit-hooks@v4.0.1...v4.1.0)

remove forbid-new-submodules pre-commit/pre-commit-hooks#707
JJMC89 pushed a commit to jjmc89-bot/jjmc89-bot-scripts that referenced this issue Dec 28, 2021
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.0.1 → v4.1.0](pre-commit/pre-commit-hooks@v4.0.1...v4.1.0)

remove forbid-new-submodules pre-commit/pre-commit-hooks#707
JJMC89 added a commit to jjmc89-bot/bsiconsbot that referenced this issue Dec 28, 2021
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.0.1 → v4.1.0](pre-commit/pre-commit-hooks@v4.0.1...v4.1.0)
- [github.com/pre-commit/mirrors-mypy: v0.920 → v0.930](pre-commit/mirrors-mypy@v0.920...v0.930)

remove forbid-new-submodules pre-commit/pre-commit-hooks#707
JJMC89 pushed a commit to jjmc89-bot/bsiconsbot that referenced this issue Dec 28, 2021
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.0.1 → v4.1.0](pre-commit/pre-commit-hooks@v4.0.1...v4.1.0)
- [github.com/pre-commit/mirrors-mypy: v0.920 → v0.930](pre-commit/mirrors-mypy@v0.920...v0.930)

remove forbid-new-submodules pre-commit/pre-commit-hooks#707
@asottile
Copy link
Member

I think the resolution of this is "use this while migrating away from submodules" and once you're done you can replace it with a simple language: fail hook:

-   repo: local
    hooks:
    -   id: no-submodules
        name: no submodules
        language: fail
        entry: submodules are not allowed in this repository
        types: [directory]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants