Re: where is PRnnnn required again?

[Martin Sebor <msebor@gmail.com>]

On 7/7/21 4:15 PM, Jonathan Wakely wrote:
> 
> 
> On Wed, 7 Jul 2021, 22:35 Martin Sebor, <msebor@gmail.com 
> <mailto:msebor@gmail.com>> wrote:
> 
>     On 7/7/21 2:42 PM, Jonathan Wakely wrote:
>      >
>      >
>      > On Wed, 7 Jul 2021, 17:39 Martin Sebor, <msebor@gmail.com
>     <mailto:msebor@gmail.com>
>      > <mailto:msebor@gmail.com <mailto:msebor@gmail.com>>> wrote:
>      >
>      >     On 7/6/21 4:09 PM, Jonathan Wakely wrote:
>      >      >
>      >      >
>      >      > On Tue, 6 Jul 2021, 22:45 Martin Sebor via Gcc,
>     <gcc@gcc.gnu.org <mailto:gcc@gcc.gnu.org>
>      >     <mailto:gcc@gcc.gnu.org <mailto:gcc@gcc.gnu.org>>
>      >      > <mailto:gcc@gcc.gnu.org <mailto:gcc@gcc.gnu.org>
>     <mailto:gcc@gcc.gnu.org <mailto:gcc@gcc.gnu.org>>>> 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 <msebor@redhat.com
>     <mailto:msebor@redhat.com>
>      >     <mailto:msebor@redhat.com <mailto:msebor@redhat.com>>
>     <mailto:msebor@redhat.com <mailto:msebor@redhat.com>
>      >     <mailto:msebor@redhat.com <mailto:msebor@redhat.com>>>>
>      >      >      >> 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