Provide option to log all pre-commit activity

[pablo-the-recurlian]

I would like to see an option to log all pre-commit activity (success, warnings, failures, errors) so that it can then be used to perform metrics against it and other uses.

[asottile]

What sort of output are you looking for. Does pre-commit run --verbose | tee pre-commit.log get you part of the way there? Is there a standard format that you'd like to see. What would the cli look like for this?

Mostly looking to scope this :)

[pablo-the-recurlian]

Not sure of what would be the best format but a log file that contain the following information would be very helpful:
datetime, hostname, user, file path, file, plugin repo, plugin version/sha, plugin, git stage, result

[Kilo59]

@asottile Do you know of a way to configure pre-commit to always run with --verbose when it's triggered by an actual git commit?

[asottile]

@Kilo59 there currently isn't a way, though you can set verbose: true on a hook-by-hook basis.

[Kilo59]

@asottile do you have any suggestions for how I might get started adding this feature?

Love the project btw.

[asottile]

Which feature? OP's feature or "verbose during git commit"?

[Kilo59]

Sorry, should have been more clear. The "verbose during git commit".

[asottile]

ok! Let's discuss the ergonomics of such a feature and the possible approaches for that -- I'll give you code hints for each of them :)

1. The first option is to pass an environment variable that pre-commit picks up on at commit time.
   this is the one I'd prefer if it fits your usecase

This would follow the precedent set by SKIP.

You'd get output something like this:

$ PRE_COMMIT_VERBOSE=1 git commit -m 'test'
[trailing-whitespace] Trim Trailing Whitespace...........................Passed
hookid: trailing-whitespace

output if it had some

[master (root-commit) ba90d2e] test
 1 file changed, 5 insertions(+)
 create mode 100644 .pre-commit-config.yaml


$ git commit -m "test"
Trim Trailing Whitespace...........................Passed
[master (root-commit) ba90d2e] test
 1 file changed, 5 insertions(+)
 create mode 100644 .pre-commit-config.yaml

This would probably be done by changing the default for --verbose based on os.environ.get('PRE_COMMIT_VERBOSE') here

You'd write a test for this behaviour here (I've highlighted a similar test that you could adjust)

This to me feels like the nicest behaviour, verbose should really be used sparingly as the more warning noise there is the more likely people are going to ignore the output (or worse, turn it off!).

2. The second option would be to add a top-level configuration value for the .pre-commit-config.yaml

This would look something like:

verbose: true
hooks:
    # ...

To add to the configuration schema, you'd add a property here (I've highlighted a similar property).

You would then update the behaviour of run inside the run command, probably hereish (not exactly sure on the best place to put that tbh, right now it is being accessed via args (argparse return value) which isn't terribly typesafe or easy to reason about -- could mutate args but that also feels wrong -- maybe make a new namedtuple to represent the values that run needs and then adjust that based on the configuration value? not sure.

You'd probably add a test here (I've highlighted a similar test which sets verbose at a specific hook level).

This has the one downside that if one of your consumers doesn't want the verbose behaviour they don't really have a way to turn it off.

3. The last option, and imo the one that has the last visibility would be to write the option to the hook template during the install stage. Something like:

$ pre-commit install --use-verbose
$ git commit -m '...'
(verbose output

Something about this irks me but I can't quite put my finger on it, perhaps it's that I want the hook template to be as logic free as possible since it's really difficult to test / debug / reason about (and the end user visibility here is really poor).

That said, there is precedent via pre-commit install --skip-on-missing-config. This adds to the hook template and then affects only the invocation of git commit afterwards.

You'd probably test this in the same place as option 1, though you'd probably follow this test

Hope this is helpful, let me know what you think!

[asottile]

Seems there hasn't been any renewed interest in this, going to close for now

[webknjaz]

FTR it'd be great to have some verbosity level causing pre-commit output the commands that it runs, not even for fetching stuff but at least for running the check itself.

[asottile]

I'd rather not expose the internals / implementation details. if pre-commit produces that output it will end up that people depend on it and muck around with the environments (already saw this happen when people were unhappy when I moved and renamed the internal repositories)

[webknjaz]

I'm mostly talking about the entrypoint and the rendered args.

[asottile]

the identity hook was created to debug that if needed (it spits out all positional arguments)

but note that the argument pattern is documented and without special cases entry *args *filenames

[webknjaz]

Yes, but sometimes it feels like it doesn't really do what's documented and you really want to see some confirmation.

[asottile]

so you don't trust it :P

[webknjaz]

Sort of. It's more of I don't trust myself. It's like debug output in any other software.

[asottile]

sure, then the debugging tool provided is the identity hook

[webknjaz]

But that requires me to change the hook entry I'm testing, right?

[asottile]

yes

[webknjaz]

Then it's unusable if you want to test how some third-party hook behaves.

[asottile]

how so? you need to set files / types / args to the same values and then it will work identically

[webknjaz]

Because it's a lot of mental overhead here compared to typical -vvv in most of the tools. I don't want to go to some hook's remote and copy-paste stuff from their config. I shouldn't have to do all of these things if I just want to figure out a few details about what's going on in the current setup.

[revolter]

Wait, from what I understood, the answer to

But that requires me to change the hook entry I'm testing, right?

should have been "no".

You don't need to change the hook, in order to use identity. You merely have to add a separate hook with the id identity, and the same options that you specified for the original hook.

For example, if you have this .pre-commit-config.yaml:

repos:
-   repo: <repo_url>
    hooks:
    -   id: <hook_id>
        files: >
            (?x)(
                .+\.m$
            )
        args:
            - --argument
            - 'value'

you only need to add:

-   repo: meta
    hooks:
    -   id: identity
        files: >
            (?x)(
                .+\.m$
            )
        args:
            - --argument
            - 'value'

to the end of it.

I didn't test this though, but if I am correct, it means that you also don't have to

go to some hook's remote and copy-paste stuff from their config

[asottile]

yep

though identity is part of repo: meta so it needs a few lines in the middle but otherwise correct

[revolter]

I updated it.

[jenstroeger]

Seems there hasn't been any renewed interest in this, going to close for now

@asottile regarding your comment above — this was never implemented, correct?

I could need this option to debug an issue where mypy run by the hook behaves differently than mypy run manually in that same terminal. I presume the two mypy runs differ either in setup or environment, but that’s hard to chase down without getting some insight into what pre-commit does when the hook runs 🤔

[asottile]

try reading the note in the readme

[vergenzt]

I wish there were more willingness to have this added. 🙁 I'm interested in this as well.

I'd rather not expose the internals / implementation details. if pre-commit produces that output it will end up that people depend on it and muck around with the environments (already saw this happen when people were unhappy when I moved and renamed the internal repositories)

Could the solution to this be to be explicit in the docs about what is considered an "implementation detail" vs what can be relied upon? I feel like it's very reasonable to say "the details about runtime environment locations and configs may change from release to release".

Some details (in no particular order) about my use case for wanting this right now:

• I've been developing some shell scripts in a repo.
• I've been using a VSCode plugin to run https://github.com/koalaman/shellcheck on my scripts and help me check for bugs.
• I'm now trying to add this check to a CI build (alongside some other checks) and I decided I'd like to use pre-commit.
• I found https://github.com/koalaman/shellcheck-precommit, and added it an initially otherwise-empty .pre-commit-config.yaml file:

  repos:
  - repo: https://github.com/koalaman/shellcheck-precommit
    rev: v0.8.0
    hooks:
    - id: shellcheck
  

• When I run pre-commit -a I get told that Shellcheck passed. Yay! 😄
• I introduce something I thought Shellcheck would complain about. (Using an $env_var without surrounding it with double quotes.)
• Shellcheck still says it passes. ☹️
• I want to figure out what commands Shellcheck is running and make sure it's not somehow filtering out the file I'm looking at.
• Can't figure out any way to assert that my file is getting included! 🙁 It sounds like this issue is suggesting that I have to go look at https://github.com/koalaman/shellcheck-precommit, know that I have to look at its .pre-commit-hooks.yaml file to get its config, see that it's not just a list of globs but a semantic name for a filetype (meaning I can't tell at a glance if shebang'd files that aren't named *.sh are included), paste that config into a new - { repo: meta, hooks: [ id: identity, ...<paste_config_here> ] } entry in my local .pre-commit-config.yaml, then run pre-commit run identity?

I wish that instead of all that I could just do pre-commit --debug or something and see shellcheck <filename> entries on stderr or stdout.

EDIT: I finally just went through and executed those instructions and got the following:

An error has occurred: InvalidConfigError: 
==> File .pre-commit-config.yaml
==> At Config()
==> At key: repos
==> At Repository(repo='meta')
==> At key: hooks
==> At Hook(id='identity')
==> At key: language
=====> Expected one of system but got: 'docker_image'
Check the log at /Users/.../.cache/pre-commit/pre-commit.log

😭

[asottile]

yeah for the identity hook you remove the language and it'll do what you want -- if you override language that breaks the implementation of identity

[asottile]

btw I'd recommend https://github.com/shellcheck-py/shellcheck-py since it doesn't use docker and tends to work better and faster