Summary:
Ref T11114. Ref T12085. I missed a few pieces of cleanup when moving all this stuff over.
In particular, load all fields which use Custom Field storage before doing commit-message-related stuff, instead of just the ones that claim they appear on commit messages.
Test Plan: Edited revisions and made API calls without apparent issues. See followup on T12085, shortly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12085, T11114
Differential Revision: https://secure.phabricator.com/D17207
Summary:
Fixes T12095. Ref T6660. The old code for this was specific to Differential, using the `DifferentialDraft` table.
Instead, make the `EditEngine` / `VersionedDraft` code create and remove a `<objectPHID, authorPHID>` edge when a particular author creates drafts.
Some applications have drafts beyond `VersionedDrafts`, notably inline comments. Before writing "yes, draft" or "no, no draft", ask the object if it has any custom draft stuff we need to know about.
This should fix all the yellow bubble bugs I created in T11114 and allow us to bring the feature to Audit fairly easily.
Test Plan: Created and deleted comments and inlines, reloading the list view after each change. Couldn't find a way to break the list view anymore.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12095, T6660
Differential Revision: https://secure.phabricator.com/D17205
Summary:
Ref T12098.
We have two methods (`supportsEditEngineConfiguration()` and `isEngineConfigurable()`) which sort of do the same thing and probably should be merged.
For now, just swap which one we override to get "Create Revision" out of the Quick Create menu.
Test Plan: No more "Create Revision" in Quick Create menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12098
Differential Revision: https://secure.phabricator.com/D17204
Summary:
Fixes T7076. This could probably use some tweaking but should get the basics in place.
This shows overall object state (e.g., "Needs Review"), not individual viewer state (e.g., "you need to review this"). After the bucketing changes it seems like we're mostly in a reasonable place on showing global state instead of viewer state. This makes the overall change much easier than it might otherwise have been.
Test Plan: {F2351867}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7076
Differential Revision: https://secure.phabricator.com/D17193
Summary: Ref T11114. Ref T10978. These hadn't made it over to EditEngine yet.
Test Plan:
- Took various actions on revisions and commits.
- Used `bin/mail show-outbound --id ...` to examine the "Vary Subject", saw it properly generate "[Accepted]", "[Resigned]", etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114, T10978
Differential Revision: https://secure.phabricator.com/D17191
Summary:
Fixes T12092. D17164 made `DiffQuery` more strict about arguments using modern conventions, but `differential.querydiffs` uses bizarre ancient conventions.
Give it more modern conventions instead.
Test Plan: Made a `querydiffs` call with only revision IDs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12092
Differential Revision: https://secure.phabricator.com/D17172
Summary: Fixes T12086. This got dropped by accident while cleaning up haunting.
Test Plan: Loaed a revision, hit "?", hit n/j/p/etc
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12086
Differential Revision: https://secure.phabricator.com/D17166
Summary:
Fixes T10968. In rare situations, we can generate a diff, then hit an error which causes this update to fail.
When it does, we tend to get stuck in a loop creating diffs, which can fill the database up with garbage. We saw this once in the Phacility cluster, and one instance hit it, too.
Instead: when we create a diff, keep track of which commit we generated it from. The next time through, reuse it if we already built it.
Test Plan:
- Used `bin/differential attach-commit <commit> <revision>` to hit this code.
- Simulated a filesystem write failure, saw the diff get reused.
- Also did a normal update, which worked properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10968
Differential Revision: https://secure.phabricator.com/D17164
Summary: Ref T11114. After evaluating typeahead tokens, we could process blocking reviewer removals incorrectly: we may get structures back.
Test Plan: Removed blocking reviewers from the web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17163
Summary:
Ref T12074. The "v3" API methods (`*.search`, `*.edit`) are currently marked as "unstable", but they're pretty stable and essentially all new code should be using them.
Although these methods are seeing some changes, almost all changes are additive (support for new constraints or attachemnts) and do not break backward compatibility. We have no major, compatibility-breaking changes planned.
I don't want to mark the older methods "deprecated" yet since `arc` still uses a lot of them and there are some capabilities not yet available on the v3 methods, but introduce a new "frozen" status with pointers to the new methods.
Overall, this should gently push users toward the newer methods.
Test Plan: {F2325323}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12074
Differential Revision: https://secure.phabricator.com/D17158
Summary:
Ref T11114. When you comment, we try to upgrade your review status to "commented".
This can conflict with upgrading it to "accepted" or "rejected", or removing it entirely.
For now, just avoid making this update. After T10967, I expect "you commented" to be orthogonal to accepted/rejected so it should stop conflicting on its own.
Test Plan:
- As an "added" reviewer, accepted a revision with a comment in the same transaction.
- Before patch: accept didn't stick.
- After patch: accept sticks.
This may be somewhat magical/order-dependent but I was able to reproduce it locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17146
Summary: Minor color saturation here, ideal for low quality monitors.
Test Plan:
Review new colors in various scenarios.
{F2305178}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17141
Summary:
Fixes T10136. This reinforces ongoing or failed builds in the comment action area.
We already emit a similar message for unit test failures from `arc unit`. This should probably obsolete that, eventually.
Test Plan:
{F2304809}
{F2304810}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10136
Differential Revision: https://secure.phabricator.com/D17140
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
Summary:
Ref T12020. Ref T11114. If we continue here on a mention, we try to generate `$old`, which requires reviewers to be attached. They won't be for simple codepaths like mentions.
Instead, just bail early: we don't need to do anything anyway since we can't possibly find any more errors with zero transactions.
Test Plan: Mentioned a revision on a task.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T11114, T12020
Differential Revision: https://secure.phabricator.com/D17059
Summary:
Ref T11114. This replaces the old edit controller with a new one based entirely on EditEngine.
This removes the CustomFieldEditEngineExtension hack for Differential, since remaining field types are fairly straightforward and work with existing EditEngine support, as far as I can tell.
Test Plan:
- Created a revision via web diffs.
- Updated a revision via web diffs.
- Edited a revision via web.
- Edited nonstandard custom fields ("Blame Revision", "JIRA Issues").
- Created a revision via CLI.
- Updated a revision via CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17054
Summary: Ref T11114. Much of this is around making the "comment-while-updating" flow work correctly.
Test Plan:
- Created new diffs by copy/pasting, then:
- used one to create a new revision;
- used one to update an existing revision, with a comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17053
Summary: Ref T11114. This one is a bit more complex, but I think I covered everything.
Test Plan:
- Added reviewers.
- Removed reviewers.
- Made reviewers blocking.
- Made reviewers nonblocking.
- Tried to make the author a reviewer.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17050
Summary: Ref T11114. The only real trick here is that we respect configuration in `differential.fields`.
Test Plan: Turned plan on and off, tried to remove the plan, edited the plan.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17048
Summary: Ref T11114. These are unambiguous and always-enabled.
Test Plan: {F2117777}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17047
Summary:
Ref T11114. Currently, all of Differential is extremely custom CustomFields. I want to back away from that somewhat and leverage more EditEngine / ModularTransactions infrastructure.
This allows EditEngine, ModularTransactions, and CustomFields to coexist in an uneasy peace. The "EditPro" controller applies a //different edit// than the CustomFields do, but everything works out in the end. I think.
Hopefully the horrible mess I am creating here will be short-lived.
Test Plan:
- Edited a revision with the normal editor.
- Edited a revision with the pro editor.
- Created a revision with `arc diff`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17044
Summary: Ref T11114. This doesn't really support anything yet, but technically works if you manually go to `/editpro/`.
Test Plan: {F2117302}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17043
Summary: Ref T10967. This makes room for a `DifferentialReviewer` object which can be a real storage table.
Test Plan: Grepped for `DifferentialReviewer`, browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17041
Summary: Ref T8475. This gets rid of most of the old "legacy hunk" code. I'll nuke the rest (and drop the old table) once we're more sure that we're in the clear.
Test Plan: Browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8475
Differential Revision: https://secure.phabricator.com/D17040
Summary:
Fixes T7899. If you create or update a revision and type an enormously long first line, we currently fatal trying to insert it into the database.
This text is only used to show a single-line summary of the diff in the "History" tab, which should probably be updated anyway. For now, stop fataling.
Test Plan:
Uploaded a diff with the description "MMMM..." (thousands of them).
Before patch: fatal on description being too long.
After patch: beautiful "MMMM" summary.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7899
Differential Revision: https://secure.phabricator.com/D17038
Summary: Spruce up the file embeds a little more, hover state, icons, file size.
Test Plan:
Add a psd and pdf, see new icons. Check differential, still see icons there too. Test mobile, desktop.
{F2042539}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16950
Summary:
Ref T11816. Since the dashboard got updated, Differential now interprets "Responsible Users: epriestley" to mean "epriestley, or any project or package epriestley is part of". You can query for just "epriestley" with "exact(epriestley)".
Give Calendar invites the same behavior: "epriestley" means "any event epriestley is invited to, or a project they are a member of is invited to". Individual invites can be queried with "exact(epriestley)".
This is a little bit copy-pastey but I want to wait for a third use case to clean it up since I think I'm going to have to do a bunch of generalization around "how does an individual PHID get turned into a bunch of PHIDs".
Test Plan: Queried for "Invited: dog", "invited: viewer", "invited; exact(dog)", etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16870
Summary: I moved and then un-moved this incorrectly in D16846.
Test Plan: Looked at the old code, which worked better.
Reviewers: jacksongabbard, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16849
Summary:
Ref T11085. To recreate the issue:
- From the web UI, click "Edit Revision".
- Write something like this as your "Summary" (i.e., put another field marker, like "Test Plan:", into the summary):
> This is a test of the
> Test Plan: field to see
> if it works.
- Save changes.
Later, when the summary is amended into a commit message, the parser will see two "Test Plan:" fields and fail to parse the message.
Instead, prevent users from making this edit.
Test Plan: {F1917640}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11085
Differential Revision: https://secure.phabricator.com/D16846
Summary:
This has been replaced by `PolicyCodex` after D16830. Also:
- Rebuild Celerity map to fix grumpy unit test.
- Fix one issue on the policy exception workflow to accommodate the new code.
Test Plan:
- `arc unit --everything`
- Viewed policy explanations.
- Viewed policy errors.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D16831
Summary:
Fixes T11836. See some prior discussion in T8376#120613.
The policy hint in headers in the UI is not exhaustive, and can not reasonably be exhaustive. For example, on a revision, it may say "All Users", but really mean "All users who can see the space this object is in and the repository it belongs to, plus the revision author and reviewers".
These rules are explained if you click (and, often, in the documentation), but "All Users" is still at least somewhat misleading.
I don't think there's any perfect solution here that balances the needs of both new and experienced users perfectly, but this change tries to do a bit better about avoiding cases where we say something very open (like "All Users") when the real policy is not very open.
Specifically, I've made these changes to the header:
- Spaces are now listed in the tag, so it will say `(S3 > All Users)` instead of `(All Users)`. They're already listed in the header, this just makes it more explicit that Spaces are a policy container and part of the view policy.
- Extended policy objects are now listed in the tag, so it will say `(S3 > rARC > All Users)` for a revision in the Arcanist repository which is also in Space 3.
- Objects can now provide a "Policy Codex", which is an object that represents a rulebook of more sophisticated policy descriptions. This codex can replace the tag with something else.
- Imported calendar events now say "Uses Import Policy" instead of, e.g., "All Users".
I've made these changes to the policy dialog:
- Split it into more visually separate sections.
- Added an explicit section for extended policies ("You must also have access to these other objects: ...").
- Broken the object policy rules into a "Special Rules" section (for rules like "you can only see a revision if you can see the repository it is part of") and an "Object Policy" section (for the actual object policy).
- Tried to make it a little more readable?
- The new policy dialogs are great to curl up with in front of a fire with a nice cup of cocoa.
I've made these changes to infrastructure:
- Implementing `PhabricatorPolicyInterface` no longer requires you to implement `describeAutomaticCapability()`.
- Instead, implement `PhabricatorPolicyCodexInterface` and return a `PhabricatorPolicyCodex` object.
- This "codex" is a policy rulebook which can set all the policy icons, labels, colors, rules, etc., to properly explain complex policies.
- Broadly, the old method was usually either not useful (most objects have no special rules) or not powerful enough (objects with special rules often need to do more in order to explain them).
Test Plan:
{F1912860}
{F1912861}
{F1912862}
{F1912863}
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T11836
Differential Revision: https://secure.phabricator.com/D16830
Summary:
Fixes T11748. This option currently implies a line limit (e.g., inline patches that are less than 100 lines long). This breaks down if a diff has a 10MB line, like a huge blob of JSON all on one line.
For now, imply a reasonable byte limit (256 bytes per line).
See T11767 for future work to make this and related options more cohesive.
Test Plan:
- With option at `1000`: sent Differential email, saw patches inlined.
- With option at `10`: sent Differential email, saw patches dropped because of the byte limit.
- `var_dump()`'d the actual limits and used `bin/worker execute --id ...` to sanity check that things were working properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11748
Differential Revision: https://secure.phabricator.com/D16714
Summary:
Ref T11650. Currently, we load packages and then discard the archived ones.
However, this gets "dominion" rules (where a more-general package gives up ownership if a more-specific package exists) wrong if the more-specific package is archived: we incorrectly give up ownership.
Instead, just ignore these packages completely when loading affected packages. This is slightly simpler.
(There are technically two pieces of code we have to do this for, which should be a single piece of code but which haven't yet been unified.)
Test Plan:
- Created packages:
- Package A, on "/" (strong dominion, autoreview).
- Package B, on "/x/" (weak dominion, autoreview).
- Package C, on "/x/y" (archived, autoreview).
- Create a revision affecting "/x/y".
- Saw correct path ownership in table of contents ("B", strongest package only).
- Saw correct autoreview behavior (A + B).
- (Prior to patch, in `master`, reproduced the problem behaviors described in T11650, with bad dominion rules and failure to autoreview B.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11650
Differential Revision: https://secure.phabricator.com/D16564
Summary:
Fixes T11610. Clean up some sketchy old code from long ago.
If you had rules that use conditions like "Accepted revision exists" and ran them in the test console, we'd never load the "CommitData" and fatal.
Instead, load CommitData in `newTestAdapter()` and generally make these pathways a little more modern.
Test Plan:
- Wrote an "Accepted Revision Exists" rule.
- Ran a commit in the test console.
- Before patch, got fatal from T11610.
- After patch, got clean test result.
- Also pushed a commit and reviewed the transcript to make sure the rule ran properly.
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T11610
Differential Revision: https://secure.phabricator.com/D16522
Summary:
This is hacky, and I'm not sure I'm happy with it; Until T9365 is done, this will show up
broken tests with an appropriate star in the Revision History.
Test Plan: Created 1M messages in a couple of old diffs in a revision. The query took ~80us (On SSD drive).
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16483
Summary:
Fixes T10423. Ref T11524. This changes `diffusion.rawdiffquery` to return a file PHID instead of a blob of data.
This is better in general, but particularly better for huge diffs (as in T10423) and diffs with non-utf8 data (as in T10423).
Test Plan:
- Used `bin/differential extract` to extract a latin1 diff, got a clean diff.
- Used `bin/repository reparse --herald` to rerun herald on a latin1 diff, got a clean result.
- Pushed latin1 diffs to test commit hooks.
- Triggered the the too large / too slow logic.
- Viewed latin1 diffs in Diffusion.
- Used "blame past this change" in Diffusion to hit the `before` logic.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T10423, T11524
Differential Revision: https://secure.phabricator.com/D16460
Summary: Ref T11132. This is a new default default (no dashboard) homepage. It offers (Diffs) (Tasks) (Repositories) in the main column and (Feed) in the side column. No NUX stuff, No logged out public view (upcoming diff). This should be complete, but unclear how to bucketize Differential.
Test Plan: Test new account's default homepage.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16449
Summary: This needs an `isset()` for cases when authority and packages don't completely overlap.
Test Plan:
- With a package set to trigger autoreview, created a revision.
- Observed error log, saw no more error.
- Saw package trigger autoreview properly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16398
Summary:
Fixes T9719. Currently, the Herald "Test Console" has a big `instanceof` thing, so new adapters (like a Calendar adapter, or third-party adapters) aren't available automatically. Instead, do a standard modular thing: load the available adapters, ask which ones can test the object the user selected, then let the user pick which one they want to move forward with.
Additionally, it isn't very clear that you can't test "commit hook" rules because they rely on push state which we don't really have a good way to simulate. When the user picks a commit, we now show them the "Hook" events, but the options are disabled and explain why they can not be selected.
Test Plan:
- Ran test rules for revisions, commits, mocks, tasks, wiki documents, questions, and outbound mail.
- Plugged in a commit, got a more-helpful choice screen explaining why you do a test run of hook rules.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9719
Differential Revision: https://secure.phabricator.com/D16360
Summary:
Ref T11404. Depends on D16351. Currently, both `differential.query` and `differential.revision.search` issue `2N` queries to fetch:
- dependencies for each revision; and
- projects for each revision.
Fix this:
- Take these custom fields out of Conduit so they don't load this data by default.
- For `differential.query`, put this data back in by hard coding it.
- For `differential.revision.search`, just leave it out. You can already optionally get projects efficiently, and this endpoint is a work in progress. I would tentatively be inclined to expose graph data as a "graph" extension once we need it.
This makes both methods execute in `O(1)` time (which is still 20-30 queries, but at least it's not 320 queries anymore).
Test Plan:
- Ran `differential.query`, observed no change in results but 199 fewer internal queries.
- Ran `differential.revision.search`, observed data gone from results and 200 fewer internal queries.
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11404
Differential Revision: https://secure.phabricator.com/D16352
Summary:
Ref T11404. Depends on D16350.
Currently, custom fields can issue "N+1" queries in some cases, so querying 100 revisions issues 100 extra queries.
This affects all `*.search` endpoints for objects with custom fields, and some older endpoints (notably `differential.query`).
This change bulk loads "normal" custom fields, which gets rid of some of these queries. Instead of loading fields for each object, we build a big list of all fields and load them all at once.
The next change will tackle the remaining inefficient edge queries.
Test Plan:
- Configured a custom field with normal database storage in Differential.
- Ran `differential.query`, looking at custom fields in results for correctness.
- Ran `differential.revision.search`, looking at custom fields in results for correctness.
- In both cases, observed queries drop from `3N` to `2N` (all the "normal" custom field stuff got bulk loaded).
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11404
Differential Revision: https://secure.phabricator.com/D16351
Summary: Ref T10909. Ref T9224. We label this field "Host" in the UI; make the storage format consistent.
Test Plan:
- Viewed month view, day view, detail view of an event.
- Created a new event, saw myself as the host.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9224, T10909
Differential Revision: https://secure.phabricator.com/D16291
Summary: Fixes T11305, Ref T7754. Makes this menu dropdown act like actions and collapse to a fa-bars menu.
Test Plan:
View on mobile, desktop, browser. Click an action, spawn new page.
{F1717953}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7754, T11305
Differential Revision: https://secure.phabricator.com/D16265
Summary:
Ref T4788. As it turns out, our tasks are very tightly connected.
Instead of loading every parent/child task, then every parent/child of those tasks, etc., etc., only load tasks in the "same direction" that we're already heading.
For example, we load children of children, but not parents of children. And we load parents of parents, but not children of parents.
Basically we only go "up" and "down" now, but not "out" as much. This should reduce the gigantic multiple-thousand-node graphs currently shown in the UI.
I still discover the whole graph for revisiosn, because I think it's probably more useful and always much smaller. That might need adjustment too, though.
Test Plan: Seems fine locally??
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16218
Summary:
Ref T4788. This separates the revision graph view into a base class with core logic and a revision class with Differential-specific logic, so I can subclass it in Maniphest, etc., and try using it in other applications to show similar graphs.
Not sure if we'll stick with it, but even if we don't this makes the code a bit cleaner and gets custom rendering logic out of the RevisionViewController, which is nice.
Test Plan: Viewed revisions, saw the stack UI completely unchanged.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16213
Summary: Ref T10628. Turn these into tabs in a single box, since "local commits" and "similar revisions" are of particularly rare use.
Test Plan: {F1707196}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10628
Differential Revision: https://secure.phabricator.com/D16209
