On 12/3/25 11:45, Daniel P. Berrangé wrote:
On Wed, Mar 12, 2025 at 10:45:29AM +0100, Markus Armbruster wrote:
I stumbled over commits that carry the author's Reviewed-by.
There may be cases where the recorded author isn't the lone author, and
the recorded author did some meaningful review of the patch's parts that
are not theirs. Mind that we do need all authors to provide their
Signed-off-by.
When the only Signed-off-by is from the recorded author, and there's
also their Reviewed-by, the Reviewed-by is almost certainly bogus.
Now, accidents happen, no big deal, etc., etc. I post this to hopefully
help reduce the accident rate :)
Is a commit with S-o-B and R-b from the matching author semantically any
different from a commit with an author S-o-B and NO other 3rd party tag
at all ?
The latter seems to be the more common case, with over a thousand examples
of commits with no 3rd party NNN-By tags.
$ cat > no-3rd-party.pl <<EOF
#!/usr/bin/perl
my @tags;
my $commit;
my $author;
while (<>) {
if (/^commit ([a-z0-9]+)$/) {
if (defined $commit) {
my $ok = 0;
foreach my $name (@tags) {
if ($name ne $author) {
$ok = 1;
last;
}
}
if (! $ok) {
print ("$commit: $author\n");
}
}
$commit = $1;
@tags = ();
} elsif (/^Author:\s+(.*)$/) {
$author = $1;
} elsif (/\s+(Signed-off-by|Acked-by|Reviewed-by):\s+(.*)$/) {
By including Tested-by the list is reduced by 80, not much, still 8%.
push @tags, $2;
}
}
EOF
$ git log --no-merges --since 'two years ago'| perl no-rb.pl | wc -l
1296
That is 8% of all commits in 2 years seemingly having no 3rd party
review.
Pretty much all of our core maintainers have commits exhibiting this,
so I won't include any names.
Some of these will be just a case of forgetting to copy the R-bs
from the list.
I suspect most are just a case of a maintainer making a pragmmatic
judgement call to merge something having given up waiting for review,
and not wanting to badger people into reviewing.
Most appear to be fairly trivial patches which is good. So that 8% of
commits is probably at least an order of magnitude less lines of code
changed in that timeframe, and the less "risky" code at that.
With regards,
Daniel
