[pycrypto] Initial review of Thorsten's Py3k changes

[Dwayne C. Litzenberger]

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 
master branch.

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 
   that?".

- 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.

Cheers,
- Dwayne


-- 
Dwayne C. Litzenberger <dlitz at dlitz.net>
  OpenPGP: 19E1 1FE8 B3CF F273 ED17  4A24 928C EC13 39C2 5CF7