[MERGE] checkout
John A Meinel
john at arbash-meinel.com
Mon Feb 13 02:19:07 GMT 2006
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
More information about the bazaar
mailing list