Talk:Development Team/Code Review

Experimental workflow for doing reviews on mailing-list

On the submitter end:

 git commit -s
 git format-patch -1
 git send-email --to maintainer --cc list foo.patch

Anyone who sees the patch can reply with inline comments. Multiple reviews are welcome.

The reviewer can conclude the message with one of these tags:

 Acked-by: Joe Hacker <jhacker@sugarlabs.org>
 Reviewed-by: Joe Hacker <jhacker@sugarlabs.org>
 Tested-by: Joe Hacker <jhacker@sugarlabs.org>

Only module maintainers and peers can ack a patch. In case someone has doubts about the meanings of the above tags, here's the detailed description.

If submitter has commit access to the repository, he/she amends the patch to incorporate

suggestions and to append any collected tags to the changelog:

 git commit --amend
 git push
 (submitters with multiple patches in their queue may need to rebase or switch to a clean branch)

In case the patch closes a bug in Trac, submitter closes the bug mentioning the commit ID as usual

If a module maintainer does not wish to approve patches submitted by email, simply state it, so we don't needlessly disappoint contributors. If a submitter still prefers the old workflow, they can keep filing patches in the bug tracker as before.

Feel free to improve the above text directly in the talk page. Add a watch to this page to note changes made by others.

Revised patches

add a version number to the subject prefix (e.g. git send-email --subject-prefix="PATCH v2 sugar")

Sounds inpractical and needles complicated, from one point all email clients support thread, from another point, with mail basis patch tracking (using only email client) there is no way to have thread status. If patch tracking systems can't detect current status w/o setting subjects it needs to be fixed, otherwise it becomes annoying from patch submitter pov.

v1->v2: also might be questionable, having it in git log is useless (since the full story is in email thread). At the same time, if there is a need out-of-git-log comments, it might happen in --compose mode.