Changes between Version 19 and Version 20 of ACKSystemReview

Timestamp:

04/05/12 12:53:02 (12 years ago)

Author:

Samuli Seppänen

Comment:

--

ACKSystemReview

@@ -35 +35 @@
  * [http://en.wikipedia.org/wiki/Design_rationale Rationale management] (feature-ACKs) 
  * [http://en.wikipedia.org/wiki/Static_testing 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 ==
@@ -86 +88 @@
 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.
- * ''Patchset'' refers to a major patchset with 10 or more patches.
+ * ''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
-
-Here's the raw data:
+ * ''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||-||Alon Bar-Lev||No||2||0||
-||2012-03-22||2012-02-29||fbae7d2||build: plugins: properly use CC, CFLAGS and LDFLAGS||-||Alon Bar-Lev||No||23||7||
-||2012-03-22||2012-02-29||553d95d||cleanup: memcmp.c: remove unused source||-||Alon Bar-Lev||No||23||16||
-||2012-03-22||2012-02-29||51bd56f||build: autotools: first pass of trivial autotools changes||-||Alon Bar-Lev||No||23||7||
+||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||[http://thread.gmane.org/gmane.network.openvpn.devel/5401/focus=6204 Yes]||45||3||
@@ -151 +153 @@
 ||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||?||
+||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||-||
@@ -159 +161 @@
 ||2011-03-25||2009-09-24||e478770||* make possible to x-compile openvpn/win32 in Linux||-||JuanJo Ciarlante||No||141||-||
 
-= Analysis =
+= 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.
+ 
+