Multiple bugs in trac.config

[Alec Thomas]

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.

[Alec Thomas]

Configuration module fixes

[Alec Thomas]

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.

[Alec Thomas]

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?

[Alec Thomas]

Previous patch plus save() now only saves modified values

[Alec Thomas]

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

[Christopher Lenz]

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

[Christopher Lenz]

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

[Alec Thomas]

I'll take a look at point 3.

[Alec Thomas]

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.

[Matthew Good]

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.

[Alec Thomas]

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)

[Christian Boos]

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)

[Christian Boos]

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

[Matthew Good]

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.

[Christian Boos]

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.

[Christian Boos]

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.

[osimons]

I'll look at this (at some stage).

[Christian Boos]

… or me, while working on the #7378 topic.

Let's start by saying we need some clearer terminology and hence a more explicit API:

• remove() a setting should be just like removing a setting from the .ini file. It's no longer specified at the level of that .ini file, so the actual value comes from the parent(s) .ini or the ultimately from the default
• now as mentioned above we also need to be able to simulate a removal of a setting present in a parent .ini file, and this without having to change the value, which wouldn't be enough anyway if what we want to dispose of is the presence of the key itself or by extension of an entire section. The idea I proposed in comment:18 would be adequate for this. Furthermore it easily extends to marking a whole section as non-existing (e.g. [!milestone-groups]); the API for this could be Section.negate(key=None) (if None, the whole section is negated). I think this fits well with both the intended meaning and the suggested use of ! as a key or section prefix.

[Remy Blank]

Preparing for 1.0.

[Ryan J Ollos]

Milestone renamed

[Ryan J Ollos]

[14275] adds test coverage for some of the issues cited in this thread. Eventually I'd like to separate the Configuration class tests fully from the filesystem integration tests. I'll address the feature in comment:20 before closing.

[Christian Boos]

Moving to some later 1.3.x version.

[Ryan J Ollos]

Milestone renamed