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

[mono] Simplify file comparisons #112295

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

lewing
Copy link
Member

@lewing lewing commented Feb 8, 2025

It's late on a Friday so it is likely I'm missing something, but the hash comparison doesn't appear to be useful at all, that said I will assume it is useful and preserve the original behavior for now.

Use two rolling buffers to do file equality comparison since Read can return less than a full buffer, only advance a buffer once it is consumed, and compare the overlapping region.

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/tasks/Common/Utils.cs:217

  • Ensure that the new behavior introduced by the CompareFiles method is covered by tests. This includes scenarios where files are identical and where they differ.
private static bool CompareFiles(string filePath1, string filePath2)
Comment on lines -222 to -224
bool areDifferent = !File.Exists(dst) ||
(useHash && ComputeHash(src) != ComputeHash(dst)) ||
(File.ReadAllText(src) != File.ReadAllText(dst));
Copy link
Member Author

@lewing lewing Feb 8, 2025

Choose a reason for hiding this comment

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

This condition appears to be incorrect given that even when useHash is true and they the hashes differ it will fall back to checking the text. It can either be dropped completely to compare the text or it can skip the text check if use hash is true.

Copy link
Member

Choose a reason for hiding this comment

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

yeah I think the intention was to use the (fast) hash comparison and only fallback to comparing the text if the hash differs.

so this could likely be

bool areSame = File.Exists(dst) && (useHash ? ComputeHash(src) != ComputeHash(dst) : File.ReadAllText(src) != File.ReadAllText(dst);

but I wonder too why we don't just always compare the hash.

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

Successfully merging this pull request may close these issues.

2 participants