git gcc-commit-mklog doesn't extract PR number to ChangeLog

Thu Jun 10 17:20:26 GMT 2021

On 6/10/21 11:06 AM, Martin Sebor wrote:
> On 6/10/21 9:56 AM, Jonathan Wakely wrote:
>> On Thu, 10 Jun 2021 at 15:56, Martin Sebor wrote:
>>>
>>> On 6/10/21 4:40 AM, Jonathan Wakely via Gcc wrote:
>>>> On Thu, 10 Jun 2021 at 11:08, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>
>>>>> On Thu, Jun 10, 2021 at 11:01:49AM +0100, Jonathan Wakely via Gcc 
>>>>> wrote:
>>>>>> On 10/06/21 10:44 +0100, Jonathan Wakely wrote:
>>>>>>>> Quite interesting idea! Are you willing to prepare a patch for it?
>>>>>>>
>>>>>>> This works.
>>>>>>
>>>>>> And this works better, because it checks the PR in the title matches
>>>>>> one in the changelog.
>>>>>>
>>>>>> I'll get something added to the tests and prep this for commit.
>>>>>
>>>>> Note, some commits fix more than one PR.  Sometimes the subject lists
>>>>> just one of them and the ChangeLog several, at other times people 
>>>>> mention
>>>>> [PRnnnnnn, PRnnnnnn] etc. in the subject.
>>>>> I think checking that at least one changeLog PR line matches at 
>>>>> least one PR
>>>>> in the subject would be good enough.
>>>>>
>>>>> Your regex will not match [PR123456, PR123457] in subject, perhaps ok
>>>>
>>>> Yeah, that wouldn't get matched, so no checks would be done for the
>>>> changelog body. Not ideal, but better than what we have no where
>>>> nothing is checked at all.
>>>>
>>>>> initially, and if I read it will will be happy if at least one line 
>>>>> matches
>>>>> it.
>>>>
>>>> Yes, if the summary line has a single PR number, it must be present in
>>>> the changelog body. Other PR numbers can also be in the body, and they
>>>> aren't checked.
>>>>
>>>> But I've hit an issue trying to test it, because the testcases in
>>>> contrib/gcc-changelog/test_patches.txt are in the form of emails, and
>>>> the Subject: line from the emails is not passed to the GitInfo
>>>> constructor, so isn't part of the message that gets checked.
>>>>
>>>> Martin, Shouldn't the GitEmail class extract the Subject: from the
>>>> email header and use that as the first line passed to the GitInfo
>>>> object?
>>>>
>>>
>>> I'm a little lost as to what's being changed,
>>
>> Nothing is being changed.
> 
> The script is being changed and Tobias' proposal that I understand
> is being discussed is:
> 
>  >> One options would be to require a 'PR <comp>/<nnnnn+>' line if there is
>  >> 'PRnnnnn+' in the commit title, rejecting the commit otherwise.
> 
> That sounds like a new requirement.  I want to understand what that
> will mean for me.
> 
>>
>> If your patch is related to a bug, you're supposed to say so:
>> https://gcc.gnu.org/codingconventions.html#ChangeLogs
>>
>> And you're also supposed to put a [PRnnnn] tag in the email subject
>> line (which best practices say should also be the Subject: of the
>> email you send to gcc-patches):
>> https://gcc.gnu.org/contribute.html#patches
>>
>> The patches being proposed verify that if you put [PRnnnn] at the end
>> of the subject line, that you also put "PR component/nnnn" in the
>> changelog part. Because if you didn't, something is wrong and you're
>> failing to follow the existing policy (why would you put a [PRnnnn]

One other note: you mention policy above, which suggests a requirement.
My understanding is that the format of a commit message is a convention
put in place to take some of the tedium out of creating ChangeLog
entries.  If that's still correct, I would be more in favor of making
the Git commit hook smarter and more forgiving than of adding new
requirements imposing a more rigid format.

Recognizing a PRnnnnn anywhere within the commit message and "doing
the thing" rather than rejecting a commit if it doesn't have a PRnnnn
on the first line (but does somewhere else) would be a change I'd vote
for.

>> tag in the subject if it's not related to that PR, and if it is
>> related, why are you not putting it in the changelog part?)
> 
> By "the subject line" are you referring to what the ChangeLog calls
> $git_description, and, AFAICS, consists of multiple lines?  (Based
> on the Example patch on the conventions page.  In that example,
> the PR87763 reference is in the middle of line 3.)
> 
> I find this confusing.  Either we're talking about the Subject line
> of the email I send to gcc-patches with the patch or we're talking
> about the Git description ($git_description).  If they're supposed
> to be the same the ChangeLog convention should say so, and
> the Example patch should be changed to show that.
> 
>>
>>> and, truth be told,
>>> what exactly the current "right" format is.  Where are the PRnnnnn
>>> strings recognized as special?
>>>
>>> The ChangeLog description doesn't seem to cover this and I've been
>>> assuming they're recognized anywhere in the ChangeLog message, but
>>> I think I also noticed they don't always end up updating all
>>> the bugs.
>>
>> There are various reasons for that, including that non-ASCII
>> characters tend to break the automation.
>>
>> But in general, if you follow the policy of mentioning "PR
>> component/nnnn" in the changelog, your git commit should add a comment
>> to the mentioned bugzilla PRs.
> 
> Again, I'm not sure what exactly you mean by "the changelog" here.
> Are you referring to the whole commit message or just to what
> the conventions refer to as the ChangeLog entry?  I'm guessing
> the latter but my understanding of this proposal is to also require
> all the affected PRnnnnn references in the first line of the commit
> message (or the whole $git_description).
> 
>>
>> Within a bugzilla comment, any string mentioning "PR nnnn" or "Bug
>> nnnn" (and variations on those) will get dynamically turned into a
>> link to that bug, but I think that happens on the fly when rendering
>> the HTML, and it's separate from the automation that adds git commits
>> to bugzilla as comments. I think the automation to add commits to
>> bugzilla uses a much stricter pattern to recognise a PR mentioned in
>> the commit.
> 
> I'm familiar with this feature and I've been (naively) assuming
> the ChangeLog/Git commit hook script worked similarly.  I.e.,
> wherever I mention PRnnnnn (or the other forms) in a commit
> message the script interprets it as a reference to a bug that
> it then updates with the commit.
> 
> If it doesn't (as it sounds like is the case) it might be helpful
> to mention that and explain how it's different. (Although I think
> it would be better to make both work the same.)
> 
> I understand that PRnmmmm references under each ChangeLog entry are
> added verbatim to the ChangeLog file, as you explain below. I don't
> care about that.  I just want to make sure the right bugs get updated
> for my commits.
> 
>>
>>>
>>> FWIW, in commits for multiple PRs I've been adding a Resolves
>>> line like this:
>>>     https://gcc.gnu.org/pipermail/gcc-cvs/2021-May/347414.html
>>>
>>> I usually also add the PR numbers under each ChangeLog but I'm not
>>> sure it's necessary.  It would be good to know and for the ChangeLog
>>> convention to document how exactly this works, and if something
>>> changes, to update the documentation.
>>
>> It already says you should put it in the changelog. For each changelog
>> piece you add it to, it will be added to that relevant ChangeLog file.
>> It probably makes sense to add it to all of them, unless some piece of
>> the commit really isn't relevant to that PR (but then it should
>> probably be a separate commit!)
>>
>> You can also put it once, before all the changelog entries e.g.
>> https://gcc.gnu.org/g:91349e57bbfd010156b9128b2ad751c8843e7245
>> which got added to two ChangeLog files by the daily bump script:
>> https://gcc.gnu.org/git/?p=gcc.git;a=blobdiff;f=gcc/cp/ChangeLog;h=5a97fc84264e85b60adfdc50187ce7faeeb6f86f;hp=225b891700e88a58639d7ea0f10ad76ffb8d87f4;hb=c60387214593445d1514bf7852f27f4523458cda;hpb=25e5ecdf82b49977e86bfaded236fb34af2705ed 
>>
>> https://gcc.gnu.org/git/?p=gcc.git;a=blobdiff;f=gcc/testsuite/ChangeLog;h=9e31d686e1cb3b63b2141f86e5394aa1794ecdf5;hp=640fcbed0ebb9ed5e0b0ab12c52e1950284f9ee9;hb=4f625f47b4456e5c05a025fca4d072831e59126c;hpb=53cb324cb4f9475d4eabcd9f5a858c5edaacc0cf 
>>
>>
>> I do want to improve the https://gcc.gnu.org/contribute.html#patches
>> and https://gcc.gnu.org/codingconventions.html#ChangeLogs docs though.
>> We shouldn't have the info split in two places, and we should update
>> the recommendations to reflect current practices.
>>
> 
> Great!  I'll do my best to help, at a minimum by critiquing your
> improvements ;)
> 
> Martin