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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
