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
Summary:
Ref T13216. See PHI985. We currently use a banner to warn you when a revision has no reviewers or only disabled users, but since the changes to track "Resign" more explicilty we'll no longer warn you if everyone has resigned.
(Previously, they'd no longer be reviewers, so you'd end up with the "no reviewers are assigned" warning if everyone resigned.)
This can still interact slightly oddly with some states (e.g., only a package or project reviewer) but I'd like to wait for T731 to tighten those cases up, and they're more advanced/unusual.
Test Plan:
{F6026832}
{F6026833}
{F6026834}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19834
Summary:
Ref T13222. See PHI683. Currently, you can "Change subtype..." via Conduit and the bulk editor, but not via the comment action stack or edit forms.
In PHI683 an install is doing this often enough that they'd like it to become a first-class action. I've generally been cautious about pushing this action to become a first-class action (there are some inevitable rough edges and I don't want to add too much complexity if there isn't a use case for it) but since we have evidence that users would find it useful and nothing has exploded yet, I'm comfortable taking another step forward.
Currently, `EditEngine` has this sort of weird `setIsConduitOnly()` method. This actually means more like "this doesn't show up on forms". Make it better align with that. In particular, a "conduit only" field can already show up in the bulk editor, which is goofy. Change this to `setIsFormField()` and convert/simplify existing callsites.
Test Plan:
There are a lot of ways to reach EditEngine so this probably isn't entirely exhaustive, but I think I got pretty much anything which is likely to break:
- Searched for `setIsConduitOnly()` and `getIsConduitOnly()`, converted all callsites to `setIsFormField()`.
- Searched for `setIsLockable()`, `setIsReorderable()` and `setIsDefaultable()` and aligned these calls to intent where applicable.
- Created an Almanac binding.
- Edited an Almanac binding.
- Created an Almanac service.
- Edited an Almanac service.
- Edited a binding property.
- Deleted a binding property.
- Created and edited a badge.
- Awarded and revoked a badge.
- Created and edited an event.
- Made an event recurring.
- Created and edited a Conpherence thread.
- Edited and updated the diff for a revision.
- Created and edited a repository.
- Created and disabled repository URIs.
- Created and edited a blueprint.
- Created and edited tasks.
- Created a paste, edited/archived a paste.
- Created/edited/archived a package.
- Created/edited a project.
- Made comments.
- Moved tasks on workboards via comment action stack.
- Changed task subtype via comment action stack.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19842
Summary:
Ref T13216. See PHI985. You currently can't commandeer an abandoned revision, but this workflow is perfectly fine.
The caution here is just around weird use cases where, e.g., users want to reopen a revision to add a revert to it. These workflows tend to create problems so we try to guide users away from them.
Test Plan: {F6026841}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19835
Summary: Depends on D19837. Ref T13216. See PHI985. There's an off-by-one error here between how inline comments store "length" and how context rendering treats "length". We need to add 1 to the length, but currently do it a little too early. Do it slightly later so that inlines on the final line of a file render properly.
Test Plan: Left an inline on the final line of a new file, saw it render properly in HTML mail.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19838
Summary:
Ref T13216. Fixes T12920. See PHI911. If you reject a revision and then resign from it, it stays in "Needs Revision".
There's some arguable motivation for this, but it's inconsistent with how "Accept" works (if the last accepting reviewer resigns, we kick you out of "Accepted"). Make it consistent.
Test Plan:
- As the only reviewer: requested changes to a revision, then resigned.
- Before: revision stays in "Needs Revision".
- After: revision moves back to "Needs Review".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216, T12920
Differential Revision: https://secure.phabricator.com/D19840
Summary:
Ref T13216. See PHI970. Ref T13054. See some discussion in T13216.
When a Harbormaster Buildable object is first created for a Diff, it has no `containerPHID` since the revision has not yet been created.
We later (after creating a revision) send the Buildable a message telling it that we've added a container and it should re-link the container object.
Currently, we send this message in `applyExternalEffects()`, which runs inside the Differential transaction. If Harbormaster races quickly enough, it can read the `Diff` object before the transaction commits, and not see the container update.
Add a `didCommitTransaction()` callback after the transactions commit, then move the message code there instead.
Test Plan:
- See T13216 for substantial evidence that this change is on the right track.
- Before change: added `sleep(15)`, reproduced the issue reliably.
- After change: unable to reproduce issue even with `sleep(15)` (the `containerPHID` always populates correctly).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216, T13054
Differential Revision: https://secure.phabricator.com/D19807
Summary:
See PHI975. Ref T13216. Ref T2543. Previously, see D19204 and PHI433.
When you're acting on a draft revision, we change the button text to "Submit Quietly" as a hint that your actions don't generate notifications yet.
However, this isn't accurate when one of your actions is "Request Review", which causes the revision to publish.
Allow actions to override the submit button text, and make the "Request Review" action change the button text to "Publish Revision".
The alternative change I considered was to remove the word "Quietly" in all cases.
I'm not //thrilled// about how complex this change is to adjust one word, but the various pieces are all fairly clean individually. I'm not sure we'll ever be able to use it for anything else, but I do suspect that the word "Quietly" was the change in D19204 with the largest effect by far (see T10000).
Test Plan:
- Created a draft revision. Saw "Submit Quietly" text.
- Added a "Request Review" action, saw it change to "Publish Revision".
- Reloaded page, saw stack saved and "Publish Revision".
- Removed action, saw "Submit Quietly".
- Repeated on a non-draft revision, button stayed put as "Submit".
- Submitted the various actions, saw them have the desired effects.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216, T2543
Differential Revision: https://secure.phabricator.com/D19810
Summary: Depends on D19789. Ref T13217. Continue updating things to use the new %Q-flavored conversions instead of smushing a bunch of strings together.
Test Plan: Browsed around, far fewer errors. These changes are largely mechanical in nature.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19790
Summary: Depends on D19785. Ref T13217. This converts many of the most common clause construction pathways to the new %Q / %LQ / %LO / %LA / %LJ semantics.
Test Plan: Browsed around a bunch, saw fewer warnings and no obvious behavioral errors. The transformations here are generally mechanical (although I did them by hand).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: hach-que
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19789
Summary:
Ref T13210. See PHI944. When parsing certain large diffs (the case in PHI944 is an 2.5-million line JSON diff), we spend ~66% of runtime and ~80% of memory doing copy detection (the little yellow bar which shows up to give you a hint that code was moved around within a diff).
This is pretty much pointless and copy hints are almost certainly never useful on large changes. Instead, just bail if the change is larger than some arbitrary "probably too big for copy hints to ever be useful" threshold (here, 65535 lines).
Test Plan:
Roughly, ran this against a 2.5 million line JSON diff:
```
$changes = id(new ArcanistDiffParser())->parseDiff($raw_diff);
$diff = DifferentialDiff::newFromRawChanges($viewer, $changes);
```
Before the changes, it took 20s + 2.5GB RAM to parse. After the changes, it took 7s + 500MB RAM to parse.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19748
Summary:
See PHI920. Ref T13210. Since the HTML is just:
```
<a>View Inline</a><span>filename.txt</span>
```
..double-clicking "filename.txt" in email selects "Inlinefilename.txt".
Add a space to stop this. At least in Safari, a space between the tags is not sufficient (perhaps because the parent is a `<div>`?). I couldn't find an authoritative-seeming source on what the rules for this actually are and adding a space here fixes the issue without changing the visual rendering, so just put it here.
Test Plan:
- Made an inline.
- Used `bin/mail show-outbound --id ... --dump-html` to dump the HTML.
- Double-clicked the filename.
{F5929186}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19742
Summary: Ref T13210. See PHI806. This enables basic export of revisions into flat data formats. This isn't too fancy, but just covers the basics since the driving use case isn't especially concerned about getting all the fields and details.
Test Plan: Exported some revisions into JSON, got sensible output.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19743
Summary:
Ref T13202. See PHI900. Fixes T12814. Pholio currently builds HTML comments in an older way that can dump a bunch of over-escaped HTML into mail bodies.
Update the logic to be more similar to the Differential rendering logic and stop over-escaping things.
The result isn't perfect, but is dramatically less broken.
Test Plan: {F5919860}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202, T12814
Differential Revision: https://secure.phabricator.com/D19733
Summary:
Ref T13202. See PHI906. This is a reasonable capability which we support in some other applications already.
(The only real reason not to support this is that it creates some clutter in the UI, but I think we're generally in better shape now than we were in the past, and we could make this UI collapse/fold at some point.)
Test Plan: Ran queries with a minimum date, a maximum date, both, and neither. Saw appropriate results in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19732
Summary:
Ref T13202. See PHI903 and PHI894. When a bot leaves 100 inline comments on the same file and the revision has a 30-member recipient list, we currently highlight the file 3000 times when building mail.
Instead, engage the parse cache so we highlight it once and reuse the cache 2,999 times.
Test Plan:
- Added debugging code to stop after mail generation and show cache hits/misses.
- Left a bunch of inlines in a file.
- Ran the worker with `bin/worker execute --id ...`.
- Before change: saw 4 comments x 2 recipients = 8 cache misses
- After change: saw 8 cache hits (cache already filled from web UI rendering)
- Removed debugging code, ran worker to completion.
- Used `bin/mail show-outbound --id ... --dump-html` to inspect resulting mail, saw no functional differences.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19721
Summary:
Ref T13195. Ref T8573. This allows reviewers to mark their own inline comments as "Done" before they submit them.
If you're leaving a non-actionable comment like "this is good", you can pre-check "Done" to give the author a hint that you don't expect any response.
Test Plan: On revisions and commits, added inlines as the author and a reviewer/auditor. Marked them done/not-done before submitting. As author, marked the not-done ones done after submitting. Checked preivews, toggled done/not done states.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19634
Summary:
Ref T13195. See PHI861. The "+ Draft" flag is not currently exposed over the API, but seems stable enough to expose.
Also expose the "hold as draft" flag, normally `arc diff --draft`.
Today, you can get "+ Draft" with some other state by:
- abandoning a draft revision ("Abandoned + Draft"); or
- using `arc diff --plan-changes` with an older `arc` version ("Changes Planned + Draft").
Test Plan: Called `differential.revision.search` via Conduit and got sensible-looking results for revisions in various states.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19648
Summary: See D19632. Agreed that this is more clear.
Test Plan: Read carefully.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19644
Summary:
Ref T13195. An install had a user apply a transaction which added about 1,000 inline comments. Rendering the email for this transaction took a very long time because the context section for each comment must be highlighted separately.
We can make the highlighting faster (in this case, by porting the lexer to PHP) but it's also sort of silly to include more than 100 inlines in an email. These emails are likely to be truncated by outbound size rules at some point anyway. Instead, limit inlines rendered directly into email to the first 100 per transaction group.
Test Plan:
Set limit to 2, added 4 comments, viewed text and HTML emails:
{F5859967}
{F5859968}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19632
Summary:
Ref T13195. Ref T8573. The inline comment controllers currently use outdated `$user = $this->getRequest()->getUser()` calls.
Instead, use `$viewer = $this->getViewer()`.
This is just a small consistency update with no behavioral changes.
Test Plan: Viewed and added inlines in Differential and Diffusion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19633
Summary:
See PHI844. Ref T13189.
Add "Revision test plan" as an available field for Herald. This is a little niche -- and a little odd because it sticks around even if you fully disable test plans -- but probably broadly reasonable.
The existing "Revision summary" field counterintuitively included the test plan. Separate this out since it's now a separate field and the behavior was weird historic nonsense. I'll note this in the changelog.
Test Plan: Wrote a rule using both fields, verified they generated the expected values.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19623
Summary: Ref T13189. See PHI710. Ref T13088. Fixes T9951. Allow callers to `harbormaster.sendmessage` to specify that the test details are remarkup so they can use rich formatting and include links, files, etc.
Test Plan: {F5840098}
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13189, T13088, T9951
Differential Revision: https://secure.phabricator.com/D19615
Summary:
Fixes T12251. Ref T13189. See PHI610. The difficulty here is that we don't want to disclose Phabricator account information to Buildkite. We're comfortable disclosing information from `git`, etc.
- For commits, use the Identity to provide authorship information from Git.
- For revisions, use the local commit information on the Diff to provide the Git/Mercurial/etc author of the HEAD commit.
Test Plan:
- Built commits and revisions in Buildkite via Harbormaster.
- I can't actually figure out how to see author information on the Buildkite side, but the values look sane when dumped locally.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13189, T12251
Differential Revision: https://secure.phabricator.com/D19614
Summary:
Ref T13187. See PHI811. If the file tree is enabled and visible, set the default tab to "History".
- This is a bit magic.
- It won't work entirely on mobile (we can't tell that you're on mobile on the server, so we'll pick the "History" tab even though the file tree isn't actually visible on your device).
- There's no corresponding logic in Diffusion. Diffusion doesn't have the same tab layout, but this makes things somewhat inconsistent.
So I don't love this, but we can try it and see if it's confusing or helpful on the balance.
Test Plan: With filetree on and off, reloaded revisions. Saw appropriate tab selected by default.
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19601
Summary: Ref T13187. Ref T13176. With drafts, we actually want to suppress this link on "first broadcast" (the first time we send mail), not on "new object" (when the revision is created as a draft).
Test Plan: Poked at this locally, will keep an eye on it in production.
Reviewers: amckinley
Maniphest Tasks: T13187, T13176
Differential Revision: https://secure.phabricator.com/D19598
Summary:
Depends on D19594. See PHI823. Ref T13164.
- Add a label for the "X" button in comment areas, like "Remove Action: Change Subscribers".
- Add a label for the floating header display options menu in Differential.
- Add `role="button"` to `PHUIButtonView` objects that we render with an `<a ...>` tag.
Test Plan:
Viewed a revision with `?__aural__=true`:
- Saw "Remove Action: ..." label.
- Saw "Display Options" label.
- Used inspector to verify that some `<a class="button" ...>` now have `<a class="button" role="button" ...>`. This isn't exhaustive, but at least improves things. A specific example is the "edit", "reply", etc., actions on inline comments.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19595
Summary:
Ref T13164. See PHI693. In Differential, you can {nav View Options > View Standalone} to get a standalone view of a single changeset. You can also arrive here via the big changeset list for revisions affecting a huge number of files.
We currently suggest that all the keyboard shortcuts work, but some do not. In particular, the "Next File" and "Previous File" keyboard shortcuts (and some similar shortcuts) do not work. In the main view, the next/previous files are on the same page. In the standalone view, we'd need to actually change the URI.
Ideally, we should do this (and, e.g., put prev/next links on the page). As a first step toward that, hide the nonfunctional shortcuts to stop users from being misled.
Test Plan:
- Viewed a revision in normal and standalone views.
- No changes in normal view, and all keys still work ("N", "P", etc).
- In standalone view, "?" no longer shows nonfunctional key commands.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19571
Summary:
Ref T13151. See PHI616. Fixes T8163.
This adds `/D123/new/`, which shows the changes to the revision since the last timeline action you took.
It also adds a link to this view to diff update emails.
Test Plan:
- Followed this link with a recent comment and no touches since update, ended up with sensible diff selections.
- Updated revision, generated email, saw an appropriate link.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151, T8163
Differential Revision: https://secure.phabricator.com/D19541
Summary:
When generating test data to solve a bug I have encountered, I noticed Lipsum was not working correctly for Differential Revisions and Pastes.
It seemed like they weren't updated after some refactoring. This fixes that by updating them.
Test Plan: Run Lipsum for all objects, and note that it has much less failure.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D19534
Summary:
See PHI746. See also T11833, perhaps. Ref T13151.
Long ago, parent revisions were called "dependent revisions". This was changed to "parent revisions" in the action UI to improve clarity, but not changed in the timeline stories.
Update the timeline stories to use the same language the actions in the UI use.
Test Plan:
{F5732876}
{F5732877}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19514
Summary:
See PHI725. Ref T13151. These actions are somewhat unusual and I considered different ways to represent them (make them look like "status" transactions; build multiple synthetic transactions) but ultimately landed on the simplest approach of just exposing them more or less as they exist internally.
I haven't included data for any of them. Most don't really have any data, but "accept" does. I'm holding off on providing more data until after T731, which may shake up the internal format.
Test Plan: Applied most of these transactions against a revision, queried for it with `transaction.search`, got distinguishable transactions out.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19509
Summary:
Depends on D19487. Ref T13151. See PHI647. For some objects, like revisions, we can build slightly more useful secure email without actually disclosing anything.
In the general case, the object monogram may disclose information (`#acquire-competitor`) but most do not, so applications can whitelist an acceptable nondisclosing subject and link.
Support doing this, and make Differential do it. When we don't have a whitelisted URI but do know the object the mail is about, include a generic PHID-based URI; these are always nondisclosing.
Test Plan:
- Without the Differential changes, sent normal mail (no changes) and secure mail (new generic PHID-based link).
- With the Differential changes, sent secure mail; got richer subject and body link.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19488
Summary:
Ref T13151. See PHI701. Unified diffs are currently missing the logic to apply the "old-full" and "new-full" classes, which results in a too-light coloration for fully added or removed lines.
Make this logic consistent with the two-up renderer so we use the same colors in both.
Test Plan: Viewed diffs and swapped between 1-up and 2-up renderers, now saw the same coloration.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19482
Summary:
Ref T13137. See that task for discussion.
When we show a diff-of-diffs, we often render stubs for files which didn't change between the diffs. These stubs usually aren't a big deal, but for certain types of changes (like refactors) they can create a lot of clutter.
Instead, hide these stubs and show a notice that we hid them.
Test Plan:
- Created a revision affecting 4 files.
- Updated it with a diff that changed only 1 of the 4 files.
- Added an inline comment to a different file.
- Viewed the diff of diffs.
- Before: 4 changesets with two "nothing changed" stubs.
- After: 2 changesets with the stubs hidden.
{F5621083}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19453
Summary:
Ref T13137. The "analyze/cache data about changesets" step is becoming more involved. We recently added detection for generated code to support "Ignore generated changes" in Owners, and I now plan to hash the new file content so we can hide changes which have no effect.
Before adding this new hashing step, pull the "detect copied code" and "detect generated code" stuff out and move them to a separate `ChangesetEngine`. Then support doing a changeset rebuild directly with `bin/differential rebuild-changesets`.
This simplifies things a bit and makes testing easier since you don't need to keep creating new revisions to re-run copy/generated/hash logic.
Test Plan: Ran `bin/differential rebuild-changesets --revision Dxxx`, saw changesets rebuild. See also next change.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19452
Summary: Ref T13137. See PHI592. Depends on D19444. Apply a limit up front to stop patches which are way too big (e.g., 600MB of videos) from generating in the first place.
Test Plan:
- Configured inline patches in git format.
- Created a normal revision, got an inline git patch.
- Created a revision with a 10MB video file, got no inline patch.
- (Added a bunch of debugging stuff to make sure the internal pathway was working.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19445
Summary:
See PHI638. When a diff is large (between 100 and 1000 files), we collapse content by default unless a change also has inline comments.
This rule isn't explicitly explained anywhere. Although it's not really a critical rule, it fits easily enough into the UI callout.
Also render the UI callout in a slightly more modern way and avoid `hsprintf()`.
Test Plan:
{F5596496}
- Also, clicked the "Expand" link and saw everything expand properly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19430
Summary:
Depends on D19425. Ref T13130. See PHI251. Now that changesets have a durable "generated" attribute, we can let owners packages check it when we're computing which packages are affected by a revision.
There's no way to actualy configure a package to have this behavior yet.
Test Plan:
- Created a revision affecting a generated file and a non-generated file.
- When I faked `mustMatchUngeneratedPaths()` to `return true;`, saw the non-generated file get no packages owning it.
- Normally: lots of packages owning it).
- Created a revision affecting only generated files.
- When I faked things, saw no Owners actions trigger.
- Normally: some packages added reviewers or subscribers.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19426
Summary:
Ref T13130. See PHI251. Currently, changesets are marked as "generated" (i.e., the file contains generated code and does not normally need to be reviewed) at display time.
An install would like support for having Owners rules ignore generated files. Additionally, future changes anticipate making "generated" and some other similar behaviors more flexible and more general.
To support these, move toward a world where:
- Changesets have "attributes": today, generated. In the future, perhaps: third-party, highlight-as, encoding, enormous-text-file, etc.
- Attributes are either "trusted" (usually: the server assigned the attribute) or "untrusted" (usually: the client assigned the attribute). For attributes like "highlight-as", this isn't relevant, but I'd like to provide tools so that you can't make `arc` mark every file as "generated" and sneak past review rules in the future.
Here, the `differential.generated-paths` config can mark a file as "generated" with a trusted attribute. The `@generated`-in-content rule can mark a file as "generated" with an untrusted attribute.
Putting these attributes on changesets at creation time instead of display time will let Owners interact with changesets cheaply: it won't have to render an entire changeset just to figure out if it's generated or not.
Test Plan:
- Created a revision touching several files, some generated and some not.
- Saw the generated files get marked properly with attribute metadata in the database, and show/fold as "Generated" in the UI.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19425
Summary:
Depends on D19416. Ref T13110. Ref T13130. See PHI598. When rendering a "Very Large" revision (affecting more than 1,000 files) we currently compute the package/changeset ownership map normally.
This is basically a big list of which packages own which of the files affected by the change. We use it to:
# Show which packages own each file in the table of contents.
# Show an "(Owns No Changed Paths)" hint in the reviewers list to help catch out-of-date packages that are no longer relevant.
However, this is expensive to build. We don't render the table of contents at all, so (1) is pointless. The value of (2) is very small on these types of changes, and certainly not worth spending many many seconds computing ownership.
Instead, just skip building out these relationships for very large changes.
Test Plan: Viewed a very large change with package owners; verified it no longer built package map data and rendered the package owners with no "(Owns No Changed Paths)" hints.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130, T13110
Differential Revision: https://secure.phabricator.com/D19418
Summary:
Ref T13110. Ref T13130. When a revision is "large" (100 - 1000 files) we hide the actual textual changes by default. When it is "very large" (more than 1000 files) we hide all the changesets by default.
For "very large" diffs, we currently still show the "large" warning, which doesn't really make sense since there aren't any actual changesets.
When a diff is "very large", don't show the "large" warning.
Test Plan:
- Viewed a small diff (<100 files), saw no warnings.
- Viewed a large diff (100-1000 files), saw just the large warning.
- Viewed a very large diff (>1000 files).
- Before: both "large" and "very large" help warnings.
- After: just "very large" warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130, T13110
Differential Revision: https://secure.phabricator.com/D19416
Summary:
See <https://twitter.com/HayleyCAnderson/status/988873585363009536>.
Currently, the action dropdown in Differential shows a heavy "X" after "Request Changes" and a heavy checkmark after "Accept Revision".
Although I'm not convinced that the messaging around "Request Changes" is too strong, I do think these marks are out of place in modern Differential. They came from a simpler time when this dropdown had fewer actions, but feel a little weird and inconsistent to me in the modern UI.
Let's try getting rid of them and see how it goes?
Test Plan:
- Viewed these actions in the dropdown, no longer saw the mark icons.
- Grepped for these unicode sequences without getting any other hits.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19405
Summary:
Ref T13130. See PHI483. Currently, "Plan Changes + Draft" uses rules like "Plan Changes", not rules like "Draft", and allows "Accept".
This isn't consistent with how "Draft" and "Accept" work in other cases. Make "Plan Changes + Draft" more like "Draft" for consistency.
Also fix a string that didn't have a natural English version.
Test Plan:
- Added a failing build plan.
- Created a revision.
- Loaded the revision before builds completed, saw a nicer piece of text about "waiting for builds" instead of "waiting for 2 build(s)".
- Builds failed, which automatically demoted the reivsion to "Changes Planned + Draft".
- As the author and as a reviewer, verified all the actions available to me made sense (particularly, no "Accept").
- Abandoned the revision to test "Abandoned + Draft".
- As the author and as a reviewer, verified all the actions available to me made sense.
- Reclaimed the revision, then used "Request Review" to send it to "Needs Review". Verified that actions made sense and, e.g., reviewers could now "Accept" normally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19398
Summary:
Ref T13124. See PHI584. When you create a draft revision and it automatically demotes to "Changes Planned + Draft" because builds fail, let it promote to "Needs Review" automatically if builds pass. Usually, this will be because someone restarted the builds and they worked the second time.
Although I'm a little wary about adding even more state transitions to the diagram in T13110#237736, I think this one is reasonably natural and not ambiguous.
Test Plan:
- Created a failing build plan with a "Throw Exception" step.
- Created a revision which hit the build plan, saw it demote to "Changes Planned" when Harbormaster failed.
- Edited the build plan to remove the "Throw Exception" step, restarted the build, got a pass.
- Saw revision promote again:
{F5526104}
I didn't exhaustively test that the other 40 state transitions still work properly, but I think the scope of this change is small enough that it's unlikely I did much collateral damage.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13124
Differential Revision: https://secure.phabricator.com/D19380
Summary:
Ref T13124. See PHI593.
When you `arc diff` in a Git or Mercurial repository, we upload some information about the local commits in your working copy which the change was generated from.
In the future (for example, with T1508) we may increase the prominence of this feature.
Provide a stable way to read this information back via the API. This roughly mirrors the information we provide about commits in "diffusion.commit.search", although the latter is less fleshed-out today.
Test Plan: Used `differential.diff.search` to retrieve commit information about Git, Mercurial, and Subversion diffs. (There's no info for Subversion, but it doesn't crash or anything.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13124
Differential Revision: https://secure.phabricator.com/D19386
Summary:
Ref T13127. Users with red/green colorblindness may have difficulty using this element in its current incarnation.
We could give it different behavior if the "Accessibility" option is set for red/green colorblind users, but try a one-size-fits-all approach since the red/green aren't wholly clear anwyay.
Test Plan: {F5530050}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13127
Differential Revision: https://secure.phabricator.com/D19385
Summary:
Depends on D19370. See T13124. See PHI549. The particular install in PHI549 migrated a large amount of data via the fallback hunk migration script, which does not compress hunks.
Add a mode to `bin/differential migrate-hunk` that amounts to "compress all the hunks which would benefit from compression".
Test Plan: Ran `bin/differential migrate-hunk` with `--auto`, `--all`, `--to`, `--id`, and `--dry-run` in various mixtures. Forced a bunch of hunks to raw ("byte") format, saw it cleanly upgrade them to compressed ("gzde") format.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19371
Summary:
Depends on D19369. Ref T13120. Add a flag to migrate every hunk.
This isn't terribly useful on its own, but I'm going to add an `--auto` flag next so that you can run `--auto --all` to migrate hunks to the preferred hunk format.
Test Plan: Ran `bin/differential migrate-hunk --all --to text`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13120
Differential Revision: https://secure.phabricator.com/D19370
Summary: This is a good spelling, but maybe a better spelling is possible.
Test Plan: hmmm
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19369
Summary:
See PHI573. Ref T13120. Drafts were recently changed so that "draft" and "broadcast" are separate flags, and you can have non-broadcasting revisions in states other than "draft" if builds fail on a draft or you abandon a draft.
However, when draft mode is entered with `arc diff --draft` and you have prototypes off, this flag wasn't being set correctly.
Test Plan: Disabled prototypes, created a revision with `arc diff --draft`, observed that `draft.broadcast` is now correctly `false`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13120
Differential Revision: https://secure.phabricator.com/D19360
Summary:
See PHI574. Ref T13120. When you `Ref Txx` or `Fixes Txxx`, we mark it "unmentionable" to prevent the task from generating both a reference and a mention.
If you add a reference to an object (like a commit hash) to a custom remarkup field, there's currently no real way to prevent it from generating a mention, except that you can explicitly mark the PHID as unmentionable on the Editor.
This isn't exactly a first-class feature, but we technically do it in `PhabricatorRepositoryCommitMessageParserWorker`, and it probably doesn't hurt or interfere with anything to support it slightly better.
In Differential, respect any existing value and append new values to it rather than overwriting the value.
Test Plan: Edited a revision summary to include `Ref Txxx`, saw only a reference (not a mention) generate.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13120
Differential Revision: https://secure.phabricator.com/D19361
Summary: This reverts D18524. See that revision for discussion.
Test Plan: Viewed home menu, saw application names as menu items.
Differential Revision: https://secure.phabricator.com/D19308
Summary:
Depends on D19296. Ref T13110.
- Remove the "Large Changesets" documentation since we now degrade very large changesets and I don't have any evidence that anyone has ever tried to follow any of the recommendations in this document.
- Remove references to it.
- When an older revision doesn't have denormalized size information on the Revision object itself, don't render a scale element (instead of rendering a bogus one).
- Try to improve terminology consistency around "Large Change" (100-1000 files) vs "Very Large Change" (1000+ files) vs "Enormous Change" (too large to hold in memory).
Test Plan: Viewed revisions; grepped for documentation.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19298
Summary: Depends on D19295. Ref T13110. Degrade the review UX when users try to interact with changes which are too large to receive human review.
Test Plan: Reduced the "very large" limit, browsed some changes, saw various elements degrade.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19296
Summary: Ref T13110. Installs have various reasons for sending unreviewable changes (changes where the text of the change will never be reviewed by a human) through Differential anyway. Prepare for accommodating this more gracefully by building a standalone changeset list page which paginates the changesets.
Test Plan: Clicked the new "Changeset List" button on a revision, was taken to a separate page.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19295
Summary: Ref T13110. See PHI230. Show revision sizes on a roughly logarithmic scale from 1-7 stars. See D16322 for theorycrafting on this element.
Test Plan: Looked at some revisions, saw plausible-looking size markers.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19294
Summary:
See PHI489. Ref T13110. At least for now, this just shows "..." at the end since you can click the revision to see the whole list anyway.
Also remove the older-style external Handle passing in favor of lazy construction via HandlePool.
Test Plan: Viewed revisions, fiddled with the 7 limit, got sensible-seeming "..." behavior.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19293
Summary: Depends on D19289. Ref T13110. This flag has been obsolete for some time and has no callers.
Test Plan: Grepped for `hasReviewTransaction`, no hits.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19290
Summary:
Depends on D19288. Ref T13110. In addition to kicking revisions back to "Changes Planned" when builds fail, notify the author that they need to fix their awful garbage change.
(The actual email could be more useful than it currently is.)
Test Plan: Created a revision with failing remote builds, saw email about the problem generate.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19289
Summary: Depends on D19287. Ref T13110. Currently, "Abandon" and then "Reclaim" moves you out of "Draft" without setting the "Should Broadcast" flag. Keep these revisions in draft instead.
Test Plan: Reclaimed an abandoned + draft revision, got a draft revision instead of a "needs review + nonbroadcast" revision (which isn't a meaningful state).
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19288
Summary:
Depends on D19286. Ref T13110. After builds fail remote builds, put revisions back in the author's queue.
This doesn't actually notify the author quite yet.
Test Plan: Made a failing build plan run on revisions, created a revision, saw it demote after builds failed.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19287
Summary: Depends on D19285. Ref T13110. When you update an "Abandoned + But, Never Promoted" revision or (in the future) a "Changes Planned + But, Never Promoted" revision, return it to the "Draft" state rather than promoting it.
Test Plan: Updated an "Abandoned + Draft" revision, saw it return to "Draft".
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19286
Summary:
Depends on D19284. Ref T13110. It's now possible to get a revision into a "Abandoned + But, Never Promoted From Draft" state. Show this in the header and provide the draft hint above the comment area.
Also, remove `shouldBroadcast()`. The method `getShouldBroadcast()` now has the same meaning.
Finally, migrate existing drafts to `shouldBroadcast = false` and default `shouldBroadcast` to `true`. If we don't do this, every older revision becomes a non-broadcasting revision because this flag was not explicitly set on revision creation before, only on promotion out of draft.
Test Plan: Ran migration; abandoned draft revisions and ended up in a draft + abandoned state.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19285
Summary:
Depends on D19283. Ref T13110. To enable "Changes Planned + But, Still A Draft" and "Abandoned + But, Never Promoted From Draft" states, decouple the "broadcast" flag from the "draft" state.
Broadcast behavior is now based only on the `shouldBroadcast` flag, and revisions in any state may have this flag.
Revisions gain this flag when created as a non-draft, or when they leave the draft state for the first time.
There are probably still some ways you can get the wrong result here -- maybe abandon + update -- but those can be cleaned up as they arise.
Test Plan: Kinda poked it a bit but I'll vet this more heavily at the end of this sequence.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19284
Summary:
Depends on D19282. Ref T13110. I want to introduce "Changes Planned + Still A Draft" and "Abandoned + Still A Draft" states, at a minimum.
I think the "hasBroadcast" flag is effectively identical to a hypothetical "stillADraft" flag, so rename it to "shouldBroadcast" to better match its intended behavior.
This just changes labels, not any behavior.
Test Plan: Grepped for `hasBroadcast` and `HAS_BROADCAST`.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19283
Summary:
Depends on D19281. This increases consistency between build timeline publishing and revision draft promotion.
There's no real behavioral change here (switching how publishing worked already changed the beahvior) but this sends more callsites down the same code paths.
Since the builds we're looking at include completed builds, change the term "active" to "impactful". This describes the same set of builds, but hopefully describes them more accurately.
Test Plan: Created a local revision, saw it plausibly interact with draft status and promote. There are a lot of moving parts here and some stuff may well have slipped through.
Differential Revision: https://secure.phabricator.com/D19282
Summary:
Depends on D19280. Ref T13110. Although Harbormaster cares about all builds, Differential does not practically care about local lint and unit results in determining build status.
In Differential, orient publishing around "remote builds" instead of "builds".
This does not yet change any of the draft logic, it just makes the timeline story use newer logic.
Test Plan: Used `bin/harbormaster publish` (with some guard-clause removal) to publish some buildables to revisions without anything crashing.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19281
Summary:
Ref T13110. Currently, build status is published the same way for every Buildable by the BuildEngine.
I want to change this to delegate publishing to each Buildable, particularly so that Differential may use more detailed rules for handling builds and drafts.
Rather than add additional methods to the existing `BuildableInterface`, add an engine generator method instead. This is a pattern which has seen more use recently (e.g., in Ferret) and lets us pay a little more upfront to pull complex pieces of logic out of the main class and let them use inheritence more easily. If we had Traits that might cover this to some degree.
I'd expect to eventually reduce the size of `BuildableInterface` and move the `CircleCI` and `BuildKite` interfaces so that the `BuildableEngine` implements them instead of the main object.
Here, this new engine does nothing and is never instantiated. In upcoming changes, publishing logic will move into it so that Differential can handle publishing differently.
Test Plan: Ran `arc liberate`, loaded pages, grepped for `BuildableInterface`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19278
Summary:
Ref T13114.
- Followup fix for D19267, which didn't work correctly with //new// revision creation.
- Followup fix for changes in T11015. Some of the querying logic was still handling "/x.y" and "/x.y/" differently. Instead, normalize consistently to "/x.y/"
Test Plan:
- Created a new revision cleanly.
- Created a package owning only a `example.txt` file and saw Differential find it as an owning package in the table of contents.
Maniphest Tasks: T13114
Differential Revision: https://secure.phabricator.com/D19268
Summary: Ref T13114. See PHI515. Updating a revision with the same, currently active diff became an error at some point (probably D19175). This is inconsistent; make it an allowable no-op instead.
Test Plan:
- Updated a revision's diff via Conduit.
- Updated to the same diff, no-op.
- Tried to update a different revision, error ("already attached elsewhere").
- Updated with a different diff.
- Tried to update with the original diff, error ("previously attached version").
Maniphest Tasks: T13114
Differential Revision: https://secure.phabricator.com/D19267
Summary:
This change prevents the following error when using PHP 7.2:
```
ERROR 2: count(): Parameter must be an array or an object that implements Countable at [/usr/local/lib/php/phabricator/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php:132]
```
A similar issue was fixed in D18964
Test Plan: Tested in a live system.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19242
Summary:
See PHI457. There's no real reason not to allow this, it just wasn't clear if it was useful. See D18626.
An install had a user `arc diff` and then sprint out the door to take a very long vacation before the builds finished. One failed, so the revision is stuck as a draft forever. This seems like a reasonable motivation for allowing "Commandeer".
Test Plan: Successfully commandeered a draft.
Differential Revision: https://secure.phabricator.com/D19228
Summary:
See PHI431. Ref T13102. An install is interested in a custom "non-sticky" accept action, roughly.
On the one hand, this is a pretty hacky patch. However, I suspect it inches us closer to T731, and I'm generally comfortable with exploring the realms of "Accept Next Update", "Unblock Without Accepting", etc., as long as most of it doesn't end up enabled by default in the upstream.
Test Plan:
- Accepted and updated revisions normally, saw accepts respect global stickiness.
- Modified the "Accept" action to explicitly be unsticky, saw nonsticky accept behavior after update.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19211
Summary: See PHI433. This beefs up reminder texts for drafts a little bit since some users in the wild aren't always seeing/remembering the existing, fairly subtle hints.
Test Plan: Created a reivsion with `--draft`, viewed it, saw richer reminders.
Differential Revision: https://secure.phabricator.com/D19204
Summary: Ref T13099. See PHI424. Fixes T11664. Several installs are interested in having these behaviors available in Owners by default and they aren't difficult to provide, it just makes the UI kind of messy. But I think there's enough general interest to justify it, now.
Test Plan: Created a package which owns "/" with a "With Non-Owner Author" review rule which I own. Created a revision, no package reviewer. Changed rule to "All", updated revision, got package reviewer.
Maniphest Tasks: T13099, T11664
Differential Revision: https://secure.phabricator.com/D19180
Summary:
Ref T13099. Ref T12787. See PHI417. Differential has new "irresponsible" warnings in the timeline somewhat recently, but these publish feed stories that don't link to the revision or have other relevant details, so they're confusing on the balance.
These have a high strength so they render on top, but we actually just want to hide them from the feed and let "abraham closed Dxyz by committing rXzzz." be the primary story.
Modularize things more so that we can get this behavior. Also, respect `shouldHideForFeed()` at display time, not just publishing time.
Test Plan: Used `bin/differential attach-commit` on a non-accepted revision to "irresponsibly land" a revision. Verified that feed story now shows "closed by commit" instead of "closed irresponsibly".
Maniphest Tasks: T13099, T12787
Differential Revision: https://secure.phabricator.com/D19179
Summary: Depends on D19175. Ref T13099. This fills in "close" and "update" transactions so that they show which commit(s) caused the action.
Test Plan: Used `transaction.search` to query some revisions, saw commit PHID information.
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19176
Summary: Ref T13099. Move most of the "Update" logic to modular transactions
Test Plan: Created and updated revisions. Flushed the task queue. Grepped for `TYPE_UPDATE`. Reviewed update transactions in the timeline and feed.
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19175
Summary:
See PHI416. If you raise a lint message in a deleted file, we don't render any text on the right hand side so the message never displays.
This is occasionally still legitimate/useful, e.g. to display a "don't delete this file" message. At least for now, show these messages on the left.
Test Plan: Posted a lint message on a deleted file via `harbormaster.sendmessage`, viewed revision, saw file expand with synthetic inline for lint.
Differential Revision: https://secure.phabricator.com/D19171
Summary:
Ref T13090. The default width changed recently to become much wider, but the behavior on this control isn't great. Instead:
- Pick a default width somewhere between the two.
- Make the width sticky across show/hide (pressing "f" twice remembers your width instead of resetting it).
- Make the width sticky across reloads (dragging the bar, then reloading the page keeps the bar in the same place).
Test Plan:
- Without settings, loaded page: got medium-width bar.
- Dragged bar wide/narrow, toggled on/off with "f", got persistent width.
- Dragged bar wide/narrow, reloaded page, got persistent width.
- Dragged bar wide/narrow, toggled it off, reloaded page, toggled it on, got persistent width.
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19129
Summary:
Ref T13083. Facts has a fair amount of weird hardcoding and duplication of responsibilities. Reduce this somewhat: no more hard-coded fact aggregates, no more database-driven list of available facts, etc. Generally, derive all objective truth from FactEngines. This is more similar to how most other modern applications work.
For clarity, hopefully: rename "FactSpec" to "Fact". Rename "RawFact" to "Datapoint".
Split the fairly optimistic "RawFact" table into an "IntDatapoint" table with less stuff in it, then dimension tables for the object PHIDs and key names. This is primarily aimed at reducing the row size of each datapoint. At the time I originally wrote this code we hadn't experimented much with storing similar data in multiple tables, but this is now more common and has worked well elsewhere (CustomFields, Edges, Ferret) so I don't anticipate this causing issues. If we need more complex or multidimension/multivalue tables later we can accommodate them. The queries a single table supports (like "all facts of all kinds in some time window") don't make any sense as far as I can tell and could likely be UNION ALL'd anyway.
Remove all the aggregation stuff for now, it's not really clear to me what this should look like.
Test Plan: Ran `bin/fact analyze` and viewed web UI. Nothing exploded too violently.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19119
Summary: Revisions with blocking reviewers had this stamp built incorrectly, which cascaded into trying to use `array()` as a PHID. Recover so these tasks succeed.
Test Plan: Will deploy production.
Differential Revision: https://secure.phabricator.com/D19082
Summary: Like "Commit Hook" rules, these also fire oddly and don't have an object PHID or a list of transactions.
Test Plan: Verified that "Call Webhooks" was no longer available from Diff rules, but still available from other rule types.
Differential Revision: https://secure.phabricator.com/D19069
Summary:
See PHI346. Ref T13054. If you have prototypes enabled on the server but use `master` / `stable` on the client and run `arc diff --plan-changes`, the transition is rejected because "Draft -> Changes Planned" isn't currently a legal transition.
Allow this transition if not coming from the web UI (to keep it out of the dropdown).
Test Plan:
- Ran `arc diff --plan-changes` on `master`, got a "Changes Planned" revision instead of a validation error.
- Ran `arc diff` without `--plan-changes`, got a draft, verified that "Plan Changes" still doesn't appear in the action dropdown.
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19067
Summary:
Depends on D19065. Ref T13054. Instead of just updating `containerPHID` and hoping for the best, queue a proper BuildWorker to process a "your container has changed, update it" message.
We also need to remove a (superfluous) `withContainerPHIDs()` when loading active diffs for a revision.
Test Plan:
- Without daemons, created a revision and saw builds stick in "preparing" with no container PHID, but also stay in draft mode.
- With daemons, saw builds actually build and get the right container PHID.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19066
Summary: Depends on D19063. Ref T13054. Prepare for the addition of a new `PREPARING` status by getting rid of the "scattered mess of switch statements" pattern of status management.
Test Plan: Searched/browsed buildables. Viewed buildables. Viewed revisions. Grepped for all affected symbols.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19064
Summary: Depends on D19056. Fixes T8475. Ref T13054. Merges "ModernHunk" back into "Hunk".
Test Plan: Grepped for `modernhunk`. Reviewed revisions. Created a new revision. Used `bin/differential migrate-hunk` to migrate hunks between storage formats and back.
Maniphest Tasks: T13054, T8475
Differential Revision: https://secure.phabricator.com/D19057
Summary: Ref T13054. Ref T8475. This table has had no readers or writers for more than a year after it was migrated to the modern table.
Test Plan: Ran migration, verified that all the data was still around.
Maniphest Tasks: T13054, T8475
Differential Revision: https://secure.phabricator.com/D19056
Summary: See PHI356. Adds inline comment and done counts to the filetree. Also makes the filetree wider by default.
Test Plan: Fiddled with filetrees in different browsers on different revisions. Added inlines, marked them done/undone.
Differential Revision: https://secure.phabricator.com/D19041
Summary: See PHI356. Makes it easier to pick out change types in the filetree view in Differential.
Test Plan: Created a diff with adds, copies, moves, deletions, and binary files. Viewed in Differential, had an easier time picking stuff out.
Differential Revision: https://secure.phabricator.com/D19040
Summary:
Depends on D19021. Ref T13053. When you "Subscribe", or make some other types of edits, we don't necessarily have reviewer data, but may now need it to do the new recipient list logic.
I don't have a totally clean way to deal with this in the general case in mind, but just load it for now so that things don't fatal.
Test Plan: Subscribed to a revision with the "Subscribe" action.
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19022
Summary:
Depends on D19019. Ref T13053. Fixes T12689. See PHI178.
Currently, if `@alice` resigns from a revision but `#alice-fan-club` is still a subscriber or reviewer, she'll continue to get mail. This is undesirable.
When users are associated with an object but have explicitly disengaged in an individal role (currently, only resign in audit/differential) mark them "unexpandable", so that they can no longer be included through implicit membership in a group (a project or package).
`@alice` can still get mail if she's a explicit recipient: as an author, owner, or if she adds herself back as a subscriber.
Test Plan:
- Added `@ducker` and `#users-named-ducker` as reviewers. Ducker got mail.
- Resigned as ducker, stopped getting future mail.
- Subscribed explicitly, got mail again.
- (Plus some `var_dump()` sanity checking in the internals.)
Reviewers: amckinley
Maniphest Tasks: T13053, T12689
Differential Revision: https://secure.phabricator.com/D19021
Summary:
Depends on D19012. Ref T13053. In D19012, I've changed "Thread-Topic" to always use PHIDs.
This change drops the selective on-object storage we have to track the original, human-readable title for objects.
Even if we end up backing out the "Thread-Topic" change, we'd be better off storing this in a table in the Mail app which just has `<objectPHID, first subject we used when sending mail for that object>`, since then we get the right behavior without needing every object to have this separate field.
Test Plan: Grepped for `original`, `originalName`, `originalTitle`, etc.
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19013
Summary:
Depends on D19009. Ref T13053. For "Must Encrypt" mail, we must currently strip the "Thread-Topic" header because it sometimes contains sensitive information about the object.
I don't actually know if this header is useful or anyting uses it. My understanding is that it's an Outlook/Exchange thing, but we also implement "Thread-Index" which I think is what Outlook/Exchange actually look at. This header may have done something before we implemented "Thread-Index", or maybe never done anything. Or maybe older versions of Excel/Outlook did something with it and newer versions don't, or do less. So it's possible that an even better fix here would be to simply remove this, but I wasn't able to convince myself of that after Googling for 10 minutes and I don't think it's worth hours of installing Exchange/Outlook to figure out. Instead, I'm just trying to simplify our handling of this header for now, and maybe some day we'll learn more about Exchange/Outlook and can remove it.
In a number of cases we already use the object monogram or PHID as a "Thread-Topic" without users ever complaining, so I think that if this header is useful it probably isn't shown to users, or isn't shown very often (e.g., only in a specific "conversation" sub-view?). Just use the object PHID (which should be unique and stable) as a thread-topic, everywhere, automatically.
Then allow this header through for "Must Encrypt" mail.
Test Plan: Processed some local mail, saw object PHIDs for "Thread-Topic" headers.
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19012
Summary:
Ref T13053. Adds revision stamps (status, reviewers, etc). Adds Herald rule stamps, like the existing X-Herald-Rules header.
Removes the "self" stamps, since you can just write a rule against `whatever(@epriestley)` equivalently. If there's routing logic around this, it can live in the routing layer. This avoids tons of self-actor, self-mention, self-reviewer, self-blocking-reviewer, self-resigned-reviewer, etc., stamps.
Use `natcasesort()` instead of `sort()` so that numeric values (like monograms) sort `9, 80, 700` instead of `700, 80, 9`.
Remove the commas from rendering since they don't really add anything.
Test Plan: Edited tasks and revisions, looked at mail stamps, saw stamps that looked pretty reasonable (with no more self stuff, no more commas, sorting numbers, and Herald stamps).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D18997
Summary:
Ref T13057. This makes "reverts" syntax more visible and useful. In particular, you can now `Reverts Dxx` in a revision or commit, and `Reverts <hash>` from a revision.
When you do, the corresponding object will get a more-visible cross-reference marker in its timeline:
{F5405517}
From here, we can look at surfacing revert information more heavily, since we can now query it on revision/commit pages via edges.
Test Plan: Used "reverts <hash>" and "reverts <revision>" in Differential and Diffusion, got sensible results in the timeline.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13057
Differential Revision: https://secure.phabricator.com/D18978
Summary:
Ref T13051. This puts a translation layer between the raw edge data in the transaction table and the UI that uses it.
The intent is to start writing new, more compact data soon. This class give us a consistent API for interacting with either the new or old data format, so we don't have to migrate everything upfront.
Test Plan: Browsed around, saw existing edge transactions render properly in transactions and feed. Added and removed subscribers and projects, saw good transaction rendering.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13051
Differential Revision: https://secure.phabricator.com/D18946
Summary:
See PHI307. Currently, when reviews undraft, we retroactively add in older activity to the mail ("alice created this revision...").
However, we don't add that activity to the mail tags, so the relevant tags (like "revision created") are dropped forever.
Instead, use the same set of transactions for both mail body and mail tag construction.
This should be obsoleted in the relatively near future by T10448, but it's a better/more correct behavior in general and we probably can't get rid of tags completely for a while.
Test Plan:
Applied patch, created a revision with builds, saw it auto-undraft after builds finished. Used `bin/mail list-outbound` and `bin/mail show-outbound` to see the mail. Verified that it included retroactive text ("created this revision") AND retroactive tags.
Note that the tag for "A new revision is created" is `DifferentialTransaction::MAILTAG_REVIEW_REQUEST` with literal value `differential-review-request`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18941
Summary: See PHI280. We have a similar field for tasks already, this is generally a reasonable sort of thing to support, and the addition of "draft" states means there are some pretty reasonable use cases.
Test Plan:
- Wrote a status-based ("status is needs revision") Herald rule.
- Tested it against a "Needs Revision" revision (passed) and a "Changes Planned" revision (failed).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18938
Summary:
Ref T13047. In some reasonable cases, knowing the path and line number where a symbol appears is useful in ranking or filtering the set of matching symbols.
Giving symbol sources more information can't hurt, and it's generally free for us to include this context since we just need to grab it out of the document and pass it along.
We can't always get this data (for example, if a user types `s idx` into global search, we have no clue) but this is similar to other types of context which are only available sometimes (like which repository a symbol appears in).
Test Plan: Command-clicked some symbols in 1-up (unified) and 2-up (side-by-side) diff views with symbol indexes configured. Got accurate path and line information in the URI I was redirected to.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18936
Summary:
Depends on D18924. Ref T13048. Each adapter defines which repetition options ("every time", "only the first time") users may select for rules.
Currently, this is all explicit and hard-coded. However, every adapter really just implements this rule (except for some bugs, see below):
> You can pick "only the first time" if this adapter fires more than once on the same object.
Since we already have a `isSingleEventAdapter()` method which lets us tell if an adapter fires more than once, just write this rule in the base class and delete all the copy/pasting.
This also fixes two bugs because of the copy/pasting: Pholio Mocks and Phriction Documents did not allow you to write "only the first time" rules. There's no reason for this, they just didn't copy/paste enough methods when they were implemented.
This will make a future diff (which introduces an "if the rule did not match last time" policy) cleaner.
Test Plan:
- Checked several different types of rules, saw appropriate options in the dropdown (pre-commit: no options; tasks: first or every).
- Checked mocks and wiki docs, saw that you can now write "only the first time" rules.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18925
Summary:
See PHI309. There is a window of time between when all builds pass and when Harbormaster actually publishes a revision out of draft.
If any other user tries to interact with the revision during that window, they'll pick up the undraft transaction as a side effect. However, they won't have permission to apply it and will be stopped by a validation error.
Instead, only automatically publish a revision if the actor is the revision author or some system/application user (essentially always Harbormaster).
Test Plan:
- Added a `echo ...; sleep(30);` to `HarbormasterBuildEngine->updateBuildable()` before the `applyTransactions()` at the bottom.
- Wrote an "Always, run an HTTP request" Herald rule and Harbormaster build plan.
- Ran daemons with `bin/phd debug task`.
- Created a new revision with `arc diff`, as user A.
- Waited for `phd` to enter the race window.
- In a separate browser, as user B, submitted a comment via `differential.revision.edit`.
- Before patch: edits during the race window were rejected with a validation error, "you don't have permission to request review".
- After patch: edits go through cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18921
Summary:
Ref T13045. See that task for discussion.
This replaces `digestForIndex()` with a "clever" algorithm in `digestForAnchor()`. The new digest is the same as `digestForIndex()` except when the original output was "." or "_". In those cases, a replacement character is selected based on entropy accumulated by the digest function as it iterates through the string.
Test Plan: Added unit tests.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13045
Differential Revision: https://secure.phabricator.com/D18909
Summary: See PHI292. This is just a generalization of D18851: feed stories have the same issue as mail. Don't hide "requested a review" in either mail or feed.
Test Plan:
- Enable prototypes.
- No harbormaster builds.
- Create a revision.
- Pre-patch: no feed story.
- Post-patch: feed story.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18886
Summary:
Ref T13042. This is a very, very old policy-violating option from yesteryear which supported build systems publishing updates by adding comments to revisions, without sending email about it.
Harbormaster has served this role for a long time and this is policy-violating in the general case (it allows attackers to act in secret).
Test Plan: Grepped for affected symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13042
Differential Revision: https://secure.phabricator.com/D18881
Summary: Ref T13035. See that task for a description of the issue.
Test Plan:
- Enabled prototypes.
- Disabled all Herald rules that trigger Harbormaster builds.
- Created a new revision.
- Before patch: initial review request email was dropped.
- After patch: initial review request email is sent.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13035
Differential Revision: https://secure.phabricator.com/D18851
Summary:
See PHI230. Currently, we denormalize raw line counts onto diffs and revisions, but not added/removed line counts.
I'd like to try a `[---+ ]` sort of size hint element (see D16322 for more) as a general approach to conveying size information at a glance and see how it feels, since I think the raw size number isn't very scannable/useful and it may be a significant improvement to hint about how much of a change is throwing stuff out vs adding new stuff.
This just makes the data available without any subquerying and doesn't actually change the UI.
Test Plan:
Created a revision, saw detailed change information populate in the database.
```
mysql> select * from differential_revision where id = 292\G
*************************** 1. row ***************************
id: 292
title: WIP
originalTitle: WIP
phid: PHID-DREV-ux3cxptibn3l5pxsug3z
status: draft
summary: asdf
testPlan: asdf
authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
lastReviewerPHID: NULL
lineCount: 41
dateCreated: 1513179418
dateModified: 1513179418
attached: []
mailKey: h4mn6perdio47o4beomyvu75zezwvredx3mbrlgz
branchName: NULL
viewPolicy: users
editPolicy: users
repositoryPHID: PHID-REPO-wif5lutk5gn3y6ursk4p
properties: {"lines.added":40,"lines.removed":1}
activeDiffPHID: PHID-DIFF-ixjphpunpkenqgukpmce
1 row in set (0.00 sec)
```
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18832
Summary:
Fixes T13027. Ref T2543. When revisions promote from "Draft" because builds finish or no builds are configured, the status currently switches from "Draft" to "Needs Review" without re-running Herald.
This means that some rules -- notably, "Send me an email" rules -- don't fire as soon as they should.
Instead of applying this promotion in a hacky way inline, queue it and apply it normally in a second edit, after the current group finishes.
Test Plan:
- Created a revision, reviewed Herald transcripts.
- Saw three Herald passes:
- First pass (revision creation) triggered builds and no email.
- Second pass (builds finished) did not trigger builds (no update) and did not trigger email (revision still a draft).
- Third pass (after promotion out of 'draft') did not trigger builds (no update) but did trigger email (revision no longer a draft).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13027, T2543
Differential Revision: https://secure.phabricator.com/D18819
Summary:
Ref T10233. See PHI231. When users ignore the `arc land` prompt about bad revision states, make it explicitly clear in the transaction log that they broke the rules.
You can currently figure this out by noticing that there's no "This revision is accepted and ready to land." message, but it's unrealistic to expect non-expert users to look for the //absence// of a message to indicate something, and this state change is often relevant.
Test Plan: {F5302351}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10233
Differential Revision: https://secure.phabricator.com/D18808
Summary:
Depends on D18801. Ref T2543. See PHI229. I missed "Accept" before, but intended to disallow it (like "Reject") since I don't want drafts to be reviewable.
However, "Resign" seems fine to allow? So let's allow that for now.
Test Plan: Was no longer offered "Accept" on draft revisions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18802
Summary:
See PHI228. Ref T2543. The current logic gets this slightly wrong: prototypes are off, you create a draft with `--draft`, then promote it with "Request Review". This misses both branches.
Instead, test these conditions a little more broadly. We also need to store broadcast state since `getIsNewObject()` isn't good enough with this workflow.
Test Plan:
- With prototypes on and autopromotion, got a rich email after builds finished.
- With prototypes off, got a rich email immediately.
- With prototypes off and `--draft`, got a rich email after "Request Review".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18801
Summary: Whitespace has semantic meaning for yaml files, so we shouldn't suppress whitespace-only lines of diff by default.
Test Plan: Edited local config to include yaml files, saw expected whitespace changes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18775
Summary:
See PHI180. Currently, if you begin creating or editing an inline and then swap display modes (for example, with "View Unified"), your edit is lost.
Persisting the editor state is complicated and this is very rare, so just prevent the action and warn the user instead.
Also make the warning persist for a little longer since a few of the messages, including this one, take a couple seconds to read now.
Test Plan:
- Edited a comment, tried to swap display modes, got a warning.
- Swapped display modes normally with no comment being edited.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18774
Summary:
Depends on D18771. See PHI206. Currently, `arc diff --draft` only holds revisions in draft mode: it doesn't put them into draft mode if the install isn't configured to use draft mode.
Instead, make it a bit more forceful so that `arc diff --draft` can create into draft mode explicitly even if protoypes are off. This aligns with expection a little more clearly.
Test Plan: Ran `arc diff --draft` with prototypes off, got a revision held in draft mode.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18772
Summary: See PHI210. Ref T2543. Currently, we don't set this flag if you have prototypes off and don't get any of the new draft stuff, so the mail drops some of the details it is supposed to have.
Test Plan: Disabled prototypes, created a revision, saw summary / test plan in the initial mail.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18771
Summary:
See PHI199. Ref T2543. When you run a RevisionQuery with a legacy status constraint (via `differential.query`), we currently don't match "Draft" revisions.
Use the actual complete map from `DifferentialRevisionStatus` instead of hard coding the status list so "Draft" is included.
Test Plan:
- Ran `differential.query` with `ids` and `status` for a draft revision.
- Before patch: revision not returned in results.
- After patch: revision returned in results.
(Note that it returns as "Needs Review", for compatibility.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18765
Summary:
See PHI190. This clarifies the ruleset a bit:
- If you accepted, then the author used "Request Review" explicitly, we now show "Accepted Earlier" instead of "Accepted" in the "Reviewers" list on the main revision page. This makes it sligthly more clear why the revision is back in your review queue without picking through the transaction log.
- Instead of moving all non-current accepts into "Ready to Review", move only voided accepts into "Ready to Review". This stops us from pulling older accepts which haven't been voided (which could have been incorrectly pulled) and correctly pulls older, voided accepts from before an update (for example: accept, then request review, then update) and generally aligns better with intent/expectation.
Test Plan:
- Accepted, requested review.
- Saw reviewer as "Accepted Earlier".
- Saw review in "Ready to Review" bucket.
- Accepted, updated (with sticky accept).
- Saw reviewer as "Accepted Prior Diff".
- Saw review as "Waiting on Authors".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18764
Summary: See PHI195. This bulks out these API methods since all the requests are pretty straightforward.
Test Plan: Ran `edge.search` and `differential.revision.search`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18762
Summary:
See PHI197. Populates "status" transactions and a few other obvious types where there's no security/performance/payload/formatting issue I can come up with.
The names here are the same as the names for editing with `maniphest.edit`.
Test Plan: Used `transaction.search` to retrieve transactions of all new types.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18761
Summary: Ref T12689. See PHI178. This isn't a complete solution (you may still get mailed via packages/projects) but should fix the obvious issue, where "Resigned" reviewers are incorrectly always sent mail directly.
Test Plan: Had Alice resign, interacted as Bailey, no mail to Alice.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12689
Differential Revision: https://secure.phabricator.com/D18758
Summary:
Depends on D18756. Fixes T12539. See PHI190. Currently, when this occurs:
- Alice accepts.
- Bailey requests review.
- Alice views her dashboard.
...the revision appears in "Waiting on Other Reviewers" (regardless of whether other reviewers actually exist or not).
Instead, ignore these voided/non-current accepts and let the revisions appear in "Ready to Review", which is more natural.
Test Plan: Went through the steps above. On `master`, saw revision in "Waiting on Other Reviewers". After patch, saw it in "Ready to Review".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12539
Differential Revision: https://secure.phabricator.com/D18757
Summary:
Ref T12539. See PHI190. Currently, each Diff has a `revisionID`, but Revisions do not point at the current active diff. To find the active diff for a given revision, we need to issue a separate query.
Furthermore, this query is inefficient for bulk loads: if we have a lot of revisions, we end up querying for all diff IDs for all those revisions first, then selecting the largest ones and querying again to get the actual diff objects. This strategy could likely be optimized but the query is a mess in any case.
In several cases, it's useful to have the active diff PHID without needing to do a second query -- sometimes for convenience, and sometimes for performance.
T12539 is an example of such a case: it would be nice to refine the bucketing logic (which only depends on active diff PHIDs), but it feels bad to make the page heavier to do it.
For now, this is unused. I'll start using it to fix the bucketing issue, and then we can expand it gradually to address other performance/convenience issues.
Test Plan:
- Ran migrations, inspected database, saw sensible values.
- Created a new revision, saw a sensible database value.
- Updated an existing revision, saw database update properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12539
Differential Revision: https://secure.phabricator.com/D18756
Summary:
Ref T2543. Fixes two relatively minor things:
- When builds finish in Harbormaster, send mail "From" the author.
- Set the `firstBroadcast` flag so that initial mail picks up earlier history (notably, the "reviewers" line).
For now, I'm not setting `firstBroadcast` on explicit "Request Review" (but maybe we should), and not trying to deal with weird cases where you leave a bunch of comments on a draft. Those might be fine as-is or may get tweaked later.
Test Plan: Created a revision with Harbormaster builds, ran builds, saw initial email come "From" the right user with more metadata.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18748
Summary: Ref T2543. After D18731, Herald build rules run more often, but now incorrectly try to run builds when Diffusion closes a revision because a commit landed.
Test Plan: Made some mundane updates locally; this is tricky to test comprehensively locally so I'm mostly planning to just push it to `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18745
Summary:
Ref T2543. Instead of autosubmitting revisions to "Needs Review" when builds finish, allow them to be held in "Draft" indefinitely.
There's currently no UI for this. I plan to just expose it as `arc diff --draft` for now, in a followup change.
Test Plan:
- Created a revision (via Conduit) with "hold as draft", saw it hold as draft after builds finished.
- Created a revision (normally), saw it autosubmit after builds finished.
- Requested review of a "hold as draft" revision to kick it out of draft state.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18737
Summary:
Depends on D18740. Prepares `arc` to receive a `--draft` flag by letting us switch to "differential.revision.edit" instead of "differential.createrevision".
To "differential.revision.edit", we need a transaction list, but we can't automatically construct this list from a field map. Return the transaction list alongside the field map.
The next change uses this list (if available) to switch us to the modern API method.
Test Plan: Ran `arc diff` on the experiemntal branch with followup changes, got a new revision.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18741
Summary: See PHI90. For now, this only provides a limited amount of information, but should satisfy the use case in PHI90 and build toward a more complete version in the future.
Test Plan: Used new Conduit method to retrieve information about diffs.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18744
Summary:
Ref T2543. Fixes T10109.
Currently, Herald only runs in Differential when a change updates the diff. This is partly for historical reasons, and partly because we don't want to restart builds every time someone makes a comment. However, this behavior is inconsistent with other applications (which always trigger on any change), and occasionally confusing to users (in T10109, for example) or otherwise undesirable.
A similar issue is that T2543 has introduced a "Draft" state, where revisions don't send normal mail until builds finish. This interacts poorly with "Send me an email" rules (which shouldn't do anything here) and particularly with "Send me an email + only run these actions the first time the rule matches", since that might have an effect like "do nothing when the revision is created, then never anything again since you already did nothing once".
To navigate both of these issues, let objects tell Herald that certain actions (like mail or builds) are currently forbidden. If a rule uses a field or action which is currently forbidden, the whole rule automatically fails before it executes, but doesn't count toward "only the first time" as far as Herald's tracking of rule execution is concerned.
Then, forbid mail for draft revisions, and forbid builds for revisions which didn't just get updated. Forbidding mail fixes the issues with "Send me an email" that were created by the introduction of the draft state.
Finally, make Herald run on every revision update, not just substantive updates to the diff. This resolves T10109.
Test Plan:
Created revisions via the draft -> submit workflow, saw different transcripts. Here's a mail action being forbidden for a draft:
{F5237324}
Here's a build action being forbidden for a "mundane" update:
{F5237326}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T10109, T2543
Differential Revision: https://secure.phabricator.com/D18731
Summary:
Ref T2543. When Harbormaster finishes builds and promotes a draft revision to review, we currently publish "Harbormaster requested review of...".
Instead, attribute this action to the author, since that's more natural and more useful.
Test Plan: Promoted a diff locally, saw it attributed to me rather than Harbormaster.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18730
Summary: Ref T12680. Use the slightly sleeker construction from D18722 in Differential.
Test Plan: Viewed revision list, reordered by date modified.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12680
Differential Revision: https://secure.phabricator.com/D18727
Summary:
Ref T2543. When a revision is created, we check if any builds are waiting/failed, and submit it for review immediately if we aren't waiting for anything.
In doing this, we ignore builds with only autotargets, since these are client-side and failures from local `arc lint` / `arc unit` should not count (the user has already chosen to ignore/skip them).
The way we do this has some issues:
- Herald may have started builds, but they may still be PENDING and not have any targets yet. In this case, we'll see "no non-autotargets" and ignore the build, which is wrong.
- We have to load targets but don't really care about them, which is more work than we really need to do.
- And it's kind of complex, too.
Instead, just let `BuildQuery` filter out "autobuilds" (builds generated from autoplans) with a JOIN.
Test Plan: Ran `arc diff` with builds configured, got a clean "Draft" state instead of an incorrect promotion directly to "Needs Review".
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18721
Summary: Ref T2543. When revisions are in the draft state, tell the user what we're waiting for or why they aren't moving forward.
Test Plan: {F5228840}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18714
Summary: Ref T2543. This is a less ambitious version of the rule in D18628, which I backed off from, since I think this probably still has a fair number of loose ends to tie up.
Test Plan: Created a revision locally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18713
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary:
Ref T13000. This marks each table as either "data" (normal data), "cache" (automatically rebuilt, no need to ever dump) or "index" (can be manually rebuilt).
By default, `bin/storage dump` dumps data and index tables, but not cache tables.
With `--no-indexes`, it dumps only data tables. Indexes can be rebuilt after a restore with `bin/search index --all ...`.
Test Plan:
- Ran `--no-indexes` and normal dumps with `--trace`, verified that cache and index (former case) or cache only (latter case) tables were dumped with `--no-data`.
- Verified dump has the same number of `CREATE TABLE` statements as before the changes.
- Reviewed persistence tags in the web UI (note Ferret engine tables are "Index"):
{F5210886}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18682
Summary:
Ref PHI91. When Owners (or Herald, or manual user action) adds package reviewers to a revision, later updates to the revision make some of them less relevant or irrelevant.
Provide a hint when a package reviewer doesn't own any of the paths that a diff changes. Humans can then decide if the reviewer is obsolete/irrelevant or not.
This is a rough cut to get the feature working, design could probably use some tweaking if it sticks.
Test Plan: {F5204309}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jboning
Differential Revision: https://secure.phabricator.com/D18663
Summary:
Ref T2543. This doesn't stand alone since mail still goes out normally, but gets this piece working: new revisions start as "Draft", then after updates if there are no builds they go into "Needs Review".
This should work in general because builds update revisions when they complete, to publish a "Harbormaster finished build yada yada" transaction. So either we'll un-draft immediately, or un-draft after the last build finishes.
I'll hold this until the mail and some other stuff (like UI hints) are in slightly better shape since I think it's probably too rough on its own.
Test Plan: Created revisions locally, saw them un-draft after builds.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18628
Summary:
Ref T2543. Currently, we always do some special things when a revision is created, mostly adding more stuff to the mail.
With drafts, we want to suppress initial mail and send this big, rich mail only when the revision actually moves out of "draft".
Prepare the code for this, with the actual methods hard-coded to the current behavior. This will probably take some tweaking but I think I got most of it.
Test Plan: Banged around in Differential so it sent some mail, saw normal mail without anything new.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18627
Summary:
Ref T2543. Most actions are not available for drafts.
Authors can "Request Review" (move out of draft to become a normal revision) or "Abandon".
Non-authors can't do anything (maybe we'll let them do something later -- like "Commandeer"? -- if there's a good reason).
Test Plan: Viewed a draft revision as an author and non-author, saw fewer actions available.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18626
Summary:
Ref T2543. There's no way to put revisions into this state yet, but start adding support for when there is.
Adds the status constant, plus support for bucketing them.
Test Plan:
- Manually put a revision in "Draft" state by updating the database directly.
- Verified my drafts showed up in a "Drafts" section on the bucket view.
- Verified others' drafts did not appear on the action bucket view.
- Viewed revisions, queried for "Draft" revisions, etc (stuff we get for free).
{F5186781}
{F5186782}
{F5186783}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18625
Summary:
Fixes T12986. I caught this bug in the changes from D18584: when we moved a large hunk to file storage, we would decompress it but keep the "deflated" flag. This could cause confusion when loading it later. I missed this in testing since I wasn't exhaustive enough in checking hunks and didn't run into a compressed one.
Instead of compressing on `save()`, compress during the normal workflow.
We currently never advise users to run this workflow so I didn't bother trying to clean up possible existing migrations.
Test Plan:
- Ran `bin/differential migrate-hunk` on compressed hunks, moving them to and from file storage. Saw them work correctly and remain compressed.
- Created new small (uncompressed) and large (compressed) hunks, verified they work properly and get compressed (if applicable).
- Used `bin/cache purge --caches changeset` to clear changeset caches and make sure the actual table was being hit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12986
Differential Revision: https://secure.phabricator.com/D18624
Summary:
See PHI71. These didn't get properly updated when we wrote Subprojects and Milestones, and should use materialized members, not raw members. Swap the query so projects you are an indirect member of (e.g., milestones you are a member of the parent for, and parent projects you are a member of a subproject of) are included in the result list.
Also fix a bad typeahead datasource.
Test Plan:
- Ran a dry run with the test console, saw project PHIDs for milestones and parent projects in the raw field value.
- Tried to set "Author's projects" to a user, no longer could.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18619
Summary:
Ref T11823. This is the meaty part of the change, and updates `RefEngine` to use separate RefCursor (for names) and RefPosition (for actual commit positions) tables.
I'll hold this whole series until after the release cut so it has some time to bake on `secure` to look for issues. It's also not a huge problem if there are bugs here since these tables are just caches anyway, although they do feed into some other things, and obviously it's never good to have bugs.
Test Plan:
- This logic can be invoked directly with `bin/repository refs <repository> --trace --verbose`.
- Ran that on unchanged repositories, new branches, removed branches, and modified branches. Saw appropriate output and cursor positions.
- Ran on a mercurial repository to test the close/open logic, saw it correct open/closed state of incorrect positions.
- Browed around Diffusion in various repositories.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18614
Summary:
Ref T11823. See PHI68. T11823 has a full description of this issue and a plan to fix it, but the full plan is relatively complicated.
Until that can happen, provide a workaround for the biggest immediate issue, where multiple copies of a ref cursor can cause `executeOne()` to throw, since it expects a single result. In practice, these copies are always identical so we can just pick the first one.
This will get cleaned up once T11823 is fixed properly.
Test Plan:
Forced the table into a duplicate/ambiguous state, reproduced a similar-looking error:
{F5180999}
Applied the patch, got the "Land" to work as expected:
{F5181000}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18599
Summary:
Ref T12033. This is a very narrow fix for this issue, but it should fix the major error: don't attach patches if they're bigger than the mail body limit (by default, 512KB).
Specifically, the logs from an install in T12033 show a 112MB patch being attached, and that's the biggest practical problem here.
I'll follow up on the tasks with more nuanced future work.
Test Plan: Enabled `differential.attach-patches`, saw a patch attached to email. Set the byte limit very low, saw patches get thrown away.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12033
Differential Revision: https://secure.phabricator.com/D18598
Summary:
Ref T12932. For long-lived installs, one of the largest tables tends to be the hunk data table. Although it doesn't grow tremendously fast, it's also well suited to storage in Files instead of the database (infrequent access, relatively large blobs of data, mostly one-at-a-time access), and earlier work anticipated eventually adding support for Files storage.
Make Files storage work, and provide `bin/differential migrate-hunk` to manually test/migrate hunks. This is currently the only way hunks get moved to file storage, but I expect to add a GC step which moves them to File storage after 30 days shortly.
The immediate motivation for this is to relieve storage pressure on db001/db002 so we have more headroom for deploying the Ferret engine and its larger indexes (see also T12819).
Test Plan:
- Used `bin/differential migrate-hunk` to move a hunk to and from file storage, verified it survived intact.
- Downloaded the actual stored file, sanity-checked it. Verified permissions.
- Destroyed a diff with `bin/remove destroy`, saw the hunk and file storage destroyed.
- Verified that going from file -> text destroys the old file properly with `migrate-hunk --trace ...`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12932
Differential Revision: https://secure.phabricator.com/D18584
Summary:
See brief discussion in D18554. All the index tables are the same for every application (and, at this point, seem unlikely to change) and we never actually pass these objects around (they're only used internally).
In some other cases (like Transactions) not every application has the same tables (for example, Differential has extra field for inline comments), and/or we pass the objects around (lots of stuff uses `$xactions` directly).
However, in this case, and in Edges, we don't interact with any representation of the database state directly in much of the code, and it doesn't change from application to application.
Just automatically define document, field, and ngram tables for anything which implements `FerretInterface`. This makes the query and index logic a tiny bit messier but lets us delete a ton of boilerplate classes.
Test Plan: Indexed objects, searched for objects. Same results as before with much less code. Ran `bin/storage upgrade`, got a clean bill of health.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18559
Summary:
Ref T12819. Currently, under the Ferret engine, we query each application's index separately and then aggregate the results.
At the moment, results are aggregated by type first, then by actual rank. For example, all the revisions appear first, then all the tasks.
Instead, surface the internal ranking data from the underlying query and sort by it.
Test Plan: Searched for "A B" with a task named "A B" and a revision named "A". Saw task first. Broadly, saw mixed task and revision order in result sets.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18551
Summary:
Ref T12819. I started trying to get individual engines to drive these constraints (e.g., `ManiphestTaskQuery` can do most of the work) but this is a big pain, especially since most engines don't support "any owner" or "no owner", and not everything has an owner, and so on and so on. Going down this path would have meant a huge pile of stub functions everywhere, I think.
Instead, drive these through the main engine using the fulltext document table, which already has everything we need to apply these constraints in a uniform way.
Also tweak some parts of query construction and result ordering.
Test Plan: Searched for documents by author, owner, unowned, any owner, tags, subscribers, fulltext in global search. Got sensible results without any application-specific code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18550
Summary:
Ref T12819. Provides a Ferret-engine-based fulltext engine to ultimately replace the InnoDB fulltext engine.
This is still pretty basic (hard-coded and buggy) but technically sort of works.
To activate this, you must explicitly configure it, so it isn't visible to users yet.
Test Plan: Searched for objects with global fulltext search, got a mixture of matching revisions and tasks back.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18548
Summary:
Fixes T12118. See PHI54. This adds a special case for the initial "reviewers" transactions, similar to the existing special case for "projects" transactions.
Although these transactions are redudnant in the web view since you can see the information clearly on the page, they're more reasonably useful in mail.
Test Plan: {F5168838}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12118
Differential Revision: https://secure.phabricator.com/D18542
Summary: Create a diff page, new UI
Test Plan: Create a diff from page
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18529
Summary: Adds a `MenuName` method to applications that `ProfileMenuItem` uses instead of the application name if set. This improves the home/menu/new user experience at little cost. Also renamed the label from Applications to Favorites, since this menu gets altered to provide more than just applications. This also allows instances to set back to Maniphest if they so choose. Overall I think this direction resolves 95% of my concerns, with maybe a small potential downside which I don't really anticipate. We already name Dashboard panels by their object, and that hasn't really caused confusion. I think these links are similar. I click 'Tasks' and get presented a list of my tasks from Maniphest.
Test Plan: Review each of the name changes as a default new install and a modified install.
Reviewers: epriestley, amckinley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18524
Summary: Ref T12819. Adds storage and indexing for the Ferret engine to Differential.
Test Plan: Ran `bin/search index D123 --force`, saw indexes appear in database. No UI/user impact yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18540
Summary:
Ref T2543. The type on these got changed by accident, it should be "string" (crazy nonsense, compatible) not "int" (sensible, not compatible).
(New API uses sensible strings like "accepted" only.)
Test Plan: Called `differential.query` from web UI, saw `"2"` and similar statuses.
Reviewers: chad, jmeador, lvital
Reviewed By: jmeador, lvital
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18493
Summary:
Ref T2543. When called from the UI to build the dropdown, there's no Editor, since we aren't actually in an edit flow.
This logic worked for actually performing the edits, just not for getting the option into the dropdown.
Test Plan: Used the dropdown to close an "Accepted" revision which I authored.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18490
Summary: These come out of the database as strings (see T12678), force them to integers for the API.
Test Plan: Called `transaction.search`, got integers in JSON instead of strings.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18476
Summary:
Ref T5873. This provides paths and line numbers for inline comments.
This is a touch hacky but I was able to keep it mostly under control.
Test Plan:
- Made inline comments.
- Called API, got path/line information.
{F5120157}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5873
Differential Revision: https://secure.phabricator.com/D18469
