Make post-review warn if installed version of git-svn is too old

Review Request #343 — Created April 4, 2008 and submitted


Review Board SVN (deprecated)


I installed git-svn the other day with an out-of-date MacPorts index, and wound up with version installed.  It turns out that 'git svn info', which is used by post-review to find repository information, wasn't added until git 1.5.4 (see ).  This adds a version check for git-svn which is run if git svn info fails, and which lets the user know that the git-svn version is out of date.
Tested against my old install and my new, hotter install.
  1. Thanks for catching this! A few comments.
  2. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)
    Blank line here isn't needed.
    1. Heh, okay, I was trying too hard to abide by this:  I'll be less vehement.
  3. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)
    This check will fail for such versions as 2.0.0 or 1.6.3. We'd need to be more specific in our checks. Since it's going to be a bit longer, I'd recommend creating a utility function that checks if a version is at least the version passed. For example:
    def check_version(version, expected):
        return (expected[0] > version[0]) or \
                (expected[0] == version[0] and expected[1] > version[1]) or \
                (expected[0] == version[0] and expected[1] == version[1] and \
                 expected[3] >= version[3]))
    version and expected should be tuples of (major, minor, micro). It would be good to document that in the function.
    Then your check would become:
    if not check_version((,
                         (1, 5, 4)):
    1. Man, what a dumb mistake on my part.  (Yay code review.)  Added.  I corrected (I think) some errors in your sketch above, but worth checking it over again just to make sure.
  4. I'd rather not have the "post-review failed: " prefix here. If we want a prefix to identify failure, we should put it in die(). Maybe just prefix "fatal: "
    1. I just knocked it back to an error sentence.
  2. Please note that not all git releases have a four-part version number.  For instance, my Cygwin installation has plain-old 1.5.4.  You're not using the fourth part anyway, so why not leave that out of the expression?
    1. Good point.  Fixed.
  2. /trunk/reviewboard/contrib/tools/post-review (Diff revision 2)
    These need to be >= and not >. Otherwise, if you call:
    is_valid_version((1, 2, 3), (1, 2, 3))
    It will return False, since 2 is not > 2 and 3 is not > 3.
    1. I believe that only the last > should be a >=.  Is that right?
  1. Looks good.  Committed as r1299.  Thanks!