Git SCMTool - update

Review Request #141 — Created Aug. 11, 2007 and submitted

Information

Review Board SVN (deprecated)
trunk
87

Reviewers

An update to Nick Loeve's git support patch reviewed at http://reviews.review-board.org/r/80/.

This patch adds support for the new DiffParser, along with some more clean up and unit test coverage. It requires a test git repository at scmtools/testdata/git_repo. A tarball of a repo that contains the correct SHA1 refs to work with the unit tests is at  http://mojain.com/static/git_repo.tar.gz.

Update 2007-08-13: uploaded a new diff that applies against rev 877.

Update 2007-08-14: uploaded a new diff (on the third attempt) that correctly handles diffs that delete files (oops!), and implements Josh Stone's suggestion for file_exists().

Update 2007-08-16: new diff that addresses Josh's comments, including restoring a bunch of get_file tests that I lost in a bad merge.
The patch contains unit tests that exercise the git diff parser and repository client code. All scmtool tests pass.
CU
  1. 
      
  2. trunk/reviewboard/scmtools/fixtures/git.json (Diff revision 5)
     
     
     
     
     
     
     
     
    Shouldn't this be part of initial_data.json?  I noticed you left it out for Hg too...
    1. That was partially confusion about what the various JSON fixtures did, but mostly because I was managing both patches (hg and git) at once, and I couldn't make them both apply to initial_data.json cleanly. :) It would be worth thinking about a more robust way of doing this that doesn't depend on hard coded primary keys, etc. 
  3. trunk/reviewboard/scmtools/git.py (Diff revision 5)
     
     
     
    I think it's enough to just use "-t".  The "-e" only optimizes the error case, and even then it's only slightly faster.  For the general case it's better to just invoke git once.
  4. trunk/reviewboard/scmtools/git.py (Diff revision 5)
     
     
    You lost the "return contents" at the end here...
    1. Yes, and I also lost the tests that would have caught that... :-/ I've been trying to convert my patch development to git (http://git.mojain.com/cgi-bin/gitweb.cgi?p=mirrors/reviewboard.git;a=shortlog;h=add-git-scmtool), so I must have broken something in the process. New patch coming soon.
  5. I tried this out against my Linux kernel tree, and it seems to work well.
    
    The only gotcha I ran into was that on one diff I got an error from git, "error: short SHA1 10bb5a8 is ambiguous."  By default, "git diff" only puts the first 7 characters of the SHA1 in the index line, which in this case turned out to be ambiguous.  One would hope that "git diff" could have avoided that automatically, à la "git rev-parse --short".  The workaround is to use "git diff --full-index" to generate the patch.  If we ever add git support to "post-review", we should probably do this by default.
    
    By the way, for RB developers: if you want a huge stress test, try a patch of a Linux kernel release.  "git diff --full-index v2.6.21..v2.6.22" is a 33MB patch, and I gave up on RB trying to load it... :/
CU
  1. 
      
  2. trunk/reviewboard/scmtools/git.py (Diff revision 5)
     
     
    This can be a class member instead of an instance member, and then you don't need an __init__() anymore.
  3. 
      
CU
  1. Looks good to me.
  2. 
      
david
  1. Looks great.  I'm going to commit this to SVN and close out both reviews.  Thanks guys!
  2.