Summary

Mercurial (hg) SCMTool

Review Request #97 — Created June 22, 2007 and submitted

Information

Owner

mrowe

Repository

Review Board SVN (deprecated)

Branch

trunk

Bugs

137

Depends On

Reviewers

Groups

reviewboard

People

Description

Update 2007-08-09:

Moved fixture data into its own JSON file. Catch up with upstream changes. General clean up.

The tests now use a hg repo in testdata/hg_repo, which I couldn't attach to the patch. I have emailed a tarball to David.

Update 2007-08-06:

Uploaded a new diff to use the new DiffParser per Christian's comment.

Update 2007-07-17:

Uploaded a new diff that addresses the issues noted by Christian.

Also, a small fix to the parsing of diffs that create files, and a small change to make us compatible with Mercurial 0.9.4 (repo.LookupError moved, so I use a more general and hopefully stable exception).

=====

Update:

Uploaded a new diff that fixes the style/whitespace issues noted.

Now applies against rev 773 (in particular, the rename of SCMException to SCMError).

I've also cleaned up the handling of diffs that create files, and overridden a more sensible method in DiffParser.

=====

Update:

Uploaded a new diff that now applies against the DiffParser patch (http://reviews.review-board.org/r/99/) directly, rather than against the GitTool patch.

=====

A very tentative attempt at Mercurial support for reviewboard.

This patch is based on Nick Loeve's git scmtool (http://reviews.review-board.org/r/80/). NOTE that it depends on the changes to DiffParser in that patch (so that HgTool can provide a parser for hg-style diffs).

If anyone is interested, I have a mirror of the reviewboard source in mercurial (http://hg.mojain.com/mirrors/reviewboard) and a Mercurial Queues repo containing this patch and the git one it depends on (http://hg.mojain.com/patches/reviewboard).

Testing Done

Update 2007-08-09: tests working again--all tests passing. 

Update 2007-08-06: the tests are currently broken, due to an upstream change to django. I'm working on getting them fixed.

=====

I have it talking to a Mercurial repository and correctly reading diffs.

There are basic unit tests for the diff parsing and other supporting methods.

I had to remove the test that actually retrieved a file from a repo, since I haven't found a good way of getting a repo into a patch (the .hg dir contains binary files). I suspect that the answer will be to create the repo dynamically as part of the fixture setup. (I have a rough version of this working, but it's not in this patch due to being *very* rough).

```
Most of this looks pretty good.
```

trunk/reviewboard/scmtools/hg.py (Diff revision 2)

Separate these onto two lines (python style guidelines)

trunk/reviewboard/scmtools/hg.py (Diff revision 2)

This shouldn't be necessary for your tool

trunk/reviewboard/scmtools/hg.py (Diff revision 2)

Trailing whitespace.  I'll only mark this one, but you've got a few of these spread around -- just look for red blocks in the diff viewer here.

trunk/reviewboard/scmtools/hg.py (Diff revision 2)

If Hg doesn't have a concept of changesets the way that perforce does, you probably don't need these for now.  At the moment, this is just used for retrieving log information from the VCS for a revision that hasn't yet been committed.

mrowe July 4, 2007, 3:45 p.m.

Hg does have a concept of changesets--in fact it's the central concept. But it doesn't have the idea of pending changesets, so if that's the only way it's used it's probably not relevant right now.

What I'd like to be able to do is review a changeset (rather than a diff). With a distributed SCM like hg, it is a reasonable workflow to have a shared "intergration" repository to which developers push changes for review (and when a change is accepted, it can be pushed "upstream" to a release repo).

Ideally, you would create a new review for a changeset id, rather than upload a diff. I will have to look more deeply to see if this is possible. I imagine the sames ideas could be applied to Nick's git backend also.

david July 5, 2007, 3:39 p.m.

Yeah, this is probably where the LocalFile backend would come into play.  The existing backends are for showing diffs against files committed to some central repository.  If you want to show a diff against file versions that only exist in your local clone of the centralized repository, you'd need to upload a copy of the source file along with the diff.

mrowe July 16, 2007, 4:24 p.m.

I was trying to say the opposite. :) With mercurial, you can push your changes (roughly equivalent to committing in a centralised SCM) to another repo, which could be the one that reviewboard uses. Then, you can create a review request that gets its diff directly from that repo, based on a changeset id.

I have this roughly working now, so I'll upload a new diff once I've cleaned it up.

trunk/reviewboard/scmtools/hg.py (Diff revision 2)

Two blank lines between classes, please (here and below)

trunk/reviewboard/scmtools/tests.py (Diff revision 2)

We should be able to add binary files to the svn repository just fine

mrowe July 4, 2007, 3:47 p.m.

Yeah, and I should be able to add them to an hg repo just fine... but somehow moving them around as patches was breaking. I will look into it some more.

trunk/reviewboard/scmtools/tests.py (Diff revision 4)
```
ups
```

trunk/reviewboard/scmtools/hg.py (Diff revision 4)

Can you alphabetize all this?

trunk/reviewboard/scmtools/hg.py (Diff revision 4)

Variables should go after the __init__ call.

trunk/reviewboard/scmtools/hg.py (Diff revision 4)

Go ahead and just remove this function. It's not used and will probably go away.

trunk/reviewboard/scmtools/hg.py (Diff revision 4)

Can you change the indentation of the text to be the same as the """ ?

trunk/reviewboard/scmtools/hg.py (Diff revision 4)

The second if should be elsif.

trunk/reviewboard/scmtools/fixtures/hg.json (Diff revision 4)

This should probably be part of the default fixture.

trunk/reviewboard/scmtools/tests.py (Diff revision 4)
```
Two lines before the class.
```

trunk/reviewboard/scmtools/tests.py (Diff revision 4)

Can you fix the indentation of the parameter?

trunk/reviewboard/scmtools/tests.py (Diff revision 4)

Might be nice to have a utility function in the class that takes the diff contents and returns the result of parse()[0], so that we don't duplicate it everywhere.

trunk/reviewboard/scmtools/tests.py (Diff revision 4)

Please fix the indentation of this comment.

trunk/reviewboard/scmtools/tests.py (Diff revision 4)

A lot of these should be one-liners.

mrowe July 16, 2007, 4:44 p.m.

What's the project standard for max line length? Some of them would be 80-90 on one line, so I'll leave those ones wrapped for now.

chipx86 July 16, 2007, 6:15 p.m.

Roughly 80. Preferably before around 75, 76.

trunk/reviewboard/scmtools/hg.py (Diff revision 5)
```
You can remove this.
```

trunk/reviewboard/scmtools/tests.py (Diff revision 5)

Perhaps send us a tarball of the binary files for this?

MR

mrowe Aug. 6, 2007, 5:59 p.m.
```
Emailed to David.
```

trunk/reviewboard/scmtools/hg.py (Diff revision 5)

This will now need to be updated to work with the new DiffParser changes in SVN (r852). You can find the review request for this on the All Review Requests page for docs on how this now works.

MR

mrowe Aug. 5, 2007, 10:22 p.m.
```
New diff uploaded.
```

Ship it!

This looks good to me.  Is there anything more to do before this goes in?

mrowe Aug. 12, 2007, 1:07 p.m.

It's complete, as far as I can tell. Working fine here. :)

david Aug. 12, 2007, 1:13 p.m.

I've just committed the patch and test repo, and will close out the review request.  Thanks for your hard work on this!