From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x335.google.com (mail-ot1-x335.google.com [IPv6:2607:f8b0:4864:20::335]) by sourceware.org (Postfix) with ESMTPS id C8276385DC30 for ; Thu, 10 Jun 2021 17:20:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C8276385DC30 Received: by mail-ot1-x335.google.com with SMTP id 36-20020a9d0ba70000b02902e0a0a8fe36so409570oth.8 for ; Thu, 10 Jun 2021 10:20:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=bST4ijUmzdT73aYyv0H8gJd5N8J5dYkXME0s38B1BqM=; b=JIvPMqCaWzS+EISZYFmzROeh5FPu4cq/9VExkcuAbcXNDfQUa2elqVpFozc9RW3/FO ypukRHdmkmkyUaLit854QoDfo79mU29ephwyH0UFoXIuFuPIZ/t+04PgvlHU0H5awkRg vBW/Ivu0ULZ++pvtMroNudiutuG5VMcpDa1YgyTcir2b3aRMFQhZVJe32sKs7HEAZ9c5 n1BCuzJS/Jd+z3UCljQg8JHgH4QPdNmGyY/Ar8O/GedcLLCLsbe6xlFGywMNniutRoN+ eM/xPGpRs0qPQYL0Yjff9+8hZmgZIpkBxrYIHLvDmirUGDOfbNeDzNrEHBjd3lPsfKik UNjA== X-Gm-Message-State: AOAM531fa/0f6REsuwjunKtLJyDr1EzYksBehIendqPYStCPrHYecb/E DxyU+5C5dYUTzyCTZkz82ow+vX/A9fc= X-Google-Smtp-Source: ABdhPJzLvP8RLz/E3fxpz31BQUsB9WKgFWjRy5UVe2lRKFPQCh/+Rz+2rffna7EQNf1h1PllwM2uUQ== X-Received: by 2002:a9d:748b:: with SMTP id t11mr3353197otk.35.1623345627988; Thu, 10 Jun 2021 10:20:27 -0700 (PDT) Received: from [192.168.0.41] (97-118-122-241.hlrn.qwest.net. [97.118.122.241]) by smtp.gmail.com with ESMTPSA id x13sm720385ote.70.2021.06.10.10.20.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 10 Jun 2021 10:20:27 -0700 (PDT) Subject: Re: git gcc-commit-mklog doesn't extract PR number to ChangeLog From: Martin Sebor To: Jonathan Wakely Cc: Jakub Jelinek , gcc@gcc.gnu.org References: <36a4f5c4-357a-ca1c-e7f5-ede6ff3ba445@suse.cz> <20210610100851.GD7746@tucnak> <1230cb99-ed83-3e4e-8362-94f03ee021bc@gmail.com> <3228435b-aba0-6157-3266-c0f025822829@gmail.com> Message-ID: <5f89ddc0-aed4-2c20-0979-dfafb29046ee@gmail.com> Date: Thu, 10 Jun 2021 11:20:26 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <3228435b-aba0-6157-3266-c0f025822829@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_LOTSOFHASH, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Jun 2021 17:20:31 -0000 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 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 /' 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