[pycrypto] Initial review of Thorsten's Py3k changes
Dwayne C. Litzenberger
dlitz at dlitz.net
Sun Jan 9 22:49:38 CST 2011
So I had a few minutes to take a look at Thorsten's Py3k patches at
https://github.com/yorickdowne/pycrypto (thanks, Thorsten!) and I'd like to
write down my thoughts on what I'll need before I can merge it into the
The main thing is that PyCrypto commits should be easily reviewable for
correctness and non-maliciousness. I suspect (and hope) that some of you
are conducting your own reviews of PyCrypto changes before you're using
them, and we all gain security-wise if I make sure that's reasonably easy.
Therefore, I have a few recommendations:
- Don't change the test vector data if you don't need to. Changing
hundreds of strings like 'd6a141a7ec3c38dfbd61' to strings like
b('d6a141a7ec3c36dfbd61') is unnecessary (since they need to be
hex-decoded somewhere anyway), and it makes it difficult to tell by
inspection that the test vectors weren't modified. (How many of you
noticed that I changed the 8 to a 6 in that example?)
- Run your tests with python -tt to ensure consistent use of whitespace. I
haven't been doing this, so the current master doesn't work, but that was
a mistake and I will be doing it from now on.
- Treat each commit like a patch that can be reviewed on its own. Think
"patch queue", not "modification history":
- If you have one commit that introduces a bug, and another commit that
removes it, squash those patches into a single commit.
- The same thing applies if you tried one approach and then switched to
another (e.g. floordiv -> divmod).
- The commands "git rebase -i", "git cherry-pick -n", "git reset", and
"git add -p" are your friends.
- If it's not too much work, try to group related commits together in
the history. It's much easier to review a series of AllOrNothing
patches, followed by a series of Crypto.Random patches, than it is to
review the patches if they're interlaced. This isn't always
reasonable, but if a simple "git rebase -i" can make things easier
for reviewers, it's worth the tiny bit of effort that it takes.
- If you need to fix whitespace, do it in a separate commit, labeled
"whitespace", that *only* fixes whitespace.
- Don't include unrelated changes in a commit. In a commit labeled "Add
Ron Rivest Test", don't make random changes to how "import os" is done in
various files, or how src/inc-msvc/stdint.h works.
- Make sure commit messages that answer the question, "why did you do
- References to things like RC5 or IDEA, which have been removed from
PyCrypto, can be removed.
- If adding additional/alternative dependencies like MPIR, include *why*
that's being done in the commit message and/or in the documentation.
That's all for now.
Dwayne C. Litzenberger <dlitz at dlitz.net>
OpenPGP: 19E1 1FE8 B3CF F273 ED17 4A24 928C EC13 39C2 5CF7
More information about the pycrypto