[MERGE] using NULL_REVISION everywhere

[Aaron Bentley]

Robert Collins wrote:

> -        last_rev = self.last_revision()
> -        if last_rev is not None:
> +        last_rev = _mod_revision.ensure_null(self.last_revision())
> +        if not _mod_revision.is_null(last_rev):
> 
> isn't this better as
>          if not last_rev == NULL_REVISION
> given that you just cast it to NULL on the line above.

Sure.  I was expecting to pull out the ensure_null after the API break,
but I guess I can still do that.

> === modified file bzrlib/repository.py // last-changed:abentley at panoramicfeedba
> ... ck.com-20070710211854-93ofaa70day2p8d3
> --- bzrlib/repository.py
> +++ bzrlib/repository.py
> @@ -498,6 +498,9 @@
>          for revision in revisions:
>              required_trees.add(revision.revision_id)
>              required_trees.update(revision.parent_ids[:1])
> +        if ('null:' in required_trees):
> +            import pdb; pdb.set_trace()
> +
> 
> I don't think this hunk should be merged :).

ACK

> === modified file bzrlib/tests/__init__.py // last-changed:abentley at panoramicfe
> ... edback.com-20070711194451-3jqhye1nnd02a9uv
> --- bzrlib/tests/__init__.py
> +++ bzrlib/tests/__init__.py
> @@ -100,7 +100,7 @@
>  # Mark this python module as being part of the implementation
>  # of unittest: this gives us better tracebacks where the last
>  # shown frame is the test code, not our assertXYZ.
> -__unittest = 1
> +#__unittest = 1
>  
>  default_transport = LocalURLServer
>  
> Or this one. (BTW - why did you need to disable it ?)

ACK.  I needed to disable it because
TestCaseWithTransport.make_branch_and_tree was raising.

> For debugging I really prefer assertEqual over assertTrue in these places. e.g.
>          self.assertEqual(NULL_REVISION,
>              _mod_revision.ensure_null(master.last_revision())))

Okay.

> +class TestUncommit(TestCaseWithWorkingTree):
> +
> +    def test_uncommit_to_null(self):
> +        tree = self.make_branch_and_tree('branch')
> +        tree.lock_write()
> +        revid = tree.commit('a revision')
> +        tree.unlock()
> +        uncommit.uncommit(tree.branch, tree=tree)
> +        self.assertEqual([], tree.get_parent_ids())
> 
> This should go in bzrlib.tests.workingtree_implementations.test_uncommit IMO.

You mean create a whole new file for this test?

> === modified file bzrlib/workingtree.py // last-changed:abentley at panoramicfeedb
> ...
> @@ -502,8 +503,8 @@
> +        last_rev = _mod_revision.ensure_null(self._last_revision())
> +        if _mod_revision.is_null(last_rev):
> 
> wouldn't NULL_REVISION == last_rev be better here now that you have cast
> it to null? Otherwise why bother casting it?

Well, without casting, _mod_revision.is_null will issue a deprecation
warning.  But yeah, after casting, there's no reason.


> @@ -2717,7 +2723,7 @@
>              # only set an explicit root id if there is one to set.
>              if basis_tree.inventory.root is not None:
>                  wt.set_root_id(basis_tree.inventory.root.file_id)
> -            if revision_id == NULL_REVISION:
> +            if revision_id is None or revision_id == NULL_REVISION:
> 
> Wouldn't a call to 'is_null' be appropriate here, or use ensure_null ?

Actually, I don't think None is a possible value

Aaron