Summary: Toss the hard-codes and use slim tags.
Test Plan: Scoped out task list.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9748
Summary: Fix for T4990, using export TERM directly in pre receive hook, tested for git
Test Plan:
pushing into repository over ssh will now not cause remote warning
No entry for terminal type "unknown";
using dumb terminal settings.
Tested with git
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Projects: #diffusion, #repositories
Maniphest Tasks: T4990
Differential Revision: https://secure.phabricator.com/D9744
Summary:
Standardizes tag rendering in Maniphest and Maniphest/Diffusion list views.
(This might need some size/spacing tweaks, I tried to make it look reasonable.)
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9741
Summary:
Fixes T5445. Some import tools and other unusual situations can leave repositories with commits that don't have authors. This fails on insert.
Instead, explicitly cast the value to a string.
Test Plan: I didn't build a local repro, but see task/GitHub.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5445
Differential Revision: https://secure.phabricator.com/D9684
Summary: Ref T1049. This moves the declaration of build variables onto HarbormasterBuildableInterface, allowing new classes implementing HarbormasterBuildableInterface to declare their own variables.
Test Plan: Implemented it on another class, saw the build variables appear.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D9618
Summary:
Via HackerOne. There are two attacks here:
- Configuring mirroring to a `file://` URI to place files on disk or overwrite another repository. This is not particularly severe.
- Configuring cloning from a `file://` URI to read repositories you should not have access to. This is more severe.
Historically, repository creation and editing explicitly supported `file://` URIs to deal with use cases where you had something else managing repositories on the same machine. Since there were no permissions, repository management was admin-only, and you couldn't mirror, this was fine.
As we've evolved, this use case is a tiny minority use case and the security implications of `file://` URIs overwhelm the utility it provides. Prevent the use of `file://` URIs. Existing configured repositories won't stop working, you just can't add any new ones.
Also prevent `localPath` from being set via Conduit (see T4039).
Test Plan:
- Tried to create a `file://` repository.
- Tried to create a `file://` mirror.
- Tried to create a `file://` repository via Conduit.
- Created a non-`file://` repository.
- Created a non-`file://` mirror.
- Created a non-`file://` repository via Conduit.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9513
Summary: Ref T4986. Instead of requiring users to know the name of an application search engine class, let them select from a list.
Test Plan:
Created a new panel.
{F165468}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9500
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary: This went smoother than expeced. Makes the rounded Card the default, also tweaked selected state a little.
Test Plan:
Test UIExamples, Maniphest, Home, Differential, Harbormaster, Audit. Everything seems normal
{F163971}
{F163973}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9408
Summary: Ref T4045. Ref T5179. Send all new writes into the modern store.
Test Plan:
- Created a diff.
- Verified it went to the modern store.
- Destroyed a revision, verified hunks were destroyed.
- Also unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9293
Summary:
Ref T4045. Ref T5179. Hunk storage has two major issues:
- It's utf8, but actual diffs are binary.
- It's huge and can't be compressed or archived.
This introduces a second datastore which solves these problems: by recording hunk encoding, supporting compression, and supporting alternate storage. There's no actual compression or storage support yet, but there's space in the table for them.
Since nothing actually uses hunk IDs, it's fine to have these tables exist at the same time and use the same IDs. We can migrate data between the tables gradually without requiring downtime or disrupting installs.
Test Plan:
- There are no writes to the new table yet.
- The only effect this has is making us issue one extra query when looking for hunks.
- Observed the query issue, but everything else continue working fine.
- Created a new diff.
- Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9290
Summary: Ref T5179. Ref T4045. Continue reducing the number of direct hunk loads we perform.
Test Plan: Pushed a closing commit, used `scripts/repository/reparse.php --message ...` to trigger this logic, got a sensible/accurate result.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9288
Summary: Fixes T5255. Currently the `./bin/repository parents` workflow is quite slow. Batching up the SQL operations should make the workflow //seem// much faster.
Test Plan: Not yet tested.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5255
Differential Revision: https://secure.phabricator.com/D9361
Summary: Currently, repositories can be deleted using `./bin/repository delete`. It makes sense to expose this operate to the `./bin/remove` script as well, for consistency.
Test Plan: Deleted a repository with `./bin/remove rTEST`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9350
Summary: Fixes T5226. It's rare (but possible) for a commit to have the same parent more than once in Git.
Test Plan: Ran `bin/repository parents` on a normal repository.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5226
Differential Revision: https://secure.phabricator.com/D9344
Summary:
This probably needs some tweaks, but the idea is to make it easier to browse and access applications without necessarily needing them to be on the homepage.
Open to feedback.
Test Plan:
(This screenshot merges "Organization", "Communication" and "Core" into a single "Core" group. We can't actually do this yet because it wrecks the homepage.)
{F160052}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5176
Differential Revision: https://secure.phabricator.com/D9297
Summary: Fixes T5195. Currently, the `./bin/repository parents` workflow doesn't respect tracked branches and will attempt to build parents caches for all branches.
Test Plan: For at least one of our repositories, this patch fixes the `Unknown commit` exception. Unfortunately, it doesn't seem to completely solve this problem though, but I suspect that this is due to commits that were overwritten with a `git push --force` or similar.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5195
Differential Revision: https://secure.phabricator.com/D9322
Summary: Point everything at the new canonical URI.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9328
Summary:
Ref T4994. This stuff works:
- You can dump a blob of coverage information into `diffusion.updatecoverage`. This wipes existing coverage information and replaces it.
- It shows up when viewing files.
- It shows up when viewing commits.
This stuff does not work:
- When viewing files, the Javascript hover interaction isn't tied in yet.
- We always show this information, even if you're behind the commit where it was generated.
- You can't do incremental updates.
- There's no aggregation at the file (this file has 90% coverage), diff (the changes in this commit are 90% covered), or directory (the code in this directory has 90% coverage) levels yet.
- This is probably not the final form of the UI, storage, or API, so you should expect occasional changes over time. I've marked the method as "Unstable" for now.
Test Plan:
- Ran `save_lint.php` to check for collateral damage; it worked fine.
- Ran `save_lint.php` on a new branch to check creation.
- Published some fake coverage information.
- Viewed an affected commit.
- Viewed an affected file.
{F151915}
{F151916}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: jhurwitz, epriestley, zeeg
Maniphest Tasks: T5044, T4994
Differential Revision: https://secure.phabricator.com/D9022
Summary: Fixes T3044. We currently don't add these to the index.
Test Plan: Made a unique inline comment on a commit, then searched for it.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T3044
Differential Revision: https://secure.phabricator.com/D9170
Summary: Ref T5058. The use of "enum" is confusing; we mean "choose one of these specific string constants". Make this more clear.
Test Plan: Viewed each call from the web UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5058
Differential Revision: https://secure.phabricator.com/D9127
Summary:
Allows you to quickly search for files within a repository. Roughly:
- We build a big tree of everything and ship it to the client.
- The client implements a bunch of Sublime-ish magic to find paths.
Test Plan: {F154007}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D9087
Summary: Ref T4986. Move push logs to a View, then have all the stuff that needs to use it use that View.
Test Plan: Viewed push logs and transaction detail in Diffusion. Created a panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9104
Summary: Ref T2683. This is a small optimization, but it has low complexity: don't rebuild a bucket more than once in the same request, since it will almost always be the same. Bucket rebuilds are pretty cheap, but this saves a few queries.
Test Plan:
- After discovering (but before parsing) a commit, viewed its browse view. Verified that this patch causes us to perform only one bucket rebuild, and therefore reduces the number of queries we issue.
- Parsed the commit and viewed the browse view again, got successful rebuild and then fills from cache.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9055
Summary: Ref T2683. Normally not a big deal, but if a readme has some codeblocks missing the cache can slow things down.
Test Plan:
- Verified we hit the cache.
- Verified TOC still works.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5028, T2683
Differential Revision: https://secure.phabricator.com/D9049
Summary:
Ref T2683. At least locally, browse views are now nearly instantaneous, even in Mercurial. We also fall back to what we were doing before if we miss or take too long, so this shouldn't make things very much worse even in extreme cases.
For a local `hg` repo, the time we spend pulling browse stuff has dropped from ~3,000ms to ~20ms. This is probably atypical, but not completely crazy or rigged or anything.
Test Plan: Viewed Git, Subversion and Mercurial repositories and observed dramatically better performance in Git and Mercurial as they took advantage of the cache.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, jhurwitz
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9047
Summary:
Ref T2683. This is a refinement and simplification of D5257. In particular:
- D5257 only cached the commit chain, not path changes. This meant that we had to go issue an awkward query (which was slow on Facebook's install) periodically while reading the cache. This was reasonable locally but killed performance at FB scale. Instead, we can include path information in the cache. It is very rare that this is large except in Subversion, and we do not need to use this cache in Subversion. In other VCSes, the scale of this data is quite small (a handful of bytes per commit on average).
- D5257 required a large, slow offline computation step. This relies on D9044 to populate parent data so we can build the cache online at will, and let it expire with normal LRU/LFU/whatever semantics. We need this parent data for other reasons anyway.
- D5257 separated graph chunks per-repository. This change assumes we'll be able to pull stuff from APC most of the time and that the cost of switching chunks is not very large, so we can just build one chunk cache across all repositories. This allows the cache to be simpler.
- D5257 needed an offline cache, and used a unique cache structure. Since this one can be built online it can mostly use normal cache code.
- This also supports online appends to the cache.
- Finally, this has a timeout to guarantee a ceiling on the worst case: the worst case is something like a query for a file that has never existed, in a repository which receives exactly 1 commit every time other repositories receive 4095 commits, on a cold cache. If we hit cases like this we can bail after warming the cache up a bit and fall back to asking the VCS for an answer.
This cache isn't perfect, but I believe it will give us substantial gains in the average case. It can often satisfy "average-looking" queries in 4-8ms, and pathological-ish queries in 20ms on my machine; `hg` usually can't even start up in less than 100ms. The major thing that's attractive about this approach is that it does not require anything external or complicated, and will "just work", even producing reasonble improvements for users without APC.
In followups, I'll modify queries to use this cache and see if it holds up in more realistic workloads.
Test Plan:
- Used `bin/repository cache` to examine the behavior of this cache.
- Did some profiling/testing from the web UI using `debug.php`.
- This //appears// to provide a reasonable fast way to issue this query very quickly in the average case, without the various issues that plagued D5257.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, jhurwitz
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9045
Summary:
Ref T4455. This adds a `repository_parents` table which stores `<childCommitID, parentCommitID>` relationships.
For new commits, it is populated when commits are discovered.
For older commits, there's a `bin/repository parents` script to rebuild the data.
Right now, there's no UI suggestion that you should run the script. I haven't come up with a super clean way to do this, and this table will only improve performance for now, so it's not important that we get everyone to run the script right away. I'm just leaving it for the moment, and we can figure out how to tell admins to run it later.
The ultimate goal is to solve T2683, but solving T4455 gets us some stuff anyway (for example, we can serve `diffusion.commitparentsquery` faster out of this cache).
Test Plan:
- Used `bin/repository discover` to discover new commits in Git, SVN and Mercurial repositories.
- Used `bin/repository parents` to rebuild Git and Mercurial repositories (SVN repos just exit with a message).
- Verified that the table appears to be sensible.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: jhurwitz, epriestley
Maniphest Tasks: T4455
Differential Revision: https://secure.phabricator.com/D9044
Summary: Ref T4986. I think this is the last of the easy ones, there are about 10 not-quite-so-trivial ones left.
Test Plan:
- Viewed app results.
- Created panels.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9025
Summary: Ref T4986. These are mostly mechanical now, I skipped a couple of slightly tricky ones. Still a bunch to go.
Test Plan:
For each engine:
- Viewed the application;
- created a panel to issue the query.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9017
Summary:
This is just a general review of config options, to reduce the amount of damage a rogue administrator (without host access) can do. In particular:
- Fix some typos.
- Lock down some options which would potentially let a rogue administrator do something sketchy.
- Most of the new locks relate to having them register a new service account, then redirect services to their account. This potentially allows them to read email.
- Lock down some general disk stuff, which could be troublesome in combination with other vulnerabilities.
Test Plan:
- Read through config options.
- Tried to think about how to do evil things with each one.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8928
Summary:
Fixes T4917. Currently, if a user doesn't have access to, e.g., Phriction, they still get a checkbox in the search results to search for Wiki Documents. Those results will be filtered anyway, so this is confusing at best.
Instead, bind PHID types to applications. This is a relatively tailored fix; some areas for potential future work:
- Go through every PHID type and bind them all to applications. Vaguely nice to have, but doesn't get us anything for now.
- If no searchable application is installed, we don't show you an error state. This isn't currently possible ("People" is always installed) but in the interest of generality we could throw an exception or something at least.
- The elasticserach thing could probably constrain types to visible types, but we don't have a viewer there easily right now.
Test Plan: Uninstalled Phriction, saw the checkbox vanish.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4917
Differential Revision: https://secure.phabricator.com/D8904
Summary: Fixes T4919. There's some special casing in Diffusion for CAN_PUSH right now, just accommodate that until things get more general.
Test Plan: Viewed a repository edit screen with a custom policy transaction. Clicked the link to view it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4919
Differential Revision: https://secure.phabricator.com/D8898
Summary:
Sometimes a commit can be huge (like a branch cut in FB www which could have more than half a million files touched). It will generate some emails with size more than 30M, and it will take quite a while to just sort the files and to send out.
Put a hard limit here to avoid such cases. Probably only matters for FB right now, but still even for a small repo with several thousand files, it is a waste to send them all out. Not sure if there is any cleaner way to do it though.
Test Plan: Tried it in FB installtion.
Reviewers: lifeihuang, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8889
Summary: ...also kills off "PhabricatorAuditCommitQuery" and "PhabricatorAuditQuery", by moving the work to "DiffusionCommitQuery". Generally cleans up some code around the joint on this too. Also provides policies for audit requests, which is basically the policy for the underlying commit. Fixes T4715. (For the TODO I added about files, I just grabbed T4713.)
Test Plan:
Audit: verified the three default views all showed the correct things, including highligthing. did some custom queries and got the correct results.
Diffusion: verified "blame view" still worked. verified paths were highlighted for packages i owned.
Home: verified audit boxes showed up with proper commits w/ audits
bin/audit: played around with it via --dry-run and got the right audits back
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4715
Differential Revision: https://secure.phabricator.com/D8805
Summary: Throwing this up for testing, swapped out all icons in timeline for their font equivelants. Used better icons where I could as well. We should feel free to use more / be fun with the icons when possible since there is no penalty anymore.
Test Plan: I browsed many, not all, timelines in my sandbox and in IE8. Some of these were just swagged, but I'm expecting we'll do more SB testing before landing.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8827
Summary:
Ref T4605. When figuring out how long to wait to update a repository, factor in when it was last pushed. For rarely updated repositories, wait longer between updates.
(A slightly funky thing about this is that empty repos update every 15 seconds, but that seems OK for the moment.)
Test Plan:
Ran `bin/phd debug pulllocal` and saw sensible calculations and output:
```
...
<VERB> PhabricatorRepositoryPullLocalDaemon Last commit to repository "rPOEMS" was 1,239,608 seconds ago; considering a wait of 6,198 seconds before update.
>>> [79] <query> SELECT * FROM `repository` r ORDER BY r.id DESC
<<< [79] <query> 514 us
>>> [80] <query> SELECT * FROM `repository_statusmessage` WHERE statusType = 'needs-update'
<<< [80] <query> 406 us
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rINIH" is not due for an update for 8,754 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rDUCK" is not due for an update for 14 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rMTESTX" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rQWER" is not due for an update for 14 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rBT" is not due for an update for 13 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rSVNX" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rINIG" is not due for an update for 13 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rHGTEST" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rBTX" is not due for an update for 14 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rGX" is not due for an update for 13 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rMTX" is currently updating.
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rPOEMS" is not due for an update for 6,198 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rPHU" is currently updating.
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rSVN" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rPHY" is currently updating.
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rGTEST" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rINIS" is not due for an update for 6,894 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rARCLINT" is not due for an update for 21,599 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rLPHX" is not due for an update for 1,979 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rARC" is not due for an update for 1,824 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rINIHG" is not due for an update for 21,599 second(s).
...
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4605
Differential Revision: https://secure.phabricator.com/D8782
Summary:
Ref T4605. Fixes T3466. The major change here is that we now run up to four simultaneous updates. This should ease cases where, e.g., one very slow repository was blocking other repositories. It also tends to increase load; the next diff will introduce smart backoff for cold repositories to ease this.
The rest of this is just a ton of logging so I can IRC debug these things by having users run them in `phd debug pulllocal` mode.
For T3466:
- You now have to hit four simultaneous hangs to completely block the update process.
- Importing repository updates are killed after 4 hours.
- Imported repository updates are killed after 15 minutes.
Test Plan:
- Ran `phd debug pulllocal` and observed sensible logs and behavior.
- Interrupted daemon from sleeps and processing with `diffusion.looksoon`.
- Ran with various `--not`, `--no-discovery` flags.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3466, T4605
Differential Revision: https://secure.phabricator.com/D8785
Summary:
Ref T4605. Before discovering branches, try to prefill the cache in bulk. For repositories with large numbers of branches, this allows us to issue dramatically fewer queries.
(Before D8780, this cache was usually held across discovery events, so being able to fill it cheaply was not as relevant.)
Test Plan: Ran discovery on Git, Mercurial and SVN repositories. Observed fewer queries for Git/Mercurial.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4605
Differential Revision: https://secure.phabricator.com/D8781
Summary:
Ref T4605. Currently, the PullLocal daemon is responsible for two relatively distinct things:
- scheduling repository updates; and
- actually updating repositories.
Move the "actually updating" part into a new `bin/repository update` command, which basically runs the pull, discover, refs and mirror commands. This will let the parent process focus on scheduling in a more understandable way and update multiple repositories at once. It also makes it easier to debug and understand update behavior since the non-scheduling pipeline can be run separately.
Test Plan:
- Ran `update --trace` on SVN, Mercurial and Git repos.
- Ran PullLocal daemon for a while without issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4605
Differential Revision: https://secure.phabricator.com/D8780
Summary:
See discussion in D8773. Three small adjustments which should help prevent this kind of issue:
- When queueing followup tasks, hold them on the worker until we finish the task, then queue them only if the work was successful.
- Increase the default lease time from 60 seconds to 2 hours. Although most tasks finish in far fewer than 60 seconds, the daemons are generally stable nowadays and these short leases don't serve much of a purpose. I think they also date from an era where lease expiry and failure were less clearly distinguished.
- Increase the default wait-after-failure from 60 seconds to 5 minutes. This largely dates from the MetaMTA era, where Facebook ran services with high failure rates and it was appropriate to repeatedly hammer them until things went through. In modern infrastructure, such failures are rare.
Test Plan:
- Verified that tasks queued properly after the main task was updated.
- Verified that leases default to 7200 seconds.
- Intentionally failed a task and verified default 300 second wait before retry.
- Removed all default leases shorter than 7200 seconds (there was only one).
- Checked all the wait before retry implementations for anything much shorter than 5 minutes (they all seem reasonable).
Reviewers: btrahan, sowedance
Reviewed By: sowedance
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8774
Summary:
Recently we see issues with huge commits (branch cuts for www) where people received hundreds of emails for the same commit. By checking all the active and archived tasks related to such commits, I saw the following pattern:
- The commit itself is marked as importStatus = 15 which means all the processing was actually done;
- In archived tasks, I see one PhabricatorRepositorySvnCommitMessageParserWorker, one PhabricatorRepositorySvnCommitChangeParserWorker, followed by many PhabricatorRepositoryCommitHeraldWorker, which means that the PhabricatorRepositoryCommitOwnersWorker (who schedule those herald tasks) was never done;
- PhabricatorRepositoryCommitOwnersWorker is always active (for days) with failureCount = 0;
- In daemon log I see a lot of lease expire exception for PhabricatorRepositoryCommitOwnersWorker.
So to me it looks like the following happened:
- Everything is fine until we schedule the PhabricatorRepositoryCommitOwnersWorker
- PhabricatorRepositoryCommitOwnersWorker actually successfully finished but its running time exceed 60s. Before it finishes, it scheduled the PhabricatorRepositoryCommitHeraldWorker task
- When we try to archive it, the lease expiration exception happened. As a result, it stayed active and will be picked up immediately since it is in the head of the queue
- The two steps above repeat forever until we kill it
I am not sure why we want to check lease expiration when we are archiving the task. For now I am giving the worker a little more time since parsing half million affected path needs some time..
Test Plan: Patched in our production and it worked.
Reviewers: lifeihuang, JoelB, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8773
Summary: wasn't working due to some type issues. Fixes T4756. I also made it display nicer while I was debugging this.
Test Plan: created a herald rule to block changes that added refs. git tag -a "test" -m "test test"; git push origin test got me blocked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4756
Differential Revision: https://secure.phabricator.com/D8724
Summary:
Fixes T4736. Currently, we incorrectly skip the `writeImportStatusFlag()` call if publishing is disabled (the `herald-disabled`) check. This means we don't flag the commit as imported, and don't move the pipeline forward correctly.
Instead, we only want to skip the owners stuff, not the pipeline stuff. Move that to a method.
(Also fix a nearby TODO now that we have a permanent failure exception.)
Test Plan:
- Used `scripts/repository/reparse.php --owners ...` to execute this code, fiddled with things to hit both the disabled and enabled branches and verified the flag stuff is still reached.
- Faked the exceptions and made sure they raise correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4736
Differential Revision: https://secure.phabricator.com/D8715
Summary: ...also link to commits we know about in "Local Commits" and "Revision Update History" tables. Fixes T4585.
Test Plan: made a repo. made a diff (foo) and committed it (bar). made a new diff that was comprised of two local commits. noted links to (bar) in various commit hashes as expected
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, chad
Maniphest Tasks: T4585
Differential Revision: https://secure.phabricator.com/D8679
