[MERGE] loosen the constraints on WorkingTree.set_last_revision.

[John Arbash Meinel]

Robert Collins wrote:
> WorkingTree.set_last_revision currently requires the revision be in the
> branch's history. However, using uncommit one can easily have a tree
> whose branch does not contain the tree's last revision - it makes sense
> to me to allow constructing that scenario directly via the api.
> 
> This is useful to allow simplifying some of the corner cases in
> baz-import, and for dirstate.
> 
> Seeking a +1.
> 
> Rob

...

> === modified file bzrlib/workingtree.py
> --- bzrlib/workingtree.py
> +++ bzrlib/workingtree.py
> @@ -1224,13 +1224,11 @@
>          if new_revision is None:
>              self.branch.set_revision_history([])
>              return False
> -        # current format is locked in with the branch
> -        revision_history = self.branch.revision_history()
>          try:
> -            position = revision_history.index(new_revision)
> -        except ValueError:
> -            raise errors.NoSuchRevision(self.branch, new_revision)
> -        self.branch.set_revision_history(revision_history[:position + 1])
> +            self.branch.generate_revision_history(new_revision)
> +        except errors.NoSuchRevision:
> +            # not present in the repo - dont try to set it deeper than the tip
> +            self.branch.set_revision_history([new_revision])
>          return True

I believe this section is for format 2 working trees (all-in-one).
I like the first part (setting the last_revision outside the history
regenerates the ancestry)
I'm not so sure about the utility of being able to set the revision
history when you don't have a history.

This seems much more like an error, since you can't do any state
comparisons, etc.

So for me, +1 on 'generate_revision_history', -1 on swallowing
NoSuchRevision.


>  
>      def _cache_basis_inventory(self, new_revision):
> @@ -1259,7 +1257,7 @@
>              path = self._basis_inventory_name()
>              sio = StringIO(xml)
>              self._control_files.put(path, sio)
> -        except WeaveRevisionNotPresent:
> +        except (errors.NoSuchRevision, errors.RevisionNotPresent):
>              pass
>  
>      def read_basis_inventory(self):
> @@ -1527,10 +1525,6 @@
>                  pass
>              return False
>          else:
> -            try:
> -                self.branch.revision_history().index(revision_id)
> -            except ValueError:
> -                raise errors.NoSuchRevision(self.branch, revision_id)
>              self._control_files.put_utf8('last-revision', revision_id)
>              return True

Here I would rather check that we have the revision in the repository,
for the same reason as above.
I suppose it isn't critical, but I would at least want a good
explanation as to why we should allow the api to set a last revision
that we don't have access to. (Better to find someone else's bug by
raising an exception than to just make it easier to run a test case)

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060807/dd0a4b4a/attachment.pgp