1513116 - Workflow: posting draft patches that are not ready for review yet

Status: RESOLVED FIXED

(Conduit :: moz-phab, enhancement, P2)

Description

[Botond Ballo [:botond]]

I sometimes want to post a work-in-progress patch series before it's ready for review (so that e.g. people can browse the patches in Phabricator's diff viewer).

However, I also like to record the reviewers I will want for my commits, as I write them. (Especially for a large patch series, having to go back at the end and add reviewers to all commits would be tedious.)

With MozReview, I could do this by passing the "--config reviewboard.deduce-reviewers=false" flag to the submission command.

I don't currently know how to do this with Phabricator. It would be nice if this workflow were supported.

Comment 1

[Steven MacLeod [:smacleod]]

(In reply to Botond Ballo [:botond] from comment #0)
> I don't currently know how to do this with Phabricator. It would be nice if
> this workflow were supported.

Phabricator has a specific workflow for this where you can tell it to keep your revision in "Draft" mode (which will prevent email from being sent, or having it appear in dashboards). You can make use of this with arc ("arc diff --draft") but I don't believe we have anything in mozphab for it right now.

That being said, we make use of draft mode, and have bugzilla move things out of draft mode, as part of our security workflow for Phabricator revisions. :dkl, did you evaluate how "arc diff --draft" will interact with the security stuff?

Comment 2

[David Lawrence [:dkl]]

Probably will do what you suspect and BMO will move it from Draft to Needs Review once it does its thing with the security policies. I will need to look at somehow differentiating between a normal revision (without --draft) and one where the author wants it to stay as draft and then leave it alone.

Comment 3

[Steven MacLeod [:smacleod]]

Instead of mucking around more with how we handle draft mode, we're going to add a feature to moz-phab for ignoring reviewers present in commit messages (similar to the feature from MozReview that :botond mentioned).

Comment 4

[:glob ✱]

for example: a --wip switch that doesn't set any reviewers on the revision and marks it as "changes planned".

Comment 5

[Piotr Zalewa [:zalun]]

--wip is already implemented - do you think we should remove reviewers recognition for this bug?

Comment 6

[:glob ✱]

(In reply to Piotr Zalewa [:zalun] from comment #5)

--wip is already implemented - do you think we should remove reviewers recognition for this bug?

yes

Comment 7

[:glob ✱]

Attachment: Bug 1513116 - Improve WIP by not adding reviewers to revisions; r=zalun!

Comment 8

[:glob ✱]

https://github.com/mozilla-conduit/review/commit/1feb537cdeece2e12b1ff0df2efa58d6ba1826fb

Comment 9

[Botond Ballo [:botond]]

Thank you for implementing this!

Comment 10

[Botond Ballo [:botond]]

Actually, I may not be clear on how to use this.

So when I initially want to post a patch series that's not ready for review yet, I run:

moz-phab submit --wip <first> <last>

This creates revisions that are marked as "plan changes", and are also in the "draft" state, and there are no reviewers assigned to them even though there were "r=<nick>" strings in the commit message. So far, so good.

Later, I update the patches and they're ready for review. I run:

moz-phab submit <first> <last>   # no --wip option

Now the revisions are no longer marked as "plan changes" (good), and they have reviewers assigned (good), but they're still in the draft state (not good, I think)? Am I supposed to be passing some other flag to get them out of the draft state?

Comment 11

[:glob ✱]

We don't use the draft state for its intended purpose (WIP patches); instead it's a transient state which is removed once Bugzilla has updated the revision to match the associated bug's security controls. If you can provide an example revision I'll investigate.

Comment 12

[Botond Ballo [:botond]]

(In reply to Byron Jones ‹:glob› 🎈 from comment #11)

If you can provide an example revision I'll investigate.

For example, D32774 is a patch I've submitted using moz-phab submit --wip hours ago. It's still in the draft state. Should it not be?

Comment 13

[:glob ✱]

Looks like you found a different bug - filed as bug 1555409.

Are you able to provide the revision where the transition from WIP to GTG didn't remove the draft state?

Comment 14

[Botond Ballo [:botond]]

(In reply to Byron Jones ‹:glob› 🎈 from comment #13)

Are you able to provide the revision where the transition from WIP to GTG didn't remove the draft state?

I just updated the same patch D32774 to not be WIP and it's still in the draft state.

Comment 15

[David Lawrence [:dkl]]

(In reply to Botond Ballo [:botond] from comment #14)

(In reply to Byron Jones ‹:glob› 🎈 from comment #13)

Are you able to provide the revision where the transition from WIP to GTG didn't remove the draft state?

I just updated the same patch D32774 to not be WIP and it's still in the draft state.

In the action menu, is there an request review choice? when a revision is ready for review, choosing that action should take it out of the draft state. BMO will not move a revision out of the draft state if it is set to "changes planned", etc.

Comment 16

[Botond Ballo [:botond]]

(In reply to David Lawrence [:dkl] from comment #15)

(In reply to Botond Ballo [:botond] from comment #14)

(In reply to Byron Jones ‹:glob› 🎈 from comment #13)

Are you able to provide the revision where the transition from WIP to GTG didn't remove the draft state?

I just updated the same patch D32774 to not be WIP and it's still in the draft state.

In the action menu, is there an request review choice? when a revision is ready for review, choosing that action should take it out of the draft state. BMO will not move a revision out of the draft state if it is set to "changes planned", etc.

Yes, there is. My hope, though, was that running moz-phab submit without the --wip option would accomplish this, without having to interact with the Phabricator UI.

I mean, suppose I have a 20-patch patch series that I previously posted as a WIP and now want to post for review. I certainly don't want to be performing a UI action for each of the 20 patches.

Comment 17

[:glob ✱]

let's move this discussion to bug 1555409