Page MenuHomePhabricator

sha1dc: initial implementation of Python extension
ClosedPublic

Authored by durin42 on Wed, Jan 8, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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

105

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.

149

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

153

why do this here instead of above?

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

Good catch!

105

Sigh, yep.

153

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

durin42 marked 3 inline comments as done.Wed, Jan 8, 5:44 PM
durin42 retitled this revision from sha1dc: initial implementation of Python extension (NOT READY) to sha1dc: initial implementation of Python extension.Wed, Jan 8, 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.Mon, Jan 13, 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.Mon, Jan 13, 5:48 PM
durin42 updated this revision to Diff 19188.
indygreg accepted this revision.Mon, Jan 13, 11:30 PM
This revision is now accepted and ready to land.Mon, Jan 13, 11:30 PM
This revision was automatically updated to reflect the committed changes.