Hi all,

As promised I did a brief review of our current ACK system:

<https://community.openvpn.net/openvpn/wiki/ACKSystemReview>

One thing that surprised me was that only on 2 occasions (out of ~60
"real" patches) a patch was modified during the review process. 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)

There seems to be a fair amount of delay from patch introduction to the
commit to Git repo. For individual patches the delay tends to be less
than a week, which I think is ok. For patchsets, the delay is much
longer (15-120 days). It's difficult to say whether we could reduce the
delay with advanced tools... after all, somebody (=human) has to review
the patches, whatever tools we use.

Due to a number of factors parsing the raw data was challenging...
unlike I assumed, finding discussions related to the patches was often
surprisingly difficult/impossible, although the "Acked-by" entries in
Git clearly show that an ACK was given at some point. Also, quite a few
patches (~20%) already go in without an ACK.

My personal feeling based is that we should probably focus the ACK
system to high-risk patches, e.g.

- patches touching critical parts of the code (rationale: better safe
than sorry)
- patches from new contributors (rationale: fix stylistic issues and
errors, ensure usefulness, educate)

We might also want to experiment with better tools to see if that speeds
up patch processing... that's definitely an issue with big patchsets.
Merging patches without a formal review might also make sense in some
scenarios, e.g. in low-risk areas which have an active maintainer.

Although the ACK system _does_ catch some problems and is clearly
useful, it also slows down flow of code into Git. This has the
side-effect that new code does not get real-life testing from users as
soon as it could. I think the key is apply each method (push quickly
into testing vs. formal review) where it makes most sense.

Also, one idea would be to review patches _after_ they've been merged to
Git, provided they make sense, i.e. nobody loudly complains about them.
For example, in PolarSSL's case, this would have allowed us to test the
code for ~4 months while the patches were being reviewed.

Anyways, let me know what you think... and please forgive me for not
getting back to this topic before Tuesday... I've been forbidden to work
during Easter by $fiancee :).

-- 
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock


[1] <https://community.openvpn.net/openvpn/wiki/PolarSSLintegration>
[2]
<https://community.openvpn.net/openvpn/wiki/GenericBuildsystemIntegration#Buildrevolutionpatchset>


Reply via email to