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. 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/>
signature.asc
Description: PGP signature
