From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x332.google.com (mail-ot1-x332.google.com [IPv6:2607:f8b0:4864:20::332]) by sourceware.org (Postfix) with ESMTPS id 44A4E3972035 for ; Wed, 7 Jul 2021 23:38:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 44A4E3972035 Received: by mail-ot1-x332.google.com with SMTP id i12-20020a05683033ecb02903346fa0f74dso3998234otu.10 for ; Wed, 07 Jul 2021 16:38:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=F0PnOGj/QvEXSJLlosu3zG8iV4StXnA1pZIFlGQI+n0=; b=DPX1ZzDYCSXHWWpQbOztikpld52t4f3bjx2ElFyy2cgrqUb1vus6s7WPO8VG+k2LCl Li3WxM3WplNT2k7XRreDVF1TCK97rVCkJrD06NSMOP8KwIkci5oY/HYCoyb21wCHSFs7 tp9qy7ZaUSb44C6T8UHko6wwzxE9qfUWR+kR6nUEv6xcFewOAvDQVBZNH3Dr5Dp2/oue 0pjZA91R9uaOafVrFalYDHIfvOu8rRkb9ESJoywV0zwFYac1vPZS7HH59Z4pSphfqsJx t1AN+xPF4UWCiagwT3xZCVB3M50PGEpvvjg3+KOJNQhRMlt7Mg1ga5xLoSj0CehNFKug mecA== X-Gm-Message-State: AOAM5318ewNuQw8y5IlfrbLvEGdtX+NQEED3Rdl8iYLMmc25E5FUfrv4 Xylx26QrIcQoqXr4TQe4V2PE6x5jsnc= X-Google-Smtp-Source: ABdhPJyZN7bTu5GFgYFNFYDe5Rbm5i2Vg2BIAPfmsvXncDlwSJxEc7fqixbrIs/LhTAzVxpwKQiEsg== X-Received: by 2002:a9d:8c7:: with SMTP id 65mr21816114otf.25.1625701112366; Wed, 07 Jul 2021 16:38:32 -0700 (PDT) Received: from [192.168.0.41] (75-166-102-22.hlrn.qwest.net. [75.166.102.22]) by smtp.gmail.com with ESMTPSA id r18sm72693ooe.23.2021.07.07.16.38.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 07 Jul 2021 16:38:32 -0700 (PDT) Subject: Re: where is PRnnnn required again? To: Jonathan Wakely Cc: Marek Polacek , gcc mailing list References: <9121724e-e741-9bad-a39d-d6ac49422589@gmail.com> From: Martin Sebor Message-ID: Date: Wed, 7 Jul 2021 17:38:31 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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 23:38:35 -0000 On 7/7/21 4:15 PM, Jonathan Wakely wrote: > > > 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". That's fine, what I'm proposing wouldn't change that. > > 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. No, my suggestion is to handle the three cases in this email: https://gcc.gnu.org/pipermail/gcc/2021-July/236696.html > 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? Probably. But before I invest time and effort into it I want to know that there isn't something fundamental that prevents it or that one of you won't shoot it down. > 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. Same as today. I'm not proposing to change that. > >    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. That's fine with me. > > 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. Yes, that would be good. It would handle my case (2). > > 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. Right. Martin