phabricator.git
11 months agoAdd a basic "harbormaster.step.edit" API method master
epriestley [Tue, 3 Nov 2020 20:22:16 +0000 (12:22 -0800)]
Add a basic "harbormaster.step.edit" API method

Summary: Ref T13585. Provide a minimal but technically functional "harbormaster.step.edit" API method.

Test Plan: Used the web console to modify the URI for a "Make HTTP Request" build step.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13585

Differential Revision: https://secure.phabricator.com/D21489

11 months agoAdd a basic "harbormaster.step.search" API method
epriestley [Tue, 3 Nov 2020 20:20:10 +0000 (12:20 -0800)]
Add a basic "harbormaster.step.search" API method

Summary: Ref T13585. This isn't particularly useful (notably, it does not include custom field values and isn't searchable by build plan PHID) but get the basics into place.

Test Plan: Used the web UI to make API calls, reviewed results.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13585

Differential Revision: https://secure.phabricator.com/D21488

11 months agoGuarantee terms in PhabricatorAuthPasswordEngine are strings
epriestley [Tue, 3 Nov 2020 18:32:26 +0000 (10:32 -0800)]
Guarantee terms in PhabricatorAuthPasswordEngine are strings

Summary:
Ref T2312. Numeric strings are read out of arrays as integers, and modern PHP raises appropriate warnings when they're then treated as strings.

For now, cast the keys to strings explicitly (we know we inserted only strings). In the future, introduction of a `StringMap` type or similar might be appropriate.

Test Plan:
  - Added "abc.12345.xyz" to the blocklist, changed my VCS password.
  - Before: fatal when trying to "strpos()" an integer.
  - After: password change worked correctly.

Maniphest Tasks: T2312

Differential Revision: https://secure.phabricator.com/D21487

11 months agoFix isValidGitShallowCloneResponse
epriestley [Fri, 30 Oct 2020 20:03:34 +0000 (13:03 -0700)]
Fix isValidGitShallowCloneResponse

Summary:
Changes the heuristic method by which non-zero exit statuses from git-http-backend are found to be due to packfile negotiation during shallow fetches, etc.

Instead of checking git-http-backend stderr for a generic "hung up" error message, see if the pack-result response contains a terminating flush packet ("0000"). This should give a greater assurance that the request was handled correctly and the response is complete.

Test Plan: Run `GIT_CURL_VERBOSE=1 git fetch --depth 1 https://host.example/source/repo.git HEAD` to ensure it completes and includes two successful POST requests during packfile negotiation (the last one actually receives the packfile).

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, dzduvall

Tags: #diffusion

Differential Revision: https://secure.phabricator.com/D21484

11 months agoAdd a missing "GROUP BY" to MailQuery when querying for multiple recipients
epriestley [Fri, 30 Oct 2020 19:34:19 +0000 (12:34 -0700)]
Add a missing "GROUP BY" to MailQuery when querying for multiple recipients

Summary:
See <https://discourse.phabricator-community.org/t/mail-details-view-broken/4315>. The change in D21400 detects a missing "GROUP BY" in some variations of this query.

Specifically, we may join multiple recipient rows (since mail may have multiple recipients) and then fail to group the results.

Fix this by adding the "GROUP BY". Additionally, remove the special-cased behavior when no authors or recipients are specified -- it's complicated and not entirely correct (e.g., may produce a "no object" instead of a policy error when querying by ID), and likely predates overheating.

Test Plan:
  - Disabled `metamta.one-mail-per-recipient` in Config.
  - Generated a message to 2+ recipients.
  - Viewed the message detail; queried for the message by specifying 2+ recipients.
  - Viewed the unfiltered list of messages, saw the query overheat.

Differential Revision: https://secure.phabricator.com/D21486

12 months agoWhen a new, deleted, draft inline is revived with "Undo", undelete it
epriestley [Mon, 19 Oct 2020 19:21:06 +0000 (12:21 -0700)]
When a new, deleted, draft inline is revived with "Undo", undelete it

Summary:
See PHI1876. Normally, deleted inlines are undeleted with an "undelete" operation, which clears the "isDeleted" flag.

However, when an inline is deleted implicitly by using "Cancel" without first saving it, the flag currently isn't cleared properly. This can lead to cases where inlines seem to vanish (they are shown to the user in the UI, but treated as deleted on submission).

Test Plan:
There are two affected sequences here:

  - Create a new inline, type text, cancel, undo.
  - Create a new inline, type text, cancel, undo, save.

The former sequence triggers an "edit" operation. The subsequent "Save" in the second sequence triggers a "save" operation.

It's normally impossible in the UI to execute a "save" without executing an "edit" first, but "save" clearly should undelete the comment if you get there somehow, so this change clears the deleted flag in both cases for completeness.

  - Executed both sequences, saw comment persist in preview, on reload, and after submission.

Differential Revision: https://secure.phabricator.com/D21483

12 months agoUpdate "arc call-conduit" instructions in Conduit API console for required "--"
epriestley [Mon, 19 Oct 2020 18:53:09 +0000 (11:53 -0700)]
Update "arc call-conduit" instructions in Conduit API console for required "--"

Summary: See PHI1912. Ref T13491. "arc" now requires "--" when stdin is not a TTY; provide this argument for users.

Test Plan: Viewed example in console, saw "--". Executed example.

Maniphest Tasks: T13491

Differential Revision: https://secure.phabricator.com/D21482

12 months agoSet an explicit height when drawing the dependent revision graph
epriestley [Fri, 16 Oct 2020 21:01:55 +0000 (14:01 -0700)]
Set an explicit height when drawing the dependent revision graph

Summary:
See PHI1900. Recent changes to how commit graphs are drawn made the height automatic in most cases, but it fails in Differential because the element isn't initially visible so the computed height is 0.

Just give them an explicit height so they show up again.

Test Plan: Viewed graphs in Maniphest, Differential, and Diffusion; saw them all render properly.

Differential Revision: https://secure.phabricator.com/D21481

12 months agoExpose the "file attached to object" and "object attached to file" edges via "edge...
epriestley [Fri, 16 Oct 2020 20:38:17 +0000 (13:38 -0700)]
Expose the "file attached to object" and "object attached to file" edges via "edge.search"

Summary:
See PHI1901. An install would like improved support for identifying files related to an object (like a task or revision) for retention/archival/backup/migration/snapshotting purposes.

The "attachment" edge is not really user-level: it just means "if you can see the object, that allows you to see the file". This set includes files that users may not think of as "attached", like thumbnails and internal objects which are attached for technical reasons.

However, this is generally an appropriate relationship to expose for retention purposes.

Test Plan: Used "edge.search" to find files attached to a revision and objects attached to a file.

Differential Revision: https://secure.phabricator.com/D21480

12 months agoAdd a "Comment content" field to Herald
epriestley [Fri, 16 Oct 2020 20:19:38 +0000 (13:19 -0700)]
Add a "Comment content" field to Herald

Summary: Ref T13583. To improve support for making it harder to improperly mix data retention policies, allow Herald to act on comment content.

Test Plan:
  - Wrote comment content Herald rules in Maniphest and Differential.
  - Submitted non-matching comments (no action) and matching comments (Herald action).
  - In Differential, triggered rules by submitting non-matching main content and a matching inline comment.

Maniphest Tasks: T13583

Differential Revision: https://secure.phabricator.com/D21479

12 months agoNever render "Show More Context" inside an inline comment suggestion diff
epriestley [Fri, 2 Oct 2020 16:41:29 +0000 (09:41 -0700)]
Never render "Show More Context" inside an inline comment suggestion diff

Summary:
See PHI1896. If you do this:

  - Create an inline comment over a wide range of lines.
  - Suggest an edit.
  - Make a change near the beginning of the block.
  - Make a change near the end of the block.
  - Save the inline.

...you get a rendering which includes a "Show More Context" fold in the middle.

Currently, this element renders in a visually broken way and consumes too many columns.

However, this element isn't ever desirable inside inline comment suggestions. Stop it from rendering entirely.

Test Plan:
  - Made an inline comment suggestion across lines 1-50 with edits at the beginning and end, saw a contiguous diff.
  - Made smaller inline comment suggestions (one line, a few lines).

Differential Revision: https://secure.phabricator.com/D21476

12 months agoUse "getInlines()", not "_inlines", to access inlines on client Changeset objects
epriestley [Fri, 2 Oct 2020 16:09:10 +0000 (09:09 -0700)]
Use "getInlines()", not "_inlines", to access inlines on client Changeset objects

Summary:
See PHI1898. An install is reporting an execution/initialization order issue where this code is reachable before `_inlines` is initialized.

I can't immediately reproduce it, but using "getInlines()" is preferable anyway and seems likely to fix the problem.

Test Plan: Viewed revisions with inlines, added/removed/edited/replied to inlines, didn't find anything broken.

Differential Revision: https://secure.phabricator.com/D21475

12 months agoWhen printing, wrap all content in Remarkup tables more aggressively
epriestley [Mon, 28 Sep 2020 16:17:52 +0000 (09:17 -0700)]
When printing, wrap all content in Remarkup tables more aggressively

Summary:
Ref T13564. See PHI1798. Earlier efforts here (see D21439) still leave us with:

  - Incorrect behavior for long URIs, like `http://www.example.com/MMMMM...`.
  - Incorrect beahvior for long text blocks, like `MMMMMM...`.
  - Undesirable behavior for monospaced text in non-printing contexts (it wraps when we'd prefer it not wrap).

Apply the wrapping rules to all "<td>" content to resolve these three prongs.

Test Plan:
  - Viewed long URIs, text blocks, and monospaced text in and out of tables, while printed and not printed, in Safari, Firefox, and Chrome.
  - All browser behavior now appears to be correct ("all content is preserved in printed document").
  - Some browser behavior when making wrapping choices is questionable, but I can't find an automatic solution for that.

Maniphest Tasks: T13564

Differential Revision: https://secure.phabricator.com/D21472

13 months agoFix an issue where known Subversion commits are incorrectly shown as "Discovering..."
epriestley [Thu, 17 Sep 2020 20:50:24 +0000 (13:50 -0700)]
Fix an issue where known Subversion commits are incorrectly shown as "Discovering..."

Summary:
Ref T13552. The behavior of "RepositoryQuery" with ambiguous identifiers under "withRepositoryPHIDs()" is tricky. This leads to failure to load commits in Subversion in some cases.

Use "withRepository()", which gives us the correct identifier resolution behavior.

Test Plan: Viewed a subversion repository history in Diffusion, saw commit details after change.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21469

13 months agoFix an out-of-order issue in the new update-during-publish behavior
epriestley [Thu, 17 Sep 2020 20:34:50 +0000 (13:34 -0700)]
Fix an out-of-order issue in the new update-during-publish behavior

Summary:
Ref T13552. The Herald field "Accepted Differential revision" (and similar fields) depend on the task/revision update steps running before Herald executes.

Herald currently executes first, so it never sees associated revisions. Swap this order.

Test Plan: Published a commit, got a clean parse/import. Will test with production rules ("Cowboy Commits").

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21468

13 months agoAdd missing indexes to DrydockRepositoryOperation
epriestley [Thu, 17 Sep 2020 19:06:52 +0000 (12:06 -0700)]
Add missing indexes to DrydockRepositoryOperation

Summary: See PHI1885. Repository operations are queryable by state and author, but neither column has a usable key. Add usable keys.

Test Plan: Ran EXPLAIN on a state query. Ran `bin/storage upgrade`. Ran EXPLAIN again, saw query go from a table scan to a `const` key lookup.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D21465

13 months agoLog unexpected exceptions raised by Conduit calls
epriestley [Wed, 16 Sep 2020 00:22:21 +0000 (17:22 -0700)]
Log unexpected exceptions raised by Conduit calls

Summary:
Ref T13581. Currently, unexpected exceptions inside Conduit calls are passed to the client, but not logged on the server.

These exceptions should generally be unexpected, and producing a server-side trace is potentially useful.

Test Plan: Simulated a during-execution exception, saw it get logged on the server.

Maniphest Tasks: T13581

Differential Revision: https://secure.phabricator.com/D21464

13 months agoFix an issue where a GROUP BY was missing when a query matched a revision using multi...
epriestley [Wed, 16 Sep 2020 00:13:21 +0000 (17:13 -0700)]
Fix an issue where a GROUP BY was missing when a query matched a revision using multiple hashes

Summary:
Ref T13581. If you query for revisions by hash and provide multiple hashes (A, B) which match a single revision (e.g., older and newer diffs for that revision), the query omits a GROUP BY clause but should contain one.

Add a GROUP BY clause in this case.

Test Plan:
With a working copy that has multiple hashes corresponding to a single revision, ran `arc branches` before and after the change. Before, got this error:

```
[2020-09-15 17:02:07] EXCEPTION: (ConduitClientException) ERR-CONDUIT-CORE: Rows passed to "loadAllFromArray(...)" include two or more rows with the same ID ("130"). Rows must have unique IDs. An underlying query may be missing a GROUP BY. at [<arcanist>/src/conduit/ConduitFuture.php:65]
```

After, clean execution.

Maniphest Tasks: T13581

Differential Revision: https://secure.phabricator.com/D21462

13 months agoWhen a commit is marked "closeable", clear the "published" flag
epriestley [Tue, 8 Sep 2020 19:51:09 +0000 (12:51 -0700)]
When a commit is marked "closeable", clear the "published" flag

Summary:
Ref T13552. When a previously discovered commit becomes reachable from a permanent ref, we re-queue workers to update it. However, the commit may already be marked as "published", so the publish worker may do nothing.

It would perhaps be simpler to not mark the commit as published when it isn't reachable from a permanent ref, but this is tricky because the flag is also part of the "imported / all steps" state (see T13580).

Until that can be cleaned up, just clear the flag.

Test Plan:
  - Pushed a commit with "fixes X" to a non-permanent branch.
  - Pushed it to a permanent branch.
  - Before change: task failed to close.
  - After change: task closes properly.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21460

13 months agoWhen an in-process worker subtask fails permanently, don't fatal the whole process
epriestley [Tue, 8 Sep 2020 19:15:34 +0000 (12:15 -0700)]
When an in-process worker subtask fails permanently, don't fatal the whole process

Summary:
Ref T13552. Fixes T13569. Currently, if a process uses in-process tasks (usually, a debugging/diagnostic workflow) and those tasks (or tasks those tasks queue) fail permanently, the exception escapes to top level and the process exits.

This isn't desirable; catch the exception and fail them locally instead.

Test Plan:
With a failing Asana integration and misconfigured Webhook, ran `bin/repository reparse --publish ...`.

  - Before: fatals on each substep.
  - After: warnings emitted for failed substep, but process completes.

Maniphest Tasks: T13569, T13552

Differential Revision: https://secure.phabricator.com/D21459

13 months agoFix a view fatal in CommitGraphView when commits are undiscovered
epriestley [Tue, 8 Sep 2020 19:00:05 +0000 (12:00 -0700)]
Fix a view fatal in CommitGraphView when commits are undiscovered

Summary:
Ref T13552. See <https://discourse.phabricator-community.org/t/viewing-repository-history-for-svn-repository-causes-unhandled-exception/4225/>.

This condition is flipped and can fatal by passing a `NULL` value for `$commit` to a typehinted method.

Test Plan: Viewed history page with undiscovered commits.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21458

13 months agoMove task and revision closure to the "publishing" step of the commit import pipeline
epriestley [Wed, 12 Aug 2020 20:07:44 +0000 (13:07 -0700)]
Move task and revision closure to the "publishing" step of the commit import pipeline

Summary:
Ref T13552. Now that these steps can build their own "CommitRef" object from storage on the "CommitData" object, move them from the "Message" step to the "Publishing" step.

This should resolve the root issue in T13552, where a commit moved from a non-permanent branch to a permanent branch does not publish closures properly.

Test Plan: Used "bin/repository reparse --publish ..." to republish changes.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21450

13 months agoMake "CommitData" wrap and persist a "CommitRef" record
epriestley [Wed, 12 Aug 2020 19:44:39 +0000 (12:44 -0700)]
Make "CommitData" wrap and persist a "CommitRef" record

Summary:
Ref T13552. Turn "CommitData" into an application-level layer on top of the repository-level "CommitRef" object.

For older commits which will not have a "CommitRef" record on disk, build a synthetic one at runtime. This could eventually be migrated.

Test Plan: Ran "bin/repository reparse --message", browsed Diffusion.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21449

13 months agoWrap all direct access to author/committer properties on "CommitData"
epriestley [Wed, 12 Aug 2020 19:34:03 +0000 (12:34 -0700)]
Wrap all direct access to author/committer properties on "CommitData"

Summary: Ref T13552. Currently, various callers read raw properties off "CommitData" directly. Wrap these in accessors to support storage changes which persist "CommitRef" information instead.

Test Plan:
- Ran "diffusion.querycommits", saw the same data before and after.
- Looked at a commit, saw authorship information and date.
- Viewed tags in a repository, saw author information.
- Ran "rebuild-identities", saw no net effect.
- Grepped for callers to "getCommitDetail(...)".

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21448

13 months agoTurn "bypassCache" into a no-op in "diffusion.querycommits"
epriestley [Wed, 12 Aug 2020 19:09:32 +0000 (12:09 -0700)]
Turn "bypassCache" into a no-op in "diffusion.querycommits"

Summary: Ref T13552. The internal caller for this now uses "internal.commit.search", which is always authority-reading. No legitimate external caller should rely on the behavior of "bypassCache"; no-op it to simplify behavior.

Test Plan: Called "diffusion.querycommits", saw the same data as before.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21447

13 months agoBuild "DiffusionCommitRef" objects from "internal.commit.search", not "diffusion...
epriestley [Wed, 12 Aug 2020 19:00:13 +0000 (12:00 -0700)]
Build "DiffusionCommitRef" objects from "internal.commit.search", not "diffusion.querycommits", in the message parser worker

Summary: Ref T13552. Swap the call we're using to build "CommitRef" objects here to the recently-introduced "internal.commit.search" method.

Test Plan: Used "bin/repository reparse --message ..." to reparse commits, added "var_dump()" to inspect results. Saw sensible CommitRef and CommitData objects get built.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21446

13 months agoRemove "bin/repository lookup-users" workflow
epriestley [Wed, 12 Aug 2020 19:00:45 +0000 (12:00 -0700)]
Remove "bin/repository lookup-users" workflow

Summary:
Ref T13552. This is one of two callsites to "diffusion.querycommits". It's an old debugging workflow which I haven't used in years and which is likely obsoleted by identities and other changes.

I believe the root problem here was also ultimately user error (a user has misconfigured their local Git author email as another user).

Test Plan: Grepped for "lookup-users", got no hits.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21444

13 months agoAdd "internal.commit.search" to replace the cache bypass mode of "diffusion.querycommits"
epriestley [Wed, 12 Aug 2020 17:13:42 +0000 (10:13 -0700)]
Add "internal.commit.search" to replace the cache bypass mode of "diffusion.querycommits"

Summary:
Ref T13552. Commit parsers currently invoke a special mode of "diffusion.querycommits", which is an older frozen method.

The replacement, "diffusion.commit.search", is not really appropriate for low-level access. This mode of having a single method which operates in "cache" or "non-cache" modes also ends up in a lot of unnecessary field shuffling.

Provide "internal.commit.search" as a modern equivalent that returns a "DiffusionCommitRef"-compatible structure.

Test Plan: Executed "internal.commit.search", got sensible low-level commit results.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21443

13 months agoLift Diffusion Conduit call proxying to the root level of Conduit
epriestley [Wed, 12 Aug 2020 18:11:07 +0000 (11:11 -0700)]
Lift Diffusion Conduit call proxying to the root level of Conduit

Summary:
Ref T13552. Some Diffusion conduit calls may only be served by a node which hosts a working copy on disk, so they're proxied if received by a different node.

This capability is currently bound tightly to "DiffusionRequest", which is a bundle of context parameters used by some Diffusion calls. However, call proxying is not fundamentally a Diffusion behavior.

I want to perform proxying on a "*.search" call which does not use the "DiffusionRequest" parameter bundle. Lift proxying to the root level of Conduit.

Test Plan: Browsed diffusion in a clusterized repsository.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21442

13 months agoDelete some commit dead parsing code
epriestley [Wed, 12 Aug 2020 16:29:52 +0000 (09:29 -0700)]
Delete some commit dead parsing code

Summary: Ref T13552. Neither "$hashes" or "$user" are used, and constructing them has no side effects.

Test Plan: Searched for these symbols.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21441

13 months agoUpdate SES API to use AWSv4 signatures
epriestley [Tue, 15 Sep 2020 16:06:59 +0000 (09:06 -0700)]
Update SES API to use AWSv4 signatures

Summary:
Ref T13570. Fixes T13235. In most cases, we use modern (v4) signatures for almost all AWS API calls, and have for several years.

However, sending email via SES currently uses an older piece of external code which uses the older (v3) signature method.

AWS is retiring v3 signatures on October 1 2020, so this pathway will stop working.

Update the pathway to use `PhutilAWSFuture`, which provides v4 signatures.

T13235 discusses poor error messages from SES. Switching to Futures fixes this for free, as they have more useful error handling.

Test Plan:
  - Configured an SES mailer, including the new `region` parameter.
  - Used `bin/mail send-test` to send mail via SES.
  - Sent invalid mail (from an unverified address); got a more useful error message.
  - Grepped for removed external, no hits.

Maniphest Tasks: T13570, T13235

Differential Revision: https://secure.phabricator.com/D21461

13 months agoFix additional "xprintf()"-class static parameter lint errors
epriestley [Tue, 8 Sep 2020 15:53:39 +0000 (08:53 -0700)]
Fix additional "xprintf()"-class static parameter lint errors

Summary: Ref T13577. After the fix in D21453, lint identifies additional static errors in Phabricator; fix them.

Test Plan: Ran `arc lint`; these messages are essentially all very obscure.

Subscribers: hach-que, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13577

Differential Revision: https://secure.phabricator.com/D21457

13 months agoRemove obsolete write to "pid" property in "annihilateProcessGroup()" in Daemon Overseer
epriestley [Fri, 4 Sep 2020 23:37:23 +0000 (16:37 -0700)]
Remove obsolete write to "pid" property in "annihilateProcessGroup()" in Daemon Overseer

Summary: Ref T13579. This property was removed in D21425, but I missed this usage site. Remove the assignment; this class no longer tracks the subprocess PID directly.

Test Plan: Searched for "->pid", no further hits.

Maniphest Tasks: T13579

Differential Revision: https://secure.phabricator.com/D21452

13 months agoWhen printing timestamps on paper: use an absolute, context-free date format
epriestley [Fri, 4 Sep 2020 19:07:01 +0000 (12:07 -0700)]
When printing timestamps on paper: use an absolute, context-free date format

Summary:
Ref T13573. Using the browser "Print" feature on pages produces "Thu, Aug 4, 12:22" timestamps which require context to interpret precisely (they don't have a year and don't have a timezone).

Instead, retain these timestamps in "screen" contexts but use "YYYY-MM-DD HH:MM:SS (UTC+X)" timestamps when printing.

Test Plan: Printed Maniphest tasks and other pages in Safari and Chrome using "?__print__=1" and "Print to PDF", saw absolute timestamps after this chagne in the printed documents.

Maniphest Tasks: T13573

Differential Revision: https://secure.phabricator.com/D21451

14 months agoRequire rows passed to "loadAllFromArray()" have unique keys
epriestley [Thu, 9 Jul 2020 17:53:20 +0000 (10:53 -0700)]
Require rows passed to "loadAllFromArray()" have unique keys

Summary:
See PHI1809, which identified a bug in Project search where queries with a large number of slugs could paginate improperly.

This change detects problems in this category: cases where multiple rows with the same ID are passed to "loadAllFromArray()". It's likely that all cases it detects are cases where a GROUP BY is missing.

Since this might have some false positives or detect some things which aren't fundamentally problematic, I'm planning to hold it until the next release.

Test Plan:
  - Reverted D21399, then created a project with multiple slugs and queried for one of them via "project.search". Hit this new exeception.
  - Browsed around a bit, didn't immediately catch any collateral damage.

Differential Revision: https://secure.phabricator.com/D21400

14 months agoFix some content/background overflow issues with commit graph lists
epriestley [Wed, 12 Aug 2020 15:48:25 +0000 (08:48 -0700)]
Fix some content/background overflow issues with commit graph lists

Summary: Ref T13552. There are currently some content overflow issues on the graph view where the menu height can exceed the content height and the frame is drawn on a sub-element. Make the frame draw around all the content.

Test Plan: Viewed commit graph history view, saw more sensible UI.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21440

14 months agoImprove handle/status list display on devices in commit graph lists
epriestley [Tue, 28 Jul 2020 17:19:20 +0000 (10:19 -0700)]
Improve handle/status list display on devices in commit graph lists

Summary: Ref T13552. Provide a richer handle/status list item for commit lists.

Test Plan: Viewed commits in various interfaces, saw richer information.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21431

14 months agoImprove commit action item layout on mobile
epriestley [Fri, 17 Jul 2020 18:57:10 +0000 (11:57 -0700)]
Improve commit action item layout on mobile

Summary:
Ref T13552. Build the "commit list" elements so that the menu action items collapse under the element on mobile.

Also change the mobile breakpoint to 512px because my Safari window can't go any narrower than 508px. Future changes to responsive design will be more content-aware anyway.

Test Plan: Looked at commits in various interfaces, at desktop and mobile widths.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21430

14 months agoImprove desktop and mobile layouts for new "CommitGridView"
epriestley [Sun, 12 Jul 2020 21:07:17 +0000 (14:07 -0700)]
Improve desktop and mobile layouts for new "CommitGridView"

Summary:
Ref T13552. The current layout doesn't work particularly well on desktops or devices.

We have some device/desktop table layout code, but it isn't generic. We also have property list layout code, but it isn't generic either.

Provide generic layout elements ("Fuel", from "Phabricator UI Layout" to "PHUIL"?) and narrowly specialize their display behavior. Then swap the ListItemView stuff to use it.

Test Plan:
Saw slightly better responsive behavior:

{F7637457}

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21418

14 months agoUnify more build, property, auditor, and status information into "CommitGraphView"
epriestley [Sun, 12 Jul 2020 19:41:18 +0000 (12:41 -0700)]
Unify more build, property, auditor, and status information into "CommitGraphView"

Summary:
Ref T13552. In unifying the various Graph/List/Table commit views, some information was dropped -- particularly, audit status.

Restore most of it. The result isn't very pretty, but has most of the required information.

Test Plan: {F7637411}

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21417

14 months agoClean up some minor commit list CSS
epriestley [Sun, 12 Jul 2020 18:01:32 +0000 (11:01 -0700)]
Clean up some minor commit list CSS

Summary: Ref T13552. Some of the CSS can be removed or simplified now that essentially all lists of commits are on a single rendering pathway.

Test Plan: Grepped for affected CSS, viewed commit graph.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21416

14 months agoRemove "PhabricatorAuditListView"
epriestley [Sun, 12 Jul 2020 17:39:09 +0000 (10:39 -0700)]
Remove "PhabricatorAuditListView"

Summary: Ref T13552. Remove yet another way to render a list of commits, and unify it with "CommitGraphView".

Test Plan:
  - Viewed commit search results.
  - Viewed owners package detail page.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21415

14 months agoRemove "DiffusionHistoryTableView" and "DiffusionHistoryView"
epriestley [Sun, 12 Jul 2020 16:23:21 +0000 (09:23 -0700)]
Remove "DiffusionHistoryTableView" and "DiffusionHistoryView"

Summary:
Ref T13552.

Currently, the "Browse" page shows a snippet of unmerged changes if you're looking at a non-default branch. Remove this for consistency with the simplified main "Browse" page. This is reachable via "Compare".

Update the "Compare" page to use the new "CommitGraphView".

Test Plan:
  - Looked at the "Browse" page of "stable".
  - Looked at the "Compare" page for "stable vs master".

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21414

14 months agoMove the view of merged changes to "DiffusionCommitGraphView"
epriestley [Sun, 12 Jul 2020 16:10:59 +0000 (09:10 -0700)]
Move the view of merged changes to "DiffusionCommitGraphView"

Summary: Ref T13552. When viewing a merge commit, merged changes are currently shown inline. Update this view to use the new "GraphView" rendering pipeline.

Test Plan:
  - Viewed a merge commit, saw merges.
  - Viewed history, profile page, etc.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21413

14 months agoReplace "DiffusionCommitListView" with "DiffusionCommitGraphView"
epriestley [Sat, 11 Jul 2020 16:24:35 +0000 (09:24 -0700)]
Replace "DiffusionCommitListView" with "DiffusionCommitGraphView"

Summary:
Ref T13552. This older view mostly duplicates other code and has only two callsites:

  - The "Commits" section of user profile pages.
  - The "Ambiguous Hash" page when you visit a commit hash page which is an ambiguous prefix of two or more commit hashes.

Replace both with "DiffusionCommitGraphView".

Test Plan:
  - Visited profile page, clicked "Commits".
  - Visited an ambiguous hash page (`rPbd3c23`).

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21412

14 months agoImprove rendering of history graph in "CommitGraphView"
epriestley [Fri, 10 Jul 2020 18:41:59 +0000 (11:41 -0700)]
Improve rendering of history graph in "CommitGraphView"

Summary: Ref T13552. In the new combined "table/list" graph view, tidy up the graph rendering.

Test Plan: {F7633504}

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21411

14 months agoIntroduce "DiffusionCommitGraphView", which unifies "HistoryListView" and "HistoryTab...
epriestley [Fri, 10 Jul 2020 18:15:41 +0000 (11:15 -0700)]
Introduce "DiffusionCommitGraphView", which unifies "HistoryListView" and "HistoryTableView"

Summary:
Ref T13552. Currently, commit lists are sometimes rendered as an object list and sometimes rendered as a table. There are two separate views for table rendering.

Add a fourth view ("list, with a graph") with the eventual intent of unifying all the other views. For now, this only replaces "HistoryListView" -- and needs some more work to really be a convincing replacement.

Test Plan:
  - Looked at "History" in Diffusion, saw an ugly view with all the information we want.
  - Grepped for "HistoryListView", no hits.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21410

14 months agoRemove the "Graph" view as a dedicated repository view
epriestley [Fri, 10 Jul 2020 17:52:38 +0000 (10:52 -0700)]
Remove the "Graph" view as a dedicated repository view

Summary:
Ref T13552. Currently, Diffusion has two effectively identical history views, the "Graph" view and the "History" view.

These arose out of product uncertainty about the importance of the graph, but I think we can just put the graph on the "object item list" view and merge these views.

Test Plan: Looked at repositories in Diffusion, no longer saw a "Graph" tab. Grepped for "graph"-related symbols.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21409

14 months agoRemove "Recent Commits" from repository landing page
epriestley [Fri, 10 Jul 2020 17:43:16 +0000 (10:43 -0700)]
Remove "Recent Commits" from repository landing page

Summary:
Ref T13552. Currently, the repository landing page has a panel with recent commits. This is accessible by clicking "History" and usually below the fold, so it's not clearly useful.

Since I'm consolidating this code anyway to fix an issue with the import pipeline, just get rid of this history view.

Test Plan: Viewed a repository landing page, no longer saw a history panel.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21408

14 months agoRemove "DiffusionTagTableView"
epriestley [Fri, 10 Jul 2020 16:24:44 +0000 (09:24 -0700)]
Remove "DiffusionTagTableView"

Summary: Ref T13552. This older class has no callers; tag and branch listings were replaced with an "ObjectList" view.

Test Plan: Grepped for "DiffusionTagTableView", got no hits.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21407

14 months agoRemove the "authored" subheader from commits
epriestley [Thu, 9 Jul 2020 23:53:39 +0000 (16:53 -0700)]
Remove the "authored" subheader from commits

Summary:
Ref T13552. I'm trying to reduce the number of direct callers to commit authorship metadata. This header seems low-value enough to simply remove; this information is shown more clearly and prominently in the "Provenance" UI.

In particular, commits have multiple dates (authored, committed, pushed) but this header shows only one. It currently shows the author identity and the commit date, which isn't entirely correct. And it potentially uses an "Identity" as a timeline actor, which is conceptually fine but not entirely firm ground.

Test Plan: Viewed a commit, saw no more subheader.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21406

14 months agoModernize "Author" and "Committer" rendering for commits
epriestley [Thu, 9 Jul 2020 23:44:19 +0000 (16:44 -0700)]
Modernize "Author" and "Committer" rendering for commits

Summary:
Ref T13552. Give "Commit" objects a more modern, identity-aware way to render author and committer information.

This uses handles in a more modern way and gives us a single read callsite for raw author and committer names.

Test Plan:
  - Grepped for callers to the old methods, found none. (There are a lot of "renderAuthor()" callers in transactions, but this call takes no parameters.)
  - Viewed some commits, saw sensible lists of authors and committers.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21405

14 months agoRemove construction of "author" information from "LastModified" payload in Diffusion
epriestley [Thu, 9 Jul 2020 23:43:16 +0000 (16:43 -0700)]
Remove construction of "author" information from "LastModified" payload in Diffusion

Summary:
Ref T13552. When viewing a directory in Diffusion, we make an Ajax call to get the last commit for each path.

This call currently pulls author information, since an older version of this UI showed author information.

The current UI does not show author information, so this parameter is unused. Delete the code which builds it.

Test Plan: Grepped for `'author'` and references to the "pull-lastmodified" behavior. This behavior is invoked in only one place, which never generates an author placeholder.

Maniphest Tasks: T13552

Differential Revision: https://secure.phabricator.com/D21404

14 months agoCoerce Chrome into breaking monospaced text when printing tables to PDFs
epriestley [Tue, 11 Aug 2020 17:50:10 +0000 (10:50 -0700)]
Coerce Chrome into breaking monospaced text when printing tables to PDFs

Summary:
See T13564. In Chrome only, printing tables with a cell containing an unbroken monospaced text element fails to wrap/break the cell.

Adding "overflow-wrap" appears to fix this without making anything worse. Try this until new problems arise.

Test Plan: Printed such a table to PDF in Chrome, got wrapping with all content visible in the PDF.

Differential Revision: https://secure.phabricator.com/D21439

14 months agoMake "Quote" work properly in Pholio
epriestley [Mon, 10 Aug 2020 20:30:11 +0000 (13:30 -0700)]
Make "Quote" work properly in Pholio

Summary:
See <https://discourse.phabricator-community.org/t/quote-comment-missing-on-mock-pages/4155>. Pholio is currently missing a couple of configuration calls to make the "Quote" action work.

Moving to EditEngine is the "real" fix, but this fix is trivial and should make "Quote" work properly with no negative effects.

Test Plan: Viewed a mock, used "quote" to quote a comment.

Differential Revision: https://secure.phabricator.com/D21437

14 months agoCorrect an apparent off-by-one error when adjusting inlines across revision changes
epriestley [Wed, 5 Aug 2020 20:06:57 +0000 (13:06 -0700)]
Correct an apparent off-by-one error when adjusting inlines across revision changes

Summary:
See PHI1834. It's not obvious why this "+1" is present in the code, but it causes inlines to be adjusted incorrectly when a file is not modified across changes. See D21435.

Remove it, which appears to produce accurate adjustment behavior.

Test Plan:
  - See D21435 for instructions to build a change, where a file with lines "A-Z" is unmodified across Diff 1 and Diff 2.
  - Left inlines on lines 14, 17-19, and 16-26 (end of the file) on Diff 1.
  - Before: saw inlines incorrectly adjusted to lines 15, 18, and 17 on Diff 2. Before D21435, the last inline was culled by the rendering engine.
  - After: saw inlines correctly adjusted to lines 14, 17, and 16 (the same lines as the original), render properly, and highlight the correct lines when hovered.

Differential Revision: https://secure.phabricator.com/D21436

14 months agoRecover inline comments which are "adjusted" off the end of a diff
epriestley [Wed, 5 Aug 2020 19:56:04 +0000 (12:56 -0700)]
Recover inline comments which are "adjusted" off the end of a diff

Summary:
See PHI1834. Currently, the inline adjustment engine can sometime "adjust" an inline off the end of a diff. If it does, we lay it out on an invalid display line here and never render it.

Instead, make sure that layout never puts a comment on an invalid line, so the UI is robust against questionable decisions by the adjustment engine: no adjustment should be able to accidentally discard an inline.

Test Plan:
  - Created a two diff revision, where Diffs 1 and 2 have "alphabet.txt" with A-Z on one line each. The file is unchanged across diffs; some other file is changed.
  - Added a comment to lines P-Z of Diff 1.
  - Before: comment is adjusted out of range on Diff 2 and not shown in the UI.
  - After: comment is still adjusted out of range internally, but now corrected into the display range and shown.

Differential Revision: https://secure.phabricator.com/D21435

14 months agoIn Jupyter notebooks, read strings stored in the raw as either "string" or "list...
epriestley [Wed, 5 Aug 2020 19:15:17 +0000 (12:15 -0700)]
In Jupyter notebooks, read strings stored in the raw as either "string" or "list<string>" more consistently

Summary:
Ref PHI1835. Generally, Jupyter notebooks in the wild may store source and markdown content as either a single string or a list of strings.

Make the renderer read these formats more consistently. In particular, this fixes rendering of code blocks stored as a single string.

This also fixes an issue where cell labels were double-rendered in diff views.

Test Plan:
Created a notebook with a code block represented on disk as a single string, rendered a diff from it.

{F7696071}

Differential Revision: https://secure.phabricator.com/D21434

14 months agoIn 1-up source diffs, retain the "No newline at end of file" on "\" lines
epriestley [Wed, 5 Aug 2020 16:46:51 +0000 (09:46 -0700)]
In 1-up source diffs, retain the "No newline at end of file" on "\" lines

Summary:
See PHI1839. Currently, the "No newline at end of file" text is dropped in the 1-up diff view for changes that affect a file with no trailing newline.

Track it through the construction of diff primitivies more carefully.

Test Plan: {F7695760}

Differential Revision: https://secure.phabricator.com/D21433

14 months agoMove "Wait for Previous Commits to Build" out of prototype
epriestley [Thu, 30 Jul 2020 19:37:34 +0000 (12:37 -0700)]
Move "Wait for Previous Commits to Build" out of prototype

Summary:
Although I'm not entirely thrilled about doing flow control like this (as an actual action in a build plan), I believe this build step works correctly and there's no fancy replacement mechanism on the immediate horizon, and this didn't send us down a slippery slope of Turing-complete builds encoded without real structure or context. Just kick it out of prototype.

(Other approaches which might be better in the long run are things like "this is a top-level behavior on the build plan itself" and/or "build plans are written in a DSL, not a Javascript UI".)

Test Plan: Added a new build step, saw this as an option in the "Flow Control" section.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D21432

15 months agoRevert use of "user-select: all" to modify tab selection behavior
epriestley [Fri, 24 Jul 2020 20:35:12 +0000 (13:35 -0700)]
Revert use of "user-select: all" to modify tab selection behavior

Summary:
Reverts D21419. See PHI1814. Previously, I used "user-select: all" to group sequences of spaces for selection.

However, this has a side effect: the sequence is now selected with a single click. I didn't read the docuementation on the CSS property thoroughly and missed this in testing, since I was focused on drag-selection behavior.

This behavior is enough of a net negative that I think we're in a worse state overall; revert it.

Test Plan: Straight revert.

Differential Revision: https://secure.phabricator.com/D21429

15 months agoRemove ancient "phd.trace" and "phd.verbose" configuration options
epriestley [Thu, 23 Jul 2020 19:17:56 +0000 (12:17 -0700)]
Remove ancient "phd.trace" and "phd.verbose" configuration options

Summary:
Ref T13556. These options are very old and effectively obsoleted by "bin/phd debug [--trace]". I haven't used either option diagnostically in many years, and they aren't mentioned in the documentation.

Remove them to simplify configuration, and because "phd.trace" doesn't work anyway and likely hasn't for a long time -- it has specific issues with TTY detection (see T13556).

Test Plan: Grepped for "phd.trace" and "phd.verbose". Ran "bin/phd debug [--trace]" and saw verbose/trace output.

Maniphest Tasks: T13556

Differential Revision: https://secure.phabricator.com/D21426

15 months agoStreamline handling of Futures and PIDs in daemons
epriestley [Thu, 23 Jul 2020 16:11:30 +0000 (09:11 -0700)]
Streamline handling of Futures and PIDs in daemons

Summary:
Ref T13555. Currently, the daemon future may resolve into a failure state immediately inside "start()", and not have a valid PID when we read it.

Instead, read PIDs from the current active future in all cases, using "hasPID()" to test for the presence of a valid PID.

Since we don't query the PID immediately, we no longer need to explicitly start the future.

Also fix an issue where the same future could be added to the overseer pool more than once if it threw on "resolve()". In general:

  - Before we "resolve()" a future, detach it from the DaemonHandle: we're always done with it.
  - Catch exceptions on resolution and treat them the same way as subprocess resolution errors. These aren't common, but are possible in the general case.
  - Have DaemonHandle add futures to the future pool directly when they're created.

Test Plan:
  - Ran daemons with intentional subprocess creation failures, saw clean recovery.
  - Ran daemons with intentional resolution exceptions, saw clean recovery.

Maniphest Tasks: T13555

Differential Revision: https://secure.phabricator.com/D21425

15 months agoManage PIDs more carefully in DaemonHandle
epriestley [Thu, 23 Jul 2020 16:27:14 +0000 (09:27 -0700)]
Manage PIDs more carefully in DaemonHandle

Summary:
Ref T13555. Although these callsites may not actually impact anything, it's possible for an active handle to have no PID (e.g., if the subprocess failed to start).

Handle these cases more carefully.

Test Plan: Started daemons, saw them run fine. See also next change.

Maniphest Tasks: T13555

Differential Revision: https://secure.phabricator.com/D21424

15 months agoFix an issue where prose diffing may fail after hitting the PCRE backtracking limit
epriestley [Thu, 23 Jul 2020 14:35:57 +0000 (07:35 -0700)]
Fix an issue where prose diffing may fail after hitting the PCRE backtracking limit

Summary:
Fixes T13554. For certain prose diff inputs and PCRE backtracking limits, this regular expression may back track too often and fail.

A characteristic input is "x x x x ...", i.e. many sequences where `(.*?)\s*\z` looks like it may be able to match but actually can not.

I think writing an expression which has all the behavior we'd like without this backtracking issue isn't trivial (at least, I don't think I know how to do it offhand); just use a strategy based on "trim()" insetad, which avoids any PCRE complexities here.

Test Plan: Locally, this passes the "x x x ..." test which the previous code failed. I'm not including that test because it won't reproduce across values of "pcre.backtrac_limit", PCRE versions, etc.

Maniphest Tasks: T13554

Differential Revision: https://secure.phabricator.com/D21422

15 months agoFix an issue with destruction of Revision and Diff objects with viewstates
epriestley [Wed, 22 Jul 2020 18:54:06 +0000 (11:54 -0700)]
Fix an issue with destruction of Revision and Diff objects with viewstates

Summary:
See <https://discourse.phabricator-community.org/t/domainexception-when-trying-to-remove-an-differentialrevision/4105>.

These queries aren't actually constructed properly, and destroying a revision or diff with viewstates currently fails.

Test Plan: Used `bin/remove destroy Dxxx` to destroy a revision with viewstates (this also destroys the associated diffs).

Differential Revision: https://secure.phabricator.com/D21421

15 months agoLikely, fix a warning when rendering modified coverage
epriestley [Sat, 18 Jul 2020 02:42:45 +0000 (19:42 -0700)]
Likely, fix a warning when rendering modified coverage

Summary: See PHI1819. This structure may have `null` elements.

Test Plan: Will confirm user reproduction case.

Differential Revision: https://secure.phabricator.com/D21420

15 months agoIn source views, wrap display tabs in "user-select: all" to improve cursor selection...
epriestley [Fri, 17 Jul 2020 21:56:39 +0000 (14:56 -0700)]
In source views, wrap display tabs in "user-select: all" to improve cursor selection behavior

Summary:
Ref T2495. See PHI1814. Currently, Phabricator replaces tabs with spaces when rendering diffs.

This may or may not be the best behavior in the long term, but it gives us more control over expansion of tabs than using tab literals.

However, one downside is that you can use your mouse cursor to select "half a tab", and can't use your mouse cursor to distinguish between tabs and spaces. Although you probably shouldn't be doing this, this behavior is less accurate/correct than selecting the entire block as a single unit.

A specific correctness issue with this behavior is that the entire block is copied to the clipboard as a tab literal if you select any of it, so two different visual selection ranges can produce the same clipboard content.

This particular behavior can be improved with "user-select: all", to instruct browsers to select the entire element as a single logical element. Now, selecting part of the tab selects the whole thing, as though it were really a tab literal.

(Some future change might abandon this approach and opt to use real tab literals with "tab-size" CSS, but we lose some ability to control alignment behavior if we do that and it doesn't have any obvious advantages over this approach other than cursor selection behavior.)

Test Plan:
  - In Safari and Firefox, dragged text to select a whitespace-expanded tab literal. Saw browsers select the whole sequence as though it were a single tab.
  - In Chorme, this also mostly works, but there's some glitchiness and flickering. I think this is still a net improvement, it's just not as smooth as Safari and Firefox.

Maniphest Tasks: T2495

Differential Revision: https://secure.phabricator.com/D21419

15 months agoAllow non-authors to "Request Review" of draft revisions
epriestley [Thu, 9 Jul 2020 20:30:05 +0000 (13:30 -0700)]
Allow non-authors to "Request Review" of draft revisions

Summary:
See PHI1810. In situations where:

  - An author submits an urgent change for review.
  - The author pings reviewers to ask them to look at it.

...the reviewers may not be able to move the review forward if the review is currently a "Draft". They can only "Commandeer" or ask the author to "Request Review" as ways forward.

Although I'm hesitant to support review actions (particularly, "Accept") on draft revisions, I think there's no harm in allowing reviewers to skip tests and promote the revision out of draft as an explicit action.

Additionally, lightly specialize some of the transaction strings to distinguish between "request review from draft" and other state transitions.

Test Plan:
  - As an author, used "Request Review" to promote a draft and to return a change to reviewers for consideration. These behaviors are unchanged, except "promote a draft" has different timeline text.
  - As a non-author, used "Begin Review" to promote a draft.

Differential Revision: https://secure.phabricator.com/D21403

15 months agoDon't raise the "Subscribers Won't Be Notified" draft warning if you aren't adding...
epriestley [Thu, 9 Jul 2020 20:30:01 +0000 (13:30 -0700)]
Don't raise the "Subscribers Won't Be Notified" draft warning if you aren't adding any non-you subscribers

Summary:
Currently, adding subscribers to a draft revision raises a warning that they won't get an email/notification.

This warning has some false positives:

  - it triggers on any subscriber change, including removing subscribers; and
  - it triggers if you're only adding yourself as a subscriber.

Narrow the scope of the warning so it is raised only if you're adding a subscriber other than yourself.

Test Plan:
  - Added a non-self subscriber, got the warning as before.
  - Added self as a subscriber, no warning (previously: warning).
  - Removed a subscriber, no warning (previously: warning).

Differential Revision: https://secure.phabricator.com/D21402

15 months agoExpand Revision transaction API to allow actions to vary more broadly based on the...
epriestley [Thu, 9 Jul 2020 19:23:31 +0000 (12:23 -0700)]
Expand Revision transaction API to allow actions to vary more broadly based on the viewer and revision state

Summary:
See PHI1810. Build toward support for "Request Review" by non-authors on drafts, to forcefully pull a revision out of draft.

Currently, some action strings can't vary based on revision state or the current viewer, so this "pull out of draft" action would have to either: say "Request Review"; or be a totally separate action.

Neither seem great, so allow the labels and messages to vary based on the viewer and revision state.

Test Plan: Grepped for affected symbols, see followup changes.

Differential Revision: https://secure.phabricator.com/D21401

15 months agoAdd some additional patterns to the "filter Mercurial --debug output" list
epriestley [Wed, 8 Jul 2020 20:25:46 +0000 (13:25 -0700)]
Add some additional patterns to the "filter Mercurial --debug output" list

Summary:
Modern Mercurial may emit some more patterns under "--debug".

This whole list is gross and can likely now be eliminated by increasing the minimum required Mercurial version (as `arc` has), but just paper over it for now.

Test Plan:
Locally, saw some views return to functional behavior that weren't previously working on a modern version of Mercurial.

The reproduction case is likely something in the vein of "repository is not writable by webserver, look at history view".

Differential Revision: https://secure.phabricator.com/D21398

15 months agoFix an issue where querying for a large number of projects by slug could paginate...
epriestley [Thu, 9 Jul 2020 17:42:42 +0000 (10:42 -0700)]
Fix an issue where querying for a large number of projects by slug could paginate incorrectly

Summary:
See PHI1809. This query may join the "slug" table, but each project may have multiple slugs, and the query does not "GROUP BY" when this join occurs.

This may lead to partial result sets and unusual paging behavior.

This could likely be caught categorically in `loadAllFromArray()`; I'll adjust this in a followup.

Test Plan:
A minimal reproduction case is something like:

  - Give project P slugs: a, b, c.
  - Give project Q slugs: d.
  - Query for slugs: a, b, c, d; with limit 2.
  - Order the query so P returns first.
  - Expect: P and Q.
  - Actual: P generates 3 raw rows and the final result is just P with no pagination cursor.

Differential Revision: https://secure.phabricator.com/D21399

15 months agoAllow the Fact daemon to hibernate
epriestley [Wed, 1 Jul 2020 11:03:53 +0000 (04:03 -0700)]
Allow the Fact daemon to hibernate

Summary:
A handful of Phacility production shards have run into memory pressure issues recently. Although there's no smoking gun, and at least two other plausible contributors, one possible concern is that the Fact daemon was written before hibernation and can not currently hibernate. Even if there's no memory leak, this creates unnecessary memory pressure by holding the processes in memory.

Allow the Fact daemon to hibernate, like other daemons do.

Test Plan: Ran "bin/phd debug fact", saw the Fact daemon hibernate.

Subscribers: yelirekim

Differential Revision: https://secure.phabricator.com/D21389

15 months agoCollapse repository URI normalization code into Arcanist
epriestley [Tue, 30 Jun 2020 17:48:55 +0000 (10:48 -0700)]
Collapse repository URI normalization code into Arcanist

Summary: Ref T13546. Companion change to D21372. Move URI normalization code to Arcanist to we can more-often resolve remote URIs correctly.

Test Plan: Grepped for affected symbols.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21373

15 months agoWhen long monospaced character sequences appear in Remarkup tables, break rather...
epriestley [Mon, 29 Jun 2020 23:12:28 +0000 (16:12 -0700)]
When long monospaced character sequences appear in Remarkup tables, break rather than scrolling

Summary:
Ref PHI1798. If you put an SSH public key in a table cell with monospaced formatting and then print the table, the cell scrolls and not all of the content appears in your physical printed document.

Generally, the current scrolling behavior for monospaced text seems never-desirable: I can't imagine any cases where we want the table cell to scroll. (There's more of an argument for complex cases where a table cell has, say, an embedded paste.)

Add `line-break: anywhere` to break monospaced text inside these cells.

Test Plan: In Safari, Firefox, and Chrome, viewed a ##|`MMMMM....`|## table. Saw scrolling before and wrapping/breaking after.

Differential Revision: https://secure.phabricator.com/D21370

16 months agoWhen acquiring a GlobalLock, put good connections that just got unlucky back in the...
epriestley [Fri, 26 Jun 2020 00:46:12 +0000 (17:46 -0700)]
When acquiring a GlobalLock, put good connections that just got unlucky back in the pool

Summary:
See PHI1794, which describes a connection exhaustion issue with a large number of webhook tasks in queue.

The "GlobalLock" mechanism manages a separate connection pool from the main pool, and webhook workers immediately try to grab a webhook lock with a 0-second wait when they start. So far, this is fine.

Prior to this change, good connections which fail to acqiure a lock are discarded. This can lead to connection exhaustion as the worker rapidly cycles through lock attempts: the connections will remain open for at least 60 seconds (since D16389) in an effort to avoid outbound port exhaustion, but they're effectively orphaned because they aren't part of the main pool and aren't part of the lock pool. We're basically leaking a connection every time we fail to lock.

Failing to lock doesn't mean we need to discard the connection: it's a completely suitable connection for reuse. Instead of dropping it on the floor, put it into the lock pool.

Test Plan:
  - Used "bin/webhook call ... --count 10000 --background" to queue a large number of webhook calls against a slow ("sleep(15);") webhook.
  - Used "bin/phd launch 32 taskmaster" to start taskmasters.
  - Observed MySQL connection behavior:
    - Before change: 2048 configured connections immediately exhausted.
    - After change: connections stable at ~160ish.
  - Ran queue for a while, saw expected single-threaded calls to webhook.

Differential Revision: https://secure.phabricator.com/D21369

16 months agoAdd "--background" and "--count" flags to "bin/webhook call"
epriestley [Fri, 26 Jun 2020 00:45:48 +0000 (17:45 -0700)]
Add "--background" and "--count" flags to "bin/webhook call"

Summary:
See PHI1794, which reports an issue where a large number of queued webhook calls led to connection exhaustion. To make this easier to reproduce and test, add "--count" and "--background" flags to "bin/webhook call".

This primarily supports "bin/webook call ... --background --count 10000" to quickly fill the queue with a bunch of calls.

Test Plan: Ran `bin/webhook call` in foreground and background modes, with and without counts. Saw appropriate console and queue behavior.

Differential Revision: https://secure.phabricator.com/D21368

16 months agoImprove the quality of SSH error messages
epriestley [Tue, 16 Jun 2020 15:43:25 +0000 (08:43 -0700)]
Improve the quality of SSH error messages

Summary: See PHI1784. Currently, users who pass an invalid SSH command to Phabricator's SSH handler get an unhelpful error message. Make it more helpful.

Test Plan: Ran `./bin/ssh-exec` with no arguments (old, helpful error), invalid arguments (before: unhelpful error; after: helpful error), and valid arguments (old, helpful behavior).

Differential Revision: https://secure.phabricator.com/D21362

16 months agoFix an issue where "Export Data" could fail if a user had a nonempty custom policy...
epriestley [Tue, 16 Jun 2020 13:35:10 +0000 (06:35 -0700)]
Fix an issue where "Export Data" could fail if a user had a nonempty custom policy preference

Summary:
The "Export Data" workflow incorrectly uses the "Policy Favorites" setting to choose a default export format. This is just a copy/paste error; the correct setting exists and is unused.

If the setting value is an array (as the "Policy Favorites" value often is), we try to use it as an array index. This generates a runtime exception after D21044.

```
[2020-06-16 06:32:12] EXCEPTION: (RuntimeException) Illegal offset type in isset or empty at [<arcanist>/src/error/PhutilErrorHandler.php:263]
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/search/controller/PhabricatorApplicationSearchController.php:460]
```

  - Use the correct setting.
  - Make sure the value we read is a string.

Test Plan:
  - Used "Export Data" with a nonempty, array-valued "Policy Favorites" setting.
    - Before: runtime exception.
    - After: clean export.
  - Used "Export Data" again, saw my selection from the first time persisted.

Differential Revision: https://secure.phabricator.com/D21361

16 months agoUpdate tab completion doc
Aviv Eyal [Mon, 15 Jun 2020 13:27:18 +0000 (13:27 +0000)]
Update tab completion doc

Test Plan: `aspell -c`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21359

16 months agoAdd an explicit "uri" to the "harbormaster.buildable.search" results
epriestley [Wed, 10 Jun 2020 17:11:01 +0000 (10:11 -0700)]
Add an explicit "uri" to the "harborbuildable.search" results

Summary: Ref T13546. This makes some "arc" tasks a little easier, and will make them more correct if "arc" ever switches to using SSH.

Test Plan: Ran "harbormaster.buildable.search" from the web UI, saw URIs in the result set.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21346

16 months agoCorrect a prose diff behavior when prose pieces include newlines
epriestley [Sat, 30 May 2020 20:31:42 +0000 (13:31 -0700)]
Correct a prose diff behavior when prose pieces include newlines

Summary:
See <https://discourse.phabricator-community.org/t/bad-regex-in-prose-diff-logic/3969>.

The prose splitting rules normally guarantee that newlines appear only at the beginning or end of blocks. However, if a prose sentence ends with text like "...x\n.", we can end up with a newline inside a "sentence".

If we do, the regular expression that breaks it into pieces will fail.

Arguably, this is an error in how sentences are split apart (we might prefer to split this into two sentences, "x\n" and ".", rather than a single "x\n." sentence) but in the general case it's not unreasonable for blocks to contain newlines, so a simple fix is to make the pattern more robust.

Test Plan: Added a failing test which includes this behavior, made it pass.

Differential Revision: https://secure.phabricator.com/D21295

17 months agoIn Phortune accounts, prevent self-removal more narrowly
epriestley [Tue, 26 May 2020 14:02:31 +0000 (07:02 -0700)]
In Phortune accounts, prevent self-removal more narrowly

Summary:
Currently, Phortune attempts to prevent users from removing themselves as account managers. It does this by checking that the new list includes them.

Usually this is sufficient, because you can't normally edit an account unless you're already a manager. However, we get the wrong result (incorrect rejection of the edit) if the actor is omnipotent and the acting user was not already a member.

It's okay to edit an account into a state which doesn't include you if you have permission to edit the account and aren't already a manager.

Specifically, this supports more formal tooling around staff modifications to billing accounts, where the actor has staff-omnipotence and the acting user is a staff member and only used for purposes of leaving a useful audit trail.

Test Plan: Elsewhere, ran staff tooling to modify accounts and was able to act as "alice" to add "bailey", even though "alice" was not herself a manager.

Differential Revision: https://secure.phabricator.com/D21288

17 months agoFix an issue where inline comments with only edit suggestions are considered empty
epriestley [Sat, 23 May 2020 15:13:41 +0000 (08:13 -0700)]
Fix an issue where inline comments with only edit suggestions are considered empty

Summary:
Ref T13513. An inline is not considered empty if it has a suggestion, but some of the shared transaction code doesn't test for this properly.

Update the shared transaction code to be aware that application comments may have more complex emptiness rules.

Test Plan:
  - Posted an inline with only an edit suggestion, comment went through.
  - Tried to post a normal empty comment, got an appropriate warning.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21287

17 months agoWhen cancelling an edit of an inline with content, don't hide the inline
epriestley [Fri, 22 May 2020 22:27:52 +0000 (15:27 -0700)]
When cancelling an edit of an inline with content, don't hide the inline

Summary: See PHI1753. This condition got rewritten for suggested edits and accidentally inverted.

Test Plan:
  - Create a comment, type text, save draft, edit comment, cancel.
  - Before: comment hides itself.
  - After: comment properly cancels into pre-edit draft state.

Differential Revision: https://secure.phabricator.com/D21286

17 months agoWhen executing a repository passthru command via CommandEngine, don't set a timeout
epriestley [Fri, 22 May 2020 12:42:34 +0000 (05:42 -0700)]
When executing a repository passthru command via CommandEngine, don't set a timeout

Summary:
Ref T13541. The passthru future does not have time limit behavior, so if we reach this code we currently fail.

Phabricator never reaches this code normally, but this code is reachable during debugging if you try to foreground a slow fetch to inspect it.

Passthru commands generally only make sense to run interactively, and the caller or control script can enforce their own timeouts (usually by pressing "^C" with their fingers).

Test Plan: Used a debugging script to run ref-by-ref fetches in the foreground.

Maniphest Tasks: T13541

Differential Revision: https://secure.phabricator.com/D21284

17 months agoFix two rendering issues with Jupyter notebooks
epriestley [Fri, 22 May 2020 18:47:10 +0000 (11:47 -0700)]
Fix two rendering issues with Jupyter notebooks

Summary:
See PHI1752.

  - Early exit of document layout can cause us to fail to populate available rows.
  - Some Jupyter documents have "markdown" cells with plain strings, apparently.

Test Plan: Successfully rendered example diff from PHI1752.

Differential Revision: https://secure.phabricator.com/D21285

17 months agoPrevent keyboard selection of change blocks inside edit suggestions
epriestley [Thu, 21 May 2020 22:01:10 +0000 (15:01 -0700)]
Prevent keyboard selection of change blocks inside edit suggestions

Summary: Ref T13513. When a revision has inlines with edit suggestions, pressing "j" and "k" can incorrectly select the blocks inside the diffs inside the inlines.

Test Plan: Used "j" to cycle through changes in a revision with inline comments with edit suggestions, didn't get jumped into the suggestion diffs.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21283

17 months agoMake "Open in Editor" use the simple line number of the current selected block
epriestley [Thu, 21 May 2020 21:03:21 +0000 (14:03 -0700)]
Make "Open in Editor" use the simple line number of the current selected block

Summary:
Ref PHI1749. Instead of opening files to the last unchanged line on either side of the change, open files to the "simple" line number of the selected block.

For inlines, this is the inline line number.

For blocks, this is the first new-file line number, or the first old-file line number if no new-file line number exists in the block.

This may not always be what the user is hoping for (we can't know what the state of their working copy is) but should produce more obvious behavior.

Test Plan:
  - In Diffusion, used "Open in Editor" with and without line selections. Saw same behavior as before.
  - Used "n" and "r" to leave an inline with the keyboard, saw same behavior as before.
  - Used "\" and "Open in Editor" menu item to open a file with:
    - Nothing selected or changeset selected (line: 1).
    - An inline selected (line: inline line).
    - A block selected (line: first line in block, per above).

Differential Revision: https://secure.phabricator.com/D21282

17 months agoDrop old "differential_commit" table
epriestley [Wed, 20 May 2020 18:53:54 +0000 (11:53 -0700)]
Drop old "differential_commit" table

Summary: Ref T13276. Ref T13513. All readers and writers were removed more than a year ago; clean up the last remnants of this table.

Test Plan: Grepped for table references, found none.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13513, T13276

Differential Revision: https://secure.phabricator.com/D21281

17 months agoUse the changeset parse cache to cache suggestion changesets
epriestley [Wed, 20 May 2020 17:59:33 +0000 (10:59 -0700)]
Use the changeset parse cache to cache suggestion changesets

Summary:
Ref T13513. Syntax highlighting is potentially expensive, and the changeset rendering pipeline can cache it. However, the cache is currently keyed ONLY by Differential changeset ID.

Destroy the existing cache and rebuild it with a more standard cache key so it can be used in a more ad-hoc way by inline suggestion snippets.

Test Plan: Used Darkconsole, saw cache hits and no more inline syntax highlighting for changesets with many inlines.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21280

17 months agoPut a readthrough cache in front of inline context construction
epriestley [Wed, 20 May 2020 17:32:52 +0000 (10:32 -0700)]
Put a readthrough cache in front of inline context construction

Summary: Ref T13513. Inline comment context information is somewhat expensive to construct and can be cached. Add a readthrough cache on top of it.

Test Plan: Loaded a source code changeset with many inline comments, used Darkconsole to inspect query activity. Saw caches get populated. Updated cache key, saw caches regenerate. Browsed Diffusion, nothing looked broken.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21279

17 months agoClean up Diffusion behaviors for inline edit suggestions
epriestley [Wed, 20 May 2020 16:18:18 +0000 (09:18 -0700)]
Clean up Diffusion behaviors for inline edit suggestions

Summary: Ref T13513. For now, I'm not supporting inline edit suggestions in Diffusion, although it's likely not difficult to do so in the future. Clean up some of the code so that plain ol' inlines work the same way they did before.

Test Plan:
  - Created, edited, reloaded, submitted inlines in Diffusion: familiar old behavior.
  - Created, edited, reloaded, submitted inlines with suggestions in Differential: familiar new behavior.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21278

17 months agoRender inline comment suggestions as real diffs
epriestley [Wed, 20 May 2020 16:01:23 +0000 (09:01 -0700)]
Render inline comment suggestions as real diffs

Summary: Ref T13513. When rendering an inline suggestion for display, use highlighting and diffing.

Test Plan: {F7495053}

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21277

17 months agoRoughly support inline comment suggestions
epriestley [Tue, 19 May 2020 21:50:47 +0000 (14:50 -0700)]
Roughly support inline comment suggestions

Summary:
Ref T13513. This still has quite a few rough edges and some significant performance isssues, but appears to mostly work.

Allow reviewers to "Suggest Edit" on an inline comment and provide replacement text for the highlighted source.

Test Plan: Created, edited, reloaded, and submitted inline comments in various states with and without suggestion text.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21276

17 months agoMake server components of inline comment content handling state-oriented
epriestley [Tue, 19 May 2020 21:50:32 +0000 (14:50 -0700)]
Make server components of inline comment content handling state-oriented

Summary: Ref T13513. Introduce a formal server-side content state object so the whole state can be saved and restored to the drafts table, read from the request, etc.

Test Plan: Created and edited inlines. Reloaded drafts with edits. Submitted normal and editing comments. Grepped for affected symbols.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21275

17 months agoAllow "has draft inlines?" queries to overheat
epriestley [Tue, 19 May 2020 20:52:14 +0000 (13:52 -0700)]
Allow "has draft inlines?" queries to overheat

Summary:
Ref T13513. If your 10 most recently authored inlines have all been deleted, these queries can fail by overheating. This is silly and probably rarely happens outside of development.

For now, just let them overheat. This may create a false negative (incorrect "no draft" signal when the real condition is "drafts, but 10 most recent comments were deleted"). This could be sorted out later with a query mode like "executeAny()", perhaps.

Test Plan:
  - Created and deleted 10 inlines.
  - Submitted comments.
  - Before: overheating fatal during draft flag generation.
  - After: clean submission.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21274

17 months agoMake inline content "state-oriented", not "string-oriented"
epriestley [Tue, 19 May 2020 20:46:58 +0000 (13:46 -0700)]
Make inline content "state-oriented", not "string-oriented"

Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.

This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.

Test Plan: Created, edited, submitted, cancelled, etc., comments.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21273

17 months agoRemove PHPMailer code which generates bogus "Message-ID" email headers
epriestley [Fri, 15 May 2020 13:20:28 +0000 (06:20 -0700)]
Remove PHPMailer code which generates bogus "Message-ID" email headers

Summary:
See <https://discourse.phabricator-community.org/t/how-to-override-localhost-localdomain-in-email-message-id/3876/>.

Currently, Phabricator generates a "Message-ID" only in a subset of cases (roughly: when the message is first-in-thread and we expect the thread may have more than one message).

In cases where it does not generate a message ID, it expects the SMTP server to generate one for it. Servers will generally do this, and some ONLY do this (that is, they ignore IDs from Phabricator and replace them). Thus, several pieces of configuration control whether Phabricator attempts to generate a "Message-ID" at all.

The PHPMailer code has fallback behavior which generates a "<random>@localhost.localdomain" message ID. This is never desirable and ignores Phabricator-level configuration that Message IDs should not be generated.

For now, remove this code: it is never the desired behavior and sometimes explicitly contradicts the intent of configuration.

Possibly, a better change may be to make Phabricator always generate a message ID in cases where it isn't forbidden from doing so by configuration. However, that's a more complicated change and it's not clear if/when it would produce better behavior, so start here for now.

Test Plan: Confirmed by affected user (see linked thread).

Differential Revision: https://secure.phabricator.com/D21272