[Sugar-devel] Code Review process changes

[Bernie Innocenti]

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.

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/