From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: New Git hooks for checking file names in commit messages Date: Sun, 23 Apr 2023 10:37:43 +0300 Message-ID: <83pm7vyrp4.fsf@gnu.org> References: <838rel46ou.fsf@gnu.org> <865y9ptfa3.fsf@aarsen.me> <83354t44kn.fsf@gnu.org> <875y9p2h3u.fsf@thaodan.de> <83h6t82yao.fsf@gnu.org> <0a76fcd4-df69-56e0-ba90-30dc211ad56e@gmail.com> <83v8hnyvou.fsf@gnu.org> <10f5fa75-f076-228d-5ad7-1aaba8b98d47@gmail.com> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="15086"; mail-complaints-to="usenet@ciao.gmane.io" Cc: bjorn.bidar@thaodan.de, arsen@aarsen.me, acm@muc.de, emacs-devel@gnu.org To: Jim Porter Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Apr 23 09:38:12 2023 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pqUIW-0003mg-NG for ged-emacs-devel@m.gmane-mx.org; Sun, 23 Apr 2023 09:38:12 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pqUHp-0006qz-0V; Sun, 23 Apr 2023 03:37:29 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pqUHn-0006pY-AR for emacs-devel@gnu.org; Sun, 23 Apr 2023 03:37:27 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pqUHl-00032k-4W; Sun, 23 Apr 2023 03:37:25 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=nItDeLRLEgDDx+nuOIM/FqlPVezz/SJ06K9TubmfLaI=; b=Fk0p+QvR/L8W vBkU52aJjPJYsJVAVUSx11hoBnBeh9dYcQCP2HG6pIIOEZTSiqunQQtWdcfDJwWv0Skt3ftVFtxeS Cs6yLW3ejdHoawJH1hSO/NtLesSAzPq72YuKLjS8zFWKsT9g7pqwthzd7K6FoLJ9LiquakE+Y7daq RWHSQX/BCJOiKo8vAodQZzZZt0DhWBggY4+4rDfH9p6jIlGEpeCEUtx3w8+k7aR+Y3GltuwnJ56jd NBTRkLHgcrczYdjRzdDg4iU0/qwTdjiESrOeI8up1KdccQK9vdXyFeuhzE0y7h7RygsLv3fHritFc J/mowgXKAOavYft+Y7qQYA==; Original-Received: from [87.69.77.57] (helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pqUHk-0004Ch-6b; Sun, 23 Apr 2023 03:37:24 -0400 In-Reply-To: <10f5fa75-f076-228d-5ad7-1aaba8b98d47@gmail.com> (message from Jim Porter on Sun, 23 Apr 2023 00:07:49 -0700) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:305599 Archived-At: > 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 > > 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.)