Only 27% of reviewed-by tags are explicit, and much more

[Felipe Contreras <felipe.contreras@gmail.com>]

Hi,

This is what SubmittingPatches says about the Reviewed-by commit
message trailers:

 `Reviewed-by:`, unlike the other tags, can only be offered by the
  reviewer and means that she is completely satisfied that the patch
  is ready for application.  It is usually offered only after a
  detailed review.

However, a quick look at the recently given Reviewed-by trailers shows
that isn't always the case: sometimes the patch submitter adds the
trailer [1], and sometimes it's the maintainer himself [2].

Any generous reading of the relevant mails would conclude that the label
was meant to be given, albeit it was not expressly so and thus strictly
speaking not following the guidelines to a T:

  1. 3127ff90ea (packfile-uri.txt: fix blobPackfileUri description)

  This looks good - it's exactly the same as the change I approved
  previously [3]

  2. 4e689d8171 (dir: update stale description of treat_directory())

  Looks good to me [4]

  3. 5f03e5126d (refs: cleanup directories when deleting packed ref)

  this v2 patch looks good to me [5]

These are just the last three, but a cursory look at others show a
similar pattern.

This prompted me to write a script [6] to programmatically find statistics
about these trailers. Obviously it isn't perfect (as all software); it
tries to avoid human fuzziness (like people pasting other patches with
scissors [-- >8 --], or just straight put pasting the patch [^From: ]), but
even so there are instances I manually had to skip [7].

Over the past 10 years my script found 1604 commits with a Reviewed-by
label, of those only 434 were expressly given.

That's merely a 27%.

This means what's written in the guidelines is not a widespread
practice.


Not content with this finding I went further and extracted statistics
specific about every reviewer.

Doing so I found this interesting piece of information:

  Jonathan Nieder: 67% (211/314)
  Jeff King: 6% (15/248)

Jonathan Nieder is by far the person with most reviews, followed by Jeff
King, but they could not be more different. While Jonathan often offers
his Reviewed-by tag explicitly, Jeff rarely does so. That's why Jonathan
has 211 explicit Reviewed-bys, while Jeff only 15.

You can see the histogram of frequency of attributed Reviewed-by
trailers vs. percentage of explicit trailers here [8]. With the
exception of Jonathan Nieder, almost *everyone* gets their Reviewed-by
trailers in an implicit way.

Some heavyweights in the 20%-30% bracket are Stefan Beller (46/190),
Elijah Newren (12/47), Jonathan Tan (7/32), Ævar Arnfjörð Bjarmason
(2/9). Other than that most reviewers are below 10% (40/673).


So where did this guideline come from?

Looking back at history, the patch comes from Ramkumar Ramachandra, who
while trying to improve something completely unrelated, stumbled upon
the following advice:

 - Again from Documentation/SubmittingPatches, I learned a while ago
   that Reviewed-by, unlike Acked-by, can only be offered by the
   reviewer and means that she is satisfied that the patch is ready
   for application.

Do you care to guess who gave that advice? Our top explicit
reviewed-byer: Jonathan Nieder [10]. And this comes straight up from the
Linux project; it's not a Git thing.

So, is it fair to say the explicit Reviewed-by trailer rule in
Documentation/SubmittingPatches is accurate? I would say unequivocally
*no*. Only one person follows it religiously, everyone else seems to be
relying on either the reviewer or the maintainer to apply their best
judgement.

And that includes our second most prolific reviewer: Jeff King, who over
the past 10 years has only given his explicit Reviewed-by 15 times (6%
of the time).

I think it's fair to say our Documentation/SubmittingPatches needs to be
updated.

Here are the top 20 reviewers over the past 10 years with their
corresponding explicit over total Reviewed-by count:

  1. Jonathan Nieder: 67% (211/314)
  2. Jeff King: 6% (15/248)
  3. Stefan Beller: 24% (46/190)
  4. Matthieu Moy: 9% (13/131)
  5. Eric Sunshine: 14% (17/116)
  6. Derrick Stolee: 6% (7/102)
  7. Taylor Blau: 32% (27/83)
  8. Michael Haggerty: 45% (25/55)
  9. Elijah Newren: 25% (12/47)
  10. Johannes Schindelin: 5% (2/35)
  11. Jonathan Tan: 21% (7/32)
  12. Nguyễn Thái Ngọc Duy: 20% (6/30)
  13. Ronnie Sahlberg: 31% (5/16)
  14. SZEDER Gábor: 0% (0/14)
  15. Luke Diamand: 0% (0/13)
  16. Felipe Contreras: 8% (1/12)
  17. Johannes Sixt: 40% (4/10)
  18. Ævar Arnfjörð Bjarmason: 22% (2/9)
  19. Stefano Lattarini: 37% (3/8)
  20. Torsten Bögershausen: 0% (0/7)

Only *two* reviewers are above 40%.

If that's not clear enough, I don't know what is.

Cheers.

[1] https://lore.kernel.org/git/1d825dfdc70b8c658c4b6317310706bb6386f468.1620432501.git.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/20210511064554.59924-1-dyroneteng@gmail.com/
[3] https://lore.kernel.org/git/20210524154354.268200-1-jonathantanmy@google.com/
[4] https://lore.kernel.org/git/CABPp-BFudyBP8c_ROmxDEeuu5AfdoyVW8hNPYqVdFPFNofJnCQ@mail.gmail.com/
[5] https://lore.kernel.org/git/YJnfV7op4yhyLqdg@coredump.intra.peff.net/
[6] https://gist.github.com/felipec/f1289a48e31195ee1f36c28ca6e482db
[7] https://lore.kernel.org/git/xmqqsgg7u3js.fsf@gitster.c.googlers.com/
[8] https://i.imgur.com/NuvePpZ.png
[9] https://lore.kernel.org/git/1285994263-31154-1-git-send-email-artagnon@gmail.com/
[10] https://lore.kernel.org/git/20101001053721.GB6184@burratino/

-- 
Felipe Contreras