[rfc][patch]

[John A Meinel]

Erik Bågfors wrote:
> I've implemented "bzr pull --revision X". Well, most of the
> functionality was already there, I just exposed it in the ui.
> 
> I am not sure about the code, am not used to python and haven't done
> anything inside bzr almost. So feel free to call me an idiot in the
> way I wrote the code.  Comments are welcome.
> 
> I also added a test case but I can't get the standard test suite to
> pass so I can't even get it to my code. So the test is totally
> untested :)
> 
> Get it from http://erik.bagfors.nu/bzr.pull-revision
> 
> Merry Christmas all.
> 
> Regards,
> Erik
> 

I'm pasting a copy of the patch, and I'll make some comments alongside.


> 
> === modified file 'bzrlib/branch.py'
> --- bzrlib/branch.py	
> +++ bzrlib/branch.py	
> @@ -412,7 +412,7 @@
>          """Return a `Tree` for the working copy if this is a local branch."""
>          raise NotImplementedError('working_tree is abstract')
>  
> -    def pull(self, source, overwrite=False):
> +    def pull(self, source, overwrite=False, stop_revision=None):
>          raise NotImplementedError('pull is abstract')
>  
>      def basis_tree(self):
> @@ -969,6 +969,7 @@
>      def update_revisions(self, other, stop_revision=None):
>          """See Branch.update_revisions."""
>          from bzrlib.fetch import greedy_fetch
> +
>          if stop_revision is None:
>              stop_revision = other.last_revision()
>          ### Should this be checking is_ancestor instead of revision_history?
> @@ -1026,13 +1027,13 @@
>          return WorkingTree(self.base, branch=self)
>  
>      @needs_write_lock
> -    def pull(self, source, overwrite=False):
> +    def pull(self, source, overwrite=False, stop_revision=None):
>          """See Branch.pull."""
>          source.lock_read()
>          try:
>              old_count = len(self.revision_history())
>              try:
> -                self.update_revisions(source)
> +                self.update_revisions(source,stop_revision)
>              except DivergedBranches:
>                  if not overwrite:
>                      raise
> 

So far, everything seems fine.


> === modified file 'bzrlib/builtins.py'
> --- bzrlib/builtins.py	
> +++ bzrlib/builtins.py	
> @@ -395,10 +395,10 @@
>      If you want to forget your local changes and just update your branch to
>      match the remote one, use --overwrite.
>      """
> -    takes_options = ['remember', 'overwrite', 'verbose']
> +    takes_options = ['remember', 'overwrite', 'revision', 'verbose']
>      takes_args = ['location?']
>  
> -    def run(self, location=None, remember=False, overwrite=False, verbose=False):
> +    def run(self, location=None, remember=False, overwrite=False, revision=None, verbose=False):
>          from bzrlib.merge import merge
>          from shutil import rmtree
>          import errno
> @@ -415,8 +415,13 @@
>          br_from = Branch.open(location)
>          br_to = tree_to.branch
>  
> +        if len(revision) == 1:
> +	    rev_id = revision[0]._match_on(br_from,br_from.revision_history()).rev_id
> +	elif len(revision) > 1:
> +	    raise BzrCommandError('bzr pull --revision takes one value.')
> +

At this point, you have to realize that 'revision' may be a list 
(because --revision was supplied), or it may be None (no '--revision' 
was supplied).

So you really need something more like:

if revision is None:
   rev_id = None
elif len(revision) == 1:
   rev_id = revision[0]._match_on(...
else:
   raise BzrCommandError('...

>          old_rh = br_to.revision_history()
> -        count = tree_to.pull(br_from, overwrite)
> +        count = tree_to.pull(br_from, overwrite, revision[0]._match_on(br_from,br_from.revision_history()).rev_id)

And then here, you don't want to get the match again, you just want to say:

count = tree.pull(br_from, overwrite, stop_revision=rev_id)

>  
>          if br_to.get_parent() is None or remember:
>              br_to.set_parent(location)
> 
> === modified file 'bzrlib/tests/blackbox/test_pull.py'
> --- bzrlib/tests/blackbox/test_pull.py	
> +++ bzrlib/tests/blackbox/test_pull.py	
> @@ -91,6 +91,33 @@
>          self.runbzr('pull ../b')
>          self.runbzr('pull ../b')
>  
> +    def test_pull_revision(self):
> +        """Pull some changes from one branch to another."""
> +        os.mkdir('a')
> +        os.chdir('a')
> +
> +        self.example_branch()
> +        file('hello2', 'wt').write('foo')
> +        self.runbzr('add hello2')
> +        self.runbzr('commit -m setup hello2')
> +        file('goodbye2', 'wt').write('baz')
> +        self.runbzr('add goodbye2')
> +        self.runbzr('commit -m setup goodbye2')
> +
> +        os.chdir('..')
> +        self.runbzr('branch -r 1 a b')
> +        os.chdir('b')
> +        self.runbzr('pull -r 2')
> +        a = Branch.open('../a')
> +        b = Branch.open('.')
> +	self.assertEquals(a.revno(),4)
> +	self.assertEquals(b.revno(),2)
> +        self.runbzr('pull -r 3')
> +	self.assertEquals(b.revno(),3)
> +        self.runbzr('pull -r 4')
> +        self.assertEquals(a.revision_history(), b.revision_history())
> +
> +

The test looks fine.

>      def test_overwrite_uptodate(self):
>          # Make sure pull --overwrite overwrites
>          # even if the target branch has merged
> 
> === modified file 'bzrlib/workingtree.py'
> --- bzrlib/workingtree.py	
> +++ bzrlib/workingtree.py	
> @@ -658,12 +658,12 @@
>                  yield stem
>  
>      @needs_write_lock
> -    def pull(self, source, overwrite=False):
> +    def pull(self, source, overwrite=False, stop_revision=None):
>          from bzrlib.merge import merge_inner
>          source.lock_read()
>          try:
>              old_revision_history = self.branch.revision_history()
> -            count = self.branch.pull(source, overwrite)
> +            count = self.branch.pull(source, overwrite,stop_revision)
>              new_revision_history = self.branch.revision_history()
>              if new_revision_history != old_revision_history:
>                  if len(old_revision_history):
> 

This looks fine as well.

John
=:->