Best practice for WIP patches to help code review office hours

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

Best practice for WIP patches to help code review office hours

Jon Robson
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:Code-Review%253D0+OR+NOT+(label:Code-Review%253D-1+OR+label:Code-Review%253D-2)),n,z

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:Code-Review%253D0+OR+NOT+(label:Code-Review%253D-1+OR+label:Code-Review%253D-2)),n,z
is manageable that code gets merged left right and center :)

Warm regards,
Jon

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

James Forrester-4
On 12 May 2016 at 14:26, Jon Robson <[hidden email]> 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:Code-Review%253D0+OR+NOT+(label:Code-Review%253D-1+OR+label:Code-Review%253D-2)),n,z
>
> 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" 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:Code-Review%253D0+OR+NOT+(label:Code-Review%253D-1+OR+label:Code-Review%253D-2)),n,z
> is manageable that code gets merged left right and center :)
>

​Use
https://gerrit.wikimedia.org/r/#/q/(NOT+message:WIP)+AND+(NOT+message:DONTMERGE)+AND+(NOT+label:Verified%253D-1)+AND+(label:Code-Review%253D0+OR+NOT+(label:Code-Review%253D-1+OR+label:Code-Review%253D-2))+age:24w,n,z
 instead.

J.
--
James D. Forrester
Lead Product Manager, Editing
Wikimedia Foundation, Inc.

[hidden email] | @jdforrester
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

Jon Robson
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
<[hidden email]> wrote:

> On 12 May 2016 at 14:26, Jon Robson <[hidden email]> 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:Code-Review%253D0+OR+NOT+(label:Code-Review%253D-1+OR+label:Code-Review%253D-2)),n,z
>>
>> 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" 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:Code-Review%253D0+OR+NOT+(label:Code-Review%253D-1+OR+label:Code-Review%253D-2)),n,z
>> is manageable that code gets merged left right and center :)
>>
>
> Use
> https://gerrit.wikimedia.org/r/#/q/(NOT+message:WIP)+AND+(NOT+message:DONTMERGE)+AND+(NOT+label:Verified%253D-1)+AND+(label:Code-Review%253D0+OR+NOT+(label:Code-Review%253D-1+OR+label:Code-Review%253D-2))+age:24w,n,z
>  instead.
>
> J.
> --
> James D. Forrester
> Lead Product Manager, Editing
> Wikimedia Foundation, Inc.
>
> [hidden email] | @jdforrester
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



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

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

Alex Monk
In reply to this post by Jon Robson
On 12 May 2016 at 22:26, Jon Robson <[hidden email]> 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
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

Mukunda Modell
In reply to this post by Jon Robson
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 <[hidden email]> wrote:

> 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
> <[hidden email]> wrote:
> > On 12 May 2016 at 14:26, Jon Robson <[hidden email]> 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:Code-Review%253D0+OR+NOT+(label:Code-Review%253D-1+OR+label:Code-Review%253D-2)),n,z
> >>
> >> 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"
> 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:Code-Review%253D0+OR+NOT+(label:Code-Review%253D-1+OR+label:Code-Review%253D-2)),n,z
> >> is manageable that code gets merged left right and center :)
> >>
> >
> > Use
> >
> https://gerrit.wikimedia.org/r/#/q/(NOT+message:WIP)+AND+(NOT+message:DONTMERGE)+AND+(NOT+label:Verified%253D-1)+AND+(label:Code-Review%253D0+OR+NOT+(label:Code-Review%253D-1+OR+label:Code-Review%253D-2))+age:24w,n,z
> >  instead.
> >
> > J.
> > --
> > James D. Forrester
> > Lead Product Manager, Editing
> > Wikimedia Foundation, Inc.
> >
> > [hidden email] | @jdforrester
> > _______________________________________________
> > Wikitech-l mailing list
> > [hidden email]
> > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
>
>
> --
> Jon Robson
> * http://jonrobson.me.uk
> * https://www.facebook.com/jonrobson
> * @rakugojon
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

Mukunda Modell
In reply to this post by Alex Monk
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 <[hidden email]> wrote:

> On 12 May 2016 at 22:26, Jon Robson <[hidden email]> 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
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

Tim Landscheidt
In reply to this post by Jon Robson
Jon Robson <[hidden email]> 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:Code-Review%253D0+OR+NOT+(label:Code-Review%253D-1+OR+label:Code-Review%253D-2)),n,z

> 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/84068).

Tim


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

Stas Malyshev
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/84068).

This looks like a nice solution.

--
Stas Malyshev
[hidden email]

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

Jon Robson
.
On 12 May 2016 4:18 p.m., "Stas Malyshev" <[hidden email]> 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/84068
).
>
> 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
> [hidden email]
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

addshorewiki
Gerrit also has drafts...

On 13 May 2016 at 01:56, Jon Robson <[hidden email]> wrote:

> .
> On 12 May 2016 4:18 p.m., "Stas Malyshev" <[hidden email]> 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/84068
> ).
> >
> > 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
> > [hidden email]
> >
> > _______________________________________________
> > Wikitech-l mailing list
> > [hidden email]
> > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>



--
Addshore
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

Greg Grossmeier-2
In reply to this post by Jon Robson
<quote name="Jon Robson" date="2016-05-12" time="17:56:24 -0700">

> .
> On 12 May 2016 4:18 p.m., "Stas Malyshev" <[hidden email]> 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/84068
> ).
> >
> > 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" <[hidden email]> 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 |

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

Greg Grossmeier-2
In reply to this post by addshorewiki
<quote name="Addshore" date="2016-05-13" time="16:48:47 +0100">
> 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 |

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

Daniel Kinzler-2
Am 13.05.2016 um 18:23 schrieb Greg Grossmeier:
> <quote name="Addshore" date="2016-05-13" time="16:48:47 +0100">
>> 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.


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

Chad
On Wed, May 18, 2016 at 8:35 AM Daniel Kinzler <[hidden email]>
wrote:

> Am 13.05.2016 um 18:23 schrieb Greg Grossmeier:
> > <quote name="Addshore" date="2016-05-13" time="16:48:47 +0100">
> >> 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
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

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

John Mark Vandenberg
On Wed, May 18, 2016 at 10:41 PM, Chad <[hidden email]> wrote:

> On Wed, May 18, 2016 at 8:35 AM Daniel Kinzler <[hidden email]>
> wrote:
>
>> Am 13.05.2016 um 18:23 schrieb Greg Grossmeier:
>> > <quote name="Addshore" date="2016-05-13" time="16:48:47 +0100">
>> >> 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.

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

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l