[Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

Jonathon Jongsma jjongsma at redhat.com
Fri Dec 5 13:38:27 PST 2014


On Fri, 2014-12-05 at 21:34 +0100, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Dec 5, 2014 at 8:59 PM, Jeremy White <jwhite at codeweavers.com> wrote:
> > To be clear, I am an advocate of mandatory review.  I, personally, would not
> > want to ever push a patch unless someone else has at least glanced at it.
> 
> Thanks, it was pretty clear already. Nevertheless, you didn't bring
> this before (did you?), so I think you took an opportunity here to
> express your opinion. There was already a lot of similar endless
> threads on our ML: there is no mandatory review of all commits in the
> Spice community, but Christophe, and now you, would like to apply it.
> I disagree for the reason I expressed before but if the majority of
> the contributors think it's better, I will follow with despite my
> disagreement. But I will be against such policy to be applied on the
> project I maintain for as long as I maintain them.
> 
> regards
> 

For what it's worth, I basically agree with Christophe and Jeremy.
(Although I think that describing it as "mandatory code review" is
over-stating the case a bit -- there is nothing but peer pressure and
polite requests preventing contributors from pushing unreviewed code).

At the same time, I'm not sure mailing lists are the right tool for code
review.  It's difficult to track which patches have been reviewed and
which haven't. Or what changes were made since the previous version of a
patch. For example, I often find myself wondering whether a particular
patch of mine was ACKed or not and I have to go trawl the list archives
to refresh my memory. And sometimes patches slip through the cracks and
sit unreviewed for many days. So I sort of wish there were a better way
to track patch status and evolution. On the other hand, introducing a
tool to improve this might make the process too cumbersome...

Jonathon



More information about the Spice-devel mailing list