Skip to content

Search results for "UTF-8 with BOM" files shifted on first line by a character - #66189

Merged
roblourens merged 3 commits into
masterfrom
unknown repository
Jan 10, 2019
Merged

Search results for "UTF-8 with BOM" files shifted on first line by a character#66189
roblourens merged 3 commits into
masterfrom
unknown repository

Conversation

@ghost

Copy link
Copy Markdown

Fix #66188

This is my first pull request in this project. Please let me know if my solution is not good enough, I am willing to improve it.

@msftclas

Microsoft Contribution License Agreements (msftclas) commented Jan 8, 2019

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a UTF-8 with BOM file and _fullPreviewRange.startLineNumber is 0, _fullPreviewLines[0] will starts with BOM, then _fullPreviewLines does not match _fullPreviewRange.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the BOM should be stripped already by ripgrepTextSearchEngine right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can move it to ripgrepTextSearchEngine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's already there, right? That file should be stripping the BOM correctly in all cases with your change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this? The BOM is 3 bytes long.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's correct thanks.

@ghost

Copy link
Copy Markdown
Author

I found another solution: Remove && options.encoding !== 'utf8'

https://github.com/Microsoft/vscode/blob/f0f3b922bcb081c6488b7299dbef4076a1cfde82/src/vs/workbench/services/search/node/ripgrepTextSearchEngine.ts#L333

then ripgrep will not output BOM character in JSON.

Or I can manually strip the BOM character in ripgrepTextSearchEngine.

Which do you prefer?

@roblourens

Copy link
Copy Markdown
Member

I want to keep that because I found that search was faster in some cases without it. Also, you could probably search with a different encoding but still end up finding results in UTF8 files with BOMs. Also I don't trust ripgrep to keep that behavior forever. So, let's keep the check to strip the BOM on our end.

@ghost

Deleted user (ghost) commented Jan 10, 2019

Copy link
Copy Markdown
Author

@roblourens

Copy link
Copy Markdown
Member

Looks great, thanks!

@roblourens
roblourens merged commit 8c2cef9 into microsoft:master Jan 10, 2019
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants