Add support for parent diffs for use in distributed version control systems

Review Request #382 — Created May 15, 2008 and submitted

chipx86
Review Board SVN (deprecated)
trunk
reviewboard
Added support for basing a diff off of another diff. This is useful in distributed version control systems where a local checkout might contain a large branch with changes and smaller topic branches off of that, and the developer wants to put the smaller topic branches up for review. Previously this would be impossible without including changes from the parent branch (as Review Board would have no way of accessing that local checkout). Now the diff between the master repository and the larger branch could be uploaded as the parent diff along with the topic branch's diff.

Any code repository can take advantage of this, whether Git, Mercurial or even Subversion or CVS. However, only the DVCS repositories actually list the Parent Diff field on the New Review Request page, as it's generally not useful for other repositories. However, post-review (in a future change) will be able to post a parent diff with any repository type, making this available when using Git as an SVN front-end.

To sum it up, with this change, Review Board begins to become a really useful tool for distributed version control systems. Near future changes will include adding this support to post-review, and probably adding some unit tests. The change is still somewhat experimental, so it would be useful to get people to test this.

NOTE: This change also introduces Django Evolution support. Django Evolution (http://django-evolution.googlecode.com) is a framework being developed to support migration of Django databases on SQLite, MySQL or PostreSQL. This is a much faster alternative to our backup-db.py and load-db.py scripts.

From now on (and starting with this change), users should run ./manage.py syncdb after updating their copy of Review Board. This will now let them know if Review Board needs a database migration. If so, the user should just have to run:

./manage.py evolve --execute

This will perform the database migration. If it can't be performed, it will inform the user right away, and shouldn't cause any problems in the database. The user should then just roll back their checkout. It also means users won't have to deal with the potentially long backup-db/load-db migration, and won't have to worry about doing anything before their update.

Once this is in, the wiki will be updated to inform users on how to handle migrations in the future.
Tested various diffs with parent diffs. Things seem to work as one would expect. There are some CSS changes that need to be made later (particularly with changes against new files in the parent diff) but it appears to be functional.

Also tested that database migration is working correctly on SQLite. I'll be testing MySQL before this goes in, but it should be working according to the docs. There might be issues in the future with really complicated migrations on MySQL but it didn't look like any of the changes we usually make should hit any of the known listed problems.
CU
  1. I haven't had a chance to try it yet, but this looks cool!
    
    If I understand it right, when I have a patch series A, B, and C, I'll need to submit 3 requests with:
    
      1. diff=A, parent=None
      2. diff=B, parent=A
      3. diff=C, parent=A+B
    
    I think that generating A+B will be confusing to people, but post-review ought to be able to handle this readily.
    
    Since I'm always looking for more enhancements, it would be great if there was a way to indicate the relationship between requests 1, 2, and 3.  (Though I understand with DVCS that you won't always have such a relationship, e.g. if you were only submitting part C for review).
  2. /trunk/reviewboard/diffviewer/forms.py (Diff revision 1)
     
     
    Probably don't want this committed.
  3. /trunk/reviewboard/reviews/forms.py (Diff revision 1)
     
     
     
    The parent is optional, but you have required=True?
  4. The great irony is that this review request would be well represented as a series of its own: add django-evolution; convert diffs to the new Base64 type; add support for parent diffs. :)
david
  1. I'm not sure that we should include parent diff on the "New Review Request" form.  As much as this form sucks already, I think adding this field will only confuse people further.
    
    I realize that this means you'll need to implement post-review support for this feature.  You were gonna do that already, right?
    
    As far as this implementation is concerned, in addition to what Josh already found:
  2. /trunk/reviewboard/diffviewer/forms.py (Diff revision 1)
     
     
     
    This should probably be in () instead of using \
  3. /trunk/reviewboard/diffviewer/models.py (Diff revision 1)
     
     
    You can get rid of this line.
  4. 
      
david
  1. Looks good.
  2. 
      
Loading...