Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mcedit anchored regex line matching inconsistencies #4072

Closed
mc-butler opened this issue Mar 28, 2020 · 22 comments
Closed

mcedit anchored regex line matching inconsistencies #4072

mc-butler opened this issue Mar 28, 2020 · 22 comments
Assignees
@aborodin
Labels
area: mcedit mcedit, the built-in text editor prio: low Minor problem or easily worked around ver: 4.8.24 Reproducible in version 4.8.24
Milestone
4.8.34

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/4072
Reporter ply (ply@…-online.hu)

The editor's F4/Replace function with regex matching shows some broken behavior when the regex begins with the line start anchor, and matches whole lines. Try specifying "^.*" for source and "X\0" for target pattern to try to prefix each line with an "X" to see the following behaviors:

  • For consecutive matching lines the replacement only happens on every other line, skipping each candidate that follows a matched line. Maybe matches on consecutive lines are considered overlapping? They shouldn't be.
  • When such a regex fully matches the current line, and the cursor is on the first column, the first match is the following line only. Except when the cursor is on the first line, then the first line also matches. I'd expect the current line to match even if the former cases.

Note

Original attachments:
@mc-butler
Copy link
Author

Changed by ply (ply@…-online.hu) on Mar 28, 2020 at 23:09 UTC (comment 1)

The source regex was supposed to be caret-dot-asterisk, but Trac apparently interpreted it...

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Mar 29, 2020 at 4:56 UTC (comment 2)

  • Description edited

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 9, 2025 at 10:05 UTC (comment 3)

  • Milestone changed from Future Releases to 4.8.34
  • Owner set to andrew_b
  • Status changed from new to accepted
  • Branch state changed from no branch to on review

Branch: 4072_mcedit_replace_bol
Initial [cf686c5fc3f9a7fd593dd3342a5056108a2b6d0d]

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 9, 2025 at 10:52 UTC (comment 4)

Nitpick: Ticket 4072 - # is missing.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 9, 2025 at 11:01 UTC

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 9, 2025 at 11:01 UTC

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 9, 2025 at 11:02 UTC (comment 5)

I'm happy to see new tests, but they fail on Ubuntu and macOS in CI. Probably because of PCRE2? Build logs are attached or available from GitHub.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 9, 2025 at 12:53 UTC (comment 5.6)

  • Branch state changed from on review to on rework

Replying to zaytsev:

Probably because of PCRE2?

Yes, with PCRE2 replace works unexpected at end of file.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 22, 2025 at 8:39 UTC (comment 7)

So, now I have tests passed with glib and pcre2 search engines but not with pcre.

The pcre library is at version 8.45. This version of PCRE is now at end of life, and is no longer being actively maintained. Version 8.45 is expected to be the final release of the older PCRE library. The latest release was in 2021-06-22.

Honestly speaking, I don't want to waste my time with pcre. I propose to drop the pcre engine from mc.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 22, 2025 at 9:56 UTC (comment 8)

Honestly speaking, I don't want to waste my time with pcre. I propose to drop the pcre engine from mc.

I agree. But actually, I wonder why we need direct support for PCRE2 at all. Do you know if there are good reasons for it? I thought that glib provides regex support, so we can just rely on that.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 22, 2025 at 10:31 UTC (comment 8.9)

Replying to zaytsev:

Honestly speaking, I don't want to waste my time with pcre. I propose to drop the pcre engine from mc.

I agree. But actually, I wonder why we need direct support for PCRE2 at all. Do you know if there are good reasons for it?

Some historical reasons. GRegex as PCRE wrapper was introduced in GLib-2.14. PCRE was required for MC on OSes with GLib < 2.14. Currently MC requires GLib >= 2.32. Since 2.73.2 (released at 2022-07-08), GRegex is based on PCRE2.

I thought that glib provides regex support, so we can just rely on that.

I agree, we can remove PCRE and PCRE2 from MC and use GLib only.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 22, 2025 at 10:51 UTC (comment 10)

I agree, we can remove PCRE and PCRE2 from MC and use GLib only.

Sounds great to me. Your explanation makes sense. Please feel free to do so in this ticket, or create another one. Thank you very much!

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 22, 2025 at 17:24 UTC (comment 10.11)

Replying to zaytsev:

Please feel free to do so in this ticket, or create another one.

#4655

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 22, 2025 at 18:18 UTC (comment 12)

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 23, 2025 at 6:09 UTC

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 23, 2025 at 6:29 UTC (comment 14)

  • Branch state changed from on rework to on review

Rebased to current master.
Branch: 4072_mcedit_replace_bol
Initial [760f09fa85116ee8d570cd52acc587cd8d64282f]

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 23, 2025 at 8:20 UTC

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 23, 2025 at 8:20 UTC (comment 15)

Now tests still fail on macOS.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 23, 2025 at 9:11 UTC (comment 16)

Fixed.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 23, 2025 at 14:01 UTC (comment 17)

  • Branch state changed from on review to approved
  • Votes set to zaytsev

In the first commit, "Ticket 4072" is missing #. Otherwise, it looks good and I'm happy about the tests. Sorry I can't give a more satisfying review.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 23, 2025 at 15:55 UTC (comment 18)

  • Resolution set to fixed
  • Votes changed from zaytsev to committed-master
  • Branch state changed from approved to merged
  • Status changed from accepted to testing

Merged to master: [be21a83].

git log --oneline 4d02bb20a..be21a83f1

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 23, 2025 at 15:56 UTC (comment 19)

  • Status changed from testing to closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aborodin aborodin

Labels
area: mcedit mcedit, the built-in text editor prio: low Minor problem or easily worked around ver: 4.8.24 Reproducible in version 4.8.24
Milestone
4.8.34
Development

No branches or pull requests
2 participants
@aborodin @mc-butler