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

[Martin Sebor <msebor@gmail.com>]

On 6/16/21 6:40 PM, Jason Merrill 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?

Will do.

> 
>> 3) adds the PR component/nnnnn to each ChangeLog
> 
> This would be reverting the r12-771 change, which seems both unrelated 
> and undesirable.

Now I'm confused.  Isn't that just what caused the problem to begin
with?  (The bug not being updated with the commit because it's not
in the ChangeLog entries?)

> 
>> 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.

Doh!

> 
>>> 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.

Thanks, I'll see if I can find some time to look at this.

Martin

> 
> Jason
>