Review promise :-)

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

Review promise :-)

Carlos O'Donell-6
Increasingly I'm actually turning to patchwork to look for patches to
review. I'm trying to structure my work day around an eventual attempt
to use Patchwork 2.0.

For example I was looking for patches from H.J. to review and I
cleaned up his list of owned patches in patchwork to track down what
still needed review and committing.

This was actually quite a nice way to work and is the kind of workflow
I wanted to have in place with gerritt but this works with email.

The hard part is that each developers queue is messy because we don't
keep them clean.

So here is my promise to *anyone*.

If you as a developer keep your patchwork queue clean.

If you ask me to look at your patches and you tell me your patchwork
queue is clean.

I will review at least one patch off that queue *immediately*.

Cheers,
Carlos.

Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Florian Weimer-5
* Carlos O'Donell:

> If you as a developer keep your patchwork queue clean.

I thought that we are going to throw away all that data soon?
Or is that not going to happen?

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Carlos O'Donell-6
On Thu, Feb 13, 2020 at 10:04 AM Florian Weimer <[hidden email]> wrote:
>
> * Carlos O'Donell:
>
> > If you as a developer keep your patchwork queue clean.
>
> I thought that we are going to throw away all that data soon?
> Or is that not going to happen?

Siddhesh,

Correct me if I'm wrong but you were able to migrate the data to Patchwork 2.0?

I thought you said you had the ability to migrate the data?

Florian,

Even if we throw the data out... you need to decide if this is worth
my review promise ;-)

This is incentive for me to do more reviews too!

Cheers,
Carlos.

Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Siddhesh Poyarekar-8
On 13/02/20 20:36, Carlos O'Donell wrote:
> Correct me if I'm wrong but you were able to migrate the data to Patchwork 2.0?
>
> I thought you said you had the ability to migrate the data?

In my last test I got all the way to 1.1 and got stuck there; this as on
the osci box that Sergio gave me access to.  The django migration
scripts broke at that point and I gave up to do other things.

I'll try to find some time to give it another shot this weekend.  I've
promised to look at the ChangeLog generation slowness too, so patchwork
goes below that in priority.

Siddhesh
Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Siddhesh Poyarekar-8
On 13/02/20 21:14, Siddhesh Poyarekar wrote:

> On 13/02/20 20:36, Carlos O'Donell wrote:
>> Correct me if I'm wrong but you were able to migrate the data to Patchwork 2.0?
>>
>> I thought you said you had the ability to migrate the data?
>
> In my last test I got all the way to 1.1 and got stuck there; this as on
> the osci box that Sergio gave me access to.  The django migration
> scripts broke at that point and I gave up to do other things.
>
> I'll try to find some time to give it another shot this weekend.  I've
> promised to look at the ChangeLog generation slowness too, so patchwork
> goes below that in priority.

I realized I didn't actually answer your question completely: yes the
intent is to migrate all data and at the moment it doesn't look
impossible.  The script breakage is not a showstopper IIRC, so we're
still on target to achieving a proper migration with data intact.

Siddhesh
Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Zack Weinberg-2
In reply to this post by Carlos O'Donell-6
On Thu, Feb 13, 2020 at 9:57 AM Carlos O'Donell <[hidden email]> wrote:

>
> Increasingly I'm actually turning to patchwork to look for patches to
> review. I'm trying to structure my work day around an eventual attempt
> to use Patchwork 2.0.
>
> For example I was looking for patches from H.J. to review and I
> cleaned up his list of owned patches in patchwork to track down what
> still needed review and committing.
>
> This was actually quite a nice way to work and is the kind of workflow
> I wanted to have in place with gerritt but this works with email.
>
> The hard part is that each developers queue is messy because we don't
> keep them clean.

Is there a way to operate on lots of patches at once?  My queue is
full of old patches that should be marked either committed or dropped,
it would be nice to not have to load the individual patch page for
each one and push the same buttons for each.

zw
Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Florian Weimer-5
* Zack Weinberg:

> On Thu, Feb 13, 2020 at 9:57 AM Carlos O'Donell <[hidden email]> wrote:
>>
>> Increasingly I'm actually turning to patchwork to look for patches to
>> review. I'm trying to structure my work day around an eventual attempt
>> to use Patchwork 2.0.
>>
>> For example I was looking for patches from H.J. to review and I
>> cleaned up his list of owned patches in patchwork to track down what
>> still needed review and committing.
>>
>> This was actually quite a nice way to work and is the kind of workflow
>> I wanted to have in place with gerritt but this works with email.
>>
>> The hard part is that each developers queue is messy because we don't
>> keep them clean.
>
> Is there a way to operate on lots of patches at once?  My queue is
> full of old patches that should be marked either committed or dropped,
> it would be nice to not have to load the individual patch page for
> each one and push the same buttons for each.

After logging in, I see checkboxes on the left here:

  <https://patchwork.sourceware.org/project/glibc/list/?submitter=73>

This enables bulk updates for me.

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Carlos O'Donell-6
In reply to this post by Zack Weinberg-2
On Thu, Feb 13, 2020 at 11:01 AM Zack Weinberg <[hidden email]> wrote:

>
> On Thu, Feb 13, 2020 at 9:57 AM Carlos O'Donell <[hidden email]> wrote:
> >
> > Increasingly I'm actually turning to patchwork to look for patches to
> > review. I'm trying to structure my work day around an eventual attempt
> > to use Patchwork 2.0.
> >
> > For example I was looking for patches from H.J. to review and I
> > cleaned up his list of owned patches in patchwork to track down what
> > still needed review and committing.
> >
> > This was actually quite a nice way to work and is the kind of workflow
> > I wanted to have in place with gerritt but this works with email.
> >
> > The hard part is that each developers queue is messy because we don't
> > keep them clean.
>
> Is there a way to operate on lots of patches at once?  My queue is
> full of old patches that should be marked either committed or dropped,
> it would be nice to not have to load the individual patch page for
> each one and push the same buttons for each.

Yeah, absolutely.

You have two options:
* Script it using the pwclient CLI
* Use the web interface, apply a filter, click the top "Patch"
checkmark to check all selected, then apply a property and it is
applied to all the selected patches.

Cheers,
Carlos.

Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Zack Weinberg-2
On Thu, Feb 13, 2020 at 11:19 AM Carlos O'Donell <[hidden email]> wrote:

> On Thu, Feb 13, 2020 at 11:01 AM Zack Weinberg <[hidden email]> wrote:
> >
> > Is there a way to operate on lots of patches at once?  My queue is
> > full of old patches that should be marked either committed or dropped,
> > it would be nice to not have to load the individual patch page for
> > each one and push the same buttons for each.
>
> Yeah, absolutely.
>
> You have two options:
> * Script it using the pwclient CLI
> * Use the web interface, apply a filter, click the top "Patch"
> checkmark to check all selected, then apply a property and it is
> applied to all the selected patches.

I see the checkboxes in the web interface, but I don't see any
controls for taking actions on the selected patches?  Yes, I'm logged
in.

zw
Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Florian Weimer-5
* Zack Weinberg:

> On Thu, Feb 13, 2020 at 11:19 AM Carlos O'Donell <[hidden email]> wrote:
>> On Thu, Feb 13, 2020 at 11:01 AM Zack Weinberg <[hidden email]> wrote:
>> >
>> > Is there a way to operate on lots of patches at once?  My queue is
>> > full of old patches that should be marked either committed or dropped,
>> > it would be nice to not have to load the individual patch page for
>> > each one and push the same buttons for each.
>>
>> Yeah, absolutely.
>>
>> You have two options:
>> * Script it using the pwclient CLI
>> * Use the web interface, apply a filter, click the top "Patch"
>> checkmark to check all selected, then apply a property and it is
>> applied to all the selected patches.
>
> I see the checkboxes in the web interface, but I don't see any
> controls for taking actions on the selected patches?  Yes, I'm logged
> in.

For me, there's a Properties box at the end of the page.

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Zack Weinberg-2
On Thu, Feb 13, 2020 at 12:58 PM Florian Weimer <[hidden email]> wrote:

> * Zack Weinberg:
> > On Thu, Feb 13, 2020 at 11:19 AM Carlos O'Donell <[hidden email]> wrote:
> >> On Thu, Feb 13, 2020 at 11:01 AM Zack Weinberg <[hidden email]> wrote:
> >> >
> >> > Is there a way to operate on lots of patches at once?  My queue is
> >> > full of old patches that should be marked either committed or dropped,
> >> > it would be nice to not have to load the individual patch page for
> >> > each one and push the same buttons for each.
> >>
> >> Yeah, absolutely.
> >>
> >> You have two options:
> >> * Script it using the pwclient CLI
> >> * Use the web interface, apply a filter, click the top "Patch"
> >> checkmark to check all selected, then apply a property and it is
> >> applied to all the selected patches.
> >
> > I see the checkboxes in the web interface, but I don't see any
> > controls for taking actions on the selected patches?  Yes, I'm logged
> > in.
>
> For me, there's a Properties box at the end of the page.

Weird.  I see a "Bundling" box, but no Properties box.

zw
Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Carlos O'Donell-6
On Thu, Feb 13, 2020 at 1:15 PM Zack Weinberg <[hidden email]> wrote:

>
> On Thu, Feb 13, 2020 at 1:12 PM Zack Weinberg <[hidden email]> wrote:
> > On Thu, Feb 13, 2020 at 12:58 PM Florian Weimer <[hidden email]> wrote:
> > > * Zack Weinberg:
> > > > On Thu, Feb 13, 2020 at 11:19 AM Carlos O'Donell <[hidden email]> wrote:
> > > >> On Thu, Feb 13, 2020 at 11:01 AM Zack Weinberg <[hidden email]> wrote:
> > > >> >
> > > >> > Is there a way to operate on lots of patches at once?  My queue is
> > > >> > full of old patches that should be marked either committed or dropped,
> > > >> > it would be nice to not have to load the individual patch page for
> > > >> > each one and push the same buttons for each.
> > > >>
> > > >> Yeah, absolutely.
> > > >>
> > > >> You have two options:
> > > >> * Script it using the pwclient CLI
> > > >> * Use the web interface, apply a filter, click the top "Patch"
> > > >> checkmark to check all selected, then apply a property and it is
> > > >> applied to all the selected patches.
> > > >
> > > > I see the checkboxes in the web interface, but I don't see any
> > > > controls for taking actions on the selected patches?  Yes, I'm logged
> > > > in.
> > >
> > > For me, there's a Properties box at the end of the page.
> >
> > Weird.  I see a "Bundling" box, but no Properties box.
>
> To avoid further round-trips, here is a screenshot of everything I can
> see on a search results page.  (My actual queue is three pages long,
> this is a very small subset.)

Fixed.

You need to be a listed project maintainer to be able to edit patch status.

I've updated MAINTAINERS to ensure future developers ask for this permission.

https://sourceware.org/glibc/wiki/MAINTAINERS

Cheers,
Carlos.

Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Zack Weinberg-2
On Thu, Feb 13, 2020 at 1:21 PM Carlos O'Donell <[hidden email]> wrote:
> > To avoid further round-trips, here is a screenshot of everything I can
> > see on a search results page.  (My actual queue is three pages long,
> > this is a very small subset.)
>
> Fixed.
>
> You need to be a listed project maintainer to be able to edit patch status.

Thanks, it works now.  (It's a little odd that _bulk_ edits require
this privilege but edits on individual patches don't, but that's
probably something to take up with patchwork upstream.)

zw
Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Joseph Myers
In reply to this post by Carlos O'Donell-6
On Thu, 13 Feb 2020, Carlos O'Donell wrote:

> The hard part is that each developers queue is messy because we don't
> keep them clean.

One benefit we were supposed to get from stopping manual ChangeLog edits
was that the git-patch-id of a commit for a patch committed with no
changes would match the git-patch-id of the patch posted to the mailing
list - and so patchwork would be able to mark such a patch as committed
automatically.

I'm not clear if this is a standard patchwork feature or just something we
hoped to be able to implement after stopping manual ChangeLog edits,
however.

(It might make sense to clean up existing patches in patchwork based on
having a git-patch-id matching a change that is now on master, as well as
doing such cleanup automatically in future.)

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Carlos O'Donell-6
On 2/13/20 3:54 PM, Joseph Myers wrote:

> On Thu, 13 Feb 2020, Carlos O'Donell wrote:
>
>> The hard part is that each developers queue is messy because we don't
>> keep them clean.
>
> One benefit we were supposed to get from stopping manual ChangeLog edits
> was that the git-patch-id of a commit for a patch committed with no
> changes would match the git-patch-id of the patch posted to the mailing
> list - and so patchwork would be able to mark such a patch as committed
> automatically.
>
> I'm not clear if this is a standard patchwork feature or just something we
> hoped to be able to implement after stopping manual ChangeLog edits,
> however.
>
> (It might make sense to clean up existing patches in patchwork based on
> having a git-patch-id matching a change that is now on master, as well as
> doing such cleanup automatically in future.)

I think that automating this is the goal with Patchwork 2.0.

Right now I'm walking developer patches queues from most recent patch
to oldest patch usually looking for something to review. I'm trying to
use patchwork in the way it was intended to see what the workflow would
look like.

--
Cheers,
Carlos.

Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Stefan Liebler-2
In reply to this post by Zack Weinberg-2
On 2/13/20 7:31 PM, Zack Weinberg wrote:

> On Thu, Feb 13, 2020 at 1:21 PM Carlos O'Donell <[hidden email]> wrote:
>>> To avoid further round-trips, here is a screenshot of everything I can
>>> see on a search results page.  (My actual queue is three pages long,
>>> this is a very small subset.)
>>
>> Fixed.
>>
>> You need to be a listed project maintainer to be able to edit patch status.
>
> Thanks, it works now.  (It's a little odd that _bulk_ edits require
> this privilege but edits on individual patches don't, but that's
> probably something to take up with patchwork upstream.)
>
> zw
>

Carlos,

Can you please help. I see the checkboxes, but it seems as I only have
the option to "Create bundle" at the bottom of the page or change each
single patch.

Its not clear to me where I have to be listed?
- https://sourceware.org/glibc/wiki/MAINTAINERS
(here I am listed)
- https://patchwork.sourceware.org/project/glibc/
(here I am not listed in the maintainers box. Is this the missing piece?
If yes, can you please add me.)

Another hint:
I had created my patchwork account with [hidden email] .
In the meantime I use [hidden email] .
I've just also linked [hidden email] to the patchwork account.

Bye,
Stefan

Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Carlos O'Donell-6
On 2/14/20 3:43 AM, Stefan Liebler wrote:

> On 2/13/20 7:31 PM, Zack Weinberg wrote:
>> On Thu, Feb 13, 2020 at 1:21 PM Carlos O'Donell <[hidden email]> wrote:
>>>> To avoid further round-trips, here is a screenshot of everything I can
>>>> see on a search results page.  (My actual queue is three pages long,
>>>> this is a very small subset.)
>>>
>>> Fixed.
>>>
>>> You need to be a listed project maintainer to be able to edit patch status.
>>
>> Thanks, it works now.  (It's a little odd that _bulk_ edits require
>> this privilege but edits on individual patches don't, but that's
>> probably something to take up with patchwork upstream.)
>>
>> zw
>>
>
> Carlos,
>
> Can you please help. I see the checkboxes, but it seems as I only have the option to "Create bundle" at the bottom of the page or change each single patch.
>
> Its not clear to me where I have to be listed?
> - https://sourceware.org/glibc/wiki/MAINTAINERS
> (here I am listed)
> - https://patchwork.sourceware.org/project/glibc/
> (here I am not listed in the maintainers box. Is this the missing piece? If yes, can you please add me.)
>
> Another hint:
> I had created my patchwork account with [hidden email] .
> In the meantime I use [hidden email] .
> I've just also linked [hidden email] to the patchwork account.

I just added you as a maintainer. Should be fixed now. Tell me if it's not.

--
Cheers,
Carlos.

Reply | Threaded
Open this post in threaded view
|

Re: Review promise :-)

Stefan Liebler-2
On 2/14/20 5:48 PM, Carlos O'Donell wrote:

> On 2/14/20 3:43 AM, Stefan Liebler wrote:
>> On 2/13/20 7:31 PM, Zack Weinberg wrote:
>>> On Thu, Feb 13, 2020 at 1:21 PM Carlos O'Donell <[hidden email]> wrote:
>>>>> To avoid further round-trips, here is a screenshot of everything I can
>>>>> see on a search results page.  (My actual queue is three pages long,
>>>>> this is a very small subset.)
>>>>
>>>> Fixed.
>>>>
>>>> You need to be a listed project maintainer to be able to edit patch status.
>>>
>>> Thanks, it works now.  (It's a little odd that _bulk_ edits require
>>> this privilege but edits on individual patches don't, but that's
>>> probably something to take up with patchwork upstream.)
>>>
>>> zw
>>>
>>
>> Carlos,
>>
>> Can you please help. I see the checkboxes, but it seems as I only have the option to "Create bundle" at the bottom of the page or change each single patch.
>>
>> Its not clear to me where I have to be listed?
>> - https://sourceware.org/glibc/wiki/MAINTAINERS
>> (here I am listed)
>> - https://patchwork.sourceware.org/project/glibc/
>> (here I am not listed in the maintainers box. Is this the missing piece? If yes, can you please add me.)
>>
>> Another hint:
>> I had created my patchwork account with [hidden email] .
>> In the meantime I use [hidden email] .
>> I've just also linked [hidden email] to the patchwork account.
>
> I just added you as a maintainer. Should be fixed now. Tell me if it's not.
>
Thanks. It now works for me.