Code Review
Some comments on code review (from the HANDE documentation)
- All development happens in branches.
- Branches are merged into master after review. Merging between development branches should be avoided.
- Branches should be reviewed by one other person (at least) before merging into master.
- Once you have something stable which you think works and might be understandable to someone else (this needn't be fully working complete program - it could be subroutines or a toy version), you should submit it to review.
- To review, send a pull request email (see git request-pull) to all developers (perhaps including a summary of work in the branch, which is not generated by request-pull!). This should be viewed as starting a conversation on the work.
- Make changes prompted by the review and resend the pull request. (This might take a few iterations.)
- How do I review code?
We're working on a workflow for this. One method is to make a branch (if you're not already in one) and just add comments to the source. It's helpful if the review is part of the git history (even if the comments never actually make it to the master). We currently are using watson-style http://goosecode.com/watson/ tags in comments for code review and discussion, for example:
! [review] - JSS: How about doing it this way? ! [reply] - AJWT: I thought about it but that causes problems due to X.
where JSS and AJWT are the initials of the reviewer and code author respectively.
- Will *my* code actually get reviewed?
We're all usually terribly busy and have very little time, but in a group effort a little from each person goes a long way. If you review others' code then they're more likely to review yours. Make it easy to review, by keeping it clean and the features short. Remember, this kind of review is far more lightweight than peer review of publications, and should be able to slot into people's 'free' time. (Each branch is far more lightweight than a paper.) A simple pull-request should be enough to get people to review. This is rather intricately tied in with the idea of project management. Prodding/cajoling/bullying emails are all possible to aid the review
- What happens if no-one replies to the pull request?
Here are some opinions:
+ I suggest that after an agreed upon time (X working days?) without even a "I'll review but am too busy until next week" reply, the author is free to merge it into master (but should be open to fixes/improvements to that work that others subsequently suggest). + Having been burdened with years-long old dirty branches from other projects, merging is certainly vital. I don't think lack of review should stop merging, but it should prompt someone to ask why. + I would view it as a sign that the work is stable and relatively complete (for the time being) and is ready to be used by others/in production calculations.
- Some advice from HANDE on pull requests and merging - this might be over the top for smaller projects!
How to generate a pull request
First push your work to the relevant branch on the git sever and then generate template text for the pull request:
$ git request-pull startref origin [endref]
where startref (endref) is the commit you want to be reviewed from (to) and origin is the name of remote configured to the git sever. startref and endref can be any way of referring to a specific commit and endref defaults to HEAD if not given. Usually the branch would have been created from master, in whichcase you can simply do (even if master has been committed to since the branch was created):
$ git request-pull master origin
which generates (for example)::
$ git request-pull master origin
The following changes since commit 7a58a8d1a8f2e8af15df1c9946e7596078649d79:
Updated the config files for cx2. (2013-12-09 11:07:52 +0000)
are available in the git repository at:
git@tyc-svn.cmth.ph.ic.ac.uk:hubbard_fciqmc config/cx2
for you to fetch changes up to 1a5522648378f406d3e5fbd87e22e3768da490bc:
Fixed typo cx2 config comment (2013-12-13 14:35:42 +0000)
----------------------------------------------------------------
William Vigor (1):
Fixed typo cx2 config comment
config/cx2 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Copy and paste this text into your email client and send the pull request to hande-dev@imperial.ac.uk (possibly with some additional text describing motivation/benchmark results/etc). If sendmail/exim4/other MTA is set up properly (naturally the CMTH ones are) then
$ git request-pull master origin | mail -s "Pull request" hande-dev@imperial.ac.uk
works as one would expect.
Merging to master
Here's a workflow to make merging to master simple. Remember that with git it's extremely difficult to make permanently destructive changes so if it goes wrong it can be fixed.
Before you start make sure your code compiles and passes the test suite. Do not merge broken code into master.
Now make sure your master branch is up to date. Here I do this in a fetch then a pull just to see what else has changed. I do a diff to be sure I'm the same as the origin master.
[master]$ git fetch
remote: Counting objects: 340, done.
remote: Compressing objects: 100% (182/182), done.
remote: Total 200 (delta 137), reused 47 (delta 16)
Receiving objects: 100% (200/200), 96.89 KiB, done.
Resolving deltas: 100% (137/137), completed with 58 local objects.
From tyc-svn.cmth.ph.ic.ac.uk:hubbard_fciqmc
c17ef9e..2d8e130 master -> origin/master
...
[master]$ git pull
Updating c17ef9e..2d8e130
Fast-forward
lib/local/parallel.F90 | 9 ++-------
src/full_diagonalisation.F90 | 30 ++++++++++++------------------
2 files changed, 14 insertions(+), 25 deletions(-)
[master]$ git diff origin/master
The blank output from this indicates we're at origin/master.
I'm going to merge the branch bug_fix/rdm_init. Crucially we use the --no-ff flag to ensure that the merge creates a commit on master; this keeps the history clean (by keeping development work in logical chunks after merging) and also makes it very easy to roll-back and revert an entire feature if problems are encounted.
[master]$ git merge --no-ff bug_fix/rdm_init
Merge made by the 'recursive' strategy.
src/fciqmc_data.f90 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[master]$ git log --graph --oneline --decorate | head
* 647b7dd (HEAD, master) Merge branch 'bug_fix/rdm_init'
|\
| * 3c67d81 (bug_fix/rdm_init) Fix uninitialised doing_exact_rdm_eigv breaking fci
* | 2d8e130 (origin/master, origin/HEAD) Merge branch 'bug_fix/small_fci_mpi'
|\ \
This shows that a new commit has been created on master.
At this point it's possible that the merge needed some manual intervention. It's fine to make these changes directly and commit them in the merge to your local master. If the merge is starting to get messy it might be best to rebase first to make it easier.
Very importantly, you should now compile the code and run the tests, even if the merge completed without any problems --- there might be unintented effects. Only continue if the code compiles and the tests pass. If you need to make changes at this point, you can modify your local existing merge commit with
[master]$ git commit --amend
Now we've made sure that the code works, all we do is push to the main repo
[master]$ git push origin master
Counting objects: 12, done.
Delta compression using up to 12 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 705 bytes, done.
Total 7 (delta 5), reused 0 (delta 0)
To git@tyc-svn.cmth.ph.ic.ac.uk:hubbard_fciqmc.git
2d8e130..647b7dd master -> master
[master]$ git log --graph --oneline --decorate | head
* 647b7dd (HEAD, origin/master, origin/HEAD, master) Merge branch 'bug_fix/rdm_init'
|\
| * 3c67d81 (bug_fix/rdm_init) Fix uninitialised doing_exact_rdm_eigv breaking fci
* | 2d8e130 Merge branch 'bug_fix/small_fci_mpi'
|\ \
Almost there. We now ought to clean up the namespace to avoid old branch names hanging around (the code of course will always stay).
[master]$ git branch --delete bug_fix/rdm_init [master]$ git push origin --delete bug_fix/rdm_init
The list of branches merged into HEAD can be found by doing
[master]$ git branch --all --merged
All done!