[MERGE] change set-pending-merges to set last_revision if that is not set.

[Martin Pool]

On 17 Aug 2006, Robert Collins <robertc at robertcollins.net> wrote:
> Hi, this patch changes the tree behaviour so that if last_revision is
> None, set_pending_merges sets it - this gives us the more consistent
> behaviour between working trees and revision trees w.r.t. parents.
> 
> I have not put in a guard against making the last-revision a ghost -
> because the current API for doing that (set_last_revision) doesn't.
> However, I have agreed with Aaron that the new API I'm doing -
> set_parent_trees and set_parent_ids will have an explicit flag to say 'I
> want the left most parent to be allowed to be a ghost'.
> 
> That patch will be coming along shortly, this one is the more intrusive
> one - it changes the core behaviour, rather than changing the way we get
> at the behaviour which is what the new API will do.

+1 with some queries.

> === modified file 'bzrlib/tests/blackbox/test_status.py'
> --- bzrlib/tests/blackbox/test_status.py	2006-07-28 16:05:23 +0000
> +++ bzrlib/tests/blackbox/test_status.py	2006-08-17 01:50:02 +0000
> @@ -43,7 +43,7 @@
>      def test_branch_status(self):
>          """Test basic branch status"""
>          wt = self.make_branch_and_tree('.')
> -        b = wt.branch
> +        wt.commit('create a parent to allow testing merge output')
>  
>          ignores._set_user_ignores(['./.bazaar'])

This seems to change the meaning of the immediately following
assertions, which say they are checking 'status' in a newly initted
branch.  This test should be split: one to check status in a virgin
branch; another to do the merge (where this change should be applied.)
(Unless there's already one like that but I don't see it in a quick
look.)

> === modified file 'bzrlib/workingtree.py'
> --- bzrlib/workingtree.py	2006-08-15 03:18:20 +0000
> +++ bzrlib/workingtree.py	2006-08-17 02:38:23 +0000
> @@ -407,7 +407,16 @@
>                  return bzrlib.tree.RevisionTree(self.branch.repository, inv,
>                                                  revision_id)
>          # FIXME? RBC 20060403 should we cache the inventory here ?
> -        return self.branch.repository.revision_tree(revision_id)
> +        try:
> +            return self.branch.repository.revision_tree(revision_id)
> +        except errors.RevisionNotPresent:
> +            # the basis tree *may* be a ghost or a low level error may have
> +            # occured. If the revision is present, its a problem, if its not
> +            # its a ghost.
> +            if self.branch.repository.has_revision(revision_id):
> +                raise
> +            # the basis tree is a ghost
> +            return self.branch.repository.revision_tree(None)

The effect is that basis_tree will now return EmptyTree if the primary
parent is a ghost.  That's OK but the docstring must be
updated to say this.   And it would be nice if the final comment said

  # the basis tree is a ghost, so return EmptyTree

because it's slightly nonobvious this is what passing None gets you.
(Uh, possibly that means we should change that api to be more obvious?
repo.empty_tree() or some such?)

> @@ -1403,9 +1420,6 @@
>          Do a 'normal' merge of the old branch basis if it is relevant.
>          """
>          old_tip = self.branch.update()
> -        if old_tip is not None:
> -            self.add_pending_merge(old_tip)
> -        self.branch.lock_read()
>          try:
>              result = 0

> @@ -1437,7 +1451,8 @@
>                                        this_tree=self)
>              return result
>          finally:
> -            self.branch.unlock()
> +            if old_tip is not None:
> +                self.add_pending_merge(old_tip)
>  
>      @needs_write_lock
>      def _write_inventory(self, inv):
> 

Why get rid of the branch read lock?

I really don't understand why this is in a finally block?

-- 
Martin