Re: How apply patches in pre-review process?

2010-05-01 Thread Jan Koprowski


On Apr 30, 7:58 pm, Chris Clark chris.cl...@ingres.com wrote:
 Jan Koprowski wrote:
  . When review is done and submitted
  developer can change everything he want in working tree and commit
  everything he want. I'm thinking how be sure that reviewed changes is
  commited changes.

 For us, we are just using the honour/honor system, i.e. no checks :-)
 This mostly works BUT occasionally people do make mistakes (e.g. submit
 an extra file that was not in the review) so a check would be nice, for
 me personally I'm not sure it is worth spending the time on it.

Implementation of this is time consuming. This problem disappear  in
post-review process (at least in my model). In my MA I assume I don't
trust developer in anything.

 The other idea similar to your manual reviewer idea would be for a
 script to pull the diff from RB (that is marked with Ship It!) and
 submit the change (assuming the patch applies cleanly). If patches don't
 apply cleanly the review could be marked with patch didn't apply
 cleanly to [rev in SCM] and the developer an resubmit. If you already
 have a buildbot system this is probably your best option (especially if
 you have tests as part of the build bot, you could add additional
 rejection checks).

I use CruiseControl and Hudson in my system. I don't see simple way to
apply patches cleanly. This is impossible. The problem is from which
point of development tree You create branch. When You create 10
branches, then locally make changes for each of them and submit one by
another to review. Look what will happen.
Central SVN repo doesn't know about Your changes because there are
only on Your computer. So You submit first review, merge to trunk and
ok. You get second review, try to merge to trunk but - You merge this
second review with code different then their baseline (to different
code then this from You branch). This is the nest of potential
problems. I don't imagine this process without human intervention.

 I've deliberately minimized the procedures/policies we have for RB at
 our site so this isn't something I plan on implementing myself but I can
 see the value.

IMHO much more simple is post-review. All developer commit to one
development branch but with each branch there is related ticket in
issue tracker with changes when I have list of revision numbers made
for this particular change. In hooks and in build machine I can still
check status of this ticket and I know what is the status of code
related with them. Also I can controll access to repository based on
status.
The great profit is the developers for all the time integrate the code
in the branch by him self. This is grate information. But there is two
problems:


1) ReviewBoard doesn't like diffs from not continuous revisions like:
1-3, 8-9. This is not a big problem I think but no one try this
before. Opportunity is allow save in ticket only continuous the
highest. But if we have two tickets:
a) revision 1-9
b) revision 3-7
First one will include changes related with second. This is a little
problem.

2) Second problem is related with first. Before build we must check
status of all tickets which was reviewed and clever find place until
all code was reviewed. This is not very hard but need to be made. We
also need to know where last build end to know which revision number
is our baseline :D

But I prefer this model then pre-review :D

-- 
Want to help the Review Board project? Donate today at 
http://www.reviewboard.org/donate/
Happy user? Let us know at http://www.reviewboard.org/users/
-~--~~~~--~~--~--~---
To unsubscribe from this group, send email to 
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at 
http://groups.google.com/group/reviewboard?hl=en


How apply patches in pre-review process?

2010-04-30 Thread Jan Koprowski
Hi!

  I'm thinking a lot about how and who should apply patches in pre-
review process? I see one problem. When review is done and submitted
developer can change everything he want in working tree and commit
everything he want. I'm thinking how be sure that reviewed changes is
commited changes. For now I see one option. Reviewer download raw diff
from ReviewBoard, apply patches to master/trunk resolving all
conflicts themselves. Is there another way which could automate this
process with avoiding conflicts? Another way is allow allow commit
changes after review but in commit hook check is diff from commiting
code is identical with ReviewBoard diffs. But this looks a little bit
too tricky. Can You share You experience?

Greetings from Poland!
--
Jan Koprowski

-- 
Want to help the Review Board project? Donate today at 
http://www.reviewboard.org/donate/
Happy user? Let us know at http://www.reviewboard.org/users/
-~--~~~~--~~--~--~---
To unsubscribe from this group, send email to 
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at 
http://groups.google.com/group/reviewboard?hl=en


Re: How apply patches in pre-review process?

2010-04-30 Thread Chris Clark

Jan Koprowski wrote:

. When review is done and submitted
developer can change everything he want in working tree and commit
everything he want. I'm thinking how be sure that reviewed changes is
commited changes.


For us, we are just using the honour/honor system, i.e. no checks :-) 
This mostly works BUT occasionally people do make mistakes (e.g. submit 
an extra file that was not in the review) so a check would be nice, for 
me personally I'm not sure it is worth spending the time on it.


For a central based SCM (like svn) you could add a commit hook that 
pulls the diff from reviewboard and compare the diff with what the SCM 
has been given, as you suggested. I suspect this is the easiest thing to 
setup and would help catch accidents  rather than deliberate 
deceptive developers ;-)


The other idea similar to your manual reviewer idea would be for a 
script to pull the diff from RB (that is marked with Ship It!) and 
submit the change (assuming the patch applies cleanly). If patches don't 
apply cleanly the review could be marked with patch didn't apply 
cleanly to [rev in SCM] and the developer an resubmit. If you already 
have a buildbot system this is probably your best option (especially if 
you have tests as part of the build bot, you could add additional 
rejection checks).


I've deliberately minimized the procedures/policies we have for RB at 
our site so this isn't something I plan on implementing myself but I can 
see the value.


Chris

--
Want to help the Review Board project? Donate today at 
http://www.reviewboard.org/donate/
Happy user? Let us know at http://www.reviewboard.org/users/
-~--~~~~--~~--~--~---
To unsubscribe from this group, send email to 
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at 
http://groups.google.com/group/reviewboard?hl=en