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

[Jason Merrill <jason@redhat.com>]

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?

> 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