[MERGE] remove crackful MutableTree.smart_add behavior

[Aaron Bentley]

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

Robert Collins wrote:
> On Tue, 2007-07-03 at 21:56 -0400, Aaron Bentley wrote:
>> My feeling was that ability to change the inventory without writing it
>> to disk, and without intending to ever write it to disk was an
>> unintentional feature, and not something to rely on.
> 
> We used to have add code that simulated adding the files, but it was
> considerably more complicated. I think if we can keeping the current
> constraint of allowing a revert-to-disk facility is better.

As it turns out, your code doesn't match the guarantee you have been
fighting for.  This already-present test case proves it doesn't:

    def test_save_false(self):
        """Dry-run add doesn't permanently affect the tree."""
        wt = self.make_branch_and_tree('.')
        self.build_tree(['file'])
        wt.smart_add(['file'], save=False)
        # the file should not be added - no id.
        self.assertEqual(wt.path2id('file'), None)

^^^ right here, the test guarantees that the "save" parameter does not
live up to the contract described in MutableTree.smart_add.  Because if
the inventory was retained in memory, without writing to disk, then
wt.path2id would produce a string.

This is not a test case I wrote.  It's one that you modified.  Here's
the diff:

- -        smart_add_tree(wt, ['file'], save=False)
- -        # The in-memory inventory is left modified in inventory-based
trees;
- -        # it may not be in dirstate trees.  Anyhow, now we reload to
make sure
- -        # the on-disk version is not modified.
+        wt.smart_add(['file'], save=False)
+        # the file should not be added - no id.
+        self.assertEqual(wt.path2id('file'), None)
+        # and the disk state should be the same - reopen to check.


This is kinda schizo-- you've modified a test to prevent behavior you
say you want.

So why is the test passing?  The code sure looks like it shouldn't:

        if len(added) > 0 and save:
            self._write_inventory(inv)
        return added, ignored

If the inventory was in that state when we did path2id, I'd expect it to
produce a string, wouldn't you?

As it turns out, WT.lock_read and WT.lock_write will sync the inventory
from disk:

    def lock_read(self):
        """See Branch.lock_read, and WorkingTree.unlock."""
        if not self.is_locked():
            self._reset_data()
        self.branch.lock_read()

No lock is being held on the tree, so the in-memory inventory is being
blown away.  Let's try holding the lock:

@@ -54,10 +54,14 @@
     def test_save_false(self):
         """Dry-run add doesn't permanently affect the tree."""
         wt = self.make_branch_and_tree('.')
- -        self.build_tree(['file'])
- -        wt.smart_add(['file'], save=False)
- -        # the file should not be added - no id.
- -        self.assertEqual(wt.path2id('file'), None)
+        wt.lock_write()
+        try:
+            self.build_tree(['file'])
+            wt.smart_add(['file'], save=False)
+            # the file should not be added - no id.
+            self.assertEqual(wt.path2id('file'), None)
+        finally:
+            wt.unlock()


Now we get this:

FAIL:
workingtree_implementations.test_smart_add.TestSmartAddTree.test_save_false(WorkingTreeFormat3)
    not equal:
a = 'file-20070704181107-u4fr6thbumcjc7lf-3'
b = None


FAIL:
workingtree_implementations.test_smart_add.TestSmartAddTree.test_save_false(WorkingTreeFormat2)
    not equal:
a = 'file-20070704181107-u4fr6thbumcjc7lf-4'
b = None

WorkingTreeFormat4 does not fail the test.  Which means that it does not
obey the requirements of the doctext:

"Note that the modified inventory is left in place, allowing further
dry-run tasks to take place."

So your own code makes your arguments for the status quo moot.
WorkingTree4 does not retain a modified-in-memory inventory, even if you
think it should.

I see 3 possible fixes:
1. Make WorkingTree4 reatain a modified-in-memory inventory
2. Revert your patch
3. Change the contract specified by MutableTree.smart_add.

Here's a patch to accomplish 3.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGi+ai0F+nu1YWqI0RAl03AJwImu/REllamSQXtCEbp/IUH1MRpACfTTW0
AEmZUiTTMVHka9xOmuFs2fw=
=aFqf
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: no-mem-only-inventory.patch
Type: text/x-patch
Size: 3282 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070704/e4d0c079/attachment.bin