fixing hg web client and git-formatted diff parser behavior

Review Request #1466 — Created March 8, 2010 and submitted


Review Board


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 ( test)
extensive human-based testing with local ReviewBoard instance
  2. reviewboard/scmtools/ (Diff revision 1)
    Should be inserted in alphabetical order.
  3. reviewboard/scmtools/ (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.
  4. reviewboard/scmtools/ (Diff revision 1)
    Should be FULL_FILE_URL.
  5. reviewboard/scmtools/ (Diff revision 1)
    Sentence casing.
  6. reviewboard/scmtools/ (Diff revision 1)
    Blank line between these.
  7. reviewboard/scmtools/ (Diff revision 1)
    Please use { instead of dict(
  1. BUMP
    still looking for feedback to latest patch :)
  1. 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? :)
  1. I can't see any issue with it, but I'm not a committer.
    1. the review is appreciated nonetheless :)
  1. 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.
  2. reviewboard/scmtools/ (Diff revision 2)
    Small nit, but can we call this file_info instead?
  3. reviewboard/scmtools/ (Diff revision 2)
    And linenum instead of lineno for consistency with other files?
  4. reviewboard/scmtools/ (Diff revision 2)
    Blank line around each if block.
  5. reviewboard/scmtools/ (Diff revision 2)
    For each of these, can you use () to wrap the lines instead? Like:
    return (self.lines[lineno].startswith(...) or
    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.
  6. reviewboard/scmtools/ (Diff revision 2)
    Please document this function so it's more clear why we need it.
  7. reviewboard/scmtools/ (Diff revision 2)
    This is going to make the log files huge. Perhaps we can remove it and only log on failure.
  8. reviewboard/scmtools/ (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.
Review request changed

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`.


Revision 3 (+168 -85)

Show changes

  1. BUMP
    seriously guys...  it's been over 3 months
    1. You're right, sorry. We've been juggling a lot of things both inside and outside of the project these past few months and tracking patches has slipped a bit.
  1. Looks good. Committed to master (30d5902)
    1. HOTTT
      thanks chipx86!