Summary:
Currently, it's hard to debug performance issues on POST pages. Add flags to stop redirects and always collect profiles.
Also fix an issue with "all" profiles. This feature is mostly just for profiling DarkConsole itself and is rarely used, I think it's been broken for some time. There's no way to get to it with the UI.
NOTE: Some JS workflows don't stop on redirect because they use JS/AJAX redirects.
Test Plan: Enabled options, browsed, got stopped on redirects and had profiles generated. Disabled options and verified redirects and profiles work normally.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2990
Summary:
- Share code between `createinline` and `getcomments`
- Make them both extend the base class.
- Sync up the parameters (this is more or less nonbreaking since the call is ~6 hours old).
Test Plan: Created and fetched inlines via the API.
Reviewers: alanh, vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1500
Differential Revision: https://secure.phabricator.com/D2988
Summary:
See T931. LLVM users would also prefer simpler mail, and have similarly harsh opinions about the current state of affairs.
Allow installs to disable the hint blocks if they don't want them.
Test Plan: Sent myself mail with these settings on/off, got mail with/without the blocks.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T931
Differential Revision: https://secure.phabricator.com/D2968
Summary: This is a minor quality-of-life improvement to prevent D2968 from being as nasty as it is.
Test Plan: Ran unit tests; generated Differential, Maniphest and Diffusion emails and verified the bodies looked sensible.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T931
Differential Revision: https://secure.phabricator.com/D2986
Test Plan: Displayed `.arcconfig` and verified that it is still highlighted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2987
Summary: Paths autocompleter sometimes omits `/`.
Test Plan:
# Go to https://secure.phabricator.com/owners/new/.
# Write `/specs/h` to path.
# Delete the second slash resulting in `/specsh`.
# Don't find `/specshistorytest.html` in autocomplete.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2981
Summary: a silly thing because I was bored
Test Plan: and I said "arc call-conduit", and there were comments
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1500
Differential Revision: https://secure.phabricator.com/D2971
Summary:
Lisk currently behaves in two different ways if you call it like `load("cow")` (throws) versus `load(99999999)` (returns null), where neither ID exists.
This was intended to catch programming errors as distinct from missing data, but in practice the former is very rare and you have to handle the latter in most cases anyway. The case where you pass "0" is particularly confusing. See D2971 for an example.
On the balance, I think this ends up being far more confusing than helpful. Instead, just return NULL if we're sure there's no such object.
Test Plan: Reasoned about program behavior.
Reviewers: alanh, btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2977
Summary:
See T1501. When users mash "save", stop them if they didn't change anything.
Also, don't default-fill the "edit notes" field with the previous notes. This is meant to be more like a commit message for your changes.
Test Plan: Edited a document with no changes, got a dialog. Edited a document with a title change only and a description change only, things worked. Edited a document with a previous "Edit notes", got a blank default fill.
Reviewers: alanh, btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1501
Differential Revision: https://secure.phabricator.com/D2978
Summary: For "accept" and "reject" action, find the time between the action and last time the revision was updated by a new diff. Show it as the responsiveness row.
Test Plan: view it for a couple of engineers.
Reviewers: epriestley, vii
Reviewed By: epriestley
CC: vrana, nh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2970
Summary: Add active-directory domain-based ldap authentication support
Test Plan: Tested on a live install against Active Directory on a Windows Server
Reviewers: epriestley
CC: aran, epriestley
Maniphest Tasks: T1496
Differential Revision: https://secure.phabricator.com/D2966
Summary: I missed this callsite in D2946. Transition it to the new markup cache.
Test Plan: Clicked "show change" on a description edit transaction, got the change instead of a fatal.
Reviewers: alanh, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1502
Differential Revision: https://secure.phabricator.com/D2972
Summary:
See D818 for an older attempt at this. Support code has matured to the point where the patch is pretty straightforward.
@tido, this was a long-standing request from Aditya back in the day.
Test Plan: Used `reparse.php --herald` to send myself a bunch of emails with various patch configurations. Confirmed that limits are respected, reasonable errors arise when they're violated, etc. (Timeout is a little funky but that's out of scope here, I think.)
Reviewers: btrahan
Reviewed By: btrahan
CC: tido, aran
Maniphest Tasks: T456
Differential Revision: https://secure.phabricator.com/D2967
Summary:
- Assigning $cc_phids clobbers the correct value assigned on line 80.
- Remove stack trace noise, the trace is always meaningless and well-known.
- Actually show the original body.
Test Plan: Piped mail to the mail receiver and verified the errors didn't CC revision CCs, no longer had traces, and included the original raw text body.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2969
Summary: Custom logos are 220px in width - make README consistent
Test Plan: Read it.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2954
Summary: See D2955, D2956, D2957.
Test Plan: Read documentation. Ran all these commands from Git Bash and cmd.exe and used the specified editors to edit blocks of text.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1309
Differential Revision: https://secure.phabricator.com/D2958
Summary:
Still not 100% sure what the repro case here is, but we do something similar on line 438 above. One case could be an SVN repo with a subpath specified, I think.
In any case, don't fatal if we're missing the commit.
Test Plan: Loaded some browse views, although I don't have a repro.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2959
Summary: http://www.networksorcery.com/enp/protocol/irc.htm
Test Plan: augment code with an additional debug line (phlog('hi');) so I can see my case was trigged and it will fall through. setup an ill-configured IRC server with ngircd. Configure an ircbot to connect to said ill-configured IRC server. verify ircbot connected to channel. verify in irc bot logs that debug line was invoked.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1452
Differential Revision: https://secure.phabricator.com/D2962
Summary: Allow the GC daemon to collect the new markup cache.
Test Plan: Ran gc daemon in "debug" mode, saw it collect cache entries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2947
Summary:
- See D2945.
- Drop `cache` field from ManiphestTransaction.
- Render task descriptions and transactions through PhabricatorMarkupEngine.
- Also pull the list of macros more lazily.
Test Plan:
- Verified transactions and transaction preview work correctly and interact with cache correctly.
- Verified tasks descriptions and task preview work correctly.
- Verified we don't hit the imagemacro table when we're rendering everything from cache anymore.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2946
Test Plan:
Created diff with a postponed linter and the lint status set to such
via arcanist. Verified lint status showed as postponed in diff view.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1332
Differential Revision: https://secure.phabricator.com/D2932
Summary:
- Old versions of Mercurial give different output for `hg log -- ''` and `hg log`. Just use `hg log`.
- Branch names with spaces can't be specified in `--rev`. I talked with hstuart in #mercurial and apparently am not crazy.
Test Plan:
- Viewed history of a repository.
- Viewed history of a file.
- Viewed branch `m m m m m 2:ffffffffffff (inactive)`
- Learned that you checkout this branch with `hg checkout ':m m m m m 2:ffffffffffff (inactive)'`
Reviewers: dschleimer, btrahan
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1268
Differential Revision: https://secure.phabricator.com/D2950
Summary:
The immediate issue this addresses is T1366, adding a rendering cache to Phriction. For wiki pages with code blocks especially, rerendering them each time is expensive.
The broader issue is that out markup caches aren't very good right now. They have three major problems:
**Problem 1: the data is stored in the wrong place.** We currently store remarkup caches on objects. This means we're always loading it and passing it around even when we don't need it, can't genericize cache management code (e.g., have one simple script to drop/GC caches), need to update authoritative rows to clear caches, and can't genericize rendering code since each object is different.
To solve this, I created a dedicated cache database that I plan to move all markup caches to use.
**Problem 2: time-variant rules break when cached.** Some rules like `**bold**` are time-invariant and always produce the same output, but some rules like `{Tnnn}` and `@username` are variant and may render differently (because a task was closed or a user is on vacation). Currently, we cache the raw output, so these time-variant rules get locked at whatever values they had when they were first rendered. This is the main reason Phriction doesn't have a cache right now -- I wanted `{Tnnn}` rules to reflect open/closed tasks.
To solve this, I split markup into a "preprocessing" phase (which does all the parsing and evaluates all time-invariant rules) and a "postprocessing" phase (which evaluates time-variant rules only). The preprocessing phase is most of the expense (and, notably, includes syntax highlighting) so this is nearly as good as caching the final output. I did most of the work here in D737 / D738, but we never moved to use it in Phabricator -- we currently just do the two operations serially in all cases.
This diff splits them apart and caches the output of preprocessing only, so we benefit from caching but also get accurate time-variant rendering.
**Problem 3: cache access isn't batched/pipelined optimally.** When we're rendering a list of markup blocks, we should be able to batch datafetching better than we do. D738 helped with this (fetching is batched within a single hunk of markup) and this improves batching on cache access. We could still do better here, but this is at least a step forward.
Also fixes a bug with generating a link in the Phriction history interface ($uri gets clobbered).
I'm using PHP serialization instead of JSON serialization because Remarkup does some stuff with non-ascii characters that might not survive JSON.
Test Plan:
- Created a Phriction document and verified that previews don't go to cache (no rows appear in the cache table).
- Verified that published documents come out of cache.
- Verified that caches generate/regenerate correctly, time-variant rules render properly and old documents hit the right caches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1366
Differential Revision: https://secure.phabricator.com/D2945
Summary:
Currently, a change may affect a very large number of paths. When we run the OwnersWorker on it, we'll execute a query which looks up packages for the paths. This may exceed "max_allowed_packet". Instead, break the list of paths into smaller chunks.
This is mostly to unblock r4nt / llvm, I'm going to add a more finessed approach to array_chunk() shortly.
Test Plan: Ran `reparse.php --owners` on a revision which affected packages, verified results are the same before and after the change. Set chunk size to 1, verified query results aggregated properly.
Reviewers: btrahan, jungejason, nh
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2943
Summary:
Certain types of things we should be storing in edges (notably, Task X depends on Task Y) should always be acyclic. Allow `PhabricatorEdgeEditor` to enforce this, since we can't correctly enforce it outside of the editor without being vulnerable to races.
Each edge type can be marked acyclic. If an edge type is acyclic, we perform additional steps when writing new edges of that type:
- We acquire a global lock on the edge type before performing any reads or writes. This ensures we can't produce a cycle as a result of a race where two edits add edges which independently do not produce a cycle, but do produce a cycle when combined.
- After performing writes but before committing transactions, we load the edge graph for each acyclic type and verify that it is, in fact, acyclic. If we detect cycles, we abort the edit.
- When we're done, we release the edge type locks.
This is a relatively high-complexity change, but gives us a simple way to flag an edge type as acyclic in a robust way.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2940
Summary:
Currently, multiple unit tests that acquire global locks will interfere with each other. Namespace the locks so they don't.
(Possibly we should also rename this to PhabricatorStorageNamespaceLock or something since it's not really global any more, but that's kind of unwieldy...)
Test Plan: Acquired locks with --trace and verified they were namespaced properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2939
Summary:
In unit tests which use fixtures, we open transactions on every connection we establish. However, since we don't track connections that are established with "$force_new" (currently, only GlobalLock connections) we never close these transactions normally.
Instead of not tracking these connections, track them using unique keys so we'll never get a cache hit on them.
Test Plan: Built unit tests on top of this, had them stop dying from unclosed transactions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2938
Summary:
A later diff adds unit tests against edges, but we need real objects to connect with edges. Add some trivial objects to the Harbormaster database to compliment the similar HarbormasterScratchTable.
On its own, this does nothing interesting.
Test Plan: Built unit tests on this in a followup.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2937
Summary: GitHub moved these pages around and made it easier to find your application list.
Test Plan: Read docs, verified links are correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2941
Summary:
We have a bit more copy-paste than we need, consolidate a bit.
(Also switch Mercurial to download git diffs, which it handles well; we use them in "arc patch".)
Test Plan:
- Downloaded a raw diff from Differential.
- Downloaded a raw change from Diffusion.
- Downloaded a raw file from Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2942
Summary: This is a fairly contentious default that we can easily move to configuration.
Test Plan: Changed the default, changed my user setting, reverted my user setting, verified the "settings" page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2935
Summary: Currently, Diffusion supports highlighting of line ranges passed in the URI. It would be helpful to be able to highlight multiple line ranges.
Test Plan: Accessed directly via URL in my sandbox. Seems to work. I'm not sure what other components use this functionality, but this change should be backwards compatible.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D2921
Summary:
Brazenly copied from differential.close. Ulterior motive
is to be able to automate discarding of useless commits from
arcanist testing.
Test Plan:
Called method in phabricator sandbox. Revisions were
successfully abandoned.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2928
Summary: Depends on D2926. Adds a simple CLI client for Aphlict to make it easier to debug stuff.
Test Plan: Ran client, saw debug messages.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2927
Summary:
See discussion here:
https://secure.phabricator.com/chatlog/channel/%23phabricator/?at=21186
Basically, MySQL usually raises a good error if we exceed "max_allowed_packet":
EXCEPTION: (AphrontQueryException) #1153: Got a packet bigger than 'max_allowed_packet' bytes
But sometimes it gives us a #2006 instead. This is documented, at least:
>"With some clients, you may also get a Lost connection to MySQL server during query error if the communication packet is too large."
http://dev.mysql.com/doc/refman//5.5/en/packet-too-large.html
Try to improve the error message to point at this as a possible explanation.
Test Plan: Faked an error, had it throw, read exception message. See also chatlog.
Reviewers: btrahan, skrul
Reviewed By: skrul
CC: aran
Differential Revision: https://secure.phabricator.com/D2923
Summary: See D2924.
Test Plan: Ran locks with blocking timeouts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2925
Summary:
accessibility covers not only a given post but also the various "published" views.
to keep the code relative clean, this diff also splits up the post list controller logic quite a bit. this also feels like good preparation for some other work around introducing "blogs" which are collections of published posts from bloggers with some fancy features around that.
Test Plan: clicked around various parts of the Phame application as a logged in user, a logged in user with no personal posts, and without any user logged in at all. various views all seemed reasonable.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D2898
Summary:
See T1448. If this file isn't present, just move on instead of failing, since it's a (sort of) legitimate repository state.
Also fix some silliness a little later that got introduced in refactoring, I think.
Test Plan: Added an external to my test repo and removed ".gitmodules". Verified that the directory is now viewable after this patch.
Reviewers: btrahan, davidreuss, jungejason
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1448
Differential Revision: https://secure.phabricator.com/D2922
