Make is_revert stricter, to match GCCs changelog scripts

[ArsenArsen]

Reverts like
https://gcc.gnu.org/cgit/gcc/commit/?id=eec8da328cf1f91db302ab4cee803e269e68ad33 are let through by the hooks, but then lead to failures in the changelog generating script:
https://inbox.sourceware.org/gccadmin/20260415001632.71DF64BA23C4@sourceware.org/

This check should prevent that.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[brobecke]

Hello @ArsenArsen ,

First of all, thank you for taking the time to propose a fix! And great bonus points for providing a testcase as well.

I went through the commit message, then followed the links to try to understand what concretely was the problem, and also looked at the commit. IIUC, the problem is that the title of the revert got split into two lines, thus making the second line not an empty line? That's what the error message seems to be saying, but the comments in your change seems to be saying different (actually, I don't understand the comment in the new function reject_almost_reversions). There's also the error about a lack of changelog, but there's no changelog entry in the commit you pointed out.

Before going further with this, and looking at my current understanding of your patch, I think this is the situation where we need properly and clearly document the extent of the problem we are trying to solve, and have a discussion about how we plan on addressing the need together with a rationale of why we select one option vs the other options. What this tells me is that we need a proper issue for this. In the issue description, I would like to avoid external links and instead provide the necessary information in place: The commit message, how that commit-message was generated (whether 100% automatically, or edited manually), and a description of the problem that this commit message caused later on that doesn't require the analysis of a program's output.

With that said, if I go by the contents of the patch rather than the script error, I think you want to catch cases where people alter a specific line in revert commits, as otherwise this affects one of GCC's production scripts.

If my understanding is correct, I would argue that your need is quite specific to your project, and perhaps the right solution is to implement this check using the hooks.commit-extra-checker , which was designed for answering this kind of project-specific requirements.

[ArsenArsen]

Hi Joel,

I went through the commit message, then followed the links to try to understand what concretely was the problem, and also looked at the commit. IIUC, the problem is that the title of the revert got split into two lines, thus making the second line not an empty line? That's what the error message seems to be saying, but the comments in your change seems to be saying different (actually, I don't understand the comment in the new function reject_almost_reversions). There's also the error about a lack of changelog, but there's no changelog entry in the commit you pointed out.

The messages printed are a red herring; the codepath that generated those warnings shouldn't have been taken because the commit should've been detected as a revert. The log is generated by a script not in git-hooks.

Said script relies on the hooks validating all commits that get pushed.

So, the actual issue is that the commit wasn't blocked on push by the hooks.

This happened because the commit is a revert.

Of course, the scripts know what a reversion is also, but they are much more strict than git-hooks: revert_regex = re.compile(r'This reverts commit (?P<hash>[0-9a-f]+)\.$')

Taking a step back, when git revert is used, it generates a description of this form:

commit 015813e3573be0d1aec92b2936d18ac972d8fd8a (origin/master, origin/HEAD)
Author: Richard Biener <rguenther@suse.de>
Date:   Mon May 4 13:11:27 2026 +0200

    Revert "tree-optimization/120003 - missed jump threading"
    
    This reverts commit 1a13684dfc7286139064f7d7341462c9995cbd1c.

(picked at random, just what I happened to have cloned locally)

... however, some people alter the This reverts commit line, making a commit message like:

commit 015813e3573be0d1aec92b2936d18ac972d8fd8a (origin/master, origin/HEAD)
Author: Richard Biener <rguenther@suse.de>
Date:   Mon May 4 13:11:27 2026 +0200

    Revert "tree-optimization/120003 - missed jump threading"
    
    This reverts commit 1a13684dfc7286139064f7d7341462c9995cbd1c, because foo.

However, git-hooks consider any commit with This reverts commit in the message a revert commit, and lets any such commit bypass checks, even if they alter that line.

Ergo, the assumption that hooks prevent any invalid commits from reaching the repo is broken.

The intention is to make this exemption case stricter (by making it match This reverts commit <hex string>. specifically, which is what scripts expect).

If my understanding is correct, I would argue that your need is quite specific to your project, and perhaps the right solution is to implement this check using the hooks.commit-extra-checker, which was designed for answering this kind of project-specific requirements.

I think the is_revert change needs to be done here anyway; AFAICT (but I may be wrong), if is_revert, then any and all checks are skipped, including the per-project extra checker.

The new rh-near-revert-check that was added is merely there to provide some feedback when a commit isn't a reversion, but it would've been considered one before.

That could be moved into the GCC-specific bits if so desired, but I figured it'd be more broadly useful.

If I'm wrong about is_revert skipping also the extra checks, then this can indeed all be done downstream instead.

I hope that clarifies it.

WRT the comments and docstrings, if we decide to do this change upstream, I'll reword them in a way that relies on GCC context less - sorry about that, I'm not 100% clear on where these hooks end and GCC-specific bits start, so it seems that I guessed wrong.

[brobecke]

Thanks for the clarifications @ArsenArsen . And also for pointing out that all checks for revert commits are disabled.

The documentation is not completely clear about whether this also disables the commit-extra-checker but a cursory read of the code confirms that the project-specific-checker is also bypassed for revert commits. Therefore, the solution I proposed doesn't work.

We can discuss possible ways forward, but for that let's create an issue so that everything is properly tracked for posterity. On the other hand, for GCC's issue at hand, I'm wondering why it's such a bad thing that people add extra information in the commit message, especially when that extra information doesn't take away information generated by git revert. Have you considered the idea of the post-production script being a bit more flexible? (don't get me wrong, not trying to avoid changes in git-hooks, and in fact, we can do both -- more flexibility in your post-production script and enhancing the hooks to allow for more specific detection as needed).

[ArsenArsen]

I'll write up a summary in an issue tomorrow.

On:

On the other hand, for GCC's issue at hand, I'm wondering why it's such a bad thing that people add extra information in the commit message, especially when that extra information doesn't take away information generated by git revert.

That's fine, and good, of course, as long as the commit contains the unaltered "This reverts commit" line (see https://gcc.gnu.org/cgit/gcc/commit/?id=e1f2d0d3e6018510a002ad8b3c0dfeb2ac7ec5ca for an example; notice the quite complete description followed by the Git-generated "This reverts commit" line).