Re: New Git hooks for checking file names in commit messages

[Eli Zaretskii]

> 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>
> 
> On 4/22/2023 11:11 PM, Eli Zaretskii wrote:
> > The risk exists, but the freedom we give developers with log messages
> > on feature branches is more important.  We should not make developing
> > features more troublesome by enforcing well-formatted log messages for
> > WIP changes, especially since frequently the commits are just
> > checkpoints, they don't signify any changes that have a useful
> > description.  So we should not block pushing of branch commits due to
> > log messages.
> 
> Ok, it should be easy to restrict the pre-push hook to master/emacs-NN 
> branches. However, just to make sure we're on the same page: the *only* 
> errors that committers will ever see from these hooks is if they listed 
> a file in GNU Change Log format that wasn't in the diff. Anything else 
> is allowed, even a message as simple as, "Do something".

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.

> > Merge commits usually don't have any meaningful log messages and don't
> > mention files.  You must explicitly add a log message mentioning files
> > and functions for a merge-commit to have a log message, and we request
> > that people who land feature branch create a log message describing
> > all the new/changed things in the merge for that single commit before
> > you push.  Most other merge-commits aren't important in the context of
> > this discussion.
> 
> 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.  When a
feature branch is landed on master, we request that all the important
changes that the branch brings be described in the log message of the
merge-commit.  So it shouldn't be ignored.

> > The commit log message which matters for those is the one of the
> > merge-commit when the branch is landed on master.
> 
> 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 was looking for examples of merge commits to test against and found 
> 289b457cac1, merging the 'abort-redisplay' branch. That's in ChangeLog.4 
> on the emacs-29 branch, as expected. However, I also see commits that 
> appear to be from that branch in the change log too, such as 
> 4b00bc47c7e. I don't see any logic in authors.el to ignore these commits 
> either. Am I just misunderstanding something?

No, there's no such logic.  But rejecting commits on a feature/scratch
branch due to this would be too much of an annoyance, so we shouldn't
do that.  Whoever runs authors.el will have to deal with this.  (My
recommendation to developers who work on branches is not to use
ChangeLog-style file name references in branch commits that aren't
important enough to appear in the generated ChangeLog files.)