[RFC PATCH] Should RevisionSpec_last check for revno < 0 ?

[Michael Ellerman]

Hiya,

Currently RevisionSpec_last (which implements -r last:x) will happily return
you a revno which is < 0. Is there any reason why it shouldn't check?

I'm also not sure if the RevisionSpec classes should be returning
RevisionInfo(None) when they can't parse a rev spec. I think an exception
throw would be more appropriate. Hmm.

Here's the patch I've got for RevisionSpec_last anyway.

cheers

=== modified file 'bzrlib/revisionspec.py'
--- bzrlib/revisionspec.py
+++ bzrlib/revisionspec.py
@@ -17,7 +17,8 @@
 
 import datetime
 import re
-from bzrlib.errors import BzrError, NoSuchRevision, NoCommits
+from bzrlib.errors import (BzrError, NoSuchRevision, NoCommits,
+        InvalidRevisionNumber)
 
 _marker = []
 
@@ -218,10 +219,17 @@
             offset = int(self.spec)
         except ValueError:
             return RevisionInfo(branch, None)
-        else:
-            if offset <= 0:
-                raise BzrError('You must supply a positive value for --revision last:XXX')
-            return RevisionInfo(branch, len(revs) - offset + 1)
+
+        if offset <= 0:
+            raise BzrError('You must supply a positive value for " \
+                    "--revision last:XXX')
+
+        revno = len(revs) - offset + 1
+
+        if revno <= 0:
+            raise InvalidRevisionNumber(revno)
+
+        return RevisionInfo(branch, revno)
 
 SPEC_TYPES.append(RevisionSpec_last)
 

-- 
Michael Ellerman
IBM OzLabs

email: michael:ellerman.id.au
inmsg: mpe:jabber.org
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person