where is PRnnnn required again?

where is PRnnnn required again?

[Martin Sebor]

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>
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

     gcc/ChangeLog:

             * diagnostic.c (get_any_inlining_info): New.
             (update_effective_level_from_pragmas): Handle inlining context.
             (diagnostic_enabled): Same.
             (diagnostic_report_diagnostic): Same.
             * diagnostic.h (struct diagnostic_info): Add ctor.
             (struct diagnostic_context): Add new member.
             * tree-diagnostic.c (set_inlining_locations): New.
             (tree_diagnostics_defaults): Set new callback pointer.



Enumerating objects: 11, done.
Counting objects: 100% (11/11), done.
Delta compression using up to 16 threads
Compressing objects: 100% (6/6), done.
Writing objects: 100% (6/6), 3.37 KiB | 3.37 MiB/s, done.
Total 6 (delta 5), reused 0 (delta 0)
remote: *** The following commit was rejected by your 
hooks.commit-extra-checker script (status: 1)
remote: *** commit: 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780
remote: *** ChangeLog format failed:
remote: *** ERR: PR 98512 in subject but not in changelog: "Improve 
warning suppression for inlined functions [PR98512]."
remote: ***
remote: *** Please see: 
https://gcc.gnu.org/codingconventions.html#ChangeLogs
remote: ***
remote: error: hook declined to update refs/heads/master
To git+ssh://gcc.gnu.org/git/gcc.git
  ! [remote rejected]         master -> master (hook declined)
error: failed to push some refs to 
'git+ssh://msebor@gcc.gnu.org/git/gcc.git'

Re: where is PRnnnn required again?

[Marek Polacek]

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>
> 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?  Either here, or...

>     gcc/ChangeLog:

...here.
 
>             * diagnostic.c (get_any_inlining_info): New.
>             (update_effective_level_from_pragmas): Handle inlining context.
>             (diagnostic_enabled): Same.
>             (diagnostic_report_diagnostic): Same.
>             * diagnostic.h (struct diagnostic_info): Add ctor.
>             (struct diagnostic_context): Add new member.
>             * tree-diagnostic.c (set_inlining_locations): New.
>             (tree_diagnostics_defaults): Set new callback pointer.
> 
> 
> 
> Enumerating objects: 11, done.
> Counting objects: 100% (11/11), done.
> Delta compression using up to 16 threads
> Compressing objects: 100% (6/6), done.
> Writing objects: 100% (6/6), 3.37 KiB | 3.37 MiB/s, done.
> Total 6 (delta 5), reused 0 (delta 0)
> remote: *** The following commit was rejected by your
> hooks.commit-extra-checker script (status: 1)
> remote: *** commit: 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780
> remote: *** ChangeLog format failed:
> remote: *** ERR: PR 98512 in subject but not in changelog: "Improve warning
> suppression for inlined functions [PR98512]."
> remote: ***
> remote: *** Please see:
> https://gcc.gnu.org/codingconventions.html#ChangeLogs
> remote: ***
> remote: error: hook declined to update refs/heads/master
> To git+ssh://gcc.gnu.org/git/gcc.git
>  ! [remote rejected]         master -> master (hook declined)
> error: failed to push some refs to
> 'git+ssh://msebor@gcc.gnu.org/git/gcc.git'
> 

Marek

Re: where is PRnnnn required again?

[Martin Sebor]

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>
>> 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 ...?  I managed to push

   https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350316.html

that uses the same style earlier today but the one below failed just
a little while later.  Copying the PR tree-optimization/86650 part
into the ChangeLog entry fixed it (I didn't think to try without
the PR summary).

Enumerating objects: 10, done.
Counting objects: 100% (10/10), done.
Delta compression using up to 16 threads
Compressing objects: 100% (6/6), done.
Writing objects: 100% (6/6), 1008 bytes | 1008.00 KiB/s, done.
Total 6 (delta 4), reused 0 (delta 0)
remote: *** The following commit was rejected by your 
hooks.commit-extra-checker script (status: 1)
remote: *** commit: e8c83cb3d4824f85588d162e7aa58c25f8cb926b
remote: *** ChangeLog format failed:
remote: *** ERR: PR 86650 in subject but not in changelog: "Add test for 
[PR86650]."
remote: ***
remote: *** Please see: 
https://gcc.gnu.org/codingconventions.html#ChangeLogs
remote: ***
remote: error: hook declined to update refs/heads/master
To git+ssh://gcc.gnu.org/git/gcc.git
  ! [remote rejected]         master -> master (hook declined)
error: failed to push some refs to 
'git+ssh://msebor@gcc.gnu.org/git/gcc.git'
tmp$ (cd /src/gcc/master/ && git show 151b423a82f..e8c83cb3d48)
commit e8c83cb3d4824f85588d162e7aa58c25f8cb926b (HEAD -> master)
Author: Martin Sebor <msebor@redhat.com>
Date:   Tue Jul 6 15:15:53 2021 -0600

     Add test for [PR86650].

     PR tree-optimization/86650 - -Warray-bounds missing inlining context

     gcc/testsuite/ChangeLog:
             * gcc.dg/Warray-bounds-76.c: New test.

diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-76.c 
b/gcc/testsuite/gcc.dg/Warray-bounds-76.c
...

Martin

>  Either here, or...
> 
>>      gcc/ChangeLog:
> 
> ...here.
>   
>>              * diagnostic.c (get_any_inlining_info): New.
>>              (update_effective_level_from_pragmas): Handle inlining context.
>>              (diagnostic_enabled): Same.
>>              (diagnostic_report_diagnostic): Same.
>>              * diagnostic.h (struct diagnostic_info): Add ctor.
>>              (struct diagnostic_context): Add new member.
>>              * tree-diagnostic.c (set_inlining_locations): New.
>>              (tree_diagnostics_defaults): Set new callback pointer.
>>
>>
>>
>> Enumerating objects: 11, done.
>> Counting objects: 100% (11/11), done.
>> Delta compression using up to 16 threads
>> Compressing objects: 100% (6/6), done.
>> Writing objects: 100% (6/6), 3.37 KiB | 3.37 MiB/s, done.
>> Total 6 (delta 5), reused 0 (delta 0)
>> remote: *** The following commit was rejected by your
>> hooks.commit-extra-checker script (status: 1)
>> remote: *** commit: 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780
>> remote: *** ChangeLog format failed:
>> remote: *** ERR: PR 98512 in subject but not in changelog: "Improve warning
>> suppression for inlined functions [PR98512]."
>> remote: ***
>> remote: *** Please see:
>> https://gcc.gnu.org/codingconventions.html#ChangeLogs
>> remote: ***
>> remote: error: hook declined to update refs/heads/master
>> To git+ssh://gcc.gnu.org/git/gcc.git
>>   ! [remote rejected]         master -> master (hook declined)
>> error: failed to push some refs to
>> 'git+ssh://msebor@gcc.gnu.org/git/gcc.git'
>>
> 
> Marek
> 

Re: where is PRnnnn required again?

[Jonathan Wakely]

On Tue, 6 Jul 2021, 22:45 Martin Sebor via Gcc, <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>
> >> 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).

Re: where is PRnnnn required again?

[Martin Sebor]

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>> 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>>
>      >> 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?)  The PR component/nnnn part that's
supposed to come before the ChangeLog is used to update ChangeLog
entries but seems to be ignored if it's followed by any text (why?)
These unnecessary gotchas aren't documented anywhere.

Martin

Re: where is PRnnnn required again?

[Jonathan Wakely]

On Wed, 7 Jul 2021, 17:39 Martin Sebor, <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>> 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>>
> >      >> 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.


The PR component/nnnn part that's
> supposed to come before the ChangeLog is used to update ChangeLog
> entries but seems to be ignored if it's followed by any text (why?)
>

See Jakub's reply.


>

Re: where is PRnnnn required again?

[Martin Sebor]

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>> 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>>> 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>>>
>      >      >> 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.

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?
If you do then is something keeping us from making those changes?

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.

> 
> 
>     The PR component/nnnn part that's
>     supposed to come before the ChangeLog is used to update ChangeLog
>     entries but seems to be ignored if it's followed by any text (why?)
> 
> 
> See Jakub's reply.
> 
> 

Re: where is PRnnnn required again?

[Marek Polacek]

On Wed, Jul 07, 2021 at 03:35:35PM -0600, Martin Sebor 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>> 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>>> 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>>>
> >      >      >> 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.
> 
> 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?
> If you do then is something keeping us from making those changes?
> 
> 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.

The script would have to derive the component name from the PR number. 
That might a complication.

> 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.

Looks like the best variant to me (I agree that enabling "- <description>"
after the PR number would be good).
 
> 3) PR only in ChangeLogs.
>   Fix foobar.
> 
>   gcc/ChangeLog:
>     PR 12345
>     * foo.c (bar): Fix it.

I would be really unhappy with this one because I often look for PR numbers
in the GCC mailing list archives and so having those numbers in email subjects
helps tremendously.  Therefore, best if people continue putting the #s in
the subject.


I'm not sure why you keep hitting so many issues; git addlog takes care of
this stuff for me and I've had no trouble pushing my patches.  Is there
a reason you don't use it also?

Marek

Re: where is PRnnnn required again?

[Martin Sebor]

On 7/7/21 3:53 PM, Marek Polacek wrote:
> On Wed, Jul 07, 2021 at 03:35:35PM -0600, Martin Sebor 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>> 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>>> 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>>>
>>>       >      >> 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.
>>
>> 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?
>> If you do then is something keeping us from making those changes?
>>
>> 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.
> 
> The script would have to derive the component name from the PR number.
> That might a complication.

Right, it would have to get from Bugzilla.  The mklog.py script
has an option to do that (get both the PR title and component).

> 
>> 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.
> 
> Looks like the best variant to me (I agree that enabling "- <description>"
> after the PR number would be good).
>   
>> 3) PR only in ChangeLogs.
>>    Fix foobar.
>>
>>    gcc/ChangeLog:
>>      PR 12345
>>      * foo.c (bar): Fix it.
> 
> I would be really unhappy with this one because I often look for PR numbers
> in the GCC mailing list archives and so having those numbers in email subjects
> helps tremendously.  Therefore, best if people continue putting the #s in
> the subject.
> 
> 
> I'm not sure why you keep hitting so many issues; git addlog takes care of
> this stuff for me and I've had no trouble pushing my patches.  Is there
> a reason you don't use it also?

I probably have a completely different workflow.  Git addlog isn't
a git command (is it some sort of a GCC extension?), and what I put
in the subject of my emails is almost never the same thing as what
I put in the commit message.  I'm not suggesting people change their
habits, just that our tooling not unnecessarily penalize those of us
who dot things a little differently.

Martin

Re: where is PRnnnn required again?

[Jonathan Wakely]

On Wed, 7 Jul 2021, 23:18 Martin Sebor, <msebor@gmail.com> wrote:

> On 7/7/21 3:53 PM, Marek Polacek wrote:
> > I'm not sure why you keep hitting so many issues; git addlog takes care
> of
> > this stuff for me and I've had no trouble pushing my patches.  Is there
> > a reason you don't use it also?
>
> I probably have a completely different workflow.  Git addlog isn't
> a git command (is it some sort of a GCC extension?), and what I put
> in the subject of my emails is almost never the same thing as what
> I put in the commit message.


Why not? Why is it useful to write two different explanations of the patch?

Re: where is PRnnnn required again?

[Martin Sebor]

On 7/7/21 4:24 PM, Jonathan Wakely wrote:
> 
> 
> On Wed, 7 Jul 2021, 23:18 Martin Sebor, <msebor@gmail.com 
> <mailto:msebor@gmail.com>> wrote:
> 
>     On 7/7/21 3:53 PM, Marek Polacek wrote:
>      > I'm not sure why you keep hitting so many issues; git addlog
>     takes care of
>      > this stuff for me and I've had no trouble pushing my patches.  Is
>     there
>      > a reason you don't use it also?
> 
>     I probably have a completely different workflow.  Git addlog isn't
>     a git command (is it some sort of a GCC extension?), and what I put
>     in the subject of my emails is almost never the same thing as what
>     I put in the commit message. 
> 
> 
> Why not? Why is it useful to write two different explanations of the patch?

Sometimes, maybe.  I don't really think about it too much.  I'm not
the only one who does it.  But what bearing does what we put in
the subject of our patch submissions have on this discussion?

You may have one way of doing things and others another.  Yours may
even be better/more streamlined, I don't know.  That doesn't mean
our tooling should make things more difficult for the the rest of us.

Martin

Re: where is PRnnnn required again?

[David Malcolm]

On Wed, 2021-07-07 at 16:58 -0600, Martin Sebor via Gcc wrote:
> On 7/7/21 4:24 PM, Jonathan Wakely wrote:
> > 
> > 
> > On Wed, 7 Jul 2021, 23:18 Martin Sebor, <msebor@gmail.com 
> > <mailto:msebor@gmail.com>> wrote:
> > 
> >     On 7/7/21 3:53 PM, Marek Polacek wrote:
> >      > I'm not sure why you keep hitting so many issues; git addlog
> >     takes care of
> >      > this stuff for me and I've had no trouble pushing my
> > patches.  Is
> >     there
> >      > a reason you don't use it also?
> > 
> >     I probably have a completely different workflow.  Git addlog
> > isn't
> >     a git command (is it some sort of a GCC extension?), and what I
> > put
> >     in the subject of my emails is almost never the same thing as
> > what
> >     I put in the commit message. 
> > 
> > 
> > Why not? Why is it useful to write two different explanations of
> > the patch?
> 
> Sometimes, maybe.  I don't really think about it too much.  I'm not
> the only one who does it.  But what bearing does what we put in
> the subject of our patch submissions have on this discussion?

FWIW if you use a different subject line for the email as for commit
message, it makes it harder to find discussion about the patch in the
list archives.

> You may have one way of doing things and others another.  Yours may
> even be better/more streamlined, I don't know.  That doesn't mean
> our tooling should make things more difficult for the the rest of us.
> 
> Martin
> 

Re: where is PRnnnn required again?

[Jonathan Wakely]

On Wed, 7 Jul 2021, 23:58 Martin Sebor, <msebor@gmail.com> wrote:

> On 7/7/21 4:24 PM, Jonathan Wakely wrote:
> >
> >
> > On Wed, 7 Jul 2021, 23:18 Martin Sebor, <msebor@gmail.com
> > <mailto:msebor@gmail.com>> wrote:
> >
> >     On 7/7/21 3:53 PM, Marek Polacek wrote:
> >      > I'm not sure why you keep hitting so many issues; git addlog
> >     takes care of
> >      > this stuff for me and I've had no trouble pushing my patches.  Is
> >     there
> >      > a reason you don't use it also?
> >
> >     I probably have a completely different workflow.  Git addlog isn't
> >     a git command (is it some sort of a GCC extension?), and what I put
> >     in the subject of my emails is almost never the same thing as what
> >     I put in the commit message.
> >
> >
> > Why not? Why is it useful to write two different explanations of the
> patch?
>
> Sometimes, maybe.  I don't really think about it too much.  I'm not
> the only one who does it.  But what bearing does what we put in
> the subject of our patch submissions have on this discussion?
>


My failed attempt to clarify the docs on commit message formats recommended
using the same text for the commit message and email. If there's a good
reason to deviate from that, I'd like to know. Not that I plan to change
those docs again, it was a waste of time.

Also, you're proposing that PR numbers don't need to be in the subject
and/or that if it's in the subject it doesn't need to be in the body. Is it
just because "it's inconvenient for my current workflow" ? If it's in the
subject of the patch, why wouldn't you put it in the email subject too?

If writing two different descriptions of the patch by hand is liable to not
meet the formatting conventions, it seems like using existing automation
for creating the message (and reusing it for the email) might be worth
trying.

That doesn't mean we can't also improve the convention.

Re: where is PRnnnn required again?

[Martin Sebor]

On 7/8/21 2:26 AM, Jonathan Wakely wrote:
> 
> 
> On Wed, 7 Jul 2021, 23:58 Martin Sebor, <msebor@gmail.com 
> <mailto:msebor@gmail.com>> wrote:
> 
>     On 7/7/21 4:24 PM, Jonathan Wakely wrote:
>      >
>      >
>      > On Wed, 7 Jul 2021, 23:18 Martin Sebor, <msebor@gmail.com
>     <mailto:msebor@gmail.com>
>      > <mailto:msebor@gmail.com <mailto:msebor@gmail.com>>> wrote:
>      >
>      >     On 7/7/21 3:53 PM, Marek Polacek wrote:
>      >      > I'm not sure why you keep hitting so many issues; git addlog
>      >     takes care of
>      >      > this stuff for me and I've had no trouble pushing my
>     patches.  Is
>      >     there
>      >      > a reason you don't use it also?
>      >
>      >     I probably have a completely different workflow.  Git addlog
>     isn't
>      >     a git command (is it some sort of a GCC extension?), and what
>     I put
>      >     in the subject of my emails is almost never the same thing as
>     what
>      >     I put in the commit message.
>      >
>      >
>      > Why not? Why is it useful to write two different explanations of
>     the patch?
> 
>     Sometimes, maybe.  I don't really think about it too much.  I'm not
>     the only one who does it.  But what bearing does what we put in
>     the subject of our patch submissions have on this discussion?
> 
> 
> 
> My failed attempt to clarify the docs on commit message formats 
> recommended using the same text for the commit message and email. If 
> there's a good reason to deviate from that, I'd like to know. Not that I 
> plan to change those docs again, it was a waste of time.
> 
> Also, you're proposing that PR numbers don't need to be in the subject 
> and/or that if it's in the subject it doesn't need to be in the body. Is 
> it just because "it's inconvenient for my current workflow" ? If it's in 
> the subject of the patch, why wouldn't you put it in the email subject too?
> 
> If writing two different descriptions of the patch by hand is liable to 
> not meet the formatting conventions, it seems like using existing 
> automation for creating the message (and reusing it for the email) might 
> be worth trying.
> 
> That doesn't mean we can't also improve the convention.

I'm sure different people do things differently but since you ask:
I use mklog.py with the -p option to create the commit message and
paste it into the patch.  Sometimes I use the long
"PR component/nnnn - bug description" as the title of the patch.
This is a force of habit from pre-Git days when there was no
convention or encouragement what to use (AFAIK).  I'll probably
get used to the new and improved way of doing things over time
but I'm not there yet.  I then use Thunderbird to compose an email
with the patch attached to it and I come up with a subject for
the email.  If I don't think of the new convention it may be
different from what's already in the patch.

I'm all for conventions and best practices but when tooling can
easily adjust things into the desired shape I think it should be
preferred to smacking people upside the head each time the don't
get everything just right.  And sometimes, taking a convention
as a guideline rather than a strict mandate is also fine (say
the 35 or 50 or 75 character limit for the subject of an email
or title of a patch).

Martin

Re: where is PRnnnn required again?

[Jonathan Wakely]

On Wed, 7 Jul 2021, 22:35 Martin Sebor, <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>> 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>>> 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>>>
> >      >      >> 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.

Re: where is PRnnnn required again?

[Martin Sebor]

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

Re: where is PRnnnn required again?

[Jakub Jelinek]

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>
> Date:   Fri Jul 2 16:16:31 2021 -0600
> 
>     Improve warning suppression for inlined functions [PR98512].

This states a PR number on the first line
> 
>     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

but these are intentionally not considered part of the ChangeLog entry,
so it isn't specified anywhere and that is why it has been rejected.

The ChangeLog entry must be in a rigid form so that random text in the middle
of the commit message isn't mistaken for start of the ChangeLog entry.

contrib/gcc-changelog/git_commit.py
has
author_line_regex = \
        re.compile(r'^(?P<datetime>\d{4}-\d{2}-\d{2})\ {2}(?P<name>.*  <.*>)')
changelog_regex = re.compile(r'^(?:[fF]or +)?([a-z0-9+-/]*)ChangeLog:?')
pr_regex = re.compile(r'\tPR (?P<component>[a-z+-]+\/)?([0-9]+)$')
dr_regex = re.compile(r'\tDR ([0-9]+)$')
star_prefix_regex = re.compile(r'\t\*(?P<spaces>\ *)(?P<content>.*)')
and will consider as first line of the ChangeLog part:
            if (changelog_regex.match(b) or self.find_changelog_location(b)
                    or star_prefix_regex.match(b) or pr_regex.match(b)
                    or dr_regex.match(b) or author_line_regex.match(b)
                    or b.lower().startswith(CO_AUTHORED_BY_PREFIX)):
                self.changes = body[i:]
                return
Once something is considered a ChangeLog part, everything after it has
to be a valid ChangeLog entry format.
Matching just PR component/NNNNN with random text afterwards at the start
of line or even somewhere in the middle would trigger any time one talks about some
PR in the description.

> 
>     gcc/ChangeLog:
> 
>             * diagnostic.c (get_any_inlining_info): New.
>             (update_effective_level_from_pragmas): Handle inlining context.
>             (diagnostic_enabled): Same.
>             (diagnostic_report_diagnostic): Same.
>             * diagnostic.h (struct diagnostic_info): Add ctor.
>             (struct diagnostic_context): Add new member.
>             * tree-diagnostic.c (set_inlining_locations): New.
>             (tree_diagnostics_defaults): Set new callback pointer.

	Jakub

Re: where is PRnnnn required again?

[Martin Sebor]

On 7/7/21 11:51 AM, Jakub Jelinek 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>
>> Date:   Fri Jul 2 16:16:31 2021 -0600
>>
>>      Improve warning suppression for inlined functions [PR98512].
> 
> This states a PR number on the first line
>>
>>      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
> 
> but these are intentionally not considered part of the ChangeLog entry,
> so it isn't specified anywhere and that is why it has been rejected.

But the PRs are used to update Bugzilla, right?  E.g., this commit:

   https://gcc.gnu.org/g:6d3bab5d5adb3e28ddb16c97b0831efdea23cf7d

updated both of:

   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98512#c12
   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98871#c7

(but not ChangeLogs).  So why can't they also be used to update
the ChangeLog entries?

> 
> The ChangeLog entry must be in a rigid form so that random text in the middle
> of the commit message isn't mistaken for start of the ChangeLog entry.

If it's okay for the PR lines to be used to update Bugzilla why is
it not also okay to use them to update ChangeLogs?  Or to put it
differently: can we change the script to do both?  I can't be
the only one who finds this onerous and confusing.

Martin

> 
> contrib/gcc-changelog/git_commit.py
> has
> author_line_regex = \
>          re.compile(r'^(?P<datetime>\d{4}-\d{2}-\d{2})\ {2}(?P<name>.*  <.*>)')
> changelog_regex = re.compile(r'^(?:[fF]or +)?([a-z0-9+-/]*)ChangeLog:?')
> pr_regex = re.compile(r'\tPR (?P<component>[a-z+-]+\/)?([0-9]+)$')
> dr_regex = re.compile(r'\tDR ([0-9]+)$')
> star_prefix_regex = re.compile(r'\t\*(?P<spaces>\ *)(?P<content>.*)')
> and will consider as first line of the ChangeLog part:
>              if (changelog_regex.match(b) or self.find_changelog_location(b)
>                      or star_prefix_regex.match(b) or pr_regex.match(b)
>                      or dr_regex.match(b) or author_line_regex.match(b)
>                      or b.lower().startswith(CO_AUTHORED_BY_PREFIX)):
>                  self.changes = body[i:]
>                  return
> Once something is considered a ChangeLog part, everything after it has
> to be a valid ChangeLog entry format.
> Matching just PR component/NNNNN with random text afterwards at the start
> of line or even somewhere in the middle would trigger any time one talks about some
> PR in the description.
> 
>>
>>      gcc/ChangeLog:
>>
>>              * diagnostic.c (get_any_inlining_info): New.
>>              (update_effective_level_from_pragmas): Handle inlining context.
>>              (diagnostic_enabled): Same.
>>              (diagnostic_report_diagnostic): Same.
>>              * diagnostic.h (struct diagnostic_info): Add ctor.
>>              (struct diagnostic_context): Add new member.
>>              * tree-diagnostic.c (set_inlining_locations): New.
>>              (tree_diagnostics_defaults): Set new callback pointer.
> 
> 	Jakub
> 

Re: where is PRnnnn required again?

[Jason Merrill]

On Wed, Jul 7, 2021 at 1:54 PM Jakub Jelinek via Gcc <gcc@gcc.gnu.org>
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>
> > Date:   Fri Jul 2 16:16:31 2021 -0600
> >
> >     Improve warning suppression for inlined functions [PR98512].
>
> This states a PR number on the first line
> >
> >     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
>
> but these are intentionally not considered part of the ChangeLog entry,
> so it isn't specified anywhere and that is why it has been rejected.
>
> The ChangeLog entry must be in a rigid form so that random text in the
> middle
> of the commit message isn't mistaken for start of the ChangeLog entry.
>
> contrib/gcc-changelog/git_commit.py
> has
> author_line_regex = \
>         re.compile(r'^(?P<datetime>\d{4}-\d{2}-\d{2})\ {2}(?P<name>.*
> <.*>)')
> changelog_regex = re.compile(r'^(?:[fF]or +)?([a-z0-9+-/]*)ChangeLog:?')
> pr_regex = re.compile(r'\tPR (?P<component>[a-z+-]+\/)?([0-9]+)$')
> dr_regex = re.compile(r'\tDR ([0-9]+)$')
> star_prefix_regex = re.compile(r'\t\*(?P<spaces>\ *)(?P<content>.*)')
> and will consider as first line of the ChangeLog part:
>             if (changelog_regex.match(b) or self.find_changelog_location(b)
>                     or star_prefix_regex.match(b) or pr_regex.match(b)
>                     or dr_regex.match(b) or author_line_regex.match(b)
>                     or b.lower().startswith(CO_AUTHORED_BY_PREFIX)):
>                 self.changes = body[i:]
>                 return
> Once something is considered a ChangeLog part, everything after it has
> to be a valid ChangeLog entry format.
> Matching just PR component/NNNNN with random text afterwards at the start
> of line or even somewhere in the middle would trigger any time one talks
> about some
> PR in the description.
>

But this *could* accept ^PR comp/NNNNN - descr$ , it just doesn't currently.

There's certainly a lot of precedent for including the description in the
PR line in ChangeLog files.

I agree we don't want to match a PR number in the middle of the line, or
without the dash.

Jason