pre-commit-ci / issues

public issues for https://pre-commit.ci
17 stars 4 forks source link

Commit message hook support #35

Open hbjydev opened 3 years ago

hbjydev commented 3 years ago

Currently, it doesn't look like this will run commit-msg stage hooks at all. We're trying to use it on rocky-linux/rockylinux.org as a compliance method for commit messages, and preferably use as few bots as possible.

asottile commented 3 years ago

We don't receive the commit messages at all currently so I don't know if this is possible without performance for every run suffering

hbjydev commented 3 years ago

@asottile Doesn't GitHub's push event include Git data?

hbjydev commented 3 years ago

Just trying to get my head around it is all

asottile commented 3 years ago

push does, but pull_request does not -- and to be a proper gating mechanism you'd need it in pull_request I assume

hbjydev commented 3 years ago

Ah, right. Perhaps there could be some logic to detect commit-msg hooks and if they're found, hit the API for those pushes? That way, the majority of use cases don't take a performance hit, because it's not hitting the API, and performance of this specific feature can be improved over time.

asottile commented 3 years ago

that's certainly possible, but it also brings up another problem: how far back does it go? this would be the api to hit: https://api.github.com/repos/python/cpython/pulls/23967/commits -- to prevent ~infinite work this should probably limit to some number of commits

running against commit messages after the fact is a bit tricky as well -- I haven't seen it done before but I imagine you could write out a file and then pre-commit run --hook-stage commit-msg --commit-msg-filename /some/tmp/file

hbjydev commented 3 years ago

Ah, of course. Maybe I could contribute a --commit-msgs-filename flag that would read multiple commit messages based on a delimiter like --- or something?

asottile commented 3 years ago

no need probably, xargs already exists

hbjydev commented 3 years ago

I'm not really familiar with how to use xargs, so I won't pretend to know how to implement it lol

hbjydev commented 3 years ago

I know we went through this on stream but I got the thing to work with xargs

https://gist.github.com/9f742fbac91f3133022e628143bf65d9

tykeal commented 3 years ago

I played with this some but here's what I found was actually needed to make this work and get a proper exit code on failure:

 curl -s -L $PR_API | jq .[].commit.message | xargs -I _ sh -c 'echo -e "_" > /tmp/cmsg; pre-commit run --hook-stage commit-msg --commit-msg-filename /tmp/cmsg'

The gist from @hbjydev added in -0 to the xargs which caused all of the commit messages to get jammed into a single file and not individually processed, additionally, the inclusing of the final rm in each of the iterations meant that xargs was seeing the final command as exiting 0 and thus never threw and error exit code back out.

It would be really nice if something like this could be added into the pre-commit-ci!