Re: Git ChangeLog policy for GCC Testsuite inquiry

[Segher Boessenkool <segher@kernel.crashing.org>]

On Thu, Feb 06, 2020 at 08:51:42AM +0000, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Mon, Feb 03, 2020 at 01:24:04PM -0700, Jeff Law wrote:
> >> ANd yes, even though I have been a regular ChangeLog user, I rely more
> >> and more on the git log these days.
> >
> > As a reviewer, the changelog is priceless still.  We shouldn't drop the
> > changelog before people write *good* commit messages (and we are still
> > quite far from that goal).
> 
> But that was the point of what others said upthread: now that the patch
> description is part of the commit message, we should review them for
> whether they're a good description.  The patch description should be
> as much a part of the review as the patch itself.
> 
> So IMO the right response to poor patch descriptions is to ask for
> a better one.

Certainly.  But I don't think we should drop changelogs until we are
there, or have confidence we'll get there very soon.

> The goal should be "have good patch descriptions" not
> "make all patch descriptions use format X".

If you read the "subject" discussion, you know I *very* much agree with
that.

> The traditional changelog
> format should be one acceptable way of writing good patch descriptions,
> but not the only acceptable way.

A changelog is not an acceptable patch description (except in very
trivial cases, fixing a typo in docs for example).  It says only *what*
changed, nothing more.  We *do* need that info, but we need more of
course.

> Sometimes the traditional changelog is better than a plain English
> description (the SVE PCS work being one recent example from my POV).
> But for modern VCSes there's IMO no benefit to a changelog like:
> 
> 2017-09-05  Richard Sandiford  <richard.sandiford@linaro.org>
> 
> 	* builtins.c (expand_builtin_powi): Use int_mode_for_size.

"Use"?  For what?  What does it do with it, or is it used instead of
seomething else, is that what this patch does?  The changelog should
say, but doesn't.

> 	(get_builtin_sync_mode): Likewise.
> 	(expand_ifn_atomic_compare_exchange): Likewise.
> 	(expand_builtin_atomic_clear): Likewise.
> 	(expand_builtin_atomic_test_and_set): Likewise.
> 	(fold_builtin_atomic_always_lock_free): Likewise.

You don't need to point out every function you did the same change in.
It's helpful when it is just a handful, or ten or so, but no one will
read a list of eighty.  And it is not required.

> (choosing one of mine to avoid picking on someone else).  I find
> reviewing changelogs like this an extra burden rather than a help,

Yes.  It's just something more to delete in the reply.  The tiresome
part is scanning it to see if there is something else in there.

> since reviewers are expected to make sure that every affected function
> is listed,

No, that is not required.  Every change should be noted, but you can
just say

	* builtins.c: Use int_mode_for_size instead of mode_for_size.
	* combine.c: Likewise.

etc.

> every sentence ends with a ".", the correct amount of spacing
> is used, etc.

Your editor can do that for you, but it is automatic as well: things
just look wrong if they are formatted differently.  In that sense,
consistent formatting is a great help to any reader of the code,
reviewer or not.

This is not specific to changelogs at all, of course.

> So I hope eventually we can be flexible and just use the changelog
> format as one optional way of achieving a goal, rather than being
> mandatory for every non-testsuite patch.

Yes.  Eventually.  People first need to learn to write commit messages
that are not even less helpful than the changelogs they write.

Writing a commit message should not be seen as a burden.  It's your
sales pitch!  You can (and should!) tell what is changed and why, maybe
why not in some other way, etc., so that people in the future can figure
out why this change was made, and what it intended to do.  A good commit
message makes life much easier for reviewers, too, since not many
questions will remain.

Also.  If you find it hard to write a good commit message (or a good
changelog), chances are that your patch series is not so well structured;
simply chopping one patch in two or three often helps.

Anyway, you know all that.  I just don't want a year of GCC commits
without useful history, I hope I managed to explain my view.


Segher