[openstack-dev] Code review study
dolph.mathews at gmail.com
Thu Aug 15 15:04:11 UTC 2013
On Thu, Aug 15, 2013 at 9:56 AM, Daniel P. Berrange <berrange at redhat.com>wrote:
> On Thu, Aug 15, 2013 at 09:46:07AM -0500, Dolph Mathews wrote:
> > On Thu, Aug 15, 2013 at 7:57 AM, Christopher Yeoh <cbkyeoh at gmail.com>
> > > On Thu, Aug 15, 2013 at 9:54 PM, Daniel P. Berrange <
> berrange at redhat.com>wrote:Commit message quality has improved somewhat
> since I first wrote &
> > > published
> > >
> > > that page, but there's definitely still scope to improve things
> > >> What
> > >> it really needs is for more reviewers to push back against badly
> > >> commit messages, to nudge authors into the habit of being more
> verbose in
> > >> their commits.
> > >>
> > >>
> > > Agreed. There is often "what" and sometimes "why", but not very often
> > > "how" in commit messages.
> > >
> > ++
> > Beyond the one line summary (which *should* describe "what" changed),
> > describing "what" changed in the commit message is entirely redundant
> > the commit itself.
> It isn't that clearcut actually. It is quite often helpful to summarize
> "what" changed in the commit message, particularly for changes touching
> large areas of code, or many files.
Acknowledged, but keyword: *summarize*! If it really won't fit in a one
line summary, that's a giant red flag that you should be producing multiple
> The diff's can't always be assumed
> to be easily readable - for example if you re-indented a large area of
> code, the actual "what" can be clear as mud.
> Or if there are related
> changes spread across many files & functions, a description of what is
> being done will aid reviewers.
This doesn't necessarily belong in the commit message... this can fall
under item #4 in the article -- annotate your code review before it begins.
> Just be pragmatic about deciding when
> a change is complex enough that it merits summarizing the 'what', as
> well as the 'why'.
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/:|
> |: http://libvirt.org -o- http://virt-manager.org:|
> |: http://autobuild.org -o- http://search.cpan.org/~danberr/:|
> |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc:|
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the OpenStack-dev