On Tue, Feb 3, 2009 at 5:26 AM, Dean McNamee <[email protected]> wrote:
> > Ever since I've been using git, I have super great and quick access to > our full history and commit logs. Being in a different time zone, I > usually warm up in the morning by reading yesterday's commits. I've > found in general we are very good and strict about review code, but > hardly anyone ever comments on the review description. I often have > to follow the code review link in the commit, and read through 30 > review messages to get the information I wanted. A few problems I see > very commonly: > > - Simple style issues: > Typos / spelling errors. Wacky line breaks, this is partially > rietveld's fault, but we should standardize on something and fix > rietveld / gcl. > > - Bigger issues > A review description that should be a review message. > Commit 1234: > "Make this change. Please review carefully, what do you think about > the last half?" > > A "funny" commit message. I'm not all dry, I like jokes, but they're > not that funny in 1 year when you're trying to find a regression. > > A review description that is completely stale from the code. When we > iterate on rounds of feedback, we should also comment on the commit > description to make sure it's updated. Because of the way GCL works, > we do the commit description first, and not last. There can be a huge > shift in the code between those points, and often the commit message > goes forgotten. > > So, here is my proposal. We review commit descriptions along with > code reviews. In general, we should think of every commit description > as if it were a comment in the code. That means proper spelling, > formatting, up to date, etc. It's important to remember that although > you might be making a quick fix that is obvious to everyone in the > room (hey, the tree was red!), in 2 days no one will remember that > situation. The code description will be left as 'qucik fix!', when it > instead should have been "Correct a typo in XXX, the previous code > fails to compile." > > Are you with me? > > Thanks > -- dean > Great suggestion. (I really wish Reitveld would be OK with 80 column-wide comments!) At least our revision history has the review links. Those are invaluable. -Darin --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
