Merge pull request #299 from anarcat/no-inplace

do not upgrade repositories in place by default

borg/archiver.py

@@ -472,7 +472,7 @@ Type "Yes I am sure" if you understand this and want to continue.\n""")
         # XXX: should auto-detect if it is an attic repository here
         repo = AtticRepositoryUpgrader(args.repository.path, create=False)
         try:
-            repo.upgrade(args.dry_run)
+            repo.upgrade(args.dry_run, inplace=args.inplace)
         except NotImplementedError as e:
             print("warning: %s" % e)
         return self.exit_code
@@ -896,7 +896,7 @@ Type "Yes I am sure" if you understand this and want to continue.\n""")
                                help='repository to prune')
 
         upgrade_epilog = textwrap.dedent("""
-        upgrade an existing Borg repository in place. this currently
+        upgrade an existing Borg repository. this currently
         only support converting an Attic repository, but may
         eventually be extended to cover major Borg upgrades as well.
 
@@ -911,13 +911,6 @@ Type "Yes I am sure" if you understand this and want to continue.\n""")
         the first backup after the conversion takes longer than expected
         due to the cache resync.
 
-        it is recommended you run this on a copy of the Attic
-        repository, in case something goes wrong, for example:
-
-            cp -a attic borg
-            borg upgrade -n borg
-            borg upgrade borg
-
         upgrade should be able to resume if interrupted, although it
         will still iterate over all segments. if you want to start
         from scratch, use `borg delete` over the copied repository to
@@ -925,11 +918,19 @@ Type "Yes I am sure" if you understand this and want to continue.\n""")
 
             borg delete borg
 
-        the conversion can PERMANENTLY DAMAGE YOUR REPOSITORY! Attic
-        will also NOT BE ABLE TO READ THE BORG REPOSITORY ANYMORE, as
-        the magic strings will have changed.
-
-        you have been warned.""")
+        unless ``--inplace`` is specified, the upgrade process first
+        creates a backup copy of the repository, in
+        REPOSITORY.upgrade-DATETIME, using hardlinks. this takes
+        longer than in place upgrades, but is much safer and gives
+        progress information (as opposed to ``cp -al``). once you are
+        satisfied with the conversion, you can safely destroy the
+        backup copy.
+
+        WARNING: running the upgrade in place will make the current
+        copy unusable with older version, with no way of going back
+        to previous versions. this can PERMANENTLY DAMAGE YOUR
+        REPOSITORY!  Attic CAN NOT READ BORG REPOSITORIES, as the
+        magic strings have changed. you have been warned.""")
         subparser = subparsers.add_parser('upgrade', parents=[common_parser],
                                           description=self.do_upgrade.__doc__,
                                           epilog=upgrade_epilog,
@@ -938,6 +939,10 @@ Type "Yes I am sure" if you understand this and want to continue.\n""")
         subparser.add_argument('-n', '--dry-run', dest='dry_run',
                                default=False, action='store_true',
                                help='do not change repository')
+        subparser.add_argument('-i', '--inplace', dest='inplace',
+                               default=False, action='store_true',
+                               help="""rewrite repository in place, with no chance of going back to older
+                               versions of the repository.""")
         subparser.add_argument('repository', metavar='REPOSITORY', nargs='?', default='',
                                type=location_validator(archive=False),
                                help='path to the repository to be upgraded')

borg/testsuite/upgrader.py

@@ -14,11 +14,9 @@ except ImportError:
 from ..upgrader import AtticRepositoryUpgrader, AtticKeyfileKey
 from ..helpers import get_keys_dir
 from ..key import KeyfileKey
+from ..remote import RemoteRepository
 from ..repository import Repository, MAGIC
 
-pytestmark = pytest.mark.skipif(attic is None,
-                                reason='cannot find an attic install')
-
 
 def repo_valid(path):
     """
@@ -63,8 +61,12 @@ def attic_repo(tmpdir):
     attic_repo.close()
     return attic_repo
 
+@pytest.fixture(params=[True, False])
+def inplace(request):
+    return request.param
 
-def test_convert_segments(tmpdir, attic_repo):
+@pytest.mark.skipif(attic is None, reason='cannot find an attic install')
+def test_convert_segments(tmpdir, attic_repo, inplace):
     """test segment conversion
 
     this will load the given attic repository, list all the segments
@@ -80,7 +82,7 @@ def test_convert_segments(tmpdir, attic_repo):
     repo = AtticRepositoryUpgrader(str(tmpdir), create=False)
     segments = [filename for i, filename in repo.io.segment_iterator()]
     repo.close()
-    repo.convert_segments(segments, dryrun=False)
+    repo.convert_segments(segments, dryrun=False, inplace=inplace)
     repo.convert_cache(dryrun=False)
     assert repo_valid(tmpdir)
 
@@ -123,6 +125,7 @@ def attic_key_file(attic_repo, tmpdir):
                                        MockArgs(keys_dir))
 
 
+@pytest.mark.skipif(attic is None, reason='cannot find an attic install')
 def test_keys(tmpdir, attic_repo, attic_key_file):
     """test key conversion
 
@@ -141,7 +144,8 @@ def test_keys(tmpdir, attic_repo, attic_key_file):
     assert key_valid(attic_key_file.path)
 
 
-def test_convert_all(tmpdir, attic_repo, attic_key_file):
+@pytest.mark.skipif(attic is None, reason='cannot find an attic install')
+def test_convert_all(tmpdir, attic_repo, attic_key_file, inplace):
     """test all conversion steps
 
     this runs everything. mostly redundant test, since everything is
@@ -155,7 +159,46 @@ def test_convert_all(tmpdir, attic_repo, attic_key_file):
     """
     # check should fail because of magic number
     assert not repo_valid(tmpdir)
+    def stat_segment(path):
+        return os.stat(os.path.join(path, 'data', '0', '0'))
+    def first_inode(path):
+        return stat_segment(path).st_ino
+    orig_inode = first_inode(attic_repo.path)
     repo = AtticRepositoryUpgrader(str(tmpdir), create=False)
-    repo.upgrade(dryrun=False)
+    # replicate command dispatch, partly
+    os.umask(RemoteRepository.umask)
+    backup = repo.upgrade(dryrun=False, inplace=inplace)
+    if inplace:
+        assert backup is None
+        assert first_inode(repo.path) == orig_inode
+    else:
+        assert backup
+        assert first_inode(repo.path) != first_inode(backup)
+        # i have seen cases where the copied tree has world-readable
+        # permissions, which is wrong
+        assert stat_segment(backup).st_mode & 0o007== 0
+
     assert key_valid(attic_key_file.path)
     assert repo_valid(tmpdir)
+
+def test_hardlink(tmpdir, inplace):
+    """test that we handle hard links properly
+
+    that is, if we are in "inplace" mode, hardlinks should *not*
+    change (ie. we write to the file directly, so we do not rewrite the
+    whole file, and we do not re-create the file).
+
+    if we are *not* in inplace mode, then the inode should change, as
+    we are supposed to leave the original inode alone."""
+    a = str(tmpdir.join('a'))
+    with open(a, 'wb') as tmp:
+        tmp.write(b'aXXX')
+    b = str(tmpdir.join('b'))
+    os.link(a, b)
+    AtticRepositoryUpgrader.header_replace(b, b'a', b'b', inplace=inplace)
+    if not inplace:
+        assert os.stat(a).st_ino != os.stat(b).st_ino
+    else:
+        assert os.stat(a).st_ino == os.stat(b).st_ino
+    with open(b, 'rb') as tmp:
+        assert tmp.read() == b'bXXX'

borg/upgrader.py

@@ -1,4 +1,5 @@
 from binascii import hexlify
+import datetime
 import logging
 logger = logging.getLogger(__name__)
 import os
@@ -15,7 +16,7 @@ ATTIC_MAGIC = b'ATTICSEG'
 
 
 class AtticRepositoryUpgrader(Repository):
-    def upgrade(self, dryrun=True):
+    def upgrade(self, dryrun=True, inplace=False):
         """convert an attic repository to a borg repository
 
         those are the files that need to be upgraded here, from most
@@ -26,6 +27,12 @@ class AtticRepositoryUpgrader(Repository):
         we nevertheless do the order in reverse, as we prefer to do
         the fast stuff first, to improve interactivity.
         """
+        backup = None
+        if not inplace:
+            backup = '{}.upgrade-{:%Y-%m-%d-%H:%M:%S}'.format(self.path, datetime.datetime.now())
+            logger.info('making a hardlink copy in %s', backup)
+            if not dryrun:
+                shutil.copytree(self.path, backup, copy_function=os.link)
         logger.info("opening attic repository with borg and converting")
         # we need to open the repo to load configuration, keyfiles and segments
         self.open(self.path, exclusive=False)
@@ -42,13 +49,14 @@ class AtticRepositoryUpgrader(Repository):
                                    exclusive=True).acquire()
         try:
             self.convert_cache(dryrun)
-            self.convert_segments(segments, dryrun)
+            self.convert_segments(segments, dryrun=dryrun, inplace=inplace)
         finally:
             self.lock.release()
             self.lock = None
+        return backup
 
     @staticmethod
-    def convert_segments(segments, dryrun):
+    def convert_segments(segments, dryrun=True, inplace=False):
         """convert repository segments from attic to borg
 
         replacement pattern is `s/ATTICSEG/BORG_SEG/` in files in
@@ -60,23 +68,37 @@ class AtticRepositoryUpgrader(Repository):
         i = 0
         for filename in segments:
             i += 1
-            print("\rconverting segment %d/%d in place, %.2f%% done (%s)"
+            print("\rconverting segment %d/%d, %.2f%% done (%s)"
                   % (i, len(segments), 100*float(i)/len(segments), filename),
                   end='', file=sys.stderr)
             if dryrun:
                 time.sleep(0.001)
             else:
-                AtticRepositoryUpgrader.header_replace(filename, ATTIC_MAGIC, MAGIC)
+                AtticRepositoryUpgrader.header_replace(filename, ATTIC_MAGIC, MAGIC, inplace=inplace)
         print(file=sys.stderr)
 
     @staticmethod
-    def header_replace(filename, old_magic, new_magic):
+    def header_replace(filename, old_magic, new_magic, inplace=True):
         with open(filename, 'r+b') as segment:
             segment.seek(0)
             # only write if necessary
             if segment.read(len(old_magic)) == old_magic:
-                segment.seek(0)
-                segment.write(new_magic)
+                if inplace:
+                    segment.seek(0)
+                    segment.write(new_magic)
+                else:
+                    # remove the hardlink and rewrite the file. this
+                    # works because our old file handle is still open
+                    # so even though the file is removed, we can still
+                    # read it until the file is closed.
+                    os.rename(filename, filename + '.tmp')
+                    with open(filename, 'wb') as new_segment:
+                        new_segment.write(new_magic)
+                        new_segment.write(segment.read())
+                    # the little dance with the .tmp file is necessary
+                    # because Windows won't allow overwriting an open
+                    # file.
+                    os.unlink(filename + '.tmp')
 
     def find_attic_keyfile(self):
         """find the attic keyfiles