[Sugar-devel] Code Review process changes

[Tomeu Vizoso]

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?

Thanks,

Tomeu

> 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
>