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.

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

Distinguish files with identical contents #1882

Merged
merged 6 commits into from
Feb 3, 2025

Conversation

liam923
Copy link
Contributor

@liam923 liam923 commented Jan 8, 2025

Currently Merlin's locate query uses a digest of the contents of a file to disambiguate files with the same name. If two files have the same digest, one is arbitrarily chosen. This can lead to Merlin confusing two files with identical contents. (See test case demonstrating the issue in the first commit.)

This PR resolves this issue by using a heuristic when multiple digests match. In a nutshell, it chooses the file whose path is most similar to the path of the build artifact it is trying to be matched to. See the comment in locate.ml for more detail.

tests/test-dirs/locate/dune Outdated Show resolved Hide resolved
@voodoos
Copy link
Collaborator

voodoos commented Jan 9, 2025

Thanks Liam, I will have a look.

Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

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

Thanks Liam, looks like a nice improvement for that corner case.
I am a bit worried about the leaking exception but apart from that the changes looks good to me.

src/utils/std.ml Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
src/analysis/locate.ml Outdated Show resolved Hide resolved
src/analysis/locate.ml Show resolved Hide resolved
@voodoos
Copy link
Collaborator

voodoos commented Feb 3, 2025

Thanks !

@voodoos voodoos merged commit 834821e into ocaml:main Feb 3, 2025
5 checks passed
@liam923 liam923 deleted the distinguish-files branch February 3, 2025 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Planned for backporting
Status: Planned for backporting
Development

Successfully merging this pull request may close these issues.

2 participants