Patch and Commit message guidelines -- first draft

[Koen Kooi]

Nit: OpenEmbedded is one word, not two :) Looks good otherwise from a quick read.

Op 4 mrt 2011, om 18:44 heeft Mark Hatle het volgende geschreven:

> In the last meeting I offered to pull together some guidelines for 'good' patch
> and commit messages.  Below is my attempt at doing this.  The guidelines are
> from many of Wind River's internal guidelines, as well as simply general
> knowledge picked up over time.
> 
> I'm sure I've missed a few things below, but I hope this is fairly close to what
> is intended.
> 
> ----
> 
> This set of guidelines is intended to help both the developer and reviewers of
> changes determine reasonable patch and change commit messages.  Often when
> working with the code, you forget that not everyone is as familiar with the
> problem and/or fix as you are.  Often the next person in the code doesn't
> understand what or why something is done so they quickly look at patch and
> commit messages.  Unless these messages are clear it will be difficult to
> understand the relevance of a given change and how future changes may impact
> previous decisions.
> 
> By following these guidelines we will have a better record of the problems and
> solutions made over the course of development.  It will also help establish a
> clear provenance of all of the code and changes within the development.
> 
> General Information
> -------------------
> 
> While specific to the Linux kernel development, the following could also be
> considered a general guide for any Open Source development:
> 
> http://ldn.linuxfoundation.org/book/how-participate-linux-community
> 
> May of the guidelines in this document are related to the items in that information.
> 
> Pay particular attention to section 5.3 that talks about patch preparation.
> They key thing to remember is to break up your changes into logical sections.
> Otherwise you run the risk of not being able to even explain the purpose of a
> change in the patch headers!
> 
> Patch and Commit Headers
> ------------------------
> There seems to always be a question or two surrounding what a person
> should put in a patch header, or commit message.
> 
> The general rules always apply, those being that there is a single line
> short log (think of this as the subject when the patch is e-mailed), and then
> the more detailed long log, and closure with tag lines like the
> "Signed-off-by:".
> 
> New development
> ---------------
> 
> A minimal patch or commit message would be of the format:
> 
> ---
> foobar: Adjusted the foo setting in bar
> 
> The foo setting in bar was adjusted to enable more foobar resources.  The new
> value was determined by determining the amount of free resources and determining
> the best value to use up all of system memory.
> 
> Signed-off-by: Joe Developer <joe.developer at example.com>
> ---
> 
> The minimal commit is good for new code development and simple changes.
> 
> The single short log message should begin with an indicator as to the primary
> item changed by this patch, followed by summary of the change.  In the above
> case we're indicating that we've changed the "foobar" item, by "adjusting the
> foo setting in bar".
> 
> Optionally, you may include pointers to defects this change corrects.  In the
> above, there was not bug number included.  The format of this reference within
> Open Embedded is not yet defined.  [MGH: Is this true?  Is there a better way to
> clarify this?]
> 
> You must then have a full description of the change.
> 
> Finally one or more sign-off-by lines should exist.  Each developer responsible
> for working on the patch is responsible for adding a Signed-off-by: line.
> 
> It is not acceptable to have an empty or non-existent header, or just a single
> line message.  The summary and description is required for all changes.
> 
> Importing from elsewhere
> -----------------------------
> If you are importing work from somewhere else, the minimum commit message is not
> enough.  It does not clearly establish the provenance of the code.
> 
> If you are pulling in a copy of code from another source, it is required that
> you full document the source as part of the commit message.  Such as:
> 
> ---
> foobar: Import foobar from Open Embedded
> 
> The foobar recipe was imported from the Open Embedded git server
> (git://git.openembedded.org/openembedded) as of commit id
> b65a0e0c84cf489bfa00d6aa6c48abc5a237100f.
> 
> Signed-off-by: Your Name <your.name at openembedded.org>
> ---
> 
> If the item being imported is a patch or commit from another source, you
> similarly have to identify the source as part of your commit message as well as
> preserve the upstream commit information.
> 
> By default you should keep the original authors summary and commit message,
> along with any sign-off information.  You are also required to sign-off on the
> change and indicate where this code came from and any changes that you made.
> 
> A message similar to the following would be created if you base your changes off
> of the commit above:
> 
> ---
> foobar: Adjusted the foo setting in bar
> 
> The foo setting in bar was adjusted to enable more foobar resources.  The new
> value was determined by determining the amount of free resources and determining
> the best value to use up all of system memory.
> 
> Signed-off-by: Joe Developer <joe.developer at example.com>
> 
> Code was imported from the foobar git (git://example.com/git/foo), commit id
> b65a0e0c84cf489bfa00d6aa6c48abc5a237100f.
> 
> The settings to bar were adjusted by 2% as necessary to correct the over use of
> the core memory.
> 
> Signed-off-by: Your Name <your.name at openembedded.org>
> ---
> 
> If the original patch or change that was imported does not have a summary and
> commit message from the original author, it is still your responsibility to add
> them to the patch.  Just as if you wrote the code should be able to clearly
> explain what the change does.  You are still required to clearly document where
> the patch originated from.
> 
> Common Errors in Patch and Commit messages
> ------------------------------------------
> 
> - Don't simply translate your C code change into English for a commit
> log. The log "Change compare from zero to one" is bad because it
> describes only the code change in the patch; we can see that from
> reading the patch itself. Let the code tell the story of the mechanics
> of the change (as much as possible), and let your comment tell the
> other details -- i.e. what the problem was, how it manifested itself
> (symptoms), and if necessary, the justification for why the fix was
> done in manner that it was.
> 
> - Don't start your commit log with "This patch..." -- we already know
> it is a patch.
> 
> - Don't repeat your short log in the long log. If you really really
> don't have anything new and informational to add in as a long log,
> then don't put a long log at all. This should be uncommon -- i.e.
> the only acceptable cases for no long log would be something like
> "Fix spelling mistakes in Documentation/README" or similar.
> 
> - Don't put absolute paths to source files that are specific to your site,
> i.e. if you get a compile error on "fs/exec.c" then tidy up the parts
> of the compile output to just show that portion. We don't need to see
> that it was /home/wally/src/git/linux/devel/2009/Nov/linux-2.6/fs/exec.c
> or similar - that full path has no value to others.
> 
> - Always use the most significant ramification of the change in the
> words of your subject/shortlog. For example, don't say "fix compile
> warning in foo" when the compiler warning was really telling us that
> we were dereferencing complete garbage off in the weeds that could
> in theory cause an OOPS under some circumstances. When people are
> choosing commits for backports to stable or distro kernels, the
> shortlog will be what they use for an initial sorting selection.
> If they see "Fix possible OOPS in...." then these people will look
> closer, whereas they most likely will skip over simple warning fixes.
> 
> - Don't put in the full 20 or more lines of a backtrace when really it is
> just the last 5 or so function calls that are relevant to the crash/fix.
> If the entry point, or start of the trace is relevant (i.e. a syscall
> or similar), you can leave that, and then replace a chunk of the
> intermediate calls in the middle with a single [...]
> 
> - Don't include timestamps or other unnecessary information, unless they are
> relevant to the situation (i.e. indicating an unacceptable delay in
> a driver initialization etc.)
> 
> _______________________________________________
> tsc mailing list
> tsc at lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/tsc