[MERGE] Shelf 4 / 5: ShelfManager

[John Arbash Meinel]

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

Aaron Bentley wrote:
> Hi all,
> 
> This patch implements ShelfMananger, which manages the stack of changes
> on a shelf.  Shelved changes are stored in numbered files, like:
> .bzr/checkout/shelf/4
> 
> The stack is ordered from lowest to highest, so 5 is popped before 4.
> When a new entry is added, it's given the highest number + 1.
> 
> Aaron

+    def active_shelves(self):
+        """Return a list of shelved changes."""
+        return [int(f) for f in self.transport.list_dir('.')]

^- This seems potentially brittle. It will give very strange tracebacks
if anything that is not an integer is placed in that directory. It also
does weird things if you have a ".bzr/checkout/shelf/1" and a
".bzr/checkout/shelf/01" in the same dir.

I don't think we have to handle every possible situation, but getting a
random traceback isn't as nice as something like:

shelves = []
for f in self.transport.list_dir('.'):
  try:
    shelves.append(int(f))
  except ValueError:
    trace.warning('Invalid filename for a shelf: %s',
	          self.transport.abspath(f))

This lets it keep working on valid shelves, and still give the user a
warning that they have some sort of clutter in that directory.

+
+    def last_shelf(self):
+        """Return the id of the last-created shelved change."""
+        active = self.active_shelves()
+        if len(active) > 0:
+            return max(active)
+        else:
+            return None

^- Would it be reasonable to change "active_shelves()" to include a
"sorted(shelves)" call? That way you could just use "active[-1]" rather
than max().

It doesn't really matter, but since list_dir() returns files in "random"
order, it might be nice to stabilize .active_shelves() by sorting them.

...

+class TestShelfManager(tests.TestCaseWithTransport):
+

...

+    def test_active_shelves(self):
+        manager = self.get_manager()
+        self.assertEqual([], manager.active_shelves())
+        shelf_id, shelf_file = manager.new_shelf()
+        shelf_file.close()
+        self.assertEqual([1], manager.active_shelves())
+

^- Would it be reasonable to extend these functions into checking for
more than 1 shelf? Just to make sure that stuff like "delete_shelf(2)"
won't accidentally delete shelf 1 or 3. Also, how things will work when
you have 'missing' shelves in the middle.

Just a thought. What you've written are reasonable smoke tests, and the
code you have isn't wrong. It just feels like something should cover
more than the trivial case.

...

=== modified file 'bzrlib/workingtree.py'
- --- bzrlib/workingtree.py	2008-09-30 20:30:04 +0000
+++ bzrlib/workingtree.py	2008-10-17 03:31:02 +0000
@@ -2504,6 +2504,11 @@
                 self)._get_rules_searcher(default_searcher)
         return self._rules_searcher

+    def get_shelf_manager(self):
+        """Return the ShelfManager for this WorkingTree."""
+        from bzrlib.shelf import ShelfManager
+        return ShelfManager(self, self._transport)
+

^- I see you did end up making it a WorkingTree function, which is what
I was thinking the "give me a shelf" should be, I just didn't know you
had it in a later patch.

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

iEYEARECAAYFAkj+AwwACgkQJdeBCYSNAAMNhgCfakZrjAx9vGbO/8n1ot0uufb1
fxsAoIIRNdcWdHjXhTNuCVB8tPqMs33b
=tdF8
-----END PGP SIGNATURE-----