Proposal: Commit messages should start with a one-line summary of the change
I'd like to propose that WebKit commit messages start with a one-line summary of the change. This change would have two benefits: 1) It would make it much easier to understand at a glance what the change actually does. Our commit logs usually include a bug title, but the bug title only tells you about the problem, not about the solution. 2) It would make many of our tools work better. For instance, <http://trac.webkit.org/log/trunk> shows the first 75 characters or so of the commit message. Right now it's just a wall of dates and author names, which are already shown in other parts of the UI. It would show much more useful information if we had a one-line summary at the top of the commit message. As another example, pretty much all git-based tools assume the commit message starts with a one-line summary (e.g., git log --pretty=oneline, gitk, webkit-patch post-commits, etc.), and work much better when that is the case. Since our commit messages are almost always just a copy of our ChangeLog entries, this should apply to ChangeLog entries too. Specifically, I suggest one line be added to our ChangeLog template, yielding the following: 2011-06-30 Adam Roben <aroben@apple.com> Need a one-line summary of your change (OOPS!) Reviewed by NOBODY (OOPS!). Need a short description and bug URL (OOPS!) * FileIModified.cpp: Given this format, commit-log-editor will put the summary right at the top of the commit log. webkit-patch will require modifications to do this correctly, as represented by <https://bugs.webkit.org/show_bug.cgi?id=26755>. Thoughts? -Adam
Sounds good to me, except I wonder how often you'd get "Fixed bug where XXXX" where XXX is the bug summary. -- Dirk On Thu, Jun 30, 2011 at 1:28 PM, Adam Roben <aroben@apple.com> wrote:
I'd like to propose that WebKit commit messages start with a one-line summary of the change.
This change would have two benefits:
1) It would make it much easier to understand at a glance what the change actually does.
Our commit logs usually include a bug title, but the bug title only tells you about the problem, not about the solution.
2) It would make many of our tools work better.
For instance, <http://trac.webkit.org/log/trunk> shows the first 75 characters or so of the commit message. Right now it's just a wall of dates and author names, which are already shown in other parts of the UI. It would show much more useful information if we had a one-line summary at the top of the commit message. As another example, pretty much all git-based tools assume the commit message starts with a one-line summary (e.g., git log --pretty=oneline, gitk, webkit-patch post-commits, etc.), and work much better when that is the case.
Since our commit messages are almost always just a copy of our ChangeLog entries, this should apply to ChangeLog entries too. Specifically, I suggest one line be added to our ChangeLog template, yielding the following:
2011-06-30 Adam Roben <aroben@apple.com>
Need a one-line summary of your change (OOPS!)
Reviewed by NOBODY (OOPS!).
Need a short description and bug URL (OOPS!)
* FileIModified.cpp:
Given this format, commit-log-editor will put the summary right at the top of the commit log. webkit-patch will require modifications to do this correctly, as represented by <https://bugs.webkit.org/show_bug.cgi?id=26755>.
Thoughts?
-Adam
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Jun 30, 2011, at 5:00 PM, Dirk Pranke wrote:
Sounds good to me, except I wonder how often you'd get "Fixed bug where XXXX" where XXX is the bug summary.
I think this is something that reviewers would have to encourage authors not to do, just like reviewers should be encouraging authors to fill out the file/function boilerplate that prepare-ChangeLog gives you with actual comments. -Adam
On 2011-06-30, at 13:28, Adam Roben wrote:
I'd like to propose that WebKit commit messages start with a one-line summary of the change.
This change would have two benefits:
1) It would make it much easier to understand at a glance what the change actually does.
Our commit logs usually include a bug title, but the bug title only tells you about the problem, not about the solution.
2) It would make many of our tools work better.
For instance, <http://trac.webkit.org/log/trunk> shows the first 75 characters or so of the commit message. Right now it's just a wall of dates and author names, which are already shown in other parts of the UI. It would show much more useful information if we had a one-line summary at the top of the commit message. As another example, pretty much all git-based tools assume the commit message starts with a one-line summary (e.g., git log --pretty=oneline, gitk, webkit-patch post-commits, etc.), and work much better when that is the case.
Since our commit messages are almost always just a copy of our ChangeLog entries, this should apply to ChangeLog entries too. Specifically, I suggest one line be added to our ChangeLog template, yielding the following:
2011-06-30 Adam Roben <aroben@apple.com>
Need a one-line summary of your change (OOPS!)
Reviewed by NOBODY (OOPS!).
Need a short description and bug URL (OOPS!)
* FileIModified.cpp:
Most ChangeLog entries already have a one-line summary immediately after the "Reviewed by" line. I'm not sure that there's any benefit to reordering these parts of the ChangeLog.
Given this format, commit-log-editor will put the summary right at the top of the commit log. webkit-patch will require modifications to do this correctly, as represented by <https://bugs.webkit.org/show_bug.cgi?id=26755>.
commit-log-editor already does the right thing given our current format. It's just that many people have switched to using webkit-patch, and it was never taught the correct format for commit messages. - Mark
On Jun 30, 2011, at 5:10 PM, Mark Rowe wrote:
Most ChangeLog entries already have a one-line summary immediately after the "Reviewed by" line. I'm not sure that there's any benefit to reordering these parts of the ChangeLog.
Most ChangeLog entries have the bug title after the "Reviewed by" line, like this one: <http://trac.webkit.org/changeset/90156/trunk/LayoutTests/ChangeLog>. As I said in my email:
Our commit logs usually include a bug title, but the bug title only tells you about the problem, not about the solution.
I don't consider the bug title and the one-line summary I'm proposing to be equivalent. I'm also not proposing moving the bug title above the "Reviewed by" line.
Given this format, commit-log-editor will put the summary right at the top of the commit log. webkit-patch will require modifications to do this correctly, as represented by <https://bugs.webkit.org/show_bug.cgi?id=26755>.
commit-log-editor already does the right thing given our current format. It's just that many people have switched to using webkit-patch, and it was never taught the correct format for commit messages.
commit-log-editor will do some wacky things that sometimes end up with the bug title and URL lines first, followed by the "Reviewed by" line. <https://bugs.webkit.org/show_bug.cgi?id=27754> seems to be the source of the wackiness. This was added in an attempt to make our commit messages work better with our tools, but doesn't help people who are browsing through ChangeLogs. I also think it's confusing that our commit messages and ChangeLogs don't follow the same order (as Adam Treat mentioned in <https://bugs.webkit.org/show_bug.cgi?id=27754#c6>). Having the bug title on the first line (as commit-log-editor currently tries to do) is better than having the date/author line first from a tools perspective. But I think having a one-line summary first would be even better. And I think it would be nice to have the ordering of our ChangeLog entries and commit messages match. -Adam
I don’t think it’s a good idea to add yet another thing to every change log entry. I do think that given the tools behavior you described, we should move the reviewer text after the bug title and patch description paragraph, even though you explicitly said that’s not what you are proposing. -- Darin
On Thu, Jun 30, 2011 at 4:52 PM, Darin Adler <darin@apple.com> wrote:
I don’t think it’s a good idea to add yet another thing to every change log entry.
I do think that given the tools behavior you described, we should move the reviewer text after the bug title and patch description paragraph, even though you explicitly said that’s not what you are proposing.
I agree with Darin's point. While I get annoyed by the fact trac log pages don't show any useful information: rniwa@webkit.org: 2011-06-30 Ryosuke Niwa Software Engineer Google Inc. < rniwa@webkit.org <rniwa@webkit.org>> Reviewed by Kent … But that can be addressed by giving the right commit message extracted from the change log as Mark suggested: On Thu, Jun 30, 2011 at 2:10 PM, Mark Rowe <mrowe@apple.com> wrote:
Most ChangeLog entries already have a one-line summary immediately after the "Reviewed by" line. I'm not sure that there's any benefit to reordering these parts of the ChangeLog.
Given this format, commit-log-editor will put the summary right at the top of the commit log. webkit-patch will require modifications to do this correctly, as represented by < https://bugs.webkit.org/show_bug.cgi?id=26755>.
commit-log-editor already does the right thing given our current format. It's just that many people have switched to using webkit-patch, and it was never taught the correct format for commit messages.
We should just fix webkit-patch so that it uses the right commit message. - Ryosuke
On Thu, Jun 30, 2011 at 4:57 PM, Ryosuke Niwa <rniwa@webkit.org> wrote:
We should just fix webkit-patch so that it uses the right commit message.
Go for it: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/checkout/... Adam
On Jun 30, 2011, at 7:52 PM, Darin Adler wrote:
I don’t think it’s a good idea to add yet another thing to every change log entry.
I wouldn't consider this something being added. Most ChangeLog entries (the good ones, anyway) already contain a description of the fix in addition to the bug title, URL and reviewer. <http://trac.webkit.org/changeset/90086> is a good example. All I'm proposing is that part of the explanation of the fix be extracted out into a one-line summary at the top of the ChangeLog. So it's not so much adding something as rearranging the information that is (or should) already be present.
I do think that given the tools behavior you described, we should move the reviewer text after the bug title and patch description paragraph, even though you explicitly said that’s not what you are proposing.
I think that would be a good first step. It would definitely be better than what we have now from a tools perspective. And in a lot of cases these days, the bug title really is a short description of the change. [1] I think these two bugs will get us started: <http://webkit.org/b/26755> webkit-patch's commit messages are less readable than commit-log-editor's <http://webkit.org/b/63804> commit-log-editor reorders ChangeLog entries in unexpected ways -Adam 1. I don't think this is necessarily a good thing; it would be better in my opinion if bug titles described user-visible symptoms or missing features. The typical webkit-patch-based workflow of writing a patch first and then uploading it to a new bug encourages these kinds of solution-based rather than issue-based bug titles. But webkit-patch makes so many things so much easier that it's hard to fault it!
I don’t think it’s a good idea to add yet another thing to every change log entry.
I wouldn't consider this something being added. Most ChangeLog entries (the good ones, anyway) already contain a description of the fix in addition to the bug title, URL and reviewer.
While I sympathize with the idea of the one line explanations, I think this adds unneeded complexity to our workflow and the added value you mentioned does not seem to overcome it (namely making a couple of tools work better). Also you are implicitly constraining people to explain their change with one-liners which IMHO is not a good constraint. I would rather see good explanations than explanations that need to match a format. Thanks, Julien
On Jul 1, 2011, at 11:57 AM, Julien Chaffraix wrote:
I don’t think it’s a good idea to add yet another thing to every change log entry.
I wouldn't consider this something being added. Most ChangeLog entries (the good ones, anyway) already contain a description of the fix in addition to the bug title, URL and reviewer.
While I sympathize with the idea of the one line explanations, I think this adds unneeded complexity to our workflow and the added value you mentioned does not seem to overcome it (namely making a couple of tools work better).
I wasn't trying to add any complexity. My hope is that people are already adding descriptions of their changes to their ChangeLog entries, like you do. I was just trying to suggest a better way to organize that description. In my original email I noted two benefits of making this change:
1) It would make it much easier to understand at a glance what the change actually does.
2) It would make many of our tools work better
I see (1) as the primary benefit, and (2) as a nice side-benefit.
Also you are implicitly constraining people to explain their change with one-liners which IMHO is not a good constraint. I would rather see good explanations than explanations that need to match a format.
I'm not trying to constrain descriptions to one line. I think that would be horrible in a lot of cases! I'm just suggesting that there be a one-line summary at the top followed by a more detailed description below. (Note that our current ChangeLog template doesn't say that you should include *any* description of the change.) -Adam
I wasn't trying to add any complexity. My hope is that people are already adding descriptions of their changes to their ChangeLog entries, like you do. I was just trying to suggest a better way to organize that description.
In my original email I noted two benefits of making this change:
1) It would make it much easier to understand at a glance what the change actually does.
2) It would make many of our tools work better
Thanks for clarifying here. I would have equated 1) to the very existence of ChangeLog which is supposed to give this background.
I see (1) as the primary benefit, and (2) as a nice side-benefit.
Also you are implicitly constraining people to explain their change with one-liners which IMHO is not a good constraint. I would rather see good explanations than explanations that need to match a format.
I'm not trying to constrain descriptions to one line. I think that would be horrible in a lot of cases! I'm just suggesting that there be a one-line summary at the top followed by a more detailed description below.
I said that it _implicitly_ added a constraint. Let me take an example to make this point clear: if you have a 1.5 line long description, it doesn't seem to make sense to split it in 2. I, maybe other people too, would side with removing maybe important information for shortness's sake. This sounds like the kind of case where the strict format is harmful. It may be theoretical but it looks like a slippery slope.
(Note that our current ChangeLog template doesn't say that you should include *any* description of the change.)
Indeed and I agree with you that this is wrong. However my feeling is that reviewers usually asks for that information to be included as part of the ChangeLog if it is not present. My argument is against the format I would say, not about making this information available / mandatory. Thanks, Julien
On Jul 1, 2011, at 10:38 AM, Adam Roben wrote:
On Jun 30, 2011, at 7:52 PM, Darin Adler wrote:
I do think that given the tools behavior you described, we should move the reviewer text after the bug title and patch description paragraph, even though you explicitly said that’s not what you are proposing.
I think that would be a good first step. It would definitely be better than what we have now from a tools perspective. And in a lot of cases these days, the bug title really is a short description of the change. [1]
I think these two bugs will get us started: <http://webkit.org/b/26755> webkit-patch's commit messages are less readable than commit-log-editor's <http://webkit.org/b/63804> commit-log-editor reorders ChangeLog entries in unexpected ways
These two bugs have now been fixed. (There was a little fallout from bug 26755, but hopefully that has been taken care of, too.) prepare-ChangeLog now puts the bug title and URL first, then the "Reviewed by" line. commit-log-editor no longer reorders the ChangeLog contents (other than extracting common lines to the top of the commit message). webkit-patch now uses commit-log-editor to create its commit messages. I think this has made some of our tools a lot more useful. (See, e.g., <http://trac.webkit.org/log/trunk?rev=90579&stop_rev=90540>. The switch from the old commit messages to the new ones is quite obvious.) I think it's also a slight improvement in readability of ChangeLogs, as in many cases the bug title is a description of the change. I'm going to drop the "put a one-line summary at the top of your commit message" suggestion for now. I think I've caused enough change for one email thread. :-) -Adam
participants (7)
-
Adam Barth
-
Adam Roben
-
Darin Adler
-
Dirk Pranke
-
Julien Chaffraix
-
Mark Rowe
-
Ryosuke Niwa