Gerrit: mark patches as waiting for review

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

Gerrit: mark patches as waiting for review

Hi,

I just wondering whether gerrit can be improved some way to make it
allow to mark patches which are actually waiting for review. It's
common to upload patches only for Jenkins to check whether the patch
builds.
It would be good to have some option to filter for patches which need
review. It would make reviewing process faster.

Best Regards,
Tamás
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Kaganski Mike Kaganski Mike
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

Hi,

On 1/3/2017 1:50 AM, Zolnai Tamás wrote:
I just wondering whether gerrit can be improved some way to make it
allow to mark patches which are actually waiting for review. It's
common to upload patches only for Jenkins to check whether the patch
builds.
It would be good to have some option to filter for patches which need
review. It would make reviewing process faster.

I use +1 for that in my own patches; it indicates that others' review is awaited.

--
Best regards,
Mike Kaganski

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

Hi,

On Tue, Jan 03, 2017 at 07:23:23AM +0000, Kaganski Mike wrote:
> I use +1 for that in my own patches; it indicates that others' review is awaited.

Please dont do that:

A patch in gerrit by DEFAULT is asking for review. No further action needed.

A patch in gerrit that is marked by the author with +2 has passed review and
thus needs no further review (aka is only waiting on the 'verified' by CI).

So if you submit a patch to gerrit you by default already ask for review.
Please dont do fancy things with it, if that is what you want. If you _dont_
want your patch reviewed (which obviously only can happen if you are allowed to
push directly to master anyway), you can self-mark your change with Code-Review
+2.

If your patch isnt yet ready to be reviewed (and you just want to put it
somewhere as backup or to discuss it), please use drafts:

 https://wiki.documentfoundation.org/Development/gerrit/SubmitPatch#Submitting_patches_as_drafts

For reviewers, here is a helpful query:

 https://gerrit.libreoffice.org/#/q/status:open+-label:Code-Review%253E1+-ownerin:Committers

It lists all the patches that arent marked as Code-Review +2 and arent owned by
someone with commit access to master anyway. Those are the changes generally
needing review.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

In reply to this post by Tamás Zolnai
Hi,

On Mon, Jan 02, 2017 at 11:50:53PM +0100, Zolnai Tamás wrote:
> It would be good to have some option to filter for patches which need
> review. It would make reviewing process faster.

As discussed in the other mail:

 https://gerrit.libreoffice.org/#/q/status:open+-label:Code-Review%253E1+-ownerin:Committers

should show you changes waiting for review by people without direct commit access.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

Hi,

On Tue, Jan 03, 2017 at 11:04:59AM +0100, Bjoern Michaelsen wrote:
> As discussed in the other mail:
>
>  https://gerrit.libreoffice.org/#/q/status:open+-label:Code-Review%253E1+-ownerin:Committers
>
> should show you changes waiting for review by people without direct commit access.

Even better:

 https://gerrit.libreoffice.org/#/q/status:open+-label:Code-Review%253E1+-ownerin:Committers+label:Verified%253D1

only shows those that passed CI -- there is no use in reviewing changes that do not even pass CI[1]

Best,

Bjoern

[1] Apart from mentoring: if a patch did fail in CI and there is no update from
    the author for some time in response to that. But that is easily caught with a different query e.g.:
    https://gerrit.libreoffice.org/#/q/status:open+-ownerin:Committers+label:Verified%253D0+age:1week
    These guys likely need help.
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Kaganski Mike Kaganski Mike
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

In reply to this post by Bjoern Michaelsen
On 1/3/2017 1:03 PM, Bjoern Michaelsen wrote:
On Tue, Jan 03, 2017 at 07:23:23AM +0000, Kaganski Mike wrote:
I use +1 for that in my own patches; it indicates that others' review is awaited.
Please dont do that:

A patch in gerrit by DEFAULT is asking for review. No further action needed.

A patch in gerrit that is marked by the author with +2 has passed review and
thus needs no further review (aka is only waiting on the 'verified' by CI).

So if you submit a patch to gerrit you by default already ask for review.
Please dont do fancy things with it, if that is what you want. If you _dont_
want your patch reviewed (which obviously only can happen if you are allowed to
push directly to master anyway), you can self-mark your change with Code-Review
+2.

If your patch isnt yet ready to be reviewed (and you just want to put it
somewhere as backup or to discuss it), please use drafts:

Sometimes, a patch that was thought by me as a correct patch (ant dso was submitted for review), turns out to need some rework; and then it goes through numerous iterations until it gets ready. I don't expect initial (or other) reviewers to spend their time to review each iteration, and I don't know of a way to make already submittes change s draft (except resubmitting another change, which isn't always desirable); so when ready, I mark it "CR+1" - i.e. fine with me, but needs other's review, with accompanying comment that now I regard it ready and ask others to do actual review. As gerrit explicitly tells that it's what CR+1 means, I don't think it's a fancy thing.

--
Best regards,
Mike Kaganski

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Kaganski Mike Kaganski Mike
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

On 1/3/2017 1:17 PM, Kaganski Mike wrote:
Sometimes, a patch that was thought by me as a correct patch (ant dso was submitted for review), turns out to need some rework; and then it goes through numerous iterations until it gets ready. I don't expect initial (or other) reviewers to spend their time to review each iteration, and I don't know of a way to make already submittes change s draft (except resubmitting another change, which isn't always desirable); so when ready, I mark it "CR+1" - i.e. fine with me, but needs other's review, with accompanying comment that now I regard it ready and ask others to do actual review. As gerrit explicitly tells that it's what CR+1 means, I don't think it's a fancy thing.

But I suppose a better thing is to mark it -1 in the beginning, and revert to 0 when ready.

--
Best regards,
Mike Kaganski

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

In reply to this post by Kaganski Mike
Hi,

On Tue, Jan 03, 2017 at 10:17:56AM +0000, Kaganski Mike wrote:
> Sometimes, a patch that was thought by me as a correct patch (ant dso was
> submitted for review), turns out to need some rework; and then it goes
> through numerous iterations until it gets ready.

That should be avoided. Whatever you have on (public) gerrit should be ready
for review.

> I don't expect initial (or
> other) reviewers to spend their time to review each iteration, and I don't
> know of a way to make already submittes change s draft (except resubmitting
> another change, which isn't always desirable); so when ready, I mark it
> "CR+1"

As said: Please stop doing that, you are creating lots of extra work for
others. gerrit is for collaboration, if your code goes there it should be ready
for review. Stuff that isnt ready for review should not be there.

If all else fails, you should mark changes that you dont yet want to be
reviewed clearly marked in such a way in a form that is recognizable from the
gerrit list view, so that reviewer dont waste time on it. E.g. self-mark your
change as -2CR -- that will make most reviewers skip over your change[1].

tl,dr: If your patch is on gerrit, by default you ask for review. If you dont
       want that, its YOUR job to make sure the patch doesnt waste reviewers
       time (e.g. by a self-inflicted CR-2).

Best,

Bjoern

[1] Prefixing the commit message with "WIP" works too, but a self-inflicted
    -2CR is better as it can be queried and filtered for.
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

In reply to this post by Kaganski Mike
Hi,

On Tue, Jan 03, 2017 at 10:34:27AM +0000, Kaganski Mike wrote:
> But I suppose a better thing is to mark it -1 in the beginning, and revert to 0 when ready.

No. No, no, no.

Very explicitly: NO!

Dont do that. In fact, be prepared that if you push stuff that is unfit to be
merged to gerrit _others_ will mark it as CR-2 to save themselves from the
noise. They have every right to do so: The social contract of gerrit is: if you
push something there, you ask for a review.

Breaking that social contract is a very Bad Thing(tm).

Again: DONT DO THAT.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Khaled Hosny Khaled Hosny
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

In reply to this post by Bjoern Michaelsen
CONTENTS DELETED
The author has deleted this message.
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

Hi,

On Tue, Jan 03, 2017 at 12:53:03PM +0200, Khaled Hosny wrote:
> Can we have this somewhere more accessible, e.g. an “Unreviewed” link
> next to the “Open Merged Abandoned” links? May be without the
> “-ownerin:Committers” part since even committers want their patches
> reviewed sometime.

wikified as:

 https://wiki.documentfoundation.org/Development/gerrit/CommonQueries

for now, as I dont know (and dont want to know really) how to patch gerrit.

As for committers wanting review, IMHO in those cases it'd be good to
explicitly add someone as a reviewer (once you are a committer, you should have
an idea who might have strong opinions on your change).

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

Hi,

On Tue, Jan 03, 2017 at 12:16:29PM +0100, Bjoern Michaelsen wrote:
> for now, as I dont know (and dont want to know really) how to patch gerrit.

Turns out patching gerrit isnt really needed as at:

 https://gerrit.libreoffice.org/#/settings/preferences

you can set up as much custom queries as you like for your gerrit.

E.g. insert:

 want review
 #/q/status:open+-label:Code-Review%253D0+-ownerin:Committers

press "+" and save and you have that shortcut.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

Hi,

On Tue, Jan 03, 2017 at 12:21:14PM +0100, Bjoern Michaelsen wrote:
>  #/q/status:open+-label:Code-Review%253D0+-ownerin:Committers

eh almost, make that:

 #/q/status:open+-label:Code-Review=0+-ownerin:Committers

(aka do NOT escape the =, because then you end up double escaped and gerrit is
angry).

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

On Tue, Jan 03, 2017 at 12:25:10PM +0100, Bjoern Michaelsen wrote:
>  #/q/status:open+-label:Code-Review=0+-ownerin:Committers

#/q/status:open+label:Code-Review=0+-ownerin:Committers

dammit.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Tamás Zolnai Tamás Zolnai
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

In reply to this post by Bjoern Michaelsen
2017-01-03 12:21 GMT+01:00 Bjoern Michaelsen <[hidden email]>:

> Hi,
>
> On Tue, Jan 03, 2017 at 12:16:29PM +0100, Bjoern Michaelsen wrote:
>> for now, as I dont know (and dont want to know really) how to patch gerrit.
>
> Turns out patching gerrit isnt really needed as at:
>
>  https://gerrit.libreoffice.org/#/settings/preferences
>
> you can set up as much custom queries as you like for your gerrit.
>
> E.g. insert:
>
>  want review
>  #/q/status:open+-label:Code-Review%253D0+-ownerin:Committers
>
> press "+" and save and you have that shortcut.

Thanks Bjoern, it works well!
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Stahl-2 Michael Stahl-2
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

In reply to this post by Bjoern Michaelsen
On 01/03/2017 11:46 AM, Bjoern Michaelsen wrote:

> Hi,
>
> On Tue, Jan 03, 2017 at 10:34:27AM +0000, Kaganski Mike wrote:
>> But I suppose a better thing is to mark it -1 in the beginning, and revert to 0 when ready.
>
> No. No, no, no.
>
> Very explicitly: NO!
>
> Dont do that. In fact, be prepared that if you push stuff that is unfit to be
> merged to gerrit _others_ will mark it as CR-2 to save themselves from the
> noise. They have every right to do so: The social contract of gerrit is: if you
> push something there, you ask for a review.
>
> Breaking that social contract is a very Bad Thing(tm).
>
> Again: DONT DO THAT.

but what if i want to try if for example some experimental change builds
on a Mac, or Windows, whatever i don't have locally?

the jenkins doesn't build draft changes, only public ones.

that is a valid use case for submitting something with a -1 self-review.

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

Hi,

On Tue, Jan 03, 2017 at 01:05:59PM +0100, Michael Stahl wrote:
> that is a valid use case for submitting something with a -1 self-review.

Whops, yes. I overlooked that you changed to explicitly marking incomplete
changes to CR-1 (instead of having them at CR0). So sorry about the ranting --
yes, having WIP changes should be ok as long as they come with a self-inflicted
-1 (or -2).

As long as Code-Review=0 only contains stuff that really needs a review, its
fine.

Sorry about the noise.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Khaled Hosny Khaled Hosny
Reply | Threaded
Open this post in threaded view
|

Re: Gerrit: mark patches as waiting for review

In reply to this post by Bjoern Michaelsen
CONTENTS DELETED
The author has deleted this message.