[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Yann E. MORIN]

Hello All!

In the Buildroot community, we make heavy use of patchwork to track the
patches that we still have pending. The maintainer (Peter, CCed) almost
exclusively use pwclient to get that list of patches to apply.

Since we also use the Linux-like tags Acked-by, Reviewed-by and
Tested-by, this is an indication to Peter that a patch is ready to be
applied.

Unfortunately, it is not easy to know at a glance what patches have such
tags, without checking the mailing list, so for each patch.

We would like to suggest that the web GUI and the pwclient CLI both
display such tags besides each patch, a bit like (hypotetical output of
pwclient):

    ID     Tags State       Subject
    123456 ART  New         Wonderfull patch to apply quickly

Where each of 'A', 'R', and 'T' would mean Acked-by, Reviewed-by and
Tested-by, respectively. An uppercase letter would mean two of more of
such tag, while a lower-case letter would mean a single such tag.

This way, it is easy to quickly sort high-profile patches that can be
easily handled by Peter without too much work (patches with tags), and
to the other contributors (some in Cc too) as a list of patches to look
at (patches without a tag).

Since patchwork already inserts those tags to the commit log (eg.
pwclient view PATCH-ID), I hope it would be easy enough to also display
it in the patch list.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Thomas Petazzoni]

Dear Jeremy Kerr,

On Tue, 12 Nov 2013 08:37:26 +0800, Jeremy Kerr wrote:

> > We would like to suggest that the web GUI and the pwclient CLI both
> > display such tags besides each patch, a bit like (hypotetical output of
> > pwclient):
> > 
> >     ID     Tags State       Subject
> >     123456 ART  New         Wonderfull patch to apply quickly
> 
> I've been looking to implement this for a while, but didn't have any
> good ideas about how to present this in the UI. I love your proposal
> here with the flags, it's simple and I think would work well for the
> majority of cases.
> 
> > Since patchwork already inserts those tags to the commit log (eg.
> > pwclient view PATCH-ID), I hope it would be easy enough to also display
> > it in the patch list.
> 
> It's easy enough to pull these out of the comment at patch-display time,
> but a little more tricky with list views (without some horrible SQL
> joining). I'll work out a nice way to to this; I think it'll involve
> applying these flags when the follow-up comment is parsed.
> 
> Other projects have different policies for their tagging (some require 3
> acks, for example), which will introduce a little complexity there too,
> but that's not insurmountable either.

Another thing that comes to mind is that not all Acked-by or Tested-by
are equal, I believe. An Acked-by from a well-known long-time
contributor will not be seen with the same level of trust that an
Acked-by from an unknown newcomer. Do we want to add a mechanism of
"rating"? But that would mean the project maintainer would have to
"rate" developers, which is not nice. Or maybe just a list of e-mails
that the project's maintainer considers as trustworthy, and only those
ones would be taken into account when displaying the ART flags?

Or maybe it doesn't matter too much, since in the vast majority of
cases, only well-known long-time contributors are doing reviews/acks.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Thomas De Schampheleire]

Hi,

On Tue, Nov 12, 2013 at 7:58 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
[..]
>> Other projects have different policies for their tagging (some require 3
>> acks, for example), which will introduce a little complexity there too,
>> but that's not insurmountable either.
>
> Another thing that comes to mind is that not all Acked-by or Tested-by
> are equal, I believe. An Acked-by from a well-known long-time
> contributor will not be seen with the same level of trust that an
> Acked-by from an unknown newcomer. Do we want to add a mechanism of
> "rating"? But that would mean the project maintainer would have to
> "rate" developers, which is not nice. Or maybe just a list of e-mails
> that the project's maintainer considers as trustworthy, and only those
> ones would be taken into account when displaying the ART flags?
>
> Or maybe it doesn't matter too much, since in the vast majority of
> cases, only well-known long-time contributors are doing reviews/acks.

Patchwork already has the concept of maintainers, that is a list of
people having patchwork change privileges. Maybe it is sufficient to
differentiate Acked/Reviewed/Tested from either maintainer or
non-maintainer? It would not require additional administration.

Best regards,
Thomas

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Arnout Vandecappelle]

On 12/11/13 07:58, Thomas Petazzoni wrote:
> Dear Jeremy Kerr,
>
> On Tue, 12 Nov 2013 08:37:26 +0800, Jeremy Kerr wrote:
>
>>> We would like to suggest that the web GUI and the pwclient CLI both
>>> display such tags besides each patch, a bit like (hypotetical output of
>>> pwclient):
>>>
>>>      ID     Tags State       Subject
>>>      123456 ART  New         Wonderfull patch to apply quickly
>>
>> I've been looking to implement this for a while, but didn't have any
>> good ideas about how to present this in the UI. I love your proposal
>> here with the flags, it's simple and I think would work well for the
>> majority of cases.
>>
>>> Since patchwork already inserts those tags to the commit log (eg.
>>> pwclient view PATCH-ID), I hope it would be easy enough to also display
>>> it in the patch list.
>>
>> It's easy enough to pull these out of the comment at patch-display time,
>> but a little more tricky with list views (without some horrible SQL
>> joining). I'll work out a nice way to to this; I think it'll involve
>> applying these flags when the follow-up comment is parsed.
>>
>> Other projects have different policies for their tagging (some require 3
>> acks, for example), which will introduce a little complexity there too,
>> but that's not insurmountable either.
>
> Another thing that comes to mind is that not all Acked-by or Tested-by
> are equal, I believe. An Acked-by from a well-known long-time
> contributor will not be seen with the same level of trust that an
> Acked-by from an unknown newcomer. Do we want to add a mechanism of
> "rating"? But that would mean the project maintainer would have to
> "rate" developers, which is not nice. Or maybe just a list of e-mails
> that the project's maintainer considers as trustworthy, and only those
> ones would be taken into account when displaying the ART flags?
>
> Or maybe it doesn't matter too much, since in the vast majority of
> cases, only well-known long-time contributors are doing reviews/acks.

  I think the ART flags in the patch list should never be used blindly 
anyway. Instead, it's an indicator of which patches are interesting to 
look at in more detail. Right?

  Regards,
  Arnout


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Yann E. MORIN]

Arnout, All,

On 2013-11-12 09:10 +0100, Arnout Vandecappelle spake thusly:
>  I think the ART flags in the patch list should never be used blindly
> anyway. Instead, it's an indicator of which patches are interesting to look
> at in more detail. Right?

That's the way I would use it, yes:
  - as a maintainer, I'd first look at patches with ART flags to apply
    them;
  - as a contributor, I'd first look at patches without ART flags to
    review them.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Yann E. MORIN]

Jeremy, All,

On 2013-11-12 08:37 +0800, Jeremy Kerr spake thusly:
> > We would like to suggest that the web GUI and the pwclient CLI both
> > display such tags besides each patch, a bit like (hypotetical output of
> > pwclient):
> > 
> >     ID     Tags State       Subject
> >     123456 ART  New         Wonderfull patch to apply quickly
> 
> I've been looking to implement this for a while, but didn't have any
> good ideas about how to present this in the UI. I love your proposal
> here with the flags, it's simple and I think would work well for the
> majority of cases.

Just checking: any progress on this? ;-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Yann E. MORIN]

Jeremy, All,

On 2013-11-11 22:54 +0100, Yann E. MORIN spake thusly:
[--SNIP--]
> We would like to suggest that the web GUI and the pwclient CLI both
> display such tags besides each patch, a bit like (hypotetical output of
> pwclient):
> 
>     ID     Tags State       Subject
>     123456 ART  New         Wonderfull patch to apply quickly
> 
> Where each of 'A', 'R', and 'T' would mean Acked-by, Reviewed-by and
> Tested-by, respectively. An uppercase letter would mean two of more of
> such tag, while a lower-case letter would mean a single such tag.

Any progress on this?

I've tried doing so myself, but my two attempts lamentably failed... :-(
Any one with Python- and SQL-fuu willing to have a look?

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Jeremy Kerr]

Hi all,

> We would like to suggest that the web GUI and the pwclient CLI both
> display such tags besides each patch, a bit like (hypotetical output of
> pwclient):

OK, I have some progress on this one; but wanted to make sure what I'm
doing is in the direction that'd be generally useful for you.

How's this look?

  http://ozlabs.org/~jk/tmp/patchwork-ART.png

- there are title attributes on the column header, so you get a tooltip
saying "Acked-by / Reviewed-by / Tested-by" when hovering over it. The
actual numbers have specific tooltips too (eg, "2 Acked-by").

Also, are you wanting to filter by the presence of these tags? Just
non-zero? How about sorting?

Anyone have objections to adding a small column to the default patch
lists? Does anyone want to *not* have this information present?

Cheers,


Jeremy

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Thomas Petazzoni]

Dear Jeremy Kerr,

On Wed, 07 May 2014 16:42:51 +0800, Jeremy Kerr wrote:

> OK, I have some progress on this one; but wanted to make sure what I'm
> doing is in the direction that'd be generally useful for you.
> 
> How's this look?
> 
>   http://ozlabs.org/~jk/tmp/patchwork-ART.png
> 
> - there are title attributes on the column header, so you get a tooltip
> saying "Acked-by / Reviewed-by / Tested-by" when hovering over it. The
> actual numbers have specific tooltips too (eg, "2 Acked-by").

This looks good! I actually made an implementation of this when I was
in the plane to the US a week ago, but I didn't had the necessary setup
to test it. Glad to see you've done it :)

> Also, are you wanting to filter by the presence of these tags? Just
> non-zero? How about sorting?

Sorting would be useful. I guess sorting for each tag is a bit too
much, so maybe sorting by the total of A+R+T is sufficient.

> Anyone have objections to adding a small column to the default patch
> lists? Does anyone want to *not* have this information present?

On Buildroot's side, we wanted this feature, so we're not really
interested in being able to disable it. But maybe some other projects
do not need it, indeed.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Yann E. MORIN]

Jeremy, All,

On 2014-05-07 16:42 +0800, Jeremy Kerr spake thusly:
> > We would like to suggest that the web GUI and the pwclient CLI both
> > display such tags besides each patch, a bit like (hypotetical output of
> > pwclient):
> 
> OK, I have some progress on this one; but wanted to make sure what I'm
> doing is in the direction that'd be generally useful for you.
> 
> How's this look?
> 
>   http://ozlabs.org/~jk/tmp/patchwork-ART.png

Yes, that's good!

I believe you will also add this in the pwclient output, right?

> - there are title attributes on the column header, so you get a tooltip
> saying "Acked-by / Reviewed-by / Tested-by" when hovering over it. The
> actual numbers have specific tooltips too (eg, "2 Acked-by").
> 
> Also, are you wanting to filter by the presence of these tags? Just
> non-zero? How about sorting?

Filtering would be a plus, sure. I'd like to have those filters:

  - has a non-zero A/R/T sum
    -> as a maintainer, I want to handle patches that have already been
       acked/reviewed/tested

  - has a zero A/R/T sum
    -> as a contributor, I want to review patches that have not already
       been acked/reviewed/tested

Maybe something like:

    pwclient list [-t|--tag N]

where N is the A/R/T sum you want to filter on.

> Anyone have objections to adding a small column to the default patch
> lists? Does anyone want to *not* have this information present?

In Buildroot and crosstool-NG, we do use A/R/T. Other projects (eg. VLC)
does not use those flags, so it should be possible to enable/disable
those flags, and disabled by default (to be backward compatible with
existing management scripts.)

In pwclient, maybe show the A/R/T flags only if filtering on them was
requested.

Thanks for putting some efforts in that! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Yann E. MORIN]

Jeremy, All,

On 2014-05-07 16:42 +0800, Jeremy Kerr spake thusly:
> > We would like to suggest that the web GUI and the pwclient CLI both
> > display such tags besides each patch, a bit like (hypotetical output of
> > pwclient):
> 
> OK, I have some progress on this one; but wanted to make sure what I'm
> doing is in the direction that'd be generally useful for you.
> 
> How's this look?
> 
>   http://ozlabs.org/~jk/tmp/patchwork-ART.png

Just a little reminder on this...

When are you planning on deploying that?
Will it also show up in the output of pwclient?

Thanks for your good work! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Jeremy Kerr]

Hi Yann,

>> How's this look?
>>
>>   http://ozlabs.org/~jk/tmp/patchwork-ART.png
> 
> Just a little reminder on this...
> 
> When are you planning on deploying that?
> Will it also show up in the output of pwclient?

I've just rolled out the initial code to patchwork.ozlabs.org. The tag
counts are all at zero to start with; I've triggered a re-parse of all
of the existing comments, so the counts should reflect reality when that
process finishes.

The filter UI and pwclient interface are still yet to be done though;
they're next on my todo list :)

Cheers,


Jeremy

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Jeremy Kerr]

Hi all,

> I've just rolled out the initial code to patchwork.ozlabs.org. The tag
> counts are all at zero to start with; I've triggered a re-parse of all
> of the existing comments, so the counts should reflect reality when that
> process finishes.

On second thoughts, I've disabled the display of the counts until the
update is complete, so we don't have false info in the UI in the
meantime. I'll re-enable once that's done.

Cheers,


Jeremy

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Yann E. MORIN]

Jeremy, All,

On 2014-06-03 22:15 +0800, Jeremy Kerr spake thusly:
> > I've just rolled out the initial code to patchwork.ozlabs.org. The tag
> > counts are all at zero to start with; I've triggered a re-parse of all
> > of the existing comments, so the counts should reflect reality when that
> > process finishes.
> 
> On second thoughts, I've disabled the display of the counts until the
> update is complete, so we don't have false info in the UI in the
> meantime. I'll re-enable once that's done.

For those not checking their web UI: the A/R/T status is now visible in
the web UI!

Thanks Jeremy!

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] Make Acked/Reviewd/Tested-by tags visible

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Jeremy, All,
 > On 2014-06-03 22:15 +0800, Jeremy Kerr spake thusly:
 >> > I've just rolled out the initial code to patchwork.ozlabs.org. The tag
 >> > counts are all at zero to start with; I've triggered a re-parse of all
 >> > of the existing comments, so the counts should reflect reality when that
 >> > process finishes.
 >> 
 >> On second thoughts, I've disabled the display of the counts until the
 >> update is complete, so we don't have false info in the UI in the
 >> meantime. I'll re-enable once that's done.

 > For those not checking their web UI: the A/R/T status is now visible in
 > the web UI!

Great, thanks all!

-- 
Bye, Peter Korsgaard