CodeReview: Difference between revisions
(initial page) |
m (→~/.gitconfig) |
||
(14 intermediate revisions by 6 users not shown) | |||
Line 1: | Line 1: | ||
= Code Review in Samba - Mandatory = |
|||
The Samba project requires code reviews, Authors must get their patches reviewed before pushing them to master via the autobuild system. |
|||
== policy == |
|||
The policy we have established for mandatory code code review is this: |
|||
* Each commit in master should have been reviewed by at least two samba team members. For this purpose, involvement in patch creation is considered as reviewing. |
|||
==commit message tags== |
|||
There is currently no prescribed way or tool to submit patches for review or mark patches as reviewed. The decisive point is that the patches that end up in master carry notice that they have been reviewed. We achieve this by adding certain tags to the commit messages using the command |
|||
git commit --amend |
|||
These are the tags we use: |
|||
;Signed-off-by: |
|||
: The Author(s) of a patch should add their signature to the commit message by adding a "Signed-off-by: " tag. |
|||
: This tag indicates that the the person it lists was involved in the creation of the patch, that it is fine with the state of the patch and submits it under the license(s) of the affected files. |
|||
: Author(s) submitting a patch on behalf of an employer should also follow the signing process as described in the Samba Copyright Policy (https://www.samba.org/samba/devel/copyright-policy.html). |
|||
: |
|||
: git offers the "-s" or "--signoff" switch for the sub-commands commit, cherry-pick and am (apply-mailbox) which allows you to add the Signed-off-by tag without having to type it. |
|||
;Reviewed-by: |
|||
: A reviewer should add a "Reviewed-by: " tag if she was not involved in the creation of the patch. |
|||
: There is currently no git switch smilar to "--signoff", and according to git developers, there won't be. So we have to add the review tags manually. |
|||
These tags are inspired by the use in the Linux kernel, but note that they don't always have the same meaning and/or significance in Samba that they have for Linux -- particularly with regard to the DCO. See [http://www.kernel.org/doc/Documentation/SubmittingPatches the Linux website about submitting patches] for detailed explanations on how they use these tags. |
|||
The Linux also makes use of an "Acked-by: " tag, but I suggest that we don't use it for now to keep things simple, since it does not make a lot of sense, unless we have established maintainers all over the code base and those need to Ack patches for their areas. |
|||
If the patch was created by multiple authors, they can optionally add a "Pair-programmed-with: " tag to the commit messages. This is redundant though. |
|||
== Tools and Scripts == |
|||
A number of developers use various tools, scripts, hacks, and shortcuts to ease the mechanics of the review process. |
|||
=== ~/.vimrc === |
|||
Here is a sample of a configuration snippet for use with vim: |
|||
<pre> |
|||
function! CommitMessages() |
|||
let g:git_ci_msg_user = substitute(system("git config --get user.name"), '\n$', '', '') |
|||
let g:git_ci_msg_email = substitute(system("git config --get user.email"), '\n$', '', '') |
|||
nmap S oSigned-off-by: <C-R>=printf("%s <%s>", g:git_ci_msg_user, g:git_ci_msg_email)<CR><CR><ESC> |
|||
nmap R oReviewed-by: <C-R>=printf("%s <%s>", g:git_ci_msg_user, g:git_ci_msg_email)<CR><ESC> |
|||
iab #S Signed-off-by: <C-R>=printf("%s <%s>", g:git_ci_msg_user, g:git_ci_msg_email)<CR> |
|||
iab #R Reviewed-by: <C-R>=printf("%s <%s>", g:git_ci_msg_user, g:git_ci_msg_email)<CR> |
|||
iab #O Signed-off-by: |
|||
iab #V Reviewed-by: |
|||
iab #P Pair-Programmed-With: |
|||
iab ME <C-R>=printf("%s <%s>", g:git_ci_msg_user, g:git_ci_msg_email)<CR> |
|||
iab ASN Andreas<SPACE>Schneider<SPACE><asn@samba.org> |
|||
iab AB Alexander<SPACE>Bokovoy<SPACE><ab@samba.org> |
|||
iab OBNOX Michael<SPACE>Adam<SPACE><obnox@samba.org> |
|||
iab VL Volker<SPACE>Lendecke<SPACE><vl@samba.org> |
|||
iab METZE Stefan<SPACE>Metzmacher<SPACE><metze@samba.org> |
|||
iab GD Guenther<SPACE>Deschner<SPACE><gd@samba.org> |
|||
iab JRA Jeremy<SPACE>Allison<SPACE><jra@samba.org> |
|||
iab JHROZEK Jakub<SPACE>Hrozek<SPACE><jhrozek@redhat.com> |
|||
iab ARIS Aris<SPACE>Adamantiadis<SPACE><aris@0xbadc0de.be> |
|||
endf |
|||
autocmd BufWinEnter COMMIT_EDITMSG,*.diff,*.patch,*.patches.txt call CommitMessages() |
|||
</pre> |
|||
=== ~/.gitconfig === |
|||
Here are a few git aliases for code review: |
|||
<pre> |
|||
[alias] |
|||
author = !"SEARCH=${@:-`git config user.email`}; set --; git log --format='%aN <%aE>' | uniq | grep -m 1 \"$SEARCH\"" |
|||
authors = !"SEARCH=${@:-`git config user.email`}; set --; git log --format='%aN <%aE>' | grep \"$SEARCH\" | uniq | awk '!a[$0]++'" |
|||
reviewed = !"BRANCH=${1:-master}; shift; AUTHORS=\"~/git-reviewed.sh\"; for A in \"${@:-`git config user.email`}\"; do AUTHORS=$AUTHORS\" \\\"`git author $A`\\\"\"; done; set --; git rebase -i -x \"$AUTHORS\" $BRANCH" |
|||
</pre> |
|||
;git author [<search>]: |
|||
: Search for the latest author tag (a string in the format of "Full Name <email@example.com>") matching a given search term. If no <search> is provided, return the latest author tag that uses the current value of "git config user.email". |
|||
;git authors [<search>]: |
|||
: Similar to "git author", except returns all author tags that match the search term. Useful in cases where a particular author has multiple e-mail addresses or multiple spellings to their name. |
|||
;git reviewed [<branch> [<author> ...]] |
|||
: Run a "git rebase -i <branch>" command that is configured to insert Reviewed-by tags for every <author> listed. <author> must be a search term as used in "git author" above. If no <author> is provided, uses the value of "git config user.email". If no <branch> is provided, "master" is used. |
|||
: NOTE: <branch> is expected as the first argument, if it exists at all, as such it must be specified before any <author> parameter. |
|||
: NOTE: This alias requires the user to have the git-reviewed.sh script (see below) saved in their home directory and set executable. If you save the script to a different location, modify the above alias so that "~/git-reviewed.sh" is changed to the location of the script. |
|||
==== git-reviewed.sh ==== |
|||
<pre> |
|||
#!/bin/bash |
|||
# git-reviewed.sh <AUTHOR> ... |
|||
# |
|||
# A git utility script to append review tags to the current commit's message |
|||
# Each tag takes the format of "Reviewed-by: <AUTHOR>". Each parameter to this |
|||
# script is treated as a separate <AUTHOR> which will have its own tag. |
|||
# Each <AUTHOR> is taken as a literal string, so if <AUTHOR> has spaces be |
|||
# sure to enclose it in double quotes. |
|||
if [[ $# -eq 0 ]] ; then |
|||
echo "ERROR: Must provide at least one author." |
|||
exit 1 |
|||
fi |
|||
AUTHORS=() |
|||
while [[ $# -gt 0 ]]; do |
|||
AUTHORS+=("$1") |
|||
shift |
|||
done |
|||
MSG_FILE=".git/COMMIT_EDITMSG" |
|||
MSG=$(git log -1 --pretty=%B) |
|||
TAGS="" |
|||
for AUTHOR in "${AUTHORS[@]}"; do |
|||
RB="Reviewed-by: "$AUTHOR |
|||
if !(echo -e "$MSG" | grep -qs "^$RB"); then |
|||
TAGS=${TAGS}${RB}"\n" |
|||
fi |
|||
done |
|||
if [ -n "$TAGS" ]; then |
|||
echo "$MSG" > $MSG_FILE |
|||
sed -i "\$a$TAGS" $MSG_FILE |
|||
git commit --amend -F $MSG_FILE --cleanup=strip |
|||
fi |
|||
</pre> |
|||
=== ~/.tigrc === |
|||
'''[http://jonas.nitro.dk/tig/ tig]''' is a text-based, vim-like interface/browser for use with git. It is also fairly extensible. The following is a simple example of a tig configuration file (~/.tigrc) that triggers 'git reviewed' against the currently selected branch HEAD in the refs view when the user enters 4. This rebases the current checked-out branch against the selected branch and marks all rebased commits as '''Reviewed-by''' for the author: |
|||
<pre> |
|||
bind refs 4 !git reviewed %(branch) |
|||
</pre> |
|||
== Where to submit for review == |
|||
Patches can be sumbitted for review to the samba-technical mailing list. It is currently not required to send patches to the mailing list, so patches can of also be send to an individual reviewer directly. |
|||
== How to produce patches for submission == |
|||
The patches should be git-patches suitable for applying them with the "git am" command. Such patches can can either be created with the command |
|||
git format-patch |
|||
and attached to a mail to the mailing list or sent direclty with |
|||
git send-email |
|||
If using git format-patch on a patchset (more than one commit), on can either create a series of patchfiles numbered 0001 ... 000N or use the syntax |
|||
git format-patch --stdout |
|||
to create a single mbox style file containing the whole patchset. This can then be applied with a single git am command. |
|||
== Using a repository == |
|||
If the author pushes his patches to a private git repository, it might be helpful to also send the gitweb url and/or git repo url, git branch name and git commit hash(es) in the patch submission email, so that potential reviewers can easily fetch the submitter's repository, check out the branch or directly cherry-pick the patch(es). This removes the need to copy around text-files. |
|||
== Tracking submitted patches == |
|||
We are currently evaluating the tool "patchwork" for tracking the patches that have been submitted for review to the samba-technical mailing list. A test installation is temporarily redirected from the address [https://patchwork.samba.org/ https://patchwork.samba.org/]. |
|||
== Reviewer's action == |
|||
How should a reviewer act on a submitted patch? |
|||
# Apply the patchset to a local checkout of the master branch. This can be done by copying the patch file and doing "git am" or by direclty pulling the submitters git repository if available. |
|||
# Review the patches (see below for hints), do all the builds and test that seem appropriate and necessary. |
|||
# If the reviewer is OK with the patch(es) she should add the "Reviewed-by: " tag to the commit message(s) and either push the patches to master via autobuild or git format-patch the adapted patches and send them back to the author. |
|||
# If the reviewer is not OK with the patch(es), she should send comments to the author and possibly wait for the next iteration of this process. |
|||
= How To Review Code = |
|||
(by metze, move to own page?) |
|||
I think everybody can learn how to do good code review |
I think everybody can learn how to do good code review |
||
and I think the most important thing is that everybody |
and I think the most important thing is that everybody |
||
reviews his/her own patches in the same way |
reviews his/her own patches in the same way one would review |
||
patches from others! |
patches from others! |
||
Here are some guidelines for a successful review process: |
|||
* First read the commit message, does that give you the impression the change is in anyway complex. If so do a "careful_mode++". |
|||
* First read the commit message. Does that give you the impression the change is in anyway complex? If so do, a "careful_mode++". |
|||
* Read the diffstat |
* Read the diffstat. Does this change touches any critical areas of code? If so, do a "careful_mode++". |
||
* First read the patch hunk by hunk and watch for Coding-Style issues. If there strange things do |
* First read the patch hunk by hunk and watch for Coding-Style issues. If there are strange things, do a "careful_mode++". |
||
* Read the patch again as whole, watch for changes which could |
* Read the patch again as whole, watch for changes which could possibly change the blackbox behavior of a function. |
||
* Depending on the "careful_mode" you should try to fully understand the change. |
* Depending on the "careful_mode", you should try to fully understand the change. |
||
* Note: Here it's sometimes useful to use the -U<contextlines> feature of git show/diff, so that you see the changes in the full context of the whole function. |
* Note: Here it's sometimes useful to use the -U<contextlines> feature of git show/diff, so that you see the changes in the full context of the whole function. |
||
* If you found a change in the behavior set "very_careful_mode = true". |
* If you found a change in the behavior, set "very_careful_mode = true". |
||
* Ask yourself why a change is needed! If you don't see why it's needed set "very_careful_mode = true". |
* Ask yourself why a change is needed! If you don't see why it's needed, set "very_careful_mode = true". |
||
* If you find things you don't understand set "verfy_careful_mode = true". |
* If you find things you don't understand, set "verfy_careful_mode = true". |
||
* If very_careful_mode != true you |
* If very_careful_mode != true you may be done. This happens most of the time and most of the time careful_mode is also 0. And the costs are not too high. |
||
* Maybe ask other people for review. Trigger discussions on the samba-technical@samba.org mailing list. |
* Maybe ask other people for review. Trigger discussions on the samba-technical@samba.org mailing list. |
||
* Read the related sources |
* Read the related sources. |
||
* |
* Use git grep to find callers of a function. |
||
* Do manual runtime tests |
* Do manual runtime tests. |
||
== Review |
== Review your own code! == |
||
I think the most important thing is that everybody |
I think the most important thing is that everybody |
Revision as of 18:42, 22 October 2015
Code Review in Samba - Mandatory
The Samba project requires code reviews, Authors must get their patches reviewed before pushing them to master via the autobuild system.
policy
The policy we have established for mandatory code code review is this:
- Each commit in master should have been reviewed by at least two samba team members. For this purpose, involvement in patch creation is considered as reviewing.
commit message tags
There is currently no prescribed way or tool to submit patches for review or mark patches as reviewed. The decisive point is that the patches that end up in master carry notice that they have been reviewed. We achieve this by adding certain tags to the commit messages using the command
git commit --amend
These are the tags we use:
- Signed-off-by
- The Author(s) of a patch should add their signature to the commit message by adding a "Signed-off-by: " tag.
- This tag indicates that the the person it lists was involved in the creation of the patch, that it is fine with the state of the patch and submits it under the license(s) of the affected files.
- Author(s) submitting a patch on behalf of an employer should also follow the signing process as described in the Samba Copyright Policy (https://www.samba.org/samba/devel/copyright-policy.html).
- git offers the "-s" or "--signoff" switch for the sub-commands commit, cherry-pick and am (apply-mailbox) which allows you to add the Signed-off-by tag without having to type it.
- Reviewed-by
- A reviewer should add a "Reviewed-by: " tag if she was not involved in the creation of the patch.
- There is currently no git switch smilar to "--signoff", and according to git developers, there won't be. So we have to add the review tags manually.
These tags are inspired by the use in the Linux kernel, but note that they don't always have the same meaning and/or significance in Samba that they have for Linux -- particularly with regard to the DCO. See the Linux website about submitting patches for detailed explanations on how they use these tags.
The Linux also makes use of an "Acked-by: " tag, but I suggest that we don't use it for now to keep things simple, since it does not make a lot of sense, unless we have established maintainers all over the code base and those need to Ack patches for their areas.
If the patch was created by multiple authors, they can optionally add a "Pair-programmed-with: " tag to the commit messages. This is redundant though.
Tools and Scripts
A number of developers use various tools, scripts, hacks, and shortcuts to ease the mechanics of the review process.
~/.vimrc
Here is a sample of a configuration snippet for use with vim:
function! CommitMessages() let g:git_ci_msg_user = substitute(system("git config --get user.name"), '\n$', '', '') let g:git_ci_msg_email = substitute(system("git config --get user.email"), '\n$', '', '') nmap S oSigned-off-by: <C-R>=printf("%s <%s>", g:git_ci_msg_user, g:git_ci_msg_email)<CR><CR><ESC> nmap R oReviewed-by: <C-R>=printf("%s <%s>", g:git_ci_msg_user, g:git_ci_msg_email)<CR><ESC> iab #S Signed-off-by: <C-R>=printf("%s <%s>", g:git_ci_msg_user, g:git_ci_msg_email)<CR> iab #R Reviewed-by: <C-R>=printf("%s <%s>", g:git_ci_msg_user, g:git_ci_msg_email)<CR> iab #O Signed-off-by: iab #V Reviewed-by: iab #P Pair-Programmed-With: iab ME <C-R>=printf("%s <%s>", g:git_ci_msg_user, g:git_ci_msg_email)<CR> iab ASN Andreas<SPACE>Schneider<SPACE><asn@samba.org> iab AB Alexander<SPACE>Bokovoy<SPACE><ab@samba.org> iab OBNOX Michael<SPACE>Adam<SPACE><obnox@samba.org> iab VL Volker<SPACE>Lendecke<SPACE><vl@samba.org> iab METZE Stefan<SPACE>Metzmacher<SPACE><metze@samba.org> iab GD Guenther<SPACE>Deschner<SPACE><gd@samba.org> iab JRA Jeremy<SPACE>Allison<SPACE><jra@samba.org> iab JHROZEK Jakub<SPACE>Hrozek<SPACE><jhrozek@redhat.com> iab ARIS Aris<SPACE>Adamantiadis<SPACE><aris@0xbadc0de.be> endf autocmd BufWinEnter COMMIT_EDITMSG,*.diff,*.patch,*.patches.txt call CommitMessages()
~/.gitconfig
Here are a few git aliases for code review:
[alias] author = !"SEARCH=${@:-`git config user.email`}; set --; git log --format='%aN <%aE>' | uniq | grep -m 1 \"$SEARCH\"" authors = !"SEARCH=${@:-`git config user.email`}; set --; git log --format='%aN <%aE>' | grep \"$SEARCH\" | uniq | awk '!a[$0]++'" reviewed = !"BRANCH=${1:-master}; shift; AUTHORS=\"~/git-reviewed.sh\"; for A in \"${@:-`git config user.email`}\"; do AUTHORS=$AUTHORS\" \\\"`git author $A`\\\"\"; done; set --; git rebase -i -x \"$AUTHORS\" $BRANCH"
- git author [<search>]
- Search for the latest author tag (a string in the format of "Full Name <email@example.com>") matching a given search term. If no <search> is provided, return the latest author tag that uses the current value of "git config user.email".
- git authors [<search>]
- Similar to "git author", except returns all author tags that match the search term. Useful in cases where a particular author has multiple e-mail addresses or multiple spellings to their name.
- git reviewed [<branch> [<author> ...]]
- Run a "git rebase -i <branch>" command that is configured to insert Reviewed-by tags for every <author> listed. <author> must be a search term as used in "git author" above. If no <author> is provided, uses the value of "git config user.email". If no <branch> is provided, "master" is used.
- NOTE: <branch> is expected as the first argument, if it exists at all, as such it must be specified before any <author> parameter.
- NOTE: This alias requires the user to have the git-reviewed.sh script (see below) saved in their home directory and set executable. If you save the script to a different location, modify the above alias so that "~/git-reviewed.sh" is changed to the location of the script.
git-reviewed.sh
#!/bin/bash # git-reviewed.sh <AUTHOR> ... # # A git utility script to append review tags to the current commit's message # Each tag takes the format of "Reviewed-by: <AUTHOR>". Each parameter to this # script is treated as a separate <AUTHOR> which will have its own tag. # Each <AUTHOR> is taken as a literal string, so if <AUTHOR> has spaces be # sure to enclose it in double quotes. if [[ $# -eq 0 ]] ; then echo "ERROR: Must provide at least one author." exit 1 fi AUTHORS=() while [[ $# -gt 0 ]]; do AUTHORS+=("$1") shift done MSG_FILE=".git/COMMIT_EDITMSG" MSG=$(git log -1 --pretty=%B) TAGS="" for AUTHOR in "${AUTHORS[@]}"; do RB="Reviewed-by: "$AUTHOR if !(echo -e "$MSG" | grep -qs "^$RB"); then TAGS=${TAGS}${RB}"\n" fi done if [ -n "$TAGS" ]; then echo "$MSG" > $MSG_FILE sed -i "\$a$TAGS" $MSG_FILE git commit --amend -F $MSG_FILE --cleanup=strip fi
~/.tigrc
tig is a text-based, vim-like interface/browser for use with git. It is also fairly extensible. The following is a simple example of a tig configuration file (~/.tigrc) that triggers 'git reviewed' against the currently selected branch HEAD in the refs view when the user enters 4. This rebases the current checked-out branch against the selected branch and marks all rebased commits as Reviewed-by for the author:
bind refs 4 !git reviewed %(branch)
Where to submit for review
Patches can be sumbitted for review to the samba-technical mailing list. It is currently not required to send patches to the mailing list, so patches can of also be send to an individual reviewer directly.
How to produce patches for submission
The patches should be git-patches suitable for applying them with the "git am" command. Such patches can can either be created with the command
git format-patch
and attached to a mail to the mailing list or sent direclty with
git send-email
If using git format-patch on a patchset (more than one commit), on can either create a series of patchfiles numbered 0001 ... 000N or use the syntax
git format-patch --stdout
to create a single mbox style file containing the whole patchset. This can then be applied with a single git am command.
Using a repository
If the author pushes his patches to a private git repository, it might be helpful to also send the gitweb url and/or git repo url, git branch name and git commit hash(es) in the patch submission email, so that potential reviewers can easily fetch the submitter's repository, check out the branch or directly cherry-pick the patch(es). This removes the need to copy around text-files.
Tracking submitted patches
We are currently evaluating the tool "patchwork" for tracking the patches that have been submitted for review to the samba-technical mailing list. A test installation is temporarily redirected from the address https://patchwork.samba.org/.
Reviewer's action
How should a reviewer act on a submitted patch?
- Apply the patchset to a local checkout of the master branch. This can be done by copying the patch file and doing "git am" or by direclty pulling the submitters git repository if available.
- Review the patches (see below for hints), do all the builds and test that seem appropriate and necessary.
- If the reviewer is OK with the patch(es) she should add the "Reviewed-by: " tag to the commit message(s) and either push the patches to master via autobuild or git format-patch the adapted patches and send them back to the author.
- If the reviewer is not OK with the patch(es), she should send comments to the author and possibly wait for the next iteration of this process.
How To Review Code
(by metze, move to own page?)
I think everybody can learn how to do good code review and I think the most important thing is that everybody reviews his/her own patches in the same way one would review patches from others!
Here are some guidelines for a successful review process:
- First read the commit message. Does that give you the impression the change is in anyway complex? If so do, a "careful_mode++".
- Read the diffstat. Does this change touches any critical areas of code? If so, do a "careful_mode++".
- First read the patch hunk by hunk and watch for Coding-Style issues. If there are strange things, do a "careful_mode++".
- Read the patch again as whole, watch for changes which could possibly change the blackbox behavior of a function.
- Depending on the "careful_mode", you should try to fully understand the change.
- Note: Here it's sometimes useful to use the -U<contextlines> feature of git show/diff, so that you see the changes in the full context of the whole function.
- If you found a change in the behavior, set "very_careful_mode = true".
- Ask yourself why a change is needed! If you don't see why it's needed, set "very_careful_mode = true".
- If you find things you don't understand, set "verfy_careful_mode = true".
- If very_careful_mode != true you may be done. This happens most of the time and most of the time careful_mode is also 0. And the costs are not too high.
- Maybe ask other people for review. Trigger discussions on the samba-technical@samba.org mailing list.
- Read the related sources.
- Use git grep to find callers of a function.
- Do manual runtime tests.
Review your own code!
I think the most important thing is that everybody reviews his/her own patches in the same way you would review patches from others!