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
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
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
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
Summary:
Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely.
I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction.
Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T3498
Differential Revision: https://secure.phabricator.com/D20185
Summary: Ref T13250. See D20149. In a number of cases, we use `setQueryParams()` immediately after URI construction. To simplify this slightly, let the constructor take parameters, similar to `HTTPSFuture`.
Test Plan: See inlines.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20151
Summary: See D20136. This method is sort of inherently bad because it is destructive for some inputs (`x=1&x=2`) and had "PHP-flavored" behavior for other inputs (`x[]=1&x[]=2`). Move to explicit `...AsMap` and `...AsPairList` methods.
Test Plan: Bit of an adventure, see inlines in a minute.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20141
Summary:
Ref T13244. See PHI1055. (Earlier, see D20091 and PHI1047.) Previously, we expanded the Owners package autoreview rules from "Yes/No" to several "Review (Blocking) If Non-Owner Author Not Subscribed via Package" kinds of rules. The sky didn't fall and this feature didn't turn into "Herald-in-Owners", so I'm comfortable doing something similar to the "Audit" rules.
PHI1055 is a request for a way to configure slightly different audit behavior, and expanding the options seems like a good approach to satisfy the use case.
Prepare to add more options by moving everything into a class that defines all the behavior of different states, and converting the "0/1" boolean column to a text column.
Test Plan:
- Created several packages, some with and some without auditing.
- Inspected database for: package state; and associated transactions.
- Ran the migrations.
- Inspected database to confirm that state and transactions migrated correctly.
- Reviewed transaction logs.
- Created and edited packages and audit state.
- Viewed the "Package List" element in Diffusion.
- Pulled package information with `owners.search`, got sensible results.
- Edited package audit status with `owners.edit`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20124
Summary:
Ref T13244. See PHI1057. Currently, if you're a member of a lot of projects/packages, you can end up with a very large `commit.authorPHID IN (...)` clause in part of the "Active Audits" query, since your `alice` token in "Responsible Users: alice" expands into every package and project you can audit on behalf of.
It's impossible for a commit to be authored by anything but a user, and evidence in PHI1057 suggests this giant `IN (...)` list can prevent MySQL from making effective utilization of the `<authorPHID, auditStatus, ...>` key on the table.
Prefilter the list of PHIDs to only PHIDs which can possibly author a commit.
(We'll also eventually need to convert the `authorPHIDs` into `identityPHIDs` anyway, for T12164, and this moves us slightly toward that.)
Test Plan: Loaded "Active Audits" before and after change, saw a more streamlined and sensible `authorPHID IN (...)` clause afterwards.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20129
Summary:
Fixes T13192. See PHI1015. When you deactivate a repository, we currently stop serving it.
This creates a problem for intracluster sync, since new nodes can't sync it. If nothing else, this means that if you "ship of theseus" your cluster and turn nodes over one at a time, you will eventually lose the entire repository. Since that's clearly a bad outcome, support sync.
Test Plan:
Testing this requires a "real" cluster, so I mostly used `secure`.
I deactivated rGITTEST and ran this on `secure002`:
```
./bin/repository thaw --demote secure002.phacility.net --force GITTEST && ./bin/repository update GITTEST
```
Before the patch, this failed:
```
[2019-01-31 19:40:37] EXCEPTION: (CommandException) Command failed with error #128!
COMMAND
git fetch --prune -- 'ssh://172.30.0.64:22/diffusion/GITTEST/' '+refs/*:refs/*'
STDOUT
(empty)
STDERR
Warning: Permanently added '172.30.0.64' (RSA) to the list of known hosts.
phabricator-ssh-exec: This repository ("rGITTEST") is not available over SSH.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
```
After applying (a similar patch to) this patch to `secure001`, the sync worked.
I'll repeat this test with the actual patch once this deploys to `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13192
Differential Revision: https://secure.phabricator.com/D20077
Summary: Now that we have a nice function for this, use it to simplify some code.
Test Plan: Ran through the Duo enroll workflow to make sure signing still works.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20053
Summary:
Ref T13242. See <https://discourse.phabricator-community.org/t/out-of-range-value-for-column-deviceversion/2218>.
The synchronization log column is `uint32?` and `-1` doesn't go into that column.
Since we're only using `-1` for convenience to cheat through `$max_version > $this_version` checks, use `null` instead and make the checks more explicit.
Test Plan: Reproducing this is a bit tricky and I cheated fairly heavily to force the code down this pathway without actually building a multi-device cluster, but I did reproduce the original exception, apply the patch, and observe that it fixed things.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20047
Summary:
In ~2012, the first of these options was added because someone who hates dogs and works at Asana also hated `[Differential]` in the subject line. The use case there was actually //removing// the text, not changing it, but I made the prefix editable since it seemed like slightly less of a one-off.
These options are among the dumbest and most useless config options we have and very rarely used, see T11760. A very small number of instances have configured one of these options.
Newer applications stopped providing these options and no one has complained.
You can get the same effect with `translation.override`. Although I'm not sure we'll keep that around forever, it's a reasonable replacement today. I'll call out an example in the changelog to help installs that want to preserve this option.
If we did want to provide this, it should just be in {nav Applications > Settings} for each application, but I think it's wildly-low-value and "hack via translations" or "local patch" are entirely reasonable if you really want to change these strings.
Test Plan: Grepped for `subject-prefix`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19993
Summary:
See PHI1014. We may not have Identities if you race the import pipeline, or in some other cases which are more "bug" / "missing migration"-flavored.
Load the commit data so we can fall back to it if we don't have identities.
Test Plan:
- Wiped out all my identities with `UPDATE ... SET authorIdentityPHID = NULL WHERE ...`.
- Before change: blame fataled with `Attempting to access attached data on PhabricatorRepositoryCommit (via getCommitData()), but the data is not actually attached.`.
- After change: blame falls back gracefully.
- Restored identities with `bin/repository rebuild-identities`, checked blame again.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19958
Summary:
Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.
`PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.
The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.
Try to clean this up:
- Stop requiring `willRenderTimeline()` to be implemented.
- Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
- Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
- If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.
This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.
Test Plan:
- Viewed audits, revisions, and mocks with inline comments.
- Used "Show Older" to page a revision back in history (this is relevant for "View Data").
- Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19918
Summary:
Ref T13217. See <https://discourse.phabricator-community.org/t/unsafe-raw-string-warnings-while-importing-git-commits/2191>.
Hunt down and fix two more `qsprintf()` things.
I just converted the "performance optimization" into a normal, safe call since we're dealing with far less SVN stuff nowadays and the actual issue has been lost in the mists of time. If it resurfaces, we can take another look.
Test Plan: Imported some commits, no longer saw these warnings in the daemon logs.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19869
Summary:
Ref T13222. See PHI995. Before making a change to inline rendering, consolidate this code for generating the "alice added inlines comments." and "alice marked X inlines as done." transactions.
Both Differential and Diffusion have four very similar chunks of code. Merge them into shared methods and reduce code duplication across the methods.
(In the next change, I plan to hide the "done" story when the mark affects your own inline, since users marking their own inlines as "done" is generally not very interesting or useful.)
Test Plan: As author and reviewer/auditor, added inlines in Differential and Diffusion. As author, marked own and others inlines as done and undone. Got sensible transaction rendering and persistence of "Done".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19858
Summary: Depends on D19856. Ref T13222. See D19829. Make access to "Track Only" slightly cleaner and more consistent..
Test Plan: Set, edited, and removed "Track Only" settings for a repository. Saw sensible persistence and display behaviors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19857
Summary:
Ref T13222. See D19829. We're inconsistent about using `getDetail()/setDetail()` to do some ad-hoc reads. Put this stuff in proper accessor methods.
Also a couple of text fixes from D19850.
Test Plan: Set, edited, and removed autoclose branches from a repository. Got sensible persistence and rendering behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19856
Summary:
See <https://discourse.phabricator-community.org/t/cannot-accept-commits-in-audit/2166/>.
In D19842, I changed `PhabricatorEditField->shouldGenerateTransactionsFromComment()`.
- Previously, it bailed on `getIsConduitOnly()`.
- After the patch, it bails on a missing `getCommentActionLabel()`.
The old code was actually wrong, and it was previously possible to apply possibly-invalid actions in some cases (or, at least, sneak them through this layer: they would only actually apply if not validated properly).
In practice, it let a different bug through: we sometimes loaded commits without loading their audit authority, so testing whether the viewer could "Accept" the commit or not (or take some other actions like "Raise Concern") would always fail and throw an exception: "Trying to access data not attached to this object..."
Fixing the insufficiently-strict transaction generation code exposed the "authority not attached" bug, which caused some actions to fail to generate transactions.
This appeared in the UI as either an unhelpful error ("You can't post an empty comment") or an action with no effect. The unhelpful error was because we show that error if you aren't taking any //other// actions, and we wouldn't generate an "Accept" action because of the interaction of these bugs, so the code thought you were just posting an empty comment.
Test Plan: Without leaving comments, accepted and rejected commits. No more error messages, and actions took effect.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: stephan.senkbeil, hskiba
Differential Revision: https://secure.phabricator.com/D19845
Summary:
Depends on D19831. Ref T13216. See PHI908. Allegedly, a user copied a large repository into itself and then pushed it. Great backup strategy, but it can create headaches for administrators.
Allow a "maximum paths you can touch with one commit" limit to be configured, to make it harder for users to make this push this kind of commit by accident.
If you actually intended to do this, you can work around this by breaking your commit into pieces (or temporarily removing the limit). This isn't a security/policy sort of option, it's just a guard against silly mistakes.
Test Plan: Set limit to 2, tried to push 3 files, got rejected. Raised limit, pushed changes successfully.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19839
Summary: Depends on D19830. Ref T13216. See PHI908. See PHI750. See PHI885. Allow users to configure a filesize limit, and allow them to adjust the clone/fetch timeout.
Test Plan:
{F6021356}
- Configured a filesize limit and pushed, hit it. Made the limit larger and pushed, change went through.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19831
Summary:
Depends on D19829. Ref T13216. See PHI908. The current implementation is kind of a lot to live in `CommitHookEngine` and will likely fail if `git diff-tree` produces more than 2GB of output.
Pull it out and make it slightly more robust against enormous commits. It's probably limited by this, now:
```
implode("\n", $every_path)
```
We could replace that with some `PhutilReverseRopeSource` primitive or something but since we don't have one of those and it seems unlikely that we'll hit this case in practice, I left it here for now with just the easy stuff converted to be stream-oriented.
Test Plan:
Used this script to test the query against various commits, got good results:
```
<?php
require_once 'scripts/init/init-script.php';
$viewer = PhabricatorUser::getOmnipotentUser();
$repository = id(new PhabricatorRepositoryQuery())
->setViewer($viewer)
->withCallsigns(array('P'))
->executeOne();
var_dump(
id(new DiffusionLowLevelFilesizeQuery())
->setRepository($repository)
->withIdentifier($argv[1])
->execute());
```
Used this to find large commits in history and pull filesizes (worked great, although our largest commit only touches a couple thousand paths):
```
for hash in `git log --format=%H`; do echo -n $hash; echo -n ' '; git diff-tree -r --no-commit-id $hash | wc -l | awk '{print $1}'; done | awk '{print $2 " " $1}' | sort -n
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19830
Summary: Depends on D19826. Ref T13216. We have a fair number of options here; add some groups so the "Build" stuff can go in a little subcategory and such.
Test Plan: {F6020896}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19827
Summary:
Ref T13222. See PHI683. Currently, you can "Change subtype..." via Conduit and the bulk editor, but not via the comment action stack or edit forms.
In PHI683 an install is doing this often enough that they'd like it to become a first-class action. I've generally been cautious about pushing this action to become a first-class action (there are some inevitable rough edges and I don't want to add too much complexity if there isn't a use case for it) but since we have evidence that users would find it useful and nothing has exploded yet, I'm comfortable taking another step forward.
Currently, `EditEngine` has this sort of weird `setIsConduitOnly()` method. This actually means more like "this doesn't show up on forms". Make it better align with that. In particular, a "conduit only" field can already show up in the bulk editor, which is goofy. Change this to `setIsFormField()` and convert/simplify existing callsites.
Test Plan:
There are a lot of ways to reach EditEngine so this probably isn't entirely exhaustive, but I think I got pretty much anything which is likely to break:
- Searched for `setIsConduitOnly()` and `getIsConduitOnly()`, converted all callsites to `setIsFormField()`.
- Searched for `setIsLockable()`, `setIsReorderable()` and `setIsDefaultable()` and aligned these calls to intent where applicable.
- Created an Almanac binding.
- Edited an Almanac binding.
- Created an Almanac service.
- Edited an Almanac service.
- Edited a binding property.
- Deleted a binding property.
- Created and edited a badge.
- Awarded and revoked a badge.
- Created and edited an event.
- Made an event recurring.
- Created and edited a Conpherence thread.
- Edited and updated the diff for a revision.
- Created and edited a repository.
- Created and disabled repository URIs.
- Created and edited a blueprint.
- Created and edited tasks.
- Created a paste, edited/archived a paste.
- Created/edited/archived a package.
- Created/edited a project.
- Made comments.
- Moved tasks on workboards via comment action stack.
- Changed task subtype via comment action stack.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19842
Summary:
Ref T13216. I want to add some new management options to repositories (e.g., filesize limit, clone timeouts). Before adding new stuff here, update the UI to a full-width, Phortune-style UI.
This partially reverts D18523. About a year ago, several UIs got converted to fixed-width (repository management, config, settings, instance management in SAAS). I didn't think these were good changes and have never really gotten used to them. The rationale wasn't clear to me and these changes just felt like "be more like GitHub". I think usability is significantly worse, e.g. actions are now hidden inside button menus instead of immediately visible.
Phortune also got converted less dramatically to a full-width-with-menu UI, which I like much better. Adjust repository management to use that UI style instead of the fixed-width style.
Test Plan:
{F6020884}
Viewed every panel, including the Subversion panel.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19826
Summary:
Depends on D19816. Ref T13216. See PHI908. See PHI750. In a few cases, users have pushed multi-gigabyte files full of various things that probably shouldn't be version controlled. This tends to create various headaches.
Add support for limiting the maximum size of any object. Specifically, we:
- list all the objects each commit touches;
- check their size after the commit applies;
- if it's over the limit, reject the commit.
This change doesn't actually hook the limit up (the limit is always "0", i.e. unlimited), and doesn't have Mercurial or SVN support. The actual parser bit would probably be better in some other `Query/Parser` class eventually, too. But it at least roughly works.
Test Plan:
Changed the hard-coded limit to other values, tried to push stuff, got sensible results:
```
$ echo pew >> magic_missile.txt && git commit -am pew && git push
[master 98d07af] pew
1 file changed, 1 insertion(+)
# Push received by "local.phacility.net", forwarding to cluster host.
# Acquiring write lock for repository "spellbook"...
# Acquired write lock immediately.
# Acquiring read lock for repository "spellbook" on device "local.phacility.net"...
# Acquired read lock immediately.
# Device "local.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local.phacility.net".
Counting objects: 49, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (48/48), done.
Writing objects: 100% (49/49), 3.44 KiB | 1.72 MiB/s, done.
Total 49 (delta 30), reused 0 (delta 0)
remote: +---------------------------------------------------------------+
remote: | * * * PUSH REJECTED BY EVIL DRAGON BUREAUCRATS * * * |
remote: +---------------------------------------------------------------+
remote: \
remote: \ ^ /^
remote: \ / \ // \
remote: \ |\___/| / \// .\
remote: \ /V V \__ / // | \ \ *----*
remote: / / \/_/ // | \ \ \ |
remote: @___@` \/_ // | \ \ \/\ \
remote: 0/0/| \/_ // | \ \ \ \
remote: 0/0/0/0/| \/// | \ \ | |
remote: 0/0/0/0/0/_|_ / ( // | \ _\ | /
remote: 0/0/0/0/0/0/`/,_ _ _/ ) ; -. | _ _\.-~ / /
remote: ,-} _ *-.|.-~-. .~ ~
remote: * \__/ `/\ / ~-. _ .-~ /
remote: \____(Oo) *. } { /
remote: ( (..) .----~-.\ \-` .~
remote: //___\\ \ DENIED! ///.----..< \ _ -~
remote: // \\ ///-._ _ _ _ _ _ _{^ - - - - ~
remote:
remote:
remote: OVERSIZED FILE
remote: This repository ("spellbook") is configured with a maximum individual file size limit, but you are pushing a change ("98d07af863e799509e7c3a639404d216f9fc79c7") which causes the size of a file ("magic_missile.txt") to exceed the limit. The commit makes the file 317 bytes long, but the limit for this repository is 1 bytes.
remote:
# Released cluster write lock.
To ssh://local.phacility.com/source/spellbook.git
! [remote rejected] master -> master (pre-receive hook declined)
error: failed to push some refs to 'ssh://epriestley@local.phacility.com/source/spellbook.git'
```
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: joshuaspence
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19817
Summary:
Ref T13216. See PHI943. When you have a large number of cluster bindings for a repository, the UI sorting can be a bit hard to manage.
One install that regularly cycles repository cluster devices had a couple dozen older disabled bindings, with the enabled bindings intermingled.
Sort the UI:
- enabled devices come first;
- in each group, sort by name.
Test Plan: Mixed disabled/enabled bindings, loaded {nav Diffusion > Repository > Storage} page with clustering configured. Before: relatively unhelpful sort order. After: more intuitive sort order.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19813
Summary:
Depends on D19814. Ref T13216. See PHI885. For various eldritch reasons, `git fetch` can hang. Although we'd probably like to fix this with `git fetch --require-sustained-network-transfer-rate=512KB/5s` or similar, that flag doesn't exist and we don't have a reasonable way to build it.
Short of that, move toward formalizing a repository "copy time limit": the longest amount of time anything may spend trying to make a copy of this repository.
This grows out of the existing intracluster sync limit, which is effectively the same thing. Here, apply it to `git clone` and `git fetch` in Drydock working copy construction, too. A future change may make it configurable.
Test Plan:
- Set the limit to 0.001.
- Tried to build and lease working copies, got sensible timeout errors (see D19815).
```
<Activation Failed> Lease activation failed: [CommandException] Command killed by timeout after running for more than 0.001 seconds.
COMMAND
ssh '-o' 'LogLevel=quiet' '-o' 'StrictHostKeyChecking=no' '-o' 'UserKnownHostsFile=/dev/null' '-o' 'BatchMode=yes' -l '********' -p '2222' -i '********' '127.0.0.1' -- '(cd '\''/var/drydock/workingcopy-163/repo/spellbook/'\'' && git clean -d --force && git fetch && git reset --hard)'
```
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19816
Summary: Depends on D19810. Ref T13217. Ref T13216. I mostly used `grep implode | grep OR` and `grep implode | grep AND` to find these -- not totally exhaustive but should be a big chunk of the callsites that are missing `%LO` / `%LA`.
Test Plan:
These are tricky to test exhaustively, but I made an attempt to hit most of them:
- Browsed Almanac interfaces.
- Created/browsed Calendar events.
- Enabled/disabled/showed the lock log.
- Browsed repositories.
- Loaded Facts UI.
- Poked at Multimeter.
- Used typeahead for users and projects.
- Browsed Phriction.
- Ran various fulltext searches.
Not sure these are reachable:
- All the lint stuff might be dead/unreachable/nonfunctional?
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13217, T13216
Differential Revision: https://secure.phabricator.com/D19814
Summary:
Ref T13216. Ref T13217. Depends on D19800. This fixes all of the remaining query warnings that pop up when you run "arc unit --everything".
There's likely still quite a bit of stuff lurking around, but hopefully this covers a big set of the most common queries.
Test Plan: Ran `arc unit --everything`. Before change: lots of query warnings. After change: no query warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217, T13216
Differential Revision: https://secure.phabricator.com/D19801
Summary: Depends on D19785. Ref T13217. This converts many of the most common clause construction pathways to the new %Q / %LQ / %LO / %LA / %LJ semantics.
Test Plan: Browsed around a bunch, saw fewer warnings and no obvious behavioral errors. The transformations here are generally mechanical (although I did them by hand).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: hach-que
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19789
Summary:
See PHI958. Ref T13210. Previously, see PHI720.
The use case for the magic in PHI720 involves multiple patterns, and no parameter can be passed to `branch` that will result in multiple patterns being passed to `git`.
Replace the implicit magic with an explicit `patterns` parameter.
This whole thing is a bit shaky but probably isn't hurting anything.
Test Plan:
- Ran query with no `patterns`.
- Ran query with invalid `patterns`, got readable error.
- Ran query with various valid `patterns` (plain branch name, globs with "?" and "*"), got sensible results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19771
Summary:
Ref T13217. This method is slightly tricky:
- We can't safely return a string: return an array instead.
- It no longer makes sense to accept glue. All callers use `', '` as glue anyway, so hard-code that.
Then convert all callsites.
Test Plan: Browsed around, saw fewer "unsafe" errors in error log.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19784
Summary: Depends on D19798. Ref T13216. This puts at least a basic UI on top of sync logs.
Test Plan:
Viewed logs from the web UI and exported data. Note that these syncs are somewhat simulated since I my local cluster is somewhat-faked (i.e., not actually multiple machines).
{F5995899}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19799
Summary:
Depends on D19797. Ref T13216.
- Put the new `hookWait` in the export and the UI.
- Put the existing waits in the UI, not just the export.
- Make order consistent: host, write, read, hook (this is the order the timers start in).
Test Plan: Pushed some stuff, viewed web UI and saw sensible numbers, exported data and got the same values.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19798
Summary: Depends on D19796. Simplify some timing code by using phutil_microseconds_since() instead of duplicate casting and arithmetic.
Test Plan: Grepped for `1000000` to find these. Pulled, pushed, made a conduit call. This isn't exhaustive but it should be hard for these to break in a bad way since they're all just diagnostic.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19797
Summary:
Depends on D19779. Ref T13216. The push logs currently record the "hostWait", which is roughly "locking + subprocess cost". We also record locking separately, so we can figure out "subprocess cost" alone by subtracting the lock costs.
However, the subprocess (normally `git receive-pack`) runs hooks, and we don't have an easy way to figure out how much time was spent doing actual `git` stuff vs spent doing commit hook processing. This would have been useful in diagnosing at least one recent issue.
Track at least a rough hook cost and record it in the push logs.
Test Plan: Pushed to a repository, saw a reasonable hook cost appear in the database table.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19780
Summary:
Depends on D19778. Ref T13216. See PHI943, PHI889, et al.
We currently have a push log and a pull log, but do not separately log intracluster synchronization events. We've encountered several specific cases where having this kind of log would be helpful:
- In PHI943, an install was accidentally aborting locks early. Having timing information in the sync log would let us identify this more quickly.
- In PHI889, an install hit an issue with `MaxStartups` configuration in `sshd`. A log would let us identify when this is an issue.
- In PHI889, I floated a "push the linux kernel + fetch timeout" theory. A sync log would let us see sync/fetch timeouts to confirm this being a problem in practice.
- A sync log will help us assess, develop, test, and monitor intracluster routing sync changes (likely those in T13211) in the future.
Some of these events are present in the pull log already, but only if they make it as far as running a `git upload-pack` subprocess (not the case with `MaxStartups` problems) -- and they can't record end-to-end timing.
No UI yet, I'll add that in a future change.
Test Plan:
- Forced all operations to synchronize by adding `|| true` to the version check.
- Pulled, got a sync log in the database.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19779
Summary:
Ref T13216. When a repository is clustered, we run this cleanup code (to tell the repository to update, and log some timing information) on both nodes. Currently, we do slightly too much work, which is unnecessary and can be a bit confusing to human readers.
The double update message doesn't hurt anything, but there's no reason to write it twice.
Likewise, the second timing information update query doesn't do anything: there's no PushEvent object with the right identifier, so it just updates nothing. We don't need to run it, and it's confusing that we do.
Instead, only do these writes if we're actually the final node with the repository on it.
Test Plan: Added some logging, saw double writes/updates before the change and no doubles afterwards, with no other behavioral changes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19778
Summary:
See <https://discourse.phabricator-community.org/t/diffusionrequest-regex-error/2057/>.
The intent of `[\d-,]` is "digits, hyphen, and comma" but `[x-y]` means "character range x-y".
Specify `[\d,-]` instead to disambiguate the hyphen as "literal hyphen", not a character range marker.
Test Plan: I can't reproduce the original error as reported, but browsed around Diffusion for a bit.
Reviewers: amckinley, avivey
Reviewed By: avivey
Differential Revision: https://secure.phabricator.com/D19770
Summary: Ref T13210. See PHI841. This mirrors D19509 for Differential.
Test Plan: Called `transaction.search` on a commit with a bunch of audit activity, got appropriate labels in the results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19760
Summary:
Ref T13210. The comment action dropdown for audits has a heavy checkmark next to "Accept" and a heavy "X" next to "Raise Concern".
We previously removed similar marks in Differential in D19405 and that seems to have gone fine. For consistency, remove these too.
Test Plan: Viewed the comment action dropdown, no longer saw checkmark and X-mark.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19759
Summary:
Depends on D19734. Ref T13202. Ref T13109. Ref T10884. See PHI905. See PHI889. We currently rank cluster nodes in three cases:
# when performing a write, we can go to any node (D19734 should make our ranking good);
# when performing a read, we can go to any node (currently random, but T10884 discusses ideas to improve our ranking);
# when performing an internal synchronization before a read or a write, we must go to an up-to-date node.
Currently, case (3) is not-exactly-deterministic but not random, and we won't spread intracluster traffic acrosss the cluster evenly if, say, half of it is up to date and half of it is still synchronizing. For a given write, I believe all nodes will tend to synchronize from whichever node first received the write today.
Instead, shuffle the list and synchronize from any up-to-date node.
(I think we could improve upon this only by knowing which nodes actually have load and selecting the least-loaded -- doable, but not trivial.)
Test Plan: Poked at it locally, will deploy to `secure`. This is hard to measure/test terribly convincingly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202, T13109, T10884
Differential Revision: https://secure.phabricator.com/D19735
Summary: Ref D18268. This typo cancelled itself out, and I can't find any other callers.
Test Plan: arc unit
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19718
Summary:
Now on the blame page, identities get `avatar.png` and there are little tooltips that show a few characters of the committer identity string.
Also add a default icon for repo identities.
Test Plan: Loaded some blame pages for files touched by users with and without repo identities attached.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19587
Summary:
Ref T13202. See PHI889. If the lock log is enabled, we can try to offer more details about lock holders.
When we fail to acquire a lock:
- check for recent acquisitions and suggest that this is a bottleneck issue;
- if there are no recent acquisitions, check for the last acquisition and print details about it (what process, how long ago, whether or not we believe it was released).
Test Plan:
- Enabled the lock log.
- Changed the lock wait time to 1 second.
- Added a `sleep(10)` after grabbing the lock.
- In one window, ran a Conduit call or a `git fetch`.
- In another window, ran another operation.
- Got useful/sensible errors for both ssh and web lock holders, for example:
> PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=12609, host=orbital-3.local, sapi=apache2handler, controller=PhabricatorConduitAPIController, method=diffusion.rawdiffquery) 3 second(s) ago. There is no record of this lock being released.)
> PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=65251, host=orbital-3.local, sapi=cli, argv=/Users/epriestley/dev/core/lib/phabricator/bin/ssh-exec --phabricator-ssh-device local.phacility.net --phabricator-ssh-key 2) 2 second(s) ago. There is no record of this lock being released.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19702
Summary:
Depends on D19657. Ref T13197. See PHI841.
This enriches the results from `diffusion.commit.search` with information similar to the information returned by the "commits" attachment from `differential.diff.search`.
Also include unreachable, imported, message, audit status, and repository PHID.
Test Plan: Called `diffusion.commit.search` and reviewed the results, which looked sensible.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19658
Summary:
Depends on D19656. Ref T13197. See PHI851.
- This class is now a real object, so get rid of the "Constants" part of the name.
- Rename it for greater consistency with other modern objects.
- Get rid of the `MODERN_` tag now that the old constants are gone.
Test Plan: Bunch of `grep`, browsed around.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19657
Summary: Depends on D19652. Ref T13197. See PHI851. This migrates the actual `auditStatus` on Commits, and older status transactions.
Test Plan:
- Ran migrations.
- Spot-checked the database for sanity.
- Ran some different queries, got unchanged results from before migration.
- Reviewed historic audit state transactions, and accepted/raised concern on new audits. All state transactions appeared to generate properly.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19655
Summary: Ref T13197. We're almost ready to migrate: let the Query accept either older integer values or new string values. Then move some callsites to use strings.
Test Plan: Called `audit.query`, browsed audits, audited commits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19650
Summary: Ref T13195. See PHI851. Continuing down the path toward replacing these legacy numeric constants with more modern string constants.
Test Plan:
- Raised concern, requested verification, verified.
- Looked at commit hovercard with audit status.
- Viewed header on a commit page.
- (Didn't test the Doorkeeper stuff since it requires linking to Asana and seems unlikely to break.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19647
Summary:
Ref T13195. Ref T8573. This allows reviewers to mark their own inline comments as "Done" before they submit them.
If you're leaving a non-actionable comment like "this is good", you can pre-check "Done" to give the author a hint that you don't expect any response.
Test Plan: On revisions and commits, added inlines as the author and a reviewer/auditor. Marked them done/not-done before submitting. As author, marked the not-done ones done after submitting. Checked preivews, toggled done/not done states.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19634
Summary:
Ref T13195. See PHI851. Add an object, analogous to the `DifferentialRevisionStatus` object, to handle audit status management.
This will primarily make it easier to swap storage over to strings later, but also cleans things up a bit.
Test Plan: Viewed audit/commit lists, saw sensible state icons. Ran `bin/audit synchronize`, got sensible output.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19646
Summary:
Ref T13195. Ref T8573. The inline comment controllers currently use outdated `$user = $this->getRequest()->getUser()` calls.
Instead, use `$viewer = $this->getViewer()`.
This is just a small consistency update with no behavioral changes.
Test Plan: Viewed and added inlines in Differential and Diffusion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19633
Summary: Ref T13077. There is no "PHUIDocumentView" so toss the "Pro" suffix from this classname.
Test Plan: Grepped for `PHUIDocumentView` and `PHUIDocumentViewPro`.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19616
Summary:
Fixes T12251. Ref T13189. See PHI610. The difficulty here is that we don't want to disclose Phabricator account information to Buildkite. We're comfortable disclosing information from `git`, etc.
- For commits, use the Identity to provide authorship information from Git.
- For revisions, use the local commit information on the Diff to provide the Git/Mercurial/etc author of the HEAD commit.
Test Plan:
- Built commits and revisions in Buildkite via Harbormaster.
- I can't actually figure out how to see author information on the Buildkite side, but the values look sane when dumped locally.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13189, T12251
Differential Revision: https://secure.phabricator.com/D19614
Summary:
Ref T13187. See PHI834. Mercurial has somewhat-recently (changeset is from Jan 2018) introduced a new "protocaps" command, that appears in Mercurial 4.7 and possibly before then.
We must explicitly enumerate all protocol commands because you can't decode the protocol without knowing how many arguments the command expects, so enumerate it.
(Also fix an issue where the related error message had an extra apostrophe.)
Test Plan:
- Ran `hg clone ...` with client and server on Mercurial 4.7.
- Before: fatal on unknown "protocaps" command.
- Midway: better typography in error message.
- After: clean clone.
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19596
Summary: Ref T12164. Updates another controller to use identities.
Test Plan:
Pretty ad-hoc, but loaded the main pages of several different repos with and without repo identities. I'm not totally convinced the `author` from this data structure is actually being used:
```
$return = array(
'commit' => $modified,
'date' => $date,
'author' => $author,
'details' => $details,
);
```
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19580
Summary: Depends on D19491.
Test Plan: Viewed some commits where the identity was mapped to a user and another that wasn't; saw the header render either a link to the user or the identity object.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19492
Summary:
See D19558. This method has no callers and just wraps `diffusion.historyquery`, since D5960 (2013).
This was introduced in D315 (which didn't make it out of FB, I think) inside Facebook for unclear purposes in 2011.
Test Plan: Grepped for callers, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: artms
Differential Revision: https://secure.phabricator.com/D19559
Summary:
`diffusion.getrecentcommitsbypath` fails with 500 error when non existing callsign is passed:
```
>>> UNRECOVERABLE FATAL ERROR <<<
Call to a member function getCommit() on null
```
Expected Behavior:
Return more graceful error notifying caller that such callsign/repository does not exist
Reproduction steps:
Open conduit: https://secure.phabricator.com/conduit/method/diffusion.getrecentcommitsbypath/
Enter:
callsign: "obviouslynotexisting"
path: "/random"
Click call method
Test Plan: after applying patch - call no longer fails with 500s
Reviewers: Pawka, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19558
Summary:
Ref T13164. See PHI765. We currently show "Rejected: Herald" in the push log UI, but don't show which rule rejected a push.
We store this data, and it's potentially useful: either for hunting down a particular issue, or for getting a general sense of how often a reject rule is triggering (maybe because you want to tune how aggressive it is).
Show this data in the web UI, and include it in the data export payload.
Test Plan:
- Pushed to a hosted repository so that I got blocked by a Herald rule.
- Viewed the push logs in the web UI, now saw which rule triggered things.
- Exported logs to CSV, saw Herald rule PHIDs in the data.
{F5776211}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19555
Summary:
See PHI775. See D19499. Originally, see PHI720.
D19499 broke the standalone "Branches" page for commits. Normally, you reach this by taking these steps:
- View a commit which is contained by 11 or more branches.
- Click the "More Branches..." link in the "Branches" field.
- You should be taken to a list of all branches which contain the commit.
The change to the 'branch' parameter was adjusted in the query that builds the "x, y, z, More Branches..." list, but not on the actual "Branches" list with the full list. Adjust it.
Test Plan:
- Set display limit to 1, viewed a commit on "master" and "stable", clicked "More Branches".
- Before: saw only "master".
- After: saw both "master" and "stable".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19532
Summary:
Ref T13151. See PHI720. If you want to test if commit X appears on specific branch Y, `git branch --contains X -- Y` is faster than (effectively) `git branch --contains X | grep Y`.
Since this call has a "branch" parameter anyway, use it as the pattern argument if provided.
Test Plan:
- Called the API method with no parameters, got all branches.
- Called the API method with `master`, got just master.
- Called the API method with `maste*`, got master. This behavior is not officially supported and may change in the future.
- Viewed a commit, still saw all branches.
- Grepped for `diffusion.branchquery` and verified that no remaining callsites pass a default "branch" parameter.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19499
Summary: Fixes T13155. Ref T13151. A recent change (D19455) changed the return format here, but I missed this special case for empty commits.
Test Plan:
- T13155 has a good set of reproduction instructions.
- Pushed an empty commit.
- Before: bunch of warning log spew.
- After: clean logs.
Reviewers: amckinley, avivey
Reviewed By: avivey
Maniphest Tasks: T13155, T13151
Differential Revision: https://secure.phabricator.com/D19500
Summary: Ref T12164. Make it easier to work with identity objects by attaching them to commits and attaching users to identities.
Test Plan: Loaded some commits with `->needIdentities(true)` and checked the resulting objects.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19491
Summary: Depends on D19429. Depends on D19423. Ref T12164. This creates new columns `authorIdentityPHID` and `committerIdentityPHID` on commit objects and starts populating them. Also adds the ability to explicitly set an Identity's assignee to "unassigned()" to null out an incorrect auto-assign. Adds more search functionality to identities. Also creates a daemon task for handling users adding new email address and attempts to associate unclaimed identities.
Test Plan: Imported some repos, watched new columns get populated. Added a new email address for a previous commit, saw daemon job run and assign the identity to the new user. Searched for identities in various and sundry ways.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19443
Summary: Depends on D19423. Ref T12164. Adds controllers capable of listing and editing `PhabricatorRepositoryIdentity` objects. Starts creating those objects when commits are parsed.
Test Plan: Reparsed some revisions, observed objects getting created in the database. Altered some `Identity` objects using the controllers and observed effects in the database. No attempts made to validate behavior under "challenging" author/committer strings.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19429
Summary:
Ref T13142. When commits are pushed, we try to handle them on one of two pathways:
- Normal changes: we load these into memory and potentially apply Herald content rules to them.
- "Enormous" changes: we don't load these into memory and skip content rules for them.
The goal is to degrade gracefully when users push huge changes: they should work, just not support all the features.
However, some changes can slip through the cracks right now:
- If you push a lot of commits at once, we'll try to cache all of the changes smaller than 1GB in memory. This can require an arbitrarily large amount of RAM.
- We calculate sizes by just looking at the `strlen()` of the diff, but a changeset takes more RAM in PHP than the raw diff does. So even if a diff is "only" 500MB, it can take much more memory than that. On systems with relatively little memory available, this may result in OOM while processing changes that are close to the "enormous" limit.
This change makes two improvements:
- Instead of caching everything, cache only 64MB of things.
- For most pushes, this is the same, since they have less than 64MB of diffs.
- For pushes of single very large changes, this is a bit slower (more CPU) since we have to do some work twice.
- For pushes of many changes, this is slower (more CPU) since we have to do some work twice, but, critically, doesn't require unlimited memory.
- Instead of flagging changes as "enormous" at 1GB, flag them as "enormous" at 256MB.
- This reduces how much memory is required to process the largest "non-enormous" changes.
- This also gets us under Git's hard-coded 512MB "always binary" cutoff; see T13143.
- This is still completely gigantic and way larger than any normal change should be.
An additional improvement would be to try to reduce the amount of memory we need to use to hold a change in process memory. I think the other changes here alone will fix the immediate issue in PHI657, but it would be nice if the "largest non-enormous change" required only a couple gigs of RAM.
Test Plan:
- Used `ini_set('memory_limit', '1G')` to artificially limit memory to 1GB.
- Pushed a series of two commits which add two 550MB text files (Temporarily, I added a `--binary` flag to trick Git into showing real diffs for these, see T13143.)
- Got a memory limit error.
- Applied the "cache only 64MB of stuff" and "consider 256MB, not 1GB, to be enormous" changes.
- Pushed again, got properly rejected as enormous.
- Added `memory_get_usage()` calls to measure how actual memory size and reported "size" estimate compare. For these changes, saw a 639MB diff require 31,479MB of memory, i.e. a factor of about 50x. This is, uh, pretty not great.
- Allowed enormous changes, pushed again, push went through.
Reviewers: amckinley
Maniphest Tasks: T13142
Differential Revision: https://secure.phabricator.com/D19455
Summary:
Fixes T13140. See PHI660.
Recent versions of Subversion can send a `(get-file true false false )` protocol frame with extra space between "false" and "false". This is allowed by the protocol spec, but never normally happens, and we do not parse it correctly.
Instead, parse it correctly.
Test Plan:
- Added unit tests.
- Ran `svn proplist svn+ssh://.../diffusion/X/file.c` under SVN 1.10 before and after the change.
- Before: indefinite hang.
- After: completed in finite time.
Reviewers: amckinley, asherkin
Reviewed By: amckinley, asherkin
Maniphest Tasks: T13140
Differential Revision: https://secure.phabricator.com/D19451
Summary:
Ref T13137. See PHI609. An install would like to filter audit requests on a particular branch, e.g. "master".
This is difficult in the general case because we can not apply this constraint efficiently under every conceivable data shape, but we can do a reasonable job in most practical cases.
See T13137#238822 for more detailed discussion on the approach here.
This is a bit rough, but should do the job for now.
Test Plan:
- Filtered commits by various branches, e.g. "master"; "lfs". Saw correct-seeming results.
- Stubbed out the "just list everything" path to hit the `diffusion.internal.ancestors` path, saw the same correct-seeming results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19431
Summary:
See PHI604. Ref T13130. Ref T13105. There's currently no way to turn blame off in Diffusion. Add a "Hide Blame" option to the "View Options" dropdown so it can be toggled off.
Also fix a couple of bugs around this: for example, if you loaded a Jupyter notebook and then switched to "Source" view, blame would incorrectly fail to activate because the original rendering of the "stage" used an asynchronous engine so `willRenderRef()` wasn't called to populate blame.
Test Plan:
- Viewed a source file, toggled blame off/on, reloaded page to see state stick in URL.
- Viewed a Jupyter notebook, toggled to "Source" view, saw blame.
- Viewed stuff in Files (no blame UI options).
- Tried to do some invalid stuff like toggle blame on a non-blame engine (options disable properly).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130, T13105
Differential Revision: https://secure.phabricator.com/D19414
Summary: Depends on D19391. Ref T13126. See that task for some details on what's going on here.
Test Plan:
- Viewed a file which includes lines that were added during the first commit to the repository.
- Before D19391: fatal.
- After D19391: blank.
- After this patch: accurate blame information.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13126
Differential Revision: https://secure.phabricator.com/D19392
Summary:
Ref T13126. When you view a file using the new document engine view and some lines were introduced in the initial commit to the repository, Git renders "^abc123" in the blame output.
We currently don't do anything about this, and later fail to look it up and fatal.
It's also unlikely-but-conceivably-possible to end up here if a commit has not imported yet or has been nuked with `bin/remove destroy`.
Let the whole thing run without fataling even if a `$commit` is missing. Future refinements could improve this behavior.
Test Plan: Viewed a file with lines introduced in the initial commit, got empty blame instead of a fatal.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13126
Differential Revision: https://secure.phabricator.com/D19391
Summary:
Depends on D19377. Ref T13125. Ref T13124. Ref T13105. Coverage reporting in Diffusion didn't initially survive the transition to Document Engine; restore it.
This adds some tentative/theoretical support for multiple columns of coverage, but no way to actually produce them in the UI. For now, the labels, codes, and colors are hard coded.
Test Plan:
Added coverage with `diffusion.updatecoverage`, saw coverage in the UI:
{F5525542}
Hovered over coverage, got labels and highlighting.
Double-checked labels for "N" (Not Executable) and "U" (Uncovered). See PHI577.
Faked some multi-column coverage, but you can't currently get this yourself today:
{F5525544}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13125, T13124, T13105
Differential Revision: https://secure.phabricator.com/D19378
Summary:
Depends on D19356. Fixes T10883. Ref T13120.
- Add a "writable" property to the bindings, defaulting to "true" with a nice dropdown.
- When selecting hosts, allow callers to request a writable host.
- If the caller wants a writable host, only return hosts if they're writable.
- In SVN and Mercurial, we sometimes return only writable hosts when we //could// return read-only hosts, but figuring out if these request are read-only or read-write is currently tricky. Since these repositories can't really cluster yet, this shouldn't matter too much today.
Test Plan:
- Without any config changes, viewed repositories via web UI and pushed/pulled via SSH and HTTP.
- Made all nodes in the cluster read-only by disabling "writable", pulled and hit the web UI (worked), tried to push via SSH and HTTP (got errors about read-only).
- Put everything back, pulled and pushed.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13120, T10883
Differential Revision: https://secure.phabricator.com/D19357
Summary:
Depends on D19355. Ref T10883. Ref T13120. Rather than adding a million parameters here, wrap the selector-parameters in an `$options`.
The next change adds a new "writable" option to support forcing selection of writable hosts.
Test Plan: Pulled and pushed via HTTP and SSH, viewed repositories via Diffusion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13120, T10883
Differential Revision: https://secure.phabricator.com/D19356
Summary:
Depends on D19313. Ref T13105. Fixes T13015. We lost the coloration for ages in the switch to Document Engine.
Restore it, and use a wider range of colors to make the information more clear.
Test Plan: Viewed some blame, saw a nice explosion of bright colors. This is a cornerstone of good design.
Maniphest Tasks: T13105, T13015
Differential Revision: https://secure.phabricator.com/D19314
Summary: Depends on D19310. Ref T13105. The "meta" value was not populating correctly because this used `phutil_tag()`.
Test Plan: Will verify on `secure`.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19311
Summary: Ref T13105. This needs refinement but blame sort of works again, now.
Test Plan: Viewed files in Diffusion and Files; saw blame in Diffusion when viewing in source mode.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19309
Summary: Ref T13105. Ref T13047. This makes symbol indexes work with DocumentEngine in Files, and restores support in Diffusion.
Test Plan: Command-clicked stuff, got taken to the symbol index with reasonable metadata in Diffusion, Differential and Files.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105, T13047
Differential Revision: https://secure.phabricator.com/D19307
Summary:
Ref T13105. Fixes some issues with line linking and highlighting under DocumentEngine:
- Adding `$1-3` to the URI didn't work correctly with query parameters.
- Reading `$1-3` from the URI didn't work correctly because Diffusion parses them slightly abnormally.
Test Plan: Clicked/dragged lines to select them. Observed URI. Reloaded page, got the right selection.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19305
Summary:
Ref T13105. This breaks about 9,000 features but moves Diffusion to DocumentEngine for rendering. See T13105 for a more complete list of all the broken stuff.
But you can't bake a software without breaking all the features every time you make a change, right?
Test Plan: Viewed various files in Diffusion, used DocumentEngine features like highlighting and rendering engine selection.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Subscribers: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19302
Summary: Ref T13105. Given that we now load blame with AJAX, it's not clear that there's any benefit to disabling it. This would also interact oddly with the document engine.
Test Plan: Viewed files in Diffusion, no longer saw blame-related options.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19300
Summary: This reverts D18524. See that revision for discussion.
Test Plan: Viewed home menu, saw application names as menu items.
Differential Revision: https://secure.phabricator.com/D19308
Summary:
See PHI489. Ref T13110. At least for now, this just shows "..." at the end since you can click the revision to see the whole list anyway.
Also remove the older-style external Handle passing in favor of lazy construction via HandlePool.
Test Plan: Viewed revisions, fiddled with the 7 limit, got sensible-seeming "..." behavior.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19293
Summary:
Depends on D19279. Ref T13110. This implements the existing publishing logic for buildables, but does so via ModularTransactions instead of a core transaction type.
Since each application is implementing build transactions independently, this removes the core type.
Next, Differential will get a similar treatment.
Test Plan: Used `bin/harbormaster publish` (with some commenting-out-guard-clauses) to publish a commit Buildable; saw unchanged feed behavior.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19280
Summary:
Ref T13110. Currently, build status is published the same way for every Buildable by the BuildEngine.
I want to change this to delegate publishing to each Buildable, particularly so that Differential may use more detailed rules for handling builds and drafts.
Rather than add additional methods to the existing `BuildableInterface`, add an engine generator method instead. This is a pattern which has seen more use recently (e.g., in Ferret) and lets us pay a little more upfront to pull complex pieces of logic out of the main class and let them use inheritence more easily. If we had Traits that might cover this to some degree.
I'd expect to eventually reduce the size of `BuildableInterface` and move the `CircleCI` and `BuildKite` interfaces so that the `BuildableEngine` implements them instead of the main object.
Here, this new engine does nothing and is never instantiated. In upcoming changes, publishing logic will move into it so that Differential can handle publishing differently.
Test Plan: Ran `arc liberate`, loaded pages, grepped for `BuildableInterface`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19278
Summary: See PHI514. Ref T13114. Ref T8951. When a push is an "initial import" (a push of at least 7 commits to an empty repository) don't run Herald or enormous change protection.
Test Plan: Pushed some non-initial changes to a repository, and some initial changes.
Maniphest Tasks: T13114, T8951
Differential Revision: https://secure.phabricator.com/D19265
Summary:
Depends on D19249. Ref T13109. Add timing information to the `PushEvent`:
- `writeWait`: Time spent waiting for a write lock.
- `readWait`: Time spent waiting for a read lock.
- `hostWait`: Roughly, total time spent on the leaf node.
The primary goal here is to see if `readWait` is meaningful in the wild. If it is, that motivates smarter routing, and the value of smarter routing can be demonstrated by looking for a reduction in read wait times.
Test Plan: Pushed some stuff, saw reasonable timing values in the table. Saw timing information in "Export Data".
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19250
Summary:
Depends on D19247. Ref T13109. When we receive an SSH request, generate a random unique ID for the request. Then thread it down through the process tree.
The immediate goal is to let the `ssh-exec` process coordinate with `commit-hook` process and log information about read and write lock wait times. Today, there's no way for `ssh-exec` to interact with the `PushEvent`, but this is the most helpful place to store this data for users.
Test Plan: Made pushes, saw the `PushEvent` table populate with a random request ID. Exported data and saw the ID preserved in the export.
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19249
Summary:
Ref T13109. Make it slightly more clear what the scope of the write and read locks are, and slightly more clear that we're actively acquiring locks, not just sitting around waiting.
While waiting on another writer, show who we're waiting on so you can walk over to their desk and glare at them.
Test Plan:
Added `sleep(15)` after `willWrite()`. Pushed in two windows. Saw new, more informative messages. In the second window, saw the new guidance:
> # Waiting for hector to finish writing (on device "repo1.local.phacility.net" for 11s)...
Reviewers: asherkin
Reviewed By: asherkin
Subscribers: asherkin
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19247
Summary:
Ref T13108. See PHI364. See the task and issue for discussion.
If a `git fetch` during synchronization hangs, the whole node currently hangs. While the causes of a `git fetch` hang aren't clear, we don't expect synchronization to ever reasonably take more than 15 minutes, so add a default timeout.
Test Plan: Will deploy and observe; this is difficult to reproduce or test directly.
Maniphest Tasks: T13108
Differential Revision: https://secure.phabricator.com/D19235
Summary: Depends on D19190. Fixes T12590. Ref T13099. Replaces the barely-usable, gigantic, poorly ordered "<select />" control with a tokenizer. Attempts to fix various minor issues.
Test Plan:
- Edited paths: include/exclude paths, from different repositories, different actual paths.
- Used "Add New Path" to add rows, got repository selector prepopulated with last value.
- Used "remove".
- Used validation typeahead, got reasonable behaviors?
The error behavior if you delete the repository for a path is a little sketchy still, but roughly okay.
Maniphest Tasks: T13099, T12590
Differential Revision: https://secure.phabricator.com/D19191
Summary:
Depends on D19189. Ref T12590. The "validate" and "complete" endpoints for this UI could incorrectly return redirect responses. These aren't critical to the behavior of Owners, but they're nice to have, and shouldn't redirect.
Instead, skip the canonicalizing redirect for AJAX requests.
Test Plan: Edited Owners paths in a repository with a short name, got completion/validation again.
Maniphest Tasks: T12590
Differential Revision: https://secure.phabricator.com/D19190
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
Summary: Ref T13093. Depends on D19145. See PHI398. Previously, see D18933. This provides the current viewer to `ConduitCall` so that we don't try to use device credentials from unprivileged web hosts.
Test Plan: Evaluated the "Branches" field locally, saw an appropriate field value.
Maniphest Tasks: T13093
Differential Revision: https://secure.phabricator.com/D19146
Summary:
Ref T13093. See PHI396. These are possibly somewhat niche, but reasonable to support and consistent with the existing "Pusher's projects".
Also relabel "Pusher's projects" and "Project tags" for consistency and, hopefully, clarity.
Test Plan:
- Created new "commit" and "hook: commit content" Herald rules which run against "Author's projects" and "Committer's projects".
- Test console'd the "Commit" rules.
- Pushed through the "Hook" rule.
- In all cases, saw fields populate appropriately.
Maniphest Tasks: T13093
Differential Revision: https://secure.phabricator.com/D19145
Summary:
Ref T13090. The default width changed recently to become much wider, but the behavior on this control isn't great. Instead:
- Pick a default width somewhere between the two.
- Make the width sticky across show/hide (pressing "f" twice remembers your width instead of resetting it).
- Make the width sticky across reloads (dragging the bar, then reloading the page keeps the bar in the same place).
Test Plan:
- Without settings, loaded page: got medium-width bar.
- Dragged bar wide/narrow, toggled on/off with "f", got persistent width.
- Dragged bar wide/narrow, reloaded page, got persistent width.
- Dragged bar wide/narrow, toggled it off, reloaded page, toggled it on, got persistent width.
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19129
Summary:
Ref T13083. Facts has a fair amount of weird hardcoding and duplication of responsibilities. Reduce this somewhat: no more hard-coded fact aggregates, no more database-driven list of available facts, etc. Generally, derive all objective truth from FactEngines. This is more similar to how most other modern applications work.
For clarity, hopefully: rename "FactSpec" to "Fact". Rename "RawFact" to "Datapoint".
Split the fairly optimistic "RawFact" table into an "IntDatapoint" table with less stuff in it, then dimension tables for the object PHIDs and key names. This is primarily aimed at reducing the row size of each datapoint. At the time I originally wrote this code we hadn't experimented much with storing similar data in multiple tables, but this is now more common and has worked well elsewhere (CustomFields, Edges, Ferret) so I don't anticipate this causing issues. If we need more complex or multidimension/multivalue tables later we can accommodate them. The queries a single table supports (like "all facts of all kinds in some time window") don't make any sense as far as I can tell and could likely be UNION ALL'd anyway.
Remove all the aggregation stuff for now, it's not really clear to me what this should look like.
Test Plan: Ran `bin/fact analyze` and viewed web UI. Nothing exploded too violently.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19119
Summary:
See PHI370. Support the "Affected packages" and "Affected package owners" Herald fields in pre-commit hooks.
I believe there's no technical reason these fields aren't supported and this was just overlooked.
Test Plan: Wrote a rule which makes use of the new fields, pushed commits through it. Checked transcripts and saw sensible-looking values.
Differential Revision: https://secure.phabricator.com/D19104
Summary: Depends on D19087. Ref T13079. This still doesn't feel like the most clean, general system in the world, but is a step forward from hard-coded `switch()` stuff.
Test Plan:
- Jumped to `r`.
- Jumped to `a`.
- Jumped to `r poe` (multiple results).
- Jumped to `r poetry` (one result).
- Jumped to `r syzygy` (no results).
- Jumped to `p`.
- Jumped to `p robot` (multiple results); `p assessment` (one result).
- The behavior for `p <string>` has changed slightly but should be more powerful now (it's consistent with `r <string>`).
- Jumped to `s <symbol>` and `s <context>-><symbol>`.
- Jumped to `d`.
- Jumped to `f`.
- Jumped to `t`.
- Jumped to `T123`, `D123`, `@dog`, `PHID-DREV-abcd`, etc.
Maniphest Tasks: T13079
Differential Revision: https://secure.phabricator.com/D19088
Summary: Ref T13079. This recently-introduced Engine/EngineExtension are a good fit for adding more datasource functions in general, but we didn't think quite big enough in naming them.
Test Plan: Used quick search typeahead, hit applications/users/monograms/symbols/etc.
Maniphest Tasks: T13079
Differential Revision: https://secure.phabricator.com/D19087
Summary: Depends on D19063. Ref T13054. Prepare for the addition of a new `PREPARING` status by getting rid of the "scattered mess of switch statements" pattern of status management.
Test Plan: Searched/browsed buildables. Viewed buildables. Viewed revisions. Grepped for all affected symbols.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19064
Summary:
See D18176. This query has no effect (other than wasting resources) and the result is unused.
`$repository` already has the URI loaded because we load them unconditionally during request initialization.
Test Plan: Viewed repository URIs.
Subscribers: jmeador
Differential Revision: https://secure.phabricator.com/D19036
Summary:
Ref T13053. Fixes T7804. Adds "Acting user" so you can have "always email me" stuff skip things you did or keep an eye on suspicious interns.
For the test console, the current user is the acting user.
For pushes, the pusher is the acting user.
Test Plan: Wrote acting user rules, triggered them via test console and via multiple actors on real objects.
Maniphest Tasks: T13053, T7804
Differential Revision: https://secure.phabricator.com/D19031
Summary:
This command also needs a "." instead of an empty string now.
(This powers the file browser typeahead in Diffusion.)
Test Plan: Will test in production since there's still no easy 2.16 installer for macOS.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19010
Summary:
Ref T13057. This makes "reverts" syntax more visible and useful. In particular, you can now `Reverts Dxx` in a revision or commit, and `Reverts <hash>` from a revision.
When you do, the corresponding object will get a more-visible cross-reference marker in its timeline:
{F5405517}
From here, we can look at surfacing revert information more heavily, since we can now query it on revision/commit pages via edges.
Test Plan: Used "reverts <hash>" and "reverts <revision>" in Differential and Diffusion, got sensible results in the timeline.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13057
Differential Revision: https://secure.phabricator.com/D18978
Summary:
Depends on D18972. Ref T13049.
Currently, the "flags" columns renders an inscrutible bitmask which you have to go hunt down in the code. Show a list of flags in human-readable text instead.
The "code" column renders a meaningless integer code. Show a text description instead.
The pull logs and push logs pages don't have a crumb to go back up out of the current query. Add one.
Test Plan: Viewed push logs, no more arcane numbers. Saw and clicked crumbs on each log page.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18973
Summary:
Depends on D18970. Ref T13049. Currently, the policy for viewing remote addresses is:
- In activity logs: administrators.
- In push and pull logs: users who can edit the corresponding repository.
This sort of makes sense, but is also sort of weird. Particularly, I think it's kind of hard to understand and predict, and hard to guess that this is the behavior we implement. The actual implementation is complex, too.
Instead, just use the rule "administrators can see remote addresses" consistently across all applications. This should generally be more strict than the old rule, because administrators could usually have seen everyone's address in the activity logs anyway. It's also simpler and more expected, and I don't really know of any legit use cases for the "repository editor" rule.
Test Plan: Viewed pull/push/activity logs as non-admin. Saw remote addresses as an admin, and none as a non-admin.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18971
Summary: Ref T13049. This is just a general nice-to-have so you don't have to export a 300MB file if you want to check the last month of data or whatever.
Test Plan: Applied filters to all three logs, got appropriate date-range result sets.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18970
Summary:
Ref T13049. All exportable objects should always have these fields, so make them builtins.
This also sets things up for extensions (like custom fields).
Test Plan: Exported user data, got the same export as before.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18951
Summary: See PHI276. Ref T13048. The fix in D18933 got one callsite, but missed the one in the `callConduit()` method, so the issue isn't fully fixed in production. Convert this adapter to use a real viewer (if one is available) more thoroughly.
Test Plan: Ran rules in test console, saw field values. Will test in production again.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18950
Summary: Ref T13050. Oh boy. Both of them run `grep`!
Test Plan: Will push again.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13050
Differential Revision: https://secure.phabricator.com/D18945
A recent version of Git has changed some piece of behavior here and we
now get "fatal: ssh variant 'simple' does not support setting port"
when using a port. Explicitly setting GIT_SSH_VARIANT to `ssh` likely
fixes this.
Summary:
Depends on D18939. Ref T13047. Symbol lookup can be activated from a diff (in Differential or Diffusion) or from the static view of a file at a particular commit.
In the latter case, we need to figure out the path a little differently. The character and line number approaches still work as written.
Test Plan:
- Command-clicked symbols in the Diffusion browse view with blame on and off; saw path, line and char populate properly.
- Command-clicked symbols in Differential diff view to check I didn't break anything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18940
Summary: Depends on D18937. Ref T13047. When available, provide character positions so external indexers can return more accurate results.
Test Plan: Clicked symbols in Safari, Firefox and Chrome, got sensible-looking character positions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18939
Summary:
Depends on D18936. Ref T13047. Third parties can define external symbol sources that let users jump to PHP or Python documentation or query some server.
Give these queries more information so they can try to get better results: the path and line where the symbol appeared, and any known repository scope.
Test Plan: Wrote a fake external source that used this data, command-clicked a symbol in Differential, saw a fake external symbol result.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18937
Summary:
Depends on D18918. Ref T13046. Ref T5954. Pull logs can currently be browsed in the web UI, but this isn't very powerful, especially if you have thousands of them.
Allow SearchEngine implementations to define exportable fields so that users can "Use Results > Export Data" on any query. In particular, they can use this workflow to download a file with pull logs.
In the future, this can replace the existing "Export to Excel" feature in Maniphest.
For now, we hard-code JSON as the only supported datatype and don't actually make any effort to format the data properly, but this leaves room to add more exporters (CSV, Excel) and data type awareness (integer casting, date formatting, etc) in the future.
For sufficiently large result sets, this will probably time out. At some point, I'll make this use the job queue (like bulk editing) when the export is "large" (affects more than 1K rows?).
Test Plan: Downloaded pull logs in JSON format.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046, T5954
Differential Revision: https://secure.phabricator.com/D18919
Summary:
Depends on D18932. Ref T13048. See PHI276. In the cluster, we don't have device keys on `web` nodes. This is generally good, since they don't need them, and it means that we aren't putting more credentials than we need on those hosts.
However, it means that when we pull diff content to test "Commit" rules via the Herald test console, we use the omnipotent user and try to use device credentials, and this fails since we don't have any.
Instead, pass the real viewer in this case so we just sign the request as them, like we do for normal Diffusion requests.
Test Plan:
Wrote and ran a commit content rule locally, no issues.
This isn't completely convincing since my local setup does have device credentials, but I'll double-check in production once this deploys.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18933
Summary:
Depends on D18931. Ref T13048. Ref T13041. This field means "the first accepting reviewer, where order is mostly arbitrary". Modern rules should almost certainly use "Accepting Reviewers" instead.
Getting rid of this completely is a pain, but we can at least reduce confusion by marking it as not-the-new-hotness. Add a "Deprecated" group, move it there, and mark it for exile.
Test Plan:
Edited a commit rule, saw it in "Deprecated" group at the bottom of the list:
{F5395001}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048, T13041
Differential Revision: https://secure.phabricator.com/D18932
Summary:
Depends on D18915. Ref T13046.
- Distinguish between HTTP and HTTPS.
- Use more constants and fewer magical strings.
- For HTTP responses, give them better type information and more helpful UI behaviors.
Test Plan: Pulled over SSH and HTTP. Reviewed resulting logs from the web UI. Hit errors like missing/invalid credentials.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18917
Summary:
Depends on D18912. Ref T13046. Add a UI to browse the existing pull log table.
The actual log still has some significant flaws, but get the basics working.
Test Plan: {F5391909}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18914
Summary:
See PHI305. Ref T13046.
The SSH workflows currently extend `PhabricatorManagementWorkflow` to benefit from sharing all the standard argument parsing code. Sharing the parsing code is good, but it also means they inherit a `getViewer()` method which returns the ommnipotent viewer.
This is appropriate for everything else which extends `ManagementWorkflow` (like `bin/storage`, `bin/auth`, etc.) but not appropriate for SSH workflows, which have a real user.
This caused a bug with the pull logs where `pullerPHID` was not recorded properly. We used `$this->getViewer()->getPHID()` but the correct code was `$this->getUser()->getPHID()`.
To harden this against future mistakes:
- Don't extend `ManagementWorkflow`. Extend `PhutilArgumentWorkflow` instead. We **only** want the argument parsing code.
- Rename `get/setUser()` to `get/setSSHUser()` to make them explicit.
Then, fix the pull log bug by calling `getSSHUser()` instead of `getViewer()`.
Test Plan:
- Pulled and pushed to a repository over SSH.
- Grepped all the SSH stuff for the altered symbols.
- Saw pulls record a valid `pullerPHID` in the pull log.
- Used `echo {} | ssh ... conduit conduit.ping` to test conduit over SSH.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18912
Summary:
Ref T13043. We have ~4 copies of this logic (registration, lost password recovery, set password, set VCS password).
Currently it varies a bit from case to case, but since it's all going to be basically identical once account passwords swap to the new infrastructure, bring it into the Engine so it can live in one place.
This also fixes VCS passwords not being affected by `account.minimum-password-length`.
Test Plan: Hit all errors in "VCS Password" panel. Successfully changed password.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18902
Summary:
Ref T13043. Migrate VCS passwords away from their dedicated table to new the new shared infrastructure.
Future changes will migrate account passwords and remove the old table.
Test Plan:
- Ran migrations.
- Cloned with the same password that was configured before the migrations (worked).
- Cloned with a different, invalid password (failed).
- Changed password.
- Cloned with old password (failed).
- Cloned with new password (worked).
- Deleted password in web UI.
- Cloned with old password (failed).
- Set password to the same password as it currently is set to (worked, no "unique" collision).
- Set password to account password. !!This (incorrectly) works for now until account passwords migrate, since the uniqueness check can't see them yet.!!
- Set password to a new unique password.
- Cloned (worked).
- Revoked the password with `bin/auth revoke`.
- Verified web UI shows "no password set".
- Verified that pull no longer works.
- Verified that I can no longer select the revoked password.
- Verified that accounts do not interact:
- Tried to set account B to account A's password (worked).
- Tried to set account B to a password revoked on account A (worked).
- Spot checked the `password` and `passwordtransaction` tables for saniity.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18898
Summary:
See <https://discourse.phabricator-community.org/t/files-created-from-repository-contents-slightly-over-one-chunk-in-size-are-truncated-to-exactly-one-chunk-in-size/988/1>. Three issues here:
- When we finish reading `git cat-file ...` or whatever, we can end up with more than one chunk worth of bytes left in the internal buffer if the read is fast. Use `while` instead of `if` to make sure we write the whole buffer.
- Limiting output with `setStdoutSizeLimit()` isn't really a reliable way to limit the size if we're also reading from the buffer. It's also pretty indirect and confusing. Instead, just let the `FileUploadSource` explicitly implement a byte limit in a straightforward way.
- We weren't setting the time limit correctly on the main path.
Overall, this could cause >4MB files to "write" as 4MB files, with the rest of the file left in the UploadSource buffer. Since these files were technically under the limit, they could return as valid. This was intermittent.
Test Plan:
- Pushed a ~4.2MB file.
- Reloaded Diffusion a bunch, sometimes saw the `while/if` buffer race and produce a 4MB file with a prompt to download it. (Other times, the buffer worked right and the page just says "this file is too big, sorry").
- Applied patches.
- Reloaded Diffusion a bunch, no longer saw bad behavior or truncated files.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18885
Summary:
Fixes T13040. To reproduce:
- View a file with blame enabled, where some line has an associated revision (say, `D123`).
- Edit `D123` so it exists and is a valid revision, but the viewer can't see it.
- Reload the page.
Instead, only add revisions to the map if we actually managed to load them.
Test Plan: Page no longer fatals.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13040
Differential Revision: https://secure.phabricator.com/D18884
Summary: See D18857. Ref T13036. See PHI275. Explain what's going on here a little better since it isn't entirely obvious and debugging these stream parsers is a gigantic pain.
Test Plan: Read text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13036
Differential Revision: https://secure.phabricator.com/D18859
Summary:
Depends on D18856. Ref T13036. See PHI275. When we receive a length frame but the buffer doesn't have any data yet, we currently emit a pointless 0-length data frame on the channel.
For normal chatter this is harmless/valid, but it causes problems when a channel has transitioned into bundle2 mode (probably it indicates "end of stream")?
In any case, it's never helpful, so if we're about to read a data block and don't have any data, just bail out until we see some more data.
Note that we can't end up here //expecting// a 0-length data block: both the `data-length` and `data-bytes` states already handle that properly.
Test Plan: Pushed 4MB changes to a Mercurial repository with Mercurial 4.1.1, was no longer able to hit channel errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13036
Differential Revision: https://secure.phabricator.com/D18857
Summary:
Depends on D18855. Ref T13036. This comment no longer seems to be accurate: anything we send over `stderr` is faithfully shown to the user with recent clients.
From [[ https://www.mercurial-scm.org/repo/hg/file/default/mercurial/help/internals/wireprotocol.txt | this document ]], the missing sauce may have been:
```
A generic error response type is also supported. It consists of a an error
message written to ``stderr`` followed by ``\n-\n``. In addition, ``\n`` is
written to ``stdout``.
```
That is, writing "\n" to stdout in addition to writing the error to stderr. However, this no longer appears to be necessary.
I think the modern client behavior is generally sensible (and consistent with the behavior of Git and Subversion) so this //probably// isn't a bug or me making a mistake.
Test Plan: With a modern client, threw some arbitrary exception during execution. Observed a helpful message on the client with no additional steps.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13036
Differential Revision: https://secure.phabricator.com/D18856
Summary:
Ref T13036. This code attempts to filter the "capabilities" message to remove "bundle2", but I think this has never worked.
Specifically, the //write// pathway is hooked, and "write" here means "client is writing a message to the server". However, the "capabilities" frame is part of the response, not part of the request. Thus, this code never fires, at least on recent versions of Mercurial.
Since I plan to support bundle2 and don't want to decode response frames, just get rid of this, assuming we'll achieve those goals.
I think this was just overlooked in D14241, which probably focused on the HTTP version. This code does (at least, potentially) do something for HTTP.
I'm leaving the actual "strip stuff" code in place for now since I think it's still used on the HTTP pathway.
Test Plan:
- Added debug logging, saw this code never hit even though `hg push --debug` shows the client believing bundle2 is supported.
- Logged both halves of the wire protocol and saw this come from the server, not the client.
- Ran the failing `hg push` of a 4MB file under hg 4.4.1, got the same error as before.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: cspeckmim
Maniphest Tasks: T13036
Differential Revision: https://secure.phabricator.com/D18855
Summary:
Fixes T13031. "Enormous" changes are basically changes which are too large to hold in memory, although the actual definition we use today is "more than 1GB of change text or `git diff` runs for more than 15 minutes".
If an install configures a Herald content rule like "when content matches /XYZ/, do something" and then a user pushes a 30 GB source file, we can't put it into memory to `preg_match()` it. Currently, the way to handle this case is to write a separate Herald rule that rejects enormous changes. However, this isn't obvious and means the default behavior is unsafe.
Make the default behavior safe by rejecting these changes with a message, similar to how we reject "dangerous" changes (which permanently delete or overwrite history) by default.
Also, change a couple of UI strings from "Enormous" to "Very Large" to reduce ambiguity. See <https://discourse.phabricator-community.org/t/herald-enormous-check/822>.
Test Plan: Changed the definition of "enormous" from 1GB to 1 byte. Pushed a change; got rejected. Allowed enormous changes, pushed, got rejected by a Herald rule. Disabled the Herald rule, pushed, got a clean push. Prevented enormous changes again. Grepped for "enormous" elsewhere in the UI.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: joshuaspence
Maniphest Tasks: T13031
Differential Revision: https://secure.phabricator.com/D18850
Summary:
See PHI262. Fixes T12578. Although this is a bit niche and probably better accomplished through advisory/soft measures ("Add blocking reviewers") in most cases, it isn't difficult to implement and doesn't create any technical or product tension.
If installs write a rule that blocks commits, that will probably also naturally lead them to an "add reviewers" rule anyway.
Also, allow packages to be hit with the typeahead. They're valid reviewers but previously you couldn't write rules against them, for no actual reason.
Test Plan: Used test console to run this against commits, got sensible results for the field value.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12578
Differential Revision: https://secure.phabricator.com/D18839
Summary: Ref T13030. See PHI254. This behavior could be cleaner than I've made it, but it fixes the "this is totally broken" issue, replacing a fatal/exception with an informative (just not terribly useful) page.
Test Plan:
- Added a submodule to a repository.
- In Diffusion, clicked some other file next to the submodule, then edited the URI to the submodule path instead.
- Before patch: fatal.
- After patch: relatively useful message about this being a submodule.
Note that it's normally hard to hit this URI directly. In the browse view, submodules are marked up as directories and linked to a separate submodule resolution flow.
{F5321524}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13030
Differential Revision: https://secure.phabricator.com/D18831
Summary: Depends on D18827. Ref T7789. See PHI204. See PHI131. This button got accidentally removed in Diffusion refactoring (`$data` is no longer used).
Test Plan: {F5321459}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D18828
Summary: See PHI131. Ref T7789. Although this probably isn't 100% complete, there don't seem to be any actual, known, practical blocking issues remaining (everything is either heresay or not reproducible).
Test Plan: Tried to push LFS locally, got blocked with a helpful message. Enabled setting, tried to push LFS locally, got a successful push.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D18825
Summary:
See <https://discourse.phabricator-community.org/t/diffusion-observed-mercurial-repository-history-broken/825>.
In D18769, I rewrote this from using the `--branch` flag (which is unsafe and does not function on branches named `--config=x.y` and such).
However, this rewrite accidentally changed the result order, which impacted Mercurial commit hisotry lists and graphs. Swap the order of the constraints so we get newest-to-oldest again, as expected.
Test Plan: Viewed a Mercurial repository's history graph, saw sensible chronology after the patch.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18817
Summary: Ref T13001, URLs that return multiple commits should show a list of those commits. Not sure if the actual list looks very pretty this way, but was wondering if this approach was vaguely correct.
Test Plan:
- Navigate to `install/rPbd3c23`
- User should see a list view providing links to `install/rPbd3c2355e8e2b220ae5e3cbfe4a057c8088c6a38` and `install/rPbd3c239d5aada68a31db5742bbb8ec099074a561`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13001
Differential Revision: https://secure.phabricator.com/D18816
Summary: Ref T13019, adds build status back to Diffusion commits
Test Plan: Open a Diffusion commit that has a build status, property list view should show the build status, but not Subscriptions, Projects, or Tokens.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13019
Differential Revision: https://secure.phabricator.com/D18813
Summary: See PHI234. In T12931 we improved the behavior of Diffusion when a repository's default branch is set to a branch that does not exist, but in T11823 the way refcursors work changed, and we can now get a cursor (just with no positions) back for a deleted branch. When we did, we didn't handle things gracefully.
Test Plan:
- Set default branch to a deleted branch, saw nice error instead of fatal.
- Set default branch to a nonexistent branch which never existed, saw nice error.
- Set default branch to existing "master", saw repository normally.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18811
Summary:
See PHI234. Several issues here:
- The warning about observing a repository in Read/Write mode checks the raw I/O type, not the effective I/O type. That means we can fail to warn if other URIs are set to "Default", and "Default" is "Read/Write" in practice.
- There's just an actual typo which prevents the "Observe" version of this error from triggering properly.
Additionally, add more forceful warnings that "Observe" and "Mirror" mean that you want to //replace// a repository with another one, not that we somehow merge branches selectively. It isn't necessarily obvious that "Observe" doesn't mean "merge/union", since the reasons it can't in the general case are somewhat subtle (conflicts between refs with the same names, detecting ref deletion).
Test Plan:
Read documentation. Hit the error locally by trying to "Observe" while in Read/Write mode:
{F5302655}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18810
Summary:
Use ClassQuery to find datasources for the quick-search.
Mostly, this allows extensions to add quicksearches.
Test Plan:
using `/typeahead/class/`, tested several search terms that make sense.
Removed the tag interface from a datasource, which removed it from results.
Reviewers: epriestley, amckinley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18760
Summary:
Ref T13012. These flags can be exploited by attackers to execute code remotely. See T13012 for discussion and context.
Additionally, harden some Mercurial commands where possible (by using additional quoting or embedding arguments in other constructs) so they resist these flags and behave properly when passed arguments with these values.
Test Plan:
- Added unit tests.
- Verified "--config" and "--debugger" commands are rejected.
- Verified more commands now work properly even with branches and files named `--debugger`, although not all of them do.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13012
Differential Revision: https://secure.phabricator.com/D18769
Summary: Give profile images a little more space, fix "/" spacing, add a tooltip.
Test Plan: {F5251205}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18749
Summary: Depends on D18746. See PHI174. Adds small author portraits next to each blame line (this is similar to GitHub).
Test Plan:
My local test data isn't that great since I don't have commits from a lot of accounts, but looks functional:
{F5251056}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18747
Summary:
Ref PHI174. This reverts most of these changes:
- 37843127e9 / D18481
- 94cad30ac3 / D18474
- 12ae08b6b1 / D18473
- 0a01334172 / D18462
- ac91ab1ef9 / D18452
These changes made the Diffusion blame view very similar to GitHub's blame view. See D18452 for a before/after of the bulk of these changes; the other revisions are bugfixes.
I think this was generally a step backward, and not motivated by solving a specific problem. I've found the new UI less usable than the old one, and at least one install (see PHI174) also has.
In particular, the revision/commit titles are very bulky and not terribly useful; the date column also isn't terribly useful; the "age" color actually IS pretty useful and was heavily de-emphasized.
I've kept one bugfix here (missing `'a'` tag type) and kept the upgraded icon for "Skip Past This Commit".
I'm going to follow this up with some additional changes:
- Show a small author profile icon, similar to GitHub, to address PHI174 more directly.
- Try a zebra-stripe on blocks of rows to make it more clear where changes affected by a particular commit begin and end.
- Try a hue shift, not just a brightness/saturation shift, to make the "age" color more distinct.
- Try computing colors as even steps, not based purely on age. Currently, if a file has one long-distant commit and several recent commits, all the recent ones show up as very bright green. I think this would probably be more useful if they were distributed more evenly across the available color bands.
Test Plan:
Viewed blame views in Diffusion, saw a more compact UI similar to the old UI.
{F5251019}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18746
Summary:
Ref T12680. See PHI167. See that task for discussion.
Rewrite `DiffusionCommitQuery` to work more like `DifferentialRevisionQuery`, and use a UNION to find "all revisions you need to audit OR respond to".
I tried to get this working a little more cleanly than RevisionQuery does, and can probably simplify that now.
Test Plan: Poked at the UI locally without hitting any apparent issues, but my local data is pretty garbage at this point. I'll take a look at how the query plans work on `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12680
Differential Revision: https://secure.phabricator.com/D18722
Summary:
See PHI158. In the RefEngine, we test if any old branch positions have been removed from the repository. This is uncommon (but not impossible) in Mercurial, and corresponds to users deleting branches in Git.
Currently, we end up running `hg log` for each position, in parallel. Because of Python's large startup overhead, this can be resource intensive for repositories with a large number of branches.
We have to do this in the general case because the caller may be asking us to resolve `tip`, `newfeature`, `tip~3`, `9`, etc. However, in the specific case where the refs are 40-digit hashes, we can bulk resolve them if they exist, like this:
```
hg log ... --rev (abcd or def0 or ab12 or ...)
```
In the general case, we could probably do less of this than we currently do (instead of testing all old heads, we could prune the list by removing commits which we know are still pointed to by current heads) but that's a slightly more involved change and the effect here is already dramatic.
Test Plan:
Verified that CPU usage drops from ~110s -> ~0.9s:
Before:
```
epriestley@orbital ~/dev/phabricator $ time ./bin/repository refs nss
Updating refs in "nss"...
Done.
real 0m14.676s
user 1m24.714s
sys 0m21.645s
```
After:
```
epriestley@orbital ~/dev/phabricator $ time ./bin/repository refs nss
Updating refs in "nss"...
Done.
real 0m0.861s
user 0m0.882s
sys 0m0.213s
```
- Manually resolved `blue`, `tip`, `9`, etc., got expected results.
- Tried to resolve invalid hashes, got expected result (no resolution).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18717
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary:
Ref PHI109. Ref T11786. We currently test elapsed time every 64 iterations (since iterations are normally very fast), but at least one install is seeing the page timeout after 30 seconds.
One reason could be that cache fills may occur, and are likely to be much slower than normal iterations. In an extreme case, we could do 64 cache fills before checking the time. Tweak thing so that we always check the time after doing a cache fill, regardless of how many iterations have elapsed since the last attempt.
Additionally, this API method currently accepts an arbitrary number of paths, but implicitly limits each cache query to 500ms. If more than 60 paths are passed, this may exceed 30s. Only let the cache churn for a maximum of 10s across all paths.
If this is more the latter issue than the former, this might replace the GraphCache timeouts with `git` timeouts, but at least our understanding of what's going on here will improve.
Test Plan: This is difficult to test convincingly locally, since I can't reproduce the original issue. It still works after these changes, but it worked fine before these changes too.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11786
Differential Revision: https://secure.phabricator.com/D18692
Summary:
See PHI112. The install presumably wants to generate links to Diffusion commits from an external tool, but only knows the short name of the repository.
Provide a `/source/phabricator/commit/abcdef908273` URI which redirects to the canonical URI for the commit.
Test Plan:
- Visited `/source/` URI for a commit, got a redirect.
- Visited normal URI for a commit, got a commit page.
- Visited `/branches/` and `/tags/` for a `/source/` repository, got proper pages.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18676
Summary:
Ref PHI101. It looks like this was maybe copy/pasted by mistake in recent design refactoring.
We need to pass the full path, not the `basename()` of the path, to the search form.
Test Plan: Searched inside `scripts/test/`, found results inside `scripts/test/`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18664
Summary:
Ref T11823. This is the meaty part of the change, and updates `RefEngine` to use separate RefCursor (for names) and RefPosition (for actual commit positions) tables.
I'll hold this whole series until after the release cut so it has some time to bake on `secure` to look for issues. It's also not a huge problem if there are bugs here since these tables are just caches anyway, although they do feed into some other things, and obviously it's never good to have bugs.
Test Plan:
- This logic can be invoked directly with `bin/repository refs <repository> --trace --verbose`.
- Ran that on unchanged repositories, new branches, removed branches, and modified branches. Saw appropriate output and cursor positions.
- Ran on a mercurial repository to test the close/open logic, saw it correct open/closed state of incorrect positions.
- Browed around Diffusion in various repositories.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18614
Summary:
See <https://discourse.phabricator-community.org/t/unable-to-use-current-mercurial-on-debian-stretch/391>.
The Mercurial commit is helpful in particular: <https://www.mercurial-scm.org/repo/hg/rev/77eaf9539499>
We weren't vulnerable to the security issue (users can not control any part of the command) but pass the working directory explicitly to get past the new safety check.
I left `setCWD()` in place (a few lines below) just because it can't hurt, and in some other contexts it sometimes matter (for example, if commit hooks execute, they might inherit the parent CWD here or in other VCSes).
Test Plan:
- Cloned from a Mercurial repo locally over HTTP.
- Verified that SSH cloning already uses `-R` (it does, see `DiffusionMercurialServeSSHWorkflow`).
- Did not actually upgrade to Mercurial 4.0/4.1.3 to completely verify this, but a user in the Discourse thread asserted that a substantially similar fix worked correctly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18611
Summary: Miss this with earlier pass, updates the VCS password page.
Test Plan: Try to set a vcs password
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18574
Summary: This should have a border
Test Plan: Reload page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18549
Summary: Adds a `MenuName` method to applications that `ProfileMenuItem` uses instead of the application name if set. This improves the home/menu/new user experience at little cost. Also renamed the label from Applications to Favorites, since this menu gets altered to provide more than just applications. This also allows instances to set back to Maniphest if they so choose. Overall I think this direction resolves 95% of my concerns, with maybe a small potential downside which I don't really anticipate. We already name Dashboard panels by their object, and that hasn't really caused confusion. I think these links are similar. I click 'Tasks' and get presented a list of my tasks from Maniphest.
Test Plan: Review each of the name changes as a default new install and a modified install.
Reviewers: epriestley, amckinley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18524
Summary: Simplifies the Repository Management pages to the new fixed column layout. I've also moved "Status" into the Basics page, which feels better, and moved "Documentation" as a nav item to a button in the header. This removed "action list" and "curtain view" from the management panels and uses the new bits from Config/Phacility. Undecided if the icons should stay or go for the nav. Left them in for Diffusion. I want to update the EditEngine pages to display in this UI and not leave the portal, but I haven't dug into that this page. I'm a bit worried it will not easily be possible.
Test Plan:
Generate a svn, git, hg repository, test each of the new pages and each of the new buttons. Activate, deactivate, etc.
{F5164674}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18523
Summary: Implements a new mobile view thats more fullscreen, not boxed, so more space. Fixes issues with mobile tables when scrolling overflowed content.
Test Plan: Test home, branch, tags, code, file browse, graph, compare, history, readme, open revisions, owners.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18505
Summary: Visually selects the button if blame is on.
Test Plan: Turn blame on and off in Diffusion on a file.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18504
Summary: 50% more line, no additional cost! Order Now! Operators are standing by.
Test Plan: Blame a file
Reviewers: epriestley, avivey
Reviewed By: avivey
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18481
Summary: My fake data was 100%, and not all tables have full revision history. This leads to a broken table. Instead check if we have //any// revisions at all, then always show the column, with or without a link inside.
Test Plan: going on a limb this is the correct fix and test on secure... again ...
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18474
Summary: There is still some layout issues with revisions, so I've tested it better and moved it to it's own column
Test Plan: Fake in some revision data, test various sizes and shapes.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18473
Summary: I missed an anchor tag here, adds it back
Test Plan: View blame, click a previous version of the file, click Back to HEAD link.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18451
Summary: Ref T12824, adds more information to the blame view, exposes date, commit summary, lighter colors.
Test Plan:
Review many diffs with and without blame on.
{F5111758}
{F5111759}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Maniphest Tasks: T12824
Differential Revision: https://secure.phabricator.com/D18452
Summary: Moves browseFile to single column, implements Owners as a list under the file (and now directory as well), improved information listed in Owners, and moves actions into the Diffusion action bar instead of the header.
Test Plan:
Test browsing directories, files, text, images, binaries, enabling blame. Mobile and desktop.
{F5111045}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18448
Summary: Adds some basic UI for open / closed state when viewing a list of branches in Mercurial. Fixes T12838
Test Plan: Close and open branches, view list.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12838
Differential Revision: https://secure.phabricator.com/D18447
Summary: Better table layouts here for branches view
Test Plan: Test git, hg repositories. See column go away.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18444
Summary: This is in the crumbs, but a little hidden. Puts branch name at the top of the browse table header.
Test Plan: Review a few branchs, change branch, see new name.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18441
Summary: Adds an icon for default branch, status for branch status
Test Plan: Review `hg` and `git` repositories, change default branch, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18443
Summary: Moves the method up to DiffusionController, so it can be more universally used. Also now center aligns tabs on mobile. Still todo, get search nicely toggled on mobile
Test Plan: Test mobile, desktop. Test search from home, from browse, and browsing a specific path.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18432
Summary: Moving this down the the "bar" to allow pattern search on home. Rebuilds the mobile layout a little.
Test Plan:
Test actions on mobile, desktop, tablet.
{F5100460}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18431
Summary: Roughs this in a little, kinda basic. Allows for grouping results by page. A bit better on mobile. Would like more content return from conduit though.
Test Plan:
Test `CMS`, `cms`, and `OMGLOLWTFBBQ`, desktop and mobile
{F5099081}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18429
Summary: This is only on browse pages, but I think could be global (home) also. Moves it from a button, field, to just a field.
Test Plan:
Review search on desktop, mobile.
{F5098886}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18428
