Felipe ContrerasHi,
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
