Ajwt3 at 18:12, 16 June 2020

2020-06-16T18:12:32Z

Ajwt3 at 18:10, 16 June 2020

2020-06-16T18:10:11Z

← Older revision		Revision as of 18:10, 16 June 2020
Line 98:		Line 98:
	c17ef9e..2d8e130 master -> origin/master		c17ef9e..2d8e130 master -> origin/master
	...		...


	[master]$ git pull		[master]$ git pull
	Updating c17ef9e..2d8e130		Updating c17ef9e..2d8e130
Line 105:		Line 105:
	src/full_diagonalisation.F90 \| 30 ++++++++++++------------------		src/full_diagonalisation.F90 \| 30 ++++++++++++------------------
	2 files changed, 14 insertions(+), 25 deletions(-)		2 files changed, 14 insertions(+), 25 deletions(-)


	[master]$ git diff origin/master		[master]$ git diff origin/master

Line 117:		Line 117:
	src/fciqmc_data.f90 \| 2 +-		src/fciqmc_data.f90 \| 2 +-
	1 file changed, 1 insertion(+), 1 deletion(-)		1 file changed, 1 insertion(+), 1 deletion(-)


	[master]$ git log --graph --oneline --decorate \| head		[master]$ git log --graph --oneline --decorate \| head
	* 647b7dd (HEAD, master) Merge branch 'bug_fix/rdm_init'		* 647b7dd (HEAD, master) Merge branch 'bug_fix/rdm_init'
Line 146:		Line 146:
	To git@tyc-svn.cmth.ph.ic.ac.uk:hubbard_fciqmc.git		To git@tyc-svn.cmth.ph.ic.ac.uk:hubbard_fciqmc.git
	2d8e130..647b7dd master -> master		2d8e130..647b7dd master -> master


	[master]$ git log --graph --oneline --decorate \| head		[master]$ git log --graph --oneline --decorate \| head
	* 647b7dd (HEAD, origin/master, origin/HEAD, master) Merge branch 'bug_fix/rdm_init'		* 647b7dd (HEAD, origin/master, origin/HEAD, master) Merge branch 'bug_fix/rdm_init'

import>Wv210 at 16:54, 17 November 2015

2015-11-17T16:54:05Z

Show changes

import>Ajwt3: Created page with "Some comments on code review (from the HANDE documentation) * All development happens in branches. * Branches are merged into master after review. Merging between developme..."

2015-11-03T23:53:55Z

Created page with "Some comments on code review (from the HANDE documentation) * All development happens in branches. * Branches are merged into master after review. Merging between developme..."

New page

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 which
case 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!

Code Review - Revision history

Ajwt3 at 18:12, 16 June 2020

Ajwt3 at 18:10, 16 June 2020

import>Wv210 at 16:54, 17 November 2015

import>Ajwt3: Created page with "Some comments on code review (from the HANDE documentation) * All development happens in branches. * Branches are merged into master after review. Merging between developme..."