[CI] broken commit message length test

[lierdakil]

So, okay, this isn't easy to fix, because GitHub creates a merge commit on pull requests. See CI output on the last commit.

[jgm]

I tried another approach (commit fdf3481).
(Just excluding merge commits from the git log output.)

[lierdakil]

Okay, this should work. I've also made the check a separate job, because I don't think it's a good idea to stop most of the pipeline due to an overly long commit message. CI overall will still fail, but other tests will run also.

P.S. side note, tested that this works here: https://github.com/jgm/pandoc/runs/706874492?check_suite_focus=true

[jgm]

I like your version better than mine! Can you rebase it on master; I got my change in first and it conflicts.

[lierdakil]

Sure, rebased.

[jgm]

To address the issue of multiple commits, would it work to do
git log master..HEAD [other options as before]?

[lierdakil]

It'd work for PRs against the master branch probably. Not sure if that'd work for direct commits to the master branch, and it wouldn't work for PRs against non-master branches (which are rare, but not unthinkable).

So, probably, not the best idea.

That's not to say it wouldn't be possible, but implementation details elude me at the moment.

[lierdakil]

Not entirely sure if it's doable in bash, but a combination of approaches used in
https://github.com/GsActions/commit-message-checker/blob/master/src/input-helper.ts
and
https://github.com/ad1992/clean-commit-action/blob/master/index.js
would probably yield a robust enough solution.

I could probably slap together something workable closer to the end of this week (have stuff to do in the next couple days, so can't really start hacking straight away)

[lierdakil]

Update, apparently, master..HEAD doesn't work at all, GitHub gives me fatal: bad revision 'master..HEAD' on push. https://github.com/lierdakil/actions-test/runs/707299669?check_suite_focus=true

[lierdakil]

Here's what I figured out so far.

On push, it's actually rather simple, we can use ${{github.event.before}}..${{github.event.after}} as the range. before is the last commit on the branch before the push, and after is the last commit in the push. Since after is the current HEAD this range can be simplified to ${{github.event.before}}..

It's possible to do the same on pull request, but there's a catch: there's no before or after when the PR is opened, only when it's updated -- so need to get a little bit more crafty. With pull requests we do have ${{github.head_ref}} and ${{github.base_ref}} representing the PR branch and the base branch (the one PR is against), however, simple ${{github.base_ref}}..${{github.head_ref}} fails with fatal: bad revision error due to GitHub not creating actual refs for those in the checked-out repo. It does, however, create refs called origin/<base branch> and pull/<pr number>/merge, which can be referenced. Hence, the range origin/${{github.base_ref}}..pull/${{github.event.number}}/merge or simply ``origin/${{github.base_ref}}..` should do the trick (with the caveat that it will check all commits in the PR, not just the most recently pushed, but that's kinda what we want anyway I guess).

Hence, this all becomes rather manageable -- the only trick is that we need to define the base for log conditionally based on ${{github.event_name}}.

I'll try to slap something together on my test repo, and will copy the result to this PR after some tests.

[lierdakil]

Okay, after testing for a bit, I found that there is a slight problem with the push hook. Namely, it fills before with 40 zeros on new branches. It does provide the list of the last 20 new commits pushed though, so that makes it possible to find the parent if the branch is initially small enough, but for large new branches this will skip over some commits.

Arguably, the point is moot anyway, because I guess we're primarily interested in checks on PRs, and not direct pushes (?). To this effect, I've actually opted to move the check into a separate workflow (so that we can control when it triggers independently of the rest of CI), and only enabled it on pull requests for now (without ignores mind).

The code itself supports both push and pull_request events, so if you'd like to run this on pushes also, feel free to add the push event to the activation hooks (or tell me to add it). Bear in mind the code might skip some commits on newly-created branches as outlined above. Those will still be caught if a PR is created for that branch at a later point. This won't be an issue if we only run this check on pushes to the master branch.

P.S. Actually, for new branches it should be possible to git log all commits on the current branch that are not on any other branch, but it gets pretty complicated pretty fast (i.e. need to list all branches on the command line), so it's an open question whether it's worth it or not.

P.P.S. I've spent way more time on this than was reasonable, but I believe I've got it, more or less. One caveat is that commits on push will only be marked as failing once, but I would argue that's preferable.

[jgm]

Great! Yes, this is one of those things that gets out of hand, because it seems like it will be simple, but it isn't.