Fixed check for manual stage

[ThunderKey]

I've noticed in our setup:

• Checks configured with default_stages = [ "manual" "pre-push" ]; or default_stages = ["manual" "push"]; as in the README
• Push-Hook works
• nix flake check does not run anything

I think there was a mistake in the detection if the manual stage should be used.

[sandydoo]

@ThunderKey, I think this is still incorrect.

The original is only true when install_stages is exactly [ "manual" ].
This PR will always run the manual stage, even if manual isn't in install_stages. Unless install_stages = [ "manual" ], in which case it won't run.

I think we need to understand what people want from this. I find the pre-commit docs mildly confusing, but my understanding is that to run hooks that only have the manual stage, we need to specify the stage. And most hooks will have "manual" in their stages, in additional to others.

So the check should be: if builtins.toString (lib.lists.elem "manual" install_stages) is equal to 1, then we run the manual stage. Otherwise, do whatever the default is.

[sandydoo]

See comment.

[sandydoo]

Another compounding issue is that default_stages is set to pre-commit. I think it should include manual. We need to split up what installs hooks vs providing default stages.

[ThunderKey]

You are right. I've misunderstood the compare.

We have changed default_stages in our project and it gets used in the Config file correctly.

I see two options:

1. Add an option to set the stage that is run in the check (default would be pre-commit). Then we don't even need the switch.
2. If pre-commit is in the install_stages use no specific stage. Otherwise take the first stage in the install_stages list. If desired we could first check if manual is included and then use this. With this a check would always run a valid stage, but maybe not the one that is expected.

Do you have any preferences?

[sandydoo]

Add an option to set the stage that is run in the check (default would be pre-commit). Then we don't even need the switch.

So something like #415.

What is pre-commit's default stage when you run pre-commit run --all-files? Is it pre-commit or does it run all stages except for manual?

If it's not pre-commit, then if we add an option, it should either be null or "manual".
If we go with manual, we would need to change how default_stages works. It currently controls both which hooks are installed and which stages each hook supports by default. Instead, what we need is to have every hook stage in default_stages (including manual) and add another option to specify which stages to install. pre-commit now has default_install_hook_types, so we could reuse that. This is, however, a breaking change.

[sandydoo]

#451 makes all of this really tricky.
Like, if you decided to use some stage during nix flake check, then that stage would also have to be installed as well.

I think if we add manual to default_stages and add an optional check.hookStage (or flakeCheck.hookStage to reduce conflict risk 🤷), then we get the least amount of breakage?

[ThunderKey]

The default of the --hook-stage in is unfortunately pre-commit (https://github.com/pre-commit/pre-commit/blob/main/pre_commit/main.py#L79-L85). Therefore, pre-commit run --all-files only runs the pre-commit stage.

Ah yes, my first option is exactly #415.

For pre-commit run --hook-stage pre-push to work, the pre-push hook does not need to be installed. I've tested it with the following steps:

ls .git/hooks/pre-push  # exists
pre-commit uninstall -t pre-push  # hook gets removed
ls .git/hooks/pre-push  # does not exist
pre-commit run --hook-stage pre-push  # hook gets executed
ls .git/hooks/pre-push  # still does not exist, just to make sure

As I understand pre-commit run is exactly what the hook executes. Therefore, the hook requires the run command but not the other way around.

I'll close this PR in favour of #415.