1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 05:02:44 +01:00
Commit graph

16701 commits

Author SHA1 Message Date
epriestley
c116deef63 Remove "Effective User" attachment from Repository Identities
Summary:
See <https://discourse.phabricator-community.org/t/phabricatorrepositoryidentity-attacheffectiveuser-must-be-an-instance-of-phabricatoruser-null-given/1820/3>.

It's possible for an Idenitity to be bound to user X, then for that user to be deleted, e.g. with `bin/remove destroy X`. In this case, we'll fail to load/attach the user to the identity.

Currently, we don't actually use this anywhere, so just stop loading it. Once we start using it (if we ever do), we could figure out what an appropriate policy is in this case.

Test Plan: Browsed around Diffusion, grepped for affected "effective user" methods and found no callsites.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20224
2019-03-05 11:35:12 -08:00
epriestley
cc8dda6299 Recognize the official "Go" magic regexp for generated code as generated
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
2019-03-05 11:34:11 -08:00
epriestley
920ab13cfb Correct a possible fatal in the non-CSRF Duo MFA workflow
Summary:
Ref T13259. If we miss the separate CSRF step in Duo and proceed directly to prompting, we may fail to build a response which turns into a real control and fatal on `null->setLabel()`.

Instead, let MFA providers customize their "bare prompt dialog" response, then make Duo use the same "you have an outstanding request" response for the CSRF and no-CSRF workflows.

Test Plan: Hit Duo auth on a non-CSRF workflow (e.g., edit an MFA provider with Duo enabled). Previously: `setLabel()` fatal. After patch: smooth sailing.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13259

Differential Revision: https://secure.phabricator.com/D20234
2019-03-05 11:33:25 -08:00
epriestley
d192d04586 Make it more visually clear that you can click things in the "Big List of Clickable Things" UI element
Summary:
Ref T13259. An install provided feedback that it wasn't obvious you could click the buttons in this UI.

Make it more clear that these are clickable buttons.

Test Plan:
{F6251585}

{F6251586}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13259

Differential Revision: https://secure.phabricator.com/D20238
2019-03-05 11:32:38 -08:00
epriestley
ee2bc07c90 No-op old search indexing migrations which no longer run and have been obsoleted by upgrade "activities"
Summary:
See T13253. After D20200 (which changed the task schema) these migrations no longer run, since the PHP code will expect a column to exist that won't exist until a `20190220.` migration runs.

We don't need these migrations, since anyone upgrading through September 2017 gets a "rebuild search indexes" activity anyway (see T11932). Just no-op them.

Test Plan: Grepped for `queueDocumentForIndexing()` in `autopatches/`, removed all of it.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D20243
2019-03-05 11:31:49 -08:00
epriestley
34e90d8f51 Clean up a few "%Q" stragglers in SVN repository browsing code
Summary: See <https://discourse.phabricator-community.org/t/unsafe-raw-string-warning-for-svn-in-diffusion/2469>.

Test Plan: Browed a Subversion repository in Diffusion. These are all reachable from the main landing page if the repository has commits/files, I think. Before change: errors in log; after change: no issues.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20244
2019-03-05 11:30:55 -08:00
epriestley
9b0b50fbf4 Give "bin/worker" flags to repeat and retry tasks
Summary:
See PHI1063. See PHI1114. Ref T13253. Currently, you can't `bin/worker execute` an archived task and can't `bin/worker retry` a successful task.

Although it's good not to do these things by default (particularly, retrying a successful task will double its effects), there are plenty of cases where you want to re-run something for testing/development/debugging and don't care that the effect will repeat (you're in a dev environment, the effect doesn't matter, etc).

Test Plan: Ran `bin/worker execute/retry` against archived/successful tasks. Got prompted to add more flags, then got re-execution.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13253

Differential Revision: https://secure.phabricator.com/D20246
2019-03-05 11:30:16 -08:00
epriestley
e15fff00a6 Use "LogLevel=ERROR" to try to improve "ssh" hostkey behavior without doing anything extreme/hacky
Summary:
Ref T13121. When you connect to a host with SSH, don't already know the host key, and don't have strict host key checking, it prints "Permanently adding host X to known hosts". This is super un-useful.

In a perfect world, we'd probably always have strict host key checking, but this is a significant barrier to configuration/setup and I think not hugely important (MITM attacks against SSH hosts are hard/rare and probably not hugely valuable). I'd imagine a more realistic long term approach is likely optional host key checking.

For now, try using `LogLevel=ERROR` instead of `LogLevel=quiet` to suppress this error. This should be strictly better (since at least some messages we want to see are ERROR or better), although it may not be perfect (there may be other INFO messages we would still like to see).

Test Plan:
  - Ran `ssh -o LogLevel=... -o 'StrictHostKeyChecking=no' -o 'UserKnownHostsFile=/dev/null'` with bad credentials, for "ERROR", "quiet", and default ("INFO") log levels.
  - With `INFO`, got a warning about adding the key, then an error about bad credentials (bad: don't want the key warning).
  - With `quiet`, got nothing (bad: we want the credential error).
  - With `ERROR`, got no warning but did get an error (good!).

Not sure this always gives us exactly what we want, but it seems like an improvement over "quiet".

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13121

Differential Revision: https://secure.phabricator.com/D20240
2019-03-04 09:38:00 -08:00
epriestley
bfe8f43f1a Use "QUERY_STRING", not "REQUEST_URI", to parse raw request parameters
Summary:
Fixes T13260. "QUERY_STRING" and "REQUEST_URI" are similar for our purposes here, but our nginx documentation tells you to pass "QUERY_STRING" and doesn't tell you to pass "REQUEST_URI". We also use "QUERY_STRING" in a couple of other places already, and already have a setup check for it.

Use "QUERY_STRING" instead of "REQUEST_URI".

Test Plan: Visited `/oauth/google/?a=b`, got redirected with parameters preserved.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13260

Differential Revision: https://secure.phabricator.com/D20227
2019-02-28 19:50:27 -08:00
epriestley
54006f4817 Stop "Mute Notifications" on Bulk Jobs from fataling
Summary:
See <https://discourse.phabricator-community.org/t/unhandled-exception-muting-completed-bulk-jobs/2449>. Bulk Jobs have an "edge" table but currently do not support edge transactions. Add support.

This stops "Mute Notifications" from fataling.

The action probably doesn't do what the reporting user expects (it stops edits to the job object from sending notifications; it does not stop the edits the job performs from sending notifications) but I think this change puts us in a better place no matter what, even if we eventually clarify or remove this behavior.

Test Plan: Clicked "Mute Notifications" on a bulk job, got an effect instead of a fatal.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20226
2019-02-28 19:49:57 -08:00
epriestley
27ea775fda Fix a log warning when searching for ranges on custom "Date" fields
Summary:
See <https://discourse.phabricator-community.org/t/traceback-rendering-task-query-in-dashboard/2450/>.

It looks like this blames to D19126, which added some more complex constraint logic but overlooked "range" constraints, which are handled separately.

Test Plan:
  - Added a custom "date" field to Maniphest with `"search": true`.
  - Executed a range query against the field.

Then:

  - Before: Warnings about undefined indexes in the log.
  - After: No such warnings.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: jbrownEP

Differential Revision: https://secure.phabricator.com/D20225
2019-02-28 19:49:18 -08:00
epriestley
75dfae1011 Don't require any special capabilities to apply a "closed a subtask" transaction to a parent task
Summary:
See PHI1059. If you close a task, we apply an "alice closed a subtask: X" transaction to its parents.

This transaction is purely informative, but currently requires `CAN_EDIT` permission after T13186. However, we'd prefer to post this transaction anyway, even if: the parent is locked; or the parent is not editable by the acting user.

Replace the implicit `CAN_EDIT` requirement with no requirement.

(This transaction is only applied internally (by closing a subtask) and can't be applied via the API or any other channel, so this doesn't let attackers spam a bunch of bogus subtask closures all over the place or anything.)

Test Plan:
  - Created a parent task A with subtask B.
  - Put task A into an "Edits Locked" status.
  - As a user other than the owner of A, closed B.

Then:

  - Before: Policy exception when trying to apply the "alice closed a subtask: B" transaction to A.
  - After: B closed, A got a transaction despite being locked.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20223
2019-02-28 19:48:28 -08:00
epriestley
4cc556b576 Clean up a PhutilURI "alter()" callsite in Diffusion blame
Summary: See <https://discourse.phabricator-community.org/t/exception-when-viewing-previous-revision-from-blame/2454/2>.

Test Plan: Viewed blame, clicked "Skip Past This Commit". Got jumped to the right place instead of a URI exception.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20222
2019-02-28 19:47:46 -08:00
epriestley
620047bcfe Add a "Recent Builds" element to the Build Plan UI and tighten up a few odds and ends
Summary:
Depends on D20218. Ref T13258. It's somewhat cumbersome to get from build plans to related builds but this is a reasonable thing to want to do, so make it a little easier.

Also clean up / standardize / hint a few things a little better.

Test Plan: {F6244116}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13258

Differential Revision: https://secure.phabricator.com/D20219
2019-02-28 07:38:37 -08:00
epriestley
8338f94057 Provide "harbormaster.buildplan.edit" in the API
Summary: Depends on D20217. Ref T13258. Mostly for completeness. You can't edit build steps so this may not be terribly useful, but you can do bulk policy edits or whatever?

Test Plan: Edited a build plan via API.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13258

Differential Revision: https://secure.phabricator.com/D20218
2019-02-28 07:36:31 -08:00
epriestley
f6ed873f17 Move Harbormaster Build Plans to modular transactions
Summary: Depends on D20216. Ref T13258. Bland infrastructure update to prepare for bigger things.

Test Plan: Created and edited a build plan.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13258

Differential Revision: https://secure.phabricator.com/D20217
2019-02-28 07:32:13 -08:00
epriestley
41c03bab39 Remove unusual "Created" element from Build Plan curtain UI
Summary: Ref T13088. Build Plans currently have a "Created" date in the right-hand "Curtain" UI, but this is unusual and the creation date is evident from the timeline. It's also not obvious why anyone would care. Remove it for simplicity/consistency. I think this may have just been a placeholder during initial implementation.

Test Plan: Viewed a build plan, no more "Created" element.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13088

Differential Revision: https://secure.phabricator.com/D20216
2019-02-28 07:31:21 -08:00
epriestley
b28b05342b Simplify one "array_keys/range" -> "phutil_is_natural_list()" in "phabricator/"
Summary: Depends on D20213. Simplify this idiom.

Test Plan: Squinted hard; `grep array_keys | grep range`.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20215
2019-02-28 07:29:36 -08:00
epriestley
814e6d2de9 Add more type checking to transactions queued by Herald
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
2019-02-28 07:10:56 -08:00
epriestley
dc9aaa0fc2 Fix a stray "%Q" warning when hiding/showing inline comments
Summary: See PHI1095.

Test Plan: Viewed a revision, toggled hide/show on inline comments. Before: warning in logs; after: smooth sailing.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20212
2019-02-25 13:51:09 -08:00
epriestley
d1546209c5 Expand documentation for "transaction.search"
Summary: Depends on D20209. Ref T13255. It would probably be nice to make this into a "real" `*.search` API method some day, but at least document the features for now.

Test Plan: Read documentation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13255

Differential Revision: https://secure.phabricator.com/D20211
2019-02-25 10:52:29 -08:00
epriestley
83aba7b01c Enrich the "change project tags" transaction in "transaction.search"
Summary:
Depends on D20208. Ref T13255. See that task for some long-winded discussion and rationale. Short version:

  - This is a list of operations instead of a list of old/new PHIDs because of scalability issues for large lists (T13056).
  - This is a fairly verbose list (instead of, for example, the more concise internal map we sometimes use with "+" and "-" as keys) to try to make the structure obvious and extensible in the future.
  - The "add" and "remove" echo the `*.edit` operations.

Test Plan: Called `transaction.search` on an object with project tag changes, saw them in the results.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13255

Differential Revision: https://secure.phabricator.com/D20209
2019-02-25 10:50:04 -08:00
epriestley
f61e825905 Make the Diffusion warning about "svnlook" and PATH more clear
Summary:
See <https://discourse.phabricator-community.org/t/display-error-on-the-status-page-for-svn-repos/2443> for discussion.

The UI currently shows a misleading warning that looks like "found svnlook; can't find svnlook".

It actually means "found svnlook, but when Subversion wipes PATH before executing commit hooks, we will no longer be able to find it".

Test Plan: {F6240967}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20210
2019-02-25 10:48:12 -08:00
epriestley
767afd1780 Support an "authorPHIDs" constraint for "transaction.search"
Summary: Ref T13255. The "transaction.search" API method currently does not support author constraints, but this is a reasonable thing to support.

Test Plan: Queried transactions by author, hit the error cases.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13255

Differential Revision: https://secure.phabricator.com/D20208
2019-02-25 06:33:04 -08:00
epriestley
66161feb13 Fix a URI construction exception when filtering the Maniphest Burnup chart by project
Summary: See <https://discourse.phabricator-community.org/t/filtering-burnup-rate-by-project-produces-exception/2442>.

Test Plan: Viewed Maniphest burnup chart, filtered by project: no more URI construction exception.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20207
2019-02-25 05:36:31 -08:00
Ariel Yang
01d0fc443a Fix a typo
Summary: Fix a type

Test Plan: No need.

Reviewers: #blessed_reviewers, amckinley

Reviewed By: #blessed_reviewers, amckinley

Subscribers: amckinley, epriestley

Differential Revision: https://secure.phabricator.com/D20203
2019-02-24 13:37:14 +00:00
epriestley
701a9bc339 Fix Facebook login on mobile violating CSP after form redirect
Summary: Fixes T13254. See that task for details.

Test Plan: Used iOS Simulator to do a login locally, didn't get blocked. Verified CSP includes "m.facebook.com".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13254

Differential Revision: https://secure.phabricator.com/D20206
2019-02-23 05:25:09 -08:00
epriestley
90064a350a Fix URI construction of typeahead browse "more" pager
Summary: Ref T13251. See <https://phabricator.wikimedia.org/T216849>.

Test Plan:
  - With more than 100 projects (or, set `$limit = 3`)...
  - Edit a task, then click the "Browse" magnifying glass icon next to the "Tags" typeahead.
  - Before change: fatal on 'q' being null.
  - After change: no fatal.

Reviewers: amckinley, 20after4

Reviewed By: amckinley

Maniphest Tasks: T13251

Differential Revision: https://secure.phabricator.com/D20204
2019-02-22 18:09:16 -08:00
Austin McKinley
abc26aa96f Track total time from task creation to task archival
Summary:
Ref T5401. Depends on D20201. Add timestamps to worker tasks to track task creation, and pass that through to archive tasks. This lets us measure the total time the task spent in the queue, not just the duration it was actually running.

Also displays this information in the daemon status console; see screenshot: {F6225726}

Test Plan:
Stopped daemons, ran `bin/search index --all --background` to create lots of tasks, restarted daemons, observed expected values for `dateCreated` and `epochArchived` in the archive worker table.

Also tested the changes to `unarchiveTask` by forcing a search task to permanently fail and then `bin/worker retry`ing it.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T5401

Differential Revision: https://secure.phabricator.com/D20200
2019-02-20 14:44:29 -08:00
epriestley
1b83256421 Don't enable the "ScopeEngine" or try to identify scope context for diffs without context
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
2019-02-20 10:16:20 -08:00
epriestley
f1a035d5c2 In Differential, give the "moved/copied from" gutter a more clear visual look
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
2019-02-20 10:12:16 -08:00
epriestley
a33409991c Remove an old Differential selection behavior
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
2019-02-20 10:09:34 -08:00
epriestley
c10b283b92 Remove some ancient daemon log code
Summary:
Ref T13253. Long ago, daemon logs were visible in the web UI. They were removed because access to logs generally does not conform to policy rules, and may leak the existence (and sometimes contents) of hidden objects, occasionally leak credentials in certain error messages, etc.

These bits and pieces were missed.

Test Plan: Grepped for removed symbols.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13253

Differential Revision: https://secure.phabricator.com/D20199
2019-02-20 09:52:11 -08:00
epriestley
cf048f4402 Tweak some display behaviors for indent indicators
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
2019-02-19 15:34:30 -08:00
epriestley
fe7047d12d Display some invisible/nonprintable characters in diffs by default
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
2019-02-19 15:21:44 -08:00
epriestley
efccd75ae3 Correct various minor diff copy behaviors
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
2019-02-19 15:18:45 -08:00
epriestley
37f12a05ea Behold! Copy text from either side of a diff!
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
2019-02-19 15:17:07 -08:00
epriestley
3ded5d3e8c Disable the JSHint "function called before it is defined" and "unused parameter" warnings
Summary:
Ref T12822. The next change hits these warnings but I think neither is a net positive.

The "function called before it is defined" error alerts on this kind of thing:

```
function a() {
  b();
}

function b() {
}

a();
```

Here, `b()` is called before it is defined. This code, as written, is completely safe. Although it's possible that this kind of construct may be unsafe, I think the number of programs where there's unsafe behavior here AND the whole thing doesn't immediately break when you run it at all is very very small.

Complying with this warning is sometimes impossible -- at least without cheating/restructuring/abuse -- for example, if you have two functions which are mutually recursive.

Although compliance is usually possible, it forces you to define all your small utility functions at the top of a behavior. This isn't always the most logical or comprehensible order.

I think we also have some older code which did `var a = function() { ... }` to escape this, which I think is just silly/confusing.

Bascially, this is almost always a false positive and I think it makes the code worse more often than it makes it better.

---

The "unused function parameter" error warns about this:

```
function onevent(e) {
  do_something();
```

We aren't using `e`, so this warning is correct. However, when the function is a callback (as here), I think it's generally good hygiene to include the callback parameters in the signature (`onresponse(response)`, `onevent(event)`, etc), even if you aren't using any/all of them. This is a useful hint to future editors that the function is a callback.

Although this //can// catch mistakes, I think this is also a situation where the number of cases where it catches a mistake and even the most cursory execution of the code doesn't catch the mistake is vanishingly small.

Test Plan: Egregiously violated both rules in the next diff. Before change: complaints. After change: no complaints.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12822

Differential Revision: https://secure.phabricator.com/D20190
2019-02-19 15:13:56 -08:00
epriestley
d4b96bcf6b Remove hidden zero-width spaces affecting copy behavior
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
2019-02-19 15:10:04 -08:00
epriestley
98fe8fae4a Use <td class="n" data-n="3"> instead of <th>3</th> for line numbers
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
2019-02-19 15:07:02 -08:00
epriestley
3f8eccdaec Put some whitespace behaviors back, but only for "diff alignment", not display
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
2019-02-19 13:11:50 -08:00
epriestley
5310f1cdd9 Remove all whitespace options/configuration everywhere
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
2019-02-19 13:09:29 -08:00
epriestley
661c758ff9 Render indent depth changes more clearly
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
2019-02-19 12:40:05 -08:00
epriestley
deea2f01f5 Allow unit tests to have arbitrarily long names (>255 characters)
Summary:
Depends on D20179. Ref T13088. See PHI351. See PHI1018. In various cases, unit tests names are 19 paths mashed together.

This is probably not an ideal name, and the test harness should probably pick a better name, but if users are fine with it and don't want to do the work to summarize on their own, accept them. We may summarize with "..." in some cases depending on how this fares in the UI.

The actual implementation is a separate "strings" table which is just `<hash-of-string, full-string>`. The unit message table can end up being mostly strings, so this should reduce storage requirements a bit.

For now, I'm not forcing a migration: new writes use the new table, existing rows retain the data. I plan to provide a migration tool, recommend migration, then force migration eventually.

Prior to that, I'm likely to move at least some other columns to use this table (e.g., lint names), since we have a lot of similar data (arbitrarily long user string constants that we are unlikely to need to search or filter).

Test Plan: {F6213819}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13088

Differential Revision: https://secure.phabricator.com/D20180
2019-02-19 11:21:42 -08:00
epriestley
312ba30714 Don't report search indexing errors to the daemon log except from "bin/search index"
Summary:
Depends on D20177. Fixes T12425. See <https://discourse.phabricator-community.org/t/importing-libphutil-repository-on-fresh-phabricator-triggers-an-error/2391/>.

Search indexing currently reports failures to load objects to the log. This log is noisy, not concerning, not actionable, and not locally debuggable (it depends on the reporting user's entire state).

I think one common, fully legitimate case of this is indexing temporary files: they may fully legitimately be deleted by the time the indexer runs.

Instead of sending these errors to the log, eat them. If users don't notice the indexes aren't working: no harm, no foul. If users do notice, we'll run or have them run `bin/search index` as a first diagnostic step anyway, which will now report an actionable/reproducible error.

Test Plan:
  - Faked errors in both cases (initial load, re-load inside the locked section).
  - Ran indexes in strict/non-strict mode.
  - Got exception reports from both branches in strict mode.
  - Got task success without errors in both cases in non-strict mode.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12425

Differential Revision: https://secure.phabricator.com/D20178
2019-02-19 11:17:11 -08:00
epriestley
aa470d2154 Show user availability dots (red = away, orange = busy) in typeaheads, tokenizer tokens, and autocompletes
Summary:
Ref T13249. See PHI810. We currently show availability dots in some interfaces (timeline, mentions) but not others (typeheads/tokenizers).

They're potentially quite useful in tokenizers, e.g. when assigning tasks to someone or requesting reviews. Show them in more places.

(The actual rendering here isn't terribly clean, and it would be great to try to unify all these various behaviors some day.)

Test Plan:
{F6212044}

{F6212045}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20173
2019-02-19 10:57:20 -08:00
epriestley
92abe3c8fb Extract scope line selection logic from the diff rendering engine so it can reasonably be iterated on
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
2019-02-19 10:55:10 -08:00
epriestley
8d348e2eeb Clean up a couple of %Q issues in "Has Parents" task queries
Summary: Stragglers from the great "%Q" migration.

Test Plan: Ran a query for tasks with parent tasks.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20183
2019-02-19 10:54:23 -08:00
epriestley
e44b40ca4d Make "Subscribe/Unsubscribe" require only "CAN_VIEW", not "CAN_INTERACT"
Summary:
Ref T13249. See PHI1059. Currently, Subscribe/Unsubscribe require CAN_INTERACT via the web UI and no permissions (i.e., effectively CAN_VIEW) via the API.

Weaken the requirements from the web UI so that you do not need "CAN_INTERACT". This is a product change to the effect that it's okay to subscribe/unsubscribe from anything you can see, even hard-locked tasks. This generally seems reasonable.

Increase the requirements for the actual transaction, which mostly applies to API changes:

  - To remove subscribers other than yourself, require CAN_EDIT.
  - To add subscribers other than yourself, require CAN_EDIT or CAN_INTERACT. You may have CAN_EDIT but not CAN_INTERACT on "soft locked" tasks. It's okay to click "Edit" on these, click "Yes, override lock", then remove subscribers other than yourself.

This technically plugs some weird, mostly theoretical holes in the API where "attackers" could sometimes make more subscription changes than they should have been able to. Now that we send you email when you're unsubscribed this could only really be used to be mildly mischievous, but no harm in making the policy enforcement more correct.

Test Plan: Against normal, soft-locked, and hard-locked tasks: subscribed, unsubscribed, added and removed subscribers, overrode locks, edited via API. Everything worked like it should and I couldn't find any combination of lock state, policy state, and edit pathway that did anything suspicious.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20174
2019-02-19 10:52:34 -08:00
epriestley
8cf6c68c95 Fix a PhutilURI issue in workboards
Ref PHI1082.
2019-02-19 09:01:10 -08:00