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é <phi...@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é <phi...@linaro.org>
    Reviewed-by: Stefan Berger <stef...@linux.ibm.com>
    [rth: Keep the linebreaks every 16 bytes]
    Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
    Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
    Message-ID: <20240412073346.458116-12-richard.hender...@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.mayd...@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.mayd...@linaro.org>
    [PMD: Check for ptew > 1, use unsigned type]
    Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
    Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
    Message-Id: <20230717213504.24777-3-phi...@linaro.org>

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

commit c4380f7bcdcb68fdfca876db366782a807fab8f7
Author: Richard Henderson <richard.hender...@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.hender...@linaro.org>
    Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
    Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
    Message-id: 20240118200643.29037-10-phi...@linaro.org
    Signed-off-by: Peter Maydell <peter.mayd...@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.

Reply via email to