From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x42d.google.com (mail-wr1-x42d.google.com [IPv6:2a00:1450:4864:20::42d]) by sourceware.org (Postfix) with ESMTPS id 7B71A3835806 for ; Wed, 7 Jul 2021 22:15:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7B71A3835806 Received: by mail-wr1-x42d.google.com with SMTP id u8so4818341wrq.8 for ; Wed, 07 Jul 2021 15:15:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=lshI2rpPSZjc9c7JI76KIZIiXoDeI/aZJav4pHxo+w0=; b=akoB1BoL6TrThv2rowdH+QMSTluLsVpnB6xjzDtQTzhfPLk7WouNYYKmFXVG7wW/OR EaGKo2NUpmnyelrgVG23PxqkLgO1cc9K/DVwpdkw7DYAWYRpHsY5ZA7LEidYl6IZZg7c boPDwgB3KKhtfeJXnMwoYktjmGsXR6Hs+UbIzyFSVDKoFtUBLe03t6Xua3xgu/MLgAeP sMDMl74XJQWeIDSj5npoxI/9ul+kwOZVoS+6k+FeiJVB2NDLF6FB6XO9k7qa5N6GDQfR ErtSh7WvK0eUw5WNrN+FzH8JjY4jPfvVH1UVK7bZPd3SaT8fiwDKjKJRxmwwoZ01BGSp p4UA== X-Gm-Message-State: AOAM532cpGdiMGg2khXT/KAf3lcqExr8ZmqbAiDcGPF2zRon0UiyzBUu 3wk1DCzkwu8VR/fqmbHIOO4NFO84WzlzLJrjgZI= X-Google-Smtp-Source: ABdhPJxGjo2wZHkHDiLglp6h/yavPabz+4xNo2a9aTesBLOW5qIr8wlN6ifUn67FQ8+ZUJTd2BEnJZcsvRgGZFFWOuU= X-Received: by 2002:a5d:6986:: with SMTP id g6mr29722565wru.321.1625696158064; Wed, 07 Jul 2021 15:15:58 -0700 (PDT) MIME-Version: 1.0 References: <9121724e-e741-9bad-a39d-d6ac49422589@gmail.com> In-Reply-To: From: Jonathan Wakely Date: Wed, 7 Jul 2021 23:15:46 +0100 Message-ID: Subject: Re: where is PRnnnn required again? To: Martin Sebor Cc: Marek Polacek , gcc mailing list X-Spam-Status: No, score=-1.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, FREEMAIL_REPLY, HTML_MESSAGE, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Jul 2021 22:16:02 -0000 On Wed, 7 Jul 2021, 22:35 Martin Sebor, wrote: > On 7/7/21 2:42 PM, Jonathan Wakely wrote: > > > > > > On Wed, 7 Jul 2021, 17:39 Martin Sebor, > > wrote: > > > > On 7/6/21 4:09 PM, Jonathan Wakely wrote: > > > > > > > > > On Tue, 6 Jul 2021, 22:45 Martin Sebor via Gcc, > > > > >> wrote: > > > > > > On 7/6/21 3:36 PM, Marek Polacek wrote: > > > > On Tue, Jul 06, 2021 at 03:20:26PM -0600, Martin Sebor via > > Gcc wrote: > > > >> I came away from the recent discussion of ChangeLogs > > requirements > > > >> with the impression that the PRnnnn bit should be in the > > subject > > > >> (first) line and also above the ChangeLog part but > > doesn't need > > > >> to be repeated again in the ChangeLog entries. But my > commit > > > >> below was rejected last Friday with the subsequent > > error. Adding > > > >> PR middle-end/98871 to the ChangeLog entry let me push > > the change: > > > >> > > > >> > > https://gcc.gnu.org/g:6feb628a706e86eb3f303aff388c74bdb29e7381 > > > >> > > > >> I just had the same error happen now, again with what > > seems like > > > >> a valid commit message. Did I misunderstand something or > has > > > >> something changed recently? > > > >> > > > >> Martin > > > >> > > > >> commit 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780 (HEAD -> > > master) > > > >> Author: Martin Sebor > > >> > > > >> Date: Fri Jul 2 16:16:31 2021 -0600 > > > >> > > > >> Improve warning suppression for inlined functions > > [PR98512]. > > > >> > > > >> Resolves: > > > >> PR middle-end/98871 - Cannot silence > > -Wmaybe-uninitialized at > > > >> declaration si > > > >> te > > > >> PR middle-end/98512 - #pragma GCC diagnostic ignored > > > ineffective in > > > >> conjunct > > > >> ion with alias attribute > > > > > > > > This should be just > > > > > > > > PR middle-end/98871 > > > > PR middle-end/98512 > > > > > > > > , no? > > > > > > Does it matter if there's text after the PR ...? > > > > > > > > > > > > Yes. With extra text the whole line is just treated as arbitrary > > text, > > > not a "PR component/nnnn" string. So with the extra text it won't > be > > > added to the ChangeLog file, and won't match the PR in the > > subject line. > > > > > > I managed to push > > > > > > https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350316.html > > > > > > that uses the same style earlier today > > > > > > > > > But will it add the PR numbers to the ChangeLog? I think the > > answer is > > > no (in which case you could edit the ChangeLog tomorrow if you > > want them > > > to be in there). > > > > It updated Bugzilla but it didn't add the PR numbers to the ChangeLog > > entries. I still don't (obviously) understand the rules the hook > uses > > for what to update or the rationale for them. It seems as though > > the PR in the subject is used to update only Bugzilla but not also > > update the ChangeLogs (why not?) > > > > > > Because they are two completely separate processes. Verifying the commit > > message format is done by a git hook, and you can run exactly the same > > checks locally before pushing a commit. > > > > Updating bugzilla is done by a separate and different process, which has > > been in place for years (decades?) before we switched to git. > > I don't mean to turn this into a contentious back and forth but > "because this is how it works" or "because this is how it's been > done for eons" aren't a rationale, at least not a satisfying one. > You didn't ask for rationale, you asked about the rules used and why you got that result. The reason is that "find the PR for the ChangeLog" and "find the PR for bugzilla" are done separately by different rules. And that's for good reasons. We want to update bugzilla for any mention of a PR in the commit message, but we only want to list a PR in the ChangeLog if it's actually what the commit directly addresses. So we need to be able to distinguish "mentions it in some way, add a comment to bugzilla" from "really addresses this PR, use it in the ChangeLog". For example, in a commit log I might say "as discussed in the comments of PR 5678, this does blah blah" and that deserves to add a comment to that PR, but not to use that PR in the ChangeLog file. So there can't be only one rule. Your discussion seems to all be centred around the "really addresses this PR" case. While improving that might be useful, you need to be conscious of the other case too. Two different use cases are hard to address with one rule. > Do you not agree that it would be better to be able to mention > the PR or PRs just once and have all our scripts work with it? > Is that possible though? Obviously "everything is simple and works perfectly" sounds good in theory. Can you do it? If you do then is something keeping us from making those changes? > The usual. Somebody needs to patch the scripts and the docs. I'm happy enough with the way it works now to not change anything (and I'm not going to try to change those docs again). > Martin > > PS To be clear, I'm suggesting that all these work the same and > update Bugzilla as well as ChangeLogs, both with and without > a space after PR and both with and without a component name after > the PR. > 1) PR only in title. > Fix foobar [PR12345] > > gcc/ChangeLog: > * foo.c (bar): Fix it. > > 2) PR (with or without additional text after it) after title and > before ChageLogs. > Fix foobar. > > PR12345 - foobar broken > > gcc/ChangeLog: > * foo.c (bar): Fix it. > > 3) PR only in ChangeLogs. > Fix foobar. > > gcc/ChangeLog: > PR 12345 > * foo.c (bar): Fix it. > Now add a descriptive commit message and consider how you want it to work if some sentence in the middle of that message mentions a PR in some context other than fixing that PR. e.g. Fix foobar. The fix for PR 9999 caused a bug, which was reported as PR 12345. The fix is similar to how PR 8888 was fixed. gcc/ChangeLog: PR 12345 * foo.c (bar): Fix it. This will (I believe) add a comment to three bugzilla reports, but only use 12345 in the ChangeLog file. And I think that's exactly what we want to happen. You need some rule for finding the important PR which is not the same as the rule used for bugzilla. By all means improve the rule, but I don't see how you can have one rule for all purposes. As I've previously suggested on this mailing list, and Jason suggested again today, we could allow this to update the ChangeLog: "\tPR12345 - foobar broken" which currently isn't matched, because there is text after the number. That does update bugzilla, because that rule doesn't care about text before or after the number. This seems to be what's confusing you. You have used the wrong format for the ChangeLog pattern, and wondered why it can still update bugzilla. The answer is that the bugzilla update is less picky, for the reasons above.