[poky] Commit and Patch message guidelines - fifth draft

[Darren Hart]

Hi Mark,

Thanks for writing this up. I think this is on the right track. A few
comments inline...

As a side note, I'm currently working with various people on improving
our review/pull process which dovetails with this a bit, but probably
doesn't need to be explicitly called out here.



On 05/12/2011 12:57 PM, Mark Hatle wrote:
> This is the fifth draft of the guidelines... All previous feedback has been
> incorporated...
> 
> (The fourth draft was reviewed and found lacking in a few key areas, so I
> skipped the public posting.)
> 
> The new concept of Upstream-Status was added by the fourth draft as
> something strongly suggested, but optional.  This is likely an area that may
> still need some revision before we call this final.
> 
> ----
> 
> Introduction
> ------------
> 
> This set of guidelines is intended to help both the developer and reviewers of
> changes determine reasonable patch headers 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 header
> 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.
> 
> This policy refers both to patches that are being applied by recipes as well as
> commit messages that are part of the source control system, usually git. A patch
> file needs a header in order to describe the specific change that that are made
> within that patch, while a commit message describes one or more changes to the
> overall project or system. Both the patch headers and commit messages require
> the same attention and basic details, however the purposes of the messages are
> slightly different. A commit message documents all of the changes made as part
> of a commit, while a patch header documents items specific to a single patch. In
> many cases the patch header can also be used as the commit message.
> 
> This policy does not cover the testing of the changes, or the technical criteria
> for accepting a patch.
> 
> 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.
> The 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!
> 
> In addition section 5.4 explains the purpose of the Signed-off-by: tag line as
> discussed in later parts of this document. Due to the importance of the
> Signed-off-by: tag line a copy of the description follows:
> 
>     Signed-off-by: this is a developer's certification that he or she has
>     the right to submit the patch for inclusion into the [project].  It is
>     an agreement to the Developer's Certificate of Origin, the full text of
>     which can be found in [Linux Kernel] Documentation/SubmittingPatches.
>     Code without a proper signoff cannot be merged into the mainline.
> 
> For more information on the Developer's Certificate of Origin and the Linux
> Kernel Documentation/SubmittingPatches, see:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches
> 
> Patch Headers and Commit Messages
> ---------------------------------
> 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
> such as "Signed-off-by:".


A complete list of acceptable tag lines should be documented. Consider:

Signed-off-by:
Acked-by:    \__ The difference between these two is subtle. Acked-by
Reviewed-by: /   implies a certain degree of authority over the affected
Tested-by:       code.
Reported-by:


> 
> 
> New development
> ---------------
> 
> A minimal patch or commit message would be of the format:
> 
>    Short log / Statement of what needed to be changed.
> 
>    (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 message 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 work,
> 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".
> 
> The single short log message is analogous to the git "commit summary". While no
> maximum line length is specified by this policy, it is suggested that it remains
> under 78 characters wherever possible.
> 
> 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. Generally these pointers will precede any long description, but as
> an optional item it may be after the long description.
> 
> For example, you might refer to an open source defect in the RPM project:
> 
>    rpm: Adjusted the foo setting in bar
> 
>    [RPM Ticket #65] -- http://rpm5.org/cvs/tktview?tn=65,5
>


Another URL example would be mailing list reference. H. Peter Anvin
recently proposed a new automated system for dealing with this for LKML.
We might want to do something similar.


>    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>
> 
> 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, such as a cherry-pick from
> another repository or a code patch pulled from a mailing list, the minimum patch
> header or commit message is not enough. It does not clearly establish the
> provenance of the code.
> 
> The following specifies the additional guidelines required for importing changes
> from elsewhere.
> 
> By default you should keep the original authors summary and description,
> along with any tag lines such as, "Signed-off-by:". 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 header. 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.


Is this right? It was my understanding that the Signed-off-by come from
each person that handles the patch between creation and commit. For the
Linux kernel, which you reference above, many (most?) patches have
multiple SOB lines.


> 
> 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 resolve conflicts, they should be documented as
> well.  When incorporating a commit or patch from an external source, changes to
> the functionality not related to resolving conflicts should be made in a
> second commit or patch.  This preserves the original external commit, and makes
> the modifications clearly visible, and prevents confusion over the author of the
> code.
> 
> Finally a "Signed-off-by:" tag line should be added to indicate that you worked
> with the patch.


OK, this seems at odds with the statement 3 paragraphs up about only the
original author adding an SOB line. A clarification is needed.


> 
> Example: Importing form elsewhere unmodified
> --------------------------------------------
> 
> 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 patch 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>
> 
> The above example shows a clear way for a developer to find the original source
> of the change. It indicates that the OpenEmbedded developer simply integrated
> the change into the system without making any further modifications.
> 
> Example: Importing from Elsewhere modified
> ------------------------------------------
> 
> When importing a patch, occasionally changes have to be made in order to resolve
> conflicts.  The following is an example of the commit message when the item needed
> to be modified:
> 
>    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 patch was imported from the OpenEmbedded git server
>    (git://git.openembedded.org/openembedded) as of commit id
>    b65a0e0c84cf489bfa00d6aa6c48abc5a237100f.
> 
>    A previous change had modified the memory threshold calculation,
>    causing a conflict.  The conflict was resolved by preserving the
>    memory threshold calculation from the existing implementation.
> 
>    Signed-off-by: Your Name <your.name at openembedded.org>
> 
> 
> Patch Header Recommendations
> ----------------------------
> In order to keep track of patches and ultimately reduce the number of patches
> that are required to be maintained, we need to track the status of the patches
> that are created. The following items are specific to patches applied by recipes.
> 
> In addition to the items mentioned above, we also want to track if it's
> appropriate to get this particular patch into the upstream project. Since we
> want to track this information, we need a consistent tag and set of status that
> can be searched.
> 
> While adding this tracking information to the patch headers is currently optional,
> it is highly recommended and some maintainers may require it.  It is optional at
> this time so that it can be evaluated as to it's usefulness over time.  Existing
> patches will be modified with the tag as they are modified.
> 
> As mentioned in the above, be sure to include any URL to bug tracking, mailing
> lists or other reference material pertaining to the patch. Then add a new tag
> "Upstream-Status:" which contains one of the following items:
> 
>    Pending
>    - No determination has been made yet or not yet submitted to upstream
> 
>    Submitted [where]
>    - Submitted to upstream, waiting approval
>    - Optionally include where it was submitted, such as the author, mailing
>      list, etc.
> 
>    Accepted
>    - Accepted in upstream, expect it to be removed at next update, include
>      expected version info
> 
>    Backport
>    - Backported from new upstream version, because we are at a fixed version,
>      include upstream version info
> 
>    Denied
>    - Not accepted by upstream, include reason in patch
> 
>    Inappropriate [reason]
>    - The patch is not appropriate for upstream, include a brief reason on the
>      same line enclosed with []
>      reason can be:
>        not author (You are not the author and do not intend to upstream this,
>                    source must be listed in the comments)
>        native
>        licensing
>        configuration
>        enable feature
>        disable feature
>        bugfix (add bug URL here)
>        embedded specific
>        no upstream (the upstream is no longer available -- dead project)
>        other (give details in comments)
> 
> The various "Inappropriate [reason]" status items are meant to indicate that the
> person reasonable for adding this patch to the system does not intend to upstream
> the patch for a specific reason.  Unless otherwise noted, another person could
> submit this patch to the upstream, if they do the status should be changed to
> "Submitted [where]", and an additional signed-off-by: line added to the patch by


s/signed-off-by:/Signed-off-by:/


> the person claiming responsibility for upstreaming.
> 
> For example, if the patch has been submitted upstream:
> 
>    rpm: Adjusted the foo setting in bar
> 
>    [RPM Ticket #65] -- http://rpm5.org/cvs/tktview?tn=65,5
> 
>    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.
> 
>    Upstream-Status: Submitted [rpm5-devel at rpm5.org]
> 
>    Signed-off-by: Joe Developer <joe.developer at example.com>
> 
> A future commit can change the value to "Accepted" or "Denied" as appropriate.
> 
> Common Errors in Patch and Commit messages
> ------------------------------------------


s/messages/Messages/


> 
> - Short log does not start with the file or component being modified.  Such as
> "foo: Update to new upstream version 5.8.9"
> 
> - Avoid starting the summary line with a capital letter, unless the component
> being referred to also begins with a capital letter.
> 
> - Don't have one huge patch, split your change into logical subparts. It's
> easier to track down problems afterward using tools such as git bisect. It also
> makes it easy for people to 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 "Documentation/README: Fix spelling mistakes".
> 
> - 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. Always reduce the path to something more
> meaningful, such as oe-core/meta/classes/insane.bbclass.
> 
> - 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.
> 
> - Don't reference mirrors, but instead always reference original authoritative
> sources.
> 
> - Avoid punctuation in the short log.


Thanks,

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel