Currently, arc has the logic for review message warnings, like "all reviewers are away".
We should move this to the server side and let fields raise warnings. We should also add additional warnings:
| Status | Assigned | Task | ||
|---|---|---|---|---|
| Resolved | epriestley | T5495 Configurable warning when not subscribing a mailing list | ||
| Open | None | T5786 Separate Differential reviewer warnings from the reviewers field | ||
| Open | None | T4631 Allow Differential to raise warnings on the server side via Conduit |
My understanding is that the root issue here is that you're adding reviewers via Herald/Owners (per T10939) so the "no reviewers" warning is inaccurate or misleading in many cases because owners will be added automatically after the revision is created.
Moving this warning to the server won't fix this, exactly: it would still evaluate too early to see reviewers added by Herald/Owners. However, it would let you disable the warning completely with configuration (in conjunction with T5786 / T6030) or by simply commenting it out.
We could potentially make the warning "correct" by introducing a draft state (T2543) and then making this a "publish" warning. So arc diff would create the revision, which would trigger Herald and add "shadow reviewers". arc would then check for pre-publish warnings and publish the revision if the user confirmed them. I don't have a clear pathway on T2543 yet and we should do T10967 before adding more metadata (like "shadow flags") to reviewers. This also gets a little tricky if the user chooses "no, don't publish" since we'll have created a Dxxx revision already and don't have a great way to throw it away.
If we did pursue this publish flow, we'd potentially have two separate prompt phases ("pre-create", "pre-publish"). We've also seen interest in prompt phases elsewhere, particularly prompts on "arc land" for, e.g., external build or deployment state that Phabricator can't necessarily know (see T10329). This motivates making prompting a generic workflow.
(Making prompting generic means we're sending console messages from the server. I think we don't currently do this, or don't do it very often. We probably can't transmit ANSI formatting characters in Conduit until T5955 and I'm not sure if we should trust the server to build ANSI strings because this represents some amount of attack surface. We don't need to deal with this now since it's relatively easy to extend later, but is perhaps something to try to anticipate.)
Even with this correction to "pre-publish", the flow still isn't ideal. Although we can't reasonably predict Herald reviewers before you write your message (since Herald can, for example, trigger on the message content itself), we can reasonably predict Owners reviewers and could give you this information upfront. This also runs into T10329 in general and T10622 in particular (defining a way to add reviewers before writing a commit message). A better flow would be for arc to have a "prefill reviewers" phase which prefills Owners reviewers and custom reviewers, then shows them to you. This would generally moot this prompt and tend to make it accurate. This is almost certainly the desired path in the long run.
More broadly, arc also needs a major update so we can deprecate old Conduit methods (see T10945). This will likely force upgrades and break compatibility, which motivates improving tools for managing arc versions and extensions (T5055) first. Somewhat beyond this there are a class of Windows-specific problems (T8298) which would benefit from additional clarity before pursuing major arc changes.
Minimally, this error is raised by differential.parsecommitmessage, which is relatively isolated and which we can extend without much real peril. If we did this and then later pursued publishing, we would currently move all of the prompts to publishing except "You don't own this revision, update it anyway?" (see T10584), which is messy. I'm not sure we'd keep this prompt in the current place in the long run, which might leave us with zero upstream warnings in this phase. It seems like being able to warn/prompt during commit validation is a useful capability in the general case -- and we will continue to raise errors in this phase which we could ship over a more general "action" mechanism -- but I can't immediately come up with custom fields which would benefit from prompting or recall any use cases for this.
Still, leaving this capability in place is probably reasonable. So a minimal reasonable implementation is probably:
Although there's probably little opportunity to make serious missteps in this design, it feels under-constrained to me in this use case. It would be nice to implement this in several places to be more certain that it does what it needs to.
"Pre-publish" prompts are hypothetical and buried deep behind T2543, and probably unrealistic.
However, there's a class of existing "pre-land" prompts which are relatively good candidates:
This is also potentially good as a future extension site for "inclement weather" warnings (e.g., bad production state).
To convert this site, we probably need to do this:
Alternatively, we could just extend differential.getcommitmessage, but its return format is currently a string so it's more difficult to extend without compatibility breaks. Current land workflow prompts/warnings also aren't really a property of the commit message.
Another possible testbed might be commands out of T5055. Although I'm not sure server side prompts make sense on the consumer install workflows, they might make sense on the publisher upload/sign workflows.
Other phutil_console_confirm() prompts which could be server side are:
Neither of these are particularly interesting.
Less minimally, the pathway forward I'd prefer to take here is more like:
Then pursue this and other changes on top of more flexible infrastructure. However, this is a very large amount of work and vastly disproportionate to the value of this feature ("users do not have to press 'Y'").
There is also some vague adjacency to arc cascade in T3875, although that doesn't really have much direct effect here.
At an extreme, we could simply remove this prompt. The value to experienced users is probably very small (they're already familiar with the workflow, and I think it is difficult to forget to add reviewers by mistake). The value to new users is probably not particularly great; the web UI also has a similar warning. The root issue of Owners or Herald making this prompt misleading is a general issue. And I definitely intend to move the prompt to the server eventually, so the client-side version of the warning will be removed at some point.
However, even if we just deleted the prompt you'd need to get everyone to arc upgrade to benefit from the change. It doesn't make sense for us to force a version bump over this, and there's currently no way for the client and server to exchange capabilities in a modular way. This also tends to motivate T5055 / T10329.
Upshot: I don't really have a pathway forward where the development cost is remotely proportional to the feature value. I'll just remove the prompt; you'll need to figure out how to get users to upgrade arc on your own until T5055.
FWIW, we have fully solved this problem in our organization and could not make use of any infrastructure provided by phabricator to this effect. T5055 would likely change nothing for us. This is especially true since there are tools that exist far outside the realm of phabricator that we need solve the same problem for.