How do do bulk reviews?

[Michael Adam]

Hi Andrew,

I usually do reviews also of a bigger patchset like this:

0. look over the whole patchset
1. apply the whole patchset to a checkout of master
2. check that it compiles and passes make test, etc
3. rebase -i origin master and choose "edit" for each commit
4. at each commit
   - look at it thoroughly
   - maybe (ideally) check it compiles
   - do "git ci --amend", adding my reviewed-by tag

For adding the reviewed-by, I use the metze's macros for vim
(see https://wiki.samba.org/index.php/CodeReview#.7E.2F.vimrc)
and only type "R" in the editor.

I think it does not harm to touch each commit message
manually, also checking the message itself.
IMHO, this is what is required to do serious and not superficial
reviews.

Cheers - Michael



On 2014-01-26 at 14:31 +1300, Andrew Bartlett wrote:
> On Fri, 2014-01-24 at 16:49 -0800, Jeremy Allison wrote:
> 
> > Thanks - going through make test right now.
> > 
> > Once it passes I'll push (probably Monday morning
> > pacific time. sorry).
> 
> Great.  Once that's in I'll push the sets Alexander reviewed for me, or
> you can find them in my param-reviewed branch. 
> 
> But one question I had:  How do folks do the marking of bulk reviews
> like this?  For small series, I just git rebase -i, and then reword the
> commit message manually (or copy/paste), or when I'm doing it for
> others, I git rebase -i and REVIEW=1 git commit --amend && git rebase
> --continue using obnox's scripts.
> 
> But for this many, what I did was:
> 
> GIT_COMMITTER_NAME="REPLACE" GIT_COMMITTER_EMAIL="ME" git format-patch
> --stdout origin/master..HEAD -s > param-reviewed.patch
> 
> And then a search/replace in emacs for Signed-off-by: REPLACE <ME> with
> Reviewed-by: Alexander Bokovoy <ab at samba.org>, then git reset --hard
> origin/master && git am param-reviewed.patch to put the patchset back
> onto a clean branch. 
> 
> It's a hack, so I'm curious what others do.  (My preferred review
> interface - gitk - sadly doesn't have a 'review' checkbox, or else I
> would love to just tick that as I go along). 
> 
> Thanks,
> 
> Andrew Bartlett
> 
> -- 
> Andrew Bartlett                       http://samba.org/~abartlet/
> Authentication Developer, Samba Team  http://samba.org
> Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 215 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140127/ba97f5ac/attachment.pgp>