Code review comment for lp:~thomir-deactivatedaccount/bzr/add-global-config

Revision history for this message
Vincent Ladeuil (vila) wrote :

Roughly speaking (summarising my previous remarks here): it's a bit early to land as is.

It introduces one more config file that will be accessed for each option query (http://pad.lv/832042).

It introduces a new file with a semantic which is not the one we want (regarding section filtering/ordering http://pad.lv/832046).

It doesn't address the defaults/overrides distinction we want to add there (which will require two distinct files, adding another queried config file for each option)

The rest seems fine, in particular:

We need to have platform specific paths for the system-wide config file.

We need to add this system-wide file at the right place in the stack definitions.

The new store *is* hooked in the right places but since this mp is based on an old trunk version, this can observed only for already migrated options. As such, we can't even measure its impact with -Econfig_stats.

So I'd say, either we land the path handling stuff only or we wait for the bugs mentioned above to be fixed before reconsidering this mp.

review: Needs Fixing

« Back to merge proposal