tahoe-lafs/trac-2024-07-25

minor code clean-up in dirnode.py #967

New issue

Closed

opened 2010-02-21 05:34:38 +00:00 by zooko · 12 comments

zooko commented

2010-02-21 05:34:38 +00:00

Owner

I don't want to commit this to trunk until after v1.6.1 release, but please go ahead and review it.

Impose micro-POLA by passing only the writekey instead of the whole node object to _encrypt_rw_uri(). Remove DummyImmutableFileNode in nodemaker.py, which is obviated by this. Add micro-optimization by precomputing the netstring of the empty string and branching on whether the writekey is present or not outside of _encrypt_rw_uri(). Add doc about writekey to docstring.

I don't want to commit this to trunk until after v1.6.1 release, but please go ahead and review it. Impose micro-POLA by passing only the writekey instead of the whole node object to `_encrypt_rw_uri()`. Remove [DummyImmutableFileNode](wiki/DummyImmutableFileNode) in nodemaker.py, which is obviated by this. Add micro-optimization by precomputing the netstring of the empty string and branching on whether the writekey is present or not outside of `_encrypt_rw_uri()`. Add doc about writekey to docstring.

tahoe-lafs added the

labels 2010-02-21 05:34:38 +00:00

tahoe-lafs added this to the 1.7.0 milestone 2010-02-21 05:34:38 +00:00

zooko commented

2010-02-21 05:35:55 +00:00

Author

Owner

Attachment dirnode-minor-cleanup.darcs.patch.txt (8778 bytes) added

**Attachment** dirnode-minor-cleanup.darcs.patch.txt (8778 bytes) added

dirnode-minor-cleanup.darcs.patch.txt

8.6 KiB

kevan commented

2010-04-04 01:59:35 +00:00

Author

Owner

All of the tests pass when the patch is applied.

The changes to _encrypt_rw_uri look good, and you don't appear to have missed any of its callers. I think that the same can be said for pack_children; I don't see any problems with those changes, and you appear to have changed all of its callers that needed changing.

There appears to be trailing whitespace on line 188 of dirnode.py (right after assert isinstance(rw_uri, str), rw_uri). Your patch didn't introduce that, but, as long as you're doing code cleanup, you may as well fix it. :)

Unless I'm missing something, DummyImmutableFileNode is no longer necessary. If you agree, then you should remove it from nodemaker.py.

Other than that, this looks good.

All of the tests pass when the patch is applied. The changes to _encrypt_rw_uri look good, and you don't appear to have missed any of its callers. I think that the same can be said for pack_children; I don't see any problems with those changes, and you appear to have changed all of its callers that needed changing. There appears to be trailing whitespace on line 188 of dirnode.py (right after `assert isinstance(rw_uri, str), rw_uri`). Your patch didn't introduce that, but, as long as you're doing code cleanup, you may as well fix it. :) Unless I'm missing something, [DummyImmutableFileNode](wiki/DummyImmutableFileNode) is no longer necessary. If you agree, then you should remove it from nodemaker.py. Other than that, this looks good.

zooko commented

2010-06-17 05:00:35 +00:00

Author

Owner

Heh, then I forgot to commit it and now I don't want to commit it before the v1.7.0 release.

However! I have created a "post-1.7" branch, and I can commit it to that...

changeset:20100221052527-92b7f-3ee334f0e78c3890d3f8b8d2b5d9b440b2a5343d/post-1.7/

Heh, then I forgot to commit it and now I don't want to commit it before the v1.7.0 release. However! I have created a "post-1.7" branch, and I can commit it to that... changeset:20100221052527-92b7f-3ee334f0e78c3890d3f8b8d2b5d9b440b2a5343d/post-1.7/

zooko commented

2010-06-17 05:06:45 +00:00

Author

Owner

Applied your two clean-up suggestions in changeset:20100617045339-92b7f-940b4924686845e26ec03bd6c7d6094beec14c7e/post-1.7. Thanks!

tahoe-lafs added the

fixed

label 2010-06-17 05:06:45 +00:00

tahoe-lafs modified the milestone from 1.7.0 to soon

2010-06-17 05:06:45 +00:00

zooko closed this issue

2010-06-17 05:06:45 +00:00

zooko commented

2010-06-17 23:44:33 +00:00

Author

Owner

<warner> zooko: your tidy-ups in the post-1.7 branch look good. You might
	 consider doing "packed = pack_children(children, writekey=None)" in
	 nodemaker.py to make it more obvious that it's creating a
	 writecap-less structure				        [17:20]
<warner> in fact, I'd consider making the writekey= argument mandatory, since
	 I think the only place that ever calls it with a known-None value is
	 that line in nodemaker.py				        [17:21]

``` <warner> zooko: your tidy-ups in the post-1.7 branch look good. You might consider doing "packed = pack_children(children, writekey=None)" in nodemaker.py to make it more obvious that it's creating a writecap-less structure [17:20] <warner> in fact, I'd consider making the writekey= argument mandatory, since I think the only place that ever calls it with a known-None value is that line in nodemaker.py [17:21] ```

zooko commented

2010-07-11 04:44:21 +00:00

Author

Owner

Attachment dirnodecleanup.dpatch.txt (11761 bytes) added

**Attachment** dirnodecleanup.dpatch.txt (11761 bytes) added

dirnodecleanup.dpatch.txt

12 KiB

zooko commented

2010-07-11 04:44:31 +00:00

Author

Owner

reopen to port to trunk (from the post-1.7 branch) and apply Brian's suggestion from comment:117485. Please review!

reopen to port to trunk (from the post-1.7 branch) and apply Brian's suggestion from [comment:117485](/tahoe-lafs/trac-2024-07-25/issues/967#issuecomment-117485). Please review!

tahoe-lafs removed the

fixed

label 2010-07-11 04:44:31 +00:00

tahoe-lafs modified the milestone from soon to 1.7.1

2010-07-11 04:44:31 +00:00

zooko reopened this issue

2010-07-11 04:44:31 +00:00

zooko commented

2010-07-17 07:24:51 +00:00

Author

Owner

Not a bugfix, so not going into 1.7.1.

tahoe-lafs modified the milestone from 1.7.1 to 1.8β

2010-07-17 07:24:51 +00:00

davidsarah commented

2010-07-17 23:44:30 +00:00

Author

Owner

Applied in changeset:6e8477114e9dfd06. Was that intentional?

tahoe-lafs added the

fixed

label 2010-07-17 23:44:30 +00:00

tahoe-lafs modified the milestone from 1.8β to 1.7.1

2010-07-17 23:44:30 +00:00

davidsarah closed this issue

2010-07-17 23:44:30 +00:00

davidsarah commented

2010-07-17 23:47:26 +00:00

Author

Owner

FWIW, the port to trunk looks correct.

zooko commented

2010-07-18 00:08:23 +00:00

Author

Owner

That was not intentional. I accidentally committed this patch to trunk. Haven't decided yet whether to undo it for 1.7.1 or just leave it.

davidsarah commented

2010-07-18 03:57:50 +00:00

Author

Owner

I checked that your port is equivalent to the similar version that was on the post1.7 branch. (The only difference was whether the writekey argument has a default -- in your version it doesn't, which is better.)