=== modified file 'NEWS' --- NEWS 2009-07-15 19:06:39 +0000 +++ NEWS 2009-07-17 02:29:14 +0000 @@ -19,6 +19,17 @@ * ``merge --interactive`` applies a user-selected portion of the merge. The UI is similar to ``shelve``. (Aaron Bentley) +* ``upgrade`` now upgrades dependent branches when a shared repository is + specified. It also supports several new options: + + * --dry-run for showing what will happen + + * --clean to remove the backup.dir directory on successful completion + + * --pack to pack the repository on successful completion. + + (Ian Clatworthy) + I'd put the options in double-backticks too. You might also find some bug numbers corresponding to these... Bug Fixes ********* === modified file 'bzrlib/builtins.py' --- bzrlib/builtins.py 2009-07-15 07:32:26 +0000 +++ bzrlib/builtins.py 2009-07-17 02:29:14 +0000 @@ -3122,27 +3122,60 @@ class cmd_upgrade(Command): - """Upgrade branch storage to current format. - - The check command or bzr developers may sometimes advise you to run - this command. When the default format has changed you may also be warned - during other operations to upgrade. + """Upgrade a repository, branch or working tree to a newer format. + + The check command or Bazaar developers may sometimes advise you to run + this command. When the default format has changed after a major new + release of Bazaar, you may also be warned during other operations + that you should upgrade. This bit about 'the check command' almost sounds like a mis-copy from cmd_reconcile. Do we ever specifically recommend an upgrade from check? Also it might be better off to have the first full paragraph be a more general explanation like this: Upgrading to a newer format may improve performance or make new features available, but it may limit interoperability with older repositories or with older versions of Bazaar. Strictly speaking you can also use upgrade to downgrade; it doesn't have to be to a newer format. + + If the location given is a shared repository, dependent branches + are also converted provided the repository converts successfully. + If the conversion of a branch fails, remaining branches are still tried. + + A backup.bzr directory is created at the start of the conversion + process. By default, this is left there on completion. If the + conversion fails, delete the new .bzr directory and rename this + one back in its place. Use the --clean option to ask for the + backup.bzr directory to be removed on successful conversion. + Alternatively, you can delete it by hand if everything looks + good afterwards. + + It is often a good idea to pack the repository after an upgrade. + Use the --pack option to request this or do it separately using + the pack command. + + For more information on upgrades, see the Bazaar 2.0 Upgrade Guide. """ - _see_also = ['check'] + _see_also = ['check', 'reconcile', 'pack'] Also current-formats etc? takes_args = ['url?'] takes_options = [ - RegistryOption('format', - help='Upgrade to a specific format. See "bzr help' - ' formats" for details.', - lazy_registry=('bzrlib.bzrdir', 'format_registry'), - converter=lambda name: bzrdir.format_registry.make_bzrdir(name), - value_switches=True, title='Branch format'), - ] + RegistryOption('format', + help='Upgrade to a specific format. See "bzr help' + ' formats" for details.', + lazy_registry=('bzrlib.bzrdir', 'format_registry'), + converter=lambda name: bzrdir.format_registry.make_bzrdir(name), + value_switches=True, title='Branch format'), + Option('clean', + help='Remove the backup.bzr directory if successful.'), + Option('pack', + help='Pack repositories that successful upgrade.'), + Option('dry-run', + help="Show what would be done, but don't actually do anything."), + ] - def run(self, url='.', format=None): + def run(self, url='.', format=None, clean=False, pack=False, + dry_run=False): from bzrlib.upgrade import upgrade - upgrade(url, format) + exceptions = upgrade(url, format, clean_up=clean, pack=pack, + dry_run=dry_run) + if exceptions: + if len(exceptions) == 1: + # This provides backwards compatibility + raise exceptions[0] + else: + return 3 class cmd_whoami(Command): === modified file 'bzrlib/bzrdir.py' --- bzrlib/bzrdir.py 2009-07-01 10:49:05 +0000 +++ bzrlib/bzrdir.py 2009-07-17 02:00:11 +0000 @@ -87,7 +87,7 @@ class BzrDir(object): - """A .bzr control diretory. + """A .bzr control directory. BzrDir instances let you create or open any of the things that can be found within .bzr - checkouts, branches and repositories. @@ -1292,6 +1292,40 @@ push_result.branch_push_result.target_branch return push_result + def _get_object_and_label(self): + """Return the primary object and type label for a control directory. + + :return: object, label where + object is a Branch, Repository or WorkingTree and + label is one of: + branch - a branch + repository - a repository + tree - a lightweight checkout + """ + try: + try: + br = self.open_branch(unsupported=True, ignore_fallbacks=True) + except TypeError: + # RemoteRepository doesn't support the unsupported parameter + br = self.open_branch(ignore_fallbacks=True) + except errors.NotBranchError: + pass + else: + return br, "branch" + try: + repo = self.open_repository() + except errors.NoRepositoryPresent: + pass + else: + return repo, "repository" + try: + wt = self.open_workingtree() + except (errors.NoWorkingTree, errors.NotLocalUrl): + pass + else: + return wt, "tree" + raise AssertionError("unknown type of control directory %s", self) + class BzrDirHooks(hooks.Hooks): """Hooks for BzrDir operations.""" === modified file 'bzrlib/tests/blackbox/test_upgrade.py' --- bzrlib/tests/blackbox/test_upgrade.py 2009-03-23 14:59:43 +0000 +++ bzrlib/tests/blackbox/test_upgrade.py 2009-07-17 00:39:20 +0000 @@ -145,6 +145,21 @@ self.run_bzr('init-repository --format=metaweave repo') self.run_bzr('upgrade --format=knit repo') + def _assert_option_legal(self, option_str): + # Confirm that an option is legal. (Lower level tests are + # expected to validate the actual functionality.) + self.run_bzr('init --format=pack-0.92 branch-foo') + self.run_bzr('upgrade --format=2a branch-foo %s' % (option_str,)) + + def test_upgrade_clean_supported(self): + self._assert_option_legal('--clean') + + def test_upgrade_pack_supported(self): + self._assert_option_legal('--pack') + + def test_upgrade_dry_run_supported(self): + self._assert_option_legal('--dry-run') + class SFTPTests(TestCaseWithSFTPServer): """Tests for upgrade over sftp.""" These seem like kind of expensive tests considering they don't actually check very much... I guess they do have some smoke-test benefit. I'd be happier (perhaps irrationally) if they at least made some simple assertions at the end, ie that there is no backup.bzr, that there's only one pack, and that it's still a pack-0.92 format respectively. It's not necessary for this change, but it seems like this code is almost getting close to the point where we have a separation between the CLI and the intermediate layer (bzrlib.upgrade) such that we can test only the CLI against some kind of mock or dummy object. On the other hand the CLI itself is so trivial that may be uninteresting. Also irrelevant to this test, I'd kind of like to see some common place, maybe a test factory, that says "a good old format to test is 0.92 and a good new format is 2a" so that's not written by hand in all tests. === modified file 'bzrlib/tests/test_foreign.py' --- bzrlib/tests/test_foreign.py 2009-06-10 03:56:49 +0000 +++ bzrlib/tests/test_foreign.py 2009-07-17 02:00:11 +0000 @@ -249,7 +249,7 @@ self._control_files = lockable_files.LockableFiles(self.transport, "lock", lockable_files.TransportLock) - def open_branch(self, ignore_fallbacks=True): + def open_branch(self, unsupported=False, ignore_fallbacks=True): return self._format.get_branch_format().open(self, _found=True) def cloning_metadir(self, stacked=False): === modified file 'bzrlib/tests/test_upgrade.py' --- bzrlib/tests/test_upgrade.py 2009-05-07 05:08:46 +0000 +++ bzrlib/tests/test_upgrade.py 2009-07-17 02:28:46 +0000 @@ -29,6 +29,7 @@ from bzrlib import ( branch as _mod_branch, bzrdir, + osutils, progress, repository, workingtree, @@ -38,7 +39,7 @@ from bzrlib.branch import Branch from bzrlib.tests import TestCaseWithTransport from bzrlib.transport import get_transport -from bzrlib.upgrade import upgrade +from bzrlib.upgrade import upgrade, smart_upgrade class TestUpgrade(TestCaseWithTransport): @@ -428,3 +429,62 @@ ), ( './dir/', ), ] + + +class TestSmartUpgrade(TestCaseWithTransport): + + from_format = "pack-0.92" + to_format = bzrdir.format_registry.make_bzrdir("2a") + + def make_standalone_branch(self): + wt = self.make_branch_and_tree("branch1", format=self.from_format) + return wt.bzrdir + + def test_upgrade_standalone_branch(self): + control = self.make_standalone_branch() + tried, worked, issues = smart_upgrade([control], format=self.to_format) + self.assertEqual(1, len(tried)) + self.assertEqual(1, len(worked)) + self.assertEqual(0, len(issues)) + self.failUnlessExists('branch1/backup.bzr') This looks a bit like it should be returning some kind of result object rather than a 3-tuple, so there's space for more or so the fields are named? + + def test_upgrade_standalone_branch_cleanup(self): + control = self.make_standalone_branch() + tried, worked, issues = smart_upgrade([control], format=self.to_format, + clean_up=True) + self.assertEqual(1, len(tried)) + self.assertEqual(1, len(worked)) + self.assertEqual(0, len(issues)) + self.failUnlessExists('branch1') + self.failUnlessExists('branch1/.bzr') + self.failIfExists('branch1/backup.bzr') + + def make_repo_with_branches(self): + repo = self.make_repository('repo', shared=True, + format=self.from_format) + b1 = self.make_branch("repo/branch1", format=self.from_format) + b2 = self.make_branch("repo/branch2", format=self.from_format) + return repo.bzrdir + + def test_upgrade_repo_with_branches(self): + control = self.make_repo_with_branches() + tried, worked, issues = smart_upgrade([control], format=self.to_format) + self.assertEqual(3, len(tried)) + self.assertEqual(3, len(worked)) + self.assertEqual(0, len(issues)) + self.failUnlessExists('repo/backup.bzr') + self.failUnlessExists('repo/branch1/backup.bzr') + self.failUnlessExists('repo/branch2/backup.bzr') + + def test_upgrade_repo_with_branches_cleanup(self): + control = self.make_repo_with_branches() + tried, worked, issues = smart_upgrade([control], format=self.to_format, + clean_up=True) + self.assertEqual(3, len(tried)) + self.assertEqual(3, len(worked)) + self.assertEqual(0, len(issues)) + self.failUnlessExists('repo') + self.failUnlessExists('repo/.bzr') + self.failIfExists('repo/backup.bzr') + self.failIfExists('repo/branch1/backup.bzr') + self.failIfExists('repo/branch2/backup.bzr') It seems a bit like these tests are a bit weak for the added code; I suppose checking that the backup.bzr exists tells you some kind of upgrade happened, but given the 'except Exception' blocks the assurance it completed is relatively weak. Also, should you be making some assertions about the contents of the results - are they paths, objects, etc. === modified file 'bzrlib/upgrade.py' --- bzrlib/upgrade.py 2009-05-07 05:08:46 +0000 +++ bzrlib/upgrade.py 2009-07-17 02:00:11 +0000 @@ -17,18 +17,36 @@ """bzr upgrade logic.""" +from bzrlib import osutils, repository from bzrlib.bzrdir import BzrDir, BzrDirFormat, format_registry import bzrlib.errors as errors from bzrlib.remote import RemoteBzrDir from bzrlib.transport import get_transport +from bzrlib.trace import mutter, note, warning import bzrlib.ui as ui class Convert(object): - def __init__(self, url, format=None): + def __init__(self, url=None, format=None, control_dir=None): + """Convert a Bazaar control directory to a given format. + + Either the url or control_dir parameter must be given. + + :param url: the URL of the control directory or None if the + control_dir is explicitly given instead + :param format: the format to convert to or None for the default + :param control_dir: the control directory or None if it is + specified via the URL parameter instead + """ self.format = format - self.bzrdir = BzrDir.open_unsupported(url) + if url is None and control_dir is None: + raise AssertionError( + "either the url or control_dir parameter must be set.") + if control_dir is not None: + self.bzrdir = control_dir + else: + self.bzrdir = BzrDir.open_unsupported(url) if isinstance(self.bzrdir, RemoteBzrDir): self.bzrdir._ensure_real() self.bzrdir = self.bzrdir._real_bzrdir It's kind of gross that this does everything from its constructor, but that's not your fault, and possibly mine. (I was thinking about this syndrome the other day and will send a separate mail on it. More to the point here, having methods that optionally take DWIM parameters (like a URL vs the object at that URL) has been a bit problematic or confusing in the past. It's probably better in general to have different factory methods if you want to offer both... But this particular thing looks ok. @@ -73,13 +91,179 @@ self.bzrdir._format) self.bzrdir.check_conversion_target(format) self.pb.note('starting upgrade of %s', self.transport.base) - self.bzrdir.backup_bzrdir() + self.backup_oldpath, self.backup_newpath = self.bzrdir.backup_bzrdir() while self.bzrdir.needs_format_conversion(format): converter = self.bzrdir._format.get_converter(format) self.bzrdir = converter.convert(self.bzrdir, self.pb) self.pb.note("finished") - -def upgrade(url, format=None): - """Upgrade to format, or the default bzrdir format if not supplied.""" - Convert(url, format) + def clean_up(self): + """Clean-up after a conversion. + + This removes the backup.bzr directory. + """ + transport = self.transport + backup_relpath = transport.relpath(self.backup_newpath) + transport.delete_tree(backup_relpath) You might want a pb here; it may take a long time over a remote transport. Just creating one should ensure the delete_tree method shows activity. + + +def upgrade(urls, format=None, clean_up=False, pack=False, dry_run=False): + """Upgrade locations to format. + + This routine wraps the smart_upgrade() routine with a nicer UI. + In particular, it ensures all URLs can be opened before starting + and reports a summary at the end if more than one upgrade was attempted. + This routine is useful for command line tools. Other bzrlib clients + probably ought to use smart_upgrade() instead. + + :param urls: a sequence of URLs to the locations to upgrade. + For backwards compatibility, if urls is a string, it is treated + as a single URL. + :param format: the format to convert to or None for the best default + :param clean-up: if True, the backup.bzr directory is removed if the + upgrade succeeded for a given repo/branch/tree + :param pack: pack repositories that successfully upgrade + :param dry_run: show what would happen but don't actually do any upgrades + :return: the list of exceptions encountered + """ + if isinstance(urls, basestring): + urls = [urls] Maybe a deprecation warning here? dwim on sequences vs strings is a bug source in Python. + control_dirs = [BzrDir.open_unsupported(url) for url in urls] + attempted, succeeded, exceptions = smart_upgrade(control_dirs, + format, clean_up=clean_up, pack=pack, dry_run=dry_run) + if len(attempted) > 1: + attempted_count = len(attempted) + succeeded_count = len(succeeded) + failed_count = attempted_count - succeeded_count + note("\nSUMMARY: %d upgrades attempted, %d succeeded, %d failed", + attempted_count, succeeded_count, failed_count) + return exceptions If this is cli-specific maybe it should just be in cmd_upgrade, and then smart_upgrade can be called 'upgrade' and used by other clients? + + +def smart_upgrade(control_dirs, format, clean_up=False, pack=False, + dry_run=False): + """Convert control directories to a new format intelligently. + + If the control directory is a shared repository, dependent branches + are also converted provided the repository converted successfully. + If the conversion of a branch fails, remaining branches are still tried. + + :param control_dirs: the BzrDirs to upgrade + :param format: the format to convert to or None for the best default + :param clean-up: if True, the backup.bzr directory is removed if the + upgrade succeeded for a given repo/branch/tree 'clean_up', also below. + :param pack: pack repositories that successfully upgrade + :param dry_run: show what would happen but don't actually do any upgrades + :return: attempted-control-dirs, succeeded-control-dirs, exceptions + """ + all_attempted = [] + all_succeeded = [] + all_exceptions = [] + for control_dir in control_dirs: + attempted, succeeded, exceptions = _smart_upgrade_one(control_dir, + format, clean_up=clean_up, pack=pack, dry_run=dry_run) + all_attempted.extend(attempted) + all_succeeded.extend(succeeded) + all_exceptions.extend(exceptions) + return all_attempted, all_succeeded, all_exceptions + + +def _smart_upgrade_one(control_dir, format, clean_up=False, pack=False, + dry_run=False): + """Convert a control directory to a new format intelligently. + + See smart_upgrade for parameter details. + """ + # If the URL is a shared repository, find the dependent branches + dependents = None + try: + repo = control_dir.open_repository() + except errors.NoRepositoryPresent: + # A branch or checkout using a shared repository higher up + pass + else: + # The URL is a repository. If it successfully upgrades, + # then upgrade the dependent branches as well. + if repo.is_shared(): + dependents = repo.find_branches() + + # Do the conversions + attempted = [control_dir] + succeeded, exceptions = _convert_items([control_dir], format, clean_up, + pack, dry_run, verbose=dependents) + if succeeded and dependents: + note("Found %d dependent branches - upgrading ...", len(dependents)) + + # Convert dependent branches + branch_cdirs = [b.bzrdir for b in dependents] + successes, problems = _convert_items(branch_cdirs, format, clean_up, + pack, dry_run, label="branch") + attempted.extend(branch_cdirs) + succeeded.extend(successes) + exceptions.extend(problems) + + # Return the result + return attempted, succeeded, exceptions + + +def _convert_items(items, format, clean_up, pack, dry_run, label=None, + verbose=True): + """Convert a sequence of control directories to the given format. + + :param items: the control directories to upgrade + :param format: the format to convert to or None for the best default + :param clean-up: if True, the backup.bzr directory is removed if the + upgrade succeeded for a given repo/branch/tree + :param pack: pack repositories that successfully upgrade + :param dry_run: show what would happen but don't actually do any upgrades + :param label: the label for these items or None to calculate one + :param verbose: if True, output a message before starting and + display any problems encountered + :return: items successfully upgraded, exceptions + """ + succeeded = [] + exceptions = [] + for control_dir in items: + # Do the conversion + location = control_dir.root_transport.base + bzr_object, bzr_label = control_dir._get_object_and_label() + if verbose: + type_label = label or bzr_label + note("Upgrading %s %s ...", type_label, location) + try: + if not dry_run: + cv = Convert(control_dir=control_dir, format=format) + except Exception, ex: + _verbose_warning(verbose, "conversion error: %s" % ex) + exceptions.append(ex) + continue + + # Do any required post processing + succeeded.append(control_dir) + if pack and isinstance(bzr_object, repository.Repository): + note("Packing ...") + try: + if not dry_run: + bzr_object.pack() + except Exception, ex: + _verbose_warning(verbose, "failed to pack %s: %s" % + (location, ex)) + exceptions.append(ex) + if clean_up: + try: + note("Removing backup ...") + if not dry_run: + cv.clean_up() + except Exception, ex: + _verbose_warning(verbose, "failed to clean-up %s: %s" % + (location, ex)) + exceptions.append(ex) + + # Return the result + return succeeded, exceptions I'm a bit scared of all those Exception blocks; there's some risk of confusing knock-on errors. Do you really need them? Again a progress bar across items might be nice to show people how it's going. + + +def _verbose_warning(verbose, msg): + mutter(msg) + if verbose: + warning(msg)