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 <ja...@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]
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
