indygreg added a comment.
This seems mostly good to me. INLINE COMMENTS > cext.c:68 > + if (SHA1DCFinal(hash_out, &ctx)) { > + PyErr_SetString(PyExc_OverflowError, > + "sha1 collision attack detected"); I'm not super keen on overloading `OverflowError` here. Or is this how `hashlib` works in the Python standard library? > cext.c:92 > + static const char hexdigit[] = "0123456789abcdef"; > + for (int i = 0; i < 20; ++i) { > + hexhash[i * 2] = hexdigit[hash[i] >> 4]; 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. > cext.c:107 > + } > + clone->ctx = self->ctx; > + return (PyObject *)clone; 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. > cext.c:172 > + sha1ctxType.tp_new = PyType_GenericNew; > + if (PyType_Ready(&sha1ctxType) < 0) > + return; Nit: please always use braces. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7815/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7815 To: durin42, #hg-reviewers Cc: indygreg, spectral, mjpieters, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel