I've updated the webrev with something that I think reflects the comments that you've made. Let me know how this looks:
http://cr.opensolaris.org/~johansen/webrev-6339/ Thanks, -j On Tue, Feb 03, 2009 at 05:10:31PM -0800, Danek Duvall wrote: > On Tue, Feb 03, 2009 at 04:48:44PM -0800, [email protected] wrote: > > > Thanks for all of the reviews, but I jumped the gun. The first fix > > didn't pass the test suite. I've re-whacked this so it works correctly. > > The new fix is equally concise, but actually passes the test suite. > > I updated the webrev in place this time, since the whole thing is so > > small. > > So from what I can tell, the problem was that you weren't regenerating the > catalog cache with the removed catalog. That seems to be happening in the > rewrite, but I don't think you're doing it in a terribly clear way. > > You're relying on the behavior of cache_catalogs() to ignore a missing > catalog, and, like Shawn said, you're relying on origin_changed being True, > which makes no sense. It's like you're shoehorning multiple meanings into > the variable just to make the code more compact. > > I think it would be less confusing if you went back to your original > change, and had a separate check for disabled down either between line 753 > and 754 (putting newauth into the auths dict and the save_config() call), > or after the latter, re-caching the catalog there. > > Danek > _______________________________________________ > pkg-discuss mailing list > [email protected] > http://mail.opensolaris.org/mailman/listinfo/pkg-discuss _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
