> 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.
Of course we've never written code that was unclear. :P
I have what I hope is a better solution. This is a diff against the
last change that ought to make it clearer what's going on. I don't like
the idea of duplicating the catalog removal into two places in that
routine. The create/update cases already make things cluttered, at
least to me.
diff -r 633e34682dfa src/modules/client/image.py
--- a/src/modules/client/image.py Tue Feb 03 16:47:06 2009 -0800
+++ b/src/modules/client/image.py Tue Feb 03 17:18:54 2009 -0800
@@ -696,7 +696,8 @@
self.history.operation_name = "set-authority"
auths = self.cfg_cache.authorities
- origin_changed = False
+ refresh_catalog = False
+ purge_catalog = False
if auth_name in auths:
# Copy old authority information to new entry.
@@ -707,7 +708,7 @@
if origin_url:
newauth["origin"] = \
misc.url_affix_trailing_slash(origin_url)
- origin_changed = True
+ refresh_catalog = True
if ssl_key:
newauth["ssl_key"] = ssl_key
if ssl_cert:
@@ -720,7 +721,10 @@
assert(not disabled or \
auth_name != self.get_default_authority())
newauth["disabled"] = disabled
- origin_changed = True
+ if disabled:
+ purge_catalog = True
+ else:
+ refresh_catalog = True
else:
newauth = {}
@@ -736,12 +740,13 @@
if disabled is None:
disabled = False
newauth["disabled"] = disabled
- origin_changed = True
+ if not newauth["disabled"]:
+ refresh_catalog = True
- if origin_changed and refresh_allowed:
+ if refresh_catalog or purge_catalog:
self.destroy_catalog(auth_name)
self.destroy_catalog_cache()
- if not newauth["disabled"]:
+ if refresh_catalog and refresh_allowed:
self.retrieve_catalogs(full_refresh=True,
auths=[newauth])
else:
self.cache_catalogs()
Does this do a better job of addressing the confusion? I think this
spells things out more explicitly than either of the two previous
solutions.
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
