Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Use CRC32 instead of MD5 for hashing the file content. #1918

Closed
wants to merge 2 commits into from
Closed

Conversation

pranavkm
Copy link
Contributor

Fixes #1800

@yishaigalatzer
Copy link
Contributor

Talking to @Eilon yesterday, I thought this was no ok to take?

Also please follow up with @GrabYourPitchforks we should probably provide a key to select hashing algorithm so we can change it if we find issues without a breaking change

@pranavkm
Copy link
Contributor Author

Spoke to @Eilon and sort of convinced him this was. His primary concern was with having algorithms checked in to Mvc. However, the alternatives weren't iterative or didn't work on byte streams so we ended up with just as much code in making things work.

we should probably provide a key to select hashing algorithm

Could we remove RazorHash and just make the Crc32 a public type instead \ change properties to be Crc32? Having algorithm lookups for this seems like overkill

@yishaigalatzer
Copy link
Contributor

Could we remove RazorHash and just make the Crc32 a public type instead \ change properties to be Crc32? Having algorithm lookups for this seems like overkill

No, this is exactly the problem. If we end up with collisions or wanting to do perf improvements for some reason you are stuck with CRC32

@pranavkm
Copy link
Contributor Author

Wouldn't we then introduce a new property at that point and use that? If we use a string based key, we'd need to store they algorithm name along with the hash. Seems cleaner to just have the property reflect the algorithm.

Assert.Equal(expected, result);
}

private class SlowStream : System.IO.MemoryStream
Copy link
Member

Choose a reason for hiding this comment

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

add using

@pranavkm
Copy link
Contributor Author

Updated.

@@ -30,6 +30,11 @@ public class RazorFileInfo
/// <summary>
/// A hash of the file content.
/// </summary>
public string Hash { get; set; }
public long Hash { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably keep as string

@yishaigalatzer
Copy link
Contributor

:shipit: when comments are addressed

@pranavkm pranavkm closed this Jan 31, 2015
@pranavkm pranavkm deleted the Hash branch January 31, 2015 02:06
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.

None yet

3 participants