Shawn Walker wrote:
> http://cr.opensolaris.org/~swalker/pkg-529/
directory.py:
- line 74: why is this line necessary?
- line 89: why the extra normpath()?
- line 109: the dnlc may make these stat() calls stupid fast, but I'd
still rather see you do a single stat call and check S_ISDIR(), etc.
- line 113: Could you put into a short comment a precis of the reason
you're not just blowing away the link and putting in the directory?
- line 128: now that we're using salvagedir() to salvage files other than
directories, it needs to have its name updated.
- if makedirs() blows up not because the leaf directory isn't a
directory, but because something up the chain isn't, what happens?
- line 184: it's still useful information, and probably should be
salvaged.
- Shouldn't the logic in file.py to prevent installing through a symlink
be used here as well?
file.py:
- line 206: why not just use locals()?
- line 437: why the islink() test?
- line 441, 443: why the existing exception in one case, and an
ActionExecutionError in the other?
- line 443: api_errors.ActionExecutionError?
hardlink.py:
- line 92, 101: so what's the qualitative difference between an
ActionExecutionError and an ActionOperationError? I know the former is
an API error and the latter isn't (and appears to get wrapped up into
an ImageStateErrors), but if all it is is something that gets wrapped
up like that, then perhaps its name could be a little more specific.
link.py:
- line 123ff: this is looking pretty familiar. Candidate for moving into
generic.py?
image.py:
- line 2485: while I understand lstripping "/" from path, I'm not sure I
understand stripping self.root.
imageplan.py:
- line 1127: this could be "except" now, right?
t_api_search.py:
- test_bug_8318: is this a mis-merge, or some other interaction with
Brock's change?
t_pkg_api_install.py:
- line 828: "577"? This should be an octal constant, so humans can read
it. Looks like plenty of other places use a decimal constant, which
will result in bad modes. If your intent is that these bad modes will
be fixed up on package install, that's fine, but I'd rather see octal
modes which are incorrect, rather than relying on the decimal -> octal
conversion going awry.
- line 831: why use self.plist here and not for install or uninstall?
t_pkg_install.py:
- line 4896: nothing uses api_obj
- line 4908: isn't this, strictly speaking, "fails as expected"?
- line 4931: this isn't ever used -- test seems incomplete
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
