[Sugar-devel] Code Review process changes

Tomeu Vizoso 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
> kernel.
> Here's the relevant excerpt from the Linux kernel documentation on
> submitting patches [1] [2]:
> -----8<----------8<----------8<----------8<----------8<----------8<-----
>  - 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
>   detail.
>  - 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
>   correctly.
>  - Cc: the named person received a copy of the patch and had the
>   opportunity to comment on it.
> -----8<----------8<----------8<----------8<----------8<----------8<-----
> 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.
> [1] http://kernel.org/doc/Documentation/SubmittingPatches
> [2] 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
> http://lists.sugarlabs.org/listinfo/sugar-devel

More information about the Sugar-devel mailing list