Merge lp:~vila/bzr/323111-backup-names into lp:bzr

[Vincent Ladeuil]

This introduces a new ``osutils.available_backup_name`` that can be used
to produce <file>.~#~ paths where we need them.

With my next submissions there will be 3 such use cases, enough
to triangulate: they are all protected by a lock so we don't have
to care about race conditions.

The case at hand is ``BzrDir.generate_backup_name``. In the
future we may try to avoid LBYL, but that's out-of-scope for this
submissions and the following ones (and I don't think we really
care here and I'm sure we don't for the 2 other cases (1 in
memory, the other on local fs).

I changed the name from 'generate' to 'available' as I felt it
was better describing: nothing is created but a check is
performed (and this fit well for the other cases too).

[Vincent Ladeuil (vila)]

>>>>> John Arbash Meinel <email address hidden> writes:

    > On 9/10/2010 9:40 AM, Vincent Ladeuil wrote:
    >> + def test_resolve_conflicts_missing_parent(self):

    > The available_backup_name seems ok. I'm not a big fan of 'transport.has'
    > or the look-before-you-leap in general, but we were already doing it.

    > I don't really understand what this is doing here, though:
    > + def test_resolve_conflicts_missing_parent(self):

    > It seems unrelated. Also why remove:
    > - def test_resolve_no_parent(self):

I've replaced it by test_resolve_conflicts_missing_parent clarifying it
while I was there.

    > === modified file 'bzrlib/workingtree.py'
    > --- bzrlib/workingtree.py 2010-08-20 19:07:17 +0000
    > +++ bzrlib/workingtree.py 2010-09-10 14:39:52 +0000
    > @@ -2078,9 +2078,10 @@
    > files_to_backup.append(path[1])

    > def backup(file_to_backup):
    > - backup_name =
    > self.bzrdir.generate_backup_name(file_to_backup)
    > + backup_name =
    > self.bzrdir._available_backup_name(file_to_backup)

    > ^- IMO: WorkingTree shouldn't be calling an '_' method on BzrDir,

Wow, we do that a lot already, what's the problem ?

The public methods or attributes are for bzrlib users while the private
ones are for bzrlib internal needs, we never say this applies to inter
class calls.

    > so we should probably be making it public.

IMHO, methods should be public if there is a need for them outside of
bzrlib and we guarantee some stability and backward compatibility.

If there is no need, the maintenance cost is lowered.

    > Then again, why is *BzrDir* finding the available name, and not
    > working tree itself?

Well, I just touched this method lightly, I didn't research the
rationale in the corresponding merge proposal.

[Andrew Bennetts (spiv)]

Regarding jam's comments: we aren't consistent about whether leading underscores indicates private to bzrlib, private to that module, or even private to that class. So you have to look at stuff case-by-case in my experience (unless the code somehow makes it clear via comments/docstrings).

In this case workingtree.py already calls self._bzrdir._get_file_mode a fair bit, so there's some precedent... but it seems odd to me to make it private, because presumably if it's a useful method on bzrdir (and it must be, you're using it), then couldn't 3rd-party code like plugins have a use for it too? After all the method you are deprecating, BzrDir.generate_backup_name, is public, perhaps there was a reason for that (perhaps not, but it's worth asking)? Given that the behaviour appears to be exactly the same what's the purpose of changing the method name here? Is it that you consider generate_backup_name to be a poor API that shouldn't be used? If so I don't follow why it's ok for bzrlib to keep using it internally but not 3rd-party code. If it's so that it's easy to find callers to fix later when we stop doing LBYL for this, then it should say so in the docstring!

A related issue is that _available_backup_name really ought to have a comment saying that it isn't race-free because it's not guarded by a lock.

in test_transform:

240 + # Since the directory doesn't exist it's seen as missing to resolve
241 + # create a conflict asking for it to be created.

That comment doesn't parse for me, and I don't understand how the test_transform changes are related to the rest of the patch.

So:

 * +1 on refactoring to create the function in osutils
 * I'm unconvinced on the change to BzrDir's API. I'm open to be convinced, but first I need to understand the intent of changing it :)
 * I don't understand the comment in the test_transform change. It's small enough that I don't mind too much that it appears to be mixed into an unrelated patch, but the comment does need to make sense.

review: Needs Fixing

[Vincent Ladeuil (vila)]

>>>>> Andrew Bennetts <email address hidden> writes:

    > Review: Needs Fixing

    > Regarding jam's comments: we aren't consistent about whether
    > leading underscores indicates private to bzrlib, private to that
    > module, or even private to that class. So you have to look at
    > stuff case-by-case in my experience (unless the code somehow makes
    > it clear via comments/docstrings).

    > In this case workingtree.py already calls
    > self._bzrdir._get_file_mode a fair bit, so there's some
    > precedent...

    > but it seems odd to me to make it private, because presumably if
    > it's a useful method on bzrdir (and it must be, you're using it),
    > then couldn't 3rd-party code like plugins have a use for it too?

I don't know.

That's why I make private :)

Th rationale is that maintaining compatibility is guaranteed for the
public methods. Despite this, plugin authors use private methods.

It seems more adequate then to drive the maintenance effort based on
real use cases rather than 'potential' use cases.

Since I couldn't see *obvious* cases where bzrdir._available_backup_name
can be used, I made it private (moreover, as jam pointed out, it may be
better located on wt instead but I wanted to limit my cleanups to the
code duplication and added the deprecations as I felt things weren't
that clear in this area so going private will help in the future if we
need to change).

The only bits I thought were worth publicizing were the backup name
policy, so I put that in osutils for lack of a better place (osutils
(as a module) must die ! (and become a package ;)).

    > After all the method you are deprecating,
    > BzrDir.generate_backup_name, is public, perhaps there was a reason
    > for that (perhaps not, but it's worth asking)?

It has been recently introduced and duplicated the existing code in
transform, see
https://code.edge.launchpad.net/~vila/bzr/323111-deprecate-get-backup-name/+merge/35111
which use 'get_backup_name' instead of 'generate_backup_name'.

    > Given that the behaviour appears to be exactly the same what's the
    > purpose of changing the method name here?

Hinting at the race condition which wasn't clear with 'generate' (or
get_backup_name in transform, see following submissions) and make it a
method.

I thought I explained that in the cover letter:

,----
| With my next submissions there will be 3 such use cases, enough
| to triangulate: they are all protected by a lock so we don't have
| to care about race conditions.
|
| The case at hand is ``BzrDir.generate_backup_name``. In the
| future we may try to avoid LBYL, but that's out-of-scope for this
| submissions and the following ones (and I don't think we really
| care here and I'm sure we don't for the 2 other cases (1 in
| memory, the other on local fs).
|
| I changed the name from 'generate' to 'available' as I felt it
| was better describing: nothing is created but a check is
| performed (and this fit well for the other cases too).
`----

    > Is it that you consider generate_backup_name to be a poor API that
    > shouldn't be used?

Well 'generate_backup_name' and 'get_backup_name' were racy and
misleading, hopefully '...

[lp:~vila/bzr/323111-backup-names]

5418. By Vincent Ladeuil on 2010-09-16

Merge cleanup into backup-names

5419. By Vincent Ladeuil on 2010-09-16

Be more explicit about race conditions and LBYL being discouraged

5420. By Vincent Ladeuil on 2010-09-21

Merge cleanup into backup-names

5421. By Vincent Ladeuil on 2010-09-21

Move NEWS entry to 2.3b2

[Vincent Ladeuil (vila)]

Updated, please review.

[Martin Pool (mbp)]

It looks basically OK to me; I'll let Andrew check if you've sufficiently addressed his points.

review: Approve

[lp:~vila/bzr/323111-backup-names]

5422. By Vincent Ladeuil on 2010-10-15

Merge cleanup into backup-names resolving conflicts

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2010-10-15 11:20:45 +0000
+++ bzrlib/bzrdir.py	2010-10-15 14:16:33 +0000
@@ -87,6 +87,10 @@
     registry,
     symbol_versioning,
     )
+from bzrlib.symbol_versioning import (
+    deprecated_in,
+    deprecated_method,
+    )
 
 
 class BzrDir(controldir.ControlDir):
@@ -497,14 +501,16 @@
                                                format=format).bzrdir
         return bzrdir.create_workingtree()
 
+    @deprecated_method(deprecated_in((2, 3, 0)))
     def generate_backup_name(self, base):
-        """Generate a non-existing backup file name based on base."""
-        counter = 1
-        name = "%s.~%d~" % (base, counter)
-        while self.root_transport.has(name):
-            counter += 1
-            name = "%s.~%d~" % (base, counter)
-        return name
+        return self._available_backup_name(base)
+
+    def _available_backup_name(self, base):
+        """Find a non-existing backup file name based on base.
+
+        See bzrlib.osutils.available_backup_name about race conditions.
+        """
+        return osutils.available_backup_name(base, self.root_transport.has)
 
     def backup_bzrdir(self):
         """Backup this bzr control directory.
@@ -512,16 +518,13 @@
         :return: Tuple with old path name and new path name
         """
 
-        backup_dir=self.generate_backup_name('backup.bzr')
         pb = ui.ui_factory.nested_progress_bar()
         try:
-            # FIXME: bug 300001 -- the backup fails if the backup directory
-            # already exists, but it should instead either remove it or make
-            # a new backup directory.
-            #
             old_path = self.root_transport.abspath('.bzr')
+            backup_dir = self._available_backup_name('backup.bzr')
             new_path = self.root_transport.abspath(backup_dir)
-            ui.ui_factory.note('making backup of %s\n  to %s' % (old_path, new_path,))
+            ui.ui_factory.note('making backup of %s\n  to %s'
+                               % (old_path, new_path,))
             self.root_transport.copy_tree('.bzr', backup_dir)
             return (old_path, new_path)
         finally:

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2010-08-24 12:36:53 +0000
+++ bzrlib/osutils.py	2010-10-15 14:16:33 +0000
@@ -2354,3 +2354,23 @@
         raise errors.BzrError("Can't decode username as %s." % \
                 user_encoding)
     return username
+
+
+def available_backup_name(base, exists):
+    """Find a non-existing backup file name.
+
+    This will *not* create anything, this only return a 'free' entry.  This
+    should be used for checking names in a directory below a locked
+    tree/branch/repo to avoid race conditions. This is LBYL (Look Before You
+    Leap) and generally discouraged.
+
+    :param base: The base name.
+
+    :param exists: A callable returning True if the path parameter exists.
+    """
+    counter = 1
+    name = "%s.~%d~" % (base, counter)
+    while exists(name):
+        counter += 1
+        name = "%s.~%d~" % (base, counter)
+    return name

=== modified file 'bzrlib/tests/per_workingtree/__init__.py'
--- bzrlib/tests/per_workingtree/__init__.py	2010-08-21 16:06:24 +0000
+++ bzrlib/tests/per_workingtree/__init__.py	2010-10-15 14:16:33 +0000
@@ -23,6 +23,7 @@
 """
 
 from bzrlib import (
+    branchbuilder,
     errors,
     tests,
     workingtree,
@@ -58,6 +59,13 @@
         made_control.create_branch()
         return self.workingtree_format.initialize(made_control)
 
+    def make_branch_builder(self, relpath, format=None):
+        if format is None:
+            format = self.bzrdir_format
+        builder = branchbuilder.BranchBuilder(self.get_transport(),
+                                              format=format)
+        return builder
+
 
 def workingtree_formats():
     """The known working tree formats."""

=== modified file 'bzrlib/tests/per_workingtree/test_merge_from_branch.py'
--- bzrlib/tests/per_workingtree/test_merge_from_branch.py	2010-04-01 14:06:48 +0000
+++ bzrlib/tests/per_workingtree/test_merge_from_branch.py	2010-10-15 14:16:33 +0000
@@ -20,7 +20,6 @@
 import os
 
 from bzrlib import (
-    branchbuilder,
     errors,
     merge
     )
@@ -119,13 +118,6 @@
 
 class TestMergedBranch(per_workingtree.TestCaseWithWorkingTree):
 
-    def make_branch_builder(self, relpath, format=None):
-        if format is None:
-            format = self.bzrdir_format
-        builder = branchbuilder.BranchBuilder(self.get_transport(),
-                                              format=format)
-        return builder
-
     def make_inner_branch(self):
         bld_inner = self.make_branch_builder('inner')
         bld_inner.start_series()

=== modified file 'bzrlib/tests/test_bzrdir.py'
--- bzrlib/tests/test_bzrdir.py	2010-08-20 19:07:17 +0000
+++ bzrlib/tests/test_bzrdir.py	2010-10-15 14:16:33 +0000
@@ -32,6 +32,7 @@
     repository,
     osutils,
     remote,
+    symbol_versioning,
     urlutils,
     win32utils,
     workingtree,
@@ -1417,6 +1418,9 @@
 
 
 class TestGenerateBackupName(TestCaseWithMemoryTransport):
+    # FIXME: This may need to be unified with test_osutils.TestBackupNames or
+    # moved to per_bzrdir or per_transport for better coverage ?
+    # -- vila 20100909
 
     def setUp(self):
         super(TestGenerateBackupName, self).setUp()
@@ -1425,9 +1429,14 @@
             possible_transports=[self._transport])
         self._bzrdir = bzrdir.BzrDir.open_from_transport(self._transport)
 
+    def test_deprecated_generate_backup_name(self):
+        res = self.applyDeprecated(
+                symbol_versioning.deprecated_in((2, 3, 0)),
+                self._bzrdir.generate_backup_name, 'whatever')
+
     def test_new(self):
-        self.assertEqual("a.~1~", self._bzrdir.generate_backup_name("a"))
+        self.assertEqual("a.~1~", self._bzrdir._available_backup_name("a"))
 
     def test_exiting(self):
         self._transport.put_bytes("a.~1~", "some content")
-        self.assertEqual("a.~2~", self._bzrdir.generate_backup_name("a"))
+        self.assertEqual("a.~2~", self._bzrdir._available_backup_name("a"))

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2010-09-27 07:24:54 +0000
+++ bzrlib/tests/test_osutils.py	2010-10-15 14:16:33 +0000
@@ -2085,3 +2085,34 @@
         encoded_username = uni_username.encode(ue)
         osutils.set_or_unset_env('LOGNAME', encoded_username)
         self.assertEqual(uni_username, osutils.getuser_unicode())
+        osutils.set_or_unset_env('LOGNAME', u'jrandom\xb6'.encode(ue))
+        self.assertEqual(u'jrandom\xb6', osutils.getuser_unicode())
+
+class TestBackupNames(tests.TestCase):
+
+    def setUp(self):
+        super(TestBackupNames, self).setUp()
+        self.backups = []
+
+    def backup_exists(self, name):
+        return name in self.backups
+
+    def available_backup_name(self, name):
+        backup_name = osutils.available_backup_name(name, self.backup_exists)
+        self.backups.append(backup_name)
+        return backup_name
+
+    def assertBackupName(self, expected, name):
+        self.assertEqual(expected, self.available_backup_name(name))
+
+    def test_empty(self):
+        self.assertBackupName('file.~1~', 'file')
+
+    def test_existing(self):
+        self.available_backup_name('file')
+        self.available_backup_name('file')
+        self.assertBackupName('file.~3~', 'file')
+        # Empty slots are found, this is not a strict requirement and may be
+        # revisited if we test against all implementations.
+        self.backups.remove('file.~2~')
+        self.assertBackupName('file.~2~', 'file')

=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py	2010-10-08 10:50:51 +0000
+++ bzrlib/tests/test_transform.py	2010-10-15 14:16:33 +0000
@@ -815,6 +815,21 @@
         self.assertIs(None, self.wt.path2id('parent'))
         self.assertIs(None, self.wt.path2id('parent.new'))
 
+    def test_resolve_conflicts_missing_parent(self):
+        wt = self.make_branch_and_tree('.')
+        tt = TreeTransform(wt)
+        self.addCleanup(tt.finalize)
+        parent = tt.trans_id_file_id('parent-id')
+        tt.new_file('file', parent, 'Contents')
+        raw_conflicts = resolve_conflicts(tt)
+        # Since the directory doesn't exist it's seen as 'missing'.  So
+        # 'resolve_conflicts' create a conflict asking for it to be created.
+        self.assertLength(1, raw_conflicts)
+        self.assertEqual(('missing parent', 'Created directory', 'new-1'),
+                         raw_conflicts.pop())
+        # apply fail since the missing directory doesn't exist
+        self.assertRaises(errors.NoFinalPath, tt.apply)
+
     def test_moving_versioned_directories(self):
         create, root = self.get_transform()
         kansas = create.new_directory('kansas', root, 'kansas-id')
@@ -2331,14 +2346,6 @@
         self.failUnlessExists('a')
         self.failUnlessExists('a/b')
 
-    def test_resolve_no_parent(self):
-        wt = self.make_branch_and_tree('.')
-        tt = TreeTransform(wt)
-        self.addCleanup(tt.finalize)
-        parent = tt.trans_id_file_id('parent-id')
-        tt.new_file('file', parent, 'Contents')
-        resolve_conflicts(tt)
-
 
 A_ENTRY = ('a-id', ('a', 'a'), True, (True, True),
                   ('TREE_ROOT', 'TREE_ROOT'), ('a', 'a'), ('file', 'file'),

=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2010-08-27 07:54:21 +0000
+++ bzrlib/transform.py	2010-10-15 14:16:33 +0000
@@ -2917,7 +2917,7 @@
             try:
                 os.rename(to, from_)
             except OSError, e:
-                raise errors.TransformRenameFailed(to, from_, str(e), e.errno)                
+                raise errors.TransformRenameFailed(to, from_, str(e), e.errno)
         # after rollback, don't reuse _FileMover
         past_renames = None
         pending_deletions = None

=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2010-09-24 16:16:13 +0000
+++ bzrlib/workingtree.py	2010-10-15 14:16:33 +0000
@@ -2080,9 +2080,10 @@
                     files_to_backup.append(path[1])
 
         def backup(file_to_backup):
-            backup_name = self.bzrdir.generate_backup_name(file_to_backup)
+            backup_name = self.bzrdir._available_backup_name(file_to_backup)
             osutils.rename(abs_path, self.abspath(backup_name))
-            return "removed %s (but kept a copy: %s)" % (file_to_backup, backup_name)
+            return "removed %s (but kept a copy: %s)" % (file_to_backup,
+                                                         backup_name)
 
         # Build inv_delta and delete files where applicable,
         # do this before any modifications to inventory.

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-10-15 11:33:33 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-10-15 14:16:33 +0000
@@ -103,6 +103,10 @@
   strictly "asciibetical".  That behavior is still available as ``bzr tags
   --sort=alpha``. (Neil Martinsen-Burrell, #640760)
 
+* ``BzrDir.generate_backup_name`` has been deprecated and replaced by a
+  private method. ``osutils.available_backup_name`` provides an extensible
+  replacement. (Vincent Ladeuil)
+
 New Features
 ************