1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-30 02:32:42 +01:00
Commit graph

16913 commits

Author SHA1 Message Date
epriestley
b4f2cef76c Prevent interruption by the PHP "set_time_limit()" mechanism while holding the durable write lock
Summary:
Ref T13590. By default, PHP kills execution after web scripts run for 30 seconds. If this occurs in the locked section of a repository write while we're holding the durable write lock, the lock will get stuck.

Use "set_time_limit(0)" to prevent this mechanism from interrupting execution while the durable lock is held.

Test Plan:
  - Added "set_time_limit(1)" before the lock and "while (1);" in the critical section of the lock.
  - Pushed, got the lock stuck.
  - Cleared the lock, applied this patch, pushed.
  - Got an infinite hang instead. (Normally, we expect the script to take more than 30 seconds to execute because there is a large push that executes in finite time, not because there's an infinte loop.)

Maniphest Tasks: T13590

Differential Revision: https://secure.phabricator.com/D21526
2021-01-26 16:14:05 -08:00
epriestley
da7d92dd0a Catch more HTTP VCS errors and convert them into VCS repsonses
Summary:
Ref T13590. Currently, errors arising from cluster locking (like the "stuck write lock" exception) are not caught and converted into VCS responses on the HTTP VCS workflow.

Catch a broader range of exceptions and convert them into appropriate responses.

Test Plan:
  - Forced a "stuck write lock" exception, pushed to a Git repository over HTTP.
  - Before: generic fatal.
  - After: VCS-specific fatal with a useful message in the "X-Phabricator-Message" response header.

Maniphest Tasks: T13590

Differential Revision: https://secure.phabricator.com/D21525
2021-01-26 16:14:04 -08:00
epriestley
32c82a53de After loading the effective Viewer during a VCS request, flag them for inline cache generation
Summary:
Ref T13590. User objects have some inline caches that don't do readthrough generation by default because it may be indicative of high-impact performance problems in code.

During a VCS request, these caches are normally unnecessary, but they may be hit on some unusual pathways (like error handling).

Flag VCS users as okay for inline generation. This does not indicate a performance problem and access to these caches is very rare, at least today.

Test Plan:
  - Executed a Git HTTP request which hit an unhandled exception (stuck write lock).
    - Before: got a second-level exception while handling the first exception, when trying to access user preferences to render a standard uncaught exception page.
    - After: no second-level exception.

Maniphest Tasks: T13590

Differential Revision: https://secure.phabricator.com/D21524
2021-01-26 16:14:04 -08:00
epriestley
3a74701555 Return Git HTTP error messages in an HTTP header
Summary:
Ref T13590. Currently, when you encounter a HTTP error in Git, there is no apparent way to make the client show any additional useful information. In particular, the response body is ignored.

We can partially get around this by putting the information in an "X-Phabricator-Message: ..." HTTP header, which is visible with "GIT_CURL_VERBOSE=1 git ...". Users won't normally know to look here, but it's still better than nothing.

Test Plan:
  - Ran "GIT_CURL_VERBOSE=1 git fetch" against a Phabricator HTTP URI that returned a HTTP/500 error.
    - Before: no clue what happened on the client.
    - After: client shows useful message in the "X-Phabricator-Message" header in debug output.

Maniphest Tasks: T13590

Differential Revision: https://secure.phabricator.com/D21523
2021-01-26 16:14:03 -08:00
epriestley
acd767c7f3 Allow "differential.createinline" to accept JSON "false" for "isNewFile"
Summary:
See <https://discourse.phabricator-community.org/t/error-creating-inline-comment-via-conduit-api/4535>. See T12678.

This API method currently does not accept a JSON "false", but reasonably should.

Test Plan:
  - Called method with "isNewFile: false".
    - Before: type error.
    - After: inline comment.

Differential Revision: https://secure.phabricator.com/D21522
2021-01-26 14:56:37 -08:00
epriestley
ed86c42b26 Improve performance of repository discovery in repositories with >65K refs
Summary:
Ref T13593. The commit cache in this Engine has a maximum fixed size (currently 65,535 entries).

If we execute discovery in a repository with more refs than this (e.g., 180K), we get fast lookups for the first 65,535 refs and slow lookups for the remaining refs.

Instead, divide the refs into chunks no larger than the cache size, and perform an explicit cache fill before each chunk is processed.

Test Plan:
  - Created a repository with 1K refs. Set cache size to 256. Ran discovery.
    - Before patch: saw one large cache fill and then ~750 single-gets.
    - After patch: saw four large cache fills.
  - Compared `bin/repository discover ... --verbose` output before and after patch for overall effect; saw no differences.

Maniphest Tasks: T13593

Differential Revision: https://secure.phabricator.com/D21521
2021-01-26 12:27:02 -08:00
epriestley
888604c9dd Fix a "setExternalURI()" fatal while browsing directories with submodules
Summary:
Ref T13595. See that task for discussion.

D21511 renamed the iteration variable here (previously "$path") but did not rename this use of it.

Test Plan:
  - In Diffusion, browsed a directory with a submodule.
    - Before: "setExternalURI()" fatal in conduit call.
    - After: directory listing including submodule.

Maniphest Tasks: T13595

Differential Revision: https://secure.phabricator.com/D21520
2021-01-26 09:14:21 -08:00
epriestley
bafe8d1bbd Correct Git repository browse behavior for differences in "ls-tree" output
Summary:
Ref T13589. The output for "git ls-tree commit:path" (the old invocation) and "git ls-tree commit -- path" (the new invocation) differs: the latter emits absolute paths.

Update the code to account for this difference in behavior.

Test Plan:
  - Browsed a non-root directory in a Git repository in Diffusion.
  - Before: saw absolute paths.
  - After: saw relative paths.

Maniphest Tasks: T13589

Differential Revision: https://secure.phabricator.com/D21519
2021-01-25 09:13:36 -08:00
epriestley
1da94dcf49 Correct some issues around IMPORTED_PERMANENT in RefEngine
Summary: Ref T13591. Fixes a few issues with the recent updates here discovered in more thorough testing.

Test Plan:
- Stopped the daemons.
- Created a new copy of Phabricator in Diffusion.
- Pulled it with `bin/repository pull ...`.
  - Got 17,278 commits on disk with `git log --all --format=%H`.
- Set permanent refs to "master".
- Discovered it with `bin/repository discover ...`.
  - This took 31.5s and inserted 17,278 tasks.
  - Verified that all tasks have priority 4,000 (PRIORITY_IMPORT).
  - Observed that 16,799 commits have IMPORTED_PERMANENT and 479 commits do not.
    - This matches `git log master --format=%H` exactly.
- Ran `bin/repository refs ...`. Expected no changes and saw no changes.
- Ran `bin/worker execute --active` for a minute or two. It processed all the impermanent changes first (since `bin/worker` is LIFO and these are supposed to process last).
  - Ran `bin/repository refs`. Expected no changes and saw no changes.
  - Marked all refs as permanent.
  - Starting state: 16,009 message tasks, all at priority 4000.
  - Ran `bin/repository refs`, expecting 479 new tasks at priority 4000.
  - Saw count rise to 16,488 as expected.
  - Saw all the new tasks have priority 4000 and all commits now have the IMPORTED_PERMANENT flag.

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21518
2021-01-22 19:51:40 -08:00
epriestley
15e022d648 Support an "--active" flag for selecting active tasks
Summary: Ref T13591. This is mostly a workaround for Big Sur not having pcntl/posix installed by default and the mess with M1 / Homebrew / SIP / Code Signing (see T13232) so I can't easily run actual daemons and need to fake them with `bin/worker execute --active`, but it's a reasonable flag on its own.

Test Plan:
  - Ran `bin/worker execute --active` and `bin/worker cancel --active`.

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21517
2021-01-22 19:51:39 -08:00
epriestley
3cb543ef8f Lift logic for queueing commit import tasks into RepositoryEngine
Summary:
Ref T13591. There are currently two pathways to queue an import task for a commit: via repository discovery, or via a ref becoming permanent.

These pathways duplicate some logic and have behavioral differences: one does not set `objectPHID` properly, one does not set the priority correctly.

Unify these pathways, make them both set `objectPHID`, and make them both use the same priority logic.

Test Plan:
  - Discovered refs.
  - See later changes in this series for more complete test cases.

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21516
2021-01-22 19:51:39 -08:00
epriestley
6716d4f6ae Separate "shouldPublishRef()" from "isPermanentRef()" and set "IMPORTED_PERMANENT" more narrowly
Summary:
Ref T13591. Currently, the "IMPORTED_PERMANENT" flag (previously "IMPORTED_CLOSEABLE", until D21514) flag is set by using the result of "shouldPublishRef()".

This method returns the wrong value for the flag when there is a repository-level reason not to publish the ref (most commonly, because the repository is currently importing).

Although it's correct that commits should not be published in an importing repository, that's already handled in the "PublishWorker" by testing "shouldPublishCommit()". The "IMPORTED_PERMANENT" flag should only reflect whether a commit is reachable from a permanent ref or not.

  - Move the relevant logic to a new method in Publisher.
  - Fill "IMPORTED_PERMANENT" narrowly from "isPermanentRef()", rather than broadly from "shouldPublishRef()".
  - Deduplicate some logic in "PhabricatorRepositoryRefEngine" which has the same intent as the logic in the Publisher.

Test Plan:
  - Ran discovery on a new repository, saw permanent commits marked as permanent from the beginning.
  - See later changes in this patch series for additional testing.

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21515
2021-01-22 19:51:38 -08:00
epriestley
2d0e7c37e1 Rename "IMPORTED_CLOSEABLE" to "IMPORTED_PERMANENT" to clarify the meaning of the flag
Summary:
Ref T13591. This is an old flag with an old name, and there's an import bug because the outdated concept of "closable" is confusing two different behaviors.

This flag should mean only "is this commit reachable from a permanent ref?". Rename it to "IMPORTED_PERMANENT" to make that more clear.

Rename the "Unpublished" query to "Permanent" to make that more clear, as well.

Test Plan:
  - Grepped for all affected symbols.
  - Queried for all commmits, permament commits, and impermanent commits.
  - Ran repository discovery.
  - See also further changes in this change series for more extensive tests.

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21514
2021-01-22 19:51:38 -08:00
epriestley
16a14af2bb Correct the behavior of "bin/repository discover --repair"
Summary:
Ref T13591. Since D8781, this flag does not function correctly in Git and Mercurial repositories, since ref discovery pre-fills the cache.

Move the "don't look at the database" behavior the flag enables into the cache lookup. D8781 should have been slightly more aggressive and done this, it was just overlooked.

Test Plan:
  - Ran `bin/repository discover --help` and read the updated help text.
  - Ran `bin/repository discover --repair` in a fully-discovered Git repository.
    - Before: no effect.
    - After: full rediscovery.

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21513
2021-01-22 19:51:38 -08:00
epriestley
e7e8ef7e39 Correct a straggling CLI format string after ref selector changes
Summary: Ref T13589. This is missing a "%s" conversion.

Test Plan: Will view a commit with a diff.

Maniphest Tasks: T13589

Differential Revision: https://secure.phabricator.com/D21512
2021-01-20 15:04:48 -08:00
epriestley
0e28105ff7 Further correct and disambigutate ref selectors passed to Git on the CLI
Summary:
Ref T13589. In D21510, not every ref selector got touched, and this isn't a valid construction in Git:

```
$ git ls-tree ... -- ''
```

Thus:

  - Disambiguate more (all?) ref selectors.
  - Correct the construction of "git ls-tree" when there is no path.
  - Clean some stuff up: make the construction of some flags and arguments more explicit, get rid of a needless "%C", prefer "%Ls" over acrobatics, etc.

Test Plan: Browsed/updated a local Git repository. (This change is somewhat difficult to test exhaustively, as evidenced by the "ls-tree" issue in D21510.)

Maniphest Tasks: T13589

Differential Revision: https://secure.phabricator.com/D21511
2021-01-20 12:07:14 -08:00
epriestley
ea9cb0b625 Disambiguate Git ref selectors in some Git command line invocations
Summary: Ref T13589. See that task for discussion.

Test Plan: Executed most commands via "bin/conduit" or in isolation.

Maniphest Tasks: T13589

Differential Revision: https://secure.phabricator.com/D21510
2021-01-13 12:31:28 -08:00
epriestley
c63c2aadef Support "control" and "return/enter" in the remarkup rule for keystrokes
Summary: These characters are missing support in `{key ...}` but are reasonable to include.

Test Plan: {F8302969}

Differential Revision: https://secure.phabricator.com/D21508
2021-01-11 19:47:40 -08:00
epriestley
04c1f67a02 Add "M<digit>" and "P<digit>" to the default Remarkup ignore list
Summary: Ref T13575. Particularly with the new Apple silicon, I think there are enough domain collisions for `M1`, `M2`, `P1`, etc., to justify adding them to the default ignore list.

Test Plan: Created a mock, then wrote a comment referencing an object on the list (`M1`) and an object not on the list (`T1`). Got text and a link respectively.

Maniphest Tasks: T13575

Differential Revision: https://secure.phabricator.com/D21507
2021-01-11 10:54:41 -08:00
epriestley
18f049a282 Fix reading of the request path when running the PHP builtin webserver
Summary:
Ref T13575. Since PHP builtin webserver support was added, the pathway for parsing request parameters became more complex. We now rebuild "$_REQUEST" later, and this rebuild will destroy any mutations made to it here, so the assignment to "__path__" is lost.

Instead of "validating" the request path, make this method "read" the request path and store it explicitly, so it will survive any later request mutations.

Test Plan:
  - Submitted any POST form while running Phabricator under the builtin PHP webserver. Old behavior was an error when accessing "__path__"; new behavior is a working application.
  - Loaded normal pages, etc.

Maniphest Tasks: T13575

Differential Revision: https://secure.phabricator.com/D21506
2021-01-11 10:54:40 -08:00
Austin McKinley
b2ab18f8f3 Change baseURI for Packages to avoid 404
Summary: Without this change, the landing page for the Packages app is https://secure.phabricator.com/packages, which is a 404. There's probably a better way to fix this, but this was the fewest characters.

Test Plan: doitlive

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21494
2020-11-19 21:48:33 +00:00
epriestley
34082efb02 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
2020-11-03 12:50:18 -08:00
epriestley
bf8707d3a9 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
2020-11-03 12:50:17 -08:00
epriestley
ae5a38f334 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
2020-11-03 11:04:49 -08:00
epriestley
c04147328f 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
2020-10-30 13:46:24 -07:00
epriestley
671986592b 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
2020-10-30 13:02:22 -07:00
epriestley
bc4f86d279 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
2020-10-19 12:34:03 -07:00
epriestley
b2e96df3a3 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
2020-10-19 12:02:30 -07:00
epriestley
2b8bbae5fb 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
2020-10-16 14:10:36 -07:00
epriestley
058d2489e7 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
2020-10-16 13:45:35 -07:00
epriestley
1f7c736f9a 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
2020-10-16 13:42:56 -07:00
epriestley
0f27cd46cc 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
2020-10-02 09:47:32 -07:00
epriestley
0f0e94ca71 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
2020-10-02 09:19:04 -07:00
epriestley
a5f20f7106 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
2020-09-28 09:47:46 -07:00
epriestley
58d3f6145a 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
2020-09-17 13:55:47 -07:00
epriestley
f21a00a315 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
2020-09-17 13:40:45 -07:00
epriestley
a754c694de 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
2020-09-17 12:10:00 -07:00
epriestley
969587f7b0 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
2020-09-15 17:36:43 -07:00
epriestley
2a83df5786 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
2020-09-15 17:36:42 -07:00
epriestley
6f78e2a91c 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
2020-09-15 17:36:42 -07:00
epriestley
737e7c8541 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
2020-09-15 17:36:41 -07:00
epriestley
93ef902ffa 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
2020-09-15 17:36:41 -07:00
epriestley
a39c590442 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
2020-09-15 17:36:40 -07:00
epriestley
cebde34425 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
2020-09-15 17:36:40 -07:00
epriestley
e454c3dafe 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
2020-09-15 17:36:39 -07:00
epriestley
7d6874d9f0 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
2020-09-15 17:36:39 -07:00
epriestley
3a80efa440 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
2020-09-15 17:36:39 -07:00
epriestley
f6238f9d9b 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
2020-09-15 17:36:38 -07:00
epriestley
a9506097ea 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
2020-09-15 17:36:38 -07:00
epriestley
a745055813 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
2020-09-15 17:36:37 -07:00