Git SCMTool - update

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


Review Board SVN (deprecated)


An update to Nick Loeve's git support patch reviewed at

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

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.
  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/ (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/ (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 (;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... :/
  2. trunk/reviewboard/scmtools/ (Diff revision 5)
    This can be a class member instead of an instance member, and then you don't need an __init__() anymore.
  1. Looks good to me.
  1. Looks great.  I'm going to commit this to SVN and close out both reviews.  Thanks guys!