[tahoe-lafs-trac-stream] [tahoe-lafs] #1465: Move ImmutableShare to backend specific module, add "backend" interface to Storage Server, use twisted's FilePath

[tahoe-lafs]

#1465: Move ImmutableShare to backend specific module, add "backend" interface to
Storage Server, use twisted's FilePath
------------------------------+---------------------------
     Reporter:  Zancas        |      Owner:  zancas
         Type:  enhancement   |     Status:  new
     Priority:  major         |  Milestone:  undecided
    Component:  code-storage  |    Version:  1.8.2
   Resolution:                |   Keywords:  review-needed
Launchpad Bug:                |
------------------------------+---------------------------
Changes (by zooko):

 * owner:  zooko => zancas


Comment:

 Regarding the first patch, [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1465/nullpass_Zancas20110803.darcs.patch#L21 the
 file test_backends has been added.], here are a few comments.

 * Please make the "patch name" more informative to someone who isn't that
 familiar with the context. See also the suggested patch names on
 wiki:Patches . For this patch, I would suggest something like "storage:
 add tests of the new feature of having the storage backend in a separate
 object from the server".
 * Emacs has a command {{{M-x whitespace-cleanup}}} that removes trailing
 whitespace from lines. Brian and I use it (or its equivalent)
 traditionally, so if you do too then there will be fewer lines
 unnecessarily touched. (I don't think David-Sarah uses emacs, but they do
 somehow always have whitespace-clean line endings...)
 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1465/nullpass_Zancas20110803.darcs.patch#L270
 TestServerWithNullBackend.test_write_share] needs a docstring explaining
 what behavior it is making sure happens or doesn't happen in the Code-
 Under-Test.
 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1465/nullpass_Zancas20110803.darcs.patch#L293
 TestServerContruction.test_create_server_fs_backend] has such a docstring!
 But it is out of date. :-) I believe we've changed the test so it is no
 longer checking whether the Code-Under-Test read or writes outside of its
 prescribed directory, and instead are merely checking whether it finishes
 running its constructor without raising an exception.
 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1465/nullpass_Zancas20110803.darcs.patch#L327
 TestServerAndFSBackend.test_write_and_read_share]: good! I'm glad to see
 this functionality being tested like this.
 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1465/nullpass_Zancas20110803.darcs.patch#L378
 TestServerAndFSBackend.test_read_old_share]: Likewise.
 * Everything that says "{{{FS}}}" is supposed to be changed to say
 "{{{Disk}}}", if I understand correctly.

 Otherwise this looks like a good patch! I look forward to reviewing the
 rest of them...

 I like the use of a mock/fake filesystem instead of the real local
 filesystem (disclaimer: I worked with Zancas on doing that for these
 patches).

-- 
Ticket URL: <http://tahoe-lafs.org/trac/tahoe-lafs/ticket/1465#comment:5>
tahoe-lafs <http://tahoe-lafs.org>
secure decentralized storage