1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 08:52:39 +01:00
Commit graph

15252 commits

Author SHA1 Message Date
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
epriestley
367cd28927 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
2020-09-15 17:36:37 -07:00
epriestley
3dfa89dd5d 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
2020-09-15 13:03:49 -07:00
epriestley
6e1b5da112 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
2020-09-08 11:45:48 -07:00
epriestley
7daaaa8463 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
2020-09-04 16:41:36 -07:00
epriestley
0854425d19 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
2020-09-04 16:36:34 -07:00
epriestley
72f149bf39 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
2020-08-12 09:05:14 -07:00
epriestley
429543b637 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
2020-08-12 09:04:09 -07:00
epriestley
0b64092d25 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
2020-08-12 09:04:08 -07:00
epriestley
49af92e903 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
2020-08-12 09:04:07 -07:00
epriestley
57f9450bcf 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
2020-08-12 09:04:07 -07:00
epriestley
8aec3f916b 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
2020-08-12 09:04:06 -07:00
epriestley
36dac46ff2 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
2020-08-12 09:00:09 -07:00
epriestley
57ee6649aa 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
2020-08-12 09:00:02 -07:00
epriestley
2b0632b442 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
2020-08-12 08:59:53 -07:00
epriestley
7087c0439a 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
2020-08-12 08:59:46 -07:00
epriestley
cd09ba5e19 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
2020-08-12 08:59:39 -07:00
epriestley
9fa2525384 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
2020-08-12 08:59:31 -07:00
epriestley
46695c76eb 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
2020-08-12 08:59:23 -07:00
epriestley
c6de7c66a3 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
2020-08-12 08:59:16 -07:00
epriestley
9afc5c6287 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
2020-08-12 08:59:07 -07:00
epriestley
c8a279957d 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
2020-08-12 08:58:59 -07:00
epriestley
60e9f64190 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
2020-08-12 08:58:51 -07:00
epriestley
7fd6bf26a9 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
2020-08-12 08:58:43 -07:00
epriestley
81e4e5b7f9 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
2020-08-12 08:58:32 -07:00
epriestley
79375c6c53 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
2020-08-10 13:40:25 -07:00
epriestley
ce0dc9a2ba 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
2020-08-05 13:12:53 -07:00
epriestley
dbdfac1e07 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
2020-08-05 13:12:52 -07:00
epriestley
2db1955159 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
2020-08-05 12:26:00 -07:00
epriestley
98e0440d45 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
2020-08-05 10:16:58 -07:00
epriestley
c1eeacd850 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
2020-07-30 12:46:47 -07:00
epriestley
017ef1927c 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
2020-07-24 13:41:26 -07:00
epriestley
a27c83757d 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
2020-07-23 12:31:32 -07:00
epriestley
78d1b62bb8 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
2020-07-23 11:22:31 -07:00
epriestley
5f0535934d 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
2020-07-23 11:22:30 -07:00
epriestley
fcb75d0503 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
2020-07-23 07:46:15 -07:00
epriestley
8f9ba48528 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
2020-07-22 13:07:11 -07:00
epriestley
0ed5569e9f 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
2020-07-17 19:45:26 -07:00
epriestley
37ffb71c4d 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
2020-07-17 15:10:06 -07:00
epriestley
1d4d860cb5 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
2020-07-09 14:20:51 -07:00
epriestley
6e85b521fe 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
2020-07-09 14:20:51 -07:00
epriestley
b21b73b8dd 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
2020-07-09 14:20:50 -07:00
epriestley
73c4240415 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
2020-07-09 10:50:13 -07:00
epriestley
56838c0e3d 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
2020-07-09 10:50:03 -07:00
epriestley
205657ac76 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
2020-07-01 06:33:06 -07:00
epriestley
7d496f2c6d 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
2020-06-30 15:54:33 -07:00
epriestley
22de618d3b 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
2020-06-25 18:06:09 -07:00
epriestley
d91abf50f7 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
2020-06-25 18:05:58 -07:00