Re: Giving your own patches your Reviewed-by

[CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>]

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