1501077 - moz-phab does not update the reviewers list in Phabricator to match local changes

Status: NEW

(Conduit :: moz-phab, defect, P3)

Description

[Māris Fogels [:mars] (please needinfo)]

When a user updates the 'r?' reviewers flag in the commit title locally the reviewers list in Phabricator remains unchanged.  This could cause confusion as a patch author could make any number of local edits to the title after initial creation and then forget to go to the Phabricator UI and make the reviewers list match reality.

STR:

1. Create a commit and revision: "Bug 1 - test r?alice"
2. Run moz-phab submit
3. Amend the commit and change the reviewer: "Bug 1 - test r?bob"
4. Run moz-phab submit again

Expected:

The reviewer in Phabricator should be "bob".

Actual:

The reviewer in Phabricator is still "alice".

Comment 1

[Māris Fogels [:mars] (please needinfo)]

With respect to conflicting changes between the reviewers list via the Phabricator UI and changes to the reviewers list locally, glob says in MozReview we relied on the local changes as the source of truth.

I'm worried the MozReview policy applied to Phabricator could create tricky and subtle workflow footguns.  It is very opinionated about where and how review edits happen, and Phabricator itself doesn't share those opinions.  The policy would have to be implemented carefully.

For example, the local commit's reviewer list has absolute control over all reviewer lists in the system but those lists are still editable elsewhere, via Arc and via the web UI.  It means there will be accidental overwrites, accidental deletes, and accidental blocking/non-blocking bit-flips if either arc or the web UI are used.  It means users have to leave the web UI and work entirely within the source tree via checkout;amend;resubmit, even for quick changes.  And it means users have to actively think when using Phabricator, "hands off the web UI".

Comment 2

[Matthew N. [:MattN]]

This is one of my main annoyances with Phabricator/moz-phab as I often submit a WIP patch to a bug without reviewers and then later added reviewers when the patch is finished. This case has less issues related to conflicts as there are no reviewers on Phabricator… could we fix this more straightforward case in the short term and deal with the conflict cases later? i.e. if you submit a diff and there are no reviewers on Phabricator, add the reviewers from the commit message. (Sure, it doesn't handle the case where all reviewers were removed on Phabricator but I don't think that's common and I think it would be expected to add them back).

Comment 3

[:glob ✱]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #2)

This is one of my main annoyances with Phabricator/moz-phab as I often submit a WIP patch to a bug without reviewers and then later added reviewers when the patch is finished.

sounds reasonable i've split this out into bug 1531627.

Comment 4

[Piotr Zalewa [:zalun]]

Attachment: Bug 1501077 - WIP add reviewers on update

Comment 5

[Phabricator Automation]

Comment on attachment 9053589 [details]
Bug 1501077 - WIP add reviewers on update

Revision D24884 was moved to bug 1531627. Setting attachment 9053589 [details] to obsolete.

Comment 8

[:glob ✱]

This is still a problem but I think we're at a point where there's more clarity around how expectations.

Proposal:

1. remove the code that maps "no reviewers" to "WIP"
2. if any reviewers are set in the commit description, that becomes the authoritative source
   • the revision on Phabricator will be updated to match the commit description
   • moz-phab needs to make it very clear when a reviewer will be removed
3. if no reviewers are set then Phabricator becomes the authoritative source
   • reviewers will not be modified during the submission process

Potential problems:

• an approval from someone not originally set in the commit description will be removed if an update is pushed

Zeid - thoughts?

Comment 9

[Zeid [:zeid]]

@glob I agree with this proposal. Just to clarify, point (1) is referring to the new functionality introduced as part of bug 1610403? Does this affect the need for the --no-wip flag introduced in bug 1700179?

To address the potential problem, we could "merge" reviewers, so that both sources contain the union of the commit message and what's on Phabricator (i.e. synchronized upon pushing an update).

Comment 10

[:glob ✱]

(In reply to Zeid Zabaneh [:zeid] from comment #9)

point (1) is referring to the new functionality introduced as part of bug 1610403

Just the part that mapped "no reviewers" to WIP.

Does this affect the need for the --no-wip flag

Yes, the --no-wip flag would become redundant – either removed or updated to just display a warning that it is no longer required.

To address the potential problem, we could "merge" reviewers, so that both sources contain the union of the commit message and what's on Phabricator (i.e. synchronized upon pushing an update).

I'm always keen to avoid bi-directional sync. I've been involved in many projects over the years that tried bi-direction sync, and all have resulting in a long tail of problems and weirdness. Having a single source of truth simplifies logic and solidifies expectations.

Syncing reviewers would make some operations less obvious IMHO; for example to remove a reviewer you'd have to remove them from the revision via the Phabricator UI as well as your local commit prior to submission.