Giving your own patches your Reviewed-by

Giving your own patches your Reviewed-by

[Markus Armbruster]

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 :)

Here's my quick & sloppy search for potentially problematic uses of
Reviewed-by:

$ git-log --since 'two years ago' | awk -F: '/^commit / { commit=$0 } /^Author: / { guy=$2 } /^    Reviewed-by: / { if ($2 == guy) { print commit; print guy } }'

Re: Giving your own patches your Reviewed-by

[Philippe Mathieu-Daudé]

Hi Markus,

(Cc'ing Yi, Clément and Zhenzhong for commit eda4c9b5b3c)

On 12/3/25 10:45, 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 :)
> 
> Here's my quick & sloppy search for potentially problematic uses of
> Reviewed-by:
> 
> $ git-log --since 'two years ago' | awk -F: '/^commit / { commit=$0 } /^Author: / { guy=$2 } /^    Reviewed-by: / { if ($2 == guy) { print commit; print guy } }'


Explaining some commits where I'm mentioned:

commit 1e0d4eb4ee7c909323bffc39bc348eb3174b426b
Author: Philippe Mathieu-Daudé <philmd@linaro.org>
Date:   Fri Apr 12 00:33:30 2024 -0700

     backends/tpm: Use qemu_hexdump_line() to avoid sprintf()

     sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
     Using qemu_hexdump_line() both fixes the deprecation warning and
     simplifies the code base.

     Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
     Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
     [rth: Keep the linebreaks every 16 bytes]
     Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
     Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
     Message-ID: <20240412073346.458116-12-richard.henderson@linaro.org>
     [PMD: Rebased]


I posted a patch with my S-o-b; Richard took it, improved and reposted
it with his S-o-b; I reviewed Richard's changes (and eventually merged).

commit 0fe4cac5dda1028c22ec3a6997e1b9155a768004
Author: Peter Maydell <peter.maydell@linaro.org>
Date:   Mon Jul 17 18:29:40 2023 +0200

     target/mips: Avoid shift by negative number in page_table_walk_refill()

     Coverity points out that in page_table_walk_refill() we can
     shift by a negative number, which is undefined behaviour
     (CID 1452918, 1452920, 1452922).  We already catch the
     negative directory_shift and leaf_shift as being a "bail
     out early" case, but not until we've already used them to
     calculated some offset values.

     The shifts can be negative only if ptew > 1, so make the
     bail-out-early check look directly at that, and only
     calculate the shift amounts and the offsets based on them
     after we have done that check. This allows
     us to simplify the expressions used to calculate the
     shift amounts, use an unsigned type, and avoids the
     undefined behaviour.

     Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
     [PMD: Check for ptew > 1, use unsigned type]
     Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
     Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
     Message-Id: <20230717213504.24777-3-philmd@linaro.org>

Peter posted the first patch, I reworked it and reposted,
Peter reviewed my changes.

commit c4380f7bcdcb68fdfca876db366782a807fab8f7
Author: Richard Henderson <richard.henderson@linaro.org>
Date:   Thu Jan 18 21:06:30 2024 +0100

     target/arm: Create arm_cpu_mp_affinity

     Wrapper to return the mp affinity bits from the cpu.

     Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
     Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
     Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
     Message-id: 20240118200643.29037-10-philmd@linaro.org
     Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Is this workflow making sense and accepted? Otherwise what should
we change? Maybe clarify along with the tags; or including all
Message-Id could make this easier to track?

Regards,

Phil.

Re: Giving your own patches your Reviewed-by

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Hi Markus,
>
(Cc'ing Yi, Clément and Zhenzhong for commit eda4c9b5b3c)
>
> On 12/3/25 10:45, 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 :)
>> 
>> Here's my quick & sloppy search for potentially problematic uses of
>> Reviewed-by:
>> 
>> $ git-log --since 'two years ago' | awk -F: '/^commit / { commit=$0 } /^Author: / { guy=$2 } /^    Reviewed-by: / { if ($2 == guy) { print commit; print guy } }'
>
>
> Explaining some commits where I'm mentioned:

[...]

> Is this workflow making sense and accepted? Otherwise what should
> we change? Maybe clarify along with the tags; or including all
> Message-Id could make this easier to track?

The workflow is good enough as is if you ask me.

Note that the patches you quoted all have more than one Signed-off-by.
My quick & sloppy search neglects to filter these out :)

Re: Giving your own patches your Reviewed-by

[Daniel P. Berrangé]

On Wed, Mar 12, 2025 at 11:03:04AM +0100, Philippe Mathieu-Daudé wrote:
> Hi Markus,
> 
> (Cc'ing Yi, Clément and Zhenzhong for commit eda4c9b5b3c)
> 
> On 12/3/25 10:45, 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 :)
> > 
> > Here's my quick & sloppy search for potentially problematic uses of
> > Reviewed-by:
> > 
> > $ git-log --since 'two years ago' | awk -F: '/^commit / { commit=$0 } /^Author: / { guy=$2 } /^    Reviewed-by: / { if ($2 == guy) { print commit; print guy } }'
> 
> 
> Explaining some commits where I'm mentioned:

> I posted a patch with my S-o-b; Richard took it, improved and reposted
> it with his S-o-b; I reviewed Richard's changes (and eventually merged).

That is totally fine, and an example of an expected scenario
that the simple shell 1-liner above can't eliminate.

> Is this workflow making sense and accepted? Otherwise what should
> we change? Maybe clarify along with the tags; or including all
> Message-Id could make this easier to track?

I don't see any problems with the commit messages as already written and
imho don't think we need to change it.

The main scenario that I would say is invalid is a commit containing
/only/ a matching rb/sob pair e.g.

 Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

as that is clearly either a "marking your own homework" case, or
someone has forgot to update the R-b/SoB tags.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Giving your own patches your Reviewed-by

[Yi Liu]

On 2025/3/12 18:03, Philippe Mathieu-Daudé wrote:
> Hi Markus,
> 
> (Cc'ing Yi, Clément and Zhenzhong for commit eda4c9b5b3c)
> 
> On 12/3/25 10:45, 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.

yeah, that might be sadly possible. :(

>>
>> Now, accidents happen, no big deal, etc., etc.  I post this to hopefully
>> help reduce the accident rate :)

a dumb question. Where can I view this issue?

>>
>> Here's my quick & sloppy search for potentially problematic uses of
>> Reviewed-by:
>>
>> $ git-log --since 'two years ago' | awk -F: '/^commit / { commit=$0 } 
>> /^Author: / { guy=$2 } /^    Reviewed-by: / { if ($2 == guy) { print 
>> commit; print guy } }'
> 
> 
> Explaining some commits where I'm mentioned:
> 
> commit 1e0d4eb4ee7c909323bffc39bc348eb3174b426b
> Author: Philippe Mathieu-Daudé <philmd@linaro.org>
> Date:   Fri Apr 12 00:33:30 2024 -0700
> 
>      backends/tpm: Use qemu_hexdump_line() to avoid sprintf()
> 
>      sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
>      Using qemu_hexdump_line() both fixes the deprecation warning and
>      simplifies the code base.
> 
>      Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>      Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>      [rth: Keep the linebreaks every 16 bytes]
>      Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>      Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>      Message-ID: <20240412073346.458116-12-richard.henderson@linaro.org>
>      [PMD: Rebased]
> 
> 
> I posted a patch with my S-o-b; Richard took it, improved and reposted
> it with his S-o-b; I reviewed Richard's changes (and eventually merged).
> 
> commit 0fe4cac5dda1028c22ec3a6997e1b9155a768004
> Author: Peter Maydell <peter.maydell@linaro.org>
> Date:   Mon Jul 17 18:29:40 2023 +0200
> 
>      target/mips: Avoid shift by negative number in page_table_walk_refill()
> 
>      Coverity points out that in page_table_walk_refill() we can
>      shift by a negative number, which is undefined behaviour
>      (CID 1452918, 1452920, 1452922).  We already catch the
>      negative directory_shift and leaf_shift as being a "bail
>      out early" case, but not until we've already used them to
>      calculated some offset values.
> 
>      The shifts can be negative only if ptew > 1, so make the
>      bail-out-early check look directly at that, and only
>      calculate the shift amounts and the offsets based on them
>      after we have done that check. This allows
>      us to simplify the expressions used to calculate the
>      shift amounts, use an unsigned type, and avoids the
>      undefined behaviour.
> 
>      Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>      [PMD: Check for ptew > 1, use unsigned type]
>      Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>      Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>      Message-Id: <20230717213504.24777-3-philmd@linaro.org>
> 
> Peter posted the first patch, I reworked it and reposted,
> Peter reviewed my changes.
> 
> commit c4380f7bcdcb68fdfca876db366782a807fab8f7
> Author: Richard Henderson <richard.henderson@linaro.org>
> Date:   Thu Jan 18 21:06:30 2024 +0100
> 
>      target/arm: Create arm_cpu_mp_affinity
> 
>      Wrapper to return the mp affinity bits from the cpu.
> 
>      Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>      Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>      Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>      Message-id: 20240118200643.29037-10-philmd@linaro.org
>      Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Is this workflow making sense and accepted? Otherwise what should
> we change? Maybe clarify along with the tags; or including all
> Message-Id could make this easier to track?

Commit eda4c9b5b3c is the similar case. Zhenzhong and Clément took
the patch from me and I was cced when Zhenzhong sent it out. I gave
my r-b after reviewing it.

-- 
Regards,
Yi Liu

Re: Giving your own patches your Reviewed-by

[CLEMENT MATHIEU--DRIF]

On 12/03/2025 13:54, Yi Liu wrote:
> Caution: External email. Do not open attachments or click links, unless 
> this email comes from a known sender and you know the content is safe.
> 
> 
> On 2025/3/12 18:03, Philippe Mathieu-Daudé wrote:
>> Hi Markus,
>>
>> (Cc'ing Yi, Clément and Zhenzhong for commit eda4c9b5b3c)
>>
>> On 12/3/25 10:45, 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.
> 
> yeah, that might be sadly possible. :(
> 
>>>
>>> Now, accidents happen, no big deal, etc., etc.  I post this to hopefully
>>> help reduce the accident rate :)
> 
> a dumb question. Where can I view this issue?
> 
>>>
>>> Here's my quick & sloppy search for potentially problematic uses of
>>> Reviewed-by:
>>>
>>> $ git-log --since 'two years ago' | awk -F: '/^commit / { commit=$0 }
>>> /^Author: / { guy=$2 } /^    Reviewed-by: / { if ($2 == guy) { print
>>> commit; print guy } }'
>>
>>
>> Explaining some commits where I'm mentioned:
>>
>> commit 1e0d4eb4ee7c909323bffc39bc348eb3174b426b
>> Author: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Date:   Fri Apr 12 00:33:30 2024 -0700
>>
>>      backends/tpm: Use qemu_hexdump_line() to avoid sprintf()
>>
>>      sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
>>      Using qemu_hexdump_line() both fixes the deprecation warning and
>>      simplifies the code base.
>>
>>      Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>      Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>>      [rth: Keep the linebreaks every 16 bytes]
>>      Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>      Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>      Message-ID: <20240412073346.458116-12-richard.henderson@linaro.org>
>>      [PMD: Rebased]
>>
>>
>> I posted a patch with my S-o-b; Richard took it, improved and reposted
>> it with his S-o-b; I reviewed Richard's changes (and eventually merged).
>>
>> commit 0fe4cac5dda1028c22ec3a6997e1b9155a768004
>> Author: Peter Maydell <peter.maydell@linaro.org>
>> Date:   Mon Jul 17 18:29:40 2023 +0200
>>
>>      target/mips: Avoid shift by negative number in 
>> page_table_walk_refill()
>>
>>      Coverity points out that in page_table_walk_refill() we can
>>      shift by a negative number, which is undefined behaviour
>>      (CID 1452918, 1452920, 1452922).  We already catch the
>>      negative directory_shift and leaf_shift as being a "bail
>>      out early" case, but not until we've already used them to
>>      calculated some offset values.
>>
>>      The shifts can be negative only if ptew > 1, so make the
>>      bail-out-early check look directly at that, and only
>>      calculate the shift amounts and the offsets based on them
>>      after we have done that check. This allows
>>      us to simplify the expressions used to calculate the
>>      shift amounts, use an unsigned type, and avoids the
>>      undefined behaviour.
>>
>>      Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>      [PMD: Check for ptew > 1, use unsigned type]
>>      Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>      Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>      Message-Id: <20230717213504.24777-3-philmd@linaro.org>
>>
>> Peter posted the first patch, I reworked it and reposted,
>> Peter reviewed my changes.
>>
>> commit c4380f7bcdcb68fdfca876db366782a807fab8f7
>> Author: Richard Henderson <richard.henderson@linaro.org>
>> Date:   Thu Jan 18 21:06:30 2024 +0100
>>
>>      target/arm: Create arm_cpu_mp_affinity
>>
>>      Wrapper to return the mp affinity bits from the cpu.
>>
>>      Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>      Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>      Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>      Message-id: 20240118200643.29037-10-philmd@linaro.org
>>      Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> Is this workflow making sense and accepted? Otherwise what should
>> we change? Maybe clarify along with the tags; or including all
>> Message-Id could make this easier to track?
> 
> Commit eda4c9b5b3c is the similar case. Zhenzhong and Clément took
> the patch from me and I was cced when Zhenzhong sent it out. I gave
> my r-b after reviewing it.

Some other commits of the same series were in a similar situation: 
initially written by me and slightly changed by Zhenzhong.
These are not caught by one-liner above because I deliberately didn't 
give an rb.

According to Daniel it seems to be ok to review a co-authored patch but
is this considered a last resort?

> 
> -- 
> Regards,
> Yi Liu

Re: Giving your own patches your Reviewed-by

[Markus Armbruster]

CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> writes:

> On 12/03/2025 13:54, Yi Liu wrote:

[...]

>> Commit eda4c9b5b3c is the similar case. Zhenzhong and Clément took
>> the patch from me and I was cced when Zhenzhong sent it out. I gave
>> my r-b after reviewing it.
>
> Some other commits of the same series were in a similar situation: 
> initially written by me and slightly changed by Zhenzhong.
> These are not caught by one-liner above because I deliberately didn't 
> give an rb.
>
> According to Daniel it seems to be ok to review a co-authored patch but
> is this considered a last resort?

Ideally, patch review is a fresh look at things.  If you can't get such
"outsider" review, and it's not for lack of trying, you work with the
maintainer to figure out what to do.  If you *are* the maintainer, you
make a judgement call.

That said, recording meaningful work can't be wrong.  If you feel you
did meaningful review work, go ahead and add your R-by.

Re: Giving your own patches your Reviewed-by

[Philippe Mathieu-Daudé]

On 12/3/25 10:45, 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 :)
> 
> Here's my quick & sloppy search for potentially problematic uses of
> Reviewed-by:
> 
> $ git-log --since 'two years ago' | awk -F: '/^commit / { commit=$0 } /^Author: / { guy=$2 } /^    Reviewed-by: / { if ($2 == guy) { print commit; print guy } }'
> 

Since you are looking at this, it reminds me an orthogonal discussion
we refresh from time to time at the KVM forum conference: is it OK to
merge unreviewed patches?

Re: Giving your own patches your Reviewed-by

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Since you are looking at this, it reminds me an orthogonal discussion
> we refresh from time to time at the KVM forum conference: is it OK to
> merge unreviewed patches?

We should make an effort to get reviews.

If we can't get any within a reasonable time, merging can be okay.

"Reasonable time" depends on the patches.  Shorter for a build fix than
for a new feature.

"Can be" also depends on the patches.  How risky do they feel?

Judgement call.  The first call should commonly be "try harder to get
reviews".

Re: Giving your own patches your Reviewed-by

[Daniel P. Berrangé]

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+(.*)$/) {
	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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Giving your own patches your Reviewed-by

[Philippe Mathieu-Daudé]

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

Re: Giving your own patches your Reviewed-by

[bibo mao]

Ah, It is a pity and bad news that I contribute almost 30% of it :(
LoongArch system actually needs more people participation and I need 
notice this also. It should happens in future again in LoongArch subsystem.

Any reviewing comments is welcome and I will slow down for deeper 
considerations. And it is my pleasure to work in this open source area 
and for better goals together.

Regards
Bibo Mao
On 2025/3/12 下午5:45, 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 :)
> 
> Here's my quick & sloppy search for potentially problematic uses of
> Reviewed-by:
> 
> $ git-log --since 'two years ago' | awk -F: '/^commit / { commit=$0 } /^Author: / { guy=$2 } /^    Reviewed-by: / { if ($2 == guy) { print commit; print guy } }'
> 

Re: Giving your own patches your Reviewed-by

[Markus Armbruster]

bibo mao <maobibo@loongson.cn> writes:

> Ah, It is a pity and bad news that I contribute almost 30% of it :(
> LoongArch system actually needs more people participation and I need notice this also. It should happens in future again in LoongArch subsystem.
>
> Any reviewing comments is welcome and I will slow down for deeper considerations. And it is my pleasure to work in this open source area and for better goals together.

Accidents & misunderstandings of process happen; we're all human.  Thank
you for your contributions!