Skip to content

fix: case-insensitive search highlights the wrong bytes - #1157

Open
VXNCXNX wants to merge 1 commit into
noborus:masterfrom
VXNCXNX:fix/case-insensitive-search-offsets
Open

fix: case-insensitive search highlights the wrong bytes#1157
VXNCXNX wants to merge 1 commit into
noborus:masterfrom
VXNCXNX:fix/case-insensitive-search-offsets

Conversation

@VXNCXNX

@VXNCXNX VXNCXNX commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

What's broken

Case-insensitive search highlights the wrong bytes when the line contains a rune whose lowercase form has a different byte length. searchWord.FindAll lowercases the target and returns offsets into that copy, but searchHighlight and searchXPos index the original string.

İ is 2 bytes and lowercases to 1, so later matches shift left. Ⱥ is 2 and lowercases to 3, so they shift right. 23 runes shrink and 2 grow.

Repro

File containing İİ error here, searching for error:

$ ov -X -F --pattern error t3.txt | cat -v

before:  M-DM-0M-DM-0^[[7m err^[[0mor here
after:   M-DM-0M-DM-0 ^[[7merror^[[0m here

The reverse-video run starts one byte early and splits the word.

The fix

toLowerWithOffsets lowercases and returns a table mapping each byte of the result back to the original, and FindAll remaps the indexes through it.

I did not touch allStringIndex, since it also serves the column-delimiter path. The remap lives in FindAll, the only case-insensitive caller.

ASCII lines take a fast path that returns a nil table and the original single strings.ToLower, so the common case allocates nothing extra.

Verification

Two subtests, one per direction, on a shrinking and a growing rune. Skipping the remap fails both with the matched substring shown, not a build error.

I checked toLowerWithOffsets against strings.ToLower for every valid rune, alone and in context, and on invalid UTF-8, and asserted the offset table is monotonic and ends at len(s). That probe was a scratch test and is not in the diff.

Output above is from real binaries under a pty. go test ./... passes, go vet and gofmt clean.

searchWord.FindAll lowercased the target and returned byte offsets into
that copy, but callers index the original string. Lowercasing changes
the byte length of some runes, so every match after one of them was
shifted and the highlight landed mid-rune.

allStringIndex is untouched: the remap is done in FindAll, its only
case-insensitive caller.
@noborus

noborus commented Aug 18, 2026

Copy link
Copy Markdown
Owner

Thanks for fixing the byte offset issue.
I think the fix itself is correct.

One concern is the additional cost of supporting Unicode lowercasing, especially since this search path is used frequently. Rather than extending case-insensitive search to cover Unicode characters, would it make sense to avoid strings.ToLower here and only convert ASCII A-Z to a-z?

That would preserve the byte length and avoid the offset mapping table, while also keeping the change simpler and faster. I’m not sure whether Unicode case-insensitive matching is an intended feature, so I’d like to hear your thoughts before deciding.

@VXNCXNX

VXNCXNX commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Good question, and I checked rather than guessed.

Unicode folding is not something this PR adds: FindAll on master already calls strings.ToLower(target), so ÉCOLE already matches école today. Restricting to ASCII A-Z would drop that:

text     query     master   ascii-only
ÉCOLE    école     true     false
ÀÉÎÕÜ    àéîõü     true     false
ЖУРНАЛ   журнал    true     false
HELLO    hello     true     true

So it would be a behaviour change for accented, Cyrillic and Greek text, not just a speedup.

On cost, toLowerWithOffsets already returns early for pure-ASCII lines and builds no table: strings.ToLower plus one isASCII scan, same single allocation. I measured 148 ns/op vs 254 ns/op on a 78-byte ASCII line, so about 100 ns on lines that are already the fast path, and the offsets table is only built for lines that actually contain non-ASCII.

If you would rather keep it strictly ASCII I am happy to change it, but I would suggest doing that as its own decision since it removes existing behaviour. Your call.

@noborus

noborus commented Aug 18, 2026

Copy link
Copy Markdown
Owner

I do not use these characters very often, so Cyrillic and Greek letters were not included intentionally. They are included because string.ToLower handles them as well.

It is difficult for me to judge how much this behavior affects users who work with these characters. If this causes problems for anyone, please leave a comment with your use case or an example.

@VXNCXNX

VXNCXNX commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Understood, and that settles it from my side: I will leave the folding as it is.

Worth noting the scope of this PR either way. It only touches FindAll, which maps the highlight positions, so it does not change which lines match. Whatever strings.ToLower folds today keeps folding exactly the same before and after; the only difference is that the highlight now lands on the right bytes.

So if you later decide to restrict folding to ASCII, that is an independent change and this one does not get in its way.

@noborus

noborus commented Aug 18, 2026

Copy link
Copy Markdown
Owner

I have created a reference implementation in #1159. I would like to wait and see if anyone finds value in this specification.
There may still be room for improvement in terms of speed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants