Git SCMTool

Review Request #80 — Created June 18, 2007 and submitted — Latest diff uploaded

Information

Review Board SVN (deprecated)
trunk
87

Reviewers

Update:

Style changes as per David's review. I also switched SCMException to SCMError

======================================================================

Update:

Include changes to the diff parsing test :)

======================================================================

Update:

Only change is storing the whole diff for a file, including the 'diff --git' line

======================================================================

Update:

This change now relies on the DiffParser class in http://reviews.review-board.org/r/99/

You can still only talk to a local git repository.

 - Refactored diff parsing to better support Git diff format
 - Added Unittests for diff parsing

======================================================================

Update:

New patch coming without DiffParser changes, better git diff parsing, tests.

======================================================================

Update:

 - made new DiffParser class
 - updated unittests
 - minimized code duplication in diff parsing 

I have made a DiffParser object now, as it seemed to be the easiest way to override specific behaviour without duplicating chunks of code. 

For other SCM systems, it may pay to split out the DiffParser class even more, so you only need to override discreet operations, but it works for parsing diffs generated with Git, and there is no real code duplication.


======================================================================

Update:

 - changed form.py call the parseDiff method on the SCMTool
 - made SCMTool.parseDiff use the default parser
 - update GitTool to reflect the above changes, but with custom per file parser

I have not looked into abstracting out the per file parser to remove code duplication

======================================================================

Update:

This diff now abstracts out the diff parsing in reviewboard a little bit:

 - an SCMTool subclass can optionally provide a whole new diff parsing method
 - an SCMTool subclass can now optionally provide a per-file diff parsing method (this was needed for Git)
 - the GitTool has been updated to provide its own per-file diff parsing method

The CVS tool could be updated to override the main diff parsing method, extract the information needed and then proceed by calling the standard parse function. This would abstract away the need for all SCMTools to have a parse_extra_info_headers method.

======================================================================

This is the start of my effort to get Git support for Review Board.

I don't think it is really ready, but I would like to see if I am heading in the right direction. At the moment it only handles talking to a Git repo on the local filesystem. This can be expanded when needed (works fine for us).

I feel like the whole usage of a revision is a bit clunky, as Git uses objects with unique identifiers. I have pretty much just used an object id as a revision, just so i could get it working. 
Update:

Unittest passes again :)

======================================================================

Update:

Current tests still pass, and I have added a unittest for testing the parsing of Git patches.


======================================================================

Update: 

I updated the unittests for using the DiffParser class, and it works fine. Again, I don't have/use Perforce, so it may pay off to try that with the new class.


======================================================================

Update:

All tests still pass

======================================================================

Update:

All current unit tests for diff parsing work. The Perforce tests are skipped, as I do not use this SCM or have the python modules installed.

======================================================================

We have it talking to a Git repository, applying patches and general review functionality is working.

No accompanying unittests yet.
    Loading...