[Freeipa-devel] Reviewer in Trac

[Rob Crittenden]

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".
>>> 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.
>
>> 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.
>
> I guess I'll keep the patchwork instance up for now ...

I agree. I can't say I'm particularly thrilled about this new attribute 
either but I'm not exactly reviewing and submitting a ton of patches 
these days so I'll let let this play out.

To me this seems weak compared to a Gerrit-style review (not that I'm 
advocating Gerrit, which I hate for various other reasons).

rob