[liberationtech] DecryptoCat

[Petter Ericson]

On 08 July, 2013 - Nadim Kobeissi wrote:

> 
> On 2013-07-08, at 2:48 PM, Reed Black <reed at unsafeword.org> wrote:
> 
> > On Mon, Jul 8, 2013 at 11:00 AM, David Goulet <dgoulet at ev0ke.net> wrote:
> >> 
> >> Furthermore, looking at those lines of code, there is simply NO comments at all,
> >> nothing to help peer review, to explain why this or that is done that way and
> >> nothing linked to any design document. This is in my opinion VERY important that
> >> developers design their system/subsystem beforehand *especially* a crypto design
> >> in a public document. And, if it has to change, the design should be peer
> >> reviewed before even making one line of code.
> >> 
> >> So, the technical critical issue, CryptoCat responded well, quickly but the
> >> point here is that there is a problem in terms of how development is done and
> >> how *little* the maintainability of the code is.
> > 
> > I think there is a bigger problem in the commit messages. Looking at
> > the history, many are "tweak" "guehh" "update" "FIX THE BUG" and some
> > of those are tied to large many-purpose Swiss Army Knife commits.
> > 
> > Without descriptive commit messages and single-purpose commits,
> > community review is highly unlikely. It takes an order of magnitude
> > more effort for a reviewer to suss out the intent of a code change.
> > The reviewer is also much less likely to ask about suspicious side
> > effects if he's starting with infinite possibility of intent on first
> > encountering the code. Few volunteers will make a routine effort.
> > 
> > 
> > Remember when someone tried slipping this into the Linux kernel?
> > 
> > + if ((options == (__WCLONE|__WALL)) && (current->uid = 0))
> > + retval = -EINVAL;
> > 
> > Ask if something that subtle have been spotted so quickly if it were
> > one of many Swiss Army Knife "guehh" commits.
> 
> I'm sure "guehh" and so on are either exceptions or relate to very irrelevant commits.
> If they're not, then we definitely have a commit documentation problem!
> 
> NK

Looking at the commit history, you do.

Specifically, when fixing bugs, you do note the trac number, but you
do not include a link to the bug in question, and neither do you mention
what the actual cause of the bug was. Instead you write "Fix #<whatever>",
sometimes with a "hopefully", or "maybe". A brief description of what the
bug/feature entailed, and how it was fixed helps immensely if anything goes
wrong later, and it also makes other peoples understanding of the codebase
significantly more likely.

Also, there are numerous instances where you do one thing, and also various
cosmetics or clean-up operations, for example d158c4cd from late May. Try to 
separate commits into fully independent change sets, where code functionality
commits are clearly marked as such, and code cosmetics likewise.

Generally, software development is messy. Try not to make that show in the
commit history.

Best

/P

> 
> > 
> > 
> > I think any project that relies on community review for security needs
> > to first stop and ask what would make community review likely. At the
> > least, that's some venue for review discussion where the developers
> > are reading, single-function commits, and descriptive commit messages.
> > Does anyone know if there's something like a "best practices" page to
> > point to for maintaining a healthy reviewer community?
> > --
> > Too many emails? Unsubscribe, change to digest, or change password by emailing moderator at companys at stanford.edu or changing your settings at https://mailman.stanford.edu/mailman/listinfo/liberationtech
> 
> --
> Too many emails? Unsubscribe, change to digest, or change password by emailing moderator at companys at stanford.edu or changing your settings at https://mailman.stanford.edu/mailman/listinfo/liberationtech

-- 
Petter Ericson (pettter at acc.umu.se)