"Shell patterns" broken beyond repair

[wjaguar]

Function mc_search__glob_translate_to_regex() in lib/search/glob.c is horribly broken, and mishandles a number of trivial patterns such as:
- an inverted character class: "[^abc]" or "[!abc]"
- a freestanding comma: "abc,def"
- a literal brace: "{" or "}"
- an incorrectly formed brace expansion: "{abc}"
- a sequence expression: "{5..10}" or "{a..f}"

Also, a nested brace expansion: "a{b,c{d,e}}f" will produce capturing parentheses for every pair of braces, instead of only the outermost pair.

All this because current code is far too simple-minded to correctly convert Bash patterns into PCRE regexes, and needs a complete rewrite.

[boris]

Please, look at the proposed patch. It does not intend to fix all Bash patterns (I believe mc never claimed to support all kinds of them), but it fixes some issues.
Namely, backslash-escaped metacharacter like {}*? will remain in the pattern (with the current code it is just stripped). Second, comma will be transformed to | only inside a group.

[slavazanko]

Created branch 2309_shell_patterns
Initial changeset:0eca32a852593c9bda46b48a8547af7d306b60b2

Review please.

[andrew_b]

Approved with simple fixup.

[slavazanko]

Merged to master [e5ec837b98663921f44349369f1a38ec8bbd50fe]:

git log --pretty=oneline f4faf60..6ca737d

[mooffie]

Ticket #2577 has been marked as a duplicate of this ticket.

[zaytsev]

Ticket #2379 has been marked as a duplicate of this ticket.

[zaytsev]

Ticket #3350 has been marked as a duplicate of this ticket.

[wjaguar]

This STILL REMAINS broken. Yes, after all these years. The fix has done nothing for inverted character classes, they still do NOT work, instead being treated as regular ones.

Naturally, all uses of MC_SEARCH_T_GLOB are affected. A couple examples:

• searching for "[^g]" or "[!g]" finds a letter "g";
• selecting "[^g]*" or "[!g]*" in mc's lib/search dir select "glob.c" instead of everything but.

All the listed problems with brace expansion also remain:

• unpaired literal braces are misinterpreted and break the RE:
  you cannot select, say, "{" or "*.{" despite files matching the pattern being there;
• a text in braces w/o a comma is misinterpreted as without braces:
  trying to select "{abc}*" selects "abc";
• a sequence expression is misinterpreted as literal text w/o braces:
  trying to select "{a..h}*" selects "a..h" and does not select "glob.c" or "hex.c";
• nested braces now just plain do not work:
  "{{a,b},c}*" selects "a,c" and "b,c", but neither "abc" nor "ccc".

[zaytsev]

I guess nobody really uses anything except *, ? and [ch]... so it's not like the interest is overwhelming. However, feel free to submit a patch with tests.

[wjaguar]

Replying to zaytsev:

Do remember that I can (and do since forever) patch my own copy of mc, very easily and with none of bureaucracy. My doing anything extra is an act of kindness.
A snooty attitude is not for the times when you are asking for a patch for a bug you are unable to fix.

[andrew_b]

Replying to wjaguar:

Do remember that I can (and do since forever) patch my own copy of mc, very easily and with none of bureaucracy.

No problem. But what are you doing here?

[zaytsev]

Do remember that I can (and do since forever) patch my own copy of mc, very easily and with none of bureaucracy. My doing anything extra is an act of kindness.

Do remember that I'm not contractually obligated to maintain mc, and my doing anything at all is an act of kindness.

A snooty attitude is not for the times when you are asking for a patch for a bug you are unable to fix.

A snooty attitude is the one you're exhibiting right now.

I'm working on triaging the tickets, and so you were probably triggered by an email notification. Immediately, you felt the urge to remind us that THE BUG YOU REPORTED (in all caps) still hasn't been fixed after all these years, even though it's a valid bug. Which is true. As are another ~600 open (and often valid) bugs.

I've observed that the priority doesn't seem too high, since there's been no activity in this ticket for 10 years. However, we would welcome a patch, but we need tests to consider applying it, because exactly these kinds of fixes tend to introduce major breakages.

Now you go on to threaten me with maintaining mc on your own computer and never talking to us again, oh my! You, random keyboard warrior on the Internet, by writing something here, are performing an act of kindness that I don't seem to appreciate. Don't you think this would have been funny if it wasn't so sad?

I guess I won't get my message across though... at least I tried :)

[wjaguar]

Replying to andrew_b:

No problem. But what are you doing here?

Documenting your decade old bug which was misleadingly labeled fixed, so that other users know what to expect. Should be obvious to you if you are here playing developer.

Also, seeing whether you guys are worth spending any more of my time on, these days.

Replying to zaytsev:

Do remember that I'm not contractually obligated to maintain mc, and my doing anything at all is an act of kindness.

That entirely depends on the quality of the results.
For what I see now in mc_search__glob_translate_to_regex(), an act of kindness would be to put it out of its misery. While you personally had no hand in its creation, still it serves as an example that sometimes NOT doing anything at all, is MORE of a kindness than doing the wildly wrong thing and leaving it for other people to have problems with.

A snooty attitude is the one you're exhibiting right now.

It is not I who let bugs languish for decades.

I'm working on triaging the tickets, and so you were probably triggered by an email notification. Immediately, you felt the urge to remind us that THE BUG YOU REPORTED (in all caps) still hasn't been fixed after all these years, even though it's a valid bug. Which is true. As are another ~600 open (and often valid) bugs.

Given that I singlehandedly maintain a project with about half as much C code as this one, and have ZERO open valid bugs in it, my heart fair bleeds for you two.

And from the reception I got, it is very obvious WHY it is only you two and no one else.

I've observed that the priority doesn't seem too high, since there's been no activity in this ticket for 10 years. However, we would welcome a patch, but we need tests to consider applying it, because exactly these kinds of fixes tend to introduce major breakages.

Then I guess you should be capable of writing said tests. It is merely text strings with patterns you wish to not break, it will take you even less time and effort than inventing excuses why you are too illustrous to be doing anything at all except playing tin god.

Given your apparent failure at understanding plain text, once again: I CANNOT CARE LESS how many years your toy bureaucracy will "consider applying" the patch this time around. For #2027 it was 5 years, and your acting makes it plainly obvious the process has not improved since then.

Now you go on to threaten me with maintaining mc on your own computer and never talking to us again, oh my! You, random keyboard warrior on the Internet,

ROFL. Your attempt at puffing up is so hilariously misplaced. Поиском научись пользоваться, чудо. Хотя бы на собственном сайте.

I guess I won't get my message across though... at least I tried :)

​https://www.azquotes.com/quote/176074
Meditate on it.

[zaytsev]

Meditate on it.

I know that quote and that's why I took the time to respond. Our conversation will remain public for everyone to read and draw their own conclusions about the participants.

It seems that our conclusions about each other have little chance of becoming any more compatible though, so it's not worth replying further.

[wjaguar]

Do with this what you wish.
The patch is for everyone to use (or not) and, as you said, "draw their own conclusions about the participants". ;)

[wjaguar]

Proper globs for 4.8.27 (the version which I use)

[wjaguar]

Proper globs for 4.8.33 (the latest version at the moment)

[wjaguar]

My battery of tests for proper globs, formatted for mc's test rig

[andrew_b]

Thanks!
But unfortunately, as usually, patch authors are totally ignore the project coding guidelines and indentation rules.

[zaytsev]

Hi Andrew, I think this is one of the rarest examples where we have a patch at all with TESTS, even ignoring the indentation and coding guidelines. This is really one of the best attempts in that direction that I've seen. Let's try to get it in after the migration.

[andrew_b]

Patch is too dirty.

GString *res;

res = g_string_sized_new(n);
res->len = convert_mask(mask, l, map, res->str);

What?

GString *res;

dest = res->str;
dest += snprintf(dest, l, cnt > 9 ? "${%d}" : "\\%d", cnt);

Direct, ignoring API, modification of GLib data structures is a bad idea.

[zaytsev]

Ticket #2577 has been marked as a duplicate of this ticket.