Wine patch code review or what to do if patches get stuck?

[Austin English]

On Tue, Apr 21, 2009 at 2:44 AM, Igor Tarasov <tarasov.igor at gmail.com> wrote:
> Hi!
>
> The reason of why I write this to you is simple: my patches have stuck
> again and I don't know what to do. But since this happening not with
> me only and not this happens regularly, I have thought about looking
> at the problem a bit wider.
>
> Is there any code review system for patches that are sent to
> wine-patches? Or just some developers monitor wine-patches and say
> something if they have something to say? Or, how does Alexander make
> decisions regarding committing patches?

It's an open list, and many times, developers do comment. But if the
patch is not so obviously wrong, or no one knows that code area well,
it may not get a reply, even if correct.

> I've been thinking about some kind of web-based system, like one used
> at http://codereview.chromium.org/
>
> It would be nice, if there was sime kind of bot, that would grab all
> patches from wine-patches, sort them, make some initial analysis
> (maybe, run local test git repository, make various tests, check that
> threre are no conflicts, check code style (at least whitespace
> changes) etc). Then it could set a flag and report back to the sender.
> For instance it would flag "tests fail" and send details back to
> sender. Or "whitespace change detected" and send back the quoted diff
> parts.

We had this for a while. Dan Kegel wrote patchwatcher
(http://wiki.winehq.org/PatchWatcher) precisely for this. It's been
offline for a while since he's been remodeling his house. If you've
got a spare machine and a bit of perl knowledge, should only be a few
minutes to set up.

> Then, somebody of developers might review it and put his mark/comments
> that would go to wine-devel (if necessary), or to sender directly.

Patchwatcher does this.

> There could be simple templates for test reviews for common problems,
> and tags, such as
>
> "sure?" that means, that more explanations are needed why that
> implementation is correct.
> "too hackish" that means that style or code itself is of bad quality.
> "whitespace" whitespacechange detected.
> "tests required" tests are really required for this patch to go in.
> "okay", means that reviewer recommends the patch for commitment.
> "wrong", means that patch is totally wrong
> ..etc
>
> Some of these tags mean that report goes directly to developer, and
> patch is marked as obsolete, others make system send some feedback to
> wine-dev.
>
> It would take just few clicks for reviewer to get it done. This would
> provide good feedback, also this would help the one who makes the
> decisions to see that others have already checked that patch and what
> they think about that.
>
> Sometimes "no feedback" means that you just dont know - is there
> something wrong? Or maybe your patch wos not understood correctly? Or
> there are doubts that what you did is right? Or what? It would be
> better to have feedback like "are you serious?" than nothing at all :)
>
> P.S: the patches in question are
> http://www.winehq.org/pipermail/wine-patches/2009-April/072009.html
> and http://www.winehq.org/pipermail/wine-patches/2009-April/072013.html
> . They both are soo simple, that I even don't know how to ask of
> what's wrong with them :)
>
> --
> Igor
>
>
>



-- 
-Austin