Don't mess up formatting when saving trac.ini

[cl_111@…]

It would be nice to specify a ticket workflow in another file (perhaps under the conf/ directory) and have a setting in trac.ini which instructs Trac to parse that file for the workflow.

I mention this idea because the Trac Admin Module/Console, which allows settings to be edited through the Trac web interface, re-arranges the workflow and strips out any existing comments and line spacing.

For complex workflows, this can be a possible maintenance problem for those who are tasked in the future with introducing additional workflows into the existing workflow.

On the whole, I love the workflow concept - great work in introducing it into 0.11 guys! :)

[Remy Blank]

Instead of storing the workflow in a separate file, it would be better to fix the configuration parser to not mess up the formatting. This has already been suggested somewhere, but I can't find the reference at the moment.

[Christian Boos]

That was suggested by me in several ticket comments and mails. There's no ticket for tracking that yet, so let's use this one, as the removal of the comments and the re-ordering of sections and entries on write is one of my main concern.

The other concerns are:

• we can't take benefit from the natural ordering of entries in the .ini files, so we sometimes have to add that ordering explicitly (.order for custom fields, .order for milestone groups)
• there's some spurious parsing going on with the % character. That "feature" of the ConfigParser is going in the way most of the time (e.g. log formats). As it has never been advertised as a feature of our .ini files, we can safely drop this

Not to mention that we already reimplement much of the ConfigParser API to deal with the inherit feature, typed entries, etc. So I suspect that at this point there's not that much left to be done.

[Hans]

I think this is too important to postpone to milestone 2.0 so please triage this again. Removing the formatting and sorting the ticket-workflow section makes it impossible to maintain a complex workflow. I have to make a separate copy of trac.ini to restore the workflow section after making changed with the admin module.

Please see this as a confirmation that workflow really works great and deserves some respect in trac.ini!

Perhaps this issue should also be promoted from enhancement to defect.

regards,

Hans.

[Hans]

Found an acceptable workaround, You can put the workflow in a separate ini file and include this in trac.ini:

[inherit] file = workflow.ini

Seems to work ok, the admin console does not rewrite (and mess up) workflow.ini when changing something else.

I can live with this,

regards,

Hans.

ps. changed the type to defect though.

[Christian Boos]

[inherit] file = ... is normally used for a globally shared configuration, so this is more of a "hack" than a real solution.

I agree we could try to get this fixed sooner, but there are two approaches possible:

• don't mess up the formatting when saving trac.ini - this ticket
• use a dedicated .ini file for the workflow which could be achieved by allowing more than one file in the [inherit] file setting - that would be ticket #5525

[cl_111@…]

The inherit file approach should be left for it original purpose and not be overloaded to suit this defect. Besides, I have a quiet possibly unfounded concern that the parser may write back out to these files leaving us with the whole do not mess with the formating issue again.

Address the don't mess with the formatting approach sounds like it's high risk in terms of overall development time and debugging costs.

I'm in favour of any simpler approach - a dedicated workflow ini file. However there is the issue of the Trac Admin Module/Console UI. Can this be kept yet still fetch/persist changes only to the dedicated file?

[Christian Boos]

The other concerns are: …

• #7573

[osimons]

Following changes committed by #8290/[8243], trac.ini supports chaining of config files. For the original request of this ticket, that seems a good match for functionality letting you keep workflows in separate files and link them into the chain using [inherit] letting you use both a global, a workflow and a project trac.ini file for instance.

Any inherited file will never be 'messed up' - Trac only writes to the project trac.ini.

Config files are naturally unordered within sections, and keeping both order and comments while handling 'set()' with inheritance hierarchy effects is going to be quite a bit of code. I'm not sure it is worth the effort, and actually unsure if if we're ever going to be able to guarantee the expected result.

[Christian Boos]

Replying to osimons:

Config files are naturally unordered within sections,

Well, as noted above (comment:2), there are times we do want to get an ordering, and add an artificial one with .order entries.

And in Python 2.7, the ConfigParser will ​preserve the order, so I think there's nothing wrong with this proposal.

and keeping both order and comments while handling 'set()' with inheritance hierarchy effects is going to be quite a bit of code.

The way I see it, it's not a big effort: while reading an .ini file, we just keep track of what's in a line, either a comment, a section or an entry. When writing the .ini back (which we do already anyway), we just have to go through that list and write the comment and values back, except if the entries are now missing. Added entries are placed at the end of a section.

That's for the basic idea, the code would need to be a bit more complex if we allow the sections to be split and scattered in the .ini file.

[osimons]

Sure, it is more or less what I meant. But doing 'set' twice may well move the option on first set (same as already exists in hierarchy or Option registry), and then a new 'set' will add it to the bottom out of context to where it was when the comments were added and where it was no more than a second ago - an unexpected result as I see it, and likely as users see it if preserving order is a feature of the parser. I'm just unsure about adding a feature that will be 'mostly' correct.

[Christian Boos]

Replying to osimons:

But doing 'set' twice may well remove the option on first set (same as already exists in hierarchy or Option registry), and then a new 'set' will add it to the bottom

Took me quite some time to figure out there was a typo ;-)

Thanks for pointing out this special case. It would indeed be problematic not to handle it properly. So instead of leaving out the option in this case, we could write it as # ... = <inherited>, like we do for options set to None. On parsing, that would count as an entry location.

I agree with you that we have to get it right, but I'm also convinced it's worth the effort, as trac.ini files are meant to be edited and maintained by the administrators. Yes we support trac-admin config get/remove/set and more generally changing the Configuration file programmatically and saving it back, but the primary intent is nevertheless that those settings are made visible and editable by admins. For settings where this is not the case, we should rather use the database.

[osimons]

Heh. A typo indeed, and one of the 'not-so-obvious' variety…

I very often comment out lines and sections to remember alternate or default settings, so I guess all that is left of this issue is then to distringuish between your 'unset-by-comment' and manual 'comment-and-copy' by admins to make sure the right version of the line gets toggled :-)

Anyway, I'll leave this alone for now and wait for patches…

[Remy Blank]

Keeping ordering is the best long-term solution, but probably not for 0.12. Using the "inherit" chain for including arbitrary .ini files is a bit of a hack, and is made more difficult by the fact that it is a chain, rather than a list of includes.

How about allowing an arbitrary number of entries in the [inherit] section, and having a list of parents in Configuration instead of a single parent? As the key names for the section could be selected arbitrarily, we could just sort them alphabetically.

If nobody thinks this is a bad idea, I'll give it a try.

[osimons]

Replying to rblank:

Using the "inherit" chain for including arbitrary .ini files is a bit of a hack, and is made more difficult by the fact that it is a chain, rather than a list of includes.

Perhaps not surprising, but I don't see it as a "hack" - it follows from the way Configuration objects are in that all they know is themselves (content and file location) and possibly a link to a parent for traversal.

How about allowing an arbitrary number of entries in the [inherit] section, and having a list of parents in Configuration instead of a single parent? As the key names for the section could be selected arbitrarily, we could just sort them alphabetically.

What you are suggesting then is extending the chain to become a tree? Each config can list many parents - and with config being totally ignorant of any Trac environments, this then suggests that each parent may in turn have one or more parents?

Btw, I don't think the format supports more than one option key in each section, so in that case you would need to more alternative names - like supporting file as well as any file.something keys (or similar).

If your idea is to use parent as a way of "pulling in" options from other configurations into a 'master' configuration for the environment, then that will generate lots of new issues for me. One example is a PathOption with relative paths that can only be calculated from the .ini file location - and if pulled in at various Trac projects of different file system locations, it will be calculated wrong.

Note also that trac-admin initenv would need to modify the --inherit option to then support many parents (by for instance allowing the option to be used multiple times).

If nobody thinks this is a bad idea, I'll give it a try.

I'm may be missing the point, but don't quite see why it is a good idea? "Don't mess with formatting" as the original ticket stated is a valid purpose, but I don't quite see that multiple parents have anything to do with it?

[Remy Blank]

Replying to osimons:

Perhaps not surprising, but I don't see it as a "hack" - it follows from the way Configuration objects are in that all they know is themselves (content and file location) and possibly a link to a parent for traversal.

Sure, that's how it's implemented today. The thing that bothers me is that it forces you to "chain" your .ini files if you want to "include" more than one, resulting in a multi-level include, whereas being able to include multiple parents keeps your inclusion hierarchy flat.

What you are suggesting then is extending the chain to become a tree? Each config can list many parents - and with config being totally ignorant of any Trac environments, this then suggests that each parent may in turn have one or more parents?

Correct. It sounds scary when you put it like this, but what will happen is that your trac.ini will pull in the parents it needs, and those parents will generally not have parents themselves - a very flat tree.

Btw, I don't think the format supports more than one option key in each section, so in that case you would need to more alternative names - like supporting file as well as any file.something keys (or similar).

Yes, that's what I meant. You use different keys in the [inherit] section, and the order of inheritance is given by sorting the keys alphabetically.

If your idea is to use parent as a way of "pulling in" options from other configurations into a 'master' configuration for the environment, then that will generate lots of new issues for me. One example is a PathOption with relative paths that can only be calculated from the .ini file location - and if pulled in at various Trac projects of different file system locations, it will be calculated wrong.

Well, that issue is present already with the current inheritance mechanism, isn't it?

I'm may be missing the point, but don't quite see why it is a good idea? "Don't mess with formatting" as the original ticket stated is a valid purpose, but I don't quite see that multiple parents have anything to do with it?

Well, the initial suggestion to "don't mess with formatting" was "use [inherit] and place your workflow into a separate file". The most flexible mechanism to do that is to have a list of includes (as requested in #5525).

With a chain of includes, once the chains of two projects meet, all the direct parents meet as well, so if you want to have a separate workflow file and a global .ini, you have to do:

ProjectA/trac.ini --- ProjectA/workflow.ini --
                                              \
                                               -- global.ini
                                              /
ProjectB/trac.ini --- ProjectB/workflow.ini --

What if you want to have a set of workflows and a set of global configurations, from which each of your projects wants to inherit one? That's not possible with chains.

Sure, this scenario is a bit borderline, but my point is really that "a list of includes" is simpler to understand for the user than "a chain of includes" — and more flexible as well.

[Remy Blank]

I have added a proof of concept on #5525, so we should probably continue the discussion there.

[Christian Boos]

Refocusing on the initial Don't mess up formatting when saving trac.ini topic.

[Christian Boos]

I've got a patch along the lines of comment:10.

Besides preserving the order of sections and entries within the sections and the comments, the other advantages of getting rid of the ConfigParser are the following:

• we have now full unicode support, the code is simpler and more robust as we no longer have to convert back and forth between unicode (Trac) and UTF-8 str (ConfigParser),
• we don't have interference from the built-in %(...)s expansion (#9091), which will even allow us to do it "our way" (#7573, #8288, and of course #9674)
• we can have case-sensitive sections and entries

About the last point, case insensitivity always looked like a misfeature to me, the ​Wikipedia:INI_file entry doesn't say a word about this, and even if in Python it's the default, the fact that you can turn it off (optionxform = str) shows that it's not taken to be an universal rule. But if there's a convincing argument for putting it back, it would be easy to do.

The fancy update and re-enable commented out setting from comment:12 is not yet implemented, but could be done as a second step, if really needed.

There will be additional clean-ups to do:

• the old .overriden logic could certainly be simplified and even fixed: shouldn't Section.set(key, None) clear the entry from the current settings and let the value be inherited or be back to the default?
• the "reset old settings if writing fails" (._old_sections) logic is not yet supported
• add support for more syntax variants like a: b style, \ escapes, ; inline comments
• there's no API yet to retrieve the entries in order; probably the simplest way would be to adapt Section.iterate
• add unit tests for the new features (the existing ones pass modulo a few adaptations)

I think it's already a good starting point for asking for some feedback, as it works great in my testing.

[Christian Boos]

config: get rid of ConfigParser (patch applies on r10593)

[Remy Blank]

I haven't looked at the patch yet, but I'm glad you're working on this. It has always struck me as odd to use ConfigParser, when all we needed was a single function to parse an .ini file.

Does your patch by any chance also solve #10044, by giving a nice error message in case a config file isn't readable?

[Remy Blank]

I missed this ticket because it wasn't scheduled for 0.13. I'm going to review it in the next few days.

[Remy Blank]

Replying to cboos:

I've got a patch along the lines of comment:10.

I get 7 failures, 6 in intertrac test cases, and one in the doctest of trac.wiki.api.WikiSystem.get_resource_description(). Could the former be due to end-of-line comment handling?

About the last point, case insensitivity always looked like a misfeature to me, the ​Wikipedia:INI_file entry doesn't say a word about this, and even if in Python it's the default, the fact that you can turn it off (optionxform = str) shows that it's not taken to be an universal rule. But if there's a convincing argument for putting it back, it would be easy to do.

Same here, I would prefer a case-sensitive trac.ini. But changing that will break a lot of installations, so it may be wiser to leave that as-is.

• the old .overriden logic could certainly be simplified and even fixed: shouldn't Section.set(key, None) clear the entry from the current settings and let the value be inherited or be back to the default?

Do we ever set an option to None in existing code? If not, then it wouldn't even be a backward-incompatible change. Then again, we could also add a new method Section.clear(key) to be more explicit about it, and leave the current behavior in Section.set().

• the "reset old settings if writing fails" (._old_sections) logic is not yet supported
• add support for more syntax variants like a: b style, \ escapes, ; inline comments

Those two are important. We should at least support all the variants that ConfigParser supported.

• there's no API yet to retrieve the entries in order; probably the simplest way would be to adapt Section.iterate

Yes, Section.iterate() should just iterate in order. Would it also make sense to add (in-order) iteration to Configuration? Currently, there's only .sections(), and it returns a list of section names sorted alphabetically.

A few questions and comments:

• Any reason why you use StringIO and not cStringIO? AFAIK, the latter also supports unicode strings, but you have to be careful not to mix string types.
• In _read(), I would prefer using trac.util.read_file() rather than file(self.filename, 'rb').read().
• You have removed the possibility for keys and values to be UTF-8. Is this a backward-compatibility issue?
• When saving, do you preserve the indentation of follow-up lines? My understanding is that follow-up line indentation is always reduced to a single space. It would be nice to keep the indentation, at least for unmodified entries, but it's not critical, so if it's too difficult, don't bother.

Good first shot! I'd really like to see this in 0.13.

[osimons]

Some feedback just based on what I read in comments:

1. Please don't make sections and options case-sensitive. Lots of things will break, and using ticket:5535 as example the errors are likely to be obscure, non-obvious and may often go unnoticed with unknown consequences. At this stage it just isn't a good idea, IMHO.

2. Overriding and clearing options is a somewhat more complex issue than just removing the option from the file, and comment:17:ticket:3037 (+ before and after) is a brief discussion of this. What is needed is both the ability to clear as in "revert to parent or registy" and clear as in "remove any effect of this option in parent or registry".

3. Does removing ConfigParser also set us on a path to be non-compatible with its parsing? For the sake of every piece of management code or plugins that manipulate config files directly using Python standard lib, there should be a test that confirms that any new adopted features still remain parseable.

[Christian Boos]

Replying to osimons:

1. Please don't make sections and options case-sensitive.

Sure, this is the obvious conservative choice. But we should nevertheless warn people they shouldn't rely on that, as it may change at some point in the future ("1.0" ;-) ).

2. Overriding and clearing options (comment:17:ticket:3037)

I agree it makes sense to address that issue jointly. I'll move the milestone.

3. Does removing ConfigParser also set us on a path to be non-compatible with its parsing?

I don't think so, the expected format is so simple it should be possible to remain 100% compatible. OTOH, using ConfigParser API directly instead of our own should be clearly documented as unsupported, as this is very much likely to break other things (trac.ini chaining via [inherited] file, point 2. above, etc.)

[Christian Boos]

Replying to rblank:

Replying to cboos:

I've got a patch along the lines of comment:10.

I get 7 failures, 6 in intertrac test cases, and one in the doctest of trac.wiki.api.WikiSystem.get_resource_description(). Could the former be due to end-of-line comment handling?

It looks like a case sensitivity issue (t vs. T) for the first 6 ones. The last one could probably be just adapted to the newly returned value, if we decide it's a good thing to return the actual value that a get() would yield. In the latter case, Configuration.set() should propagate Section.set(), for consistency.

About the last point, case insensitivity always looked like a misfeature to me, the ​Wikipedia:INI_file entry doesn't say a word about this, and even if in Python it's the default, the fact that you can turn it off (optionxform = str) shows that it's not taken to be an universal rule. But if there's a convincing argument for putting it back, it would be easy to do.

Same here, I would prefer a case-sensitive trac.ini. But changing that will break a lot of installations, so it may be wiser to leave that as-is.

See my answer to Simon, let's make it case insensitive for now, but opt for case sensitivity in the long term.

• the old .overriden logic could certainly be simplified and even fixed: shouldn't Section.set(key, None) clear the entry from the current settings and let the value be inherited or be back to the default?

Do we ever set an option to None in existing code? If not, then it wouldn't even be a backward-incompatible change. Then again, we could also add a new method Section.clear(key) to be more explicit about it, and leave the current behavior in Section.set().

Right, see my comment on #3037, though there I suggest negate() rather than clear(). I find clear() too close to remove(), in the sense you don't expect to "see" anything left after a clearing, whereas negate() suggests something "active".

• the "reset old settings if writing fails" (._old_sections) logic is not yet supported
• add support for more syntax variants like a: b style, \ escapes, ; inline comments

Those two are important. We should at least support all the variants that ConfigParser supported.

The first point is not so obvious to me: if we can't write back a change to the .ini file and then decide to "revert" all changes done since the last write, why not simply just re-read the .ini file (i.e. parse_if_needed(true))?

• there's no API yet to retrieve the entries in order; probably the simplest way would be to adapt Section.iterate

Yes, Section.iterate() should just iterate in order.

OK.

Would it also make sense to add (in-order) iteration to Configuration? Currently, there's only .sections(), and it returns a list of section names sorted alphabetically.

I don't see an use case for this yet. We could add it when needed (make an API note in sections() saying the order is not yet defined).

A few questions and comments:

• Any reason why you use StringIO and not cStringIO? AFAIK, the latter also supports unicode strings, but you have to be careful not to mix string types.

I always assumed it did not support unicode objects, as most of the Trac code was implying this. Let's check: the ​2.5.2 cStringIO docs say:

Unlike the memory files implemented by the StringIO module, those provided by this module are not able to accept Unicode strings that cannot be encoded as plain ASCII strings.

Python 2.5.2 (r252:60911, Feb 21 2008, 13:17:27) [MSC v.1400 64 bit (AMD64)] on win32
...
>>> from cStringIO import StringIO
>>> buf = StringIO()
>>> buf.write(u"été")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe9' in position 0: ordinal not in range(128)

So, no ;-)

• In _read(), I would prefer using trac.util.read_file() rather than file(self.filename, 'rb').read().

Sure (just noticing we should probably use mode='rb' as the default, there; won't hurt and is more robust on Windows).

• You have removed the possibility for keys and values to be UTF-8. Is this a backward-compatibility issue?

I don't think so, in the sense that the whole UTF-8 code which I removed was meant to workaround the lack of unicode support in ConfigParser (r8458). Now that we're in control, it's unicode all the way to the IO boundary, where we do decode/encode from/to UTF-8.

• When saving, do you preserve the indentation of follow-up lines? My understanding is that follow-up line indentation is always reduced to a single space. It would be nice to keep the indentation, at least for unmodified entries, but it's not critical, so if it's too difficult, don't bother.

I can have a try.

Good first shot! I'd really like to see this in 0.13.

Yes, that was really just a first shot in order to get some base version running that we could discuss. Thanks to both of you for the review.

[Remy Blank]

Replying to cboos:

The first point is not so obvious to me: if we can't write back a change to the .ini file and then decide to "revert" all changes done since the last write, why not simply just re-read the .ini file (i.e. parse_if_needed(true))?

That's fine, too, as long as we get back to a consistent state if saving fails (which was the whole point of the ._old_sections logic).

I always assumed it did not support unicode objects, as most of the Trac code was implying this. Let's check: the ​2.5.2 cStringIO docs say:

I was remembering wrong, due to the following quirk:

>>> from cStringIO import StringIO
>>> s = StringIO(u"été")
>>> s.getvalue()
'\xe9\x00t\x00\xe9\x00'

So it accepts a unicode input at construction time, but silently encodes it as UTF-8. Sorry, my bad.

[Christian Boos]

Replying to cboos:

Replying to rblank:

• You have removed the possibility for keys and values to be UTF-8. Is this a backward-compatibility issue?

I don't think so, in the sense that the whole UTF-8 code which I removed was meant to workaround the lack of unicode support in ConfigParser (r8458). Now that we're in control, it's unicode all the way to the IO boundary, where we do decode/encode from/to UTF-8.

Oh, you were probably referring to the unit tests lines I removed, where we directly store UTF-8 encoded str as values. This code dates back from r3556, in the 0.10dev days, when migration to unicode happened. It was probably done for compatibility reasons with the pre-0.10 API, so this can safely be discarded now.

[Remy Blank]

Preparing for 1.0.

[Ryan J Ollos]

Normalizing keywords.

[Ryan J Ollos]

Moved ticket assigned to next-stable-1.0.x since maintenance of 1.0.x is coming to a close. Please move the ticket back if it's critical to fix on 1.0.x.

[Ryan J Ollos]

Milestone renamed