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

16581 commits

Author SHA1 Message Date
epriestley
c79094d7a8 Add static errors, supported protocols, and a dynamic function listing to external editor settings page
Summary:
Ref T13515.

  - Previously valid editor URIs may become invalid without being changed (if an administrator removes a protocol from the list, for example), but this isn't explained very well. Show an error on the settings page if the current value isn't usable.
  - Generate a list of functions from an authority in the parser.
  - Generate a list of protocols from configuration.

Test Plan: {F7370872}

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21146
2020-04-19 09:37:31 -07:00
epriestley
7a79131bf2 Replace old hard-coded URI-based "changes saved" jank with new overgeneralized cookie-based "changes saved" jank
Summary:
Ref T13515. Settings currently has some highly specialized code for rendering "Changes saved." messages. The "saved" state is communicated across a redirect-after-POST by adding `/saved/` to the end of the URI.

This isn't great. It needs a lot of moving pieces, including special accommodations in routing rules. It's user-visible. It has the wrong behavior if you reload the page or navigate directly to the "saved" URI.

Try this scheme, which is also pretty sketchy but seems like an upgrade on the balance:

  - Set a cookie on the redirect which identifies the form we just saved.
  - On page startup: if this cookie exists, save the value and clear it.
  - If the current page started with a cookie identifying the form on the page, treat the page as a "saved" page.

This supports passing a small amount of state across the redirect-after-POST flow, and when you reload the page it doesn't keep the message around. Applications don't need to coordinate it, either. Seems somewhat cleaner?

Test Plan: In Firefox, Safari, and Chrome: saved settings, saw a "Saved changes" banner without any URI junk. Reloaded page, saw banner vanish properly.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21144
2020-04-19 09:04:31 -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
4168335619 Remove the "Edit Multiple Files" external editor setting
Summary: Ref T13515. No callsites actually use this, most editors don't support it, it doesn't seem terribly useful for the ones that do, it makes template-based APIs for line-number substitution complicated, and we can probably just loop on `window.open()` anyway.

Test Plan: Grepped for affected symbols, found no more references. Loaded settings page, saw no more setting.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21142
2020-04-19 09:02:03 -07:00
epriestley
8bdc713352 Make the "Keyboard Shortcuts" dialog in Differential less hideous
Summary:
Ref T13515. Adding "\" ("Open in External Editor") made this slighlty worse, but it was already pretty bad.

Long ago the keys had a special style on them, but this got changed and dropped somewhere around D16568 -- although at the time, I think they still had a grey background (see T11654).

Some later change removed this background.

Put the background back and separate the keystrokes into groups.

Test Plan: {F7370615}

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21141
2020-04-19 09:01:07 -07:00
epriestley
84cd4a3854 Move "External Editor" settings to a separate settings group
Summary:
Ref T13515. It's not intuitive that these settings are "Display Preferences", even thought they're intenrally related to some of the other display preferences.

Give them a separate group.

Test Plan: {F7370500}

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21140
2020-04-19 08:59:43 -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
3adf082002 When inlines would disable a file shield in a diff, still apply the shield if all the comments are collapsed
Summary:
Ref T13515. We "shield" some changesets, including generated code and intradiffs with no intermediate changes.

These files don't get shielded if they have inline comments.

But, if the viewer has collapsed all the comments, we can shield the file again.

Test Plan:
  - Created a change affecting files A and B, with three diffs:
    - Touch A and B.
    - Touch B only.
    - Touch nothing.
  - Added an inline to A and collapsed it.
  - Viewed Diff 1 vs Diff 2:
    - Saw A collapse with a note about inlines.
    - Saw B changes, normally.
  - Viewed Diff 2 vs Diff 3:
    - Saw A collapse with a note about inlines.
    - Saw B collapse normally.
  - Uncollapsed the inline, viewed 1v2 and 2v3, saw A expand in both cases.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21136
2020-04-17 10:52:40 -07:00
epriestley
3d966d8a41 Add an "Open in External Editor" keystroke to Differential
Summary: Ref T13515. See PHI1661. If a file is selected, add a keystroke to click the "Open in External Editor" link.

Test Plan: In Safari, Chrome, and Firefox: used "J" to select a file, then "\" to open it in an external editor. (In Safari and Chrome, this prompts.)

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21135
2020-04-17 10:06:46 -07:00
epriestley
83eb7447a1 When Owners packages are archived, annotate them in tokenizer results
Summary: Fixes T13512. Archived packages in Owners are missing hinting, but should have it.

Test Plan:
Before:

{F7369122}

After:

{F7369128}

Maniphest Tasks: T13512

Differential Revision: https://secure.phabricator.com/D21134
2020-04-17 06:18:04 -07:00
epriestley
925d2b051c Fix a "flickering" behavior with the menu bar transition animations in Chrome
Summary:
Fixes T13508. The "Notification" and "Messages" icons in the menu bar have a CSS transition animation on hover.

In Chrome, when this element moves up 2px, you can get a flicker in and out of the hover state if the user's cursor is at the very bottom of the element, since the bounding box for the element is rapidly sliding in and out of the area under the cursor.

To fix this: as we move the element up, also make it taller.

Test Plan: In Safari, Chrome, and Firefox: put my cursor at the very bottom of the element, no longer saw any animation flickering.

Maniphest Tasks: T13508

Differential Revision: https://secure.phabricator.com/D21133
2020-04-17 06:03:36 -07:00
epriestley
3e676bce9e No-op an ancient Paste edge migration which no longer functions after a database rename
Summary:
Fixes T13510. This migration currently fails because it tries to affect the "paste" database, but when it runs this database will be named "pastebin".

Since the cost of fixing it in place or moving it past the rename migration both seem relatively high (and the cost of throwing it away is plausibly zero) just throw it for now.

Test Plan: Looked at file, saw no more code that can execute.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13510

Differential Revision: https://secure.phabricator.com/D21132
2020-04-17 05:38:31 -07:00
Austin McKinley
ef1340bd32 Add Ferret support to Paste
Summary:
Ref PHI1292. Enable fulltext searchs in paste. Maybe this should only index a snippet instead of the entire content?

Also updates table names in `PhabricatorPasteQuery`.

Test Plan: Created some pastes, indexed them, searched for them.

Reviewers: amckinley

Subscribers: codeblock, Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D20650
2020-04-16 14:10:23 -07:00
epriestley
2748f83e12 Modularize Ferret fulltext functions
Summary: Ref T13511. Currently, Ferret fulltext field functions (like "title:") are hard-coded. Modularize them so extensions may define new ones.

Test Plan: Added a new custom field which emits data for the indexer, searched for "animal-noises:moo", "animal-noises:-", etc., in global search and application search.

Maniphest Tasks: T13511

Differential Revision: https://secure.phabricator.com/D21131
2020-04-16 13:41:13 -07:00
epriestley
894d9b6587 Remove Ferret function aliases and overrides
Summary:
Ref T13511. Ferret functions currently define "aliases", and some applications override the default aliases.

This probably isn't really the right model, since it means the available function aliases in global search depend on the types of documents you're searching for. This isn't fundamentally unworkable but is kind of weird.

Regardless, these don't actually work. Searching for "description:x" is a syntax error.

Since they don't work, it's a good bet no one is relying on them. Just get rid of them until there's a clearer argument for the feature.

Test Plan: Grepped for "getFunctionMap", got no other hits. Ran some queries with the alias functions, got syntax errors.

Maniphest Tasks: T13511

Differential Revision: https://secure.phabricator.com/D21130
2020-04-16 13:40:17 -07:00
epriestley
9bdf477f2f Combine the two different ngram-splitting algorithms into a single engine
Summary:
Ref T13501. Depends on D21127. With the "prefix" behavior removed in D21127, we now have two virtually identical copies of the same code.

The newer one in Ferret is better: it slices utf8 correctly and is slightly more efficient on large inputs. Pull it out and make all callers call into it.

Test Plan:
  - Grepped for all affected symbols.
  - Ran `bin/search index --force ...` to reindex various objects (tasks, files).
  - Searched for things in the UI.

Maniphest Tasks: T13501

Differential Revision: https://secure.phabricator.com/D21128
2020-04-16 09:45:00 -07:00
epriestley
fb3f423279 Remove broken and unfixable "prefix" ngram behavior
Summary:
Ref T13501. The older ngram code has some "prefix" behavior that tries to handle cases where a user issues a very short (one or two character) query.

This code doesn't work, presumably never worked, and can not be made to work (or, at least, I don't see a way, and am fairly sure one does not exist).

If the user searches for "xy", we can find trigrams in the form "xy*" using the index, but not in the form "*xy". The code makes a misguided effort to look for " xy", but this will only find "xy" in words that begin with "xy", like "xylophone".

For example, searching Files for "om" does not currently find "random.txt".

Remove this behavior. Without engaging the trigram index, these queries fall back to an unidexed "LIKE" table scan, but that's about the best we can do.

Test Plan: Searched for "om", hit "random.txt".

Maniphest Tasks: T13501

Differential Revision: https://secure.phabricator.com/D21127
2020-04-16 09:44:37 -07:00
epriestley
b1b9c844ac Remove unused "getAllFunctionFields()" from Ferret
Summary: Ref T13511. This function does nothing interesting and has no callers.

Test Plan: Grepped for callers.

Maniphest Tasks: T13511

Differential Revision: https://secure.phabricator.com/D21126
2020-04-16 09:43:25 -07:00
epriestley
3573170dfa Compress file downloads if the client sends "Accept-Encoding: gzip" and we guess the file might compress alright
Summary:
Ref T13507. We currently compress normal responses, but do not compress file data responses because most files we serve are images and already compressed.

However, there are some cases where large files may be highly compressible (e.g., huge XML files stored in LFS) and we can benefit from compressing responses.

Make a reasonable guess about whether compression is beneficial and enable compression if we guess it is.

Test Plan:
  - Used `curl ...` to download an image with `Accept-Encoding: gzip`. Got raw image data in the response (as expected, because we don't expect images to be worthwhile to recompress).
  - Used `curl ...` to download a text file with `Accept-Encoding: gzip`. Got a compressed response. Decompressed the response into the original file.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21125
2020-04-15 11:53:35 -07:00
epriestley
d86506052c Update a very old Phriction migration which incorrectly uses "save()"
Summary:
See <https://discourse.phabricator-community.org/t/storage-upgrade-error/3748>.

It is broadly unsafe for migrations to use "save()". If the object gains new fields later, the query will include "SET newField = X", which will fail against the old schema which is in the process of being upgraded.

Instead, migrations must issue raw SQL against the schema as it is expected to exist at the time the migration executes.

Migrations have followed this rule for a long time, but this ~6 year old migration was overlooked. Update it to issue a raw query to perform the policy update.

Test Plan: This is somewhat flimsy since rebuilding a genuine reproduction case is messy, but used "bin/storage --apply ..." to at least get the new query to execute against modern Phabricator without issues.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D21124
2020-04-15 08:06:30 -07:00
epriestley
45665dd3b4 Hide "notification.servers" configuration and don't follow redirects from Aphlict
Summary:
See <https://hackerone.com/reports/850114>.

An attacker with administrator privileges can configure "notification.servers" to connect to internal services, either directly or with chosen parameters by selecting an attacker-controlled service and having it issue a "Location" redirect.

Generally, we allow this attack to occur. The same administrator can use an authentication provider or a VCS repository to perform the same attack, and we can't reasonably harden these workflows without breaking things that users expect to be able to do.

There's no reason this particular variation of the attack needs to be allowable, though, and the current behavior isn't consistent with how other similar things work.

  - Hide the "notification.servers" configuration, which also locks it. This is similar to other modern service/server configuration.
  - Don't follow redirects on these requests. Aphlict should never issue a "Location" header, so if we encounter one something is misconfigured. Declining to follow this header likely makes the issue easier to debug.

Test Plan:
  - Viewed configuration in web UI.
  - Configured a server that "Location: ..." redirects, got a followed redirect before and a failure afterward.

{F7365973}

Differential Revision: https://secure.phabricator.com/D21123
2020-04-15 07:00:51 -07:00
epriestley
b52fa96238 Disable automatic decoding of "Content-Encoding" responses during "Accept-Encoding" setup test
Summary:
Ref T13507. Now that we handle processing of "Content-Encoding: gzip" headers by default, this setup check can get a decompressed body back. Since it specifically wants a raw body back, disable this behavior.

Also, "@" a couple things which can get in the way if they fail now that error handling is more aggressive about throwing on warnings.

Test Plan: Ran setup check after other changes in T13507, got clean result.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21122
2020-04-15 06:28:29 -07:00
epriestley
0ea6d131e0 In Conduit responses, assert that Phabricator supports a "gzip" capability
Summary: Ref T13507. If we believe the server can accept "Content-Encoding: gzip" requests, make the claim in an "X-Conduit-Capabilities" header in responses. Clients can use request compression on subsequent requests.

Test Plan: See D21119 for the client piece.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21120
2020-04-14 16:51:03 -07:00
epriestley
6b05d2be28 Add a setup warning to detect "SetInputFilter DEFLATE" and other "Content-Encoding" request mangling
Summary: Ref T13507. See that task for discussion.

Test Plan: Faked different response behaviors and hit both variations of this error.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21116
2020-04-14 14:48:43 -07:00
epriestley
99cbc20778 Reduce the verbosity of the "Aphlict" log
Summary:
See PHI1692. Currently, the Aphlict log is ridiculously verbose. As an initial pass at improving this:

  - When starting in "debug" mode, pass "--debug=1" to Node.
  - In Node, separate logging into "log" (lower-volume, more-important messages) and "trace" (higher-volume, less-important messages).
  - Only print "trace" messages in "debug" mode.

Test Plan: Ran Aphlict in debug and non-debug modes. Behavior unchanged in debug mode, but log has more sensible verbosity in non-debug mode.

Differential Revision: https://secure.phabricator.com/D21115
2020-04-14 13:24:44 -07:00
epriestley
59c855276b Provide a "--local" flag to "bin/conduit call" to force in-process execution
Summary:
See PHI1692. Currently, it's hard to get a local profile or "--trace" of some Diffusion API methods, since they always proxy via HTTP -- even if the local node can serve the request.

This always-proxy behavior is intentional (so we always go down the same code path, to limit surprises) but inconvenient when debugging. Allow an operator to connect to a node which can serve a request and issue a `--local` call to force in-process execution.

This makes it straightforward to "--trace" or "--xprofile" the call.

Test Plan: Ran `bin/conduit call ...` with and without `--local` using a Diffusion method on a clustered repository. Without `--local`, saw proxy via HTTP. With `--local`, saw in-process execution.

Differential Revision: https://secure.phabricator.com/D21114
2020-04-14 13:24:26 -07:00
epriestley
4655a5f059 Document the "field present" and "field absent" operators in Ferret
Summary: Ref T13509. Adds documentation for the new operators.

Test Plan: Read documentation, tried examples, got sensible-seeming results.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21112
2020-04-14 11:08:34 -07:00
epriestley
b3a8754013 Make the Ferret query compiler keep functions sticky across non-initial quoted tokens
Summary: Ref T13509. In `title:big "red" dog`, keep "title" sticky across all three terms, since this seems like it's probably the best match for intent.

Test Plan: Added unit tests; ran unit tests.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21111
2020-04-14 11:00:20 -07:00
epriestley
0511b2a012 Implement the "present" and "absent" operators in the Ferret execution engine
Summary:
Ref T13509. Now that the compiler can parse these queries, actually implement them.

These are fairly easy to implement:

  - For present, just "JOIN". If it works, the field is present.
  - For absent, we "LEFT JOIN" and then "WHERE any_column IS NULL".

Test Plan: Searched for various documents with and without fields present, got sensible results in Maniphest. For example, "body:-" finds tasks with no body, "body:- duck" finds tasks with no body and "duck" elsewhere in the content, and so on.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21110
2020-04-14 10:55:30 -07:00
epriestley
143f86d60b Tighten query compiler rules around spaces inside and after operators
Summary:
Ref T13509. Since `title:- cat` is now ambiguous, forbid spaces after operators.

Also, forbid spaces inside operators, although this has no effect today.

Test Plan: Added unit tests, ran unit tests.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21109
2020-04-14 10:51:55 -07:00
epriestley
8fa8d0e648 Make Ferret query functions sticky only if their values are not quoted
Summary:
Ref T13509. Currently, functions are "sticky", but this stickness is in the query execution layer.

Instead:

  - move stickiness to the query compiler; and
  - make it so that functions are not sticky if their arguments are quoted.

For example:

  - `title:x y` previously meant `title:x title:y` (and still does). The "title:" is sticky.
  - `title:"x" y` previously meant `title:x title:y`. It now means `title:x all:y`. The "title:" is not sticky because the argument is quoted.

Test Plan: Added unit tests, ran unit tests.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21108
2020-04-14 10:47:51 -07:00
epriestley
f31b9987ba Add "absent" and "present" field operators to the Ferret query compiler
Summary: Ref T13509. Parse "xyz:-" as "xyz is absent" and "xyz:~" as "xyz is present". These are new operators which the compiler emits separately from "not" and "substring".

Test Plan: Added unit tests, ran unit tests.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21107
2020-04-14 10:47:20 -07:00
epriestley
5c30a60e30 Tighten Ferret query parsing of empty tokens and empty functions
Summary:
Ref T13509. Certain query tokens like `title:=""` are currently accepted by the parser but discarded, and have no impact on the query. This isn't desirable.

Instead, require that tokens making an assertion about field content must be nonempty.

Test Plan: Added unit tests, made them pass.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21106
2020-04-14 10:32:46 -07:00
epriestley
471e89a8b7 Add "uri" to "paste.search" API output
Summary: Ref T13490. This simplifies some client behavior in the general case.

Test Plan: Called API method, saw URIs.

Maniphest Tasks: T13490

Differential Revision: https://secure.phabricator.com/D21105
2020-04-13 16:17:33 -07:00
epriestley
19e0abcb27 Fix an issue where raw diffs that are not attached to revisions could skip repository policy checks
Summary:
See PHI1697. If a diff is not attached to a revision (for example, if it was created with "arc diff --only"), but is attached to a repository, it is supposed to be visible only to users who can see that repository.

It currently skips this extended policy check and may incorrectly be visible to too many users.

(Once a diff is attached to a revision, this rule is enforced properly via the revision policy.)

Test Plan:
  - Set repository R to be visible only to Alice.
  - As Alice, created a diff from a working copy of repository R with "arc diff --only".
  - As Bailey, viewed the diff.
    - Before: visible diff.
    - After: policy exception (as expected).

Differential Revision: https://secure.phabricator.com/D21103
2020-04-13 12:08:35 -07:00
epriestley
5597f4e6f2 Add "uri" to the fields returned by "differential.revision.search"
Summary: Ref T13490. This simplifies mostly-theoretical cases where you're accessing Phabricator via arc-over-ssh and the Conduit protocol + domain may differ from the production protocol + domain.

Test Plan: Called API via web UI, saw sensible URI values in results.

Maniphest Tasks: T13490

Differential Revision: https://secure.phabricator.com/D21102
2020-04-13 12:06:39 -07:00
epriestley
c3be82fe6e Fix an out-of-date API call on the destruction pathway for Pholio mocks
Summary:
See <https://discourse.phabricator-community.org/t/destroying-a-mock-using-bin-remove-destroy-mx-gives-an-error/3728>.

Currently, Pholio calls an older API method on the mock destruction pathway. This call was introduced in D19911 but the callsite was only partially updated in D19914.

Test Plan: Ran "bin/remove destroy Mx" to destroy a mock. Before: fatal with a bad call; after: clean destruction.

Differential Revision: https://secure.phabricator.com/D21081
2020-04-10 08:01:34 -07:00
epriestley
a2fb91b8af Remove the (hopefully) obsolete "post_max_size" check during startup
Summary:
Ref T13507. See that task for discussion. This check appears to be obsolete in all common cases and misfires if the client submits compressed requests.

Since the cases where it could still trigger correctly are extremely rare and should still have plausible behavior, just remove it.

Test Plan: Grepped for calls.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21077
2020-04-09 13:33:24 -07:00
epriestley
58fbf64a27 Refine handling of "@task" attributes in Diviner
Summary: Ref T13505. See that task for details. When a class has exactly one "@task" block, this API returns a string. Some day, this should be made more consistent.

Test Plan: Viewed a class with exactly one "@task", no more fatal. Viewed classes with zero and more than one "@task" attributes, got clean renderings.

Maniphest Tasks: T13505

Differential Revision: https://secure.phabricator.com/D21062
2020-04-06 11:51:33 -07:00
epriestley
271e104c7e Update DivinerAtomController for a long-ago change to the docblock parser API
Summary: Ref T13505. See that task for discussion.

Test Plan: Ran `diviner generate` locally, found a page fataling on this `strlen()`, applied patch, got a sketchy but not-broken page.

Maniphest Tasks: T13505

Differential Revision: https://secure.phabricator.com/D21061
2020-04-06 11:31:31 -07:00
epriestley
f1d1ec3d77 Add an "isDone" flag to "transaction.search" for Differential inline comments
Summary: See PHI1684. Expose the published state of the "Done" checkbox to the API.

Test Plan: Made API calls on a comment in all four states, got correct published states via the API in all cases.

Differential Revision: https://secure.phabricator.com/D21059
2020-04-05 09:36:15 -07:00
epriestley
33b73d887a If daemon running-as-user setup check fails its query, don't bother with it
Summary:
See <https://discourse.phabricator-community.org/t/upgrade-from-sep-30-2016/3702/>. A user performing an upgrade from 2016 to 2020 ran into an issue where this setup query is overheating.

This is likely caused by too many rows changing state during query execution, but the particulars aren't important since this setup check isn't too critical and will catch the issue eventually. It's fine to just move on if this query fails for any reason.

Test Plan: Forced the query to overheat, loaded setup issues, got overheating fatal. Applied patch, no more fatal.

Differential Revision: https://secure.phabricator.com/D21057
2020-04-03 16:18:55 -07:00
epriestley
1e7cc72cd8 Improve performance when marking commits as unreachable after multiple ref deletions
Summary:
See PHI1688. If many refs with a large amount of shared ancestry are deleted from a repository, we can spend much longer than necessary marking their mutual ancestors as unreachable over and over again.

For example, if refs A, B and C all point near the head of an obsolete "develop" branch and have about 1K shared commits reachable from no other refs, deleting all three refs will lead to us performing 3,000 mark-as-unreachable operations (once for each "<ref, commit>" pair).

Instead, we can stop exploring history once we reach an already-unreachable commit.

Test Plan:
  - Destroyed 7 similar refs simultaneously.
  - Ran `bin/repository refs`, saw 7 entries appear in the `oldref` table.
  - Ran `bin/repository discover` with some debugging statements added, saw sensible-seeming behavior which didn't double-mark any newly-unreachable refs.

Differential Revision: https://secure.phabricator.com/D21056
2020-04-03 13:28:42 -07:00
epriestley
1a59cae743 Update some Phabricator behaviors for changes to Futures
Summary:
Depends on D21053. Ref T11968. Three things have changed:

  - Overseers can no longer use FutureIterator to continue execution of an arbitrary list of futures from any state. Use FuturePool instead.
  - Same with repository daemons.
  - Probably (?) fix an API change in the Harbormaster exec future.

Test Plan:
  - Ran "bin/phd debug task" and "bin/phd debug pull", no longer saw Future-management related errors.
  - The Harbormaster future is easiest to test by just seeing if production works once this change is deployed there.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11968

Differential Revision: https://secure.phabricator.com/D21054
2020-04-03 12:28:16 -07:00
epriestley
067b04aaf1 If HTTP response headers are already sent, don't fiddle with "zlib.output_compression"
Summary:
We write some synthetic HTTP responses inside unit tests. Some responses have an indirect side effect of adjusting "zlib.output_compression", but this adjustment fails if headers have already been output. From a CLI context, headers appear to count as already-output after we write anything to stdout:

```
<?php

echo headers_sent() ? "Y" : "N";
echo "\n";
echo headers_sent() ? "Y" : "N";
echo "\n";
```

This script prints "N", then "Y".

Recently, the default severity of warnings was increased in libphutil; this has been a long-standing warning but now causes test failures.

This behavior is sort of silly but the whole thing is kind of moot anyway. Just skip it if "headers_sent()" is true.

Test Plan: Ran "arc unit --everything", got clean results.

Differential Revision: https://secure.phabricator.com/D21055
2020-04-03 12:24:58 -07:00
Arturas Moskvinas arturas@uber.com
62f5bdbbd2 According to Jira Project keys must start with an uppercase letter, followed by one or more uppercase alphanumeric characters
Summary: Jira allows creating projects which contain number in names, phabricator will not allow such projects but it should

Test Plan: Pasted URL with Jira project which contain number in project name and it was parsed and resolved properly in phabricator

Reviewers: epriestley, Pawka, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21040
2020-03-09 22:04:23 +02:00
epriestley
d0f4554dbe Read both email addresses and Google Account IDs from Google OAuth
Summary:
Ref T13493. Google returns a lower-quality account identifier ("email") and a higher-quality account identifier ("id"). We currently read only "email".

Change the logic to read both "email" and "id", so that if Google ever moves away from "email" the transition will be a bit easier.

Test Plan: Linked/unlinked a Google account, looked at the external account identifier table.

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21028
2020-02-24 13:26:42 -08: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
epriestley
d3f4af4a3a Add more layout constraints to tokenizer CSS to prevent layout issues with Chinese glyphs in Firefox 73
Summary:
Fixes T13495. See that task for details.

Tokenizer tokens which contain Chinese glyphs are slightly taller than normal tokens in Firefox 73, and at some non-100% zoom levels in other browsers.

This cauess the tokenizer list to layout and line break oddly.

Fix this by clamping tokenizer sizes more aggressively. Specifying a `max-height` means they can no longer line wrap, so this also requires more specification of overflow behavior.

Test Plan:
Before:

{F7216435}

After:

{F7216439}

Maniphest Tasks: T13495

Differential Revision: https://secure.phabricator.com/D21026
2020-02-24 08:00:44 -08:00