Summary:
Depends on D20465. Ref T13277. Currently, when a commit is unpublished, we put a single line about it on the "Edit Commit" page. This is pretty much impossible to find.
Move it to the main page. This treatment is more big/bold than I'd probably like to end up, but we should probably overshoot on the explanatory text until users get used to this behavior.
Also, allow searching for only published / unpublished commits.
Test Plan: {F6395705}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20466
Summary:
Depends on D20464. Ref T13277. Broadly:
- Move all the "should publish X" and "why aren't we publishing X" stuff to a separate class (`PhabricatorRepositoryPublisher`).
- Rename things to be more consistent with modern terminology ("Publish", "Permanent Refs").
Test Plan:
This could use some trial-by-fire on `secure`, but:
- Grepped for all symbols.
- Viewed various commits.
- Reparsed commits.
- Here's a commit with an explanation:
{F6394569}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20465
Summary: Depends on D20463. Ref T13277. This flag was added some time before 2015 and I don't think I've ever used it. Just get rid of it.
Test Plan: Grepped for `force-autoclose`, `forceAutoclose`, `AUTOCLOSE_FORCED`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20464
Summary:
Depends on D20462. Ref T13276. Currently, the "Message" parser also updates related tasks and revisions when a commit is published.
For PHI1165, which ran into a race with message parsing, I originally believed we needed to separate this logic and lock + yield to avoid the race. D20462 provides what is probably a better approach for avoiding the race.
Still, I think separating these "update related revisions" and "updated related tasks" chunks into separate workers is a net improvement. There may still be some value in doing lock + yield in the future to deal with other issues, and when we occasionally run into problems with pulling a diff out of the repository to update the revision (usually because the diff is too big) this isolates the problem better and allows the commit to import.
I think the only thing to watch out for here is that Herald may now run before the revision and commit are attached to one another. This is fine for all current Herald rules, we just need to be mindful in implementing new rules.
Test Plan: Used `bin/repository reparse --message` on various commits, including commits that close revisions and close tasks.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20463
Summary: Depends on D20459. Ref T13276. I'll file a followup to actually destroy the table.
Test Plan:
- Grepped for `TABLE_COMMIT`.
- Ran `bin/storage upgrade -f`, got a clean bill of health.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20461
Summary:
Ref T13280. In D20421, I changed our observe strategy to `git fetch <uri>` in all cases.
This doesn't work in an ancient, non-bare repository if `master` is checked out and `master` is also fetch: `git` refuses to overwrite the local ref unless we pass `--update-head-ok`. Pass this flag.
Also, remove some code which examines branches and tags in a special way for non-bare working copies. The old `git fetch <origin>` code without explicit revsets meant that `refs/remotes/orgin/heads/xyz` got updated instead of `refs/heads/xyz`. We now update our local refs in all cases (bare and non-bare) so we can throw away this special casing.
Test Plan:
- Replaced a modern bare working copy with a non-bare working copy by explicitly using `git clone` without `--bare`.
- Ran `bin/repository update`, hit the `--update-head-ok` error. Applied the patch, got a clean update.
- Used the "repository.branchquery" API method...
- ...with "contains" to trigger the "git branch" case. Got identical results after removing the special casing.
- ...without "contains" to trigger the "low level ref" case. Got identical results after removing the special casing.
- Grepped for `isWorkingCopyBare()`. The only remaining callsites deal with hook paths, and genuinely need to be specialized.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13280
Differential Revision: https://secure.phabricator.com/D20450
Summary:
Ref T13279. Charting changes alter how the "line-chart" behavior works, but the "Burnup Chart" still relies on the old behavior.
Although I'm intending to remove "Maniphest > Reports" once Facts is a minimally sufficient replacement, copy this behavior to keep it working until we're ready to pull the trigger.
Also fix a leftover typo from D20435.
Test Plan: Viewed a legacy Maniphest burnup rate report.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20449
Summary:
Depends on D20434. Fixes T5963. Broadly, the issue here is that when:
- You create a new, empty repository.
- Then, you work on some branch other than `master`, without ever creating `master`.
...you get a warning on `git clone`:
> warning: remote HEAD refers to nonexistent ref, unable to checkout
To fix this, point the symbolic-ref HEAD at `refs/heads/<default-branch>` after installing commit hooks.
This fixes the warning, and also means that `git clone` will check out the repository default branch by default, which is nice.
There are a few caveats about this behavior (see T5963 for discussion) but nothing too substantial.
The only real issue is that Git prevents deletion of the default branch without a config setting. Just set that settting.
Test Plan:
See T5963.
In a repository, set `HEAD` to point somewhere invalid. Ran `bin/repository update ...`. Saw HEAD pointed back at the repository default branch.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5963
Differential Revision: https://secure.phabricator.com/D20435
Summary:
Depends on D20427. Ref T13277. As an optimization, when we discover that a commit which was previously only on a non-permanent ref ("tmp-epriestley-123") is now reachable from a permanent ref ("master"), we currently queue only a new "message" parse step.
This is an optimization because these commits previously got the full treatment (feed, publish, audit, etc) as soon as they were discovered. Now, those steps only happen once a commit is reachable from a permanent ref, so we need to run everything.
Test Plan:
- Pushed local "tmp-123" branch to remote tag "tmp-123".
- Updated repository with "bin/repository update", saw commit import as a "not on any permanent ref" commit, with no herald/audit/etc.
- Merged "tmp-123" tag into "master".
- Pushed new "master".
- Updated repository with "bin/repository refs ... --trace --verbose", saw commit detected as now reachable from a permament ref.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20428
Summary:
Depends on D20425. Ref T13277. See PHI1067. There's currently no way to retrieve branch/ref rules over the API, which makes some management operations against a large number of repositories difficult.
Expose these rules to the API.
Test Plan: Called `diffusion.repository.search`, got rules in the result set.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20426
Summary:
Depends on D20423. Ref T13277. Repositories currently have separate toggles for "Autoclose" and "Publishing".
Merge the "Autoclose" toggle into the "Publishing" toggle. I'm unaware of any valid use case for enabling one but not the other.
(This doesn't fix all the documentation, yet.)
Test Plan: Edited a repository, saw only one publishing option.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20424
Summary:
Depends on D20422. Ref T13277. Currently, "track only", "publish", and "autoclose" are three separate ideas. I'd like to generally merge them into a more natural idea called "permanent refs".
Since "Autoclose" effectively now controls both "autoclose" and "publish", rename it.
This doesn't rename all the methods or internals, and the documentation needs an update, but it renames most of the UI-facing stuff.
(You also can only specify branches as "Permanent Refs" today, but we may let you specify tags and other arbitrary refs in the future.)
Test Plan: Grepped, poked around the UI, saw UI show "Permanent" / "Permanent Refs" more often and "Autoclose" less.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20423
Summary:
Depends on D20421. Ref T13277. I'd generally like to move away from "Track Only".
Some of the use cases for "Track Only" (or adjacent to "Track Only") are better resolved with "Fetch Rules" -- basically, rules to fetch only some subset of refs from the observed remote.
Add configurable "Fetch Rules" for Git repositories. For example, if you only want to fetch `master`, you can now speify:
```
refs/heads/master
```
If you only want to fetch branches and tags, you can use:
```
refs/heads/*
refs/tags/*
```
In theory, this is slightly less powerful in the general case than "Track Only", but gives us better behavior in some cases (e.g., when the remote has 50K random temporary branches). In practice, I think this and a better "Autoclose Only" will let us move away from "Track Only", get default behavior which is better aligned with what users actually expect, and dodge all the "track tags/refs" questions.
Test Plan: Configured repositories with "Fetch Refs" rules, used `bin/repository pull --verbose --trace ...` to run pulls, saw expected pull/fetch behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20422
Summary:
Depends on D20420. Ref T13277. We currently spend substantial effort trying to detect and correct the URL of the "origin" remote in Git repositories.
I believe this is unnecessary, and we can always `git fetch <url> ...` to get the desired result instead of `git muck-with-origin + git fetch origin ...`. We already do this in the more recent parts of the codebase (e.g., intracluster sync) and it works correctly in every case I'm aware of.
Test Plan:
- Grepped for `origin`, ` origin `.
- Ran `bin/repository update ...` to fetch a mirrored repository.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20421
Summary:
Depends on D20418. Ref T13277. Fixes T11314.
Currently, when you push commits to some arbitrary ref or tag (like `refs/pull/123` on GitHub, `refs/tags/phabricator/diff/123` on Phabricator, or `refs/changes/whatever` on Gerrit), we do not "autoclose" related objects. This means that we don't process `Ref T123` to create links to tasks, and don't process `Differential Revision: xyz` to close revisions.
However, we //do// still publish these commits. "Publish" means: trigger audits, publish feed stories, and run Herald rules.
- Stop publishing these commits.
- In the UI, show these commits as "Not Permanent" with a note that they are "Not [on any permanent branch]."
These commits will publish and autoclose if they ever become reachable from an "autoclose" ref (most commonly, if they are later merged to "master").
Test Plan:
- Pushed a commit to `refs/tags/quack`.
- Before: got a feed story.
- After: no feed story, UI shows commit as "Not Permanent".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277, T11314
Differential Revision: https://secure.phabricator.com/D20419
Summary:
Depends on D20419. Ref T13277. Fixes T8936. Fixes T9383. Fixes T12300. When you push arbitrary refs to Phabricator, the push log currently complains if those refs are not tags or branches.
Upstream Git now features "notes", and there's no reason to prevent writes to arbitrary refs, particularly beause we plan to start using them soon (see T13278).
Allow these writes as affecting raw refs.
Test Plan:
- Pushed an arbitrary ref.
- Pushed some Git notes.
- Wrote a Herald ref rule, saw "ref" in the dropdown.
{F6376492}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277, T8936, T9383, T12300
Differential Revision: https://secure.phabricator.com/D20420
Summary:
Ref T12164. Ref T13276. Currently, the parsing pipeline copies the author and committer names and PHIDs into the transcaction record as metadata. They are then rendered directly from the metadata.
This makes planned changes to the parsing pipeline (to prevent races when multiple commits matching a single revision are pushed simultaneously) more difficult, and generally won't work with repository identities.
Instead, load the commit and use its identities.
Test Plan: Loaded a revision, saw the same story rendering for a "Closed by commit" story.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276, T12164
Differential Revision: https://secure.phabricator.com/D20418
Summary:
Ref T13276. This was introduced in D2586 to power a "trigger audits when the committed change does not match the reviewed change" feature. It was removed without ceremony in D15939. Broadly, rebases mean that this sort of feature can't really work like this and this approach is inherently unreliable; see also T182.
This property no longer has readers, and is unlikely to get any in the future since my planned pathway for "committed code must match reviewed code, modulo an automated rebase" is automating the rebase via "Land Revision", not comparing the diff text.
Remove this to simplify the flow of data here so that things in T13276 can be fixed more easily.
Test Plan: Grepped for `vsDiff`, no hits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20395
Summary: See downstream <https://phabricator.wikimedia.org/T182232>. We currently don't mark repository handles as closed.
Test Plan: Mentioned two repositories with `R1` (active) and `R2` (inactive). After patch, saw `R2` visually indicated as closed/inactive.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20309
Summary:
Depends on D20292. Ref T13259. This converts the rest of the `getPagingValueMap()` callsites to operate on internal cursors instead.
These are pretty one-off for the most part, so I'll annotate them inline.
Test Plan:
- Grouped tasks by project, sorted by title, paged through them, saw consistent outcomes.
- Queried edges with "edge.search", paged through them using the "after" cursor.
- Poked around the other stuff without catching any brokenness.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20293
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 D20126. I was trying to be a little too cute here with the names and ended up confusing myself, then just tested the method behavior. :/
Test Plan: Persudaded by arguments in D20126.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20135
Summary:
Depends on D20129. Ref T13244. See PHI1058. When a revision has an "Accept" from a package, count the owners as "involved" in the change whether or not any actual human owners are actually accepting reviewers.
If a user owns "/" and uses "force accept" to cause "/src/javascript" to accept, or a user who legitimately owns "/src/javascript" accepts on behalf of the package but not on behalf of themselves (for whatever reason), it generally makes practical sense that these changes have owners involved in them (i.e., that's what a normal user would expect in both cases) and don't need to trigger audits under "no involvement" rules.
Test Plan: Used `bin/repository reparse --force --owners <commit>` to trigger audit logic. Saw a commit owned by `O1` with a revision counted as "involved" when `O1` had accepted the revision, even though no actual human owner had accepted it.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20130
Summary:
Depends on D20124. Ref T13244. See PHI1055. Add a few more builtin audit behaviors to make Owners more flexible.
(At the upper end of flexibility you can trigger audits in a very granular way with Herald, but you tend to need to write one rule per Owners package, and providing a middle ground here has worked reasonably well for "review" rules so far.)
Test Plan:
- Edited a package to select the various different audit rules.
- Used `bin/repository reparse --force --owners <commit>` to trigger package audits under varied conditions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20126
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:
Depends on D20115. See <https://discourse.phabricator-community.org/t/transaction-search-endpoint-does-not-work-on-differential-diffs/2369/>.
Currently, `getApplicationTransactionCommentObject()` throws by default. Subclasses must override it to `return null` to indicate that they don't support comments.
This is silly, and leads to a bunch of code that does a `try / catch` around it, and at least some code (here, `transaction.search`) which doesn't `try / catch` and gets the wrong behavior as a result.
Just make it `return null` by default, meaning "no support for comments". Then remove the `try / catch` stuff and all the `return null` implementations.
Test Plan:
- Grepped for `getApplicationTransactionCommentObject()`, fixed each callsite / definition.
- Called `transaction.search` on a diff with transactions (i.e., not a sourced-from-commit diff).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jbrownEP
Differential Revision: https://secure.phabricator.com/D20121
Summary:
See PHI1015. If you add new repository nodes to a cluster, we may not actually sync some repositories for up to 6 hours (if they've had no commits for 30 days).
Add an explicit check for out-of-sync repositories to trigger background sync.
Test Plan:
- Ran `bin/phd debug pullocal`.
- Fiddled with the `repository_workingcopy` version table to put the local node in and out of sync with the cluster.
- Saw appropriate responses in the daemon (sync; wait if the last sync trigger was too recent).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20078
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:
See PHI1030. When you edit an Almanac object, we attempt to discover all the related objects so we can dirty the repository cluster routing cache: if you modify a Device or Service that's part of a clustered repository, we need to blow away our cached view of the layout.
Currently, we don't correctly find linked Bindings when editing a Device, so we may miss Services which have keys that need to be disabled. Instead, discover these linked objects.
See D17000 for the original implementation and more context.
Test Plan:
- Used `var_dump()` to dump out the discovered objects and dirtied cache keys.
- Before change: editing a Service dirties repository routing keys (this is correct), but editing a Device does not.
- After change: editing a Device now correctly dirties repository routing keys.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20003
Summary: See PHI1030. When installs hit this error, provide more details about which node we ended up on and what's going on.
Test Plan:
```
$ git pull
phabricator-ssh-exec: This repository request (for repository "spellbook") has been incorrectly routed to a cluster host (with device name "local.phacility.net", and hostname "orbital-3.local") which can not serve the request.
The Almanac device address for the correct device may improperly point at this host, or the "device.id" configuration file on this host may be incorrect.
Requests routed within the cluster by Phabricator are always expected to be sent to a node which can serve the request. To prevent loops, this request will not be proxied again.
(This is a read request.)
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
```
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20002
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:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).
The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.
The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.
So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.
Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.
Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19920
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.
All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.
Test Plan: Used `grep willRenderTimeline` to find callsites, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19919
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: 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: Ref T13222. See PHI992. If you lose an entire cluster, you may want to aggressively demote it out of existence. You currently need to `xargs` your way through this. Allow `--demote <service>`, which demotes all devices in a service.
Test Plan: Demoted with `--demote <device>` and `--demote <service>`. Hit the `--promote service` error.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19850
Summary:
Ref T13222. See PHI992. If you've lost an entire cluster (or have lost a device and are willing to make broad assumptions about the state the device was in) you currently have to `xargs` to thaw everything or do something else creative.
Since this workflow is broadly reasonable, provide an easier way to accomplish the goal.
Test Plan:
- Ran with `--all-repositories`, a list of repositories, both (error) and neither (error).
- Saw a helpful new list of affected repositories.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19849
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:
Ref T13222. See PHI992. If you lose all hosts in a service cluster, you may need to get a list of affected repositories to figure out which backups to pull.
Support doing this via the API.
Test Plan: Queried by service PHID and saw service PHIDs in the call results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19848
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 D19827. Ref T13221. Ref T13216. To prepare Repositories for a move to ModularTransactions, throw away some very old transaction rendering code.
This will cause these very old transactions (none of which have been written since at least April 2016) to render "epriestley edited this repository." instead of "epriestley changed the SSH login for this repository from X to Y."
These edits were generally obsoleted by repository URIs, Passphrase credentials, and general modernization.
Test Plan: Grepped for all constants, got no hits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13221, T13216
Differential Revision: https://secure.phabricator.com/D19828
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:
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:
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
