Lately I have observed more and more and more changes going into WebKit that lack any details about why a particular change was made. It is intended that the ChangeLog (and commit message) contain some details about your change, not just the bug title and URL. The contributing information on subject is pretty sparse, which has likely caused this problem to manifest. However, the example ChangeLog linked from that page is prime example of what we all should strive for when describing our changes. To help curb this lack of detail in ChangeLogs, I propose we add script (or augment an existing script) to check for this missing information and inform the contributor. It is clear not all reviewers are asking patch authors to provide this information when reviewing, and such a tool would help enforce it. — Timothy Hatcher
On Mar 21, 2012, at 2:29 PM, Timothy Hatcher wrote:
Lately I have observed more and more and more changes going into WebKit that lack any details about why a particular change was made. It is intended that the ChangeLog (and commit message) contain some details about your change, not just the bug title and URL.
The contributing information on subject is pretty sparse, which has likely caused this problem to manifest. However, the example ChangeLog linked from that page is prime example of what we all should strive for when describing our changes.
To help curb this lack of detail in ChangeLogs, I propose we add script (or augment an existing script) to check for this missing information and inform the contributor. It is clear not all reviewers are asking patch authors to provide this information when reviewing, and such a tool would help enforce it.
I think this is a reasonable suggestion. I think we should make the contributor docs more clear about what is expected, as well. Regards, Maciej
I think the challenge in part is to explain why the ChangeLog is useful. Comments in there hopefully explain why as a guide to reviewers to give the reviewers and future onlookers a guide to the change. It feels like what comments are that useful but often get inserted in there to fill it out. Is there a good way to address this? (Should folks just leave what comments out or put in the obvious what?) Since we have an example ChangeLog, I'll point out what I mean using it (but it is evident in many ChangeLog's from me as well). What value does this comment add? - runtime/JSGlobalData.h: Replaced the HashSet and HashCountedSet with a Vector. On the other hand, I found this very informative: wtf/PassRefPtr.h: (WTF::PassRefPtr::~PassRefPtr): The most common thing to do with a PassRefPtr in hot code is to pass it and then destroy it once it's set to zero. Help the optimizer by telling it that's true. Here again, we see a what comment: parser/Nodes.cpp: (JSC::NodeReleaser::releaseAllNodes): ALWAYS_INLINE. (JSC::NodeReleaser::adopt): Ditto. Which doesn't tell me why ALWAYS_INLINE was added (which I can easily see from the patch). dave On Wed, Mar 21, 2012 at 2:33 PM, Maciej Stachowiak <mjs@apple.com> wrote:
On Mar 21, 2012, at 2:29 PM, Timothy Hatcher wrote:
Lately I have observed more <http://trac.webkit.org/changeset/111595> and more <http://trac.webkit.org/changeset/111577> and more<http://trac.webkit.org/changeset/111527> changes going into WebKit that lack any details about why a particular change was made. It is intended that the ChangeLog (and commit message) contain some details about your change, not just the bug title and URL.
The contributing information<http://www.webkit.org/coding/contributing.html#changelogs> on subject is pretty sparse, which has likely caused this problem to manifest. However, the example ChangeLog <http://trac.webkit.org/changeset/43259> linked from that page is prime example of what we all should strive for when describing our changes.
To help curb this lack of detail in ChangeLogs, I propose we add script<https://bugs.webkit.org/show_bug.cgi?id=81828> (or augment an existing script) to check for this missing information and inform the contributor. It is clear not all reviewers are asking patch authors to provide this information when reviewing, and such a tool would help enforce it.
I think this is a reasonable suggestion. I think we should make the contributor docs more clear about what is expected, as well.
Regards, Maciej
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Wed, Mar 21, 2012 at 2:41 PM, David Levin <levin@chromium.org> wrote:
I think the challenge in part is to explain why the ChangeLog is useful.
Comments in there hopefully explain why as a guide to reviewers to give the reviewers and future onlookers a guide to the change.
I do agree that "why" comments are more useful than "what" comments are most cases. However, I'd say it's also helpful to summarize what a new function / class does as well if the patch adds one. Since comments in the change log is for a specific revision unlike comments in the code, it's less dangerous to add "what" comment. - Ryosuke
On Wed, Mar 21, 2012 at 2:33 PM, Maciej Stachowiak <mjs@apple.com> wrote:
On Mar 21, 2012, at 2:29 PM, Timothy Hatcher wrote:
Lately I have observed more and more and more changes going into WebKit that lack any details about why a particular change was made. It is intended that the ChangeLog (and commit message) contain some details about your change, not just the bug title and URL.
The contributing information on subject is pretty sparse, which has likely caused this problem to manifest. However, the example ChangeLog linked from that page is prime example of what we all should strive for when describing our changes.
To help curb this lack of detail in ChangeLogs, I propose we add script (or augment an existing script) to check for this missing information and inform the contributor. It is clear not all reviewers are asking patch authors to provide this information when reviewing, and such a tool would help enforce it.
I think this is a reasonable suggestion. I think we should make the contributor docs more clear about what is expected, as well.
I think this is a reasonable suggestion, but I don't agree with it :). I would prefer that we try to get good changelogs through culture and convention rather than through good tooling. This is of course based on my experience in my changes and the types of changes I review, but I personally find what value there is at all in ChangeLogs in the paragraphs at the top of the change, and I find the lists of changed files and to be distracting noise far more often than not. (Perhaps things are different in changes to the core rendering code than changes to tooling and test code). I think it is difficult to say what a "good" changelog is an any sort of algorithmic sense, and trying to implement something that would be done programmatically will be more annoying than useful (even if it means that I just have to delete a bunch of "OOPS" lines). -- Dirk
Regards, Maciej
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Mar 21, 2012, at 3:14 PM, Dirk Pranke wrote:
On Wed, Mar 21, 2012 at 2:33 PM, Maciej Stachowiak <mjs@apple.com> wrote:
On Mar 21, 2012, at 2:29 PM, Timothy Hatcher wrote:
Lately I have observed more and more and more changes going into WebKit that lack any details about why a particular change was made. It is intended that the ChangeLog (and commit message) contain some details about your change, not just the bug title and URL.
The contributing information on subject is pretty sparse, which has likely caused this problem to manifest. However, the example ChangeLog linked from that page is prime example of what we all should strive for when describing our changes.
To help curb this lack of detail in ChangeLogs, I propose we add script (or augment an existing script) to check for this missing information and inform the contributor. It is clear not all reviewers are asking patch authors to provide this information when reviewing, and such a tool would help enforce it.
I think this is a reasonable suggestion. I think we should make the contributor docs more clear about what is expected, as well.
I think this is a reasonable suggestion, but I don't agree with it :). I would prefer that we try to get good changelogs through culture and convention rather than through good tooling.
This is of course based on my experience in my changes and the types of changes I review, but I personally find what value there is at all in ChangeLogs in the paragraphs at the top of the change, and I find the lists of changed files and to be distracting noise far more often than not. (Perhaps things are different in changes to the core rendering code than changes to tooling and test code).
I agree that setting the right conventions, and getting agreement on this, is the most important step. I think the ChangeLogs that are most worrisome are the ones that do not even have an explanatory paragraph at the top, just a bug URL and title. It can be hard to understand the I often really appreciate ChangeLogs with file-by-file or even function-by-function explanations, but I wouldn't say that is the bare minimum required for all changes. For simple changes, a short paragraph above may be totally sufficient. It is very rare that just the bug URL and title are sufficient, though. Regards, Maciej
On Mar 21, 2012, at 3:14 PM, Dirk Pranke wrote:
I think this is a reasonable suggestion, but I don't agree with it :). I would prefer that we try to get good changelogs through culture and convention rather than through good tooling.
This is of course based on my experience in my changes and the types of changes I review, but I personally find what value there is at all in ChangeLogs in the paragraphs at the top of the change, and I find the lists of changed files and to be distracting noise far more often than not. (Perhaps things are different in changes to the core rendering code than changes to tooling and test code).
I find the comments useful, even for scripts. ChangeLogs for tests are often more mundane. My particular interest is the Web Inspector, which I follow by watching bugs and commits. Often I find myself asking "why?" or "what does this do?" when perusing the commits. It sometimes isn't very obvious and a nice concise description in the ChangeLog would help. This is even more important when folks are separated by timezones or are not easy to reach for explanation. They also provide insight when looking back on changes from months or years ago when tracking down a regression.
I think it is difficult to say what a "good" changelog is an any sort of algorithmic sense, and trying to implement something that would be done programmatically will be more annoying than useful (even if it means that I just have to delete a bunch of "OOPS" lines).
It would be difficult to make the tool smart. I merely looking for reminder to push folks to describe their changes in some fashion, not a analytical tool parsing for good vs bad. — Timothy Hatcher
On Wed, Mar 21, 2012 at 4:18 PM, Timothy Hatcher <timothy@apple.com> wrote:
On Mar 21, 2012, at 3:14 PM, Dirk Pranke wrote:
I think this is a reasonable suggestion, but I don't agree with it :). I would prefer that we try to get good changelogs through culture and convention rather than through good tooling.
This is of course based on my experience in my changes and the types of changes I review, but I personally find what value there is at all in ChangeLogs in the paragraphs at the top of the change, and I find the lists of changed files and to be distracting noise far more often than not. (Perhaps things are different in changes to the core rendering code than changes to tooling and test code).
I find the comments useful, even for scripts. ChangeLogs for tests are often more mundane.
I should clarify that I do find value in describing the why of the change, so I could agree w/ Maciej that requiring *something* more than the subject + bug + reviewer might be justified for changes. Just linking to a bug would be overly terse. My annoyance with ChangeLogs stems from just integrating them into my workflow (which often involves multiple pipelined changes and/or lots of merging and rebasing), but that's off-topic for this thread. I am also sometimes annoyed by ChangeLogs when a change happens to span multiple ChangeLogs and I either have to repeat myself or figure out how to split my description between the files, which is slightly more on-topic. Mostly I just don't want there to be a lot of boilerplate or noise in the ChangeLogs. -- Dirk
My particular interest is the Web Inspector, which I follow by watching bugs and commits. Often I find myself asking "why?" or "what does this do?" when perusing the commits. It sometimes isn't very obvious and a nice concise description in the ChangeLog would help. This is even more important when folks are separated by timezones or are not easy to reach for explanation.
They also provide insight when looking back on changes from months or years ago when tracking down a regression.
I think it is difficult to say what a "good" changelog is an any sort of algorithmic sense, and trying to implement something that would be done programmatically will be more annoying than useful (even if it means that I just have to delete a bunch of "OOPS" lines).
It would be difficult to make the tool smart. I merely looking for reminder to push folks to describe their changes in some fashion, not a analytical tool parsing for good vs bad.
— Timothy Hatcher
There are some changes which have bug descriptions which are complete enough to not need additional comments and any changes to existing scripts should keep this in mind. One way to do this is to check for the number of lines/files the diff has. Konrad Sent from my BlackBerry on the Rogers Wireless Network From: Timothy Hatcher [mailto:timothy@apple.com] Sent: Wednesday, March 21, 2012 05:29 PM To: webkit-dev@lists.webkit.org <webkit-dev@lists.webkit.org> Subject: [webkit-dev] ChangeLogs Lately I have observed more<http://trac.webkit.org/changeset/111595> and more<http://trac.webkit.org/changeset/111577> and more<http://trac.webkit.org/changeset/111527> changes going into WebKit that lack any details about why a particular change was made. It is intended that the ChangeLog (and commit message) contain some details about your change, not just the bug title and URL. The contributing information<http://www.webkit.org/coding/contributing.html#changelogs> on subject is pretty sparse, which has likely caused this problem to manifest. However, the example ChangeLog<http://trac.webkit.org/changeset/43259> linked from that page is prime example of what we all should strive for when describing our changes. To help curb this lack of detail in ChangeLogs, I propose we add script<https://bugs.webkit.org/show_bug.cgi?id=81828> (or augment an existing script) to check for this missing information and inform the contributor. It is clear not all reviewers are asking patch authors to provide this information when reviewing, and such a tool would help enforce it. — Timothy Hatcher --------------------------------------------------------------------- This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.
On Wed, Mar 21, 2012 at 2:29 PM, Timothy Hatcher <timothy@apple.com> wrote:
Lately I have observed more and more and more changes going into WebKit that lack any details about why a particular change was made. It is intended that the ChangeLog (and commit message) contain some details about your change, not just the bug title and URL.
Since I was the reviewer for <http://trac.webkit.org/changeset/111577> (although if you look in the bug thread, you'll see that I was just forwarding fishd's R+ after some trivial cleanups), I'm curious what additional information you think should have been included in that ChangeLog. There really isn't much to say about that change. Tommy was just adding thin API wrappers around WebCore objects as part of the implementation of the MediaStream API. The main point of discussion in the bug thread was whether to use the name ICE or Ice. Adam
On Mar 21, 2012, at 2:46 PM, Adam Barth wrote:
On Wed, Mar 21, 2012 at 2:29 PM, Timothy Hatcher <timothy@apple.com> wrote:
Lately I have observed more and more and more changes going into WebKit that lack any details about why a particular change was made. It is intended that the ChangeLog (and commit message) contain some details about your change, not just the bug title and URL.
Since I was the reviewer for <http://trac.webkit.org/changeset/111577> (although if you look in the bug thread, you'll see that I was just forwarding fishd's R+ after some trivial cleanups), I'm curious what additional information you think should have been included in that ChangeLog.
There really isn't much to say about that change. Tommy was just adding thin API wrappers around WebCore objects as part of the implementation of the MediaStream API. The main point of discussion in the bug thread was whether to use the name ICE or Ice.
Adam
"Adding thin API wrappers around WebCore objects as part of the implementation of the MediaStream API." would be a good start. I shouldn't have to troll through a bug with 21 comments to figure out a change. Also the WebCore changes are just case changes, but you wouldn't know that by just perusing the ChangeLog/commit message. — Timothy Hatcher
On Wed, Mar 21, 2012 at 3:45 PM, Timothy Hatcher <timothy@apple.com> wrote:
On Mar 21, 2012, at 2:46 PM, Adam Barth wrote:
On Wed, Mar 21, 2012 at 2:29 PM, Timothy Hatcher <timothy@apple.com> wrote:
Lately I have observed more and more and more changes going into WebKit that lack any details about why a particular change was made. It is intended that the ChangeLog (and commit message) contain some details about your change, not just the bug title and URL.
Since I was the reviewer for <http://trac.webkit.org/changeset/111577> (although if you look in the bug thread, you'll see that I was just forwarding fishd's R+ after some trivial cleanups), I'm curious what additional information you think should have been included in that ChangeLog.
There really isn't much to say about that change. Tommy was just adding thin API wrappers around WebCore objects as part of the implementation of the MediaStream API. The main point of discussion in the bug thread was whether to use the name ICE or Ice.
"Adding thin API wrappers around WebCore objects as part of the implementation of the MediaStream API." would be a good start. I shouldn't have to troll through a bug with 21 comments to figure out a change. Also the WebCore changes are just case changes, but you wouldn't know that by just perusing the ChangeLog/commit message.
Yeah, I definitely find ChangeLog messages useful for filtering which changes to dig into in more detail (e.g., when hunting for a regression). Thanks! Adam
participants (7)
-
Adam Barth
-
David Levin
-
Dirk Pranke
-
Konrad Piascik
-
Maciej Stachowiak
-
Ryosuke Niwa
-
Timothy Hatcher