On Thu, 10 Jun 2021 at 18:06, Martin Sebor <mse...@gmail.com> 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 <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.
It's what the policies already recommend. It would mean you cannot say "[PR1234]" in the first line of your git commit if you don't list a PR in the changelog. Why would that affect you? Why would you be doing that anyway? It was always wrong. > > > > 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.) No, I mean the first line of the $git_description, which would also be Please read https://chris.beams.io/posts/git-commit/ for git log best practices. If you follow that, your git description has a single line summary (the subject), followed by a blank line, followed by zero or more lines of text. The subject is the first line. Which also makes an excellent email Subject: when you send the patch. > > 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. Yes, which is what I'm planning to do to the contribute.html and codingconventions.html docs. You are complaining about exactly the problems I'm pointing out and planning to fix. We currently describe a useful and rich format for patch emails, but allow any old garbage in the actual commit message. This is stupid. We should describe and strongly recommend a useful and rich format for the commit message itself. Then turning that into a useful email is utterly trivial (you just add "[PATCH]" or "[committed]" at the start of the first line of the commit message, and you have yourself a high quality email for gcc-patches. Many people are already doing it this way, because it produces better git commit messages, and takes less effort when sending that as an email. It's win win. We should document that as best practice, and persuade everybody to do it that way (and IMHO enforce at least some of it, but one step at a time). > > > >> 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? The latter. > 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). No, that is not the proposal, you've got it backwards. The suggestion is that **IF** the first line mentions a PR, then the ChangeLog entry must also mention that PR (and optionally, additional PRs as well). That is not the same as "if the ChangeLog entry mentions a PR, the first line must mention it too". > > > > > 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. The references that get added to the ChangeLog file **also** update bugzilla (modulo bugs due to non-ASCII characters, or other unexplained bugs in the BZ updating). So if you follow the policy for mentioning bugs in the ChangeLog entry (which has been in place for many, many years), it Just Works. > > > >> > >> 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 >
