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

1741 commits

Author SHA1 Message Date
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
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
959a835b95 When executing a repository passthru command via CommandEngine, don't set a timeout
Summary:
Ref T13541. The passthru future does not have time limit behavior, so if we reach this code we currently fail.

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

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

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

Maniphest Tasks: T13541

Differential Revision: https://secure.phabricator.com/D21284
2020-05-22 11:54:36 -07:00
epriestley
5d0ae283a9 Put a readthrough cache in front of inline context construction
Summary: Ref T13513. Inline comment context information is somewhat expensive to construct and can be cached. Add a readthrough cache on top of it.

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

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21279
2020-05-20 14:28:37 -07:00
epriestley
d2d7e7f5ff Clean up Diffusion behaviors for inline edit suggestions
Summary: Ref T13513. For now, I'm not supporting inline edit suggestions in Diffusion, although it's likely not difficult to do so in the future. Clean up some of the code so that plain ol' inlines work the same way they did before.

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

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21278
2020-05-20 14:28:12 -07:00
epriestley
4b2a447003 Allow "has draft inlines?" queries to overheat
Summary:
Ref T13513. If your 10 most recently authored inlines have all been deleted, these queries can fail by overheating. This is silly and probably rarely happens outside of development.

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

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

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21274
2020-05-20 14:25:34 -07:00
epriestley
fa2d30ee36 Lift inline comment draft behaviors to "InlineController"
Summary:
Ref T13513. Currently, if you:

  - click a line to create an inline;
  - type some text;
  - wait a moment; and
  - close the page.

...you don't get an "Unsubmitted Draft" marker in the revision list.

Lift all the draft behavior to "InlineController" and make saving a draft dirty the overall container draft state.

Test Plan:
  - Took the steps described above, got a draft state marker.
  - Created, edited, submitted, etc., inlines in Diffusion and Differential.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21235
2020-05-08 06:52:29 -07:00
epriestley
94a95efa05 Replace "loadUnsubmittedInlineComments()" with a modern "DiffQuery"
Summary: Ref T13513. All queries now go through a reasonably minimal set of pathways and should have consistent behavior.

Test Plan:
- Loaded a revision with inlines.
- Created a new empty inline, reloaded page, saw it vanish.
- Created a new empty inline, typed draft text, did not save, reloaded page, saw draft present.
- Created a new empty inline, typed draft text. Submitted feedback, got prompt, answered "Y", saw draft text submit.
- Created a new empty inline, typed draft text, scrolled down to bottom of page, typed non-draft text, saw preview include draft text.
- Marked and submitted "Done".
- Used hide/show on inlines, verified state persisted.
- Did much of the same stuff in Diffusion, where it all works the same way (except: there's no prompt when submitting draft is-editing inlines).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21234
2020-05-07 16:11:02 -07:00
epriestley
79107574a7 Remove "DifferentialInlineCommentQuery"
Summary: Ref T13513. Replaces "DifferentialInlineCommentQuery" with the similar but more modern "DifferentialDiffInlineCommentQuery".

Test Plan: Viewed comments in timeline, changesets. Created, edited, and submitted comments. Hid and un-hid comments, reloading (saw state preserved).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21233
2020-05-07 16:07:55 -07:00
epriestley
983d77848b Move the "Inline List" view to "DiffInlineCommentQuery"
Summary: Ref T13513. Continue removing usage sites for the obsolete "DifferentialInlineCommentQuery".

Test Plan: Viewed the inline list in Differential, saw sensible inlines.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21232
2020-05-07 16:07:23 -07:00
epriestley
af5b94b234 Lift most "InlineController" querying to the base class
Summary: Ref T13513. Move querying to "DiffInlineCommentQuery" classes and lift them into the base Controller.

Test Plan: In Differential and Diffusion, created, edited, and submitted inline comments.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21231
2020-05-07 16:04:51 -07:00
epriestley
949b9163d0 Replace remaining pseudo-query methods on AuditInlineComment
Summary: Ref T13513. Another step closer to the light.

Test Plan: Created, edited, deleted, replied to, and submitted inline comments in Diffusion.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21230
2020-05-07 16:04:27 -07:00
epriestley
6c59db20a3 Replace "loadDraftComments()" with a Query
Summary: Ref T13513. Take another step toward coherent query pathways for inlines.

Test Plan:
  - Created, previewed, and submitted inlines in Diffusion.
  - Got a (mostly) appropriate draft state.
  - Got proper comment peristence, preview behavior, and submission behavior.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21229
2020-05-07 16:02:35 -07:00
epriestley
d0593a5a78 Replace "loadDraftAndPublishedComments()" with a Query
Summary: Ref T13513. Continue marching toward coherent query pathways for all access to inline comments.

Test Plan:
  - Viewed a commit and a path within that commit, as a user with unpublished inlines and a different user.
  - Saw appropriate inlines in all cases (published inlines, plus undeleted unpublished inlines authored by the current viewer).
  - Grepped for "loadDraftAndPublishedComments()".

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21228
2020-05-07 16:02:10 -07:00
epriestley
c1f1345cc0 Make InlineCommentQueries more robust/consistent
Summary:
Ref T13513. Improve consistency and robustness of the "InlineComment" queries.

The only real change here is that these queries now implicitly add a clause for selecting inlines ("pathID IS NULL" or "changesetID IS NULL").

Test Plan: Browed, created, edited, and submitted inlines.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21227
2020-05-07 16:00:28 -07:00
epriestley
0067f1a521 Remove the obsolete "DiffusionInlineCommentPreviewController"
Summary: Ref T13513. This controller was obsoleted by EditEngine and appears unreachable without explicitly typing the URL.

Test Plan:
  - Grepped for the route, didn't find any hits.
  - Deleted the controller, successfully previewed comments in Diffusion.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21224
2020-05-06 10:13:28 -07:00
epriestley
b48a22bf50 Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.

In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.

---

Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.

On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.

Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).

To simplify this:

  - Make `PhabricatorInlineCommentInterface` an abstract base class instead.
  - Lift as much code out of the `Audit` and `Differential` subclasses as possible.
  - Delete methods which no longer have callers, or have only trivial callers.

---

Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.

Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.

These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.

The `EditView` can not fully render its content. Move the content rendering code into the view.

---

Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.

This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.

---

Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.

Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.

This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.

---

Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".

Test Plan:
  - Created comments on either side of a diff.
  - Edited a comment, reloaded, saw edit stick.
  - Saved comments, reloaded, saw save stick.
  - Edited a comment, typed text, cancelled, "unedited" to get state back.
  - Created a comment, typed text, cancelled, "unedited" to get state back.
  - Deleted a comment, "undeleted" to get state back.

Weirdness / known issues:

  - Drafts don't autosave yet.
  - Fixed in D21187:
    - When you create an empty comment then reload, you get an empty editor. This is a bit silly.
    - "Cancel" does not save state, but should, once drafts autosave.
  - Mostly fixed in D21188:
    - "Editing" comments aren't handled specially by the overall submission flow.
    - "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.

Subscribers: jmeador

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21186
2020-05-04 13:10:30 -07:00
epriestley
604811bfc5 Fix a Diffusion issue where commits that do not show changesets would incorrectly try to render changesets
Summary:
See <https://discourse.phabricator-community.org/t/loading-certain-svn-commits-cause-unhandled-exception/3795/>.

Commits with no changesets (for example, deleted commits) don't generate a "$changesets".

Test Plan: Viewed a commit with no changesets. Before change: exception. After change: saw unusual commit state.

Differential Revision: https://secure.phabricator.com/D21175
2020-04-27 10:36:09 -07:00
epriestley
60de1506fe Make "hidden" changesets sticky, and show hidden state in the filetree
Summary:
Ref T13455. Make "hidden" a changeset property similar to other changeset properties.

We don't need to render this on the server, so we make a request (to update the setting) and just discard the response.

Test Plan: {F7375468}

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21158
2020-04-22 16:12:42 -07:00
epriestley
12eddb18fb Entirely replace the old filetree UI with the "flank" UI
Summary:
Ref T13516. Deletes all old filetree / flex / active / collapse nav code in favor of the new code.

Restores the inline tips in the path tree.

Test Plan: {F7374175}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21154
2020-04-22 08:32:02 -07:00
epriestley
ef69c7969f Restore editor behavior to Diffusion and support "\" shortcut
Summary:
Ref T13515. This restores the "Open in Editor" behavior to Diffusion, and makes "\" work there.

The URI pattern is now sent as a structured template to the client, so the code will work properly if a file path contains "%l".

Test Plan:
  - Clicked "Open in Editor" and pressed "\" in Diffusion when viewing a file.
  - Clicked a line, hit "\", got the file opened to that line.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21149
2020-04-19 09:41:37 -07:00
epriestley
3984c14260 Tokenize external editor links so they can be safely materialized on the client
Summary:
Ref T13515. Currently, opening a file to a particular line in an external editor relies on replacing "%l" with "%l" (which is escaped as "%25l") on the server, and then replacing "%25l" with the line number on the client. This will fail if the file path (or any other variable) contains "%l" in its unencoded form.

The parser also can't identify invalid variables.

Pull the parser out, formalize it, and make it generate an intermediate representation which can be sent to the client and reconstituted.

(This temporarily breaks Diffusion and permanently removes the weird, ancient integration in Dark Console.)

Test Plan:
  - Added a bunch of tests for the actual parser.
  - Used "Open in Editor" in Differential.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21143
2020-04-19 09:02:49 -07:00
epriestley
c3c55d82ae Make "renderer", "engine", and "encoding" sticky across reloads in Differential and Diffusion
Summary:
Ref T13455. Update the other "view state" properties to work like "highlight" now works.

Some complexity here arises from these concerns:

  - In "View Standalone", we render the changeset inline. This is useful for debugging/development, and desirable to retain.
  - In all other cases, we render the changeset with AJAX.

So the client needs to be able to learn about the "state" properties of the changeset on two different flows. Prior to this change, each pathway had a fair amount of unique code.

Then, some bookkeeping issues:

  - At inital rendering time, we may not know which renderer will be selected: it may be based on the client viewport dimensions.
  - Prior to this change, the client didn't separate "value of the property for the changeset as rendered" and "desired value of the property".

Test Plan:
  - Viewed changes in Differential, Diffusion, and in standalone mode.
  - Toggled renderer, character sets, and document engine (this one isn't terribly useful). Reloaded, saw them stick.
  - Started typing a comment, cancelled it, hit the undo UI.

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21138
2020-04-19 08:59:09 -07:00
epriestley
8aac55cc57 Make "Highlight As..." sticky across reloads in Diffusion and Differential
Summary:
Ref T13455. Add container-level storage for persistent view state, and persist "Highlight As..." inside it.

The storage generates a "PhabricatorChangesetViewState" configuration object as an output.

When preferences are expressed on a diff and that diff is later attached to a revision, we attempt to copy the preferences.

The internal storage tracks per-changeset settings, but currently always uses "last update wins" to apply the settings in the UI.

Test Plan:
  - Viewed revisions, changed highlighting, reloaded. Saw highlighting stick in revision view and standalone view.
  - Viewed commits, changed highlighting, reloaded. Saw highlighting stick.
  - Created a diff, changed highlighting, turned it into a revision, saw highlighting persist.

Subscribers: jmeador, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21137
2020-04-19 08:58:39 -07:00
epriestley
785f3c98da Extract raw commit messages from Git more faithfully across Git versions
Summary:
Fixes T5028. Older versions of Git (apparently, from before 2010) did not provide a way to extract the raw body of a commit message from "git log", so we approximate it with "subject" and "wrapped body".

In newer versions of Git, the raw body can be extracted exactly.

Adjust how we extract messages based on the version of Git, and try to be more faithful to edge cases: particularly, be more careful to extract the correct number of trailing newlines.

Test Plan:
  - Added "var_dump()" + "die(1)" later in this method, then pushed various commit messages. Used "&& false" to force execution down the old path (either path should work in modern Git).
  - Observed more faithful extraction of messages, including a more faithful extraction of the number of trailing newlines. Extraction is fully faithful if we can go down the "%B" path, which we should be able to in nearly all modern cases.
  - Not all messages extract faithfully or consistently across the old and new versions, but the old extraction is destructive so this is likely about as close as we can realistically ever get.

Maniphest Tasks: T5028

Differential Revision: https://secure.phabricator.com/D21027
2020-02-24 12:37:45 -08:00
Arturas Moskvinas arturas@uber.com
8cc6fe465c Fix diffusion.branchquery returning dictionary instead of array when branches are filtered out
Summary:
`diffusion.branchquery` can return dictionary instead of array if some branches are filtered out.
Eg.:
```
{
  "result": [
    {
      "shortName": "master",
      "commitIdentifier": "2817b0d8f79748ddfad0220c46d9b20bea34f460",
      "refType": "branch",
      "rawFields": {
        "objectname": "2817b0d8f79748ddfad0220c46d9b20bea34f460",
        "objecttype": "commit",
```
might become:
```
{
  "result": {
    "1": {
      "shortName": "master",
      "commitIdentifier": "2817b0d8f79748ddfad0220c46d9b20bea34f460",
      "refType": "branch",
      "rawFields": {
        "objectname": "2817b0d8f79748ddfad0220c46d9b20bea34f460",
        "objecttype": "commit",

```
Reproduction - find repository which has couple of branches, setup to track only some of them, execute `diffusion.branchquery` API call - result is dictionary instead of array

Test Plan: Apply patch, execution `diffusion.branchquery` call - result is no longer dictionary if it was one before

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D20973
2020-02-12 11:50:22 -08:00
epriestley
26c2a1ba68 Move existing "Console" interfaces away from "setFixed(...)" on "TwoColumnView"
Summary: Depends on D20931. Ref T13362. Move all "Console"-style interfaces to use a consistent layout based on a new "LauncherView" which just centers the content.

Test Plan: Viewed all affected interfaces.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13362

Differential Revision: https://secure.phabricator.com/D20933
2020-02-04 06:52:23 -08:00
epriestley
42e46bbe5a Fix an issue where Herald rules could fail to evaluate at post-commit time
Summary: Ref T13480. Some Herald fields need audit information, which recent changes to Herald adapters discarded. For now, just load it unconditionally.

Test Plan: Triggered an Audit-related rule locally.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20962
2020-02-03 05:09:43 -08:00
epriestley
c99485e8a0 Add "Author's Packages" and "Committer's Packages" Herald rules for Commits and Hooks
Summary: Fixes T13480. Adds the remaining missing Owners package rules for Herald commit adapters.

Test Plan: Created hooks which care about these fields, pushed commits, saw sensible transcript values.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20957
2020-01-29 15:52:07 -08:00
epriestley
6628cd2b4f In Herald "Commit" rules, use repository identities to identify authors and committers
Summary: Ref T13480. The Herald "Commit" rules still use raw commit data properties to identify authors and committers. Instead, use repository identities.

Test Plan: Wrote a Herald rule using all four fields, ran it against various commits with and without known authors. Checked transcript for sensible field values.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20955
2020-01-29 15:48:59 -08:00
epriestley
41f143f7fe Respect repository identities when figuring out authors/committers in Herald pre-commit hook rules
Summary:
Ref T13480. Currently, Herald commit hook rules use a raw address resolution query to identify the author and committer for a commit. This will get the wrong answer when the raw identity string has been explicitly bound to some non-default user (most often, it will fail to identify an author when one exists).

Instead, use the "IdentityEngine" to properly resolve identities.

Test Plan: Authored a commit as `X <y@example.com>`, a raw identity with no "natural" matches to users (e.g., no user with that email or username). Bound the identity to a particular user in Diffusion. Wrote a Herald pre-commit content rule, pushed the commit. Saw Herald recognize the correct user when evaluating rules.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20953
2020-01-29 15:15:11 -08:00
epriestley
374f8b10b3 Add a "--dry-run" flag to "bin/repository rebuild-identities"
Summary: Ref T13444. Allow the effects of performing an identity rebuild to be previewed without committing to any changes.

Test Plan: Ran "bin/repository rebuild-identities --all-identities" with and without "--dry-run".

Maniphest Tasks: T13444

Differential Revision: https://secure.phabricator.com/D20922
2019-11-19 12:38:20 -08:00
epriestley
a7aca500bc Update repository identities after all mutations to users and email addresses
Summary:
Ref T13444. Currently, many mutations to users and email addresses (particularly: user creation; and user and address destruction) do not propagate properly to repository identities.

Add hooks to all mutation workflows so repository identities get rebuilt properly when users are created, email addresses are removed, users or email addresses are destroyed, or email addresses are reassigned.

Test Plan:
- Added random email address to account, removed it.
- Added unassociated email address to account, saw identity update (and associate).
  - Removed it, saw identity update (and disassociate).
- Registered an account with an unassociated email address, saw identity update (and associate).
  - Destroyed the account, saw identity update (and disassociate).
- Added address X to account A, unverified.
  - Invited address X.
  - Clicked invite link as account B.
  - Confirmed desire to steal address.
  - Saw identity update and reassociate.

Maniphest Tasks: T13444

Differential Revision: https://secure.phabricator.com/D20914
2019-11-19 09:41:59 -08:00
epriestley
18da346972 Add additional flags to "bin/repository rebuild-identities" to improve flexibility
Summary:
Ref T13444. Repository identities have, at a minimum, some bugs where they do not update relationships properly after many types of email address changes.

It is currently very difficult to fix this once the damage is done since there's no good way to inspect or rebuild them.

Take some steps toward improving observability and providing repair tools: allow `bin/repository rebuild-identities` to effect more repairs and operate on identities more surgically.

Test Plan: Ran `bin/repository rebuild-identities` with all new flags, saw what looked like reasonable rebuilds occur.

Maniphest Tasks: T13444

Differential Revision: https://secure.phabricator.com/D20911
2019-11-19 09:39:48 -08:00
epriestley
0014d0404c Consolidate repository identity resolution and detection code
Summary: Ref T13444. Send all repository identity/detection through a new "DiffusionRepositoryIdentityEngine" which handles resolution and detection updates in one place.

Test Plan:
  - Ran `bin/repository reparse --message ...`, saw author/committer identity updates.
  - Added "goose@example.com" to my email addresses, ran daemons, saw the identity relationship get picked up.
  - Ran `bin/repository rebuild-identities ...`, saw sensible rebuilds.

Maniphest Tasks: T13444

Differential Revision: https://secure.phabricator.com/D20910
2019-11-19 09:39:11 -08:00
epriestley
6afbb6102d Remove "PhabricatorEventType::TYPE_DIFFUSION_LOOKUPUSER" event
Summary: Ref T13444. This is an ancient event and part of the old event system. It is not likely to be in use anymore, and repository identities should generally replace it nowadays anyway.

Test Plan: Grepped for constant and related methods, no longer found any hits.

Maniphest Tasks: T13444

Differential Revision: https://secure.phabricator.com/D20909
2019-11-19 09:38:03 -08:00
epriestley
a2b2c391a1 Distinguish between "Assigned" and "Effective" identity PHIDs more clearly and consistently
Summary:
Ref T13444. You can currently explicitly unassign an identity (useful if the matching algorithm is misfiring). However, this populates the "currentEffectiveUserPHID" with the "unassigned()" token, which mostly makes things more difficult.

When an identity is explicitly unassigned, convert that into an explicit `null` in the effective user PHID.

Then, realign "assigned" / "effective" language a bit. Previously, `withAssigneePHIDs(...)` actualy queried effective users, which was misleading. Finally, bulk up the list view a little bit to make testing slightly easier.

Test Plan:
  - Unassigned an identity, ran migration, saw `currentEffectiveUserPHID` become `NULL` for the identity.
  - Unassigned a fresh identity, saw NULL.
  - Queried for various identities under the modified constraints.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13444

Differential Revision: https://secure.phabricator.com/D20908
2019-11-19 09:37:44 -08:00
epriestley
df0f5c6cee Make repository identity email address association case-insensitive
Summary:
Ref T13444. Currently, identities for a particular email address are queried with "LIKE" against a binary column, which makes the query case-sensitive.

  - Extract the email address into a separate "sort255" column.
  - Add a key for it.
  - Make the query a standard "IN (%Ls)" query.
  - Deal with weird cases where an email address is 10000 bytes long or full of binary junk.

Test Plan:
  - Ran migration, inspected database for general sanity.
  - Ran query script in T13444, saw it return the same hits for "git@" and "GIT@".

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13444

Differential Revision: https://secure.phabricator.com/D20907
2019-11-19 09:37:26 -08:00
epriestley
502ca4767e When "Fetch Refs" are configured for a repository, highlight the "Branches" menu item in the Diffusion Management UI
Summary: Ref T13448. Minor UI issue: setting a "Fetch Refs" value does not highlight the associated menu item, but should.

Test Plan: Set only "Fetch Refs", now saw menu item highlighted.

Maniphest Tasks: T13448

Differential Revision: https://secure.phabricator.com/D20895
2019-11-07 16:28:33 -08:00
epriestley
97bed35085 Show repository information (and use repository identities) in commit hovercards
Summary:
Ref T12164. Ref T13439. Commit hovercards don't currently show the repository. Although this is sometimes obvious from context, it isn't at other times and it's clearly useful/important.

Also, use identities to render author/committer information and show committer if the committer differs from the author.

Test Plan: {F6989595}

Maniphest Tasks: T13439, T12164

Differential Revision: https://secure.phabricator.com/D20881
2019-10-31 09:58:20 -07:00
epriestley
5b36f0c97a Add default branch, description, and metrics (commit count, recent commit) to "diffusion.repository.search"
Summary: Fixes T13430. Provide more information about repositories in "diffusion.repository.search".

Test Plan: Used API console to call method (with new "metrics" attachment), reviewed output. Saw new fields returned.

Maniphest Tasks: T13430

Differential Revision: https://secure.phabricator.com/D20862
2019-10-24 17:16:58 -07:00
epriestley
06edcf2709 Fix an issue where ancestors of permanent refs might not be published during import or if a branch is later made permanent
Summary:
Fixes T13284. See that task for substantial discussion. There are currently two cases where we'll skip over commits which we should publish:

  - if a branch is not permanent, then later made permanent; or
  - in some cases, the first time we examine branches in a repository.

In both cases, this error is one-shot and things work correctly going forward. The root cause is conflation between the states "this ref currently permanent" and "this ref was permanent the last time we updated refs".

Separate these pieces of state and cover all these cases. Also introduce a "--rebuild" flag to fix the state of bad commits.

Test Plan:
See T13284 for the three major cases:

  - initial import;
  - push changes to a nonpermanent branch, update, then make it permanent;
  - push chanegs to a nonpermanent branch, update, push more changes, then make it permanent.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13284

Differential Revision: https://secure.phabricator.com/D20829
2019-09-25 08:54:50 -07:00
epriestley
8ff3a133c4 Generalize repository proxy retry logic to writes
Summary:
Ref T13286. The current (very safe / conservative) rules for retrying git reads generalize to git writes, so we can use the same ruleset in both cases.

Normally, writes converge rapidly to only having good nodes at the head of the list, so this has less impact than the similar change to reads, but it generally improves consistency and allows us to assert that writes which can be served will be served.

Test Plan:
  - In a cluster with an up node and a down node, pushed changes.
  - Saw a push to the down node fail, retry, and succeed.
  - Did some pulls, saw appropriate retries and success.
  - Note that once one write goes through, the node which received the write always ends up at the head of the writable list, so nodes need to be explicitly thawed to reproduce the failure/retry behavior.

Maniphest Tasks: T13286

Differential Revision: https://secure.phabricator.com/D20778
2019-09-03 12:34:10 -07:00
epriestley
ff3d1769b4 Instead of retrying safe reads 3 times, retry each eligible service once
Summary: Ref T13286. When retrying a read request, keep retrying as long as we have canididate services. Since we consume a service with each attempt, there's no real reason to abort early, and trying every service allows reads to always succeed even if (for example) 8 nodes of a 16-node cluster are dead because of a severed network link between datacenters.

Test Plan: Ran `git pull` in a clustered repository with an up node and a down node; saw retry count dynamically adjust to available node count.

Maniphest Tasks: T13286

Differential Revision: https://secure.phabricator.com/D20777
2019-09-03 10:43:33 -07:00
epriestley
95fb237ab3 On Git cluster read failure, retry safe requests
Summary:
Depends on D20775. Ref T13286. When a Git read request fails against a cluster and there are other nodes we could safely try, try more nodes.

We DO NOT retry the request if:

  - the client read anything;
  - the client wrote anything;
  - or we've already retried several times.

Although //some// requests where bytes went over the wire in either direction may be safe to retry, they're rare in practice under Git, and we'd need to puzzle out what state we can safely emit.

Since most types of failure result in an outright connection failure and this catches all of them, it's likely to almost always be sufficient in practice.

Test Plan:
  - Started a cluster with one up node and one down node, pulled it.
  - Half the time, hit the up node and got a clean pull.
  - Half the time, hit the down node and got a connection failure followed by a retry and a clean pull.
  - Forced `$err = 1` so even successful attempts would retry.
  - On hitting the up node, got a "failure" and a decline to retry (bytes already written).
  - On hitting the down node, got a failure and a real retry.
  - (Note that, in both cases, "git pull" exits "0" after the valid wire transaction takes place, even though the remote exited non-zero. If the server gave Git everything it asked for, it doesn't seem to care if the server then exited with an error code.)

Maniphest Tasks: T13286

Differential Revision: https://secure.phabricator.com/D20776
2019-09-03 10:08:43 -07:00
epriestley
b6420e0f0a Allow repository service lookups to return an ordered list of service refs
Summary:
Ref T13286. To support request retries, allow the service lookup method to return an ordered list of structured service references.

Existing callsites continue to immediately discard all but the first reference and pull a URI out of it.

Test Plan: Ran `git pull` in a clustered repository with an "up" node and a "down" node, saw 50% serivce failures and 50% clean pulls.

Maniphest Tasks: T13286

Differential Revision: https://secure.phabricator.com/D20775
2019-09-03 10:05:40 -07:00
epriestley
3c26e38487 Provide a simple read-only maintenance mode for repositories
Summary:
Ref T13393. While doing a shard migration in the Phacility cluster, we'd like to stop writes to the migrating repository. It's safe to continue serving reads.

Add a simple maintenance mode for making repositories completely read-only during maintenance.

Test Plan: Put a repository into read-only mode, tried to write via HTTP + SSH. Viewed web UI. Took it back out of maintenance mode.

Maniphest Tasks: T13393

Differential Revision: https://secure.phabricator.com/D20748
2019-08-29 15:23:10 -07:00
epriestley
6c41508906 Fix an issue where lines with more than one pattern match highlighted improperly in Diffusion
Summary:
Ref T13339. If a search pattern matches more than once on a line, we currently render the line incorreclty, duplicating some of the text.

`substr()` is being called as though the third parameter was `end_offset`, but it's actually `length`. Correct the parameter.

Test Plan:
Before:

{F6676625}

After:

{F6676623}

Maniphest Tasks: T13339

Differential Revision: https://secure.phabricator.com/D20695
2019-08-02 09:44:59 -07:00
epriestley
1fe6311167 Modernize user and repository "delete" workflows and improve documentation
Summary:
Fixes T8830. Fixes T13364.

  - The inability to destroy objects from the web UI is intentional. Make this clear in the messaging, which is somewhat out of date and partly reflects an earlier era when things could be destroyed.
  - `bin/remove destroy` can't rewind time. Document expectations around the "put the cat back in the bag" use case.

Test Plan: Read documentation, clicked through both workflows.

Maniphest Tasks: T13364, T8830

Differential Revision: https://secure.phabricator.com/D20694
2019-08-02 09:30:50 -07:00
Austin McKinley
2f313a0e0d Remove "unstable" status and T2784-specific warning message
Summary: Ref T2784. These are lookin' pretty stable. Subclasses like `DiffusionGetLintMessagesConduitAPIMethod` have their warnings about unstable methods, so just remove this warning in the base class.

Test Plan: Loaded `/conduit`, observed lack of unstable warnings. Only unstable methods are now `diffusion.getlintmessages`, `diffusion.looksoon`, and `diffusion.updatecoverage`.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T2784

Differential Revision: https://secure.phabricator.com/D20651
2019-07-15 14:05:26 -07:00
epriestley
75c3598359 Require commit identities when editing commits to resolve an issue with audit actions not applying properly
Summary:
See <https://discourse.phabricator-community.org/t/cannot-audit-a-git-commit/2848>. In D20581, I made some audit behavior dependent upon identities, but the actual edit flow doesn't load them. This can cause us to raise an "attach identities first" exception in the bowels of the edit workflow and trigger unexpected behavior at top level.

Load identities when editing a commit so that the transaction flows have access to identity information and can use it to figure out if a user is an author, etc.

Test Plan:
  - As an auditor, applied an "Accept Commit" action to an open audit after D20581.
  - Before patch: accept no-ops internally since the preconditions throw.
  - After patch: accept works properly.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20612
2019-06-24 10:50:23 -07:00
epriestley
5d8ee504d6 Replace weird, redundant list of branches in Diffusion "Manage" UI with a link to the main branch list
Summary:
Fixes T13312. Currently, {nav Manage > Branches} has a list of branches on the same page. This has a few minor issues:

  - Pager is at the top (see T13312), which is weird.
  - "Default" icon is mystery meat.
  - Table is kind of pointless/redundant in general?

Previously, this table had more information about technical status of each branch (autoclose/track/publish) but most of these details have been simplified/eliminated, and the main "Branches" view now has more information than it did before.

Get rid of this and just link to the main view.

Test Plan: Viewed "Branches" in UI, saw a link to the main view instead of a weird table.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13312

Differential Revision: https://secure.phabricator.com/D20584
2019-06-18 15:19:53 -07:00
epriestley
dda5c13ef5 Parse "shallow" frames in the Git "upload-pack" wire protocol parser
Summary:
Fixes T13309. If you void the warranty on a repository on disk and turn it into a shallow clone, Phabricator currently can't serve it.

We don't support hosting shallow working copies, but we should still parse and proxy the protocol rather than breaking in a mysterious way.

Test Plan:
  - Created a shallow working copy with `mv X X.full; git clone --depth Y file://.../X.full X` in the storage directory on disk.
  - Cloned it with `git clone <uri>`.
  - Deleted all the refs inside it so the wire only has "shallow" frames; cloned it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13309

Differential Revision: https://secure.phabricator.com/D20577
2019-06-18 15:14:52 -07:00
epriestley
bab35f28e4 Respect repository identities when selecting author vs auditor actions
Summary:
Depends on D20580. Fixes T13311. When we choose which actions to show a user, we can either show them "auditor" actions (like "raise concern") or "author" actions (like "request verification").

Currently, we don't show "author" actions if you're the author of the commit via an identity mapping, but we should. Use identity mappings where they exist.

(Because I've implemented `getEffectiveAuthorPHID()` in a way that requires `$data` be attached, it's possible this will make something throw a "DataNotAttached" exception, but: probably it won't?; and that's easy to fix if it happens.)

Test Plan:
See D20580. As `@alice`, viewed the commit in the UI.

  - Before: got auditor actions presented to me.
  - After: got author actions.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13311

Differential Revision: https://secure.phabricator.com/D20581
2019-06-17 13:56:38 -07:00
epriestley
1eff4fdca3 Prevent "Differential Revision: ..." from counting as a mention in commit messages
Summary:
Ref T13290. Ref T13291. Now that a full URI is a "mention", the full URI in "Differential Revision: ..." also triggers a mention.

Stop it from doing that, since these mentions are silly/redundant/unintended.

The API here is also slightly odd; simplify it a little bit to get rid of doing "append" with "get + append + set".

Test Plan: Used `bin/repository reparse --publish` to republish commits with "Differential Revision: ..." and verified that the revision PHID was properly dropped from the mention list.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13291, T13290

Differential Revision: https://secure.phabricator.com/D20544
2019-05-22 16:22:01 -07:00
epriestley
4180b337cf Add a "{src ...}" Remarkup rule to provide a more flexible way to reference source files in Diffusion
Summary: Depends on D20538. Ref T13291. We now recognize full source URIs, but encoding full URIs isn't super human-friendly and we can't do stuff like relative links with them. Add `{src ...}` as a way to get to this behavior that supports options and more flexible syntax.

Test Plan: {F6463607}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13291

Differential Revision: https://secure.phabricator.com/D20539
2019-05-21 13:12:28 -07:00
epriestley
56e7bde68d Recognize self-URI links to Diffusion files and give them special rendering behavior
Summary:
Depends on D20530. Ref T13291. When users paste links to files in Diffusion into remarkup contexts, identify them and specialize the rendering.

When the URIs are embedded with `{...}`, parse them in more detail.

This is a lead-up to a `{src ...}` rule which will use the same `View` but give users more options to customize presentation.

Test Plan: {F6463580}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13291

Differential Revision: https://secure.phabricator.com/D20538
2019-05-21 13:07:37 -07:00
epriestley
5305ebddda To improve wrapping behavior of rendered README files, don't use "PHUIDocumentView" in Diffusion
Summary:
See PHI1268. We currently do some weird width handling when rendering Diffusion readmes in a document directory view.

I think this came from D12330, which used `PHUIDocumentViewPro` to change the font, but we later reverted the font and were left with the `DocumentView`. Other changes after that modified `DocumentView` to have fixed-width behavior, but it doesn't make much sense here since the content panel is clearly rendered full-width.

Today, the `DocumentView` is a more structural element with methods like `setCurtain()`. Just get rid of it to simplify things, at least as a first step.

Test Plan:
Before:

{F6463493}

After:

{F6463492}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20536
2019-05-21 12:18:18 -07:00
epriestley
c5ecc388a2 Make branch status more clear on Diffusion branches view
Summary:
See PHI1225. Ref T13277. In Diffusion, show "default", "permanent", or "not permanent" when looking at branches.

For repositories with 100 or fewer branches, put default and permanent branches on top.

Test Plan: {F6426814}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: leoluk

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20493
2019-05-16 12:24:39 -07:00
epriestley
5dc4e76eb9 Fix the direction of the commit/revision edge
Summary: Ref T13276. This edge is pointed the wrong way. Point it the right way.

Test Plan: Will verify production works better.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20490
2019-05-02 07:28:49 -07:00
epriestley
a15b0063df Add "Revision has passing builds" Herald rules for commit content (pushes) and commits (discovery)
Summary:
Depends on D20469. Ref T13276. See PHI1159. See PHI953. See PHI901.

Allow Herald to detect when "arc land" would (or did) warn users about failed or ongoing builds. This respects the "Warn on Landing" build plan behavior.

To accomplish this:

  - When we close a revision, set a "wrong build state" flag if it lands in the wrong build state.
  - If the revision is closed when we hit Herald, look for the flag.
  - If not (common for push rules, can happen for commit rules if we race against the revision update worker), hit Harbormaster ourselves and check the current state.

Test Plan:

  - Wrote a "Require Green" rule.
  - Ran it against various commits with various build states (good, not good).
  - Fiddled with "Warn on Landing" and saw the effect in rule evaluation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20470
2019-05-01 10:02:47 -07:00
epriestley
a78f141b3f Unify code for parsing "Reverts X" magic, and when something "reverts <hash>", also revert associated revisions
Summary:
Depends on D20468. Ref T13276. See PHI1008.

When a commit or revision "reverts <hash>", and that's the hash of a commit which has a revision, also write a "reverts" edge to the revision.

Test Plan:
Created "reverts X" objects for:

  - a revision;
  - a commit;
  - a commit with a revision (both got marked properly).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20469
2019-05-01 09:47:40 -07:00
epriestley
56f5c2443b Consolidate readers of "differential.revisionID" property
Summary:
Depends on D20467. Ref T13277. Currently, the "MessageParserWorker" writes this property on commits, then Herald and Audit both read it.

Make them share code so this property has one writer and one reader. This property isn't great, but at least now the badness is hidden.

Currently, we can't just use edges because they may not have been written yet. I am likely to just do this, soon:

  - Just write the edges (in "MessageParserWorker").
  - Hide the edges from mail.

However, we'll sort-of lose the "revisionMatchData" explanation thing if I do that. Maybe this is fine? But when commits match because hashes match, it legitimately isn't obvious.

For now, just reduce the amount of harm/badness here.

Test Plan:
  - Ran `bin/repository reparse --publish ...`.
  - Ran a Herald "Audit" rule using the "Accepted Differential revision" field.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20468
2019-05-01 09:46:17 -07:00
epriestley
0fab41ff3c Show "hold reasons" on commit page, not on "Edit" page
Summary:
Depends on D20465. Ref T13277. Currently, when a commit is unpublished, we put a single line about it on the "Edit Commit" page. This is pretty much impossible to find.

Move it to the main page. This treatment is more big/bold than I'd probably like to end up, but we should probably overshoot on the explanatory text until users get used to this behavior.

Also, allow searching for only published / unpublished commits.

Test Plan: {F6395705}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20466
2019-04-25 09:22:49 -07:00
epriestley
8f43c773b8 Remove nearly all remaining references to "Autoclose"
Summary:
Depends on D20464. Ref T13277. Broadly:

  - Move all the "should publish X" and "why aren't we publishing X" stuff to a separate class (`PhabricatorRepositoryPublisher`).
  - Rename things to be more consistent with modern terminology ("Publish", "Permanent Refs").

Test Plan:
This could use some trial-by-fire on `secure`, but:

  - Grepped for all symbols.
  - Viewed various commits.
  - Reparsed commits.
  - Here's a commit with an explanation:

{F6394569}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20465
2019-04-24 08:29:41 -07:00
epriestley
45b9859f02 Remove "--force-autoclose" from "bin/repository reparse"
Summary: Depends on D20463. Ref T13277. This flag was added some time before 2015 and I don't think I've ever used it. Just get rid of it.

Test Plan: Grepped for `force-autoclose`, `forceAutoclose`, `AUTOCLOSE_FORCED`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20464
2019-04-24 08:24:06 -07:00
epriestley
a2d3d8edeb Move "update related object after commit" to a separate worker in the task queue
Summary:
Depends on D20462. Ref T13276. Currently, the "Message" parser also updates related tasks and revisions when a commit is published.

For PHI1165, which ran into a race with message parsing, I originally believed we needed to separate this logic and lock + yield to avoid the race. D20462 provides what is probably a better approach for avoiding the race.

Still, I think separating these "update related revisions" and "updated related tasks" chunks into separate workers is a net improvement. There may still be some value in doing lock + yield in the future to deal with other issues, and when we occasionally run into problems with pulling a diff out of the repository to update the revision (usually because the diff is too big) this isolates the problem better and allows the commit to import.

I think the only thing to watch out for here is that Herald may now run before the revision and commit are attached to one another. This is fine for all current Herald rules, we just need to be mindful in implementing new rules.

Test Plan: Used `bin/repository reparse --message` on various commits, including commits that close revisions and close tasks.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20463
2019-04-24 06:32:27 -07:00
epriestley
7e8dc0742b Remove all callers to "DifferentialRevision->loadIDsByCommitPHIDs()"
Summary: Depends on D20457. Ref T13276. Kill all remaining callers to this method and delete it.

Test Plan:
- Grepped for `loadIDsByCommitPHIDs`.
- Viewed blame again to make sure I didn't break it.
- Viewed "History" view for commits with revisions.
- Viewed "Graph" view for commits with revisions.
- Viewed "Merged Commits" table for commits with revisions.
- Viewed "Compare" table for commits with revisions.
- Viewed "Repository" main page history table for commits with revisions.
- Grepped for `linkRevision`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20458
2019-04-24 05:53:35 -07:00
epriestley
fc92cf4382 Move BlameController away from ancient "TABLE_COMMIT"
Summary:
Ref T13276. Differential has a pre-edge "TABLE_COMMIT" with about a half-dozen weird callers I'd like to get rid of.

Move blame to use edges instead. (Bonus: this makes blame respect edge edits in the UI.)

Since there are some more callers to clean up this code may move into some "RelatedObjectQueryThing" class or something, but I'm taking it one step at a time for now.

Test Plan: {F6394106}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20457
2019-04-24 05:44:40 -07:00
epriestley
a82a2b8459 Simplify non-bare working copy rules for the new "git fetch" strategy
Summary:
Ref T13280. In D20421, I changed our observe strategy to `git fetch <uri>` in all cases.

This doesn't work in an ancient, non-bare repository if `master` is checked out and `master` is also fetch: `git` refuses to overwrite the local ref unless we pass `--update-head-ok`. Pass this flag.

Also, remove some code which examines branches and tags in a special way for non-bare working copies. The old `git fetch <origin>` code without explicit revsets meant that `refs/remotes/orgin/heads/xyz` got updated instead of `refs/heads/xyz`. We now update our local refs in all cases (bare and non-bare) so we can throw away this special casing.

Test Plan:
  - Replaced a modern bare working copy with a non-bare working copy by explicitly using `git clone` without `--bare`.
  - Ran `bin/repository update`, hit the `--update-head-ok` error. Applied the patch, got a clean update.
  - Used the "repository.branchquery" API method...
    - ...with "contains" to trigger the "git branch" case. Got identical results after removing the special casing.
    - ...without "contains" to trigger the "low level ref" case. Got identical results after removing the special casing.
  - Grepped for `isWorkingCopyBare()`. The only remaining callsites deal with hook paths, and genuinely need to be specialized.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13280

Differential Revision: https://secure.phabricator.com/D20450
2019-04-19 07:06:10 -07:00
epriestley
7be671fb07 Update "Autoclose" documentation to focus on "Permanent Refs" instead
Summary:
Depends on D20433. Ref T13277. Since "Autoclose" no longer exists, update the documentation.

Currently, this documentation focuses a lot on troubleshooting because users historically had a lot of trouble with figuring out why things were or were not autoclosing. I haven't seen any real confusion about this in years, so I suspect we may have improved the import pipeline and/or UI to make this less of a problem.

It's also possible that this document "fixed" the problem, but usually I expect a documentation fix to not affect the frequency of reports, just make them easier to resolve, so I doubt it.

If unclear things remain //and// documentation really did fix it, maybe we can fix the issues. Or we can just put the troubleshooting documentation back.

Test Plan: Read documentation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20434
2019-04-18 05:43:15 -07:00
epriestley
c33f544e74 Deprecate "Track Only" in the Diffusion UI
Summary:
Depends on D20432. Ref T13277. Fixes T12967. Removes some "Track Only" hints and warns that the feature is deprecated in favor of "Permanent Refs" and "Fetch Only".

(This "fixes" T12967 by mooting it.)

Test Plan: Viewed "branches" sectino of the manage UI, edited "braches" section of a repository.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277, T12967

Differential Revision: https://secure.phabricator.com/D20433
2019-04-18 05:40:02 -07:00
epriestley
6449a0ecb2 Rename some internal "Autoclose" mentions to "Permanent Refs"
Summary: Depends on D20428. Ref T13277.

Test Plan: Grep / reading.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20432
2019-04-18 05:38:09 -07:00
epriestley
9107c2e262 Deprecate the "Commit is on autoclose/permanent branch" Herald "Commit" field
Summary:
Depends on D20426. Ref T13277. The new behavior is to fire Herald only once a commit becomes reachable from a permanent ref (previously, an "Autoclose" branch).

That means that every "Commit" Herald rule implicitly has this field as a condition, and it no longer does anything.

Test Plan: Wrote a Herald rule, saw this as an option in the "Deprecated" section.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20427
2019-04-18 05:32:45 -07:00
epriestley
7a4ef2bad8 Move the repository "Publishing" option to the "Basics" panel in repository management
Summary:
Depends on D20424. Ref T13277. Now that the "Actions" panel only has one item ("Publishing"), just move it to the "Basics" panel.

Update the UI to show active/publishing status more clearly and relate them to one another and importing state.

Test Plan: {F6378087}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20425
2019-04-18 05:24:27 -07:00
epriestley
ec9237fe13 In repository settings, fold "Autoclose On/Off" into "Publishing On/Off"
Summary:
Depends on D20423. Ref T13277. Repositories currently have separate toggles for "Autoclose" and "Publishing".

Merge the "Autoclose" toggle into the "Publishing" toggle. I'm unaware of any valid use case for enabling one but not the other.

(This doesn't fix all the documentation, yet.)

Test Plan: Edited a repository, saw only one publishing option.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20424
2019-04-18 05:16:59 -07:00
epriestley
c7b2553ca0 Rename most user-facing "Autoclose" strings to "Permanent Refs"
Summary:
Depends on D20422. Ref T13277. Currently, "track only", "publish", and "autoclose" are three separate ideas. I'd like to generally merge them into a more natural idea called "permanent refs".

Since "Autoclose" effectively now controls both "autoclose" and "publish", rename it.

This doesn't rename all the methods or internals, and the documentation needs an update, but it renames most of the UI-facing stuff.

(You also can only specify branches as "Permanent Refs" today, but we may let you specify tags and other arbitrary refs in the future.)

Test Plan: Grepped, poked around the UI, saw UI show "Permanent" / "Permanent Refs" more often and "Autoclose" less.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20423
2019-04-18 05:14:36 -07:00
epriestley
e910c76e65 Add "Fetch Rules" to observed Git repositories
Summary:
Depends on D20421. Ref T13277. I'd generally like to move away from "Track Only".

Some of the use cases for "Track Only" (or adjacent to "Track Only") are better resolved with "Fetch Rules" -- basically, rules to fetch only some subset of refs from the observed remote.

Add configurable "Fetch Rules" for Git repositories. For example, if you only want to fetch `master`, you can now speify:

```
refs/heads/master
```

If you only want to fetch branches and tags, you can use:

```
refs/heads/*
refs/tags/*
```

In theory, this is slightly less powerful in the general case than "Track Only", but gives us better behavior in some cases (e.g., when the remote has 50K random temporary branches). In practice, I think this and a better "Autoclose Only" will let us move away from "Track Only", get default behavior which is better aligned with what users actually expect, and dodge all the "track tags/refs" questions.

Test Plan: Configured repositories with "Fetch Refs" rules, used `bin/repository pull --verbose --trace ...` to run pulls, saw expected pull/fetch behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20422
2019-04-18 05:09:36 -07:00
epriestley
1cda1402c7 Do not publish/notify about commits which are not reachable from any "Autoclose" ref
Summary:
Depends on D20418. Ref T13277. Fixes T11314.

Currently, when you push commits to some arbitrary ref or tag (like `refs/pull/123` on GitHub, `refs/tags/phabricator/diff/123` on Phabricator, or `refs/changes/whatever` on Gerrit), we do not "autoclose" related objects. This means that we don't process `Ref T123` to create links to tasks, and don't process `Differential Revision: xyz` to close revisions.

However, we //do// still publish these commits. "Publish" means: trigger audits, publish feed stories, and run Herald rules.

  - Stop publishing these commits.
  - In the UI, show these commits as "Not Permanent" with a note that they are "Not [on any permanent branch]."

These commits will publish and autoclose if they ever become reachable from an "autoclose" ref (most commonly, if they are later merged to "master").

Test Plan:
  - Pushed a commit to `refs/tags/quack`.
  - Before: got a feed story.
  - After: no feed story, UI shows commit as "Not Permanent".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277, T11314

Differential Revision: https://secure.phabricator.com/D20419
2019-04-18 05:05:00 -07:00
epriestley
904dbf0db6 Make the "git upload-pack" proxy more robust
Summary:
Depends on D20381. Ref T8093. This makes minor improvements to the protocol proxy to handle cases where we add, remove, or replace refs and may need to move the "capabilities" section.

Rather than invoking a callback on every ref: parse the whole ref list into a data structure, mutate it if necessary (in a future diff), then dump it back into wire format.

This allows us to shift the capabilities data (which needs to be coupled with the first ref) around if we modify the first ref, and reorder the reflist alphabetically like git does.

When the server has no refs, Git sends no capabilities data. This is easy to emulate, just surprising.

Test Plan:
Tested the cases not covered by D20381:

  - Fetching where the fetch actually fetches data.
  - `ls-remote` when we hide the first ref (capabilities data properly moves to the first visible ref).
  - `ls-remote` when the remote is empty (we just drop the capabilities frame completely).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8093

Differential Revision: https://secure.phabricator.com/D20436
2019-04-18 05:04:05 -07:00
epriestley
e08ba99dd3 Proxy the "git upload-pack" wire protocol
Summary:
Depends on D20380. Ref T8093. When prototypes are enabled, inject a (hopefully?) no-op proxy into the Git wire protocol.

This proxy decodes "git upload-pack" and allows the list of references to be rewritten, in a similar way to how we already proxy the Subversion protocol to rewrite URIs and proxy the Mercurial protocol to distinguish between read and write operations.

The piece we care about comes at the beginning, and looks like this:

```
<frame-length><ref-hash> <ref-name>\0<server-capabilities>\n
<frame-length><ref-hash> <ref-name>\n
<frame-length><ref-hash> <ref-name>\n
...
<0000>
```

We can add, remove, or modify this section to make it appear that the server has different refs than the refs that exist on disk.

Things I have tried:

  - `git ls-remote`
  - `git ls-remote` where the server hides some refs.
  - `git fetch` where the fetch is a no-op.

Things I have not tried:

  - `git fetch` where the fetch is not a no-op.
  - Tricking things into doing protocol v2. Or: I tried this, I wasn't successful. In v2, additional "\0" tricks are used to hide data in the capabilities, I think?
  - `git ls-remote` where we rewrite/hide the first ref in the list, and need to move the capabilities frame elsewhere.
  - `git ls-remote` where the server has no refs at all, or we remove every ref.

So the "interesting" piece of this works, but it almost certainly needs some cleanup to survive interaction with the real world.

Test Plan: See above.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8093

Differential Revision: https://secure.phabricator.com/D20381
2019-04-18 04:57:51 -07:00
epriestley
35539a019c Add an optional protocol log to git SSH workflows
Summary:
Ref T8093. Support dumping the protocol bytes to a side channel logfile, as a precursor to parsing the protocol and rewriting protocol frames to virtualize refs.

The protocol itself is mostly ASCII text so the raw protocol bytes are pretty comprehensible.

Test Plan:
{F6363221}

{F6363222}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8093

Differential Revision: https://secure.phabricator.com/D20380
2019-04-18 04:57:04 -07:00
epriestley
23b86bae6c Accept pushes with arbitrary Git refs
Summary:
Depends on D20419. Ref T13277. Fixes T8936. Fixes T9383. Fixes T12300. When you push arbitrary refs to Phabricator, the push log currently complains if those refs are not tags or branches.

Upstream Git now features "notes", and there's no reason to prevent writes to arbitrary refs, particularly beause we plan to start using them soon (see T13278).

Allow these writes as affecting raw refs.

Test Plan:
  - Pushed an arbitrary ref.
  - Pushed some Git notes.
  - Wrote a Herald ref rule, saw "ref" in the dropdown.

{F6376492}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277, T8936, T9383, T12300

Differential Revision: https://secure.phabricator.com/D20420
2019-04-17 12:43:20 -07:00
epriestley
4b8a67ccde Index and show Owners packages affected by Herald rules
Summary:
Depends on D20412. See PHI1147.

  - Index the targets of "Add Reviewer", "Add Blocking Reviewer", "Add Auditor", "Add Subscriber", and "Remove Subscriber" Herald rules. My major goal is to get Owners packages. This will also hit projects/users, but we just don't read those edges (for now, at least).
  - Add a "Related Herald Rules" panel to Owners Package pages.
  - Add a migration to reindex Herald rules for the recent build plan stuff and this, now that such a migration is easy to write.

Test Plan:
Ran migration, verified all rules reindexed.

{F6372695}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D20413
2019-04-17 12:17:30 -07:00
epriestley
e7a31832bf Show a warning when "git" is too old to support filesize limits
Summary: See <https://discourse.phabricator-community.org/t/git-push-failed-with-filesize-limit-on/2635/2>

Test Plan: {F6378519}

Reviewers: amckinley, avivey

Reviewed By: avivey

Differential Revision: https://secure.phabricator.com/D20431
2019-04-16 05:33:29 -07:00
epriestley
6182193cf5 Give the "Code" tab in Diffusion more consistent (path-retaining) behavior
Summary:
Fixes T13270. In Diffusion, the "Code" tab is linked in a weird way that isn't consistent with the other tabs.

Particularly, if you navigate to `x/y/z/` and toggle between the "Branches" and "History" tabs (or other tabs), you keep your path. If you click "Code", you lose your path.

Instead, retain the path, so you can navigate somewhere and then toggle to/from the "Code" tab to get different views of the same path.

Test Plan: Browed into a repository, clicked "History", clicked "Code", ended up back in the place I started.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13270

Differential Revision: https://secure.phabricator.com/D20323
2019-03-25 14:45:02 -07:00
epriestley
d4847c3eeb Convert simple query subclasses to use internal cursors
Summary:
Depends on D20291. Ref T13259. Move all the simple cases (where paging depends only on the partial object and does not depend on keys) to a simple wrapper.

This leaves a smaller set of more complex cases where we care about external data or which keys were requested that I'll convert in followups.

Test Plan: Poked at things, but a lot of stuff is still broken until everything is converted.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13259

Differential Revision: https://secure.phabricator.com/D20292
2019-03-19 13:00:27 -07:00
epriestley
9913754a2a Improve utilization of "AuthTemporaryToken" table keys in LFS authentication queries
Summary:
See PHI1123. The key on this table is `<resource, type, code>` but we currently query for only `<type, code>`. This can't use the key.

Constrain the query to the resource we expect (the repository) so it can use the key.

Test Plan: Pushed files using LFS. See PHI1123 for more, likely.

Reviewers: amckinley

Reviewed By: amckinley

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

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

Reviewers: amckinley

Reviewed By: amckinley

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

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

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

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

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

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13121

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

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

Reviewers: amckinley

Reviewed By: amckinley

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

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

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

Test Plan: {F6240967}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20210
2019-02-25 10:48:12 -08:00