This is an archive of the discontinued Mercurial Phabricator instance.

sha1dc: initial implementation of Python extension
ClosedPublic

Authored by durin42 on Jan 8 2020, 4:27 PM.

Details

Summary

A future change will use this when available to avoid sha1 collision
issues until we can get moved to something else.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

durin42 created this revision.Jan 8 2020, 4:27 PM
durin42 updated this revision to Diff 19113.Jan 8 2020, 4:31 PM
spectral added inline comments.
mercurial/thirdparty/sha1dc/cext.c
24

Nit: "clang-format on" (typo: s/oon/on/)

104

I think you need a {NULL} entry here, or else it's going to walk off the end of this array when creating the type.

148

You may also need a NULL entry here, haven't tracked down where this is used.

152

why do this here instead of above?

durin42 edited the summary of this revision. (Show Details)Jan 8 2020, 5:38 PM
durin42 updated this revision to Diff 19115.
durin42 updated this revision to Diff 19116.
durin42 marked 4 inline comments as done.Jan 8 2020, 5:41 PM
durin42 added inline comments.
mercurial/thirdparty/sha1dc/cext.c
24

Good catch!

104

Sigh, yep.

152

Not sure. This is just how I've always seen it done...

durin42 marked 3 inline comments as done.Jan 8 2020, 5:44 PM
durin42 retitled this revision from sha1dc: initial implementation of Python extension (NOT READY) to sha1dc: initial implementation of Python extension.Jan 8 2020, 8:30 PM
durin42 edited the summary of this revision. (Show Details)
durin42 updated this revision to Diff 19119.

This seems mostly good to me.

mercurial/thirdparty/sha1dc/cext.c
69

I'm not super keen on overloading OverflowError here. Or is this how hashlib works in the Python standard library?

93

I didn't verify this is correct because I didn't want to think too hard. Test coverage for parity with hashlib would be appreciated.

108

Should this be a memcpy or some such? Or is this opaque type safe to copy by value? Test coverage for this demonstrating that a seeded hasher which is copied can properly diverge would be appreciated.

173

Nit: please always use braces.

durin42 marked 3 inline comments as done.Jan 13 2020, 4:55 PM

Good call on tests, I had the return type of hexdigest() wrong. Please do let me know if you think of any missing cases here.

mercurial/thirdparty/sha1dc/cext.c
69

This is new functionality compared to hashlib (this is the "zomg someone is breaking ur sha1" error case), so there's not anything comparable. I didn't really want to go to the effort of defining our own custom exception type for a case we should never see in the wild.

108

I verified by inspection (when writing this) that copying by value is okay here (no pointers, etc), and we now have test coverage that will catch any regression here.

durin42 marked an inline comment as done.Jan 13 2020, 5:48 PM
durin42 updated this revision to Diff 19188.
indygreg accepted this revision.Jan 13 2020, 11:30 PM
This revision is now accepted and ready to land.Jan 13 2020, 11:30 PM
This revision was automatically updated to reflect the committed changes.