Re: Fix quilt merge error in acpi-cpufreq.c

[Rusty Russell <rusty@rustcorp.com.au>]

On Thu, 16 Apr 2009 11:25:41 pm Jonathan Corbet wrote:
> On Thu, 16 Apr 2009 11:30:26 +0930
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
> > Anyone want to try to write a guide on writing good commit messages?
> 
> I've tried to do that about halfway through
> Documentation/development-process/5.Posting.  No doubt it can be
> improved...patches (with good commit messages) welcome :)

I think Ted put his finger on it: the definition of a good commit
message is one which is kind to the reader.

There are several readers: someone reviewing the patch, someone looking
for a specific bug, someone backporting, someone dealing with an API change,
someone bisecting.  A reviewer needs to know why the patch is done the way it
is, the bughunter needs to be able to find the commit from the bug symptoms,
the backporter needs to know what the patch fixes, how severe it is, and what
side effects are expected, the what-happened reader needs to know how to port
to the new API (and probably why it changed), and the bisector doesn't need
much at all (since the patch is buggy, the commentry is probably wrong anyway,
but perhaps they need to judge what reverting the patch will do).

Here are my thoughts:

- Actual demonstrated fixes for demonstrated bugs should aim for subject
  "<subsystem>: fix <symptom> <condition>...", eg: "modules: fix crash when out
  of memory" or "modules: fix compile error on arm" or "modules: fix module
  load when CONFIG_FOO=y, CONFIG_BAR=n, moon=full".

  Body should include:
  - how likely (if not obvious from subject).
  - how important.  Root can crash box maybe isn't important, but if
    NetworkManager default configuration tickles the bug, it might be.
  - text of message which results if any.  Oops (trimmed), compile error,
    unexpected output of utility.
  - when the bug was introduced (or exposed).
  - upstream (bugzilla, ml message, or at least reporter) so it can be dug
    further if required.

  I suggest that the keyword "fix" not be used for theoretical or soon-to-be-
  exposed limitations of code.  eg. "virtio: correction to BAD_RING
  macro if arg is not called 'vq'" not "virtio: fix BAD_RING macro when..."
  (this was a real example, but the arg was always called 'vq').

- Neatening-and-documenting commits should aim for "<subsystem>: cleanup...",
  eg. "lguest: cleanup typos in headers" or "lguest: cleanup: rename internal
  function", or "module: cleanup: move function out of header".

  Body should include:
  - larger plan if there is one (eg. removing obsolete function, reordering
    functions because we're going to do something later, exposing functions
    because we're going to need them in successive patch).

- No subject should ever contain the word "trivial".  If it's really trivial,
  you can sum it up in the subject and we'll know it's trivial.  Plus the
  diffstat shows it.  'trivial' is propaganda to sneak a patch into -rc7.

- API changes or deprecation should say why the change is happening, and
  how to port.  The subject line should say what's changed in preference
  to why.  eg. "per_cpu: deprecate per_cpu_ptr in favor of percpu_ptr", or
  "list.h: add list_for_each_reverse_rcu_backflip".

- The change message should always be about the change, not the new code.
  For example, the above commit might say "we need this for the new backflip.c
  debugging code", but the documentation of the function belongs in the code
  itself, not the git log!

Writing good commit messages takes time, practice and feedback.  But
if you're having trouble writing a good commit message, it's often a sign
that your patch needs to be reworked anyway.

Sorry for the too-long post, but this stuff actually matters...
Rusty.