[PATCH][reminder] implementation and tests for update -r

[John Arbash Meinel]

Matthieu Moy wrote:
> Hi,
> 
> Just sending again my "update -r" patch (with the latest bzr.dev stuff
> merged in and conflicts resolved). Up to now, I think no one commented
> on it.

...

> === modified file bzrlib/builtins.py
> --- bzrlib/builtins.py
> +++ bzrlib/builtins.py
> @@ -817,23 +817,43 @@
>      'bzr revert' instead of 'bzr commit' after the update.
>      """
>      takes_args = ['dir?']
> +    takes_options = ['revision']
>      aliases = ['up']
>  
> -    def run(self, dir='.'):
> +    def run(self, dir='.', revision=None):
> +        if revision is not None and len(revision) != 1:
> +            raise BzrCommandError("bzr update --revision takes exactly one revision")
>          tree = WorkingTree.open_containing(dir)[0]
> +        b = tree.branch

^- we are trying to avoid variable names shorter than 3 characters. 'b'
exists in the code already, but we probably would prefer to avoid adding
new uses of it.

>          tree.lock_write()
>          try:
>              existing_pending_merges = tree.pending_merges()
> -            last_rev = tree.last_revision()
> -            if last_rev == tree.branch.last_revision():
> -                # may be up to date, check master too.
> -                master = tree.branch.get_master_branch()
> -                if master is None or last_rev == master.last_revision():
> -                    revno = tree.branch.revision_id_to_revno(last_rev)
> -                    note("Tree is up to date at revision %d." % (revno,))
> -                    return 0
> -            conflicts = tree.update()
> -            revno = tree.branch.revision_id_to_revno(tree.last_revision())
> +            # potentially get new revisions from the master branch.
> +            # needed for the case where -r N is given, with N not yet
> +            # in the local branch for a heavyweight checkout.
> +            if revision is not None:
> +                try:
> +                    rev = revision[0].in_history(b).rev_id
> +                    # no need to run branch.update()
> +                    old_tip=None

^- you should use 'old_tip = None'

> +                except errors.NoSuchRevision:
> +                    # revision was not there, but is maybe in the master.
> +                    old_tip = b.update()
> +                    rev = revision[0].in_history(b).rev_id

^- do you have any tests for the case where '--revision' is supplied,
and it doesn't exist in either the branch or the master? Also, for some
commands, the revision only has to exist in the repository. Do you want
that behavior here? (I believe right now in_history() actually looks
in_repository(), so you might consider it broken) But it seems like the
above code might let you do:
bzr update -r revid:otherproject

And get this tree to change drastically.

> +            else:
> +                old_tip = b.update()
> +                rev = b.last_revision()
> +            if tree.last_revision() == rev:
> +                revno = b.revision_id_to_revno(rev)
> +                note("Tree is up to date at revision %d." % (revno,))
> +                return 0

v- And you need tree.update(rev, old_tip) here.

> +            try:
> +                conflicts = tree.update(rev,old_tip)
> +            except errors.NoSuchRevision, e:
> +                raise BzrCommandError("branch has no revision %s\n"
> +                                      "bzr update --revision works only for a revision in the branch history"
> +                                      % (e.revision))
> +            revno = b.revision_id_to_revno(tree.last_revision())
>              note('Updated to revision %d.' % (revno,))
>              if tree.pending_merges() != existing_pending_merges:
>                  note('Your local commits will now show as pending merges with '
> 

...

> === modified file bzrlib/workingtree.py
> --- bzrlib/workingtree.py
> +++ bzrlib/workingtree.py
> @@ -1383,7 +1383,7 @@
>          raise NotImplementedError(self.unlock)
>  
>      @needs_write_lock
> -    def update(self):
> +    def update(self,revision=None,old_tip=0):

^- you need more spaces inbetween your variables:
def update(self, revision=None, old_tip=0):

But I'm a little frightened that you are passing '0' as the default
value. I think you are looking for a signal that nothing has been
passed. But for that I would use:

_marker = object()

def update(self, revision=None, old_tip=_marker):
   ...

   if old_tip is _marker:
     # Maybe this should be deprecated??
     old_tip = self.branch.update()
   ...

Using 0 means that calling code can pass it if they want. But I really
don't like to see it here. Better to use a semi-anonymous marker to
indicate that nothing was passed.

Also, it is odd that you use 'None' as the marker for 'revision' but not
as the marker for 'old_tip'. (I understand *why* you need to do that,
but I think it indicates a problem).


>          """Update a working tree along its branch.
>  
>          This will update the branch if its bound too, which means we have multiple trees involved:
> @@ -1401,22 +1401,32 @@
>          There isn't a single operation at the moment to do that, so we:
>          Merge current state -> basis tree of the master w.r.t. the old tree basis.
>          Do a 'normal' merge of the old branch basis if it is relevant.
> +
> +        :param revision: The target revision to update to. Must be in the revision history.
> +        :param old_tip: If branch.update() has already been run, the value it returned 
> +        (old tip of the branch or nil). Otherwise, the number 0.
>          """

Otherwise things seem okay. Though I think I would prefer whitebox tests
directly on the WorkingTree.update() call, rather than assuming
everything is correct from the blackbox tests.

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/20060901/b1b5fe12/attachment.pgp