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 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:
Fixes paging on the Diffusion Repository List.
PhabricatorRepositoryQuery needs to specify a behavior for `null` on the OderableColumns definition for the `callsign` column.
See https://phabricator.wikimedia.org/T180457
Test Plan:
1. On an instance with more than 100 repositories
* some of which are missing a callsign
2. Attempt to sort by callsign.
3. See the sorted results
Previously:
3. Exception: "Column "0" has null value, but does not specify a null behavior."
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18773
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:
Ref T11823. I think this is the last callsite which relies on the old data format: `bin/repository parents` rebuilds a cache which we don't currently use very heavily.
Update it to work with the new data.
Test Plan: Ran `bin/repository parents <repository> --trace`, saw successful script execution and reasonable-looking output.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18615
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:
Ref T11823. This change isn't standalone, but prepares for the more involved code change by dropping obsolete columns from the RefCursor table and adding the unique key we need to prevent the ambiguous/duplicate refs issue.
This data was moved to the RefPosition table in D18612.
Test Plan: Ran storage upgrade. See next revision for more substantial testing of this change series.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18613
Summary:
Ref T11823. Currently, we have a "RefCursor" table which stores rows like `<branch or tag name, commit it is pointing at>` with some more data.
Because Mercurial can have a single branch pointing at several different places, this table must allow multiple rows with the same branch or tag name.
Among other things, this means there isn't a single PHID which can be used to identify a branch name in a stable way. However, we have several UIs where we want to be able to do this.
Some specific examples where we run into trouble: in Mercurial, if there are 5 heads for "default", that means there are 5 phids. And currently, if someone deletes a branch, we lose the PHID for it. Instead, we'd rather retain it so the whole world doesn't break if you accidentally delete a branch and then fix it a little later.
(I'll likely hold this until the rest of the logic is fleshed out a little more in followup changes.)
Test Plan: Ran `bin/storage upgrade`, saw the table get created without warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18602
Summary:
Ref T12819. Obsoleted by the Ferret engine "Query" field.
This is a compatibility break, I'll note it in the changelog.
Test Plan: Searched for repositories by name with "Query" instead of "Name Contains".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18588
Summary: Ref T12819. More ferret engine support.
Test Plan: Indexed and searched commits and repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18572
Summary:
Ref T2543. These are the last `ArcanistDifferentialRevisionStatus` callsites.
This removes the very old legacy `precommitRevisionStatus` field, which has no other readers. This was obsoleted by the `CLOSED_FROM_ACCEPTED` stuff, but retained for compatibility.
Test Plan:
- Poked these with the test console, although they're a little tricky to be sure about.
- Grepped for `ArcanistDifferentialRevisionStatus`, no more hits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18416
Summary:
Ref T2543. This cleans up a couple of remaining rough edges:
- We could do an older TYPE_ACTION "close" via the daemons.
- We could do an older TYPE_ACTION "close" via `arc close-revision`, explicitly or implicitly in `arc land`, via API (`differential.close`).
- We could do an older TYPE_ACTION "rethink" ("Plan Changes") via the API, via `arc diff --plan-changes` (`differential.createcomment`).
Move these to modern modular transactions, then get rid of all the validation and application logic for them. This nukes a bunch of `ArcanistDifferentialRevision::...` junk.
Test Plan:
- Used `bin/repository reparse --message rXYZ...` to reparse a commit, closing a corresponding revision.
- Used `differential.close` to close a revision.
- Used `differential.createcomment` to plan changes to a revision.
- Reviewed transaction log for full "closed by commit" message (linking to commit and mentioning author).
- Grepped for `::TYPE_ACTION` to look for remaining callsites, didn't find any.
- Grepped for `differential.close` and `differential.createcomment` in `arcanist/` to look for anything suspicious, seemed clean.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18412
Summary:
Ref T12961. Fixes T4416. Currently, for observed Mercurial repositories, we build a working copy with `pull -u` (for "update").
This should be unnecessary, and we don't do it for hosted Mercurial repositories. We also stopped doing it years ago for Git repositories. We also don't clone Mercurial repositories with a working copy.
It's possible something has slipped through the cracks here so I'll hold this until after the release cut, but I believe there are no actual technical blockers here.
Test Plan:
- Observed a public Mercurial repository on Bitbucket.
- Let it import.
- Browsed commits, branches, file content, etc., without any apparent issues.
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Maniphest Tasks: T12961, T4416
Differential Revision: https://secure.phabricator.com/D18390
Summary:
Ref T2543. Add `isPublished()` to mean: exactly the status 'closed', which is now interally called 'published', but still shown as 'closed' to users.
We have some callsites which are about "exactly that status", vs "any 'closed' status", e.g. including "abandoned".
This also introduces `isChangePlanned()`, which felt less awkward than `isChangesPlanned()` but more consistent than `hasChangesPlanned()` or `isStatusChangesPlanned()` or similar.
Test Plan: `grep`, loaded revisions, requested review.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18341
Summary:
Fixes T12946. `bin/remove destroy` does not remove working copies: it's more dangerous than usual, and we can't do it in the general (clustered) case.
Print a notification message after destroying a repository.
Test Plan:
- Destroyed a repository, got a hint about the working copy.
- Destroyed a task, things worked normally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12946
Differential Revision: https://secure.phabricator.com/D18313
Summary:
Fixes T12942.
- Adds binary version and path information to {nav Config > Version Information}.
- Replaces old code all over the place with new consolidated code.
Test Plan:
{F5073531}
Also faked some cases of missing binaries, bad versions, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12942
Differential Revision: https://secure.phabricator.com/D18306
Summary: Fixes T12945.
Test Plan:
Mostly faked this, got a censored error:
```
$ ./bin/repository update R38
[2017-07-31 19:40:13] EXCEPTION: (Exception) Working copy at "/Users/epriestley/dev/core/repo/local/38/" has a mismatched origin URI, "https://********@example.com/". The expected origin URI is "https://github.com/phacility/libphutil.git". Fix your configuration, or set the remote URI correctly. To avoid breaking anything, Phabricator will not automatically fix this. at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryEngine.php:186]
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12945
Differential Revision: https://secure.phabricator.com/D18304
Summary: This moves the clone details on the Repository Home to a button / dialog. Functionally this is to pull content on the page way up, while giving full space to all the clone options. I think we can build this into some FancyJS if needed, but this seems to clean ui the UI dramatically with little overhead. I don't want to attempt the JS dropdown unless we're sure that's the best path (it exposes the most common URI by default, saving a click).
Test Plan: Tested hg, svn, git repositories and the raw URL page. Test close button.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18203
Summary: Fixes T12840. This adds a parallel "graph" button next to history on home and on the history list page. I'll think more about better placement of how to get to this page with the upcoming redesign that's still sitting in Pholio.
Test Plan: View History, View Graph, Try pager, go to a file, click view history, see no graph button.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12840
Differential Revision: https://secure.phabricator.com/D18131
Summary: Builds out some images to use to identify repositories. Fixes T12825.
Test Plan:
Try setting custom, built in, and null images.
{F4998175}
{F4998192}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12825
Differential Revision: https://secure.phabricator.com/D18116
Summary:
Ref T12613. Currently, the SVNTEST and HGTEST repositories are improperly configured on `secure`. These repositories use VCS systems which do not support synchronization, so they can not be served from cluster services with multiple hosts.
However, I've incorrectly configured them the same way as all the Git repositories, which support synchronization. This causes about 50% of requests to randomly fail (when they reach the wrong host).
Detect this issue and warn the user that the configuration is not valid.
It should be exceptionally difficult for normal installs to run into this.
Test Plan:
- Mostly faked these conditions locally, verified that `secure` really has this configuration.
- I'll push this, verify that the issue is detected correctly in production, then fix the config which should resolve the intermittent issues with SVNTEST.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12613
Differential Revision: https://secure.phabricator.com/D17774
Summary:
Via HackerOne (<https://hackerone.com/reports/220909>). When we close commits in response to "Fixes Txxx", we currently act as the omnipotent user. This allows users to close tasks they can't see by pushing commits with "Fixes Txxx" in the message.
However, we can't actually tell who authored or committed a change: we're just using the "Author" and "Committer" values from Git in most cases, and anyone can forge those. So we can't really get this right, in a security sense.
(We can tell who //pushed// a change if we host it, but that's often not the right user. If GPG signing was more prevalent, we could use that. In the future, we could use side channels like having `arc land` tell Phabrcator who was pushing changes.)
Since I think the impact of this is fairly minor and this isn't //really// a security issue (more of a confusion/abuse/product issue) I think the behavior is okay more-or-less as-is, but we can do better when we do identify an author: drop permissions, and use their privileges to load the tasks which the commit "fixes".
This effectively implements this rule:
> If we identify the author of a commit as user X, that commit can only affect tasks which user X can see and edit.
Note that:
- Commits which we can't identify the author for can still affect any task.
- Any user can forge any other user's identity (or an invalid identity) and affect any task.
So this is just a guard rail to prevent mistakes by good-faith users who type the wrong task IDs, not a real security measure.
Also note that to perform this "attack" you must already have commit access to a repository (or permission to create a repository).
Test Plan:
- Used `bin/repository reparse --message <commit> --force-autoclose` to run the relevant code.
- Made the code `throw` before it actually applied the edit.
- Verified that the edit was rejected if the author was recognized and can not see or could not edit the task.
- Verified that the edit is accepted if the author can see+edit the task.
- Verified that the edit is accepted if we can't figure out who the author is.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17688
Summary:
Ref T12298. The PullLocal daemon has had hibernation code for a little while, but it never actually activated because we don't sleep for more than 15 seconds in any case.
Add a maximum sleep instead and use that to control the longest sleep we'll do for hibernation purposes.
Also, when a repository or repository URI is edited, write a NEEDS_UPDATE event into the message table to make sure the daemons de-hibernate.
Test Plan: Used `bin/phd debug pull`, saw the daemon actually hibernate instead of just sleeping for 15 seconds.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17635
Summary:
This implements a simplistic `PhabricatorRepositoryFulltextEngine`
Currently only the repository name, description, timestamps and
status are indexed.
Note: I had to change the `search index` workflow to disambiguate
PhabricatorRepository from PhabricatorRepositoryCommit
Test Plan:
* ran `./bin/search index --type PhabricatorRepository --force`
* searched for some repositories. Saw reasonable results matching on either title or description.
* Edited a repository in the web ui
* Added unique key words to the repo description.
* I was then able to find that repo by searching for the new keywords.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Tags: #search, #diffusion
Differential Revision: https://secure.phabricator.com/D17300
Summary: Ref T12298. This allows the PullLocal daemon to hibernate like the Trigger daemon, but automatically wakes it back up when it needs to do something.
Test Plan:
- Ran `bin/phd debug pulllocal --trace`.
- Saw the daemon hibernate after doing a checkup on repositories.
- Saw periodic queries to look for new update messages.
- After clicking "Update Now" in the web UI to schedule an update, saw the daemon wake up immediately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17540
Summary:
Ref T10967. Improves some method names:
- `Revision->getReviewerStatus()` -> `Revision->getReviewers()`
- `Revision->attachReviewerStatus()` -> `Revision->attachReviewers()`
- `Reviewer->getStatus()` -> `Reviewer->getReviewerStatus()` (this is mostly to make this more greppable)
Test Plan:
- bunch o' `grep`
- Browsed around.
- If I missed anything, it should fatal in an obvious way. We have a lot of other `getStatus()` calls and it's hard to be sure I got them all.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17522
Summary: Ref T10967. The old name was because we had a `getReviewers()` tied to `needRelationships()`, rename this method to use a simpler and more clear name.
Test Plan: `grep`, browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17519
Summary: Fixes T12416. See that task for discussion. Slightly older versions of `git` do not appear to support use of `--` to separate flags and arguments.
Test Plan:
- Ran `bin/repository update PHABX`.
- In T12416, had a user with Git 2.1.4 confirm that `git ls-remote X` worked while `git ls-remote -- X` failed.
- Read `git help ls-remote` to look for any kind of suspicious `--destroy-the-world` flags, didn't see any that made me uneasy.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T12416
Differential Revision: https://secure.phabricator.com/D17508
Summary:
Ref T12392. The logic currently goes like this:
- Try a fetch.
- If that fails, try repairing the origin URI.
- Then try again.
This is pretty complicated, and we can use this simpler logic instead:
- Set the origin URI to the right value.
- Try a fetch.
Setting the origin URI is very fast. This can normally only get us in any trouble in very obscure situations which haven't occurred for many years:
- Pretty much all of this is already covered by `verifyGitOrigin()`, which we run earlier.
- Origins could be configured to have multiple URIs for some reason, but shouldn't be.
- Years ago, you could configure Phabricator to point at a local repository it didn't own and that could conceivably have a different "origin" that you might not want us to delete. If you did this, the daemons have been spewing errors for 3-4 years without you fixing it. The cost of fixing the remote URI is very small even if anyone is affected by this (just set it back to the old value) and there's zero reason to do this and the scenario is ridiculous.
Test Plan: Ran `bin/repository update PHABX --trace --verbose`, saw fetches go through cleanly after URI adjustment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12392
Differential Revision: https://secure.phabricator.com/D17498
Summary:
Ref T12296. Ref T12392. Currently, when we're observing a remote repository, we periodically run `git fetch ...`.
Instead, periodically run `git ls-remote` (to list refs in the remote) and `git for-each-ref` (to list local refs) and only continue if the two lists are different.
The motivations for this are:
- In T12296, it appears that doing this is //faster// than doing a no-op `git fetch`. This effect seems to reproduce locally in a clean environment (900ms for `ls-remote` + 100ms for `for-each-ref` vs about 1.4s for `fetch`). I don't have any explanation for why this is, but there it is. This isn't a huge change, although the time we're saving does appear to mostly be local CPU time, which is good for us.
- Because we control all writes, we could cache `git for-each-ref` in the future and do fewer disk operations. This doesn't necessarily seem too valuable, though.
- This allows us to tell if a fetch will do anything or not, and make better decisions around clustering (in particular, simplify how observed repository versioning works). With `git fetch`, we can't easily distinguish between "fetch, but nothing changed" and "legitimate fetch".
If a repository updates very regularly we end up doing slightly more work this way (that is, if `ls-remote` always comes back with changes, we do a little extra work), but this is normally very rare.
This might not get non-bare repositories quite right in some cases (i.e., incorrectly detect them as changed when they are unchanged) but we haven't created non-bare repositories for many years.
Test Plan: Ran `bin/repository update --trace --verbose PHABX`, saw sensible construction of local and remote maps and accurate detection of whether a fetch would do anything or not.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12392, T12296
Differential Revision: https://secure.phabricator.com/D17497
Summary:
Ref T12296. This cache is used to cache Git ref heads (branches, tags, etc). Reasonable repositories may have more than 2048 of these.
When we miss the cache, we need to single-get refs to check them, which is relatively expensive.
Increasing the size of the cache to 65535 should only require about 7.5MB of RAM.
Additionally, fill only as much of the cache as actually fits. The FIFO nature of the cache can get us into trouble otherwise.
If we insert "A, B, C, D" and then lookup A, B, C, D, but the cache has maximum size 3, we get this:
- Insert A, B, C, D: cache is now "B, C, D".
- Lookup A: miss, single get, insert, purge, cache is now "C, D, A".
- Lookup B: miss, singel get, insert, purge, cache is now "D, A, B".
Test Plan:
- Reduced cache size to 5, observed reasonable behavior on the `array_slice()` locally with `bin/repository update` + `var_dump()`.
- Used this script to estimate the size of 65535 cache entries as 7.5MB:
```
epriestley@orbital ~ $ cat size.php
<?php
$cache = array();
$mem_start = memory_get_usage();
for ($ii = 0; $ii < 65535; $ii++) {
$cache[sha1($ii)] = true;
}
echo number_format(memory_get_usage() - $mem_start)." bytes\n";
epriestley@orbital ~ $ php -f size.php
7,602,176 bytes
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12296
Differential Revision: https://secure.phabricator.com/D17409
Summary:
Ref T12173.
- If we want to fetch a tag, Buildkite needs it as a "branch" (this means more like "ref to fetch").
- The API gets upset if we pass "refs/tags/...", so just pass the tag name without the prefix, which works.
- Do a better job with commits and pass a real branch to fetch.
Test Plan:
- Built a commit with Buildkite.
- Build a revision with Buildkite.
Reviewers: chad
Reviewed By: chad
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12173
Differential Revision: https://secure.phabricator.com/D17282
Summary:
Ref T10978. This updates audits triggered by Owners to use a modern transaction. Minor changes:
- After D17264, we no longer need the "AUDIT_NOT_REQUIRED" fake-audits to record package membership. This no longer creates them.
- This previously saved English-language, untranslatable text strings about audit details onto the audit relationship. I've removed them, per discussion in D17263.
The "Audit Reasons" here are potentially a little more useful than the Herald/Explicit-By-Owner ones were, since the rules are a little more complex, but I'd still like to see evidence that we need them.
In particular, the transaction record now says "Owners added auditors: ...", just like Differential, so the source of the auditors should be clear:
{F2549087}
T11118 (roughly "add several Owners audit modes", despite the title at time of writing) might impact this too. Basically, this is simple and maybe good enough; if it's not quite good enough we can refine it.
Test Plan: Ran `bin/repository reparse --owners <commit>` saw appropriate owners audits trigger.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17266
Summary:
Ref T10978. Currently, during commit import, we write an "Audit Not Required" auditor for commits which don't require an audit.
This auditor is used to power the "Commits in this package" query in Owners.
This conflates audits and commit/package membership. I think it might even predate edges. Code needs to dance around this mess and we get the wrong result in some cases, since auditors are now editable.
Instead, write an explicit edge which just says "this commit is part of such-and-such packages". Then use that to run the query. Logical!
I'll issue guidance on this but I'm not migrating it, since it fixes itself going forward and only really affects the UI in Owners.
Test Plan:
- Ran `bin/audit update-owners` with various arguments.
- Viewed packages in web UI, saw them load the proper commits.
- Queried by packages in Diffusion explicitly.
- Clicked the "View All" link in Owners and got to the right search UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17264
Summary:
Fixes T2393. This allows authors to explicitly say "I think I fixed everything, please accept my commit now thank you".
Also improves behavior of "re-accept" and "re-reject" after new auditors you have authority over get added.
Test Plan:
- Kicked a commit back and forth between an author and auditor by alternately using "Request Verification" and "Raise Concern".
- Verified it showed up properly in bucketing for both users.
- Accepted, added a project, accepted again (works now; didn't before).
- Audited on behalf of projects / packages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2393
Differential Revision: https://secure.phabricator.com/D17252
Summary:
Ref T2393. We had three copies of this code ("which packages/projects can a user accept on behalf of?"). I removed one in D17250. This consolidates the other two.
This still isn't perfect and it should probably live in a Query or something some day, but there's some weird stuff going on with the viewer in the editor context, and at least the code handles the viewer correctly now and isn't living somewhere weird and totally unrelated to auditing, and the callsites don't need to do a bunch of extra work.
This also moves towards fixing the "re-accept if you've already accepted but then a new package you have authority over was added" bug, which we fixed recently in Differential. This should be less common in Audit, but should still be fixed.
Test Plan: Viewed and audited commits with a mixture of user, package, and project auditors. Saw actions apply to the expected set of auditors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2393
Differential Revision: https://secure.phabricator.com/D17251
Summary: Fixes T5889. You can't write a rule like "if no other Herald rules did anything...", but you can use this rule to check for Owners or an explicit "Auditors" field doing things.
Test Plan: Using the test console, ran an "Auditors" rule against a commit with and without an auditor. Got expected pass/fail outcomes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5889
Differential Revision: https://secure.phabricator.com/D17221
Summary: Fixes T6660. Uses the new stuff in Audit to build an EditEngine-aware icon.
Test Plan: {F2364304}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6660
Differential Revision: https://secure.phabricator.com/D17208
Summary: Ref T10978. This is bare bones, but the SearchEngine is at least mostly in reasonable shape now, so get it in place and freeze the old stuff. I previously froze `audit.query`, which did much the same thing.
Test Plan: Issued some queries with the API, technically got results back.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17194
Summary:
Fixes T7076. This could probably use some tweaking but should get the basics in place.
This shows overall object state (e.g., "Needs Review"), not individual viewer state (e.g., "you need to review this"). After the bucketing changes it seems like we're mostly in a reasonable place on showing global state instead of viewer state. This makes the overall change much easier than it might otherwise have been.
Test Plan: {F2351867}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7076
Differential Revision: https://secure.phabricator.com/D17193
Summary: Fixes T7504. I think that task legitimately describes a bug and that the current behavior is counterintuitive.
Test Plan: Manually added an auditor to a commit with none; saw it become "Audit Required" as an overall state.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7504
Differential Revision: https://secure.phabricator.com/D17185
Summary: Ref T10978. Ref T7676. Make auditors work more like reviewers, so they can be freely added or removed.
Test Plan:
- Interacted with auditors via "Edit Commit" and API.
- Comment area is still oldschool and doesn't work yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978, T7676
Differential Revision: https://secure.phabricator.com/D17181
Summary:
Fixes T12087. When transitioning into a clustered configuration for the first time, the documentation recommends using a one-device cluster as a transitional step.
However, installs may not do this for whatever reason, and we aren't as clear as we could be in warning about clusterizing directly into a multi-device cluster.
Roughly, when you do this, we end up believing that working copies exist on several different devices, but have no information about which copy or copies are up to date. //Usually// they all were already synchronized and are all up to date, but we can't make this assumption safely without risking data.
Instead, we err on the side of caution, and require a human to tell us which copy we should consider to be up-to-date, using `bin/repository thaw --promote`.
Test Plan:
```
$ ./bin/repository clusterize rLOCKS --service repos001.phacility.net
Service "repos001.phacility.net" is actively bound to more than one device
(local002.local, local001.phacility.net).
If you clusterize a repository onto this service it will be unclear which
devices have up-to-date copies of the repository. This leader/follower
ambiguity will freeze the repository. You may need to manually promote a
device to unfreeze it. See "Ambiguous Leaders" in the documentation for
discussion.
Continue anyway? [y/N]
```
Read other changes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12087
Differential Revision: https://secure.phabricator.com/D17169
Summary:
Ref T12074. The "v3" API methods (`*.search`, `*.edit`) are currently marked as "unstable", but they're pretty stable and essentially all new code should be using them.
Although these methods are seeing some changes, almost all changes are additive (support for new constraints or attachemnts) and do not break backward compatibility. We have no major, compatibility-breaking changes planned.
I don't want to mark the older methods "deprecated" yet since `arc` still uses a lot of them and there are some capabilities not yet available on the v3 methods, but introduce a new "frozen" status with pointers to the new methods.
Overall, this should gently push users toward the newer methods.
Test Plan: {F2325323}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12074
Differential Revision: https://secure.phabricator.com/D17158
