Bug 81828

Summary: An automated tool in the patch submission process should complain about ChangeLogs without comments
Product: WebKit Reporter: Timothy Hatcher <timothy>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: abarth, beidson, darin, dpranke, eric, levin, mjs, pfeldman
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   

Description Timothy Hatcher 2012-03-21 12:48:29 PDT
The ChangeLog is designed to be a template for the developer to edit and add comments about what changed. I see many ChangeLogs that end up being just the bug title and url and the default output of changed files/functions. Anyone looking at these changes will have limited insight for why the change was made.

We should make check-webkit-style (and the style bot) complain about this to reinforce the practice of commenting what changed and why.
Comment 1 Brady Eidson 2012-03-21 13:17:12 PDT
One idea would be to add an "OOPS" to each function that changed, or at least once for each file that changed.  That would make some existing tools/scripts pick up the failure to use the ChangeLog correctly.
Comment 2 Timothy Hatcher 2012-03-21 13:23:50 PDT
I think an OOPS per file would be enough. Also adding a space after the colon for each function would make editing easier and cause style warnings about trailing whitespace (and is less heavy handed as adding OOPS.)
Comment 3 Adam Barth 2012-03-21 13:26:42 PDT
I would encourage you to mark this bug as WONTFIX.  It's tempting to make the style bot a nag for filling out TPS reports, but it's a better tool when it tells you something interesting that you'd like to know rather than when it nags you.
Comment 4 Timothy Hatcher 2012-03-21 13:30:02 PDT
It already "nags" for missing bug URL, etc. Why is this not important enough to complain about?
Comment 5 Brady Eidson 2012-03-21 13:32:23 PDT
As long as the style bot is what forces us to fill out our TPS reports, it should make sure we do so correctly.
Comment 6 Timothy Hatcher 2012-03-21 13:34:15 PDT
It is clear we need such a tool. There are numerous commits every day that lack details in the ChangeLog.

Maybe revoking reviewer status would be a better tool?
Comment 7 Adam Barth 2012-03-21 13:37:55 PDT
> It already "nags" for missing bug URL, etc. Why is this not important enough to complain about?

I don't have statistics, but I don't believe that check fires very much.  We search and see whether folks who received that message were happy and changed their patch or whether they ignored it.  If they ignored the message, we should remove it.

> As long as the style bot is what forces us to fill out our TPS reports, it should make sure we do so correctly.

That's not the goal of the style bot.  The goal of the style bot is to save reviewers from pointing out obvious mistakes in a patch that a computer can flag.  That makes the project more efficient because reviewer time is a precious resource.

> It is clear we need such a tool.

Is it?

> Maybe revoking reviewer status would be a better tool?

I think you're being hyperbolic.  I suspect folks would laugh you off the mailing list if you actually tried doing that.
Comment 8 Maciej Stachowiak 2012-03-21 13:43:42 PDT
(In reply to comment #3)
> I would encourage you to mark this bug as WONTFIX.  It's tempting to make the style bot a nag for filling out TPS reports, but it's a better tool when it tells you something interesting that you'd like to know rather than when it nags you.

Having useful comments in the ChangeLog (and therefore in the commit message) is intended to be a way to enable other contributors to better understand your change, not time-wasting bureaucracy.

It's true that we haven't been very good about consistently practicing good ChangeLog comments, but as the project grows, it becomes more important to consider the human audience of one's changes, not just the machine audience.

Perhaps a good first step would be to expand <https://www.webkit.org/coding/contributing.html#changelogs> to make more clear what is expected. It says the ChangeLog should "describe your change" but it could be more clear that bug title and URL is generally not in itself a sufficient description, at least for a nontrivial change.
Comment 9 Brady Eidson 2012-03-21 13:47:14 PDT
(In reply to comment #7)

> > As long as the style bot is what forces us to fill out our TPS reports, it should make sure we do so correctly.
> 
> That's not the goal of the style bot.  The goal of the style bot is to save reviewers from pointing out obvious mistakes in a patch that a computer can flag.  That makes the project more efficient because reviewer time is a precious resource.

Stating that stylebot has precisely one goal is a bit disingenuous.

Yes it's great that the stylebot automates part of the review process allowing reviewers to focus on more important things.

But another goal of the stylebot is that it enforces many parts of the review that reviewers have traditionally been too lax about enforcing.  If reviewers had actually been good about enforcing style guidelines the stylebot might never have came in to existence.

I think the point of this bug is that SOMETHING needs to enforce ChangeLogs are filled out properly.  Relying on reviewers obviously isn't working.

Since stylebot already enforces part of the ChangeLog responsibilities it made sense to suggest it enforce all of them.

Perhaps there should be an entirely separate tool that looks at just the ChangeLog and enforces both the brain dead part of filling out the ChangeLog in addition to the part that reviewers aren't doing a good enough job enforcing on their own.

I'm not sure what advantage there would be to making that a separate tool.
Comment 10 Maciej Stachowiak 2012-03-21 13:51:48 PDT
(In reply to comment #7)
> > It already "nags" for missing bug URL, etc. Why is this not important enough to complain about?
> 
> I don't have statistics, but I don't believe that check fires very much.  We search and see whether folks who received that message were happy and changed their patch or whether they ignored it.  If they ignored the message, we should remove it.
> 
> > As long as the style bot is what forces us to fill out our TPS reports, it should make sure we do so correctly.
> 
> That's not the goal of the style bot.  The goal of the style bot is to save reviewers from pointing out obvious mistakes in a patch that a computer can flag.  That makes the project more efficient because reviewer time is a precious resource.

Based on your statement of the goal of the stylebot, it seems like the relevant standard for evaluating this change should be "will it save reviewers time in having to point out the mistake of insufficient ChangeLog" rather than "will contributors be happy when the script complains to them". And analogously for the bug URL requirement.
Comment 11 Adam Barth 2012-03-21 13:54:47 PDT
> Stating that stylebot has precisely one goal is a bit disingenuous.

Given that I created the style-queue [1], I can say with authority what my goals were in creating the bot.  To wit:

[[
The primary goal of the style-queue is to reduce review latency by (1)
giving immediate feedback to contributors and (2) making human
reviewer more efficient by relieving them of mechanical tasks (like
asking for tabs to be replaced with spaces).
]]

https://lists.webkit.org/pipermail/webkit-dev/2009-November/010649.html

The scope of this discussion is getting beyond what's appropriate for a bug thread.  I'd encourage interested folks to take this discussion to webkit-dev.

[1] Note: I didn't create check-webkit-style.  That was Dave Levin.
Comment 12 Brady Eidson 2012-03-21 14:02:10 PDT
(In reply to comment #11)
> > Stating that stylebot has precisely one goal is a bit disingenuous.
> 
> Given that I created the style-queue [1], I can say with authority what my goals were in creating the bot.  To wit:
> 
> [[
> The primary goal of the style-queue is to reduce review latency by (1)
> giving immediate feedback to contributors and (2) making human
> reviewer more efficient by relieving them of mechanical tasks (like
> asking for tabs to be replaced with spaces).
> ]]
> 
> https://lists.webkit.org/pipermail/webkit-dev/2009-November/010649.html

Fair enough.  I suppose it was a mistake to equate a singular design goal with the multiple tangible benefits seen in practice.

One substantial collateral benefit was that we actually *caught* style problems that reviewers weren't catching by themselves.

That's what Tim and I are advocating for here; A tool to do the similar task of catching sloppy ChangeLog use that reviewers are not catching by themselves.

You seem to be very against further conflating of the stylebot for this task.  Perhaps we should create a second tool that also runs against every patch, and remove the ChangeLog enforcement responsibilities that stylebot currently has.

> The scope of this discussion is getting beyond what's appropriate for a bug thread.  I'd encourage interested folks to take this discussion to webkit-dev.

Definitely seems like it deserves a webkit-dev discussion.  That doesn't make this bug WONTFIX.
Comment 13 Maciej Stachowiak 2012-03-21 14:06:15 PDT
(In reply to comment #12)
> (In reply to comment #11)
> 
> > The scope of this discussion is getting beyond what's appropriate for a bug thread.  I'd encourage interested folks to take this discussion to webkit-dev.
> 
> Definitely seems like it deserves a webkit-dev discussion.  That doesn't make this bug WONTFIX.

I agree that a webkit-dev thread is a good next step.

I think maybe a good starting point is reviewing what is expected in a ChangeLog and why; and how to update our practices, docs and toolchain to better enforce the expected level of quality (or alternately, define a lower standard of what is expected and be consistent about that, though this would not be my first choice).
Comment 14 Brady Eidson 2012-03-21 14:06:35 PDT
In an effort to set aside the distraction of whether or not the stylebot should be the specific entity to do this, retitling to "An automated tool in the patch submission process should complain about ChangeLogs without comments"
Comment 15 Timothy Hatcher 2012-03-21 14:07:38 PDT
Yes, I don't care what the tool is.

I'll raise the issue on webkit-dev.
Comment 16 David Levin 2012-03-21 14:54:28 PDT
(In reply to comment #11)
> 
>  Note: I didn't create check-webkit-style.  That was Dave Levin.

I know we're way off topic at this point, but actually it was hamaji (who did a lot of work to portGoogle's style tool and ajwong who was able to the licensed change to be WebKit compatible -- since it was all Googler contributed code). I just have reviewed a lot of changes to it and did a few myself.
Comment 17 Dirk Pranke 2012-03-21 15:08:20 PDT
(In reply to comment #2)
> I think an OOPS per file would be enough. Also adding a space after the colon for each function would make editing easier and cause style warnings about trailing whitespace (and is less heavy handed as adding OOPS.)

Most of my changes would (IMO) gain no clarity from commenting each function or each file that is changed; generally I think having a few sentences or a paragraph underneath the subject/bug/reviewer lines is sufficient, and I actually wish that the text wasn't there (I will grant that there are some cases where having per-file or per-method comments have been helpful).

Further, generating the list of files and functions that have changes can be fragile; if I generate a changelog, and then make changes to the patch, the changelog can get stale, and recreating it is even more of a hassle - this means that I often have to throw away the changelog I wrote and create a new one, just to be sure that it's up to date. Perhaps merging can make this better, but I'm skeptical.

I would much rather address this through culture and convention than through strict checking via a tool.