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

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 17/06/2021 01:40, Jason Merrill via Gcc wrote:
> On 6/16/21 8:17 PM, Martin Sebor wrote:
>> On 6/16/21 5:45 PM, Jason Merrill wrote:
>>> On Wed, Jun 16, 2021 at 5:46 PM Martin Sebor <msebor@gmail.com 
>>> <mailto:msebor@gmail.com>> wrote:
>>>
>>>     On 6/16/21 2:49 PM, Jason Merrill wrote:
>>>      > On 6/15/21 11:42 PM, Jason Merrill wrote:
>>>      >> On Tue, Jun 15, 2021 at 10:04 PM 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 6/15/21 6:56 PM, Hans-Peter Nilsson wrote:
>>>      >>      > On Fri, 11 Jun 2021, Martin Sebor via Gcc wrote:
>>>      >>      >
>>>      >>      >> On 6/11/21 11:32 AM, Jonathan Wakely wrote:
>>>      >>      >>> On Fri, 11 Jun 2021 at 18:02, Martin Sebor wrote:
>>>      >>      >>>> My objection is to making our policies and tools more
>>>      >> restrictive
>>>      >>      >>>> than they need to be.  We shouldn't expect everyone to
>>>     study
>>>      >> whole
>>>      >>      >>>> manuals just to figure out how to successfully 
>>> commit a
>>>      >> change (or
>>>      >>      >>>> learn how to format it just the right way).  It should
>>>     be easy.
>>>      >>      >>>
>>>      >>      >>> I agree, to some extent. But consistency is also 
>>> good. The
>>>      >>     conventions
>>>      >>      >>> for GNU ChangeLog formatting exist for a reason, and so
>>>     do the
>>>      >>      >>> conventions for good Git commit messages.
>>>      >>      >>>
>>>      >>      >>>> Setting this discussion aside for a moment and using a
>>>      >> different
>>>      >>      >>>> example, the commit hook rejects commit messages that
>>>     don't
>>>      >> start
>>>      >>      >>>> ChangeLog entries with tabs.  It also rejects commit
>>>      >> messages that
>>>      >>      >>>> don't list all the same test files as those changed by
>>>     the
>>>      >> commit
>>>      >>      >>>> (and probably some others as well).  That's in my view
>>>      >> unnecessary
>>>      >>      >>>> when the hook could just replace the leading spaces 
>>> with
>>>      >> tabs and
>>>      >>      >>>> automatically mention all the tests.
>>>      >>      >>>>
>>>      >>      >>>> I see this proposal as heading in the same direction.
>>>      >> Rather than
>>>      >>      >>>> making the script fix things up if we get them wrong
>>>     it would
>>>      >>     reject
>>>      >>      >>>> the commit, requiring the user to massage the 
>>> ChangeLog by
>>>      >>     hand into
>>>      >>      >>>> an unnecessarily rigid format.
>>>      >>      >>>
>>>      >>      >>> You cannot "fix things up" in a server-side receive 
>>> hook,
>>>      >> because
>>>      >>      >>> changing the commit message would alter the commit
>>>     hash, which
>>>      >>     would
>>>      >>      >>> require the committer to do a rebase to proceed. That
>>>     breaks the
>>>      >>      >>> expected behaviour and workflow of a git repo.
>>>      >>      >>>
>>>      >>      >>> You can use the scripts on the client side to verify
>>>     your commit
>>>      >>      >>> message before pushing, so you don't have to be 
>>> surprised
>>>      >> when the
>>>      >>      >>> server rejects it.
>>>      >>      >>
>>>      >>      >> That sounds like a killer argument.  Do we have shared
>>>      >> client-side
>>>      >>      >> scripts that could fix things up for us, or are we each
>>>     on our
>>>      >> own
>>>      >>      >> to write them?
>>>      >>      >
>>>      >>      > I hope I got your view wrong.  If not: the "scripts 
>>> fixing
>>>      >>      > things up for us" direction is flawed (compared to the
>>>     "scripts
>>>      >>      > rejecting bad formats"), unless offered as a non-default
>>>     option;
>>>      >>      > please don't proceed.
>>>      >>      >
>>>      >>      > Why?  For one, there'll always be bugs in the scripting.
>>>      >>      > Mitigate those situations: while wrongly rejecting a
>>>     commit is
>>>      >>      > bad, wrongly "fixing things up" is worse, as a general 
>>> rule.
>>>      >>      > Better avoid that.  (There's probably a popular "pattern
>>>     name"
>>>      >>      > for what I try to describe.)
>>>      >>
>>>      >>     The word that comes to mind is Technophobia.  Is it wise to
>>>     trust
>>>      >>     compilers to transform programs from their source form into
>>>      >>     executables?  What if there are bugs in either?  What about
>>>     the OS?
>>>      >>     The whole computer, or the Internet?  Our cars? 
>>> Fortunately, there's
>>>      >>     more to gain than to lose by trusting automation.  If there
>>>     weren't
>>>      >>     human progress would be stuck sometime in the 1700's.
>>>      >>
>>>      >>     But we're not talking about anything anywhere that 
>>> sophisticated
>>>      >>     here: a sed script to copy and paste a piece of text in
>>>      >>     the description of a change from one place to another.  It's
>>>     been
>>>      >>     done a few times before with more important data than
>>>     ChangeLogs.
>>>      >>
>>>      >>
>>>      >> git gcc-commit-mklog already automates most of the process.  It
>>>     could
>>>      >> also automate adding [PRxxxxx] to the first line.  Is that what
>>>     you're
>>>      >> asking for?
>>>      >
>>>      > Like, say:
>>>
>>>     I don't think this solves the problem Xionghu Luo was asking about:
>>>     https://gcc.gnu.org/pipermail/gcc/2021-June/236346.html
>>>
>>>
>>> Indeed, their problem was not mentioning the PR in the testcase, 
>>> which a script isn't going to fix.
>>>
>>>     i.e., they did have a [PRnnnn] in the one line subject but not in
>>>     their ChangeLog entries.  It also not clear if they used mklog.py
>>>     at all.  IME, mklog.py already puts in a [PRnnnn] near the top of
>>>     a patch if it finds in one of the tests.  Though it doesn't seem
>>>     to put in the ChangeLog entries.  Odd.
>>>
>>>
>>> It currently puts in
>>>
>>>   PR comp/nnnnn
>>>
>>> at the beginning of the ChangeLog entries; it used to put them in the 
>>> entries for each ChangeLog file, but that changed in r12-771.  My 
>>> patch also adds the [PRnnnn] to the subject line.
>>
>> To say I'm not good at Python would be an understatement but I hacked
>> up the attached patch that:
>>
>> 1) extracts PR numbers from test names,
>> 2) gets the component for each PR from Bugzilla,
> 
> That seems useful for testcases like the OP's that put the PR number in 
> the filename rather than a comment.  Maybe submit it as a patch?

It seems a bit dangerous to me to rely on just extracting PR numbers 
from tests.  What if the patch is just adjusting a test to make it 
compatible with the remainder of the change?

Perhaps we should scan for such changes and ask the user to confirm if 
they are the right and then to also give the chance to 
add/remove/specify primary PRs as appropriate.  Finally, we can scrape 
bugzilla and print out the bug summaries as a way of validating that the 
correct PR number has been added.  It's far easier to spot that the 
summary doesn't match expectations than the PR number typed.

> 
>> 3) adds the PR component/nnnnn to each ChangeLog
> 
> This would be reverting the r12-771 change, which seems both unrelated 
> and undesirable.
> 
>> Running the hacked up script with -p on the patch for PR 100085 prints
>> the following:
>>
>> Resolves:
>> PR 100085/target - Bad code for union transfer from __float128 to 
>> vector types
> 
> You have the number and component switched.
> 
>>> We could do various cleanup in the 'commit-msg' hook on the user 
>>> side. Or, probably better, git gcc-verify could clean up some of the 
>>> issues it finds.
>>
>> I'm not familiar with these.  Where should I look for them to learn
>> how to do this?
> 
> We currently have no commit-msg hook; see git help hooks for a 
> description, or .git/hooks/commit-msg.sample for an example hook.
> 
> git gcc-verify is an alias to run 
> contrib/gcc-changelog/git_check_commit.py , which is the same checker 
> that runs on the server.  I don't know how hard it would be to adjust it 
> to have a fixup mode as well, for when it is run from git gcc-verify.
> 
> Jason
> 

R.