Best practice for WIP patches to help code review office hours

List overview All Threads
Download

newer

older

Ownership of Selenium tests

I'd love to contribue. Can anyone...

Jon Robson

12 May 2016 12 May '16

9:26 p.m.

Gerrit is commonly used as a place to share works in progress early. This is great, but it has an unfortunate side effect of making it harder for would-be reviewers to find patches that need reviewing using this query: https://gerrit.wikimedia.org/r/#/q/(NOT+label:Verified%253D-1)+AND+(label:C… Could I ask that as a norm, if you post a WIP patch that you also self -2 it? In addition to this, if a patch is open for longer than several months you may want to abandon it - it's much more useful to link to an abandoned patchset in a phabricator task which has all the context for someone who might be able to solve the problem it tries to solve. I'd love to get us to a place where https://gerrit.wikimedia.org/r/#/q/(NOT+label:Verified%253D-1)+AND+(label:C… is manageable that code gets merged left right and center :) Warm regards, Jon

Show replies by date

James Forrester

12 May 12 May

9:29 p.m.

New subject: Best practice for WIP patches to help code review office hours

On 12 May 2016 at 14:26, Jon Robson <jdlrobson(a)gmail.com> wrote:

...

I disagree with this suggestion. The convention to date is having "WIP" or "DONTMERGE" in the git commit title. What's wrong with that?

...

I'd love to get us to a place where https://gerrit.wikimedia.org/r/#/q/(NOT+label:Verified%253D-1)+AND+(label:C… is manageable that code gets merged left right and center :)

Jon Robson

9:44 p.m.

New subject: Best practice for WIP patches to help code review office hours

I didn't know you could search message. Thanks. Even so.. the crux of what I'm asking for is a reliable way to filter these out. Even with the search you gave me I see "DONOTSUBMIT" in certain messages :) On Thu, May 12, 2016 at 2:29 PM, James Forrester <jforrester(a)wikimedia.org> wrote:

...

On 12 May 2016 at 14:26, Jon Robson <jdlrobson(a)gmail.com> wrote:

I disagree with this suggestion. The convention to date is having "WIP" or "DONTMERGE" in the git commit title. What's wrong with that?

I'd love to get us to a place where https://gerrit.wikimedia.org/r/#/q/(NOT+label:Verified%253D-1)+AND+(label:C… is manageable that code gets merged left right and center :)

Use https://gerrit.wikimedia.org/r/#/q/(NOT+message:WIP)+AND+(NOT+message:DONTM… instead. J. -- James D. Forrester Lead Product Manager, Editing Wikimedia Foundation, Inc. jforrester(a)wikimedia.org | @jdforrester _______________________________________________ Wikitech-l mailing list Wikitech-l(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

-- Jon Robson * http://jonrobson.me.uk * https://www.facebook.com/jonrobson * @rakugojon

Mukunda Modell

10:19 p.m.

New subject: Best practice for WIP patches to help code review office hours

We definitely need consistency for any convention like this to be useful. Phabricator has the equivalent of self-2 in Differential: `arc diff --plan-changes` It's a good convention, and I will try to adhere to the self-2 in addition to added WIP in the commit subject. On Thu, May 12, 2016 at 4:44 PM, Jon Robson <jdlrobson(a)gmail.com> wrote:

...

On 12 May 2016 at 14:26, Jon Robson <jdlrobson(a)gmail.com> wrote: > Gerrit is commonly used as a place to share works in progress early. > > This is great, but it has an unfortunate side effect of making it > harder for would-be reviewers to find patches that need reviewing > using this query: > >

https://gerrit.wikimedia.org/r/#/q/(NOT+label:Verified%253D-1)+AND+(label:C…

> > Could I ask that as a norm, if you post a WIP patch that you also self

-2

it?

I disagree with this suggestion. The convention to date is having "WIP"

"DONTMERGE" in the git commit title. What's wrong with that? > I'd love to get us to a place where > >

https://gerrit.wikimedia.org/r/#/q/(NOT+label:Verified%253D-1)+AND+(label:C…

is manageable that code gets merged left right and center :)

Use

https://gerrit.wikimedia.org/r/#/q/(NOT+message:WIP)+AND+(NOT+message:DONTM…

instead. J. -- James D. Forrester Lead Product Manager, Editing Wikimedia Foundation, Inc. jforrester(a)wikimedia.org | @jdforrester _______________________________________________ Wikitech-l mailing list Wikitech-l(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

-- Jon Robson * http://jonrobson.me.uk * https://www.facebook.com/jonrobson * @rakugojon _______________________________________________ Wikitech-l mailing list Wikitech-l(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Alex Monk

10:12 p.m.

New subject: Best practice for WIP patches to help code review office hours

On 12 May 2016 at 22:26, Jon Robson <jdlrobson(a)gmail.com> wrote:

...

Could I ask that as a norm, if you post a WIP patch that you also self -2 it?

I think you can only -2 if you have the rights necessary to +2?

Mukunda Modell

10:22 p.m.

New subject: Best practice for WIP patches to help code review office hours

Oh good point, you are correct, at least it would seem that way. I only have the option to -1 and +1 on operations/puppet, no -2 On Thu, May 12, 2016 at 5:12 PM, Alex Monk <krenair(a)gmail.com> wrote:

...

On 12 May 2016 at 22:26, Jon Robson <jdlrobson(a)gmail.com> wrote:

Could I ask that as a norm, if you post a WIP patch that you also self -2 it?

I think you can only -2 if you have the rights necessary to +2? _______________________________________________ Wikitech-l mailing list Wikitech-l(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Tim Landscheidt

10:37 p.m.

Jon Robson <jdlrobson(a)gmail.com> wrote:

...

Gerrit is commonly used as a place to share works in progress early.

...

This is great, but it has an unfortunate side effect of making it harder for would-be reviewers to find patches that need reviewing using this query: https://gerrit.wikimedia.org/r/#/q/(NOT+label:Verified%253D-1)+AND+(label:C…

...

Could I ask that as a norm, if you post a WIP patch that you also self -2 it?

...

[…]

No, -2 is restricted to project owners and thus not an op- tion for the vast majority of contributors. For that pur- pose, I proposed a Gerrit label "WIP" (cf. http://permalink.gmane.org/gmane.science.linguistics.wikipedia.technical/84…). Tim

Stas Malyshev

11:17 p.m.

New subject: Best practice for WIP patches to help code review office hours

Hi!

...

This looks like a nice solution. -- Stas Malyshev smalyshev(a)wikimedia.org

Jon Robson

13 May 13 May

12:56 a.m.

New subject: Best practice for WIP patches to help code review office hours

. On 12 May 2016 4:18 p.m., "Stas Malyshev" <smalyshev(a)wikimedia.org> wrote:

...

Hi! > No, -2 is restricted to project owners and thus not an op- > tion for the vast majority of contributors. For that pur- > pose, I proposed a Gerrit label "WIP" > (cf.

http://permalink.gmane.org/gmane.science.linguistics.wikipedia.technical/84… ).

...

This looks like a nice solution.

Seconded. What's stopping us from adopting? It seems in that thread nothing happened. Greg - is this something we can do?

...

-- Stas Malyshev smalyshev(a)wikimedia.org _______________________________________________ Wikitech-l mailing list Wikitech-l(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Addshore

3:48 p.m.

New subject: Best practice for WIP patches to help code review office hours

Gerrit also has drafts... On 13 May 2016 at 01:56, Jon Robson <jdlrobson(a)gmail.com> wrote:

...

. On 12 May 2016 4:18 p.m., "Stas Malyshev" <smalyshev(a)wikimedia.org> wrote:

Hi! > No, -2 is restricted to project owners and thus not an op- > tion for the vast majority of contributors. For that pur- > pose, I proposed a Gerrit label "WIP" > (cf.

http://permalink.gmane.org/gmane.science.linguistics.wikipedia.technical/84… ).

This looks like a nice solution.

Seconded. What's stopping us from adopting? It seems in that thread nothing happened. Greg - is this something we can do?

_______________________________________________ Wikitech-l mailing list Wikitech-l(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

-- Addshore

Greg Grossmeier

4:23 p.m.

New subject: Best practice for WIP patches to help code review office hours

...

Gerrit also has drafts...

Drafts are only visible to the author, unfortunately. But yes, that'd work, but it's also fairly drastic. And people might not know about it before they push ('git-review .') and then they can't (afaict) make their change into a draft (per https://phabricator.wikimedia.org/T63124 ). Upstream docs: https://gerrit-review.googlesource.com/Documentation/intro-user.html#drafts I like the WIP status label better, personally :) Greg -- | Greg Grossmeier GPG: B2FA 27B1 F7EB D327 6B8E | | identi.ca: @greg A18D 1138 8E47 FAC8 1C7D |

Daniel Kinzler

18 May 18 May

3:35 p.m.

New subject: Best practice for WIP patches to help code review office hours

Am 13.05.2016 um 18:23 schrieb Greg Grossmeier:

...

Gerrit also has drafts...

Drafts are only visible to the author, unfortunately.

And anyone the author adds as a reviewer. Works nicely in my experience.

Chad

3:41 p.m.

New subject: Best practice for WIP patches to help code review office hours

On Wed, May 18, 2016 at 8:35 AM Daniel Kinzler <daniel.kinzler(a)wikimedia.de> wrote:

...

Am 13.05.2016 um 18:23 schrieb Greg Grossmeier:

Gerrit also has drafts...

Drafts are only visible to the author, unfortunately.

And anyone the author adds as a reviewer. Works nicely in my experience.

Which I guess is nice if you really only want a specific group of people to see it, but it keeps the casual reviewer from chiming in. This is why they seemed useful for security patches at first, except that they're disclosable over `git fetch` if you know where to look. So yeah, I'm convinced they're defective by design as they're both too secret and not secret enough at the same time. -Chad

John Mark Vandenberg

19 May 19 May

1:22 a.m.

New subject: Best practice for WIP patches to help code review office hours

On Wed, May 18, 2016 at 10:41 PM, Chad <innocentkiller(a)gmail.com> wrote:

...

On Wed, May 18, 2016 at 8:35 AM Daniel Kinzler <daniel.kinzler(a)wikimedia.de> wrote:

Am 13.05.2016 um 18:23 schrieb Greg Grossmeier:

Gerrit also has drafts...

Drafts are only visible to the author, unfortunately.

And anyone the author adds as a reviewer. Works nicely in my experience.

For non-security patches, discovery isnt a problem. They are cookie-licked bugs, and should already been discoverable via a link on a Phabricator task. If there is no Phabricator task, they are an unloved patch for an undefined problem. Hiding them as a draft un-licks them. But that depends on https://phabricator.wikimedia.org/T63124 , and probably an upgrade of Gerrit, which will take forever because of the plan to switch to using Phabricator for code review. -- John Vandenberg

Greg Grossmeier

13 May 13 May

4:20 p.m.

New subject: Best practice for WIP patches to help code review office hours

...

. On 12 May 2016 4:18 p.m., "Stas Malyshev" <smalyshev(a)wikimedia.org> wrote:

Hi! > No, -2 is restricted to project owners and thus not an op- > tion for the vast majority of contributors. For that pur- > pose, I proposed a Gerrit label "WIP" > (cf.

http://permalink.gmane.org/gmane.science.linguistics.wikipedia.technical/84… ).

This looks like a nice solution.

Seconded. What's stopping us from adopting? It seems in that thread nothing happened. Greg - is this something we can do?

Certainly. "Adopting"? :) Making it official? I'd guess just setting the Gerrit config to exposing the WIP label and then documenting it on mw.org, maybe one of: * https://www.mediawiki.org/wiki/Gerrit/Advanced_usage * https://www.mediawiki.org/wiki/Gerrit/Code_review (only focused on the review part, not the submission part, should it expand in scope?) * https://www.mediawiki.org/wiki/Gerrit/Getting_started ? maybe too basic/focused to have this? * Maybe a new page describing the stages of a change and how to start contributing? I think of this because of this task:https://phabricator.wikimedia.org/T73357 - "Add a welcome bot to Gerrit for first time contributors" Task to track this for Gerrit I've created: https://phabricator.wikimedia.org/T135245 (Below is just some documentation on how to do it that I wrote last night...) How to do it via the command line? For Gerrit, use what was proposed in that last thread by Tim L: <quote name="Tim Landscheidt" date="2015-09-14" time="21:07:47 +0000">

...

"C. Scott Ananian" <cananian(a)wikimedia.org> wrote:

I'd use this tag more often if I could set it from the gerrit command-line when I upload a patch. Otherwise it will be pretty inconvenient to keep this in sync with the summary line of my patch.

That should be possible with Gerrit's command line inter- face (cf. https://gerrit-review.googlesource.com/Documentation/cmd-review.html). For example, I just voted on https://gerrit.wikimedia.org/r/#/c/238201/ with: | ssh -p 29418 gerrit.wikimedia.org gerrit review --label Code-Review=+1 4266a950bf7d0984cc5177b0f2f8d76b7d0b3c55 This /should/ work for arbitrary labels.

For those experimenting with Differential already it's even easier per Mukunda: <quote name="Mukunda Modell" date="2016-05-12" time="17:19:06 -0500">

...

We definitely need consistency for any convention like this to be useful. Phabricator has the equivalent of self-2 in Differential: `arc diff --plan-changes`

...

From arcanist's help information:

greg@x230 ~ % arc help diff <snip> --plan-changes Create or update a revision without requesting a code review. <snip> Which makes something like this: https://secure.phabricator.com/D15906 Then that change does not show up in searches for Diffs needing review. :) Best, Greg -- | Greg Grossmeier GPG: B2FA 27B1 F7EB D327 6B8E | | identi.ca: @greg A18D 1138 8E47 FAC8 1C7D |

2899

days inactive

2906

days old

wikitech-l@lists.wikimedia.org

Manage subscription

14 comments

11 participants

tags (0)

participants (11)

Addshore
Alex Monk
Chad
Daniel Kinzler
Greg Grossmeier
James Forrester
John Mark Vandenberg
Jon Robson
Mukunda Modell
Stas Malyshev
Tim Landscheidt