Re: Giving your own patches your Reviewed-by

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

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.