On Tue, Feb 03, 2009 at 06:34:04PM -0800, [email protected] wrote: > 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
Any further comments? I'd like to get this integrated soon. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
