[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
On Wed, Jul 22, 2009 at 11:57 AM, Ojan Vafai o...@chromium.org wrote: On Wed, Jul 22, 2009 at 10:07 AM, Darin Fisher da...@chromium.org wrote: On Wed, Jul 22, 2009 at 10:03 AM, Adam Langley a...@chromium.org wrote: * By having the ChangeLog in the review, reviewers can critique it. Many of our commit messages are little brief. Some are far too brief. But, the commit message should match the change message on the code review, so our reviewers can already critique it. So, this would appear to be a review issue rather than a technological issue. I think we need to remember to review the CL description. It is easy to overlook when reviewing an issue. This is an understatement. We really do a poor job with commit descriptions. There is a lot to be gained by having better commit descriptions. We can learn from WebKit process here without adding the burden of ChangeLog files. They have great change descriptions and it's frequently very useful. TOTALLY agree. We're too lax in reviewing CL descriptions in Chrome. WebKit reviewers scrutinizes them as much as the code itself, and that's a good thing. To be a bit more concrete, gcl change could autopopulate your change description with something like the following that has the list of modified functions: DETAILED DESCRIPTION HERE BUG=required TEST=required RELEASE_NOTES=optional Autogenerated list of modified methods here. So here's what an example would look like (poached from AGL's recent webkit commit): Chromium Linux: Allow for setting the global font rendering preferences. Added static functions for setting hinting, anti-alias and subpixel glyph preferences. BUG=12345 TEST=none RELEASE_NOTES=none I feel like RELEASE_NOTES should only be there if it matters (i.e. isn't 'none'). Honestly, I'd say the same for TEST. Maybe the default CL description when you first run 'gcl change' should add BUG=\n TEST=\n RELEASE_NOTES= but then have the convention that we should delete them if they don't apply to our change? platform/graphics/chromium/FontPlatformDataLinux.cpp: (WebCore::FontPlatformData::setHinting): (WebCore::FontPlatformData::setAntiAlias): (WebCore::FontPlatformData::setSubpixelGlyphs): (WebCore::FontPlatformData::setupPaint): Modified to do something super special. platform/graphics/chromium/FontPlatformDataLinux.h: Is it worth putting this into the change list? If so, maybe 'gcl commit' can do it (since doing it any earlier risks it getting stale as the CL changes). I know a couple of WebKit developers find it helpful, but I'm not sure it's worth the overhead. Especially if people are used to it currently. Also note that when a function or file warrants a special note, there's nothing stopping someone from making that note whether or not we have a convention of always listing the files/functions modified. J --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
On Wed, Jul 22, 2009 at 12:26 PM, Jeremy Orlow jor...@chromium.org wrote: On Wed, Jul 22, 2009 at 11:57 AM, Ojan Vafai o...@chromium.org wrote: This is an understatement. We really do a poor job with commit descriptions. There is a lot to be gained by having better commit descriptions. We can learn from WebKit process here without adding the burden of ChangeLog files. They have great change descriptions and it's frequently very useful. TOTALLY agree. We're too lax in reviewing CL descriptions in Chrome. WebKit reviewers scrutinizes them as much as the code itself, and that's a good thing. Forgot to say, if someone felt really motivated to make this scrutinization happen, a change to reitveld to allow commenting on the change description and making it part of the list of things to review would work great for this. platform/graphics/chromium/FontPlatformDataLinux.cpp: (WebCore::FontPlatformData::setHinting): (WebCore::FontPlatformData::setAntiAlias): (WebCore::FontPlatformData::setSubpixelGlyphs): (WebCore::FontPlatformData::setupPaint): Modified to do something super special. platform/graphics/chromium/FontPlatformDataLinux.h: Is it worth putting this into the change list? If so, maybe 'gcl commit' can do it (since doing it any earlier risks it getting stale as the CL changes). The benefit of this is that you can give more detailed comments inline. You see some WebKit commits do this (e.g. Darin Adler's) and it makes it much more clear from reading the change description what happened. So, if we're going to do it, it's not useful to do at commit time. It would be great if people got in the habit of writing detailed descriptions like this. Here's an example of a Darin Adler commit where this leads to a nice clear and detailed description. WebCore: 2009-07-15 Darin Adler da...@apple.com da...@apple.com Reviewed by John Sullivan. After double-clicking a word, using Shift-arrow to select behaves unpredictably https://bugs.webkit.org/show_bug.cgi?id=27177https://bugs.webkit.org/show_bug.cgi?id=27177 rdar://problem/7034324 rdar://problem/7034324 Test: editing/selection/extend-selection-after-double-click.html The bug was due to the m_lastChangeWasHorizontalExtension flag, which was not being cleared in many cases where it should have been. - editing/SelectionController.cpp: (WebCore::SelectionController::setSelection): Set m_lastChangeWasHorizontalExtension to false. This catches all sorts of cases that don't flow through the modify function. Before, the flag would reflect the last call to the modify function, which was not necessarily the last selection change. (WebCore::SelectionController::willBeModified): Rearrange function for clarity. Remove code that sets m_lastChangeWasHorizontalExtension; that is now handled elsewhere. (WebCore::SelectionController::modify): Call setLastChangeWasHorizontalExtension after setSelection when setting up a trial selection controller, since setSelection now clears that flag. Also changed both trial selection controller cases to set the flag, although it's not strictly necessary in both cases. Added code to set m_lastChangeWasHorizontalExtension when extending the selection, which used to be handled in willBeModified. Now we need to do it after the selection change. LayoutTests: 2009-07-15 Darin Adler da...@apple.com da...@apple.com Reviewed by John Sullivan. After double-clicking a word, using Shift-arrow to select behaves unpredictably https://bugs.webkit.org/show_bug.cgi?id=27177https://bugs.webkit.org/show_bug.cgi?id=27177 rdar://problem/7034324 rdar://problem/7034324 - editing/selection/extend-selection-after-double-click-expected.txt: Added. - editing/selection/extend-selection-after-double-click.html: Copied from LayoutTests/editing/selection/word-granularity.html. Then turned it into a new test. --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
On Wed, Jul 22, 2009 at 1:17 PM, Ojan Vafai o...@chromium.org wrote: platform/graphics/chromium/FontPlatformDataLinux.cpp: (WebCore::FontPlatformData::setHinting): (WebCore::FontPlatformData::setAntiAlias): (WebCore::FontPlatformData::setSubpixelGlyphs): (WebCore::FontPlatformData::setupPaint): Modified to do something super special. platform/graphics/chromium/FontPlatformDataLinux.h: Is it worth putting this into the change list? If so, maybe 'gcl commit' can do it (since doing it any earlier risks it getting stale as the CL changes). The benefit of this is that you can give more detailed comments inline. You see some WebKit commits do this (e.g. Darin Adler's) and it makes it much more clear from reading the change description what happened. So, if we're going to do it, it's not useful to do at commit time. It would be great if people got in the habit of writing detailed descriptions like this. I really, really hate this style. I have never encountered a case where a combination of comments-in-code and good-change-description are not both sufficient, and far more readable than the above style. Here's an example of a Darin Adler commit where this leads to a nice clear and detailed description. WebCore: 2009-07-15 Darin Adler da...@apple.com da...@apple.com Reviewed by John Sullivan. After double-clicking a word, using Shift-arrow to select behaves unpredictably https://bugs.webkit.org/show_bug.cgi?id=27177https://bugs.webkit.org/show_bug.cgi?id=27177 rdar://problem/7034324 Test: editing/selection/extend-selection-after-double-click.html The bug was due to the m_lastChangeWasHorizontalExtension flag, which was not being cleared in many cases where it should have been. - editing/SelectionController.cpp: (WebCore::SelectionController::setSelection): Set m_lastChangeWasHorizontalExtension to false. This catches all sorts of cases that don't flow through the modify function. Before, the flag would reflect the last call to the modify function, which was not necessarily the last selection change. This kind of thing should be a comment in the code, and the fact that WebKit does not generally comment code is much to their detriment. - editing/selection/extend-selection-after-double-click.html: Copied from LayoutTests/editing/selection/word-granularity.html. Then turned it into a new test. This kind of thing, if really important, should be a comment in the change description. PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
platform/graphics/chromium/FontPlatformDataLinux.cpp: (WebCore::FontPlatformData::setHinting): (WebCore::FontPlatformData::setAntiAlias): (WebCore::FontPlatformData::setSubpixelGlyphs): (WebCore::FontPlatformData::setupPaint): Modified to do something super special. platform/graphics/chromium/FontPlatformDataLinux.h: Is it worth putting this into the change list? If so, maybe 'gcl commit' can do it (since doing it any earlier risks it getting stale as the CL changes). The benefit of this is that you can give more detailed comments inline. You see some WebKit commits do this (e.g. Darin Adler's) and it makes it much more clear from reading the change description what happened. So, if we're going to do it, it's not useful to do at commit time. It would be great if people got in the habit of writing detailed descriptions like this. I am against ... oh, PK already responded and said exactly what I was gonna say. If there are notes that are relevant to some bit of code, they belong as comments beside the code. --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
On Wed, Jul 22, 2009 at 1:22 PM, Peter Kasting pkast...@google.com wrote: On Wed, Jul 22, 2009 at 1:17 PM, Ojan Vafai o...@chromium.org wrote: platform/graphics/chromium/FontPlatformDataLinux.cpp: (WebCore::FontPlatformData::setHinting): (WebCore::FontPlatformData::setAntiAlias): (WebCore::FontPlatformData::setSubpixelGlyphs): (WebCore::FontPlatformData::setupPaint): Modified to do something super special. platform/graphics/chromium/FontPlatformDataLinux.h: Is it worth putting this into the change list? If so, maybe 'gcl commit' can do it (since doing it any earlier risks it getting stale as the CL changes). The benefit of this is that you can give more detailed comments inline. You see some WebKit commits do this (e.g. Darin Adler's) and it makes it much more clear from reading the change description what happened. So, if we're going to do it, it's not useful to do at commit time. It would be great if people got in the habit of writing detailed descriptions like this. I really, really hate this style. I have never encountered a case where a combination of comments-in-code and good-change-description are not both sufficient, and far more readable than the above style. Here's an example of a Darin Adler commit where this leads to a nice clear and detailed description. WebCore: 2009-07-15 Darin Adler da...@apple.com da...@apple.com Reviewed by John Sullivan. After double-clicking a word, using Shift-arrow to select behaves unpredictably https://bugs.webkit.org/show_bug.cgi?id=27177https://bugs.webkit.org/show_bug.cgi?id=27177 rdar://problem/7034324 Test: editing/selection/extend-selection-after-double-click.html The bug was due to the m_lastChangeWasHorizontalExtension flag, which was not being cleared in many cases where it should have been. - editing/SelectionController.cpp: (WebCore::SelectionController::setSelection): Set m_lastChangeWasHorizontalExtension to false. This catches all sorts of cases that don't flow through the modify function. Before, the flag would reflect the last call to the modify function, which was not necessarily the last selection change. This kind of thing should be a comment in the code, and the fact that WebKit does not generally comment code is much to their detriment. Yes. For some reason, WebKit likes to replace comments in the code with comments in the ChangeLog which seems insane to me. If you need comments this detailed in the ChangeLog then something is wrong with the code IMHO. I think there are very few cases where you need down to the function level of detail. In most case, you should be putting comments in the code or splitting your CL into multiple that each address one specific issue. - editing/selection/extend-selection-after-double-click.html: Copied from LayoutTests/editing/selection/word-granularity.html. Then turned it into a new test. This kind of thing, if really important, should be a comment in the change description. PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
I'm OK with not having the function list. I can see the advantages of the webkit approach as it means that comments in the code don't get outdated. But, then it forces you to look at the changelog history of every line of code. Anyways, how about we prepopulate gcl/git-cl change with this text: DETAILED_DESCRIPTION_HERE BUG=http://bugs.chromium.org/BUG_NUMBER_HERE TEST=required RELEASE_NOTES=optional On Wed, Jul 22, 2009 at 1:28 PM, Jeremy Orlow jor...@chromium.org wrote: On Wed, Jul 22, 2009 at 1:22 PM, Peter Kasting pkast...@google.comwrote: On Wed, Jul 22, 2009 at 1:17 PM, Ojan Vafai o...@chromium.org wrote: platform/graphics/chromium/FontPlatformDataLinux.cpp: (WebCore::FontPlatformData::setHinting): (WebCore::FontPlatformData::setAntiAlias): (WebCore::FontPlatformData::setSubpixelGlyphs): (WebCore::FontPlatformData::setupPaint): Modified to do something super special. platform/graphics/chromium/FontPlatformDataLinux.h: Is it worth putting this into the change list? If so, maybe 'gcl commit' can do it (since doing it any earlier risks it getting stale as the CL changes). The benefit of this is that you can give more detailed comments inline. You see some WebKit commits do this (e.g. Darin Adler's) and it makes it much more clear from reading the change description what happened. So, if we're going to do it, it's not useful to do at commit time. It would be great if people got in the habit of writing detailed descriptions like this. I really, really hate this style. I have never encountered a case where a combination of comments-in-code and good-change-description are not both sufficient, and far more readable than the above style. Here's an example of a Darin Adler commit where this leads to a nice clear and detailed description. WebCore: 2009-07-15 Darin Adler da...@apple.com da...@apple.com Reviewed by John Sullivan. After double-clicking a word, using Shift-arrow to select behaves unpredictably https://bugs.webkit.org/show_bug.cgi?id=27177https://bugs.webkit.org/show_bug.cgi?id=27177 rdar://problem/7034324 Test: editing/selection/extend-selection-after-double-click.html The bug was due to the m_lastChangeWasHorizontalExtension flag, which was not being cleared in many cases where it should have been. - editing/SelectionController.cpp: (WebCore::SelectionController::setSelection): Set m_lastChangeWasHorizontalExtension to false. This catches all sorts of cases that don't flow through the modify function. Before, the flag would reflect the last call to the modify function, which was not necessarily the last selection change. This kind of thing should be a comment in the code, and the fact that WebKit does not generally comment code is much to their detriment. Yes. For some reason, WebKit likes to replace comments in the code with comments in the ChangeLog which seems insane to me. If you need comments this detailed in the ChangeLog then something is wrong with the code IMHO. I think there are very few cases where you need down to the function level of detail. In most case, you should be putting comments in the code or splitting your CL into multiple that each address one specific issue. - editing/selection/extend-selection-after-double-click.html: Copied from LayoutTests/editing/selection/word-granularity.html. Then turned it into a new test. This kind of thing, if really important, should be a comment in the change description. PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
Here comes the bike shedding...but what about this: BUG= TEST= RELEASE_NOTES= I don't think any ALL CAPS TEXT at the top is going to make people put in better descriptions. If you want bug to be a url, it should use crbug.cominstead of the full string, but the convention so far has been a bar bug number which I think is fine. I don't really see why TEST is required...it's none much more often than BUG. I think simply populating it by default will get people to annotate it if they have anything to say. Otherwise they should just delete it. And I don't like the idea of needing to delete instructions on how to use the change descriptions every time I make a CL. I think what I'm proposing makes it obvious enough for newcomers while optimizing for people who submit a lot of CLs. J On Wed, Jul 22, 2009 at 1:43 PM, Ojan Vafai o...@chromium.org wrote: I'm OK with not having the function list. I can see the advantages of the webkit approach as it means that comments in the code don't get outdated. But, then it forces you to look at the changelog history of every line of code. Anyways, how about we prepopulate gcl/git-cl change with this text: DETAILED_DESCRIPTION_HERE BUG=http://bugs.chromium.org/BUG_NUMBER_HERE TEST=required RELEASE_NOTES=optional On Wed, Jul 22, 2009 at 1:28 PM, Jeremy Orlow jor...@chromium.org wrote: On Wed, Jul 22, 2009 at 1:22 PM, Peter Kasting pkast...@google.comwrote: On Wed, Jul 22, 2009 at 1:17 PM, Ojan Vafai o...@chromium.org wrote: platform/graphics/chromium/FontPlatformDataLinux.cpp: (WebCore::FontPlatformData::setHinting): (WebCore::FontPlatformData::setAntiAlias): (WebCore::FontPlatformData::setSubpixelGlyphs): (WebCore::FontPlatformData::setupPaint): Modified to do something super special. platform/graphics/chromium/FontPlatformDataLinux.h: Is it worth putting this into the change list? If so, maybe 'gcl commit' can do it (since doing it any earlier risks it getting stale as the CL changes). The benefit of this is that you can give more detailed comments inline. You see some WebKit commits do this (e.g. Darin Adler's) and it makes it much more clear from reading the change description what happened. So, if we're going to do it, it's not useful to do at commit time. It would be great if people got in the habit of writing detailed descriptions like this. I really, really hate this style. I have never encountered a case where a combination of comments-in-code and good-change-description are not both sufficient, and far more readable than the above style. Here's an example of a Darin Adler commit where this leads to a nice clear and detailed description. WebCore: 2009-07-15 Darin Adler da...@apple.com da...@apple.com Reviewed by John Sullivan. After double-clicking a word, using Shift-arrow to select behaves unpredictably https://bugs.webkit.org/show_bug.cgi?id=27177https://bugs.webkit.org/show_bug.cgi?id=27177 rdar://problem/7034324 Test: editing/selection/extend-selection-after-double-click.html The bug was due to the m_lastChangeWasHorizontalExtension flag, which was not being cleared in many cases where it should have been. - editing/SelectionController.cpp: (WebCore::SelectionController::setSelection): Set m_lastChangeWasHorizontalExtension to false. This catches all sorts of cases that don't flow through the modify function. Before, the flag would reflect the last call to the modify function, which was not necessarily the last selection change. This kind of thing should be a comment in the code, and the fact that WebKit does not generally comment code is much to their detriment. Yes. For some reason, WebKit likes to replace comments in the code with comments in the ChangeLog which seems insane to me. If you need comments this detailed in the ChangeLog then something is wrong with the code IMHO. I think there are very few cases where you need down to the function level of detail. In most case, you should be putting comments in the code or splitting your CL into multiple that each address one specific issue. - editing/selection/extend-selection-after-double-click.html: Copied from LayoutTests/editing/selection/word-granularity.html. Then turned it into a new test. This kind of thing, if really important, should be a comment in the change description. PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
On Wed, Jul 22, 2009 at 1:49 PM, Jeremy Orlow jor...@chromium.org wrote: Here comes the bike shedding Yes, Evan already concluded that was where we were. RELEASE_NOTES= I thought the conclusion was that people should just write better commit messages? Maybe I'm wrong. I'd almost prefer: Replace this with one-line summary of your change Replace this with more details as needed BUG= TEST= ...although I also agree with you that deleting instruction can be really annoying. PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
Bug filed: http://code.google.com/p/chromium/issues/detail?id=17471 On Wed, Jul 22, 2009 at 1:49 PM, Jeremy Orlow jor...@chromium.org wrote: Here comes the bike shedding...but what about this: BUG= TEST= RELEASE_NOTES= I don't think any ALL CAPS TEXT at the top is going to make people put in better descriptions. If you want bug to be a url, it should use crbug.cominstead of the full string, but the convention so far has been a bar bug number which I think is fine. I don't really see why TEST is required...it's none much more often than BUG. I think simply populating it by default will get people to annotate it if they have anything to say. Otherwise they should just delete it. And I don't like the idea of needing to delete instructions on how to use the change descriptions every time I make a CL. I think what I'm proposing makes it obvious enough for newcomers while optimizing for people who submit a lot of CLs. J On Wed, Jul 22, 2009 at 1:43 PM, Ojan Vafai o...@chromium.org wrote: I'm OK with not having the function list. I can see the advantages of the webkit approach as it means that comments in the code don't get outdated. But, then it forces you to look at the changelog history of every line of code. Anyways, how about we prepopulate gcl/git-cl change with this text: DETAILED_DESCRIPTION_HERE BUG=http://bugs.chromium.org/BUG_NUMBER_HERE TEST=required RELEASE_NOTES=optional On Wed, Jul 22, 2009 at 1:28 PM, Jeremy Orlow jor...@chromium.orgwrote: On Wed, Jul 22, 2009 at 1:22 PM, Peter Kasting pkast...@google.comwrote: On Wed, Jul 22, 2009 at 1:17 PM, Ojan Vafai o...@chromium.org wrote: platform/graphics/chromium/FontPlatformDataLinux.cpp: (WebCore::FontPlatformData::setHinting): (WebCore::FontPlatformData::setAntiAlias): (WebCore::FontPlatformData::setSubpixelGlyphs): (WebCore::FontPlatformData::setupPaint): Modified to do something super special. platform/graphics/chromium/FontPlatformDataLinux.h: Is it worth putting this into the change list? If so, maybe 'gcl commit' can do it (since doing it any earlier risks it getting stale as the CL changes). The benefit of this is that you can give more detailed comments inline. You see some WebKit commits do this (e.g. Darin Adler's) and it makes it much more clear from reading the change description what happened. So, if we're going to do it, it's not useful to do at commit time. It would be great if people got in the habit of writing detailed descriptions like this. I really, really hate this style. I have never encountered a case where a combination of comments-in-code and good-change-description are not both sufficient, and far more readable than the above style. Here's an example of a Darin Adler commit where this leads to a nice clear and detailed description. WebCore: 2009-07-15 Darin Adler da...@apple.com da...@apple.com Reviewed by John Sullivan. After double-clicking a word, using Shift-arrow to select behaves unpredictably https://bugs.webkit.org/show_bug.cgi?id=27177https://bugs.webkit.org/show_bug.cgi?id=27177 rdar://problem/7034324 Test: editing/selection/extend-selection-after-double-click.html The bug was due to the m_lastChangeWasHorizontalExtension flag, which was not being cleared in many cases where it should have been. - editing/SelectionController.cpp: (WebCore::SelectionController::setSelection): Set m_lastChangeWasHorizontalExtension to false. This catches all sorts of cases that don't flow through the modify function. Before, the flag would reflect the last call to the modify function, which was not necessarily the last selection change. This kind of thing should be a comment in the code, and the fact that WebKit does not generally comment code is much to their detriment. Yes. For some reason, WebKit likes to replace comments in the code with comments in the ChangeLog which seems insane to me. If you need comments this detailed in the ChangeLog then something is wrong with the code IMHO. I think there are very few cases where you need down to the function level of detail. In most case, you should be putting comments in the code or splitting your CL into multiple that each address one specific issue. - editing/selection/extend-selection-after-double-click.html: Copied from LayoutTests/editing/selection/word-granularity.html. Then turned it into a new test. This kind of thing, if really important, should be a comment in the change description. PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
On Wed, Jul 22, 2009 at 2:52 PM, Darin Fisher da...@chromium.org wrote: On Wed, Jul 22, 2009 at 2:02 PM, Peter Kasting pkast...@google.comwrote: On Wed, Jul 22, 2009 at 1:49 PM, Jeremy Orlow jor...@chromium.orgwrote: Here comes the bike shedding Yes, Evan already concluded that was where we were. RELEASE_NOTES= I thought the conclusion was that people should just write better commit messages? Maybe I'm wrong. I'd almost prefer: Replace this with one-line summary of your change Replace this with more details as needed BUG= TEST= ...although I also agree with you that deleting instruction can be really annoying. PK I like the suggestion of a short (one line) summary followed by a paragraph. I also like the BUG=, TEST= fields. What about R=/TBR=? Is this something people typically grep for? If not, then I think the codereview.chromium.org link is enough. --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
On Wed, Jul 22, 2009 at 14:02, Peter Kasting pkast...@google.com wrote: On Wed, Jul 22, 2009 at 1:49 PM, Jeremy Orlow jor...@chromium.org wrote: Here comes the bike shedding Yes, Evan already concluded that was where we were. RELEASE_NOTES= I thought the conclusion was that people should just write better commit messages? I still want a RELEASE_NOTES flag, but not a NOTES=I'm repeating what I just wrote up above. Writing concise summaries as the first line of a description is good form that we should encourage for all changes, not just noteworthy changes. Given a token that says hey, this is noteworthy or a user visible change, we can write scripts to pull out the first line of the description for inclusion in the release notes. I'll look for a place to document writing good log messages on dev.chromium.org. --Mark +1 for painting the bikeshed blue Maybe I'm wrong. I'd almost prefer: Replace this with one-line summary of your change Replace this with more details as needed BUG= TEST= ...although I also agree with you that deleting instruction can be really annoying. PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
On Wed, Jul 22, 2009 at 2:02 PM, Peter Kasting pkast...@google.com wrote: On Wed, Jul 22, 2009 at 1:49 PM, Jeremy Orlow jor...@chromium.org wrote: Here comes the bike shedding Yes, Evan already concluded that was where we were. RELEASE_NOTES= I thought the conclusion was that people should just write better commit messages? Maybe I'm wrong. I'd almost prefer: Replace this with one-line summary of your change Replace this with more details as needed BUG= TEST= ...although I also agree with you that deleting instruction can be really annoying. PK I like the suggestion of a short (one line) summary followed by a paragraph. I also like the BUG=, TEST= fields. What about R=/TBR=? -Darin --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: better commit descriptions WAS: Proposal for adding ChangeLog files to Chromium
Even though git-cl doesn't have presubmit checks per se, I do like how it forces your first line to = 100 characters (rietveld limitation, I believe). gcl gets around this issue by truncating your description and adding an ellipsis. On Wed, Jul 22, 2009 at 2:52 PM, Darin Fisher da...@chromium.org wrote: On Wed, Jul 22, 2009 at 2:02 PM, Peter Kasting pkast...@google.comwrote: On Wed, Jul 22, 2009 at 1:49 PM, Jeremy Orlow jor...@chromium.orgwrote: Here comes the bike shedding Yes, Evan already concluded that was where we were. RELEASE_NOTES= I thought the conclusion was that people should just write better commit messages? Maybe I'm wrong. I'd almost prefer: Replace this with one-line summary of your change Replace this with more details as needed BUG= TEST= ...although I also agree with you that deleting instruction can be really annoying. PK I like the suggestion of a short (one line) summary followed by a paragraph. I also like the BUG=, TEST= fields. What about R=/TBR=? -Darin --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
