I also finally got a chance to look at this code. I've included my comments below.
Meta comment about OSBase: It's incredibly cumbersome to call a static method of get_instance() every time we want to look a the attributes for the current OSBase. This isn't going to change during the runtime of the script. Instead of calling OSBase.get_instance().<method>, create an OSBase instance when the client/depot are initialized. Then callers can simply to ob.<method> instead of the long, convoluted approach that is currently used. directory.py:107 - errno can only return one error code at a time, so this conditional is impossible. I think you meant if e.errno != errno.EEXIST or e.errno != errno.ENOTEMPTY generic.py:146 - Why are we introducing a file_closer()? And, more importantly, why does the file closer perform an unlink instead of a close in this case? filelist.py:190 - Why did you change make_opener() from a private static method to a public instance method? Is this method being called from outside the filelist code? The make_opener function isn't doing anything with the object that it's operating on. It looks to me like both make_opener and make_closer should remain private static methods. transaction.py:231 - You need to send the client an error code here. My guess is that 501 - "Not Implemented" is appropriate, but Danek may have other suggestions. Meta comment about os_util: Do you need to include a wrapper for shutil in this or OSBase? The shutil documentation says that, "on MacOS the resource fork and other metadata are not used. For file copies, this means that resources will be lost and file type and creator codes will not be correct." We use shutil copy and rmtree, and perhaps other functions too. I'm pretty sure that we want these to work correctly across a variety of platforms. That's all I have for now. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
