Re: Giving your own patches your Reviewed-by

[Yi Liu <yi.l.liu@intel.com>]

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