Shawn Walker wrote: > On 02/24/12 14:52, Danek Duvall wrote: > >Shawn Walker wrote: > > > >> https://cr.opensolaris.org/action/browse/pkg/swalker/pkg-7145863/webrev/ > > >generic.py: > > > > - line 276: maybe an assert() would be appropriate here? > > No, I'm playing tricks here. Since the 'hash' attribute gets added to > 'sattrs' at runtime, I take advantage of the fact that the only attribute > that's possibly in sattrs but not in self.attrs is the 'hash' so I can just > get that and move on. > > Or are you suggesting I assert that k == 'hash' ?
That was it, but it seems okay without. > >manifest.py: > > > > - line 361: why all the changes from strip to rstrip? I would think it'd > > be more critical to lstrip than to rstrip, and if the action parsing > > code doesn't care about leading whitespace (it's not clear to me that > > it doesn't), perhaps we can just get rid of the strips entirely? > > The change was because these strips are all for things read from the > manifest where the left side should already be stripped, and only the > newline needs to be removed from the right side of the string. > > The action parsing code does care about leading whitespace, but also cares > about trailing newlines. > > Removing the strips causes manifest unit test failures. I also experimented > with having the action parsing code simply ignore leading whitespace and > trailing whitespace and it turned out to be slower (go figure). Wacky. Okay. > > - line 609: why just for hardlinks now? > > I've added a comment here already, but it's because all actions except > hardlinks have had the leading '/' stripped from paths for quite some time > now during action __init__. Why we never stripped the leading '/' from > paths for hardlinks is a historical mystery at this point. How much it > actually matters since we have stripped them from the manifests that were > written out for a very long time also remains uncertain. I think your changeset ab6a2324f73a misunderstood what was going on w.r.t. hardlinks -- all paths were getting stripped up until that point, but since hardlinks are subclassed from links, there wasn't any code in hardlink.py to do that work. Sorry I didn't catch that back then. We could probably also torch 601-604, per the comment on line 597. If nothing else, setting attrs and aname should come before the comment, which only applies to 601-604. Thanks, Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
