= Introduction =

The purpose of this analysis is to estimate the usefulness of the ACK/NACK system as used in OpenVPN project's development. This study extends back two years, and the data is obtained from Git logs:

{{{
$ git log --since="2 years ago"
}}}

This data is cross-referenced with the emails stored in the [http://dir.gmane.org/gmane.network.openvpn.devel openvpn-devel mailing list archive]. As reviewing all patches and ACKs is not possible, a random sample is taken. 

The hypothesis is that the ACK/NACK process is most suitable for reviewing individual patches or small patchsets sent to the openvpn-devel mailing list by developers not intimately familiar with OpenVPN. A less bureaucratic process might be more suitable for core developers, especially when less security-sensitive parts are being modified.

= Current ACK/NACK code review system =

== Scope ==

Most patches have required an ACK before going into Git "master". There are a only few exceptions:

 * Patches from James Yonan's [http://svn.openvpn.net/ Subversion repository] are merged without review
 * Anything else?

== Evolution ==

The original ACK system required an ACK from one developer for the code to enter the Git repository. This process was later (add date?) split into two logical parts:

 * Feature-ACK: this should be the primary consideration when reviewing patches. Merging useless patches makes no sense whatsoever.
 * Code-ACK: if the feature makes sense, the next step would be to review that the code itself makes sense (e.g. has no obvious errors and follows established conventions)

Still later (add date?) a move towards [http://communitymgt.wikia.com/wiki/Lazy_consensus lazy consensus] was taken. So, as it stands now (Mar 2012), code can go to the Git repository without an ACK, but with a considerable delay.

== Pros ==

The formal ACK/NACK system was imposed to increase OpenVPN code quality by decreasing the chances of bad code getting into the tree. The mandatory ACK forces peer review. In software engineering jargon, it fulfills the roles of

 * [http://en.wikipedia.org/wiki/Design_rationale Rationale management] (feature-ACKs) 
 * [http://en.wikipedia.org/wiki/Static_testing Static testing] (code-ACKs)

== Limitations ==

There are limits to what the ACK process can do. For example:

 * Runtime errors may be very difficult to notice
 * Seeing the "big picture" may be difficult, especially with big patchsets
 * Whether a particular problems is noticed depends highly on the reviewer's skills

== Cons ==

The ACK/NACK process has it's cons:

 * Requiring an ACK slows down flow of code from contributors to the "master" development tree. This problem gets worse as the patchset size increases.
 * Due to the above, it takes longer to get the code into wide circulation. This means bugs are reported later than if the code had been merged as soon as it's ready. 

= Raw data =

This raw data is based on Git clone done on 5th Apr 2012:

== Generic statistics ==

 * Commit count: 635
 * ACK number: 458
 * Average ACK count: .72

== ACKs by person ==

 * Adriaan de Jong 20
 * Alon Bar-Lev 3
 * Davide Guerri 1
 * David Sommerseth 155
 * Eric F Crist 1
 * Gert Doering 107
 * Gilles Espinasse 1
 * Heiko Hund 3
 * James Yonan 116
 * Jan Just Keijser 2
 * Kazuyoshi Aizawa 2
 * krzee 3
Generic statistics:

 * Commit count: 622
 * ACK number: 439
 * Average ACK count: 0.70

ACKs by person:

 * Adriaan de Jong: 18
 * Alon Bar-Lev: 3
 * Davide Guerri: 1
 * David Sommerseth: 148
 * Eric F Crist: 1
 * Gert Doering: 101
 * Gilles Espinasse: 1
 * Heiko Hund: 3
 * James Yonan: 113
 * Jan Just Keijser: 2
 * Kazuyoshi Aizawa: 2
 * krzee: 3
 * Peter Stuge: 11
 * Samuli Seppänen: 32

 * Peter Stuge 11
 * Samuli Seppänen 33

== Effects of the ACK system on patches ==

The following Git command was used to generate a list of all patches in Trac format:

{{{
$ git log --date=short --since="2 years ago" --pretty=format:"||%ad||%h||%s||-||%an||-||||"
}}}

A random sample of 64 patches (out of 634) was then taken using ''shuf -n 64''. The following random sample of patches was the result:

||Commit date||Identifier||Patch name||Patchset||Author||Modified||Delay||
||2010-03-02||d04b858||Several updates to openvpn.8 (man page updates)||-||Karl O. Pinc||-||||
||2011-06-16||0f2bc0d||Do some file/directory tests before really starting openvpn||-||David Sommerseth||-||||
||2011-06-23||b01cb9e||Refactored crypto initialisation functions||-||Adriaan de Jong||-||||
||2012-02-29||fbae7d2||build: plugins: properly use CC, CFLAGS and LDFLAGS||-||Alon Bar-Lev||-||||
||2011-03-15||58704ea||Updated INSTALL-win32.txt||-||Samuli Seppänen||-||||
||2011-04-27||b70d99f||Fix compile issues when using --enable-small and --disable-ssl/--disable-crypto||-||Gustavo Zacarias||-||||
||2011-06-23||4a5a603||Refactored NTLM DES key generation||-||Adriaan de Jong||-||||
||2011-03-25||dc2ccc8||Clarify --tmp-dir option||-||chantra||-||||
||2011-10-11||359adbf||Raised D_PID_DEBUG_LOW from level 3 to 4 to reduce replay error verbosity at level 3.||-||James Yonan||-||||
||2010-07-27||75dfe3d||Added "net stop dnscache" and "net start dnscache" in front of existing --register-dns commands.||-||James Yonan||-||||
||2009-09-24||e478770||* make possible to x-compile openvpn/win32 in Linux||-||JuanJo Ciarlante||-||||
||2010-04-27||7d5e26c||Fix certificate serial number export||-||Davide Brini||-||||
||2010-03-10||1e02046||On TARGET_LINUX define _GNU_SOURCE if not defined||-||David Sommerseth||-||||
||2011-07-25||576dc96||Merge remote branch SVN 2.1 into the git tree||-||David Sommerseth||-||||
||2011-06-30||3e44ea5||Refactored tls-verify script code||-||Adriaan de Jong||-||||
||2010-02-20||a698464||* undo mroute.c changes related to ipv6 payload,   nothing to do w/ipv6 transport afterall.||-||JuanJo Ciarlante||-||||
||2012-02-14||8e5613c||Migrated x509_get_sha1_hash to use the garbage collector||-||Adriaan de Jong||-||||
||2010-10-30||f0eac1a||Make "topology subnet" work on Solaris (ifconfig + route metric changes by Kazuyoshi Aizawa, adding of local "connected subnet" route by me)||-||Gert Doering||-||||
||2011-10-31||54628d1||Moved prng_uninit out of crypto_uninit_lib||-||Adriaan de Jong||-||||
||2011-07-01||8c96419||Fixed a missing include in ssl_backend.h||-||Adriaan de Jong||-||||
||2011-10-16||eaacf8d||Moved to PolarSSL 1.0.0:||-||Adriaan de Jong||-||||
||2011-03-21||1df945e||Version 2.1.3n||-||James Yonan||-||||
||2011-06-29||d22a379||Made domake-win builds to use easy-rsa/2.0/openssl-1.0.0.cnf||-||Samuli Seppänen||-||||
||2011-09-16||8ca19c0||Platform cleanup for NetBSD||-||Gert Doering||-||||
||2011-04-12||e51935d||For Mac OSX, when DARWIN_USE_IPCONFIG is defined, retry ipconfig command on failure once every second for up to 15 seconds.  This is necessary to work around an issue observed on OSX 10.5 where the ipconfig command sometimes fails if executed immediately after the tun device open.||-||James Yonan||-||||
||2011-07-28||c94eff3||Added back checks for ks->authenticated in verify_user_pass||-||Adriaan de Jong||-||||
||2011-02-11||a7f0fc3||Added configure.h and version.m4 variable parsing to win/config.py||-||Samuli Seppänen||-||||
||2010-11-13||22178d0||Merge branch 'svn-BETA21' into bugfix2.1||-||David Sommerseth||-||||
||2011-11-03||8407991||Fixed client issues with DHCP Router option extraction/deletion when using layer 2 with DHCP proxy:||-||James Yonan||-||||
||2010-11-15||7581c8f||Removed functions not being used anywhere||-||David Sommerseth||-||||
||2011-10-31||1d90851||Moved from strsep to strtok, for Windows compatibility||-||Adriaan de Jong||-||||
||2010-07-16||4c91fc8||Fixes openssl-1.0.0 compilation warning||-||chantra||-||||
||2011-04-24||d549726||Added 'dir' flag to "crl-verify" (see man page for info).||-||James Yonan||-||||
||2012-01-22||62c613d||Platform cleanup for FreeBSD||-||Gert Doering||-||||
||2011-07-05||557624e||Hardening: periodically reset the PRNG's nonce value||-||Adriaan de Jong||-||||
||2010-02-18||058f3d0||[PATCH] Change verify-cn so cn is no longer hardcoded in openvpn's config file||-||Karl O. Pinc||-||||
||2011-03-17||d02a86d||Renamed branch to reflect that it is no longer beta.||-||James Yonan||-||||
||2011-08-31||d90428d||add --mark option to set SO_MARK sockopt||-||Heiko Hund||-||||
||2011-05-26||21fc2ed||Don't define ENABLE_PUSH_PEER_INFO if SSL is not available||-||David Sommerseth||-||||
||2010-03-07||94d50a1||when deleting a route on win32, also add gateway address (otherwise netsh.exe will succeed, but silently ignore request)||-||Gert Doering||-||||
||2011-05-26||739fa98||Fix a Visual Studio 2008 build error in options.c||-||Samuli Seppanen||-||||
||2011-06-20||54c739e||Revert "Add new openssl.cnf to easy-rsa/Windows"||-||David Sommerseth||-||||
||2010-04-22||892e64b||Fix missing /bin/bash -> /bin/sh||-||Davide Brini||-||||
||2011-08-16||f0257ab||For all accesses to "struct route_list * rl", check first that rl is non-NULL||-||Gert Doering||-||||
||2009-10-05||e293510||* TODO.ipv6 update||-||JuanJo Ciarlante||-||||
||2010-09-30||58f8d94||Add HTTP/1.1 Host header||-||Lars Hupel||-||||
||2011-08-19||2627335||CC_PRINT character class now allows any 8-bit character value >= 32. This is done to allow UTF-8 and restrict the use of control characters in usernames, passwords, common names, etc.||-||James Yonan||-||||
||2011-07-05||a9bf901||Added an extra define to allow building without PKCS#11||-||Adriaan de Jong||-||||
||2012-02-18||4ebc587||define access mode flag X_OK as 0 on Windows||-||Heiko Hund||-||||
||2011-02-11||4e4aa65||Several modifications to win/make_dist.py to allow building the NSI installer||-||Samuli Seppänen||-||||
||2010-02-19||adfe37f||verb 5 logging wrongly reports received bytes||-||David Sommerseth||-||||
||2010-11-15||33ee747||Fixed potential misinterpretation of boolean logic||-||David Sommerseth||-||||
||2011-08-17||14a382a||remove wrapper code for Windows CryptoAPI function||-||Heiko Hund||-||||
||2011-06-27||eab0cf2||Refactored TLS_PRF to new hmac and md primitives||-||Adriaan de Jong||-||||
||2012-02-29||51bd56f||build: autotools: first pass of trivial autotools changes||-||Alon Bar-Lev||-||||
||2012-02-29||553d95d||cleanup: memcmp.c: remove unused source||-||Alon Bar-Lev||-||||
||2010-10-21||59afc4a||Fix problem with special case route targets ('remote_host')||-||Gert Doering||-||||
||2010-04-16||add7fe0||Updated the man page to reflect the behavioural change of create_temp_file()||-||David Sommerseth||-||||
||2011-10-31||7ac7170||Reordered functions to ensure warning-free Windows build||-||Adriaan de Jong||-||||
||2011-04-03||9ed122e||Fixed bug in port-share that could cause port share process to crash with output like this:||-||James Yonan||-||||
||2012-03-28||3144411||Enable pedantic in windows compilation||-||Alon Bar-Lev||-||||
||2011-04-12||2a12831||For Mac OSX, when DARWIN_USE_IPCONFIG is defined, retry ipconfig command on failure once every second for up to 15 seconds.  This is necessary to work around an issue observed on OSX 10.5 where the ipconfig command sometimes fails if executed immediately after the tun device open.||-||James Yonan||-||||
||2012-03-24||405f338||build: windows: set vendor to openvpn project + cleanups||-||Alon Bar-Lev||-||||
||2011-02-28||a75c7dd||Fixed typo in plugin.h||-||Stefan Hellermann||-||||

= Analysis =