Re: Giving your own patches your Reviewed-by

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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