From: Felipe Contreras <hidden> Date: 2021-06-18 03:47:32
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
From: Eric Sunshine <hidden> Date: 2021-06-18 05:06:06
On Thu, Jun 17, 2021 at 11:47 PM Felipe Contreras
[off-list ref] wrote:
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].
Here are the top 20 reviewers over the past 10 years with their
corresponding explicit over total Reviewed-by count:
...
5. Eric Sunshine: 14% (17/116)
Does your script check cover letters? Based upon a quick glance at it,
it doesn't seem to.
Although I've reviewed thousands of patches over the years, I almost
never give my Reviewed-by:; it is an exceedingly rare occurrence.
However, when I do give it, it's almost always in response to the
cover letter (saying "this entire series is reviewed by <me>"), not in
response to individual patches. I've seen other reviewers do so, as
well. So, if your script doesn't take cover letters into account, then
you might want to revise it to do so in order to get a more accurate
picture. In fact, if my memory is correct, some reviewers give their
Reviewed-by: to an entire series in response to one of the patches
rather than to the cover letter, so perhaps you can come up with a
heuristic to identify those cases too.
From: Felipe Contreras <hidden> Date: 2021-06-18 16:00:55
Eric Sunshine wrote:
On Thu, Jun 17, 2021 at 11:47 PM Felipe Contreras
[off-list ref] wrote:
quoted
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].
Here are the top 20 reviewers over the past 10 years with their
corresponding explicit over total Reviewed-by count:
...
5. Eric Sunshine: 14% (17/116)
Does your script check cover letters? Based upon a quick glance at it,
it doesn't seem to.
Not really, that's a good point.
Although I've reviewed thousands of patches over the years, I almost
never give my Reviewed-by:; it is an exceedingly rare occurrence.
However, when I do give it, it's almost always in response to the
cover letter (saying "this entire series is reviewed by <me>"), not in
response to individual patches. I've seen other reviewers do so, as
well. So, if your script doesn't take cover letters into account, then
you might want to revise it to do so in order to get a more accurate
picture.
I've updated the script to consider all responses to the cover letter
that start with 'Re: '.
In fact, if my memory is correct, some reviewers give their
Reviewed-by: to an entire series in response to one of the patches
rather than to the cover letter, so perhaps you can come up with a
heuristic to identify those cases too.
That's true. Depending whether or not that's the exception or the rule it
might make sense to simply consider all Reviewed-by responses to apply
to the entire series and make the heuristic match the cases where it's
only for a single patch.
Anyway, with the updated script the explicit reviewed-bys are 40%, and
here are the stats:
1. Jonathan Nieder: 80% (254/314)
2. Jeff King: 7% (18/248)
3. Stefan Beller: 28% (54/190)
4. Matthieu Moy: 64% (84/131)
5. Eric Sunshine: 38% (45/116)
6. Derrick Stolee: 11% (12/102)
7. Taylor Blau: 46% (39/83)
8. Michael Haggerty: 76% (42/55)
9. Elijah Newren: 25% (12/47)
10. Johannes Schindelin: 11% (4/35)
11. Jonathan Tan: 28% (9/32)
12. Nguyễn Thái Ngọc Duy: 20% (6/30)
13. Ronnie Sahlberg: 100% (16/16)
14. SZEDER Gábor: 0% (0/14)
15. Luke Diamand: 7% (1/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)
Jonathan Nieder, Matthieu Moy, Michael Haggerty and Ronnie Sahlberg got
considerably more percentage, but others didn't.
The histogram [1] shows an increase in the 60%-100% range, in particular
the 80% range (thanks to Jonathan Nieder), but there's still plenty
below 50%.
You got considerably more, from 17 to 45, but still pretty far from a
100%.
I think the conclussion still stands: Reviewed-by isn't always expressly
given, in fact, the majority of case it isn't.
Cheers.
[1] https://i.imgur.com/gr6YjsZ.png
--
Felipe Contreras
From: Felipe Contreras <hidden> Date: 2021-06-18 16:05:57
Bagas Sanjaya wrote:
On 18/06/21 10.47, Felipe Contreras wrote:
quoted
I think it's fair to say our Documentation/SubmittingPatches needs to be
updated.
With what new procedure? After any reviews?
Something like this:
. `Reviewed-by:`, means the reviewer is completely satisfied with the
patch after a detailed analysis. Unlike other tags, it's recommended
that the reviewers themselves offer it.
It is recommended, not a must.
--
Felipe Contreras
From: Eric Sunshine <hidden> Date: 2021-06-19 07:00:55
On Fri, Jun 18, 2021 at 12:00 PM Felipe Contreras
[off-list ref] wrote:
Eric Sunshine wrote:
quoted
On Thu, Jun 17, 2021 at 11:47 PM Felipe Contreras
[off-list ref] wrote:
quoted
Here are the top 20 reviewers over the past 10 years with their
corresponding explicit over total Reviewed-by count:
...
5. Eric Sunshine: 14% (17/116)
Does your script check cover letters? Based upon a quick glance at it,
it doesn't seem to.
Not really, that's a good point.
quoted
Although I've reviewed thousands of patches over the years, I almost
never give my Reviewed-by:; it is an exceedingly rare occurrence.
However, when I do give it, it's almost always in response to the
cover letter (saying "this entire series is reviewed by <me>"), not in
response to individual patches. I've seen other reviewers do so, as
well. So, if your script doesn't take cover letters into account, then
you might want to revise it to do so in order to get a more accurate
picture.
I've updated the script to consider all responses to the cover letter
that start with 'Re: '.
Anyway, with the updated script the explicit reviewed-bys are 40%, and
here are the stats:
5. Eric Sunshine: 38% (45/116)
You got considerably more, from 17 to 45, but still pretty far from a
100%.
The numbers produced by your script don't agree with my own
investigative spelunking through my own mailbox. What I found is that,
via 33 emails, I've given my Reviewed-by: to 133 patches[1]. If the
116 computed by your script is accurate, then that means that not all
of my Reviewed-by:'s made it into the project, which is believable.
Nevertheless, according to my own mailbox -- accounting for 133
patches -- I have almost certainly given an explicit Reviewed-by: to
all 116 of those patches your script found, which means the script
output should probably be "100% (116/116)". This mismatch between your
script output and my own investigation throws doubt upon the
script-generated tallies for other people, as well.
(Aside: I also gave another 37 explicit Reviewed-by:'s via another 7
emails[2], however, the authors of those series ended up re-rolling
after my Reviewed-by:, so I did not count those in the numbers I
presented above. There also have been several times when patch
submitters added my Reviewed-by: in error[3], though I don't think
those ever made it into the project.)
[1]: explicitly given Reviewed-by: (patch-count, message-id)
10: CAPig+cSXi7Ct48gqkBVvBtOm6bMqDhPcxXeiK3ZytAitZXNT5Q@mail.gmail.com
7: CAPig+cTt-TyOR8kc6YvBVLpf-bgFdJ+FVYA2NvG_Vvn7tMbBkQ@mail.gmail.com
1: CAPig+cS7SEARwoBn25SsxhkvdJfDe56FyVjDGk+sJq2kXfDbjQ@mail.gmail.com
1: CAPig+cS8hvX4GCsnfLBnQ4Q_AkUad=bw7rjVcaOqSEqcLZvx8w@mail.gmail.com
1: CAPig+cQxvq3MzyB3e8-ZeVSdCot04=9p4L8CZRnpYbrmnR70_g@mail.gmail.com
39: CAPig+cTxfheSPHJvC3_=jQjef0S7FiMEWCQ71ER7epfpeD_5OQ@mail.gmail.com
1: CAPig+cSDxBFhnJSmH+WzNZBjY4O0OczazZ7EOqn1P=zgL+ec4g@mail.gmail.com
4: CAPig+cRor4UXXZhoAkhOQfe6W+oC84YFmA-KwpLspuEh_A4Zng@mail.gmail.com
1: CAPig+cTgoD-GvpoBQ6tcGX4T2XhuKccJAZ40P76vxVD_PfEc9A@mail.gmail.com
1: CAPig+cTcqSa6AfeMQivnSdL=y2+WWw2MtSavDciMc84RcKURMA@mail.gmail.com
5: CAPig+cQVaMOKtbUCWdZqYDO8ZUZkVcSJH14S=2xrZiDVJ59Xdg@mail.gmail.com
6: CAPig+cQ2NRO4yaFkcGmUpY3TZcWkdg-vu6d7Fq7JgHzYSkcRgg@mail.gmail.com
1: CAPig+cS4Bj4N8d1a29z8=f30owOec1pB=yF32ZUPmDH2Tu2kXA@mail.gmail.com
1: CAPig+cQPbwM0+6yruK0VKKq2ujFLoCLogS7eQNN7WWgRjG5V0w@mail.gmail.com
6: CAPig+cR9i1a7pxOxV4QU2TnoJWKn4mHHVT2tG3+uRysw=sc6qQ@mail.gmail.com
7: CAPig+cR3diDfn893-ExKNZps=C7Z=M7DFAy-zbJzH3wKCmxVeQ@mail.gmail.com
1: CAPig+cRimDGFDk7A2p2rKpJ2GR27_R=BJdpyPK4xeyDU1TruWQ@mail.gmail.com
5: CAPig+cQPFzgEgdNUJSa9baUvc4BuJp8BHOLA88QJqW8N3hE8AQ@mail.gmail.com
11: CAPig+cS8sJn9tV2kW=ASN2wTtKiK-H5oa8bThiiGfu_vMv7DoQ@mail.gmail.com
1: 20160216050915.GA5765@flurp.local
1: CAPig+cSxVdZN_wr3XuqDGuKn14J3B7s=S8OoH19v+AjMvcX6+Q@mail.gmail.com
1: CAPig+cTaOTfwzodKSabdy9HFbF66RuEXwmvjZ8QuQVFMaVpA7w@mail.gmail.com
1: CAPig+cSSdGeMuV1XLqROXvSeYfmkNc_N7E_pzfJWdDR6wfD80A@mail.gmail.com
2: CAPig+cRySp4_uJYAwC-PY_Yh-YjBb0y1Tq=TwoD7d-Bpb_YANg@mail.gmail.com
5: CAPig+cQtYjyj2dVFX_8CjyacRPd+dU6aMMXYGsoX9+q+zgjdZQ@mail.gmail.com
1: CAPig+cSuCouNCuKa99mct4UMPykuMVy3+7sqB6y+v+UtP2oeTw@mail.gmail.com
2: CAPig+cSC8RZJ-+uP=ZExVH2ZyexfQmLjzdjoBA7yuWkdYE4EGQ@mail.gmail.com
3: CAPig+cQ-yLnjrsB1E-7=UXfGzuJHat6YtfS8EVRNP2dcjj_6TA@mail.gmail.com
1: CAPig+cT0-ftZZyRORx-W2_Nit6XdgrpiyGS=pRjGtHoz1jW+Kg@mail.gmail.com
1: CAPig+cQb6k8ktWR5Fz+dstfhFj5dZ+kpfzVk1Vp5piYJ3zy4rg@mail.gmail.com
1: CAPig+cRd5+9nq3YNA6e1R_tvmBTHByOg-=KRWG1m3Fxb0e_vFQ@mail.gmail.com
1: CAPig+cRpD0B2YRZYyJVUiM70AFcduTPOuJsuRFFKZE+bXttW1Q@mail.gmail.com
1: CAPig+cSuBfuzL-NXYkvFoz5+jPrEUNfTqoMf-iAYyMSv3jDsqg@mail.gmail.com
[2]: explicitly given Reviewed-by: but submitter ended up re-rolling
6: CAPig+cRz4stVQWFD-NroVHft2xFvyZJi1ePX9T4zZ3k3=X6ZrA@mail.gmail.com
6: CAPig+cSmb9wFCV+9PS4LYfd3hAH5s6ifRk8orVv+e2Q=h7F3Ag@mail.gmail.com
6: CAPig+cQ4n5j4Q-WF-0cd=2+5eSAaimh3A7La+8Fe9Ox4anjtBQ@mail.gmail.com
1: 20160327194948.GA9295@flurp.local
12: CAPig+cTrh4u7vgQRXOT0a-5St2a6TV4qfhOMCVSetbQD0kGTrQ@mail.gmail.com
1: CAPig+cTCRq9VCT7t8E9yjk4QcHYB2_qeBwGB_31keB4nTjkLkA@mail.gmail.com
5: 20150519004356.GA12854@flurp.local
[3]: Reviewed-by: not explicitly given
11: CAPig+cRmz2C7mAzc7Z=ZStAxd3qDSmbC4sbwyLGKqvkf2yzVPA@mail.gmail.com
1: CAPig+cQG16AhLPMeOFAw1GF81oXivFSDHvQ5B8kX20YGAT_BxQ@mail.gmail.com
2: CAPig+cRryaafwP4gBLe_6ebWZo12HWtEC6e2CbbP6a5gVh6W4w@mail.gmail.com
7: CAPig+cR0jG65LbopxqPpidaaNUSTRq9tboZpv0RPWyWUkSEGUw@mail.gmail.com
1: CAPig+cR=u_ak_=J=gSAWfiNB01R7FBG+bCrx+k1HNAE0xHtwFQ@mail.gmail.com
From: Felipe Contreras <hidden> Date: 2021-06-19 21:45:46
Eric Sunshine wrote:
On Fri, Jun 18, 2021 at 12:00 PM Felipe Contreras
[off-list ref] wrote:
quoted
I've updated the script to consider all responses to the cover letter
that start with 'Re: '.
Anyway, with the updated script the explicit reviewed-bys are 40%, and
here are the stats:
5. Eric Sunshine: 38% (45/116)
You got considerably more, from 17 to 45, but still pretty far from a
100%.
The numbers produced by your script don't agree with my own
investigative spelunking through my own mailbox. What I found is that,
via 33 emails, I've given my Reviewed-by: to 133 patches[1]. If the
116 computed by your script is accurate, then that means that not all
of my Reviewed-by:'s made it into the project, which is believable.
Nevertheless, according to my own mailbox -- accounting for 133
patches -- I have almost certainly given an explicit Reviewed-by: to
all 116 of those patches your script found, which means the script
output should probably be "100% (116/116)".