On Sat, 18 Jan 2014 01:44:46 -0500 Mike Frysinger <[email protected]> wrote:
> On Thursday 16 January 2014 12:44:57 Alexander Berntsen wrote: > > But basically, if someone authors a non-trivial patch, that person > > should *never* push themselves. Whoever reviews it should push it, > > adding the Reviewed-by field. The reviewer should also get an ACK by > > the team lead (via IRC or whatever) and add that to the commit > > before pushing. > > err, what ? that level of process really doesn't make sense here. > the project isn't large enough or have enough active people to > require it. > > any dev with commit access is trusted to do the right thing and to > shepherd their work through. > -mike Yes, adding these annotations could cause more overhead than necessary; because either a patch has been reviewed here on the ML or the patch is too trivial for review. In the former case you can find whom agreed on the list (and also relevant discussion), in the latter case there was no need for agreement. It feels like spending time on adding all these fields the correct way would cost more than the benefit it would cause; it can be good taste and best practice, but if it doesn't actually gain us something then it is doubtful whether we should spend effort on that. Commits already have an author and committer listed, if we just keep those fields correct; then we already have 1) the person whom written the patch (could be bug reporter, could be one of us) and 2) the person whom reviewed / acked (must do before committing) and added the patch. Commenting on them one by one: - Signed-off-by: Wrote (a substantial portion of) the patch As it is rarely written by multiple authors, the author field suffices. - Reviewed-by: Reviewed the patch thoroughly What extra information does this tell us? This can be seen on ML. - Tested-by: Tested the patch thoroughly This might make sense for the most important / big patches, but the occasional patch I'm in doubt about; you can't foresee everything in your testing anyway and it could be that errors are caught only after the patch has been committed. For an upstream project like the kernel such patch getting accepted into Git is critical, as a lot of kernel developers run from Git; however, downstream here at Portage the story is quite different. - Acked-by: Approved the concept but did not read the patch in detail (typically used by the maintainer of a specific portion, or our lead) It sounds odd to acknowledge the patch but not read it. I think Acked-by and/or Reviewed-by reflect the committer that is listed. - Suggested-by: Designed the implementation This would be rather rarely used; and when we need to use it, we probably forget this annotation exist. - Reported-by: Reported the bug/feature request This is occasionally mentioned in the commit message. We can however add these out of respect for the people involved; the alternative is to add them to the commit message, it kind of depends on how (in)formal we want to deal with this. The advantage of using annotations here is that you can quickly summarize ones involvement: eg. Person X has reported X bugs, reviewed Y patches, ... It feels like a bad practice, good taste thing for a smaller project. As a final remark I'd like to suggest to keep matters like review on the ML as much as possible; if we do too much on IRC, we'll lose track of things like "Why is your patch not the one you have send to the ML?". -- With kind regards, Tom Wijsman (TomWij) Gentoo Developer E-mail address : [email protected] GPG Public Key : 6D34E57D GPG Fingerprint : C165 AF18 AB4C 400B C3D2 ABF0 95B2 1FCD 6D34 E57D
signature.asc
Description: PGP signature
