Summary:
Fixes T9276. Fixes T8650. The story so far:
- We once published build updates to Revisions.
- An unrelated fix (D10911) sent them to the Diffs instead of Revisions, which isn't useful, since you can't see a diff's timeline anywhere.
- This also caused a race condition, where the RevisionEditor and DiffEditor would update the diff simultaneously (T8650).
- The diff update was just disabled to avoid the race (part of D13441).
- Instead, allow the updates to go somewhere else. In this case, we send commit updates to the commit but send diff updates to the revision so you can see 'em.
- Since everything will be using the revision editor now, we should either get proper lock behavior for free or it should be easy to add if something whack is still happening.
- Overall, this should pretty much put us back in working order like we were before D10911.
This behavior is undoubtedly refinable, but this should let us move forward.
Test Plan:
Saw a build failure in timeline:
{F2304575}
Reviewers: chad
Reviewed By: chad
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T9276, T8650
Differential Revision: https://secure.phabricator.com/D17139
Summary: Ref T11114. Move email/command actions, like "!reject", to modular transactions + editengine.
Test Plan: Used `bin/mail receive-test` to pipe "!stuff" to an object, saw appropraite effects in web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17133
Summary:
Ref T11114. When a user selects "Accept", and then selects "Reject", remove the "Accept". It does not make sense to both accept and reject a revision.
For now, every one of the "actions" conflicts: accept, reject, resign, claim, close, commandeer, etc, etc. I couldn't come up with any combinations that it seems like users are reasonably likely to want to try, and we haven't received combo-action requests in the past that I can recall.
Test Plan:
- Selected "Accept", then selected "Reject". One replaced the other.
- Selected "Accept", then selected "Change Subscribers". Both co-existed happily.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17132
Summary:
Fixes T9648. Diffs currently use `return $this->getRevision()->getViewPolicy();` to inherit their revision's view policy.
After the introduction of object policies, this is wrong for policies like "Subscribers", because it means "Subscribers to this object, the diff". Since Diffs have no subscribers, this always fails.
Instead, use extended policies so that the object policy evaluates in the context of the correct object (the revision).
Test Plan:
- Create a revision.
- Subscribe `alice` to it.
- Set view policy to "Subscribers".
- View revision as `alice`.
- Before patch: nonsense fatal about missing diff because of policy error.
- After patch: `alice` can see the revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9648
Differential Revision: https://secure.phabricator.com/D17123
Summary: Fixes T10312. If your first line is "Reviewers: xyz", it's a title, not a "Reviewers" field.
Test Plan: Added unit test.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10312
Differential Revision: https://secure.phabricator.com/D17122
Summary:
Fixes T8360. We will now parse revisions out of "Differential Revision: X" followed by other ad-hoc fields which we do not recognize. Previously, these fields would be treated as part of the value.
(In the general case, other fields may line wrap so we can't assume that fields are only one line long. However, we can make that assumption safely for this field.)
Also maybe fix whatever was going on in T9965 although that didn't really have a reproduction case.
Test Plan: Added unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8360
Differential Revision: https://secure.phabricator.com/D17121
Summary: Ref T11114. Fixes T10323.
Test Plan:
- Marked comments as done only: no warning about not leaving a comment.
- Did nothing: warning about posting an empty comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114, T10323
Differential Revision: https://secure.phabricator.com/D17120
Summary: Ref T11114. Although I plan to rewrite this system eventually (T10448) it's easy enough to punt for now.
Test Plan: punt
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17119
Summary:
Ref T11114. This restores:
- Commandeering should exeucte Herald.
- Commandeering should swap reviewers.
- "Request Review" on an "Accepted" revision should downgrade reviewers so they have to accept again.
Test Plan:
- Commandeered, saw Herald run and reviewers swap.
- Requested review of an accepted revision, saw it drop down to "Needs Review" with "Accepted Prior" on the reviewer.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17118
Summary: Ref T11114. This restores warnings (e.g., failing unit tests) and fixes "Quote" behavior for comments.
Test Plan:
- Quoted a comment.
- Viewed a warning.
{F2283275}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17117
Summary: Ref T11114. This comments nearly working on EditEngine. Only significant issue I caught is that the "View" link doesn't render properly because it depends on JS which is tricky to hook up. I'll clean that up in a future diff.
Test Plan: {F2279201}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17116
Summary:
Ref T11114. See D17114 for some discussion.
For review actions: accept, reject, resign.
For revision actions, order is basically least-severe to most-severe action pairs: plan changes, request review, close, reopen, abandon, reclaim, commandeer.
Test Plan: Viewed revisions as an author and a reviewer, saw sensible action order within action groups.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17115
Summary:
Ref T11114. Differential has more actions than it once did, and may have further actions in the future.
Make this dropdown a little easier to parse by grouping similar types of actions, like "Accept" and "Reject".
(The action order still needs to be tweaked a bit.)
Test Plan: {F2274526}
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17114
Summary:
Ref T11114. Some rough edges, but this largely makes Accept, Reject and Resign work in the new EditEngine comment area.
Ref T11050. This lays a little bit of groundwork for having "resign" mean "I don't want to review this, even if projects or packages I'm a member of need to", not just "remove me personally as a user reviewer".
Test Plan: Accepted, rejected and resigned from revisions without any major state issues.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114, T11050
Differential Revision: https://secure.phabricator.com/D17113
Summary:
Ref T11114. This has two pieces of side-effect logic which I've noted locally:
- Commandeer needs to apply Herald rules.
- Commandeer needs to move the old author to become a reviewer and remove
the actor as a reviewer.
Test Plan: Commandeered some revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17111
Summary:
Ref T11114. This restores these actions.
One behavior is incomplete: "Request Review" on an accepted revision does not downgrade reviewers properly. I've noted this locally.
Test Plan: Planned changes and requested review of a revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17109
Summary:
Ref T11114. This restores these actions as selectable in the comment area.
This does not implement one special rule ("Closing a revision in response to a commit is OK from any status.") but I have a note about that separately.
Test Plan: Closed and reopened revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17108
Summary: Ref T11114. This begins restoring comment actions to Differential, but on top of EditEngine.
Test Plan: {F2263148}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17107
Summary:
Ref T11114. This is a transitional change that breaks a bunch of stuff. I'll hold it until I've restored features.
This stuff works:
- Commenting.
- Subscribers/tags/reviewers.
- Pinning.
- Drafts.
This stuff does not work yet:
- Preview of inline comments.
- Probably submitting inlines, whatsoever.
- Comment-area warnings like "There are failing tests."
- All meaningful actions (accept, reject, etc).
Test Plan: Commented on a revision. Essentially nothing else works yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17106
Summary: See D17058.
Test Plan: Ran `arc diff`, which parsed fields as a side effect.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17112
Summary: Ref T11114. We seem to be in reasonable shape here and I don't think anything needs to revert, so rename this back to boring old "edit".
Test Plan: Created, updated, edited a revision via web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17091
Summary: Ref T11114. This is still mostly in use, but toss a few commit message parsing things.
Test Plan: Viewed/edited/upated blame rev from CLI/web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17089
Summary:
Ref T11114. Keep UI, throw everything else away.
Includes an imperfect-but-not-too-awful fix to keep the field actually working.
Test Plan: Edited tasks from CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17088
Summary: Ref T11114. Keep mail and UI, toss the rest.
Test Plan: Edited/viewed repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17087
Summary: Ref T11114. Keep rendering and mail, toss the rest.
Test Plan: Edited and viewed reviewers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17086
Summary: Ref T11114. Keep UI stuff and mail stuff, toss editing.
Test Plan: Viewed and edited revision summaries.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17085
Summary: Ref T11114. This leaves mail integration and UI integration, but strips all the editing (now handled by EditEngine) and commit message stuff (now handled by CommitMessageField).
Test Plan: Viewed and edited test plans and test plan transactions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17084
Summary: Ref T11114. Obsoleted by Modular Transactions + EditEngine + CommitMessageField + we just "hard code" the title of revisions into the page because we're craaazy.
Test Plan:
- Made an edit on `stable`.
- Viewed the edit on this change, it still had the proper UI strings.
- Edited/created/updated revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17083
Summary: Ref T11114. Obsoleted by EditEngine.
Test Plan: Edited the view policy of a revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17082
Summary: Ref T11114. This is obsoleted by `DifferentialSubscribersCommitMessageField` and EditEngine.
Test Plan: Edited a revision's subscribers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17081
Summary: Ref T11114. Obsoleted by `DifferentialRevisionIDCommitMessageField`.
Test Plan:
- Grepped for removed class.
- Created a new revision, verified that the amended message included a proper revision ID.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17080
Summary: Ref T11114. This is replaced by `DifferentialReviewedByCommitMessageField.php`.
Test Plan:
- Used `differential.getcommitmessage` to query an accepted revision, saw "Reviewed By".
- Grepped for removed class name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17079
Summary: Ref T11114. This is entirely obsoleted by EditEngine.
Test Plan: Edited projects on a revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17078
Summary: Ref T11114. This was obsoleted by UI changes and hacked around for performance in T11404. It no longer does anything.
Test Plan: Grepped for removed class name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17077
Summary: Ref T12027. This is purely a UI hint for new users that I'd like to integrate into "Land Revision" in the future instead.
Test Plan: Grepped for removed class, browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12027
Differential Revision: https://secure.phabricator.com/D17076
Summary: Ref T11114. This is obsolted by the narrower `DifferentialGitSVNIDCommitMessageField`.
Test Plan: Browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17075
Summary: Ref T11114. This is now entirely handled by EditEngine and standard policy code.
Test Plan: Edited the edit policy of a revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17074
Summary: Ref T11114. This is a pure paring field and now entirely handled by `DifferentialConflictsCommitMessageField`.
Test Plan: Grepped for removed class name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17073
Summary: Ref T11114. This was obsoleted by the "Stack" graph and does nothing.
Test Plan: Viewed revisions, still saw dependency graphs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17072
Summary: Ref T11114. This hasn't done anything since we moved author information to the subheader.
Test Plan: Browsed Differential, still saw author information.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17071
Summary: Ref T11114. This field just stores the value of "Auditors" so you can trigger auditors explicitly later on if you want.
Test Plan: Created and edited revisions with "Auditors".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17070
Summary: Ref T12026. This simplifies the UI and makes T11114 easier. I plan to integrate this into "Download Raw Diff" in the future.
Test Plan:
- Browsed revisions.
- Grepped for removed class name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12026
Differential Revision: https://secure.phabricator.com/D17069
Summary: Ref T11114. This makes the unusual stored custom fields ("Blame Rev", "Revert Plan", etc) work somewhat correctly (?) with EditEngine.
Test Plan:
- Created, updated and edited revisions with unusual stored custom fields like "Blame Rev".
- Observed that these fields now populate in "differential.revision.edit" when available.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17068
Summary:
Ref T11114. This creates `differential.revision.edit` (a modern, v3 API method) and redefines the existing methods in terms of it.
Both `differential.createrevision` and `differential.updaterevision` are now internally implemented by building a `differential.revision.edit` API call and then executing it.
I //think// this covers everything except custom fields, which need some tweaking to work with EditEngine. I'll clean that up in the next change.
Test Plan:
- Created, updated, and edited revisions via `arc`.
- Called APIs manually via test console.
- Stored custom fields ("Blame Rev", "Revert Plan") aren't exposed yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17067
Summary:
Ref T11114. This probably still has some bugs, but survives basic sanity checks.
Continue pulling commit message logic out of CustomField so we can reduce the amount of responsibility/bloat in the classtree and send more code through EditEngine.
Test Plan:
- Called `differential.getcommitmessage` via API console for various revisions/parameters (edit and create mode, with and without fields, with and without revisions).
- Used `--create`, `--edit` and `--update` modes of `arc diff` from the CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17066
Summary:
Fixes T11660. Currently, if you try to diff a path with more than 255 total characters, we fail to create the diff because we have a `text255` column.
There are actually two issues here:
- File names may be arbitrarily long (T11660).
- File names may not be UTF8 (T6633, etc). This is much more complicated and has other issues -- largely that we can't JSON-encode non-UTF8 filenames. I'm punting on that for now and will deal with it later. This doesn't specifically address non-UTF8 paths, although it is a change that's (probably?) required to eventually support them.
This will cause some potentially slow migrations, but better to do them now, if possible, so we have fewer complicated/slow upgrades overall.
Test Plan:
Created a change touching file: //very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_directory_name/very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_long_filename.txt//
{F2137737}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11660
Differential Revision: https://secure.phabricator.com/D17062
Summary: Ref T11114. Missed this while converting.
Test Plan: Tried to create a revision with no test plan. Before: fatal; after: helpful message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17061
Summary:
Ref T11114. See that task for some discussion.
Overall, Differential custom fields ended up with too many responsibilities. Later work in EditEngine provides a more promising model for achieving modularity with smaller, more consistent components.
In particular, we have some custom fields like `DifferentialGitSVNIDField` and `DifferentialConflictsField` which serve //only// to support the field parser.
This starts pulling commit message responsibilities out of the core list of custom fields and into simpler dedicated parsers.
Test Plan: Created and edited revisions from the CLI. Added a bit of test coverage.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17058
Summary:
Ref T11114. I want to move this step away from custom fields. To start with, isolate all the parsing in one class with a clearer API boundary.
Next, I'll make this class use new field objects to perform parsing, without CustomField interactions.
Test Plan: Created and edited revisions from the CLI, using valid and invalid commit messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17055
