moz-phab overwrites "accepted" status if reviewers are added via Phabricator
Categories
(Conduit :: moz-phab, defect, P2)
Tracking
(Not tracked)
People
(Reporter: mhentges, Unassigned)
Details
(Keywords: conduit-triaged)
(Note that this use case is not resolved by the 0.1.97 update)
Depending on the patch I'm making, sometimes I don't put any reviewers in my commit message, but rather I submit and add the reviewers on Phabricator. This is useful for two reasons:
- Herald will automatically add some reviewers that are necessary.
- I may not have the reviewer name memorized, so being able to search is helpful.
When moz-phab submits my patch, it sees that there are no reviewers on my revision, and it decides that this is a WIP patch. The downside of this is that it overwrites the state of the real Phabricator revision, which means:
- If the patch had reviewers attached and was accepted, then:
- It's moved back to "Changes Planned".
- A "WIP: " prefix is placed at the front.
I think that moz-phab should check the state of the revision on Phabricator. If it has has reviewers assigned and it's status isn't still "Changes Planned", then don't overwrite its status or title.
|
Reporter | |
Comment 1•4 years ago
|
||
Related: since there's a Changes Planned status, should we be putting WIP: on the front of the title?
This also impedes the "submit and set reviewers on Phabricator" workflow, because it's easy to miss that Phabricator edited your title for you.
|
|
Reporter | |
Updated•4 years ago
|
This was addressed in Bug 1700179.
|
|
Reporter | |
Comment 3•4 years ago
|
||
It's possible to capture this use case without requiring a --no-wip flag to be added, no?
Using the state of the revision on Phabricator allows us to make a more nuanced, workflow-supporting decision when submitting the patch.
I've been mulling this over and I think I was too quick to duplicate it; the use-case you outlined, particularly the automatic reviewer selection, is important. Sorry about that, reopening.
I'll chat with Zeid to figure out a consistent approach here, which might include just reverting the "no reviewers means WIP" change.
|
||
Updated•4 years ago
|
|
||
Comment 5•6 months ago
|
||
Should be fixed by bug 1887834. Specifically, this doesn't happen anymore.
When moz-phab submits my patch, it sees that there are no reviewers on my revision, and it decides that this is a WIP patch. The downside of this is that it overwrites the state of the real Phabricator revision, which means:
- If the patch had reviewers attached and was accepted, then:
- It's moved back to "Changes Planned".
- A "WIP: " prefix is placed at the front.
Description
•