[MERGE] RevisionSpec.in_history() should raise nicer errors

[Robert Collins]

On Wed, 2006-08-23 at 15:22 -0500, John Arbash Meinel wrote:
> Currently with bzr, if you do 'bzr log -r10000' you get a NoSuchRevision
> exception, which is an internal exception, meant for indicating you are
> missing something from your repository.
> 
> Also, it turned out that while we tested the very basic revision spec
> functionality, we didn't do much error or edge case testing.
> 
> The attached patch adds a new error: InvalidRevisionSpec, which gets
> formatted into a much nicer user error.
> 
> It also adds lots of edge-case testing to all of the current revisions
> specs.
> 
> There are a few actual functional changes that probably should be discussed:
> 
> 1) '-rbefore:revid:X' used to return revision X if it was not in the
> local branches revision history. This has been fixed to return the
> left-hand parent of X.
> 
> 2) '-rbefore:0' used to return the Null revision. This has been changed
> to raise an error. Since nothing is before the null revision.
> 
> 3) I changed '-rbranch:branch/sub/path', '-rancestor:branch/sub/path',
> and '-rrevno:10:branch/sub/path' to be errors, rather than using the
> branch at 'url'. This is probably a little controversial, but it is easy
> to change back. We never asserted the old behavior, and my feeling is
> that in cases like this, to help prevent accidentally going to the wrong
> branch, it is better to require the exact path to a branch.
> 
> 4) I updated the '-rrevno:N' parser, so that it handles 'N' just like
> '-rN'. Meaning you can supply negative numbers, etc.
> 
> 5) Splits up the revspec tests into a lot more small tests, and
> explicitly tests each currently implemented revision spec, including
> testing edge cases and exceptions.
> 
> 6) The very last patch changes 'bzr log -r-10' so that if there are only
> 8 revisions, it returns the 1st revision, rather than raising an error.
> This makes the alias 'log=log -r-10..-1' really nice, since it gives you
> summaries, without giving as many exceptions all the time.
> 
> 
> Is this 0.10 worthy? From the amount of effort possibly not. From the
> user-visible and code-visible changes, perhaps. Changing
> Branch.open_containing => Branch.open() may be sufficient. Though I can
> revert that if it means we get it into 0.10.

Hi, I think this is appropriate for 0.11 and not 0.10. Its a step in the
right direction, but there is more we can do - and doing that in 0.10
would be way to much, as it is its already borderline IMO.

So I'd rather say 'lets get this really right for 0.11 and not change
for 0.10'.




+def _get_int_revno_helper(branch, revs, revno, spec):
+    """Just a helper for the common functionality between int and revno
specs.
+
+    :param branch: The branch to lookup the revno in

...

This should really be a class method - its all about the request being
looked up. Its not one now because there are two unrelated classes - one
for 'int' and one for 'revno:int'. I think we should just consolidate
these, which will require some rearrangement of the parser, but be well
worth it.


I think the new exception should not be a subclass of NoSuchRevision -
lets do it right here, and we'll wear the bugs from the first day 0.11
is open :).

-Rob


-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060824/c65c19ae/attachment.pgp