[tahoe-dev] Comments on Kevan's review of #833 (and what is up with the trac?)

[David-Sarah Hopwood]

The trac seems to be throwing a 500 Internal Server Error whenever I try
to add a comment or attachment. So here is a comment for Kevan that I was
going to add to #833. The relevant unified diff (to be applied on top of
mutable-in-immutable-darcspatch and misc-tweaks-darcspatch) is attached.


Replying to [comment:50 kevan]:
> test/test_dirnode.py:
> {{{
> +        bad_future_node = UnknownNode(future_write_uri, None)
> +        bad_kids1 = {u"one": (bad_future_node, {})}
>          d.addCallback(lambda ign:
> +                      self.shouldFail(MustNotBeUnknownRWError, "bad_kids1",
> +                                      "cannot attach unknown",
>                                        nm.create_new_mutable_directory,
>                                        bad_kids1))
> }}}
> Could you add a comment here explaining why this should fail? If I
understand correctly, it is because the cap you provide doesn't start with
ro. or imm. and since it is unknown we don't know how to diminish it to a
readonly cap?

Yes, exactly. Comment added.

> (Few of the tests so far in test_dirnode.py have explanations, but I've
been able to understand them without. Explanations aren't a bad idea, though)

Well, they didn't start with explanations :-) This is a good point, but I
think I'll need to address it after the 1.6 release.

> I think that FakeMutableFile? needs to implement raise_error.

Actually the tests never call it, but it should have this method for
consistency. Fixed.

> test_filenode.py:
>
> around line 41,
> {{{
> +        self.failUnlessEqual(fn1.is_unknown(), False)
> +        self.failUnlessEqual(fn1.is_allowed_in_immutable_directory(), True)
> }}}
> could be re-written as
>
> {{{
>     self.failIf(fn1.is_unknown())
>     self.failUnless(fn1.is_allowed_in_mutable_directory())
> }}}

Changed for all cases in test_filenode.py.

> test/test_uri.py:
>
> It would be nice to have a comment explaining why the invalid uris are
invalid, though it's easy enough to figure out after a few seconds of
looking at it. Just a thought.

You mean the ones that result in instances of UnknownURI? (Technically there
are no invalid uris any more, because uri.from_string will catch the
BadURIError and construct an UnknownURI that remembers it. The BadURIError
might be reraised later, though.)

I think that if it's easy enough to work out, that's as it should be. The
trouble with putting too many comments in is that reviewers trust the
comments, when they shouldn't.

> test_web.py:
>
> I'm assuming that existing tests will already fail if we attempt to add an
unknown node using PUT /uri/$DIRCAP/SUBDIRS../CHILDNAME?t=uri and POST
/uri/$DIRCAP/SUBDIRS../?t=uri&name=CHILDNAME&uri=CHILDCAP, since that was
the existing behavior before your changes.

Don't make that assumption! The patch removed some of the restrictions on
UnknownNodes, so there's definitely a need to explicitly test this.

In fact the addition should succeed (for a mutable directory) when the URI
is prefixed with "ro." or "imm." -- in those cases we don't need to be able
to diminish it to a readcap, because it already is one. Tests for the six
possible cases (test_{PUT_NEWFILEURL, POST_link}_uri_unknown_{bad, ro_good,
imm_good) added.

> You've changed the _create_initial_children and _create_immutable_children
to include unknown URIs, which threads tests for those nicely through the
existing tests.

Yes, that made it a lot easier to see what needed to be tested.

> Other comments:
>
> Can you add a test for the change you made in the chunk starting around
line 230 of dirnode.py that references #925?

I'll do that separately tomorrow.

-- 
David-Sarah Hopwood  ⚥  http://davidsarah.livejournal.com
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: address-comments-from-kevan-diff.txt
Url: http://allmydata.org/pipermail/tahoe-dev/attachments/20100127/e3ff5f17/attachment.txt 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 292 bytes
Desc: OpenPGP digital signature
Url : http://allmydata.org/pipermail/tahoe-dev/attachments/20100127/e3ff5f17/attachment.pgp