On Tue, 13 May 2025 at 11:13, Alejandro Colomar <a...@kernel.org> wrote: > > Hi Jonathan, > > On Tue, May 13, 2025 at 10:39:21AM +0100, Jonathan Wakely wrote: > > On Mon, 12 May 2025 at 23:15, Alejandro Colomar <a...@kernel.org> wrote: > > > <https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by> > > > > > > Acked-by: may also be used by other stakeholders, such as people > > > with domain knowledge (e.g. the original author of the code > > > being modified), userspace-side reviewers for a kernel uAPI > > > patch or key users of a feature. > > > > > > [...] > > > > > > Acked-by: is also less formal than Reviewed-by:. For instance, > > > maintainers may use it to signify that they are OK with a patch > > > landing, but they may not have reviewed it as thoroughly as if a > > > Reviewed-by: was provided. Similarly, a key user may not have > > > carried out a technical review of the patch, yet they may be > > > satisfied with the general approach, the feature or the > > > user-facing interface. > > > > > > > My guess would be that it indicates approval for the patch, but Jim is > > > > not an approver for the C front end, so he can't approve this patch. > > > > > > That would be a Reviewed-by:. > > > > In GCC I've been using Reviewed-by: for anybody who reviews a patch, > > not necessarily approval from a maintainer. > > There are only seven occurrences of Acked-by on the gcc master branch. > > Four of them are duplicating a Reviewed-by: trailer in the same commit > > which seems unnecessary. > > > > > > > Acked-by: can be used by a reviewer when > > > they like the patch but haven't reviewed as seriously as a Reviewed-by: > > > tag would imply. It can also be used --like in this case-- for when > > > someone who can't approve it, still wants to express approval. > > > > > > > Does Acked-by: indicate something other than approval? > > > > > > There are degrees of approval. The formal one would be Reviewed-by:. > > > The informal one would be Acked-by:. > > > > Should we agree on > > > > > > When it's > > > > somebody who can't approve the patch, how is it different to > > > > Reviewed-by:? > > > > > > Someone who can't aapprove the patch wouldn't usually emit a > > > Reviewed-by:. Unless they feel so strongly qualified as an exception to > > > review the patch (e.g., if you review a patch for the man pages about > > > _Atomic, you could say you've Reviewed-by, because even when you don't > > > have commit rights, I'm going to trust your review more than my own). > > > > > > > I'm not overjoyed by the idea of trailers that mean something in some > > > > other project (e.g. the kernel) but are just co-opted to mean > > > > something slightly (or completely) different in the GCC repo without > > > > some kind of agreement from the community about what they mean *here*. > > > > > > I use them with the exact meaning of > > > <https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by>. > > > > Yes, I read that, and "maintainer" seems to have a different meaning > > to how we use it in GCC. > > > > "Acked-by: is meant to be used by those responsible for or involved > > with the affected code in one way or another. Most commonly, the > > maintainer when that maintainer neither contributed to nor forwarded > > the patch." > > That sounds like approval from a maintainer (in GCC we don't "forward" > > patches because we only have one tree, there are no subsystem trees > > where work is collected then forwarded to Linus). > > > > And the description of Reviewed-by: doesn't imply approval from a > > maintainer, it implies a thorough review by somebody knowledgeable > > about the area: > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#reviewer-s-statement-of-oversight > > Yes. That means for example it would be appropriate for you to emit > Reviewed-by: in the Linux man-pages project for a patch that changes > _Atomic stuff (as we have something about that pending). Or glibc > maintainers can emit them for manual pages about APIs that they work > with. > > Maintainer isn't a black-or-white thing, at least in some projects, like > the kernel or the man-pages. It's up to judgement of someone reading a > trailer to know what relation it has with the project or the specific > subsystem.
It is black-or-white for GCC, which is why I think deferring to the kernel docs is misleading for GCC. > The actual maintainer that does this, usually is the one that takes the > patch and commits it (adding its Signed-off-by). The one that signs > is supposed to know the reviewers, and what value brings each review. > So for example, if Joseph will be taking these patches from me, then > it's up to him to evaluate what an Acked-by: from James means. > > > I think the kernel's uses of Reviewed-by: and Acked-by: don't really > > map to GCC's development/review/approval model. > > > > For GCC, I think it would make more sense to use Reviewed-by: to mean > > somebody competent reviewed the patch, > > That's already acceptable by the kernel. It depends on what you > interpret by *competent* and by *reviewed*, but it can be acceptable. > > > and (if we feel it's needed) > > something like Approved-by: to mean formal approval by a maintainer > > who is able to approve patches in that area. > > I don't think this is necessary, because committers already know which > review means approval (by who emmitted it) and which don't. So you can > use Reviewed-by: for both formal approval for commit, and for strong > reviews. > > > If we do want to use Acked-by: for review (possibly informal, or not a > > thorough review) and Reviewed-by: for formal approval by a maintainer, > > I think Acked-by: is still useful for documenting other people doing > light reviews, or simply saying they like the idea of a patch, even if > they didn't thoroughly review it. > > > I'd be OK with that but I'd like to see it documented for GCC. The > > kernel docs don't really answer my questions about what it means for > > GCC, and it seems you and I are already using the trailers > > differently. > > Okay, that's up to you GCC maintainers to decide and document. Let me > know if I should change anything in my patches, and feel free to remove > tags when committing my patches if my tags don't match your expectations > of a commit trailer. > > > Cheers, > Alex > > -- > <https://www.alejandro-colomar.es/>
