[Sugar-devel] Code Review process changes
tomeu at tomeuvizoso.net
Fri Apr 23 06:43:33 EDT 2010
On Fri, Apr 23, 2010 at 03:46, Bernie Innocenti <bernie at codewiz.org> wrote:
> On Tue, 2010-04-20 at 18:26 +0200, Sascha Silbe wrote:
>> We'd like to try a different approach that's used by many successful
>> projects - both small and large ones.
>> Patches are sent to sugar-devel for review. Every Sugar developer (*)
>> can review patches (and multiple reviews are quite welcome). Since
>> the number of developers with commit access is limited, we have a
>> sufficient level of QA even without limiting who "can" do reviews.
Cannot find the rest of Sascha's email, was it sent to a list?
The bits from Sascha that Bernie quoted refers to some context I
cannot find, can someone repost? Or better, can we do what we agreed
on #sugar and have a coherent proposal sent to the list?
About Bernie's account on the kernel review practices, is it something
that is being proposed for Sugar or is just food for thought?
> I noticed that we're already getting into this habit spontaneously, but
> it might be good to mention how the various tags are used in the Linux
> Here's the relevant excerpt from the Linux kernel documentation on
> submitting patches  :
> - Signed-off-by: this is a developer's certification that he or she has
> the right to submit the patch for inclusion into the kernel. It is an
> agreement to the Developer's Certificate of Origin, the full text of
> which can be found in Documentation/SubmittingPatches. Code without a
> proper signoff cannot be merged into the mainline.
> - Acked-by: indicates an agreement by another developer (often a
> maintainer of the relevant code) that the patch is appropriate for
> inclusion into the kernel.
> - Tested-by: states that the named person has tested the patch and found
> it to work.
> - Reviewed-by: the named developer has reviewed the patch for correctness;
> see the reviewer's statement in Documentation/SubmittingPatches for more
> - Reported-by: names a user who reported a problem which is fixed by this
> patch; this tag is used to give credit to the (often underappreciated)
> people who test our code and let us know when things do not work
> - Cc: the named person received a copy of the patch and had the
> opportunity to comment on it.
> Git handles some of these automatically. When you save a patch, you
> could simply do:
> # Export a patch with your "Signed-off-by"
> git format-patch -1 -s
> # Send patch to a maintainer, and cc the mailing list
> git send-email --to someone at sugarlabs.org --cc sugar-devel at lists.sugarlabs.org
> Committers should record in the patch who reviewed and ack'd a patch.
> This can be easily done by editing the comment in interactive mode:
> git am -i foo.patch
> (then use e to edit the patch)
> Alternatively, one could also edit the comment after the fact with "git
> commit --amend".
> Attributing due credit to reviewers and testers is important because
> they're a scarce resource. A healthy development process depends on them
> as much as developers.
>> Personal note:
>> Instead of using a patch tracker, we could also ask patch submitters to
>> file tickets at bugs.sugarlabs.org if there has been no review for, say,
>> 3 days. This gives a streamlined process for most patches while still
>> ensuring nothing gets lost.
> Another possibility is pinging on the list after a few days, perhaps
> adding on Cc people who are known to have worked in the area affected by
> the patch.
> for informational purposes, I continued to attach some patches to
> existing tickets. The difference is that it's no longer a required step
> for getting a patch in the mainline tree.
>  http://kernel.org/doc/Documentation/SubmittingPatches
>  http://kernel.org/doc/Documentation/development-process/5.Posting
> // Bernie Innocenti - http://codewiz.org/
> \X/ Sugar Labs - http://sugarlabs.org/
> Sugar-devel mailing list
> Sugar-devel at lists.sugarlabs.org
More information about the Sugar-devel