|
|
|
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
|
|
|
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
|
|
|
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_draftsFor reviewers, here is a helpful query:
https://gerrit.libreoffice.org/#/q/status:open+-label:Code-Review%253E1+-ownerin:CommittersIt 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
|
|
|
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
|
|
|
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
|
|
|
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
|
|
|
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
|
|
|
CONTENTS DELETED
The author has deleted this message.
|
|
|
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/CommonQueriesfor 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
|
|
|
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
|
|
|
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
|
|
|
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
|
|
|
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
|
|
|
CONTENTS DELETED
The author has deleted this message.
|
|
Locked
