|
From: | Jim Porter |
Subject: | Re: New Git hooks for checking file names in commit messages |
Date: | Sun, 23 Apr 2023 12:15:37 -0700 |
More generally, if people find these hooks to be annoying or unhelpful, I truly won't be offended if they get backed out. I wrote them in the hopes that it will make it easier to maintain Emacs, but if they can't meet that goal, then they shouldn't stick around causing problems.
On 4/23/2023 12:37 AM, Eli Zaretskii wrote:
Date: Sun, 23 Apr 2023 00:07:49 -0700 Cc: bjorn.bidar@thaodan.de, arsen@aarsen.me, acm@muc.de, emacs-devel@gnu.org From: Jim Porter <jporterbugs@gmail.com> Ok, it should be easy to restrict the pre-push hook to master/emacs-NN branches. ...It's a compromise (ideally, other mistakes in file names should also be detected), but I think it's the best we can do without too many false positives.
Ok, done.
This part should be easy enough to handle: if a commit is a merge commit, we just won't validate any file names listed in the log message.This is not what I meant, though. If a merge-commit does have a meaningful log message, its file names should be validated.
It turns out this is actually very easy to do, now that I've learned about the "--first-parent" option in Git. Using that on a merge commit, I can get a list of all the changes that were merged into the target branch (i.e. everything from the feature branch that's now on, say, master).
With this fix, commit 289b457cac1 (the merge of 'abort-redisplay') now passes the tests, as it should.
Suppose I make commits A, B, and C on a branch, and then merge to master with merge-commit D. Does your message mean that the only commit that ends up in the ChangeLog file is commit D? (Or that commit D is the only one that authors.el looks at?)The only one that it is reasonable to validate is commit D, yes. The other commits should not be rejected due to their log messages, at least not ideally.
I believe "--first-parent" also solves this case. In particular, by using this option, the hook correctly ignores the commits from the Eglot branch that were merged in. This means that any merge like that in the future won't run afoul of these hooks.
I'll have to do a bit more testing here to try out other edge cases, but this should only result in false negatives, not false positives. (False negatives would just require some fixup when making the tarball.)
[Prev in Thread] | Current Thread | [Next in Thread] |