Multiple bugs in trac.config

[athomas]

While testing some global configuration changes, I noticed they weren't working with InterTrac. After some investigation it seems there are a few problems with the current config module:

1. Section.__contains__() does not use the global config, nor does Section.get()
2. ConfigParser ignores case, but config.Configuration._defaults does not.
3. ConfigParser.read() overlays previous configuration. The issue with this is that we use read() when a configuration modification is detected. Deleted options are not removed.

The attached patch fixes the first two issues. It also updates one of the config unit tests, as it can now fail if a global configuration file has entries (NB. I'm not sure whether the fix in the unit test is too hackish?)

The third issue would require some kind of dirty configuration list, for both the site and env configs. I'm not sure the issue is bad enough to warrant the ugliness.

[athomas]

Configuration module fixes

[athomas]

Thinking about it, the fix could also break other unit tests that rely on the global config not being loaded so perhaps a test-suite wide solution is preferable.

[athomas]

And on a semi-related note, what's the opinion on not saving values to the env config that are either the Trac default or the site config default? This would make the env config a lot cleaner I think, although perhaps having it populated is desirable as hints to the user?

[athomas]

Previous patch plus save() now only saves modified values

[athomas]

I think only saving options that differ from the default is a good idea. If users need examples there's always trac.ini.sample.

[cmlenz]

Agreed about only saving values that differ from the default. I'll try the patch later.

[cmlenz]

Patch integrated in [3186]. Leaving this open for point 3).

[athomas]

I'll take a look at point 3.

[athomas]

This is tricky when the global config has an option set but the in-memory option is remove()ed. All is fine until the config is saved and reloaded. The environments config will not have an entry for the option, so the global default will be used, consequently ignoring the remove().

Saving the list of options that have been removed to a special [removed_options] section in the environment's config would solve this, but is spectacularly ugly.

I propose we simply remove the Configuration.remove() method for 0.10. It is not used at all in Trac core, but is used by WebAdmin and two plugins on TracHacks (HackInstallPlugin and GeneralLinkSyntaxPlugin). From a cursory examination I suspect all of these can be changed to use sentinel values or defaults.

[mgood]

Why should't it fall back on the global config when an option is removed? This would be consistent with the expected behavior when an option is deleted from the file directly.

[athomas]

Replying to mgood:

Why should't it fall back on the global config when an option is removed? This would be consistent with the expected behavior when an option is deleted from the file directly.

That is definitely one way to look at it, and when done manually, probably expected. But from a programmatic perspective I think remove()ing an item should be more explicit. eg. WebAdmin has self.config.remove('logging', 'log_level') which may or may not actually reset the log level, depending on the global setting. If remove() can't be relied on to actually do the job, why have it? If its behaviour is to reset to the default, perhaps it would be preferable to rename it to set_default() or similar.

(also unclosing this ticket, but lowering priority/severity as now the first two issues are closed it's not that much of a problem)

[cboos]

I also think that removing an option should only remove it in the given config file.

I've not yet looked into the details of the WebAdmin, but ideally one should be able to:

• edit either the local or the global configuration entries (defaults to editing the local entries)
• make a difference between setting an option to an empty value (i.e. override) and unsetting an option (i.e. revert to default)

[cboos]

Doesn't the fix for #3620 (i.e. r3744) also help here?

[mgood]

Replying to cboos:

Doesn't the fix for #3620 (i.e. r3744) also help here?

No, I think these are all separate issues.

[simon-code@…]

I have just added ticket #5135 with patch that has some bearing on this issue, specifically the senctence in point 3 in description: "Deleted options are not removed."

It is more a multi-process issue in my use context (allthough likely very common), but it has bearing on the fact that remove() also needs to handle better a third case "unset" (in addition to "empty" and "default"). Current parsing has no idea on how to completely "unset" an existing setting - removing all traces of it.

[cboos]

This mostly works now in 0.11 and it's not going to change in 0.10-stable.

[osimons]

Replying to cboos:

This mostly works now in 0.11 and it's not going to change in 0.10-stable.

Mostly, yes. But not quite. I've left it open due to the summary in comment:15 - we need the ability to unset working variables to make administration of larger Trac installs more flexible.

Take the example of a default [milestone-groups] implemented in a shared/global trac.ini. In my local instance I would like the ability to remove active settings and split it into assigned and unsassigned for instance. There currently is no way to do this, and 'Active ticket' will appear in view regardless. That is what is left for this ticket - if we deem that important.

One way could be to use ! as the value for the setting, and if so we keep it the setting behind the scenes as a value for writing to updated config, but when using config.get() and config.has_option() we report it as if it did not exist anywhere. So to nullify any effect of a shared config from my example, I would include local settings like:

[milestone-groups]

active = !
active.order = !
active.css_class = !

; New settings here...

The same applies to other groups such as [ticket-custom] and [ticket-workflow] that will both 'bleed' into the local config without any way to fully remove settings. They can be redefined, but not removed.

[cboos]

Ok, but I'd prefer:

[milestone-groups]

!active =
!active.order =
!active.css_class =

as it would be more general: it would also work for the entries where "!" is valid content. It is also easier to explain: the key is negated, the entry itself is disabled.

Of course, we would have to forbid having a leading "!" in valid configuration keys, but I think that's a guideline that is easier to enforce than not using "!" in values, as it concerns us and plugin writers, not Trac administrators.