People: |
|
|---|
fixing hg web client and git-formatted diff parser behavior
Review Request #1466 — Created March 8, 2010 and submitted
Initial attempts at posting patches for Mercurial repos with --git formatted patches resulted in massive fail. This patch attempts to do two things, in particular:
- support parsing of --git formatted patches in addition to "regular" Mercurial patches
- fix fetching of remote files with the HgWebClient (old behavior fetched html pages due to an extra '/' in url)
ran full test suite (manage.py test) extensive human-based testing with local ReviewBoard instance
-
-
-
reviewboard/scmtools/hg.py (Diff revision 1) Can you just reuse GitDiffParser? If anything has changed between them, we could make it easier to subclass. It's complex code, so sharing it would be better.
-
-
-
-
DB
-
BUMP again The company at which I work is moving ever-closer to using Mercurial as the primary VCS and I really don't want there to be a gap in ReviewBoard usage :-/ Pretty please review? :)
-
Sorry for the delays on this. As I mentioned in another review, I've been swamped, and haven't had time for reviews. Trying to get to some this weekend. Some small things I'd like to see and then I'm happy with it.
-
-
reviewboard/scmtools/git.py (Diff revision 2) And linenum instead of lineno for consistency with other files?
-
-
reviewboard/scmtools/git.py (Diff revision 2) For each of these, can you use () to wrap the lines instead? Like: return (self.lines[lineno].startswith(...) or self.lines[lineno].startswith(...)) Would also be good to pull the line out before the checking, so we don't have to get it from the array more than once per call. -
reviewboard/scmtools/git.py (Diff revision 2) Please document this function so it's more clear why we need it.
-
reviewboard/scmtools/hg.py (Diff revision 2) This is going to make the log files huge. Perhaps we can remove it and only log on failure.
-
reviewboard/scmtools/hg.py (Diff revision 2) Errors should be in sentence casing and should be prefixed with the class name. I'd rather we be more specific with this Exception and, instead of showing a full stack trace, just show the error message. It's pretty much always going to be an HTTP error. It's fine to have two excepts for this, with one being generic and logging the exception, but the other should be the specific one.
DB
Change Summary:
changes per feedback It's worth mentioning that running the test suite *did* result in failures and errors, although they were the *exact same* failures and errors reported when running the test suite for `origin/master`.
Diff: |
Revision 3 (+168 -85) |
|---|