running against last week's stable instance of Phabricator (and this week's, with no difference):
expect review message to be:
testing Summary: Test Plan: Reviewers: foo Subscribers:
actual:
testing Summary: Reviewers: foo Test Plan: Reviewers: Subscribers:
present in libphutil rPHU9c03af6 and arcanist rARC2374403
goes away after upgrading both to latest
based on the fact that 6 month old code worked fine until a backend upgrade last week, I'm guessing the breakage happened because of server-side changes
I also recognize that the latest versions of the entire stack do not exhibit the problematic behavior
I would hope that you guys would look into either:
Throwing a ticket on the backlog to implement a mechanism for detecting slightly or grossly incompatible versions of the stack when they try to talk to each other and throwing up a useful warning for the user would also be a positive move on this ticket.
making the old version of arc continue working as it did, if trivial, with new versions of the backend
I do not see a way to reasonably do this. The change which changed the behavior, D17122, fixes a different behavior (T10312). The two contexts are not meaningfully distinguishable from the server side so we can not retain the old behavior for the --reviewers phase and use the new behavior for other phases. In particular, a parse generated by --reviewers is indistinguishable on the server from a commit titled "Reviewers: ...", which previously would have parsed the first line as a "Reviewers" field (incorrect in the general case, per T10312) and now parses it as a title (correct, per T10312).
making the old version somehow signal that it's time for an upgrade
We can do this, but it is enormously disruptive and forces all users to upgrade, not just the tiny portion of users who use --reviewers. This is particularly problematic and disruptive for installs that fork arc.
In the past, we did this a few times but every time we got a ton of negative feedback that we shouldn't ever break arc like this and that forcing users to upgrade was miserable and disruptive. In response to this feedback, we haven't done this again since D4076 in 2012. The feedback was so consistently negative that we will probably never bump the version using this mechanism again in the future (and I expect to remove it in T11429).
Since then, we have generally maintained compatibility by:
Capability tests are probably the way forward here in general (and trying new methods and failing if they don't exist is a crude capability test) but we do not have a capability test available for this change. When I made the change, it also wasn't obvious that it was a compatibility break.
I believe this change introduces a very minor break to a very small number of users. It only affects users who use --reviewers or --cc, which I believe are used very rarely. It has an obvious effect (writing the field multiple times) and clear failure mode (an explicit error about duplicate fields) which users can remedy by editing the message, and there are easily identifiable workarounds (don't use --reviewers).
We could revert D17122 and D17147 and add separate parsing modes, but this would undo the fix in T10312 for users of old clients. I suspect more users may be affected by T10312 than use --reviewers, so this might be a net negative.
make it clear in the release notes that backwards incompatible changes happened to the API somewhere and that all clients are urged to upgrade their arc/libphutil.
Clients aren't urged to upgrade. They should only upgrade if they use --reviewers or --cc, and their environment has upgraded to 2017 Week 1 or newer, and they do not fork arc or Phabricator.
If you'd like to suggest some language which offers appropriate guidance while balancing these concerns, you should be able to edit 2017 Week 1 (Early January) to add it yourself.