[Freeipa-devel] Reviewer in Trac

Thu Feb 20 16:29:07 UTC 2014

On 02/20/2014 04:55 PM, Simo Sorce wrote:
> On Thu, 2014-02-20 at 16:34 +0100, Petr Viktorin wrote:
>> On 02/20/2014 04:15 PM, Simo Sorce wrote:
>>> On Thu, 2014-02-20 at 16:13 +0100, Martin Kosek wrote:
>>>> On 02/20/2014 04:09 PM, Simo Sorce wrote:
>>>>> On Thu, 2014-02-20 at 15:59 +0100, Martin Kosek wrote:
>>>>>> On 02/20/2014 03:52 PM, Jakub Hrozek wrote:
>>>>>>> On Thu, Feb 20, 2014 at 01:22:56PM +0100, Petr Viktorin wrote:
>>>>>>>> On 02/20/2014 01:14 PM, Martin Kosek wrote:
>>>>>>>>> We had a discussion with other developers how better track who is reviewing
>>>>>>>>> which patch. Recently, we introduced the Reviewed-By tag in a commit message,
>>>>>>>>> but that is a post-review tag which is not useful for someone who wants to know
>>>>>>>>> which patches are already reviewed and which are not reviewed.
>>>>>>>>>
>>>>>>>>> We were testing Patch Work [1] in last months to contain this information, but
>>>>>>>>> I personally think that it is suboptimal - it introduces 2 tracking tools that
>>>>>>>>> needs to be maintained (Trac and Patch Work) and the Patch Work still requires
>>>>>>>>> lot of manual actions when maintaining it's state.
>>>>>>>>>
>>>>>>>>> I think it would be better to hold this information rather in a single tracking
>>>>>>>>> tool - Trac. There are 2 options:
>>>>>>>>>
>>>>>>>>> 1) "Patch on review" flag, similar to "Patch posted for review" flag which
>>>>>>>>> would hold 1 bit information if the patch is just lying there or has somebody
>>>>>>>>> assigned.
>>>>>>>>>
>>>>>>>>> 2) "Reviewed by" text field which would hold a login of a person who is
>>>>>>>>> reviewing it. It would be filled either by a person starting the review or by a
>>>>>>>>> supervisor like me to forcefully assign a reviewer ;-)
>>>>>>>>>
>>>>>>>>> With that information in Trac, we could run using a single tracking tool for
>>>>>>>>> all patches that have a ticket (which is 95% of patches). It would be then
>>>>>>>>> fairly easy to see which patches are sent for review but are reviewer-less.
>>>>>>>>>
>>>>>>>>> It would also have a benefit for Petr's sendpatches.py script which could pull
>>>>>>>>> the reviewer from a ticket and one would not have to use the "-r" option to
>>>>>>>>> hard code a reviewer.
>>>>>>>>>
>>>>>>>>> Any objections to using "Reviewed by" field?
>>>>>>>>
>>>>>>>> +1, this is the only thing I used Patchwork for, and keeping
>>>>>>>> Patchwork up to date just for this took a lot of unnecessary
>>>>>>>> mindless clicking.
>>>>>>>>
>>>>>>>> Just a nitpick: name it "Patch Reviewer"
>>>>>>>> - there's more to a ticket than a patch
>>>>>>>> - the review is not done yet when the field is filled out
>>>>>>>
>>>>>>> The only use-case I use patchwork for right now is a 'dashboard' to see
>>>>>>> which patches need attention. If we could get this dashboard-like view
>>>>>>> from Trac with some custom query, then I'm fine with deprecating
>>>>>>> Patchwork.
>>>>>>
>>>>>> +1. I would like to add the reviewer to default report 3 + prepare a new view
>>>>>> "My Active Reviews by Milestone" to see the reviews which one should track.
>>>>>>
>>>>>>>
>>>>>>> However, one feature of patchwork was that each re-submission of a
>>>>>>> patch triggered a new thread so as a reviewer you could easily see there
>>>>>>> is a new instance of the patch that you need to look at. I suspect Trac
>>>>>>> wouldn't give us anything like that?
>>>>>>
>>>>>> When I get a review, I like to get the response to inbox - then I always know.
>>>>>> When replies are only being sent to the list, we would have to use the advanced
>>>>>> Trac workflow and cautiously change states between accepted - submitted - onreview.
>>>>>
>>>>> I think this means we'll be back to have to carefully track the mailing
>>>>> list because stuff will be missed. This is something patchwork "fixed".
>>
>> No. The only thing that happened automatically in Patchwork was that
>> entries got created. Patchwork doesn't even have threads
>
> This is not true, patchwork keeps together all answers to a patch (the
> review) until you send a new one.
>
>>   - each version
>> of a patch needed to be individually marked as superseded. Very much
>> mindless clicking is needed to keep Patchwork up to date.
>
> It is not perfect for sure, not claiming that.
>
>>>>> I wonder if we can build some automatism to not loose the good things
>>>>> here.
>>>>>
>>>>> Simo.
>>>>
>>>> Majority of patches going to freeipa-devel are tied to some Trac ticket. These
>>>> are tracked and watched by the on_review flag and the new reviewer field.
>>>
>>> If people remember to do it every time they just send an email, very
>>> process heavy.
>>
>> How is this different from Patchwork?
>
> You get a new thread with every submission, so you know there is
> something new autoatically by just looking at the list of patches, all

Yeah, one part is automatic. That's not enough.

Patchwork:
  patch arrives: nothing
  mark self as reviewer: use web interface
  send review: reply, find patch in Patchwork, mark status
  send fixed patch: send the mail, find patch in Patchwork, mark status, 
find old patch in Patchwork, mark old patch as superseded
  patch pushed/superseded: find patch in Patchwork, mark as pushed

(where "find patch in Patchwork" is frustrating when done so often)

Mail+Trac:
  patch arrives: tag message TODO when it comes in (1 keystroke)
  mark self as reviewer: use web interface (or API)
  send review: just reply ("changes requested" status is not kept though)
  send fixed patch: send the mail
  patch pushed: move from the push messsage to the tagged message (~1 
keystroke), untag message (1 keystroke)

> replys are also inline, so you know if a specific version of a patch has
> got replies (there are caveats I know).

That assumes I read the replies in patchwork.
Obviously Patchwork can't hope to be a better mail client than my 
favorite mail client.

>>>> Those that are not covered by any Trac ticket need to be tracked and cared of
>>>> manually by the submitter IMO.
>>>
>>> Not very friendly to external submitters.
>>
>> External submitters can easily open tickets.
>
> Very process heavy, we need to make things easier not more complicated.

They don't have to. But if they forget their patches *and* we manage let 
it slip, all that means is that nobody cares.
Anyway, with the amount of outside contributions we're getting, this is 
premature optimization.

>>> I guess I'll keep the patchwork instance up for now ...
>>
>> I was basically the only one who used the IPA Patchwork any more. I have
>> stopped using it and I'm waiting for the Patch Reviewer field (in any
>> form!).
>> Without someone to *manually* mark all the patches as On review, then
>> Changes Requested, then Pushed... Patchwork is quite useless.
>
> I will eventually retire it for freeipa, but I am not satisfied by using
> a field in trac.

Not a silver bullet, but better than Patchwork I'm afraid.

>> You can keep it running but no one from the FreeIPA team will use it. Sorry.
>
> This is fine, nobody is forced to. I still keep it on for the SSSD
> people which uses it so far.

Sounds good. Thanks for the experiment, it just didn't work out for us.

-- 
Petr³