Merge lp:~jelmer/bzr/314314-obsolete-packs into lp:bzr

[Jelmer Vernooij]

Deal with the obsolete_packs directory being removed from under us.

Create obsolete_packs directory when repacking if it does not exist, and ignore it when it doesn't exist when trying to clean it up.

Original proposal by Jonathan Riddell, updated to address review comments.

[Jelmer Vernooij (jelmer)]

I'm not sure if this is the right place for the test, but I don't have an idea for a better one. Ideally we would want to test this for all formats for which it is relevant (rather than in a blackbox test, which only exercises the default format). That's pretty hard though - we can't use bt.per_repository, as it's an implementation detail.

[Jelmer Vernooij (jelmer)]

Either way, this seems reasonable enough for now. We'll have to move this test when the default format changes, but that's bearable.

review: Approve

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/19/2011 3:42 PM, Jonathan Riddell wrote:
> Jonathan Riddell has proposed merging
> lp:~jr/bzr/314314-obsolete-packs into lp:bzr.
>
> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #314314 in
> Bazaar: "bzr does not automatically create 'obsolete_packs' when it
> is missing." https://bugs.launchpad.net/bzr/+bug/314314
>
> For more details, see:
> https://code.launchpad.net/~jr/bzr/314314-obsolete-packs/+merge/79816
>
> * Create obsolete_packs directory when repacking if it does not
> exist. (Jonathan Riddell, #314314)
>

I really doubt this is correct:
@@ -1221,6 +1222,10 @@
         """
         for pack in packs:
             try:
+ os.mkdir("../obsolete_packs/")
+ except OSError:
+ pass
+ try:
                 pack.pack_transport.move(pack.file_name(),
                     '../obsolete_packs/' + pack.file_name())

Given that the following code is using "pack.pack_transport" that
indicates that this is handling Remote objects. I really don't want to
see "obsolete_packs" suddenly show up in my working directory after
doing 'bzr push'.

 review: needsfixing

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk6e2qoACgkQJdeBCYSNAAPBdgCcDEEfUEJBp+0Kilbk0vq9Afkq
0wAAn2nnPqtBvgWvGGAbiXKj6qAH3PK5
=qwd8
-----END PGP SIGNATURE-----

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

Thanks John, you're right. Not sure how I missed that.

[Martin Packman (gz)]

One other concern:

         for pack in packs:
             try:
+ os.mkdir("../obsolete_packs/")
+ except OSError:
+ pass
+ try:
                 pack.pack_transport.move(pack.file_name(),
                     '../obsolete_packs/' + pack.file_name())
             except (errors.PathError, errors.TransportError), e:

Ignoring the transport issue for now, this does for every pack, try to create the directory and ignoring any errors in the process, then move the file. Presumably this doubles the number of (V)FS operations per pack, which still shouldn't be a big number but seems suboptimal given nearly all the time the directory will already exist.

A better approach would be to detect if the move failed because the move-into directory was missing, create it then, and retry the move. However, it may be that we can't reliably do that over all the different transports we support (translating errors into what they actually mean is very hackish), so always trying to create the directory may still be the best way. Doing that once before this loop, rather than for each pack, should be sufficient though.

review: Needs Fixing

[Martin Packman (gz)]

Moving this to work in progress for now, but will see if I can pick it up and address the review comments.

[Martin Packman (gz)]

The code looks good. I just wonder if this also needs a per_transport or similar test as well. The current tests seem to only cover the local case, and fallout is most likely over ftp and such like.

review: Approve

[Jelmer Vernooij (jelmer)]

Am 12/12/11 12:28, schrieb Martin Packman:
> Review: Approve
>
> The code looks good. I just wonder if this also needs a per_transport or similar test as well. The current tests seem to only cover the local case, and fallout is most likely over ftp and such like.
Yeah, that would indeed be nice to make sure that has appropriate
testing, regardless of this. I'll leave that for another time though,
this change should improve the current situation in either case.

Cheers,

Jelmer

[Jelmer Vernooij (jelmer)]

sent to pqm by email

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/12/2011 12:28 PM, Martin Packman wrote:
> Review: Approve
>
> The code looks good. I just wonder if this also needs a
> per_transport or similar test as well. The current tests seem to
> only cover the local case, and fallout is most likely over ftp and
> such like.

I think a patch like this could be reasonably backported to older
versions of bzr. At least to 2.4, maybe not all the way back to
Lucid's 2.1.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk7l8X4ACgkQJdeBCYSNAAPrGQCdH/7xx/iY1Vbl4893BYPyic7f
S8MAn0bz2ELKEV7e6nDqX5e0uP55AdBk
=2IvX
-----END PGP SIGNATURE-----

[Jelmer Vernooij (jelmer)]

sent to pqm by email

=== modified file 'bzrlib/repofmt/pack_repo.py'
--- bzrlib/repofmt/pack_repo.py	2011-12-07 14:03:01 +0000
+++ bzrlib/repofmt/pack_repo.py	2011-12-12 13:49:25 +0000
@@ -1221,8 +1221,18 @@
         """
         for pack in packs:
             try:
-                pack.pack_transport.move(pack.file_name(),
-                    '../obsolete_packs/' + pack.file_name())
+                try:
+                    pack.pack_transport.move(pack.file_name(),
+                        '../obsolete_packs/' + pack.file_name())
+                except errors.NoSuchFile:
+                    # perhaps obsolete_packs was removed? Let's create it and
+                    # try again
+                    try:
+                        pack.pack_transport.mkdir('../obsolete_packs/')
+                    except errors.FileExists:
+                        pass
+                    pack.pack_transport.move(pack.file_name(),
+                        '../obsolete_packs/' + pack.file_name())
             except (errors.PathError, errors.TransportError), e:
                 # TODO: Should these be warnings or mutters?
                 mutter("couldn't rename obsolete pack, skipping it:\n%s"
@@ -1494,7 +1504,11 @@
         obsolete_pack_transport = self.transport.clone('obsolete_packs')
         if preserve is None:
             preserve = set()
-        for filename in obsolete_pack_transport.list_dir('.'):
+        try:
+            obsolete_pack_files = obsolete_pack_transport.list_dir('.')
+        except errors.NoSuchFile:
+            return found
+        for filename in obsolete_pack_files:
             name, ext = osutils.splitext(filename)
             if ext == '.pack':
                 found.append(name)

=== modified file 'bzrlib/tests/test_repository.py'
--- bzrlib/tests/test_repository.py	2011-12-11 03:42:09 +0000
+++ bzrlib/tests/test_repository.py	2011-12-12 13:49:25 +0000
@@ -1082,6 +1082,23 @@
             sorted(set([osutils.splitext(n)[0] for n in
                         packs._index_transport.list_dir('.')])))
 
+    def test__obsolete_packs_missing_directory(self):
+        tree, r, packs, revs = self.make_packs_and_alt_repo(write_lock=True)
+        r.control_transport.rmdir('obsolete_packs')
+        names = packs.names()
+        pack = packs.get_pack_by_name(names[0])
+        # Schedule this one for removal
+        packs._remove_pack_from_memory(pack)
+        # Now trigger the obsoletion, and ensure that all the remaining files
+        # are still renamed
+        packs._obsolete_packs([pack])
+        self.assertEqual([n + '.pack' for n in names[1:]],
+                         sorted(packs._pack_transport.list_dir('.')))
+        # names[0] should not be present in the index anymore
+        self.assertEqual(names[1:],
+            sorted(set([osutils.splitext(n)[0] for n in
+                        packs._index_transport.list_dir('.')])))
+
     def test_pack_distribution_zero(self):
         packs = self.get_packs()
         self.assertEqual([0], packs.pack_distribution(0))
@@ -1357,6 +1374,12 @@
         obsolete_names = set([osutils.splitext(n)[0] for n in obsolete_packs])
         self.assertEqual([pack.name], sorted(obsolete_names))
 
+    def test_pack_no_obsolete_packs_directory(self):
+        """Bug #314314, don't fail if obsolete_packs directory does
+        not exist."""
+        tree, r, packs, revs = self.make_packs_and_alt_repo(write_lock=True)
+        r.control_transport.rmdir('obsolete_packs')
+        packs._clear_obsolete_packs()
 
 
 class TestPack(TestCaseWithTransport):

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-12-12 13:05:24 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-12-12 13:49:25 +0000
@@ -41,6 +41,9 @@
 * Allow configuration option default value to be a python callable at
   registration. (Vincent Ladeuil, #832064)
 
+* Create obsolete_packs directory when repacking if it does not
+  exist. (Jonathan Riddell, Jelmer Vernooij, #314314)
+
 * Properly ignore '\n' in an option reference since this cannot be part of a
   config option identifier. (Vincent Ladeuil, #902125)