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 goes back two years, and the data is obtained from Git logs on a Git clone taken on 5th April 2012:

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

This data is cross-referenced with the emails stored in the ​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 ​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 ​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

• ​Rationale management (feature-ACKs)
• ​Static testing (code-ACKs)

The review process obviously helps most when less-knowledgeable people send in patches which often have obvious errors and omissions.

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

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:"||%ci||%ad||%h||%s||-||%an||-||-||-||"

A random sample of 64 patches (out of 634) was then taken using shuf -n 64. A few notes:

• Delay is calculated as "day when applied to git" - "day when sent to the ml". For example, 26th Mar - 24th Mar = 2 days. When calculating large (i.e. 1+ month) delays each month is counted as 30 days.
• Patchset refers to a major patchset; in practice it's either Buildsystem or PolarSSL.
• Author date is the date the patch was introduced (e.g. sent to the mailing list)
• Commit date is the date when the patch was commited to current Git master repository. Note that changes to Git repository's structure (e.g. "allmerged -> master") seem to have messed up Git's commit dates. This means that some patches introduced before 2010-07-27 seem to have been "floating around" for months, whereas in reality they were merged to Git fairly quickly. Attempts to find out real dates often failed, so the faulty numbers were left as-is.
• For many patches it was impossible to locate the ACK - see "?" in "Delay (after ACK)" column. These may represent ACKs given in the IRC outside IRC meetings. ACKs given in the official IRC meetings should pop up when searching the mailing list archives.
• In some cases it was impossible to determine where a patch was originally sent to. In these cases figuring out whether it was modified in the process was also impossible.

Commit date	Author date	Identifier	Patch name	Patchset	Author	Modified	Delay (total)	Delay (after ACK)
2012-03-30	2012-03-28	3144411	Enable pedantic in windows compilation	Buildsystem	Alon Bar-Lev	No	2	0
2012-03-26	2012-03-24	405f338	build: windows: set vendor to openvpn project + cleanups	Buildsystem	Alon Bar-Lev	No	2	0
2012-03-22	2012-02-29	fbae7d2	build: plugins: properly use CC, CFLAGS and LDFLAGS	Buildsystem	Alon Bar-Lev	No	23	7
2012-03-22	2012-02-29	553d95d	cleanup: memcmp.c: remove unused source	Buildsystem	Alon Bar-Lev	No	23	16
2012-03-22	2012-02-29	51bd56f	build: autotools: first pass of trivial autotools changes	Buildsystem	Alon Bar-Lev	No	23	7
2012-02-20	2012-02-18	4ebc587	define access mode flag X_OK as 0 on Windows	-	Heiko Hund	Unknown	2	-
2012-03-30	2012-02-14	8e5613c	Migrated x509_get_sha1_hash to use the garbage collector	-	Adriaan de Jong	​Yes	45	3
2012-01-23	2012-01-22	62c613d	Platform cleanup for FreeBSD	-	Gert Doering	No	1	0
2011-12-14	2011-11-03	8407991	Fixed client issues with DHCP Router option extraction/deletion when using layer 2 with DHCP proxy:	-	James Yonan	No	41	-
2011-11-21	2011-10-31	7ac7170	Reordered functions to ensure warning-free Windows build	-	Adriaan de Jong	No	21	11
2011-11-21	2011-10-31	54628d1	Moved prng_uninit out of crypto_uninit_lib	-	Adriaan de Jong	No	21	?
2011-11-21	2011-10-31	1d90851	Moved from strsep to strtok, for Windows compatibility	-	Adriaan de Jong	No	21	?
2011-10-22	2011-10-16	eaacf8d	Moved to PolarSSL 1.0.0:	-	Adriaan de Jong	No	8	?
2011-12-14	2011-10-11	359adbf	Raised D_PID_DEBUG_LOW from level 3 to 4 to reduce replay error verbosity at level 3.	-	James Yonan	No	34	-
2011-09-21	2011-09-16	8ca19c0	Platform cleanup for NetBSD	-	Gert Doering	No	5	0
2011-08-31	2011-08-31	d90428d	add --mark option to set SO_MARK sockopt	-	Heiko Hund	No	0	0
2011-08-24	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	No	5	-
2012-02-21	2011-08-17	14a382a	remove wrapper code for Windows CryptoAPI function	-	Heiko Hund	No	4	0
2011-08-24	2011-08-16	f0257ab	For all accesses to "struct route_list 2 3 AUTHORS build, ... ,version.sh.in rl", check first that rl is non-NULL	-	Gert Doering	Unknown	8	?
2011-10-22	2011-07-28	c94eff3	Added back checks for ks->authenticated in verify_user_pass	PolarSSL	Adriaan de Jong	No	86	?
2011-08-19	2011-07-25	576dc96	Merge remote branch SVN 2.1 into the git tree	-	David Sommerseth	-	-	-
2011-10-22	2011-07-05	a9bf901	Added an extra define to allow building without PKCS#11	PolarSSL	Adriaan de Jong	No	109	?
2011-10-22	2011-07-05	557624e	Hardening: periodically reset the PRNG's nonce value	PolarSSL	Adriaan de Jong	No	109	?
2011-10-22	2011-07-01	8c96419	Fixed a missing include in ssl_backend.h	PolarSSL	Adriaan de Jong	No	113	?
2011-10-22	2011-06-30	3e44ea5	Refactored tls-verify script code	PolarSSL	Adriaan de Jong	​Yes	114	?
2011-07-01	2011-06-29	d22a379	Made domake-win builds to use easy-rsa/2.0/openssl-1.0.0.cnf	-	Samuli Seppänen	No	2	?
2011-10-19	2011-06-27	eab0cf2	Refactored TLS_PRF to new hmac and md primitives	PolarSSL	Adriaan de Jong	No	114	?
2011-10-19	2011-06-23	b01cb9e	Refactored crypto initialisation functions	PolarSSL	Adriaan de Jong	No	117	?
2011-10-19	2011-06-23	4a5a603	Refactored NTLM DES key generation	PolarSSL	Adriaan de Jong	No	117	?
2011-06-20	2011-06-20	54c739e	Revert "Add new openssl.cnf to easy-rsa/Windows"	-	David Sommerseth	No	0	0
2011-11-25	2011-06-16	0f2bc0d	Do some file/directory tests before really starting openvpn	-	David Sommerseth	No	1	0
2011-08-25	2011-05-26	739fa98	Fix a Visual Studio 2008 build error in options.c	-	Samuli Seppanen	No	90	?
2011-05-27	2011-05-26	21fc2ed	Don't define ENABLE_PUSH_PEER_INFO if SSL is not available	-	David Sommerseth	No	1	0
2011-04-27	2011-04-27	b70d99f	Fix compile issues when using --enable-small and --disable-ssl/--disable-crypto	-	Gustavo Zacarias	No	0	0
2011-04-26	2011-04-24	d549726	Added 'dir' flag to "crl-verify" (see man page for info).	-	James Yonan	No	2	-
2011-04-26	2011-04-12	e51935d	For Mac OSX, when DARWIN_USE_IPCONFIG is defined ,.. the ipconfig command sometimes fails if executed immediately after the tun device open.	-	James Yonan	No	14	-
2011-04-12	2011-04-12	2a12831	For Mac OSX, when DARWIN_USE_IPCONFIG is defined ... the ipconfig command sometimes fails if executed immediately after the tun device open.	-	James Yonan	-	-	-
2011-04-14	2011-04-03	9ed122e	Fixed bug in port-share that could cause port share process to crash with output like this:	-	James Yonan	No	11	-
2011-03-25	2011-03-25	dc2ccc8	Clarify --tmp-dir option	-	chantra	No	0	0
2011-03-21	2011-03-21	1df945e	Version 2.1.3n	-	James Yonan	-	-	-
2011-03-17	2011-03-17	d02a86d	Renamed branch to reflect that it is no longer beta.	-	James Yonan	-	-	-
2011-03-21	2011-03-15	58704ea	Updated INSTALL-win32.txt	-	Samuli Seppänen	No	6	3
2011-03-25	2011-02-28	a75c7dd	Fixed typo in plugin.h	-	Stefan Hellermann	No	25	?
2011-02-27	2011-02-11	a7f0fc3	Added configure.h and version.m4 variable parsing to win/config.py	-	Samuli Seppänen	?	16	?
2011-02-27	2011-02-11	4e4aa65	Several modifications to win/make_dist.py to allow building the NSI installer	-	Samuli Seppänen	?	16	?
2010-11-18	2010-11-15	7581c8f	Removed functions not being used anywhere	-	David Sommerseth	No	3	?
2010-11-18	2010-11-15	33ee747	Fixed potential misinterpretation of boolean logic	-	David Sommerseth	No	3	?
2010-11-13	2010-11-13	22178d0	Merge branch 'svn-BETA21' into bugfix2.1	-	David Sommerseth	-	-	-
2010-11-12	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	No	12	?
2010-10-21	2010-10-21	59afc4a	Fix problem with special case route targets ('remote_host')	-	Gert Doering	No	0	0
2010-11-12	2010-09-30	58f8d94	Add HTTP/1.1 Host header	-	Lars Hupel	No	43	40
2010-07-27	2010-07-27	75dfe3d	Added "net stop dnscache" and "net start dnscache" in front of existing --register-dns commands.	-	James Yonan	No	0	0

The delays shown in these patches are probably faulty (i.e. too big):

Commit date	Author date	Identifier	Patch name	Patchset	Author	Modified	Delay (total)	Delay (after ACK)
2010-10-21	2010-07-16	4c91fc8	Fixes openssl-1.0.0 compilation warning	-	chantra	No	95	?
2010-10-21	2010-04-27	7d5e26c	Fix certificate serial number export	-	Davide Brini	No	172	?
2010-10-21	2010-04-22	892e64b	Fix missing /bin/bash -> /bin/sh	-	Davide Brini	No	179	?
2010-10-21	2010-04-16	add7fe0	Updated the man page to reflect the behavioural change of create_temp_file()	-	David Sommerseth	No	185	3
2010-10-21	2010-03-10	1e02046	On TARGET_LINUX define _GNU_SOURCE if not defined	-	David Sommerseth	No	221	?
2011-04-24	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	No	47	-
2010-10-21	2010-03-02	d04b858	Several updates to openvpn.8 (man page updates)	-	Karl O. Pinc	No	229	?
2011-03-25	2010-02-20	a698464	* undo mroute.c changes related to ipv6 payload, nothing to do w/ipv6 transport afterall.	-	JuanJo? Ciarlante	No	35	-
2010-10-21	2010-02-19	adfe37f	verb 5 logging wrongly reports received bytes	-	David Sommerseth	No	242	240
2010-10-21	2010-02-18	058f3d0	[PATCH] Change verify-cn so cn is no longer hardcoded in openvpn's config file	-	Karl O. Pinc	No	243	243
2011-03-25	2009-10-05	e293510	* TODO.ipv6 update	-	JuanJo? Ciarlante	No	130	-
2011-03-25	2009-09-24	e478770	* make possible to x-compile openvpn/win32 in Linux	-	JuanJo? Ciarlante	No	141	-

Big patchsets

For comparison, let's look at two big patchsets:

• PolarSSL support [1], ~115 patches
  • total of 8 changes were asked for
  • 3 of these seem trivial stylistic issues/typos

• Buildsystem revolution[2], 52 patches
  • a couple of changes were asked for (e.g. to enable lzo by default)

Conclusions

Although this review was fairly limited in scope (~10% of 634 patches), some interesting things surfaced:

• A patch was modified during review only on two occasions (0,32% of all patches). That said, it was often difficult to know whether a patch was modified before being introduced. This could have had happened with patches that did not go through the usual mailing list scrutiny, see "?" in "Delay (after ACK)" column.
• The larger the patchset, the longer it takes to get merged (e.g. ~110 days for PolarSSL patches). Although this is a no-brainer, there's probably lots of room for improvement.
• Quite a few patches already go into Git without review, see "-" (i.e. "not applicable") in "Delay (after ACK)" column.
• The majority of ACKs were given by only 3 persons.
• It's often very difficult to find discussion about patches: see "?" in "Delay (after ACK)" column.