Develop rivigo custom -> rivigo-custom #12
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 30 commits
April 3, 2020 12:24
…tput_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
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
…e 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
…ith 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
…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
…rser 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
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
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
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
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
… 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
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
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
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
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
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
…n 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
…al 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
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
…ecution 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
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
…tent-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
…ility 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
…cept-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
…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
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
…nd 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
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
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
…gine 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
…nditions Summary: See <phacility#854>. In some situations, `line-break: anywhere` produces better behavior than `word-break: break-all`. It never appears to produce worse behavior. Test Plan: - Break behavior changes if a line contains "<span />" elements caused by syntax highlighting. This CSS adjustment only appears to apply to text with internal "<span />" elements. - This specifically impacts certain internal breakpoints adjacent to punctuation, so the test case is highly specific. Generic test cases with latin word characters do not evidence any behavioral changes. - This change appears to have no impact on Safari, which uses the better behavior in all cases. - Before Patch: In Firefox and Chrome, this specific change breaks awkwardly. There is more room for text to fit on the broken line: Firefox {F7480567} Chrome {F7480568} - After Patch: Firefox and Chrome break the line better. Here's Firefox: {F7480569} - Additional context: Safari Behavior (Unchanged) {F7480570} Chrome with no highlighting (desirable behavior). Firefox does the same thing. {F7480571} Also tested other cases, which seem never-worse in any browser. {F7480574} Differential Revision: https://secure.phabricator.com/D21247
Summary: Ref T11401. Ref T13513. This paves the way for more comment actions, particularly an edit-after-submit action. Test Plan: Took all actions from menus, via mouse and via keyboard (where applicable). Maniphest Tasks: T13513, T11401 Differential Revision: https://secure.phabricator.com/D21244
Summary: Ref T13513. Ref T11401. Support viewing raw remarkup for inlines. Test Plan: Viewed raw remarkup on inlines. Maniphest Tasks: T13513, T11401 Differential Revision: https://secure.phabricator.com/D21246
…ctly Summary: Ref T13513. Support direct text selection for inlines. This is currently just an alternate way to get to the same place as using line numbers, but can preserve offset/range information in the future. Test Plan: - Selected some text, hit "c", clicked "New Inline Comment", got sensible comments on both sides of a diff in Safari, Chrome, and (with limitations) Firefox. - Caveats: no unified support, doesn't work across lines in Firefox. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21248
Summary: Ref T13513. - Firefox represents multiple selected rows as a discontinuous range. Accommodate this. - Unified diffs don't have a "copy" marker. Do something sort-of-reasonable for them. Test Plan: - Selected multiple lines of content in Firefox, got an option to add a comment. - Selected content in unified mode, got an option to add a comment. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21249
… comments
Summary:
Ref T13513. When a user selects a text range and uses "New Inline Comment" to create a comment directly from a range, store the offset information alongside the comment.
When hovering the comment, highlight the original range.
Test Plan: {F7480926, size=full}
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21250
…ne offsets Summary: Ref T13513. Currently, when creating an inline by selecting a line range, slightly careless handling leads to an inline with "0" offsets (by passing "undefined" to the server). This causes the block to highlight every line except the last one as fully bright, which is incorrect. An inline with "0" offsets and an inline with no offsets are different. Be more careful about passing offsets around and rendering them. Test Plan: - Used the line numbers to add an inline to lines 4-8 of a change. - Hovered the inline. - Saw all four lines marked as "dull"-highlighted (previously: three bright lines, one dull line). Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21252
… response arrives Summary: Ref T13513. This fixes a bug where clicking a line number, then clicking "Cancel" causes the paths panel to briefly update with an extra inline comment counted on the file. Test Plan: - Clicked a line number. - Typed some text. - Clicked "Cancel". - Before patch: paths panel flashes "1". - After patch: paths panel stays stable. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21253
… behaviors interact Summary: Ref T13513. In Safari, do this: - view a 2-up diff with content on both sides; - select more than one line on the right side; and - use your mouse to click "New Inline Comment" in the context menu that pops up. The mousedown event for the "New Inline Comment" click removes the "copy selection" behavior and creates a flash where both sides of the diff are selected. This doesn't happen with (most) normal clicks because mouse down on a non-grabbable element clears the document selection. To avoid this, don't reset the copy selection behavior if the user mouses down on an "<a />". This might not be robust, but seems simple and plausible as a fix. Test Plan: - See above. - Before patch: flash of overbroad selection when clicking "New Inline Comment". - After patch: no selection flash. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21254
…ents Summary: Ref T13513. Currently, clicking inline or changeset headers eats the click events. This doesn't serve any clear purpose, and means these clicks do not clear text selections from the document, which is unusual. Test Plan: - Selected some text in a diff. - Clicked a changeset header to select it. - Before patch: text selection and context menu were retained. - After patch: text selection was cleared and context menu was dismissed. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21255
Summary: Ref T13513. Give selected inlines a selection state and visual cues which are similar to the changeset selection state. Also fix a couple of minor issues with select interactions and offset comments. Test Plan: Selected inlines, saw obvious visual cues. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21256
Summary: Ref T13513. The way I'm highlighting lines won't work for Jupyter notebooks or other complex content blocks, and I don't see an obvious way to make it work that's reasonably robust. However, we can just ignore the range behavior for complex content and treat the entire block as selected. This isn't quite as fancy as the source behavior, but pretty good. Also, adjust unified diff behavior to work correctly with highlighting and range selection. Test Plan: - Used range selection in a Jupyter notebook, got reasonable behavior (range is treated as "entire block"). - Used range selection in a unified diff, got equivalent behavior to 2-up diffs. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21257
Summary: Ref T13529. Now that instances can be renamed, an instance may have multiple valid SSH usernames and the preferred SSH username may not be the intenal instance name. `PhacilitySiteSource` should already always set `diffusion.ssh-username` correctly, to the current preferred SSH username (which may be "new-name" after a rename from "old-name"), so we should never be able to reach this code without an accurate `diffusion.ssh-username` value available. The code to resolve names into instances also already works for both "ssh old-name@..." and "ssh new-name@...". So I believe this code has no beneficial effects and only causes harm: it may force us to return "old-name" when falling through would correctly return "new-name". Test Plan: - Previously: renamed an instance, then SSH'd to it using both the old and new names. Both work. - Previously: verified that `diffusion.ssh-username` is set correctly after a rename. - Verified that Diffusion "Clone" UI now shows "new-name" after an instance rename. - The real question here is: does this break something I'm not thinking of? And the change probably has to go to production to answer that. Maniphest Tasks: T13529 Differential Revision: https://secure.phabricator.com/D21259
…evisions Summary: Ref T13513. See PHI1734, which raises a concern about the performance of large revisions near the 100-change threshold. Currently, `getInlines()` is called whenever the scroll position transitions between two changesets, and it performs a relatively complicated DOM scan to lift inlines out of the document. This shows up as taking a small but nontrivial amount of time in Firefox profiles and should be safely memoizable. Test Plan: - Under Firefox profiling, scrolled through a large revision. - Before change: `getInlines()` appeared as the highest-cost thing we're explicitly doing on profiles. - After change: `getInlines()` was no longer meaningfully represented on profiles. - Created inlines, edited inlines, etc. Didn't identify any broken behavior. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21261
Summary: Ref T13513. The on-hover-inline reticle has switched over to have cell-based behavior. Switch the on-hover-line-number reticle to use the same behavior. Also, clean up the dirty/redraw loop slightly: we no longer need to dirty on resize, and we don't need to redraw if the range isn't actually dirty. Test Plan: Highlighted lines and line ranges. Hovered over inlines. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21262
Summary: Ref T13513. On `secure`, I caught an issue where inlines may be accessed directly before they're constructed. Instead, access them through the relevant accessor. Test Plan: Will deploy. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21263
…sers Summary: On the "New User" web workflow, if you use an invalid email address, you get a failure with an empty message. Test Plan: - Before: Tried to create a new user with address "asdf". Got no specific guidance. - After: Got specific guidance about email address formatting and length. Differential Revision: https://secure.phabricator.com/D21264
…y widths Summary: Fixes T13536. See that task for discussion. Older versions of MySQL (roughly, prior to 8.0.19) emit "int(10)" types. Newer versions emit "int" types. Accept these as equivalent. Test Plan: Ran `bin/storage upgrade --force` against MySQL 8.0.11 and 8.0.20. Got clean adjustment lists on both versions. Maniphest Tasks: T13536 Differential Revision: https://secure.phabricator.com/D21265
Summary:
Ref T13538. See PHI1739. Synthetic Git commits with no author and/or no commit message currently extract `null` and then fail to parse.
Ideally, we would carefully distinguish between `null` and empty string. In practice, that requires significant schema changes (these columns are non-nullable and have indexing requirements) and these cases are degenerate. These commits are challenging to build and can not normally be constructed with `git commit`.
At least for now, merge the `null` cases into the empty string cases so we can survive import.
Test Plan:
- Constructed a commit with no author and no commit message using the approach described in T13538; pushed and parsed it.
- Before: fatals during identity selection and storing the commit message (both roughly NULL inserts into non-null columns).
- After: clean import.
This produces a less-than-ideal UI in Diffusion, but it doesn't break anything:
{F7492094}
Maniphest Tasks: T13538
Differential Revision: https://secure.phabricator.com/D21266
…on end-of-file modifications Summary: Fixes T13539. See that task for discussion and a reproduction case. This algorithm currently counts "\ No newline at end of file" lines as though they were normal source lines. This can cause offset issues in the rare case that a diff contains two of these lines (for each side of the file) and has changes between them (because the last line of the file was modified between the diffs). Instead, don't count "\" as a display line. Test Plan: - See T13539 and PHI1740. - Before: got fatals on the "wild" diff and the synthetic simplified version. - After: clean intradiff rendering in both cases. Maniphest Tasks: T13539 Differential Revision: https://secure.phabricator.com/D21267
Summary: Ref T13513. The code which added "r" and "R" to the inline menu accidentally discarded the difference between the keystrokes. Test Plan: - Clicked an inline, pressed "r", got new empty inline (previously: inline with quote). - Clicked an inline, pressed "R", got a new quoted inline. - Repeated steps with the menu items, got the expected behaviors. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21268
… plans Summary: See PHI1743. If a build has no initiator PHID, the rendering pathway incorrectly tries to access a handle for it anyway. Test Plan: - Set a build to have no initiator PHID. - Viewed the build plan for the build. - Before: fatal when trying to access the `null` handle. - After: clean build plan rendering. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D21269
Summary: See PHI1745. This callsite for "ChangesetParser" was not properly updated for recent changes. Test Plan: - Set `metamta.differential.inline-patches` to 100. - Created a new revision with a small (<100 line) diff, with at least one reviewer. - Ran `bin/phd debug` and observed outbound mail queue with `bin/mail list-outbound`. - Before: fatal when trying to generate the inline changes for mail. - After: clean mail generation. Differential Revision: https://secure.phabricator.com/D21270
…ses in the worker queue Summary: See PHI1745. Under PHP7, errors raised as Throwable miss this "generic exception" logic and don't increment their failure count. Instead, treat any "Throwable" we don't recognize like any "Exception" we don't recognize. Test Plan: - Under PHP7, caused a worker task to raise a Throwable (e.g., call to undefined method, see D21270). - Ran `bin/worker execute --id ...`. - Before: worker failed, but did not increment failure count. - After: worker fails and increments failure count as it would for other types of unknown error. Differential Revision: https://secure.phabricator.com/D21271
Summary: See <https://discourse.phabricator-community.org/t/how-to-override-localhost-localdomain-in-email-message-id/3876/>. Currently, Phabricator generates a "Message-ID" only in a subset of cases (roughly: when the message is first-in-thread and we expect the thread may have more than one message). In cases where it does not generate a message ID, it expects the SMTP server to generate one for it. Servers will generally do this, and some ONLY do this (that is, they ignore IDs from Phabricator and replace them). Thus, several pieces of configuration control whether Phabricator attempts to generate a "Message-ID" at all. The PHPMailer code has fallback behavior which generates a "<random>@localhost.localdomain" message ID. This is never desirable and ignores Phabricator-level configuration that Message IDs should not be generated. For now, remove this code: it is never the desired behavior and sometimes explicitly contradicts the intent of configuration. Possibly, a better change may be to make Phabricator always generate a message ID in cases where it isn't forbidden from doing so by configuration. However, that's a more complicated change and it's not clear if/when it would produce better behavior, so start here for now. Test Plan: Confirmed by affected user (see linked thread). Differential Revision: https://secure.phabricator.com/D21272
upstream
Develop rivigo custom
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.