wiki:ACKSystemReview

Version 6 (modified by Samuli Seppänen, 8 years ago) (diff)

--

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

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

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
  • Peter Stuge 11
  • Samuli Seppänen 33

Effects of the ACK system on patches

The following random sample of patches was analyzed:

Analysis

Attachments (1)

  • gitlogparse.sh (1.8 KB) - added by Samuli Seppänen 8 years ago. A very ugly script used to generate statistics during ACK system review

Download all attachments as: .zip