RFC: supporting code review and partial commits better

[Robert Collins]

This is about a feature we're missing; how to do it tastefully from the
UI, and possibly some implementation thoughts. Jelmer, James Westby,
Daniel Watkins and I are sprinting in London - we've sketched some very
high level concepts - please read this and give us feedback!

One of the tasks a reviewer has to do is to decide whether some code is
'good' or not. Long patches (a few hundred lines) will often not fit in
the reviewers head all at once, so some means is needed to mark parts of
the patch as 'good' and others as 'still in review', and perhaps some
parts as 'bad'. ('bad' is useful when one is going to reject-or-fix a
patch, but there is a lot not-yet-reviewed still needing review).

Review seems to work in two primary ways: online, and as a merge to a
branch. In the former case, we can review by reading the patch in e.g.
email, or bundlebuggy. One of the reasons I prefer to review in my email
client is that I can achieve this marking of bits as 'good' by deleting
them in my reply.

However, and I've been strongly reminded of this with the recent PQM
work, when reviewing by merging a patch, there is no easy facility in
bzr to mark parts of a pending change as 'accepted' / 'rejected'.

So - I would love the ability to mark (in varying granularity down to
the hunk level) parts of the workingtree<->basis difference with a tag
of some sort. I think that putting this in the core will give best
result. Before getting into the theory, here is a possible transcript:

---
$ bzr diff
=== foo.c [selected]
--- foo.c
+++ foo.c
@ -22,3 +22,3 @@ []
 foo
-bar
+quux
 gam
@ --44,3 +44,2 @@ []
 strange
-gone
 text
$ bzr mark
1 modified files
examining foo.c [selected]
@ 22,3
---
 foo
-bar
+quux
 gam
=======
1 file 'selected'
[q/d/m/M/u/n/s/?]: ?
q=quit
d=done (save marks)
m=mark (current tag='selected')
M=mark with a different tag
u=unmark
n=next hunk
s=show all hunks
[q/d/m/M/u/n/s/?]: M
Tag to mark with: reviewed
@ --44,3 +44,2 @@ []
strange
-gone
text
1 file 'selected'
1 hunk 'reviewed'
[q/d/m/M/u/n/s/?]: d
$ bzr diff
=== foo.c [selected]
--- foo.c
+++ foo.c
@ -22,3 +22,3 @@ [reviewed]
 foo
-bar
+quux
 gam
@ --44,3 +44,2 @@ []
 strange
-gone
 text
$ bzr diff -M=-reviewed
=== foo.c [selected]
--- foo.c
+++ foo.c
@ --44,3 +44,2 @@ []
 strange
-gone
 text
$ bzr diff -M=reviewed
=== foo.c [selected]
--- foo.c
+++ foo.c
@ -22,3 +22,3 @@ [reviewed]
 foo
-bar
+quux
 gam
$ bzr commit -M=reviewed -m "Commit reviewed code"
M foo.c
committed revision 1234
$ bzr diff
=== foo.c [selected]
--- foo.c
+++ foo.c
@ --44,3 +44,2 @@ []
 strange
-gone
 text
---


Now, how would this hang together. Firstly, there is a new concept of
marks on the working tree state. Our current working trees are
equivalent to a '/':[selected] tag. That is, everything in a working
tree is tagged selected. By default diff, status, commit, will only
operate on selected content. Using the -M (or --Marked) option to
diff/status/commit will cause the diff to only include tags matching.
E.g. diff -t=-selected is (diff everything that is not selected). diff
-M=reviewed is 'diff reviewed changes only'. And diff -M=-reviewed is
'diff unreviewed changes'. because of the default of 'selected' applying
to diff/status/commit, doing 'bzr status' and 'bzr diff' will always
show you what commit will do. revert and commit will clear marks back to
the default ([selected]).

bzr shelve could be integrated with this - 'bzr shelve -M=cruft
--all' :).

This would give a really flexible system for managing complex merges -
or even just complex sets of changes in your working tree. 

Partial commit (commit -i) can then be a two-phase operation - phase
one, put up the mark editor for whatever the user requested to commit.
E.g 'bzr commit -i -M=-reviewed' (commit interactive unreviewed
changes). Mark with something like 'interactive-commit', then backend to
commit -M=interactive-commit.

A further feature some folk request is the ability to specify that only
some files are being edited. By having a configuration option to toggle
the default mark from [selected] to [], commit, diff, status etc would
all ignore all changes in the tree and need explicit instruction to
examine a file. We could add to tree building operations a check for
'selected' in the taglist - and if its not present make the files
readonly.

-Rob & the London sprinters.

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080717/f2291e87/attachment.pgp