Re: [gentoo-portage-dev] Signing off patches

[Tom Wijsman]

On Sat, 18 Jan 2014 01:44:46 -0500
Mike Frysinger <vapier@g.o> 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  : TomWij@g.o
GPG Public Key  : 6D34E57D
GPG Fingerprint : C165 AF18 AB4C 400B C3D2  ABF0 95B2 1FCD 6D34 E57D