Re: [PATCH v5 1/1] [gdb]: add git trailer information on gdb/MAINTAINERS

[Kevin Buettner <kevinb@redhat.com>]

On Thu,  5 Oct 2023 13:35:34 +0200
Guinevere Larsen <blarsen@redhat.com> wrote:

> In the GDB BoF in 2023's GNU tools cauldron it was discussed and agreed
> that Acked-by is already in use to represent partial approvals for
> projects like the Linux Kernel and QEMU, so it makes sense to use it
> similarly on this project.
[...]
> + - Acked-By:
> +
> +   Used by a responsible or global maintainer when the patch touches multiple
> +   areas of GDB, and the maintainer in question is only approving some of
> +   those areas.  When using this tag, add the area(s) at the end of the text.
> +   This tag is also often described as "partial approval"
> +   Usage: "Acked-By: Your Name <your@email> (area)"

To indicate partial approval or partial review or even partial testing,
I recommend that we simply add "(area)" after the tag.

E.g.  if a reviewer has partially reviewed a patch and finds the parts
that change the gdbserver portions of it to be acceptable, they might
say:

Reviewed-by: Random J Developer <random@developer.example.org> (gdbserver)

The same could be done with the "Approved-by" tag.  If this is done,
then multiple partial approvals may be needed.

I think that an "Acked-by" tag could be useful when someone else has
done a detailed analysis and an area or global maintainer wishes to
indicate that they've read or skimmed the review, but have not done
any detailed analysis themselves.  It might also be used to indicate
agreement with direction of the patch after reading the commit log. 
But I don't think that it should be considered as strong as
"Reviewed-by" and definitely not as strong as "Approved-by".

I reached those conclusions after looking at how two other projects,
the git and kernel projects, use "Acked-by" and "Reviewed-by".  The
links that I looked at are:

https://git-scm.com/docs/SubmittingPatches
https://docs.kernel.org/process/submitting-patches.html

The git project has this to say about "Acked-by":

    Acked-by: says that the person who is more familiar with the area
    the patch attempts to modify liked the patch.

For the git project, "Reviewed-by" is much stronger:

    Reviewed-by: unlike the other tags, can only be offered by the
    reviewers themselves when they are completely satisfied with the
    patch after a detailed analysis.

Here's how the kernel project uses Acked-by:

    Aked-by:  is often used by the maintainer of the affected code
    when that maintainer neither contributed to nor forwarded the
    patch.

    Acked-by:  is not as formal as Signed-off-by:.  It is a record
    that the acker has at least reviewed the patch and has indicated
    acceptance.  Hence patch mergers will sometimes manually convert
    an acker's "yep, looks good to me" into an Acked-by:  (but note
    that it is usually better to ask for an explicit ack).

    Acked-by:  does not necessarily indicate acknowledgement of the
    entire patch.  For example, if a patch affects multiple subsystems
    and has an Acked-by:  from one subsystem maintainer then this
    usually indicates acknowledgement of just the part which affects
    that maintainer's code.  Judgement should be used here.  When in
    doubt people should refer to the original discussion in the
    mailing list archives.

However, for the kernel project, "Acked-by" is definitely not as
strong as "Reviewed-by":

    Reviewed-by:, instead, indicates that the patch has been reviewed
    and found acceptable according to the Reviewer's Statement:

    Reviewer's statement of oversight

    By offering my Reviewed-by:  tag, I state that:

	a. I have carried out a technical review of this patch to
	   evaluate its appropriateness and readiness for inclusion
	   into the mainline kernel.

	b. Any problems, concerns, or questions relating to the patch
	   have been communicated back to the submitter.  I am
	   satisfied with the submitter's response to my comments.

	c. While there may be things that could be improved with this
	   submission, I believe that it is, at this time, (1) a
	   worthwhile modification to the kernel, and (2) free of
	   known issues which would argue against its inclusion.

	d. While I have reviewed the patch and believe it to be
	   sound, I do not (unless explicitly stated elsewhere) make
	   any warranties or guarantees that it will achieve its
	   stated purpose or function properly in any given situation.

    A Reviewed-by tag is a statement of opinion that the patch is an
    appropriate modification of the kernel without any remaining
    serious technical issues.  Any interested reviewer (who has done
    the work) can offer a Reviewed-by tag for a patch.  This tag
    serves to give credit to reviewers and to inform maintainers of
    the degree of review which has been done on the patch. 
    Reviewed-by:  tags, when supplied by reviewers known to understand
    the subject area and to perform thorough reviews, will normally
    increase the likelihood of your patch getting into the kernel.

For the GDB project, I have been thinking of "Approved-by" as meaning
"Reviewed-by"-with-approval.  But I don't think that should
necessarily be the case.  I think it makes sense for a global
maintainer to see that there are "Reviewed-by" and possibly "Acked-by"
tags and then make a decision to approve the patch (giving it an
"Approved-by" tag) without necessarily having done a detailed analysis
(i.e.  review) themselves.

Kevin