[MERGE] checkout
Robert Collins
robertc at robertcollins.net
Mon Feb 13 00:50:30 GMT 2006
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.
> ...
>
>
> ...
>
> > + def test_update_up_to_date_checkout(self):
> > + self.make_branch_and_tree('branch')
> > + self.runbzr('checkout branch checkout')
> > + out, err = self.runbzr('update branch')
> > + self.assertEqual('Tree is up to date.\n', err)
> > + self.assertEqual('', out)
>
> Don't you need to 'chdir("checkout")' here?
> What is your concept for update. Does it take a local path to update, or
> a remote path to update from. (Obviously you have it written as a local
> path to update).
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'.
> > +
> > + def test_update_out_of_date_standalone_tree(self):
> > + # FIXME the default format has to change for this to pass
> > + # because it currently uses the branch last-revision marker.
> > + raise TestSkipped('default format too old')
> > + self.make_branch_and_tree('branch')
> > + # make a checkout
> > + self.runbzr('checkout branch checkout')
> > + self.build_tree(['checkout/file'])
> > + self.runbzr('add checkout/file')
> > + self.runbzr('commit -m add-file checkout')
> > + # now branch should be out of date
> > + out,err = self.runbzr('update branch')
> > + self.assertEqual('Updated to revision 1.\n', out)
> > + self.assertEqual('', err)
> > + self.failUnlessExists('branch/file')
>
> I thought your BzrDir code implemented last_revision for working trees.
It implemented the split out of the working tree, and it implemented
branch references. But it did not implement the last revision property
on working trees themselves.
> > +
> > + def test_update_out_of_date_checkout(self):
> > + self.make_branch_and_tree('branch')
> > + # make two checkouts
> > + self.runbzr('checkout branch checkout')
> > + self.runbzr('checkout branch checkout2')
> > + self.build_tree(['checkout/file'])
> > + self.runbzr('add checkout/file')
> > + self.runbzr('commit -m add-file checkout')
> > + # now checkout2 should be out of date
> > + out,err = self.runbzr('update checkout2')
> > + self.assertEqual('All changes applied successfully.\n'
> > + 'Updated to revision 1.\n',
> > + err)
> > + self.assertEqual('', out)
>
> 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.
> > +
> > + def test_update_conflicts_returns_2(self):
> > + self.make_branch_and_tree('branch')
> > + # make two checkouts
> > + self.runbzr('checkout branch checkout')
> > + self.build_tree(['checkout/file'])
> > + self.runbzr('add checkout/file')
> > + self.runbzr('commit -m add-file checkout')
> > + self.runbzr('checkout branch checkout2')
> > + # now alter file in checkout
> > + a_file = file('checkout/file', 'wt')
> > + a_file.write('Foo')
> > + a_file.close()
> > + self.runbzr('commit -m checnge-file checkout')
> > + # now checkout2 should be out of date
> > + # make a local change to file
> > + a_file = file('checkout2/file', 'wt')
> > + a_file.write('Bar')
> > + a_file.close()
> > + out,err = self.runbzr('update checkout2', retcode=1)
> > + self.assertEqual(['1 conflicts encountered.',
> > + 'Updated to revision 2.'],
> > + err.split('\n')[1:3])
> > + self.assertContainsRe(err, 'Diff3 conflict encountered in.*file\n')
> > + self.assertEqual('', out)
> > + self.failUnlessExists('checkout2/file')
> > +
>
> I think this failUnless is in the wrong function. Since you want to
> check that file is conflicted here. Not that it just exists.
Yes, cruft as I developed. There should be no failUnlessExists line
here. Removing it.
> > === 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.
> > + 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)' ?
> >
> > class cmd_renames(Command):
> > @@ -629,6 +672,33 @@
> > renames.sort()
> > for old_name, new_name in renames:
> > print "%s => %s" % (old_name, new_name)
> > +
> > +
> > +class cmd_update(Command):
> > + """Update a tree to have the latest code committed to its branch.
> > +
> > + This will perform a merge into the working tree, and may generate
> > + conflicts. If you have any uncommitted changes, you will still
> > + need to commit them after the update.
> > + """
> > + takes_args = ['dir?']
> > +
> > + def run(self, dir='.'):
> > + tree = WorkingTree.open_containing(dir)[0]
> > + tree.lock_write()
> > + try:
> > + if tree.last_revision() == tree.branch.last_revision():
> > + note("Tree is up to date.")
> > + return
> > + conflicts = tree.update()
> > + note('Updated to revision %d.' %
> > + (tree.branch.revision_id_to_revno(tree.last_revision()),))
> > + if conflicts != 0:
> > + return 1
> > + else:
> > + return 0
> > + finally:
> > + tree.unlock()
>
> Shouldn't this be using 'BzrDir.open_containing()' or something like
> that? I thought BzrDir was replacing all of the other Branch/WorkingTree
> factories.
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.
> > === modified file 'bzrlib/bzrdir.py'
> > --- bzrlib/bzrdir.py
> > +++ bzrlib/bzrdir.py
> > @@ -76,7 +76,7 @@
> > pass
> > try:
> > self.open_workingtree().clone(result, basis=basis_tree)
> > - except (errors.NotBranchError, errors.NotLocalUrl):
> > + except (errors.NoWorkingTree, errors.NotLocalUrl):
> > pass
> > return result
> >
> > @@ -187,8 +187,11 @@
> > bzrdir = BzrDir.create_branch_and_repo(safe_unicode(base)).bzrdir
> > return bzrdir.create_workingtree()
> >
> > - def create_workingtree(self):
> > - """Create a working tree at this BzrDir"""
> > + def create_workingtree(self, revision_id=None):
> > + """Create a working tree at this BzrDir.
> > +
> > + revision_id: create it as of this revision id.
> > + """
> > raise NotImplementedError(self.create_workingtree)
> >
> > def get_branch_transport(self, branch_format):
> > @@ -364,7 +367,7 @@
> > self.open_workingtree().clone(result,
> > revision_id=revision_id,
> > basis=basis_tree)
> > - except (errors.NotBranchError, errors.NotLocalUrl):
> > + except (errors.NoWorkingTree, errors.NotLocalUrl):
> > result.create_workingtree()
> > return result
> >
> > @@ -395,9 +398,19 @@
> > """See BzrDir.create_repository."""
> > return self.open_repository()
> >
> > - def create_workingtree(self):
> > + def create_workingtree(self, revision_id=None):
> > """See BzrDir.create_workingtree."""
> > - return self.open_workingtree()
> > + # this looks buggy but is not -really-
> > + # clone and sprout will have set the revision_id
> > + # and that will have set it for us, its only
> > + # specific uses of create_workingtree in isolation
> > + # that can do wonky stuff here, and that only
> > + # happens for creating checkouts, which cannot be
> > + # done on this format anyway. So - acceptable wart.
> > + result = self.open_workingtree()
> > + if revision_id is not None:
> > + result.set_last_revision(revision_id)
> > + return result
>
> 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.
> ...
>
> > +
> > +
> > +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.
> > + 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.
> > wt = WorkingTree.open(basedir)
> > self.branch = wt.branch
> > self.basedir = wt.basedir
> > @@ -219,11 +224,18 @@
> > "base directory %r is not a string" % basedir
> > basedir = safe_unicode(basedir)
> > mutter("openeing working tree %r", basedir)
> > - 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.
> > + 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 ?
> 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.
> ...
>
> 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
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060213/62b63c34/attachment.pgp
More information about the bazaar
mailing list