Patch and Commit message guidelines -- second draft

[Stefan Schmidt]

Hello.

On Mon, 2011-03-07 at 13:19, Mark Hatle wrote:
> This is the second draft of the guidelines...  All previous feedback has been
> incorporated...

Thanks.

> I did not include the patch-upstream-status: item however.  If people agree this
> is necessary in here give me some suggestions on how to document it.

I'm ok with leaving this out for the first version. It was more of an idea and I
would like to keep it in mind as it seems useful to me.

> A few items were incorporated from:
> http://openembedded.org/index.php/Commit_Policy and
> http://openembedded.org/index.php/Commit_log_example
> 
> Question, if we agree to this guideline, how do we want to merge the policies?
> Simply by extending the Commit Policy and Commit Log Example?

That opens up another can of worms. ;) Right now we have informations in the
wiki, the OE manual, the Poky handbook and maybe in the source tree itself. To
me it sounds like another agenda item we need to add to find out where
informations should go best to help people coming up to speed.

> Note: there is still a question in the middle, search for "[MGH:".

We used different formats here. Like "fixes bug #1234", or "for #1234". It was
always a bit of a battle around the bugzilla usage and therefor not many
references could be found and no standard format for it.

> 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 or summary of the change (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:".
> 
> See the examples below for examples and details.
> 
> 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
> OpenEmbedded is not yet defined.  [MGH: Is this true?  Is there a better way to
> clarify this?]

See above.

> 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 OpenEmbedded
> 
> The foobar recipe was imported from the OpenEmbedded 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 have one huge patch, split your change into logical subparts. It's
> easier to track down problems afterwards with a binary search and also people
> can then cherrypick changes into things like stable branches.
> 
> - Don't simply translate your 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/oe-core/meta/classes/insane.bbclass
> or similar - that full path has no value to others.

Heh, this is mixed up now. A compile error on fs/exec.c that came from
insane.bbclass. ;)

How about:

"i.e. if you get a compile error on "meta/recipes-core/busybox/busybox_1.17.3.bb"
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/oe-core/meta/recipes-core/busybox/busybox_1.17.3.bb
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.)
> 
> - Don't use links to temporary resources like pastebin and friends.  The commit
> message may be read long after this resource timed out.

Looks good to me otherwise.

regards
Stefan Schmidt