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

[Jonathan Riddell]

* Create obsolete_packs directory when repacking if it does not
  exist. (Jonathan Riddell, #314314)

[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.

=== modified file 'bzrlib/repofmt/pack_repo.py'
--- bzrlib/repofmt/pack_repo.py	2011-07-25 07:51:35 +0000
+++ bzrlib/repofmt/pack_repo.py	2011-10-19 13:45:30 +0000
@@ -16,6 +16,7 @@
 
 import re
 import sys
+import os
 
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
@@ -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())
             except (errors.PathError, errors.TransportError), e:
@@ -1491,6 +1496,10 @@
             were found in obsolete_packs.
         """
         found = []
+        try:
+            self.transport.mkdir('obsolete_packs')
+        except errors.FileExists:
+            pass
         obsolete_pack_transport = self.transport.clone('obsolete_packs')
         if preserve is None:
             preserve = set()

=== modified file 'bzrlib/tests/blackbox/test_pack.py'
--- bzrlib/tests/blackbox/test_pack.py	2011-05-13 20:44:45 +0000
+++ bzrlib/tests/blackbox/test_pack.py	2011-10-19 13:45:30 +0000
@@ -83,3 +83,14 @@
 
         pack_names = transport.list_dir('repository/obsolete_packs')
         self.assertTrue(len(pack_names) == 0)
+
+    def test_pack_no_obsolete_packs_directory(self):
+        """Bug #314314, don't fail if obsolete_packs directory does
+        not exist."""
+        wt = self.make_branch_and_tree('.')
+        open('foo', 'w').write('foo')
+        self.run_bzr(['add'])
+        self.run_bzr(['commit', '-m', '"commit 1"'])
+        import os
+        os.rmdir('.bzr/repository/obsolete_packs/')
+        self.run_bzr(['pack'])

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-10-19 12:46:41 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-10-19 13:45:30 +0000
@@ -48,6 +48,9 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* Create obsolete_packs directory when repacking if it does not
+  exist. (Jonathan Riddell, #314314)
+
 * ``bzr mkdir --quiet`` now does not print a line for every created
   directory. (Martin von Gagern, #869915)