Thanks for catching this! A few comments.

/trunk/reviewboard/contrib/tools/post-review (Diff revision 1)
		846	else:
		847
		848	# Versions of git-svn before 1.5.4 don't (appear to) support

Blank line here isn't needed.

1. Marc Hedlund 7 months, 2 weeks ago (April 8th, 2008, 9:06 p.m.)

   Heh, okay, I was trying too hard to abide by this: http://reviews.review-board.org/r/334/#comment763  I'll be less vehement.

/trunk/reviewboard/contrib/tools/post-review (Diff revision 1)
		856	if int(version_parts.group(1)) < 1 or \
		857	int(version_parts.group(2)) < 5 or \
		858	int(version_parts.group(3)) < 4:

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((version_parts.group(1),
                      version_parts.group(2),
                      version_parts.group(2)),
                     (1, 5, 4)):
    die(...)

1. Marc Hedlund 7 months, 2 weeks ago (April 8th, 2008, 9:08 p.m.)

   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.

/trunk/reviewboard/contrib/tools/post-review (Diff revision 1)
		859	die("post-review failed: git-svn must be " + \
		860	"version 1.5.4 or later")

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. Marc Hedlund 7 months, 2 weeks ago (April 8th, 2008, 9:08 p.m.)

   I just knocked it back to an error sentence.

/trunk/reviewboard/contrib/tools/post-review (Diff revision 1)
		853	version_parts = re.search('version (\d+)\.(\d+)\.(\d+)\.(\d+)',

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. Marc Hedlund 7 months, 2 weeks ago (April 8th, 2008, 9:09 p.m.)

   Good point.  Fixed.

/trunk/reviewboard/contrib/tools/post-review (Diff revision 2)
		875	return (expected[0] > actual[0]) or \
		876	(expected[0] == actual[0] and expected[1] > actual[1]) or \
		877	(expected[0] == actual[0] and expected[1] == actual[1] and \
		878	expected[2] > actual[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. Marc Hedlund 7 months ago (April 18th, 2008, 6:30 p.m.)

   I believe that only the last > should be a >=.  Is that right?
   
   Thanks.

Looks good.  Committed as r1299.  Thanks!