Summary: Ref T13516. Generate a tree structure based on the page changesets. Still missing styles and a whole lot of behavior.
Test Plan: {F7373967}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21152
Summary: Ref T13516. This glues "FormationView" to "ChangesetList". The actual tree is not functional in any meaningful way yet.
Test Plan: {F7373838}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21151
Summary:
Ref T13515. Currently, opening a file to a particular line in an external editor relies on replacing "%l" with "%l" (which is escaped as "%25l") on the server, and then replacing "%25l" with the line number on the client. This will fail if the file path (or any other variable) contains "%l" in its unencoded form.
The parser also can't identify invalid variables.
Pull the parser out, formalize it, and make it generate an intermediate representation which can be sent to the client and reconstituted.
(This temporarily breaks Diffusion and permanently removes the weird, ancient integration in Dark Console.)
Test Plan:
- Added a bunch of tests for the actual parser.
- Used "Open in Editor" in Differential.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21143
Summary:
Ref T13455. Update the other "view state" properties to work like "highlight" now works.
Some complexity here arises from these concerns:
- In "View Standalone", we render the changeset inline. This is useful for debugging/development, and desirable to retain.
- In all other cases, we render the changeset with AJAX.
So the client needs to be able to learn about the "state" properties of the changeset on two different flows. Prior to this change, each pathway had a fair amount of unique code.
Then, some bookkeeping issues:
- At inital rendering time, we may not know which renderer will be selected: it may be based on the client viewport dimensions.
- Prior to this change, the client didn't separate "value of the property for the changeset as rendered" and "desired value of the property".
Test Plan:
- Viewed changes in Differential, Diffusion, and in standalone mode.
- Toggled renderer, character sets, and document engine (this one isn't terribly useful). Reloaded, saw them stick.
- Started typing a comment, cancelled it, hit the undo UI.
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21138
Summary:
Ref T13455. Add container-level storage for persistent view state, and persist "Highlight As..." inside it.
The storage generates a "PhabricatorChangesetViewState" configuration object as an output.
When preferences are expressed on a diff and that diff is later attached to a revision, we attempt to copy the preferences.
The internal storage tracks per-changeset settings, but currently always uses "last update wins" to apply the settings in the UI.
Test Plan:
- Viewed revisions, changed highlighting, reloaded. Saw highlighting stick in revision view and standalone view.
- Viewed commits, changed highlighting, reloaded. Saw highlighting stick.
- Created a diff, changed highlighting, turned it into a revision, saw highlighting persist.
Subscribers: jmeador, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21137
Summary:
Ref T13515. We "shield" some changesets, including generated code and intradiffs with no intermediate changes.
These files don't get shielded if they have inline comments.
But, if the viewer has collapsed all the comments, we can shield the file again.
Test Plan:
- Created a change affecting files A and B, with three diffs:
- Touch A and B.
- Touch B only.
- Touch nothing.
- Added an inline to A and collapsed it.
- Viewed Diff 1 vs Diff 2:
- Saw A collapse with a note about inlines.
- Saw B changes, normally.
- Viewed Diff 2 vs Diff 3:
- Saw A collapse with a note about inlines.
- Saw B collapse normally.
- Uncollapsed the inline, viewed 1v2 and 2v3, saw A expand in both cases.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21136
Summary: Ref T13515. See PHI1661. If a file is selected, add a keystroke to click the "Open in External Editor" link.
Test Plan: In Safari, Chrome, and Firefox: used "J" to select a file, then "\" to open it in an external editor. (In Safari and Chrome, this prompts.)
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21135
Summary:
Ref T13511. Ferret functions currently define "aliases", and some applications override the default aliases.
This probably isn't really the right model, since it means the available function aliases in global search depend on the types of documents you're searching for. This isn't fundamentally unworkable but is kind of weird.
Regardless, these don't actually work. Searching for "description:x" is a syntax error.
Since they don't work, it's a good bet no one is relying on them. Just get rid of them until there's a clearer argument for the feature.
Test Plan: Grepped for "getFunctionMap", got no other hits. Ran some queries with the alias functions, got syntax errors.
Maniphest Tasks: T13511
Differential Revision: https://secure.phabricator.com/D21130
Summary: Ref T13490. This simplifies some client behavior in the general case.
Test Plan: Called API method, saw URIs.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21105
Summary:
See PHI1697. If a diff is not attached to a revision (for example, if it was created with "arc diff --only"), but is attached to a repository, it is supposed to be visible only to users who can see that repository.
It currently skips this extended policy check and may incorrectly be visible to too many users.
(Once a diff is attached to a revision, this rule is enforced properly via the revision policy.)
Test Plan:
- Set repository R to be visible only to Alice.
- As Alice, created a diff from a working copy of repository R with "arc diff --only".
- As Bailey, viewed the diff.
- Before: visible diff.
- After: policy exception (as expected).
Differential Revision: https://secure.phabricator.com/D21103
Summary: Ref T13490. This simplifies mostly-theoretical cases where you're accessing Phabricator via arc-over-ssh and the Conduit protocol + domain may differ from the production protocol + domain.
Test Plan: Called API via web UI, saw sensible URI values in results.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21102
Summary: See PHI1684. Expose the published state of the "Done" checkbox to the API.
Test Plan: Made API calls on a comment in all four states, got correct published states via the API in all cases.
Differential Revision: https://secure.phabricator.com/D21059
Summary: Fixes T2543. This mode has been a stable prototype for a very long time now; promote it so "--draft" can promote out of "experimental" in Arcanist.
Test Plan: See T2543 for discussion.
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D20983
Summary: Ref T13395. Move cache classes, syntax highlighters, other markup classes, and sprite sheets to Phabricator.
Test Plan: Attempted to find any callers for any of this stuff in libphutil or Arcanist and couldn't.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20977
Summary:
Fixes T13468. See that task for discussion. The older source-rendering code mixes "line number" / "1-based" lists with "block number" / "0-based" lists and then has other bugs which cancel this out.
For block-based diffs, build an explicit block-based mask with only block numbers. This sort of sidesteps the whole issue.
Test Plan: Viewed the diff with the original reproduction case, plus various other block-based diffs, including one-block image diffs, in unified and side-by-side mode. Didn't spot any oddities.
Maniphest Tasks: T13468
Differential Revision: https://secure.phabricator.com/D20959
Summary: Ref T13480. Add an "Author's packages" field to Herald to support writing rules like "if affected packages include X, and author's packages do not include X, raise the alarm".
Test Plan: Wrote and executed rules with the field, saw a sensible field value in the transcript.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20947
Summary:
Fixes T13443. When a panel raises an exception during edit action generation, it currently escapes to top level. Instead, catch it more narrowly.
Additionally, mark "ChangesetSearchEngine" as not being a suitable search engine for use in query panels. There's no list view or search URI so it can't generate a sensible panel.
Test Plan:
- Added a changeset panel to a dashboard.
- Before: entire dashboard fataled.
- After: panel fataled narrowly, menu fatals narrowly, dashboard no longer permits creation of another Changeset query panel.
Maniphest Tasks: T13443
Differential Revision: https://secure.phabricator.com/D20902
Summary: Ref T13425. When a change adds or removes a block (vs adding or removing a document, or changing a block), we currently try to render `null` as cell content in the unified view. Make this check broader to catch both "no opposite document" and "no opposite cell".
Test Plan: {F7008772}
Subscribers: artms
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20897
Summary: Ref T13440. This feature is used in only one interface which I'm about to rewrite, so throw it away.
Test Plan: Grepped for all affected symbols, didn't find any hits anywhere.
Maniphest Tasks: T13440
Differential Revision: https://secure.phabricator.com/D20882
Summary:
Ref T13425. The changes in D20865 could incorrectly lead to selection of a DocumentEngine that can not generate document diffs if a file was added or removed (for example, when a source file is added).
Move the engine pruning code to be shared -- we should always discard engines which can't generate a diff, even if we don't have both documents.
Test Plan: Viewed an added source file, no more document ref error arising from document engine selection.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20866
Summary: Ref T13425. When a file (like a Jupyter notebook) is added or removed, we can still render a useful line-by-line diff.
Test Plan:
- Viewed add/modify/remove of Jupyter, source code, and images in 2up/1up mode, everything looked okay.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20865
Summary:
See PHI1468. Engine selection for diffs is currently too aggressive in trying to find a shared engine and will fall back a shared engine with a very low score, causing all ".json" files to render as Jupyter files.
Only pick an engine as a difference engine by default if it's the highest-scoring engine for the new file.
Test Plan: Viewed ".json" files and ".ipynb" files in a revision. Before, both rendered as Jupyter. Now, the former rendered as JSON and the latter rendered as Jupyter.
Differential Revision: https://secure.phabricator.com/D20850
Summary:
Depends on D20844. Ref T13425. When we line up two blocks and they can be interdiffed (generally: they both have the same type of content), let the Engine interdiff them.
Then, make the Jupyter engine interdiff markdown.
Test Plan: {F6898583}
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20845
Summary:
Depends on D20843. Ref T13425. Add very basic support for "Show Hidden Context", in the form of folding it behind an unclickable shield. This isn't ideal, but should be better than nothing.
Prepare for "intraline" diffs on content blocks.
Fix newline handling in Markdown sections in Jupyter notebooks.
Remove the word "visibile" from the codebase.
Test Plan: {F6898192}
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20844
Summary:
Ref T13425. Some diff checks currently sequence incorrectly:
- When we're rendering block lists, syntax highlighting isn't relevant.
- The "large change" guard can prevent rendering of otherwise-renderable changes.
- Actual errors in the document engine (like bad JSON in a ".ipynb" file) aren't surfaced properly.
Improve sequencing somewhat to resolve these issues.
Test Plan:
- Viewed a notebook, no longer saw a "highlighting disabled" warning.
- Forced a notebook to fail, got a useful inline error instead of a popup dialog error.
- Forced a notebook to have a large number of differences, got a rendering out of it.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20843
Summary:
Ref T13425. Currently, code inputs and all outputs are grouped into a single block. This is fine for display notebooks but not great for diffing notebooks.
Instead, split source code input into individual lines with one line per block, and each output into its own block.
This allows you to leave actual line-by-line inlines on source, and comment on outputs individually.
Test Plan: {F6888583}
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20840
Summary:
Depends on D20835. Ref T13425. Ref T13414. When a document has a list of content blocks, we may not be able to diff it directly, but we can hash each block and then diff the hashes (internally "diff" also does approximately the same thing).
We could do this ourselves with slightly fewer layers of indirection, but: diff already exists; we already use it; we already have a bunch of abstractions on top of it; and it's likely much faster on large inputs than the best we can do in PHP.
Test Plan: {F6888169}
Maniphest Tasks: T13425, T13414
Differential Revision: https://secure.phabricator.com/D20836
Summary:
Depends on D20834. Ref T13425. After the change from "th" to "td" for accessibility, the algorithm picks which cells it should highlight slightly improperly (it picks too many cells since it can no longer find the line numbers).
Ideally, it would probably highlight //only// the source content, but there isn't an easy way to do this right now. Settle for an incremental improvement for the moment.
Test Plan: Hovered over line numbers, saw a more accurate highlight area.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20835
Summary: Depends on D20833. Ref T13425. This look like it "just works"?
Test Plan: Left inline comments on a Juptyer notebook. Nothing seemed broken? Confusing.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20834
Summary:
Depends on D20832. Ref T13425. Emit Jupyter notebooks as diffable blocks with block keys.
No diffing or proper inlines yet.
Test Plan: {F6888058}
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20833
Summary:
Depends on D20831. Ref T13425. As an escape hatch to get out of future DocumentEngine rendering behavior, provide a "View As.." option.
Now I can break DocumentEngine real bad and no one can complain.
Test Plan: Used "View As" to swap document engines for image files.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20832
Summary:
Depends on D20830. Ref T13425. Have the image engine elect into block rendering, then emit blocks.
This is rough (the blocks aren't actually diffed yet) but image diffs were already pretty rough so this is approximately a net improvement.
Test Plan: Viewed image diffs, saw nothing worse than before.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20831
Summary:
Ref T13425. Allow DocumentEngines to claim they can produce diffs. If both sides of a change can be diffed by the same document engine and it can produce diffs, have it diff them.
This has no impact on runtime behavior because no upstream engine elects into diff generation yet.
Test Plan: Loaded some revisions, nothing broke.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20830
Summary:
Fixes T13386. See PHI1391. These constraints largely exist already, but are not yet exposed to Conduit.
Also, tweak some keys to support the underlying query.
Test Plan: Ran `differential.revision.search` queries with the new constraints.
Maniphest Tasks: T13386
Differential Revision: https://secure.phabricator.com/D20730
Summary:
Ref T13385. Currently, if you run `arc diff` in a CWD with more than 255 characters, the workflow fatals against the length of the `sourcePath` database column.
In the long term, removing this property is likely desirable.
For now, truncate long values and continue. This only meaningfully impacts relatively obscure interactive SVN workflows negatively, and even there, "some arc commands are glitchy in very long working directories in SVN" is still better than "arc diff fatals".
Test Plan:
- Modified `arc` to submit very long source paths.
- Ran `arc diff`.
- Before: Fatal when inserting >255 characters into `sourcePath`.
- After: Path truncated at 255 bytes.
Maniphest Tasks: T13385
Differential Revision: https://secure.phabricator.com/D20727
Summary:
See <https://discourse.phabricator-community.org/t/view-task-from-maniphest-e-mail-doesnt-have-url/2827>.
I added "View Task" / "View Commit" buttons recently but the logic for generating URIs isn't quite right. Fix it up.
Test Plan:
- Commented on a task.
- Used `bin/mail show-outbound --id ... --dump-html > out.html` to dump the HTML.
- Previewed the HTML in a browser.
- This time, actually clicked the button to go to the task.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20586
Summary:
Fixes T13313. The "Download Raw Diff" workflow in Differential currently uses an older way of interacting with Files that doesn't engage the chunk engine and can't handle 8MB+ files.
Update to `IteratorFileUploadSource` -- we're still passing in a single giant blob, but this approach can be chunked.
This will still break somewhere north of 8MB (it will break at 2GB with the PHP string limit if nowhere sooner, since we're putting the entire raw diff in `$raw_diff` rather than using a rope/stream) but will likely survive diffs in the hundreds-of-megabytes range for now.
Test Plan:
- Added `str_repeat('x', 1024 * 1024 * 9)` to the `$raw_diff` to create a 9MB+ diff.
- Configured file storage with no engine explicitly configured for >8MB chunks (i.e., "reasonably").
- Clicked "Download Raw Diff".
- Before: misleading file storage engine error ("no engine can store this file").
- After: large, raw diff response.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13313
Differential Revision: https://secure.phabricator.com/D20579
Summary: Ref T13303. See B22967. This should be "msortv()" but didn't get updated properly.
Test Plan: The system works!
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13303
Differential Revision: https://secure.phabricator.com/D20585
Summary:
Fixes T13300. Currently, if you create a revision and then immediately land it (either using `--draft` or just beating Harbormaster to the punch) it can be stuck in "Draft" forever.
Instead, count landing changes like this as a publishing action.
Test Plan:
- Used `arc diff --hold` to create a revision, then pushed the commit immediately.
- Before change: revision closed, but was stuck in draft.
- After change: revision closed and was promoted out of draft.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13300
Differential Revision: https://secure.phabricator.com/D20565
Summary:
See downstream <https://phabricator.wikimedia.org/T1050>. Some time ago, we added a "View Revision" button to Differential mail. This hasn't created any problems and generally seems good / desirable.
It isn't trivial to just add everywhere since we need a translation string in each case, but at least add it to Maniphest for now. Going forward, we can fill in more applications as they come up.
Test Plan:
Used `bin/mail show-outbound --id <x> --dump-html`:
{F6470461}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20561
Summary:
Ref T13290. Prior to recent changes, if we parsed some commit C which was associated with a revision R, but R was already closed, we'd skip the whole set of updates because the "close the revision" transaction would fail and we'd throw because we did not `setContinueOnNoEffect()`.
We now continue on no effect so we can get the edge ("commit has revision" / "revision has commit"), since we want it in all cases, but this means we may also apply an extra "Updated revision to reflect committed changes" transaction and new diff. This can happen even if we're careful about not trying to apply this transaction to closed revisions, since two workers may race. (Today, we aren't too careful about this.)
To fix this, just make this transaction no-op itself if the revision is already closed by the time it tries to apply.
This happened on D20451 because a merge commit with the same hash as the last diff was pushed, but it's easiest to reproduce by just running `bin/repository reparse --message <commit>`, which updates related revisions with a new diff every time.
Test Plan:
- Ran `bin/repository reparse --messsage <commit>` several times, on a commit with an associated revision.
- Before: each run attached a new diff and created a new "updated to reflect committed changes" transaction.
- After: repeated runs had no effects.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13290
Differential Revision: https://secure.phabricator.com/D20545
Summary:
Ref T13290. Ref T13291. Now that a full URI is a "mention", the full URI in "Differential Revision: ..." also triggers a mention.
Stop it from doing that, since these mentions are silly/redundant/unintended.
The API here is also slightly odd; simplify it a little bit to get rid of doing "append" with "get + append + set".
Test Plan: Used `bin/repository reparse --publish` to republish commits with "Differential Revision: ..." and verified that the revision PHID was properly dropped from the mention list.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13291, T13290
Differential Revision: https://secure.phabricator.com/D20544
Summary:
See PHI1222. When we publish several transactions to feed at once, we sort them by "action strength" to figure out which one gets to be the title story.
This sort currently uses `msort()`, which uses `asort()`, which is not a stable sort and has inconsistent behavior across PHP versions:
{F6463721}
Switch to `msortv()`, which is a stable sort. Previously, see also T6861.
If all transactions have the same strength, we'll now consistently pick the first one.
This probably (?) does not impact anything in the upstream, but is good from a consistency point of view.
Test Plan:
Top story was published after this change and uses the chronologically first transaction as the title story.
Bottom story was published before this change and uses the chronologically second transaction as the title story.
Both stories have two transactions with the same strength ("create" + "add reviewer").
{F6463722}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20540
Summary:
Ref T13289. In Maniphest, you can currently search for "Owner: none()" to find tasks with no owner, but there's no way to search for "Reviewers: none()" in Differential right now.
Add support for this, since it's consistent and reasonable and doesn't seem too weird or niche.
Test Plan: Searched for "Reviewers: none()", found revisions with no reviewers. Searched for "Reviewers: alice, none()", "Reviewers: alice", and "Reviewers: <no constraint>" and got sensible results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20537
Summary:
See PHI1210. For certain large inputs, we spend more time than we need to replacing tabs with spaces. Add some fast paths:
- When a line only has tabs at the beginning of the line, we don't need to do as much work parsing the rest of the line.
- When a line has no unicode characters, we don't need to vectorize it to get the right result.
Test Plan:
- Added test coverage.
- Profiled this, got a ~60x performance increase on a 36,000 line 3MB text file.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20477
Summary:
See PHI1232, which describes a reasonable use case for wanting information about the "draft" ("Hold as Draft / Do Not Auto-Promote") flag.
Also, flesh out "testPlan" and "summary". It's possible these "blob of remarkup" fields might have metadata some day (e.g., a rendered version or a list of PHIDs or something), but we could add more keys, and we already have some other transactions which work like this.
Test Plan: Used "transaction.search" to fetch these transaction types, saw type information and metadata.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20507
Summary:
Depends on D20469. Ref T13276. See PHI1159. See PHI953. See PHI901.
Allow Herald to detect when "arc land" would (or did) warn users about failed or ongoing builds. This respects the "Warn on Landing" build plan behavior.
To accomplish this:
- When we close a revision, set a "wrong build state" flag if it lands in the wrong build state.
- If the revision is closed when we hit Herald, look for the flag.
- If not (common for push rules, can happen for commit rules if we race against the revision update worker), hit Harbormaster ourselves and check the current state.
Test Plan:
- Wrote a "Require Green" rule.
- Ran it against various commits with various build states (good, not good).
- Fiddled with "Warn on Landing" and saw the effect in rule evaluation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20470
Summary:
Depends on D20468. Ref T13276. See PHI1008.
When a commit or revision "reverts <hash>", and that's the hash of a commit which has a revision, also write a "reverts" edge to the revision.
Test Plan:
Created "reverts X" objects for:
- a revision;
- a commit;
- a commit with a revision (both got marked properly).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20469
Summary:
See PHI1218. When rendering "A vs B", we currently show the properties of diff A without modification.
Instead, take properties from the same place we're taking change details.
See T12664 for a followup.
Test Plan:
- In diff A, removed "+x" from a file.
- In diff B, changed the file but did not remove "+x" from it.
- Diffed B vs A.
- Before change: UI incorrectly shows "+x" removed (both sides incorrect, just showing the change from diff A).
- After change: UI shows 100644 -> null, which is half right.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20478
Summary:
Depends on D20462. Ref T13276. Currently, the "Message" parser also updates related tasks and revisions when a commit is published.
For PHI1165, which ran into a race with message parsing, I originally believed we needed to separate this logic and lock + yield to avoid the race. D20462 provides what is probably a better approach for avoiding the race.
Still, I think separating these "update related revisions" and "updated related tasks" chunks into separate workers is a net improvement. There may still be some value in doing lock + yield in the future to deal with other issues, and when we occasionally run into problems with pulling a diff out of the repository to update the revision (usually because the diff is too big) this isolates the problem better and allows the commit to import.
I think the only thing to watch out for here is that Herald may now run before the revision and commit are attached to one another. This is fine for all current Herald rules, we just need to be mindful in implementing new rules.
Test Plan: Used `bin/repository reparse --message` on various commits, including commits that close revisions and close tasks.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20463
Summary: Depends on D20459. Ref T13276. I'll file a followup to actually destroy the table.
Test Plan:
- Grepped for `TABLE_COMMIT`.
- Ran `bin/storage upgrade -f`, got a clean bill of health.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20461
Summary:
Depends on D20458. Ref T13276. Although I'm not thrilled about "needCommitPHIDs()", it has a few callers, including custom fields. Allow "need + attach + get" to survive for now since they're reasonably modern, at least.
However, use edges instead of "TABLE_COMMIT" and require `need...()` + `get...()`, removing the direct `load...()`.
Also remove `RevisionQuery->withCommitPHIDs(...)`, which has no callers.
Test Plan:
- Grepped for `loadCommitPHIDs` (only two hits, the private `RevisionQuery` method).
- Called "differential.getrevision", got commits.
- Viewed a revision, saw "Commits: ...".
- Grepped for `withCommitPHIDs()`, no callers on `RevisionQuery` (some other query classes have methods with this name).
- Called "differential.query", got commits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20459
Summary: Depends on D20457. Ref T13276. Kill all remaining callers to this method and delete it.
Test Plan:
- Grepped for `loadIDsByCommitPHIDs`.
- Viewed blame again to make sure I didn't break it.
- Viewed "History" view for commits with revisions.
- Viewed "Graph" view for commits with revisions.
- Viewed "Merged Commits" table for commits with revisions.
- Viewed "Compare" table for commits with revisions.
- Viewed "Repository" main page history table for commits with revisions.
- Grepped for `linkRevision`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20458
Summary:
See PHI1213. If we don't have identities for "revision X closed by commit Y" stories, just do the plain non-attribution rendering rather than trying to fall back. Falling back won't work since we don't load the data, which should be OK now since identities seem like they're in generally good shape.
(We could probably just throw out the fallback behavior instead, but we can always clean things up later.)
Test Plan: Forced no commit identity on a revision, loaded it, saw reasonable story rendering.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20460
Summary:
Ref T12164. Ref T13276. Currently, the parsing pipeline copies the author and committer names and PHIDs into the transcaction record as metadata. They are then rendered directly from the metadata.
This makes planned changes to the parsing pipeline (to prevent races when multiple commits matching a single revision are pushed simultaneously) more difficult, and generally won't work with repository identities.
Instead, load the commit and use its identities.
Test Plan: Loaded a revision, saw the same story rendering for a "Closed by commit" story.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276, T12164
Differential Revision: https://secure.phabricator.com/D20418
Summary:
Depends on D20412. See PHI1147.
- Index the targets of "Add Reviewer", "Add Blocking Reviewer", "Add Auditor", "Add Subscriber", and "Remove Subscriber" Herald rules. My major goal is to get Owners packages. This will also hit projects/users, but we just don't read those edges (for now, at least).
- Add a "Related Herald Rules" panel to Owners Package pages.
- Add a migration to reindex Herald rules for the recent build plan stuff and this, now that such a migration is easy to write.
Test Plan:
Ran migration, verified all rules reindexed.
{F6372695}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20413
Summary:
Ref T13276. This was introduced in D2586 to power a "trigger audits when the committed change does not match the reviewed change" feature. It was removed without ceremony in D15939. Broadly, rebases mean that this sort of feature can't really work like this and this approach is inherently unreliable; see also T182.
This property no longer has readers, and is unlikely to get any in the future since my planned pathway for "committed code must match reviewed code, modulo an automated rebase" is automating the rebase via "Land Revision", not comparing the diff text.
Remove this to simplify the flow of data here so that things in T13276 can be fixed more easily.
Test Plan: Grepped for `vsDiff`, no hits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20395
Summary:
See PHI1175. An install would like to trigger some reminders/guidance if users don't link revisions to JIRA issues.
Expose "JIRA Issue URIs" as a field so Herald can act on the presence or absence of issues.
I'm exposing "JIRA Issue URIs", not a field like "[ Has Jira Issue ][ is true ]", since it's a bit more flexible: you can use a regexp to test against particular `PROJ-123` project prefixes in JIRA, for example.
Test Plan: {F6367696}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20386
Summary:
Depends on D20359. Fixes T12098. When you add a new "Form" item and pick "Create Revision", you currently get a bad link. This is because Differential is kind of special and the form isn't usable directly, even though Differential does use EditEngine.
Allow EditEngine to specify a different create URI, then specify the web UI paste-a-diff flow to fix this.
Test Plan:
- Added "Create Revision" to a portal, clicked it, was sensibly put on the diff flow.
- Grepped for `getCreateURI()`, the only other real use case is to render the "Create X" dropdowns in the upper right.
- Clicked one of those, still worked great.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12098
Differential Revision: https://secure.phabricator.com/D20360
Summary: See PHI985. The layers above this may return `array()` to mean "one hunk with a line-1 offset". Accept either `array()` or `array(1 => ...)` to engage the scope engine.
Test Plan: See PHI985.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20363
Summary:
Depends on D20291. Ref T13259. Move all the simple cases (where paging depends only on the partial object and does not depend on keys) to a simple wrapper.
This leaves a smaller set of more complex cases where we care about external data or which keys were requested that I'll convert in followups.
Test Plan: Poked at things, but a lot of stuff is still broken until everything is converted.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20292
Summary:
Ref T13249.
- When a line has only increased in indent depth, don't red-fill highlight the left side of the diff. Since reading a diff //mostly// involves focusing on the right side, indent depth changes are generally visible enough without this extra hint. The extra hint can become distracting in cases where there is a large block of indent depth changes.
- Move the markers slightly to the left, to align them with the gutter.
- Make them slightly opaque so they're a little less prominent.
Test Plan: See screenshots.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20251
Summary:
Ref T13258. The general idea here is "if arc land prompted you and you hit 'y', you get a warning about it on the timeline".
This is similar to the existing warning about landing revisions in the wrong state and hitting "y" to get through that. See D18808, previously.
These warnings make it easier to catch process issues at a glance, especially because the overall build status is now more complicated (and may legally include some failures on tests which are marked as unimportant).
The transaction stores which builds had problems, but I'm not doing anything to render that for now. I think you can usually figure it out from the UI already; if not, we could refine this.
Test Plan:
- Used `bin/differential attach-commit` to trigger extraction/attachment.
- Attached a commit to a revision with various build states, and various build plan "Warn When Landing" flags.
- Got sensible warnings and non-warnings based on "Warn When Landing" setting.
{F6251631}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20239
Summary: Ref T13258. Makes the new "Hold Drafts" behavior actually work.
Test Plan:
- Created a build plan which does "Make HTTP Request" somewhere random and then waits for a message.
- Created a Herald rule which "Always" runs this plan.
- Created revisions, loaded them, then sent their build targets a "fail" message a short time later.
- With "Always": Current behavior. Revision was held as a draft while building, and returned to me for changes when the build failed.
- With "If Building": Revision was held as a draft while building, but promoted once the build failed.
- With "Never": Revision promoted immediately, ignoring the build completely.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20232
Summary: See PHI1112. See T784. Although some more general/flexible solution is arriving eventually, adding this rule seems reasonable for now, since it's not a big deal if we remove it later to replace this with some fancier system.
Test Plan: Created a diff with the official Go generated marker, saw the changeset marked as generated.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20237
Summary:
See PHI1096. Depends on D20213. An install is reporting a hard-to-reproduce issue where a non-transaction gets queued by Herald somehow. This might be in third-party code.
Sprinkle the relevant parts of the code with `final` and type checking to try to catch the problem before it causes a fatal we can't pull a stack trace out of.
Test Plan: Poked around locally (e.g., edited revisions to cause Herald to trigger), but hard to know if this will do what it's supposed to or not without deploying and seeing if it catches anything.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20214
Summary:
Depends on D20197. Ref T13161. We currently try to build a "ScopeEngine" even for diffs with no context (e.g., `git diff` instead of `git diff -U9999`).
Since we don't have any context, we won't really be able to figure out anything useful about scopes. Also, since ScopeEngine is pretty strict about what it accepts, we crash.
In these cases, just don't build a ScopeEngine.
Test Plan: Viewed a diff I copy/pasted with `git diff` instead of an `arc diff` / `git diff -U99999`, got a sensible diff with no context instead of a fatal.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20198
Summary:
Depends on D20196. See PHI985. When empty, the "moved/copied" gutter currently renders with the same background color as the rest of the line. This can be misleading because it makes code look more indented than it is, especially if you're unfamiliar with the tool:
{F6225179}
If we remove this misleading coloration, we get a white gap. This is more clear, but looks a little odd:
{F6225181}
Instead, give this gutter a subtle background fill in all casses, to make it more clear that it's a separate gutter region, not a part of the text diff:
{F6225183}
Test Plan: See screenshots. Copied text from a diff, added/removed inlines, etc.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20197
Summary:
Ref T12822. Ref PHI878. This is some leftover code from the old selection behavior that prevented visual selection of the left side of a diff if the user clicked on the right -- basically, a much simpler attack on what ultimately landed in D20191.
I think the change from `th` to `td` "broke" it so it didn't interfere with the other behavior, which is why I didn't have to remove it earlier. It's no longer necessary, in any case.
Test Plan: Grepped for behavior name, selected stuff on both sides of a diff.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822
Differential Revision: https://secure.phabricator.com/D20196
Summary:
Ref T13161.
- Don't show ">>" when the line indentation changed but the text also changed, this is just "the line changed".
- The indicator seems a little cleaner if we just reuse the existing "bright" colors, which already have colorblind colors anyway.
Test Plan: Got slightly better rendering for some diffs locally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20195
Summary:
Ref T12822. Ref T2495. This is the good version of D20193.
Currently, we display various nonprintable characters (ZWS, nonbreaking space, various control characters) as themselves, so they're generally invisible.
In T12822, one user reports that all their engineers frequently type ZWS characters into source somehow? I don't really believe this (??), and this should be fixed in lint.
That said, the only real reason not to show these weird characters in a special way was that it would break copy/paste: if we render ZWS as "🐑", and a user copy-pastes the line including the ZWS, they'll get a sheep.
At least, they would have, until D20191. Now that this whole thing is end-to-end Javascript magic, we can copy whatever we want.
In particular, we can render any character `X` as `<span data-copy-text="Y">X</span>`, and then copy "Y" instead of "X" when the user copies the node. Limitations:
- If users select only "X", they'll get "X" on their clipboard. This seems fine. If you're selecting our ZWS marker *only*, you probably want to copy it?
- If "X" is more than one character long, users will get the full "Y" if they select any part of "X". At least here, this only matters when "X" is several spaces and "Y" is a tab. This also seems fine.
- We have to be kind of careful because this approach involves editing an HTML blob directly. However, we already do that elsewhere and this isn't really too hard to get right.
With those tools in hand:
- Replace "\t" (raw text / what gets copied) with the number of spaces to the next tab stop for display.
- Replace ZWS and NBSP (raw text) with a special marker for display.
- Replace control characters 0x00-0x19 and 0x7F, except for "\t", "\r", and "\n", with the special unicode "control character pictures" reserved for this purpose.
Test Plan:
- Generated and viewed a file like this one:
{F6220422}
- Copied text out of it, got authentic raw original source text instead of displayed text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822, T2495
Differential Revision: https://secure.phabricator.com/D20194
Summary:
Ref T12822. Fixes a few things:
- Firefox selection of weird ranges with an inline between the start and end of the range now works correctly.
- "Show More Context" rows now render, highlight, and select properly.
- Prepares for nodes to have copy-text which is different from display-text.
- Don't do anything too fancy in 1-up/unified mode. We don't copy line numbers after the `content: attr(data-n)` change, but that's as far as we go, because trying to do more than that is kind of weird and not terribly intuitive.
Test Plan:
- Selected and copied weird ranges in Firefox.
- Kept an eye on "Show More Context" rows across select and copy operations.
- Generally poked around in Safari/Firefox/Chrome.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822
Differential Revision: https://secure.phabricator.com/D20192
Summary:
Ref T12822. Ref T13161. By default, when users select text from a diff and copy it to the clipboard, they get both sides of the diff and all the line numbers. This is usually not what they intended to copy.
As of D20188, we use `content: attr(...)` to render line numbers. No browser copies this text, so that fixes line numbers.
We can use "user-select" CSS to visually prevent selection of line numbers and other stuff we don't want to copy. In Firefox and Chrome, "user-select" also applies to copied text, so getting "user-select" on the right nodes is largely good enough to do what we want.
In Safari, "user-select" is only visual, so we always need to crawl the DOM to figure out what text to pull out of it anyway.
In all browsers, we likely want to crawl the DOM anyway because this will let us show one piece of text and copy a different piece of text. We probably want to do this in the future to preserve "\t" tabs, and possibly to let us render certain character codes in one way but copy their original values. For example, we could render "\x07" as "␇".
Finally, we have to figure out which side of the diff we're copying from. The rule here is:
- If you start the selection by clicking somewhere on the left or right side of the diff, that's what you're copying.
- Otherwise, use normal document copy rules.
So the overall flow here is:
- Listen for clicks.
- When the user clicks the left or right side of the diff, store what they clicked.
- When a selection starts, and something is actually selected, check if it was initiated by clicking a diff. If it was, apply a visual effect to get "user-select" where it needs to go and show the user what we think they're doing and what we're going to copy.
- (Then, try to handle a bunch of degenerate cases where you start a selection and then click inside that selection.)
- When a user clicks elsewhere or ends the selection with nothing selected, clear the selection mode.
- When a user copies text, if we have an active selection mode, pull all the selected nodes out of the DOM and filter out the ones we don't want to copy, then stitch the text back together. Although I believe this didn't work well in ~2010, it appears to work well today.
Test Plan: This mostly seems to work in Safari, Chrome, and Firefox. T12822 has some errata. I haven't tested touch events but am satisfied if the touch event story is anything better than "permanently destroys data".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20191
Summary:
Ref T13161. Ref T12822. Today, we use invisible Zero-Width Spaces to try to improve copy/paste behavior from Differential.
After D20188, we no longer need ZWS characters to avoid copying line numbers. Get rid of these secret invisible semantic ZWS characters completely.
This means that both the left-hand and right-hand side of diffs become copyable, which isn't desired. I'll fix that with a hundred thousand lines of Javascript in the next change: this is a step toward everything working better, but doesn't fix everything yet.
Test Plan:
- Grepped for `zws`, `grep -i zero | grep -i width`.
- Copied text out of Differential: got both sides of the diff (not ideal).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20189
Summary:
Ref T13161. Ref T12822. See PHI870. Long ago, the web was simple. You could leave your doors unlocked, you knew all your neighbors, crime hadn't been invented yet, and `<th>3</th>` was a perfectly fine way to render a line number cell containing the number "3".
But times have changed!
- In PHI870, this isn't good for screenreaders. We can't do much about this, so switch to `<td>`.
- In D19349 / T13105 and elsewhere, this `::after { content: attr(data-n); }` approach seems like the least bad general-purpose approach for preventing line numbers from being copied. Although Differential needs even more magic beyond this in the two-up view, this is likely good enough for the one-up view, and is consistent with other views (paste, harbormaster logs, general source display) where this technique is sufficient on its own.
The chance this breaks //something// is pretty much 100%, but we've got a week to figure out what it breaks. I couldn't find any issues immediately.
Test Plan:
- Created, edited, deleted inlines in 1-up and 2-up views.
- Replied, keyboard-navigated, keyboard-replied, drag-selected, poked and prodded everything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20188
Summary:
Depends on D20185. Ref T13161. Fixes T6791.
See some discusison in T13161. I want to move to a world where:
- whitespace changes are always shown, so users writing YAML and Python are happy without adjusting settings;
- the visual impact of indentation-only whitespace changes is significanlty reduced, so indentation changes are easy to read and users writing Javascript or other flavors of Javascript are happy.
D20181 needs a little more work, but generally tackles these visual changes and lets us always show whitespace changes, but show them in a very low-impact way when they're relatively unimportant.
However, a second aspect to this is how the diff is "aligned". If this file:
```
A
```
..is changed to this file:
```
X
A
Y
Z
```
...diff tools will generally produce this diff:
```
+ X
A
+ Y
+ Z
```
This is good, and easy to read, and what humans expect, and it will "align" in two-up like this:
```
1 X
1 A 2 A
3 Y
4 Z
```
However, if the new file looks like this instead:
```
X
A'
Y
Z
```
...we get a diff like this:
```
- A
+ X
+ A'
+ Y
+ Z
```
This one aligns like this:
```
1 A
1 X
2 A'
3 Y
4 Z
```
This is correct if `A` and `A'` are totally different lines. However, if `A'` is pretty much the same as `A` and it just had a whitespace change, human viewers would prefer this alignment:
```
1 X
1 A 2 A'
3 Y
4 Z
```
Note that `A` and `A'` are different, but we've aligned them on the same line. `diff`, `git diff`, etc., won't do this automatically, and a `.diff` doesn't have a way to say "these lines are more or less the same even though they're different", although some other visual diff tools will do this.
Although `diff` can't do this for us, we can do it ourselves, and already have the code to do it, because we already nearly did this in the changes removed in D20185: in "Ignore All" or "Ignore Most" mode, we pretty much did this already.
This mostly just restores a bit of the code from D20185, with some adjustments/simplifications. Here's how it works:
- Rebuild the text of the old and new files from the diff we got out of `arc`, `git diff`, etc.
- Normalize the files (for example, by removing whitespace from each line).
- Diff the normalized files to produce a second diff.
- Parse that diff.
- Take the "alignment" from the normalized diff (whitespace removed) and the actual text from the original diff (whitespace preserved) to build a new diff with the correct text, but also better diff alignment.
Originally, we normalized with `diff -bw`. I've replaced that with `preg_replace()` here mostly just so that we have more control over things. I believe the two behaviors are pretty much identical, but this way lets us see more of the pipeline and possibly add more behaviors in the future to improve diff quality (e.g., normalize case? normalize text encoding?).
Test Plan:
{F6217133}
(Also, fix a unit test.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T6791
Differential Revision: https://secure.phabricator.com/D20187
Summary:
Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely.
I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction.
Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T3498
Differential Revision: https://secure.phabricator.com/D20185
Summary:
Ref T13161. See PHI723. Our whitespace handling is based on whitespace flags like `diff -bw`, mostly just for historical reasons: long ago, the easiest way to minimize the visual impact of indentation changes was to literally use `diff -bw`.
However, this approach is very coarse and has a lot of problems, like detecting `"ab" -> "a b"` as "only a whitespace change" even though this is always semantic. It also causes problems in YAML, Python, etc. Over time, we've added a lot of stuff to mitigate the downsides to this approach.
We also no longer get any benefits from this approach being simple: we need faithful diffs as the authoritative source, and have to completely rebuild the diff to `diff -bw` it. In the UI, we have a "whitespace mode" flag. We have the "whitespace matters" configuration.
I think ReviewBoard generally has a better approach to indent depth changes than we do (see T13161) where it detects them and renders them in a minimal way with low visual impact. This is ultimately what we want: reduce visual clutter for depth-only changes, but preserve whitespace changes in strings, etc.
Move toward detecting and rendering indent depth changes. Followup work:
- These should get colorblind colors and the design can probably use a little more tweaking.
- The OneUp mode is okay, but could be improved.
- Whitespace mode can now be removed completely.
- I'm trying to handle tabs correctly, but since we currently mangle them into spaces today, it's hard to be sure I actually got it right.
Test Plan: {F6214084}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20181
Summary:
Ref T13249. Ref T11738. See PHI985. Currently, we have a crude heuristic for guessing what line in a source file provides the best context.
We get it wrong in a lot of cases, sometimes selecting very silly lines like "{". Although we can't always pick the same line a human would pick, we //can// pile on heuristics until this is less frequently completely wrong and perhaps eventually get it to work fairly well most of the time.
Pull the logic for this into a separate standalone class and make it testable to prepare for adding heuristics.
Test Plan: Ran unit tests, browsed various files in the web UI and saw as-good-or-better context selection.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249, T11738
Differential Revision: https://secure.phabricator.com/D20171
Summary: Ref T13088. Prepares for putting test names in a separate table to release the 255-character limit.
Test Plan: Viewed revisions, buildables, builds, test lists, specific tests.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D20179
Summary: Ref T13250. See D20149. Mostly: clarify semantics. Partly: remove magic "null" behavior.
Test Plan: Poked around, but mostly just inspection since these are pretty much one-for-one.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20154
Summary:
See <https://discourse.phabricator-community.org/t/conduit-call-is-generating-phd-log-error-message/2380/>. If you run builds against a diff which is not attached to a revision (this is unusual) we still try to publish to the associated revision. This won't work since there is no associated revision.
Since bare diffs don't really have a timeline, just publish nowhere for now.
Test Plan:
- Created a diff.
- Did not attach it to a revision.
- Created a build plan with "make http request + wait for response".
- Manually ran the build plan against the bare diff.
- Used `bin/phd debug task` to run the build and hit a "revision not attached" exception during publishing.
- Applied patch.
- Ran `bin/phd debug task`, got clean (no-op) publish.
- Sent build a failure message with "harbormaster.sendmessage", got a failed build.
This isn't a real workflow, but shouldn't fail.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20156
Summary: See D20136. This method is sort of inherently bad because it is destructive for some inputs (`x=1&x=2`) and had "PHP-flavored" behavior for other inputs (`x[]=1&x[]=2`). Move to explicit `...AsMap` and `...AsPairList` methods.
Test Plan: Bit of an adventure, see inlines in a minute.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20141
Summary: The textarea is, in fact, above the description!
Test Plan: Description text changed.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D20092
Summary: See PHI1048. We have similar transactions elsewhere already (particularly, on tasks) and these edges don't have any special properties (like "Closes Txx As Wontfix" edges do) so this doesn't create any sort of peril.
Test Plan: {F6170556}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20075
Summary:
See PHI1044. When you push a commit, we try to detect if the diff is the same as the last diff in Differential. This is a soft/heuristic check and only used to provide a hint in email ("CHANGED SINCE LAST DIFF").
For some changes, we can end up on this pathway with a binary changeset for a deleted file in the commit, and try to fetch the file data. This will fail since the file has been deleted. I'm not //entirely// sure why this hasn't cropped up before and didn't try to truly build an end-to-end test case, but it's a valid state that we shouldn't fatal in.
When we get here in this state, just detect a change. This is safe, even if it isn't always correct.
(This will be revisited in the future so I'm favoring fixing the broken behavior for now.)
Test Plan: This required some rigging, but I modified `bin/differential extract` to run the `isDiffChangedBeforeCommit()` code, then rigged a commit/diff to hit this case and reproduced the reported error. After the change, the comparison worked cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20056
Summary:
In ~2012, the first of these options was added because someone who hates dogs and works at Asana also hated `[Differential]` in the subject line. The use case there was actually //removing// the text, not changing it, but I made the prefix editable since it seemed like slightly less of a one-off.
These options are among the dumbest and most useless config options we have and very rarely used, see T11760. A very small number of instances have configured one of these options.
Newer applications stopped providing these options and no one has complained.
You can get the same effect with `translation.override`. Although I'm not sure we'll keep that around forever, it's a reasonable replacement today. I'll call out an example in the changelog to help installs that want to preserve this option.
If we did want to provide this, it should just be in {nav Applications > Settings} for each application, but I think it's wildly-low-value and "hack via translations" or "local patch" are entirely reasonable if you really want to change these strings.
Test Plan: Grepped for `subject-prefix`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19993
Summary:
Fixes T7477. Fixes T13066. Currently, inbound mail is processed by the first receiver that matches any "To:" address. "Cc" addresses are ignored.
**To, CC, and Multiple Receivers**
Some users would like to be able to "Cc" addresses like `bugs@` instead of having to "To" the address, which makes perfect sense. That's the driving use case behind T7477.
Since users can To/Cc multiple "create object" or "update object" addresses, I also wanted to make the behavior more general. For example, if you email `bugs@` and also `paste@`, your mail might reasonably make both a Task and a Paste. Is this useful? I'm not sure. But it seems like it's pretty clearly the best match for user intent, and the least-surprising behavior we can have. There's also no good rule for picking which address "wins" when two or more match -- we ended up with "address order", which is pretty arbitrary since "To" and "Cc" are not really ordered fields.
One part of this change is removing `phabricator.allow-email-users`. In practice, this option only controlled whether users were allowed to send mail to "Application Email" addresses with a configured default author, and it's unlikely that we'll expand it since I think the future of external/grey users is Nuance, not richer interaction with Maniphest/Differential/etc. Since this option only made "Default Author" work and "Default Author" is optional, we can simplify behavior by making the rule work like this:
- If an address specifies a default author, it allows public email.
- If an address does not, it doesn't.
That's basically how it worked already, except that you could intentionally "break" the behavior by not configuring `phabricator.allow-email-users`. This is a backwards compatility change with possible security implications (it might allow email in that was previously blocked by configuration) that I'll call out in the changelog, but I suspect that no installs are really impacted and this new behavior is generally more intuitive.
A somewhat related change here is that each receiver is allowed to react to each individual email address, instead of firing once. This allows you to configure `bugs-a@` and `bugs-b@` and CC them both and get two tasks. Useful? Maybe not, but seems like the best execution of intent.
**Sender vs Author**
Adjacently, T13066 described an improvement to error handling behavior here: we did not distinguish between "sender" (the user matching the email "From" address) and "actor" (the user we're actually acting as in the application). These are different when you're some internet rando and send to `bugs@`, which has a default author. Then the "sender" is `null` and the "author" is `@bugs-robot` or whatever (some user account you've configured).
This refines "Sender" vs "Author". This is mostly a purity/correctness change, but it means that we won't send random email error messages to `@bugs-robot`.
Since receivers are now allowed to process mail with no "sender" if they have some default "actor" they would rather use instead, it's not an error to send from an invalid address unless nothing processes the mail.
**Other**
This removes the "abundant receivers" error since this is no longer an error.
This always sets "external user" mail recipients to be unverified. As far as I can tell, there's no pathway by which we send them email anyway (before or after this change), although it's possible I'm missing something somewhere.
Test Plan:
I did most of this with `bin/mail receive-test`. I rigged the workflow slightly for some of it since it doesn't support multiple addresses or explicit "CC" and adding either would be a bit tricky.
These could also be tested with `scripts/mail/mail_handler.php`, but I don't currently have the MIME parser extension installed locally after a recent upgrade to Mojave and suspect T13232 makes it tricky to install.
- Ran unit tests, which provide significant coverage of this flow.
- Sent mail to multiple Maniphest application emails, got multiple tasks.
- Sent mail to a Maniphest and a Paste application email, got a task and a paste.
- Sent mail to a task.
- Saw original email recorded on tasks. This is a behavior particular to tasks.
- Sent mail to a paste.
- Sent mail to a mock.
- Sent mail to a Phame blog post.
- Sent mail to a Legalpad document.
- Sent mail to a Conpherence thread.
- Sent mail to a poll.
- This isn't every type of supported object but it's enough of them that I'm pretty confident I didn't break the whole flow.
- Sent mail to an object I could not view (got an error).
- As a non-user, sent mail to several "create an object..." addresses.
- Addresses with a default user worked (e.g., created a task).
- Addresses without a default user did not work.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13066, T7477
Differential Revision: https://secure.phabricator.com/D19952
Summary:
Depends on D19953. Ref T9141. We have a "MetaMTAAttachment" object, rename it to "MailAttachment".
Also add a "Header" object and an "EmailMessage" object. Currently, mail adapters have a large number of methods like `setSubject()`, `addTo()`, etc, that I would like to remove.
I'd like the API to be more like `sendMessage(PhabricatorMailExternalMessage $message)`. This is likely a significant simplification anyway, since the implementations of all these methods are just copy/pasted boilerplate anyway (lots of `$this->subject = $subject;`) and this will let Adapters support other message media (SMS, APNS, Whatsapp, etc.)
That's a larger change, but move toward a world where we can build a concrete `$message` object for "email" or "sms".
The `PhabricatorMailEmailMessage` object is just a dumb, flat object representation of the information an adapter needs to actually send mail. The existing `PhabricatorMetaMTAMail` is a much more complex object and has a lot of rich data (delivery status, related object PHIDs, etc) and is a storage object.
The new flow will be something like:
- `PhabricatorMetaMTAMail` (possibly renamed) is the storage object for any outbound message on this channel. It tracks message content, acceptable delivery media (SMS vs email), delivery status, related objects, has a PHID, and has a daemon worker associated with delivering it.
- It builds a `PhabricatorMailExternalMessage`, which is a simple, flat description of the message it wants to send. The subclass of this object depends on the message medium. For email, this will be an `EmailMessage`. This is just a "bag of strings" sort of object, with appropriate flattened values for the adapter to work with (e.g., Email has email addresses, SMS has phone numbers).
- It passes the `ExternalMessage` (which is a `MailMessage` or `SMSMessage` or whatever) to the adapter.
- The adapter reads the nice flat properties off it and turns them into an API request, SMTP call, etc.
This is sort of how things work today anyway. The major change is that I want to hand off a "bag of strings" object instead of calling `setX()`, `setY()`, `setZ()` with each individual value.
Test Plan: Grepped for `MetaMTAAttachment`. This doesn't change any behavior yet.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T9141
Differential Revision: https://secure.phabricator.com/D19954
Summary:
Ref T7477. The various "create a new X via email" applications (Paste, Differential, Maniphest, etc) all have a bunch of duplicate code.
The inheritance stack here is generally a little weird. Extend these from a shared parent to reduce the number of callsites I need to change when this API is adjusted for T7477.
Test Plan: Ran unit tests. This will get more thorough testing once more pieces are in place.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7477
Differential Revision: https://secure.phabricator.com/D19950
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).
The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.
The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.
So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.
Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.
Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19920
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.
All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.
Test Plan: Used `grep willRenderTimeline` to find callsites, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19919
Summary:
Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.
`PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.
The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.
Try to clean this up:
- Stop requiring `willRenderTimeline()` to be implemented.
- Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
- Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
- If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.
This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.
Test Plan:
- Viewed audits, revisions, and mocks with inline comments.
- Used "Show Older" to page a revision back in history (this is relevant for "View Data").
- Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19918
Summary:
Ref T13222. See PHI995. Before making a change to inline rendering, consolidate this code for generating the "alice added inlines comments." and "alice marked X inlines as done." transactions.
Both Differential and Diffusion have four very similar chunks of code. Merge them into shared methods and reduce code duplication across the methods.
(In the next change, I plan to hide the "done" story when the mark affects your own inline, since users marking their own inlines as "done" is generally not very interesting or useful.)
Test Plan: As author and reviewer/auditor, added inlines in Differential and Diffusion. As author, marked own and others inlines as done and undone. Got sensible transaction rendering and persistence of "Done".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19858
