Do not escape opening brackets in middle of filter text while serializing

[mjethani]

Background

Currently the serializeFilters function in lib/subscriptionClasses.js escapes any occurrence of an opening bracket in a filter (e.g. ##a[href^="http://ads."]) with a backslash (\). At the time of parsing patterns.ini, the INIParser class unescapes the opening bracket before parsing the filter.

Escaping of an opening bracket is relevant only when it occurs at the beginning of a line optionally preceded only by whitespace; therefore, this escaping is mostly unnecessary.

What to change

In the serializeFilters function in lib/subscriptionClasses.js, replace filter.text.replace(/\[/g, "\\[") with filter.text.replace(/^(\s*)\[/, "$1\\[") or some equivalent.

After a few releases, modify INIParser to only unescape the opening bracket when it occurs at the beginning of the line (optionally preceded by whitespace).

(Question: Should we make both changes at the same time and bump the file format version number instead?)

[mjethani]

My bad! It's obviously not a problem for filters because the escaping and unescaping is symmetrical (I got confused by my local changes). Nevertheless we don't have to escape every opening bracket, only the ones that occur at the beginning of a line. This would likely speed up the loading of the initial subscriptions in INIParser.

[sergz]

After a few releases, modify INIParser to only unescape the opening bracket when it occurs at the beginning of the line (optionally preceded by whitespace).

BTW, what will happen with custom filters?

(Question: Should we make both changes at the same time and bump the file format version number instead?)

It seems that the version should be bumped when we start to save and support filters in the new format.

Additionally FYI some of our partners have a very long update delay and if it's not a huge burden it would be great to keep the backward compatibility to a relatively longer time.

[mjethani]

Replying to sergz:

After a few releases, modify INIParser to only unescape the opening bracket when it occurs at the beginning of the line (optionally preceded by whitespace).

BTW, what will happen with custom filters?

Sorry, what do you mean by custom filters?

(Question: Should we make both changes at the same time and bump the file format version number instead?)

It seems that the version should be bumped when we start to save and support filters in the new format.

Additionally FYI some of our partners have a very long update delay and if it's not a huge burden it would be great to keep the backward compatibility to a relatively longer time.

OK, in that case we can read the version 5 INI file using the old parsing logic and save it as version 6 the next time; this way everyone gets upgraded cleanly. We can leave the version 5 parsing in for a long time.

[mjethani]

Based on the above discussion and some of my own exploration, it looks like the best way to make any change here is to bump the file format version number and then be able to read the old format and write it to the new format. If we're going to bump the version number, it also gives us the opportunity to make more performance-related changes to the file format, the exact details of which would be a separate discussion.

I am closing this issue now.