[MERGE] checkout

[John A Meinel]

Robert Collins wrote:
> On Sun, 2006-02-12 at 18:19 -0600, John A Meinel wrote:
>> Robert Collins wrote:
>>> On Sun, 2006-02-12 at 17:18 -0600, John A Meinel wrote:
>>>> My mail client sucks, so I lost the text of your message.
>>>> But I checked out your branch. And it seems to depend on your BzrDir
>>>> work. So can we wait for that to land before I evaluate checkouts?
>>>> Otherwise it is difficult to find the checkout changes.
>>> It does indeed depend on that work.... I'm attaching a diff of the
>>> checkout work - if you are happy with that I'll merge it into the
>>> bzr-dir branch, and they can come in together.
>>>
>> Reviewing... (Do you want to merge in the BoundBranch work as well? The
>> only thing it was waiting on was a new branch format. I can add it once
>> BzrDir lands, though)
> 
> I have that on my todo plate right after versioned file - checkouts was
> a quick hack in the weekend to give me a break from the deep stuff I'd
> been doing.
> 
> My queue is currently:
>  * repository support to finish the bzrdir api
>  * upgrades properly integrated into bzrdir
>  * versioned file for knits.
>  * bound branches.
>  * encodings
>  * take a breath and see where we are at.
> 
> Any help folk want to do in that queue is much appreciated :).
> 
> The order is like it is because versionedfile and bound branches both
> need the fully robust format upgrade support in place, and of version
> file and bound branches, I expect versioned file to be a more traumatic
> change, so I want it in asap.
> 

I agree that versioned file is the more traumatic change. But I'm
definitely looking forward to it.

...

> 
> I don't think update should take a remote path to operate on, because
> its defined as moving forward along the line of development that *this*
> branch represents. To move onto a different line of development people
> should use 'pull'.

I agree, I'm just not sure if it should take a local path either. But
I'm okay if it does.

...
>> Shouldn't this also check that 'file' exists in checkout2?
> 
> No - thats unit tested by the working tree implementation tests. The CLI
> tests are using whatever the current default working tree environment
> supports - and there could be multiple implementors of this, so we unit
> test the merge facilities are working correctly in the
> workingtree_implementations tests.

...

> 
> Yes, cruft as I developed. There should be no failUnlessExists line
> here. Removing it.
> 

As long as it is getting tested.

> 
>>> === modified file 'bzrlib/builtins.py'
>>> --- bzrlib/builtins.py	
>>> +++ bzrlib/builtins.py	
>>> @@ -17,10 +17,13 @@
>>>  """builtin bzr commands"""
>>>  
>>>  
>>> +import errno
>>>  import os
>>> +from shutil import rmtree
>>>  import sys
>>>  
>>>  import bzrlib
>>> +import bzrlib.branch as branch
>> This seems very dangerous, since we frequently name our variables "branch".
> 
> Hmm. 'branch' is a terrible variable name. But I can change it if you
> wish.
> 

I can debate its utility as a variable, but I think it is a very good
parameter name.

def do_something(branch):

Often you just want a branch that you are doing stuff to. I don't think
you need to use a parameter name other than 'branch'.

> 
>>> +                raise
>>> +        checkout = bzrdir.BzrDirMetaFormat1().initialize(to_location)
>>> +        branch.BranchReferenceFormat().initialize(checkout, source)
>>> +        checkout.create_workingtree(revision_id)
>>>  
>> I think this would be clearer if we just used
>> bzrlib.branch.BranchReference...
> 
> do you mean 'bzrlib.branch.BranchReferenceFormat().initialize(checkout,
> source)' ?
> 

Well, assuming that 'branch' at this point was your module, and not a
variable, yes.

> 

...

> 
> If you want a specific type, the shorthands exist so that you dont need
> to do:
> dir = bzrdir.BzrDir.open_containing(path)[0]
> tree = dir.open_workingtree()
> all over the place.
> 
> This command can only operate where there is a working tree, so there is
> no need for the use of the bzrdir facility in it. Compare with 'pull'
> and 'log' which can work when there is only a branch.
> 

So does that mean that you have caved on the meaning of
"Branch.open_containing"? Since it should have the same effect as
"BzrDir.open_containing(path)[0].open_branch()"?

Originally you were arguing that if we only have a checkout, it should
not open the associated branch. Or is Branch.open_containing() going to
have a subtle yet drastic difference from
BzrDir.open_containing().open_branch()?


...

>> Shouldn't clone & sprout take a last_revision parameter, rather than
>> sprouting and then setting the revision?
> 
> They do take such a parameter. Now consider a bzrdir (with the meta-dir
> format) with a branch and a repository but no working tree. (say its
> been deleted for some reason). Now, you can do 'bzr checkout -r -3 .'
> sanely in that directory. create_workingtree() thus takes a revision_id.
> 

I'm fine with create_workingtree() taking revision_id. I'm wondering why
it does set_last_revision here, rather than doing it deeper.

(For example, right here you have no checking to make sure that the
revision you are asking for actually exists in the branch).

>> ...
>>
>>> +
>>> +
>>> +class OutOfDateTree(BzrNewError):
>>> +    """Working tree is out of date, please run 'bzr update'."""
>> This should probably take a path defining what is out of date. (It may
>> not print it, but it should have it internally for people catching this
>> error).
> 
> Ok.
> 
>   
>>>  import bzrlib
>>> +import bzrlib.branch as branch
>>>  from bzrlib.branch import Branch
>>>  import bzrlib.bzrdir as bzrdir
>>>  from bzrlib.bzrdir import BzrDir
>> Again, I would strongly discourange "import bzrlib.branch as branch"
> 
> Waiting for your feedback above on this.
> 
>>> +    def test_update_sets_last_revision(self):
>>> +        # working tree formats from the meta-dir format and newer support
>>> +        # setting the last revision on a tree independently of that on the 
>>> +        # branch. Its concievable that some future formats may want to 
>>> +        # couple them again (i.e. because its really a smart server and
>>> +        # the working tree will always match the branch). So we test
>>> +        # that formats where initialising a branch does not initialise a 
>>> +        # tree - and thus have separable entities - support skewing the 
>>> +        # two things.
>> Since we disabled using the doc string instead of the test path when
>> using --verbose, we can use docstrings instead of comments. (There are
>> other tests you added which can be changed as well)
> 
> Other test runners such as the unittestgui will still show the
> docstrings. I think its cleaner to use comments when we are describing
> intention here.

Okay.

> 
>>> +            warn("WorkingTree() is deprecated ass of bzr version 0.8. "
>>> +                 "Please use bzrdir.open_workingtree or WorkingTree.open().",
>>> +                 DeprecationWarning,
>>> +                 stacklevel=2)
>> Is this a Freudian slip? I think you want *as*.
> 
> :) Fixing.
> 
...


>>> -        if branch is None:
>>> -            branch = Branch.open(basedir)
>>> -        assert isinstance(branch, Branch), \
>>> -            "branch %r is not a Branch" % branch
>>> -        self.branch = branch
>>> +        if deprecated_passed(branch):
>>> +            if not _internal:
>>> +                warn("WorkingTree(..., branch=XXX) is deprecated as of bzr 0.8."
>>> +                     " Please use bzrdir.open_workingtree().",
>>> +                     DeprecationWarning,
>>> +                     stacklevel=2
>>> +                     )
>>> +            self.branch = branch
>>> +        else:
>>> +            self.branch = self.bzrdir.open_branch()
>>> +        assert isinstance(self.branch, Branch), \
>>> +            "branch %r is not a Branch" % self.branch
>> This error supports my earlier comment that you should use BzrDir
>> instead of WorkingTree.open()
> 
> Errr. This is still quite unclean but does not affect the API.
> WorkingTree.open() is defined as
>     @staticmethod
>     def open(path=None, _unsupported=False):
>         """Open an existing working tree at path.
> 
>         """
>         if path is None:
>             path = os.path.getcwdu()
>         control = bzrdir.BzrDir.open(path, _unsupported)
>         return control.open_workingtree(_unsupported)
> 
> So I'm really not sure how that supports your comment. The code you are
> critiquing there is backwards compatability for 0.7 api users, and in
> fact that assert there exists in 0.7.
> 

Actually, I was talking about the warning. Not the assertion. (It was
just the end of the patch).

> 
>>> +    def _remove_old_basis(self, old_revision):
>>> +        """Remove the old basis inventory 'old_revision'."""
>> Can we merge my basis-inventory changes so that we don't have this
>> _basis_inventory_name stuff?
>>
>> (They were also waiting on a BranchFormat which would delete
>> ancestry.weave and any basis-inventory.* files).
> 
> Yes please, I'll put them in my todo queue or you can prep them for the
> new world order and I'll review. Where are they ?
> 

I don't have a problem resubmitting them for review after BzrDir lands.

But you can find some of the code:
http://bzr.arbash-meinel.com/branches/bzr/inventory-w-revision-id/

There is also some "upgrade deletes basis-inventory" code in:
http://bzr.arbash-meinel.com/branches/bzr/format-7/

But honestly upgrade deleting basis-inventory is pretty trivial, and
would need a lot of cleanup now that we have branch-formats, and you
redo upgrade.

> 
>> st-revision', revision_id)
>>> +            return True
>>> +
>> It would be nice to at least mark with a comment when WorkingTree2
>> format will be expired. (It also will help us remember to remove it when
>> the time comes).
> 
> Well IIRC the previously agreed policy was no sooner than two months
> after a release that superceeds the format, so May at the earliest. And
> if we want to support upgrading from it, then no specific timeframe
> should exist.
> 

Well what about the former. # This will be supported until at least May.
Otherwise come September we might be trying to backtrack and figure out
when this was introduced/deprecated that we can remove it.


>> ...
>>
>> So, I'll give you a +1 to merge this into BzrDir as long as you address
>> my comments. (Though please wait to do so. So we don't have to review it
>> again to allow BzrDir to land).
> 
> Thanks. There are a couple of disagreements above, so lets resolve
> those :)
> 
> Rob
> 

Sure. I certainly want to work through this.

John
=:->



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