[oe] Commit and Patch message guidelines - third draft.

Thu Mar 31 20:29:12 UTC 2011

Dear Mark,

I forgot to add one thing.

Am Donnerstag, den 31.03.2011, 21:06 +0200 schrieb Paul Menzel:

> Am Donnerstag, den 31.03.2011, 12:57 -0500 schrieb Mark Hatle:
> > This is the third draft of the guidelines... All previous feedback has been
> > incorporated...
> > 
> > It is the first of the drafts being sent to the OpenEmbedded-devel mailing list
> > for comments.  The intention of the TSC is to use the following guidelines to
> > help increase the quality of the commit and patches within Open Embedded.
> 
> No space in OpenEmbedded. ;-)
> 
> > Please review the following guidelines and let me know if you have any
> > questions, comments or concerns with them.
> > 
> > ----
> > 
> > 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.
> > 
> > Both patches and commit messages require the same attention and basic details,
> > however the purposes of the messages are slightly different. A patch needs to
> > describe a specific change that fixes an individual problem in a component,
> 
> Excuse my ignorance and please keep in mind that English is not my
> native language. I do not get that sentence. How can a patch describe
> something.
> 
> Reading further down, I now get in what context you use patch here.
> Maybe you should make clear in the beginning that you mean patches which
> gets applied by the recipe and not patches to be submitted directly to
> OpenEmbedded. (That is still ambiguous.)
> 
> > while a commit message describes one or more changes to the overall project or
> > system.
> > 
> > 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
> > 
> > Many 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.
> 
> s/They/The/
> 
> > 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:" or "Integrated-by:"
> > 
> > See the examples below for examples and details.
> > 
> > New development
> > ---------------
> > 
> > A minimal patch or commit message would be of the format:
> > 
> >    Short log / Statement of what needed to be changed.
> 
> 1. I think that line is also named »commit summary« in Git terms.
> 2. Do we suggest a maximum line length for the short log?
> 
> >    (Optional pointers to external resources, such as defect tracking)
> > 
> >    The intent of your change.
> > 
> >    (Optional, if it's not clear from above) how your change resolves the
> >    issues in the first part.
> > 
> >    Tag line(s) at the end.
> > 
> > For example:
> > 
> >    foobar: Adjusted the foo setting in bar
> > 
> >    When using foobar on systems with less than a gigabyte of RAM common
> >    usage patterns often result in an Out-of-memory condition causing
> >    slowdowns and unexpected application termination.
> > 
> >    Low-memory systems should continue to function without running into
> >    memory-starvation conditions with minimal cost to systems with more
> >    available memory.  High-memory systems will be less able to use the
> >    full extent of the system, a dynamically tunable option may be best,
> >    long-term.
> > 
> >    The foo setting in bar was decreased from X to X-50% in order to
> >    ensure we don't exhaust all system memory with foobar threads.
> > 
> >    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 indicates what needed to be changed. It should
> > begin with an indicator as to the primary item changed by this patch,
> > followed by a short 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. Unless
> > the defect format is specified by the component you are modifying, it is
> > suggested that you use a full URL to specify the reference to the defect
> > information.
> 
> Example.
> 
> > You must then have a full description of the change. Specifying the intent of
> > your change and if necessary how the change resolves the issue.
> > 
> > As mentioned above this is intended to answer the "what were you thinking"
> > questions down the road and to know what other impacts are likely to
> > result of the change needs to be reverted. It also allows for better
> > solutions if someone comes along later and agrees with paragraph 1
> > (the problem statement) and either disagrees with paragraph 2
> > (the design) or paragraph 3 (the implementation).
> > 
> > Finally one or more tag lines should exist. Each developer responsible
> > for working on the patch is responsible for adding a Signed-off-by: tag 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. The following
> > specified the guidelines required for importing code from elsewhere.
> > 
> > By default you should keep the original authors summary and commit message,
> > along with any Signed-off-by: information. If the original change that was
> > imported does not have a summary and/or commit message from the original author,
> > it is still your responsibility to add them to the patch. Just as if you wrote
> > the code, you should be able to clearly explain what the change does. It is also
> > necessary to document the original author of the change. You should indicate the
> > original author by simply stating "written by" or "posted to the ... mailing
> > list by". Only the original author can commit using a Signed-off-by: tag line.
> > 
> > It is also required that the origin of the work be fully documented. The origin
> > should be specified as part of the commit message in a way that an average
> > developer would be able to track down the original code. URLs should reference
> > original authoritative sources and not mirrors.
> > 
> > If changes were required to the patch, they should be documented as well.
> 
> Is a tag line also needed then? What kind of tag line?
> 
> > Finally a tag line should be added to indicate that you worked with the patch.
> > The "Integrated-by:" tag line should be used to indicate that you did not need
> > to modify the patch, it was integrated unchanged. The "Signed-off-by:" tag line
> > should only be used when changes to the original patch were necessary.
> 
> Are the tag lines also needed in the patch?
> 
> > For example, if you were to pull in the patch from the above example unmodified:
> > 
> >    foobar: Adjusted the foo setting in bar
> > 
> >    When using foobar on systems with less than a gigabyte of RAM common
> >    usage patterns often result in an Out-of-memory condition causing
> >    slowdowns and unexpected application termination.
> > 
> >    Low-memory systems should continue to function without running into
> >    memory-starvation conditions with minimal cost to systems with more
> >    available memory.  High-memory systems will be less able to use the
> >    full extent of the system, a dynamically tunable option may be best,
> >    long-term.
> > 
> >    The foo setting in bar was decreased from X to X-50% in order to
> >    ensure we don't exhaust all system memory with foobar threads.
> > 
> >    Signed-off-by: Joe Developer <joe.developer at example.com>
> > 
> >    The foobar recipe was imported from the OpenEmbedded git server
> >    (git://git.openembedded.org/openembedded) as of commit id
> >    b65a0e0c84cf489bfa00d6aa6c48abc5a237100f.
> > 
> >    Integrated-by: Your Name <your.name at openembedded.org>
> > 
> > The above example shows a clear way for a developer to find the original source
> > of the change. It indicates that the open embedded
> 
> s/open embedded/OpenEmbedded/
> 
> > developer simply integrated
> > the change into the system without making any further modifications.
> > 
> > However in the same example, if the author did have to make some changes to the
> > original patch it would look like this:
> > 
> >    foobar: Adjusted the foo setting in bar
> > 
> >    When using foobar on systems with less than a gigabyte of RAM common
> >    usage patterns often result in an Out-of-memory condition causing
> >    slowdowns and unexpected application termination.
> > 
> >    Low-memory systems should continue to function without running into
> >    memory-starvation conditions with minimal cost to systems with more
> >    available memory.  High-memory systems will be less able to use the
> >    full extent of the system, a dynamically tunable option may be best,
> >    long-term.
> > 
> >    The foo setting in bar was decreased from X to X-50% in order to
> >    ensure we don't exhaust all system memory with foobar threads.
> > 
> >    Signed-off-by: Joe Developer <joe.developer at example.com>
> > 
> >    The foobar recipe was imported from the OpenEmbedded git server
> >    (git://git.openembedded.org/openembedded) as of commit id
> >    b65a0e0c84cf489bfa00d6aa6c48abc5a237100f.
> > 
> >    It was necessary to change the memory threshold from 50% to 42%
> >    due to the constraints of the implementation of bar in OpenEmbedded.
> > 
> >    Signed-off-by: Your Name <your.name at openembedded.org>
> > 
> > 
> > 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
> 
> Mention `git bisect`?
> 
> > and also people can then cherrypick
> 
> s/cherrypick/cherry pick/?
> 
> > 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.
> 
> A common case is also when updating/adding program versions.
> 
> 	foo: add version 10.μ.β.π
> 
> > - 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.
> 
> How much should it be stripped?
> 
> > - 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 [...]
> 
> I have to remember that. ;-)
> 
> > - 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.
> > 
> > - Don't reference mirrors, but instead always reference original authoritative
> > sources.
> > 
> > - Avoid punctuation in the short log.
> 
> I would then also propose to *not* start with a capital letter.
> 
> In OpenEmbedded it is common to write the recipe name (and if necessary
> the version) in front of the commit summary. I kind of liked that rule.

Additionally for quality assurance I would ask to add a line what
distribution and `MACHINE` the patch was build tested (or even run
tested) with. Adding the output »Build configuration« of BitBake should
be sufficient.

> Thank you for the write up/proposal. All in all it is quite long. Maybe
> a summary/short version/example should be put right at the beginning.

Thanks,

Paul
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.openembedded.org/pipermail/openembedded-devel/attachments/20110331/9faebd62/attachment-0002.sig>