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:
Fixes T12062. Like the commits from the year 3500, you can artificially build commits with no date information.
We could explicitly store these as `null` to fully respect the underlying datastore. However, I think it's very unlikely that these commits are intentional/meaningful or that this is valuable.
Additionally, "git show" interprets these commits as "Jan 1, 1970". Just store a `0` to mimic its behavior.
Test Plan:
- Following the process in T11537#192019, artificially created a commit with //no// date information (I deleted all date information from the message).
- Used `git show` / `git log --format ...` to inspect it: "Jan 1, 1970" on `git show`, no information at all on `%aD`, `%aT`, etc.
- Pushed it.
- Saw exception for trying to insert empty string into epoch colum from `bin/repository update`.
- Applied patch.
- Got a clean import.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12062
Differential Revision: https://secure.phabricator.com/D17136
Summary:
Fixes T11677. This makes two minor adjustments to the repository import daemons:
- The first step ("Message") now queues at a slightly-lower-than-default (for already-imported repositories) or very-low (for newly importing repositories) priority level.
- The other steps now queue at "default" priority level. This is actually what they already did, but without this change their behavior would be to inherit the priority level of their parents.
This has two effects:
- When adding new repositories to an existing install, they shouldn't block other things from happening anymore.
- The daemons will tend to start one commit and run through all of its steps before starting another commit. This makes progress through the queue more even and predictable.
- Before, they did ALL the message tasks, then ALL the change tasks, etc. This works fine but is confusing/uneven/less-predictable because each type of task takes a different amount of time.
Test Plan:
- Added a new repository.
- Saw all of its "message" steps queue at priority 4000.
- Saw followups queue at priority 2000.
- Saw progress generally "finish what you started" -- go through the queue one commit at a time, instead of one type of task at a time.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11677
Differential Revision: https://secure.phabricator.com/D16585
Summary:
Ref T11665. Currently, when a repository hits an error, we retry it after 15s. This is correct if the error was temporary/transient/config-related (e.g., bad network or administrator setting up credentials) but not so great if the error is long-lasting (completely bad authentication, invalid URI, etc), as it can pile up to a meaningful amount of unnecessary load over time.
Instead, record how many times in a row we've hit an error and adjust backoff behavior: first error is 15s, then 30s, 45s, etc.
Additionally, when computing the backoff for an empty repository, use the repository creation time as though it was the most recent commit. This is a good proxy which gives us reasonable backoff behavior.
This required removing the `CODE_WORKING` messages, since they would have reset the error count. We could restore them (as a different type of message), but I think they aren't particularly useful since cloning usually doesn't take too long and there's more status information avilable now than there was when this stuff was written.
Test Plan:
- Ran `bin/phd debug pull`.
- Saw sensible, increasing backoffs selected for repositories with errors.
- Saw sensible backoffs selected for empty repositories.
Reviewers: chad
Maniphest Tasks: T11665
Differential Revision: https://secure.phabricator.com/D16575
Summary:
Fixes T11537. See that task for discussion.
Although we could accommodate these faithfully, it requires a huge migration and affects one repository on one install which was written with buggy tools.
At least for now, just replace out-of-32-bit-range epoch values with the current time, which is often somewhat close to the real value.
Test Plan:
- Following the instructions in T11537, created commits in 40,000 AD.
- Tried to import them, reproducing the "epoch" database issue.
- Applied the patch.
- Successfully imported future-commits, with some liberties around commit dates. Note that author date (not stored in an `epoch` column) is still shown faithfully:
{F1789302}
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T11537
Differential Revision: https://secure.phabricator.com/D16456
Summary:
Fixes T11269. The basic issue is that `git log` in an empty repository exits with an error message.
Prior to recent Git (2.6?), this message reads:
> fatal: bad default revision 'HEAD'
This message was somewhat recently changed by <ce11360467>. After that, it reads:
> fatal: your current branch 'master' does not have any commits yet
This change isn't //technically// a //complete// fix because you could still hit this issue like this:
- Create an empty repository.
- Push some stuff to `master`.
- Delete `master`.
However, this is very rare and even in this case the repository will fix itself once you push something again. We can try to fix that if any users ever actually hit it.
Test Plan:
- Created a new empty Git repository.
- Ran `bin/repository update Rxx`.
- Before patch: "git log" error because of the empty repository.
- After patch: clean update.
- Also ran `repository update` on a non-empty repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11269
Differential Revision: https://secure.phabricator.com/D16234
Summary:
Ref T11208. See that task for a more detailed description of revprops.
This allows revprop changes in a hosted Subversion repository if the repository has the "allow dangerous changes" flag set.
In the future, we could expand this into real Herald support, but the only use case we have for now is letting `svnsync` work.
Test Plan:
Edited revprops with `svn propset --revprop -r 2 propkey propvalue repositoryuri`:
- Tried before patch, got a "configure a commit hook" error.
- Tried after patch, got a "dangerous change" error.
- Allowed dangerous changes.
- Did a revprop edit.
- Prevented dangerous changes.
- Got an error again.
- Made a normal commit to an SVN repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11208
Differential Revision: https://secure.phabricator.com/D16174
Summary:
Fixes T11180. In Git, it's possible to tag a tag (????). When you do, we try to log the tag-object, which automatically resolves to the commit and fails.
Just skip these. If "A" points at "B" which points at "C", it's fine to ignore "A" and "B" since we'll get the same stuff when we process "C".
Test Plan:
- Tagged a tag.
- Pushed it.
- Discovered it.
- Before patch: got exception similar to the one in T11180.
- After patch: got tag-tag skipped. Also got slightly better error messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11180
Differential Revision: https://secure.phabricator.com/D16149
Summary:
Ref T9028. This allows us to detect when commits are unreachable:
- When a ref (tag, branch, etc) is moved or deleted, store the old thing it pointed at in a list.
- After discovery, go through the list and check if all the stuff on it is still reachable.
- If something isn't, try to follow its ancestors back until we find something that is reachable.
- Then, mark everything we found as unreachable.
- Finally, rebuild the repository summary table to correct the commit count.
Test Plan:
- Deleted a ref, ran `pull` + `refs`, saw oldref in database.
- Ran `discover`, saw it process the oldref, mark the unreachable commit, and update the summary table.
- Visited commit page, saw it properly marked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16133
Summary:
Ref T9028. This improves the daemon behavior for unreachable commits. There is still no way for commits to become marked unreachable on their own.
- When a daemon encounters an unreachable commit, fail permanently.
- When we revive a commit, queue new daemons to process it (since some of the daemons might have failed permanently the first time around).
- Before doing a step on a commit, check if the step has already been done and skip it if it has. This can't happen normally, but will soon be possible if a commit is repeatedly deleted and revived very quickly.
- Steps queued with `bin/repository reparse ...` still execute normally.
Test Plan:
- Used `bin/repository reparse` to run every step, verified they all mark the commit with the proper flag.
- Faked the `reparse` exception in the "skip step" code, used `repository reparse` to skip every step.
- Marked a commit as unreachable, ran `discover`, saw daemons queue for it.
- Ran daemons with `bin/worker execute --id ...`, saw them all skip + queue the next step.
- Marked a commit as unreachable, ran `bin/repository reparse` on it, got permanent failures immediately for each step.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16131
Summary:
Ref T9028. This is the easy part of dealing with deleted commits:
- Add a flag for unreachable commits (nothing sets this flag yet).
- Ignore unreachable commits when querying for known commits during discovery, so we pretend they do not exist.
- When recording a commit, try just reviving an existing unreachable commit first. If that works, bail out.
Test Plan:
- Artificially marked a commit as unreachable with raw SQL.
- Verified it said "deleted: unreachable" in the UI.
- Ran `repository discover --trace --verbose`.
- Saw the discovery process ignore the commit when filling the cache.
- Saw the discovery process revive the commit instead of trying to record it again.
- Web UI now shows the commit as normal.
- Running `repository discover` again doesn't make any further changes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16130
Summary:
Ref T9028. Fixes T6878. Currently, we only fetch and discover branches. This is fine 99% of the time but sometimes commits are pushed to just a tag, e.g.:
```
git checkout <some hash>
nano file.c
git commit -am '...'
git tag wild-wild-west
git push origin wild-wild-west
```
Through a similar process, commits can also be pushed to some arbitrary named ref (we do this for staging areas).
With the current rules, we don't fetch tag refs and won't discover these commits.
Change the rules so:
- we fetch all refs; and
- we discover ancestors of all refs.
Autoclose rules for tags and arbitrary refs are just hard-coded for now. We might make these more flexible in the future, or we might do forks instead, or maybe we'll have to do both.
Test Plan:
Pushed a commit to a tag ONLY (`vegetable1`).
<cf508b8de6>
On `master`, prior to the change:
- Used `update` + `refs` + `discover`.
- Verified tag was not fetched with `git for-each-ref` in local working copy and the web UI.
- Verified commit was not discovered using the web UI.
With this patch applied:
- Used `update`, saw a `refs/*` fetch instead of a `refs/heads/*` fetch.
- Used `git for-each-ref` to verify that tag fetched.
- Used `repository refs`.
- Saw new tag appear in the tags list in the web UI.
- Saw new refcursor appear in refcursor table.
- Used `repository discover --verbose` and examine refs for sanity.
- Saw commit row appear in database.
- Saw commit skeleton appear in web UI.
- Ran `bin/phd debug task`.
- Saw commit fully parse.
{F1689319}
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T6878, T9028
Differential Revision: https://secure.phabricator.com/D16129
Summary:
Ref T4292. For hosted, clustered repositories we have a good way to increment the internal version of the repository: every time a user pushes something, we increment the version by 1.
We don't have a great way to do this for observed/remote repositories because when we `git fetch` we might get nothing, or we might get some changes, and we can't easily tell //what// changes we got.
For example, if we see that another node is at "version 97", and we do a fetch and see some changes, we don't know if we're in sync with them (i.e., also at "version 97") or ahead of them (at "version 98").
This implements a simple way to version an observed repository:
- Take the head of every branch/tag.
- Look them up.
- Pick the biggest internal ID number.
This will work //except// when branches are deleted, which could cause the version to go backward if the "biggest commit" is the one that was deleted. This should be OK, since it's rare and the effects are minor and the repository will "self-heal" on the next actual push.
Test Plan:
- Created an observed repository.
- Ran `bin/repository update` and observed a sensible version number appear in the version table.
- Pushed to the remote, did another update, saw a sensible update.
- Did an update with no push, saw no effect on version number.
- Toggled repository to hosted, saw the version reset.
- Simulated read traffic to out-of-sync node, saw it do a remote fetch.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15986
Summary:
Ref T4292. Currently, we hold one big lock around the whole `bin/repository update` workflow.
When running multiple daemons on different hosts, this lock can end up being contentious. In particular, we'll hold it during `git fetch` on every host globally, even though it's only useful to hold it locally per-device (that is, it's fine/good/expected if `repo001` and `repo002` happen to be fetching from a repository they are observing at the same time).
Instead, split it into two locks:
- One lock is scoped to the current device, and held during pull (usually `git fetch`). This just keeps multiple daemons accidentally running on the same host from making a mess when trying to initialize or update a working copy.
- One lock is scoped globally, and held during discovery. This makes sure daemons on different hosts don't step on each other when updating the database.
If we fail to acquire either lock, assume some other process is legitimately doing the work and bail more quietly instead of fataling. In approximately 100% of cases where users have hit this lock contention, that was the case: some other daemon was running somewhere doing the work and the error didn't actually represent an issue.
If there's an actual problem, we still raise a diagnostically useful message if you run `bin/repository update` manually, so there are still tools to figure out that something is hung or whatever.
Test Plan:
- Ran `bin/repository update`, `pull`, `discover`.
- Added `sleep(5)`, forced processes to contend, got lock exceptions and graceful exit with diagnostic message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15903
Summary:
Ref T10923. Fixes T9554.
When hosting a repository, we currently have a heuristic that tries to detect when you're doing an initial import: if you push more than 7 commits to an empty repository, it counts as an import and we disable mail/feed/etc.
Do something similar for observed repositories: if the repository is empty and we discover more than 7 commits, switch to import mode until we catch up.
This should align behavior with user expectation more often when juggling hosted vs imported repositories.
Test Plan:
- Created a new hosted repository.
- Activated it and allowed it to fully import.
- Added an "Observe URI".
- Saw it automatically drop into "Importing" mode until the import completed.
- Swapped it back to hosted mode.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9554, T10923
Differential Revision: https://secure.phabricator.com/D15877
Summary:
Ref T10748. This migrates and swaps mirroring to `PhabricatorRepositoryURI`, obsoleting `PhabricatorRepositoryMirror`.
This prevents you from editing, adding or disabling mirrors unless you know a secret URI (until the UI cuts over fully), but existing mirroring is not affected.
Test Plan:
- Added a mirroring URI to an old repository.
- Verified it worked with `bin/repository mirror`.
- Migrated forward.
- Verified it still worked with `bin/repository mirror`.
- Wow, mirroring: https://github.com/epriestley/locktopia-mirror
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15841
Summary:
Ref T4039. Long ago these were more freely editable and there were some security concerns around creating a repository, then setting its local path to point somewhere it shouldn't.
Local paths are no longer editable so there's no real reason we need to provide a uniqueness guarantee anymore, but you could still make a mistake with `bin/repository move-paths` by accident, and it's a little cleaner to pull them out into their own column with a key.
(We still don't -- and, largely can't -- guarantee that two paths aren't //equivalent// since one might be symlinked to the other, or symlinked only on some hosts, or whatever, but the primary value here is as a sanity check that you aren't goofing things up and pointing a bunch of repositories at the same working copy by mistake.)
Test Plan:
- Ran migrations.
- Grepped for `local-path`.
- Listed and moved paths with `bin/repository`.
- Created a new repository, verified its local path populated correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4039
Differential Revision: https://secure.phabricator.com/D15837
Summary: Ref T10860. This doesn't change anything, it just separates all this stuff out of `PhabricatorRepository` since I'm planning to add a bit more state to it and it's already pretty big and fairly separable.
Test Plan: Pulled, pushed, browsed Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15790
Summary:
Ref T4292. Before we write or read a hosted, clustered Git repository over SSH, check if another version of the repository exists on another node that is more up-to-date.
If such a version does exist, fetch that version first. This allows reads and writes of any node to always act on the most up-to-date code.
Test Plan: Faked my way through this and got a fetch via `bin/repository update`; this is difficult to test locally and needs more work before we can put it in production.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15757
Summary:
Ref T4245. Two effects:
- First, let hooks work for future repositories without callsigns.
- Second, provide a better error when users push directly to hosted repositories.
Test Plan: Ran `bin/commit-hook PHID-REPO-xxx`.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15293
Summary: Fixes T9701. I don't want to try to autofix this because destroying the directory could destroy important files, but we can improve the error message.
Test Plan: Faked a failure, ran `repository update X`, got a more tailored error message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9701
Differential Revision: https://secure.phabricator.com/D14971
Summary: Ref T4245. These are all descriptive or UI-facing.
Test Plan:
- Ran `bin/repository pull ...` with various identifiers.
- Ran `bin/repository mirror ...` with various identifiers.
- Ran `bin/repository discover ...` with various identifiers.
- Ran `bin/phd debug pull X Y --not Z` with various identifiers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14926
Summary: Fixes T9966. In this unusual, difficult-to-reach case, we throw `Exception` (which has no censoring) instead of `CommandException` (which has censoring). Throw `CommandException` instead.
Test Plan:
- Hacked up a bunch of stuff in order to hit this: disabled origin validation, origin correction, and pointed repository at a bad domain.
- Verified message is now censored correctly.
{F1022217}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9966
Differential Revision: https://secure.phabricator.com/D14745
Summary: As described in T7959, it looks like Diffusion does not provide Mercurial the required HTTP credentials when pulling from an external repository.
Test Plan: Add an external Mercurial repository to Diffusion, that requires HTTP authentication. A private BitBucket repository for example.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Projects: #mercurial, #diffusion
Differential Revision: https://secure.phabricator.com/D14092
Summary: Return `$this` from setter methods for consistency. I started writing a [[https://secure.phabricator.com/differential/diff/32506/ | linter rule]] to detect this, but I don't think it is trivial to do this properly.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13422
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary: Ref T6160. Ref T7100. Mercurial branch heads can be closed; track this state so we can be smarter about it.
Test Plan: Closed a branch, run `repository update`, saw it close in the cursor table.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6160, T7100
Differential Revision: https://secure.phabricator.com/D12550
Summary: Ref T5896. See that task and inline for a description.
Test Plan:
- Ran `bin/repository refs HGTESTX --trace`, saw sensible commands and a reasonable result.
- Faked the heads, set chunk size to 2, got this correct output from the algorithm:
> (((((((((1 or 2) or (3 or 4)) or ((5 or 6) or (7 or 8))) or (((9 or 10) or (11 or 12)) or ((13 or 14) or (15 or 16)))) or ((((17 or 18) or (19 or 20)) or ((21 or 22) or (23 or 24))) or (((25 or 26) or (27 or 28)) or ((29 or 30) or (31 or 32))))) or (((((33 or 34) or (35 or 36)) or ((37 or 38) or (39 or 40))) or (((41 or 42) or (43 or 44)) or ((45 or 46) or (47 or 48)))) or ((((49 or 50) or (51 or 52)) or ((53 or 54) or (55 or 56))) or (((57 or 58) or (59 or 60)) or ((61 or 62) or (63 or 64)))))) or ((((((65 or 66) or (67 or 68)) or ((69 or 70) or (71 or 72))) or (((73 or 74) or (75 or 76)) or ((77 or 78) or (79 or 80)))) or ((((81 or 82) or (83 or 84)) or ((85 or 86) or (87 or 88))) or (((89 or 90) or (91 or 92)) or ((93 or 94) or (95 or 96))))) or (((((97 or 98) or (99 or 100)) or ((101 or 102) or (103 or 104))) or (((105 or 106) or (107 or 108)) or ((109 or 110) or (111 or 112)))) or ((((113 or 114) or (115 or 116)) or ((117 or 118) or (119 or 120))) or (((121 or 122) or (123 or 124)) or ((125 or 126) or (127 or 128))))))) or (((((((129 or 130) or (131 or 132)) or ((133 or 134) or (135 or 136))) or (((137 or 138) or (139 or 140)) or ((141 or 142) or (143 or 144)))) or ((((145 or 146) or (147 or 148)) or ((149 or 150) or (151 or 152))) or (((153 or 154) or (155 or 156)) or ((157 or 158) or (159 or 160))))) or (((((161 or 162) or (163 or 164)) or ((165 or 166) or (167 or 168))) or (((169 or 170) or (171 or 172)) or ((173 or 174) or (175 or 176)))) or ((((177 or 178) or (179 or 180)) or ((181 or 182) or (183 or 184))) or (((185 or 186) or (187 or 188)) or ((189 or 190) or (191 or 192)))))) or ((((((193 or 194) or (195 or 196)) or ((197 or 198) or (199 or 200))) or (((201 or 202) or (203 or 204)) or ((205 or 206) or (207 or 208)))) or ((((209 or 210) or (211 or 212)) or ((213 or 214) or (215 or 216))) or (((217 or 218) or (219 or 220)) or ((221 or 222) or (223 or 224))))) or (((((225 or 226) or (227 or 228)) or ((229 or 230) or (231 or 232))) or (((233 or 234) or (235 or 236)) or ((237 or 238) or (239 or 240)))) or ((((241 or 242) or (243 or 244)) or ((245 or 246) or (247 or 248))) or (((249 or 250) or (251 or 252)) or ((253 or 254) or (255 or 256)))))))) or ((((((((257 or 258) or (259 or 260)) or ((261 or 262) or (263 or 264))) or (((265 or 266) or (267 or 268)) or ((269 or 270) or (271 or 272)))) or ((((273 or 274) or (275 or 276)) or ((277 or 278) or (279 or 280))) or (((281 or 282) or (283 or 284)) or ((285 or 286) or (287 or 288))))) or (((((289 or 290) or (291 or 292)) or ((293 or 294) or (295 or 296))) or (((297 or 298) or (299 or 300)) or ((301 or 302) or (303 or 304)))) or ((((305 or 306) or (307 or 308)) or ((309 or 310) or (311 or 312))) or (((313 or 314) or (315 or 316)) or ((317 or 318) or (319 or 320)))))) or ((((((321 or 322) or (323 or 324)) or ((325 or 326) or (327 or 328))) or (((329 or 330) or (331 or 332)) or ((333 or 334) or (335 or 336)))) or ((((337 or 338) or (339 or 340)) or ((341 or 342) or (343 or 344))) or (((345 or 346) or (347 or 348)) or ((349 or 350) or (351 or 352))))) or (((((353 or 354) or (355 or 356)) or ((357 or 358) or (359 or 360))) or (((361 or 362) or (363 or 364)) or ((365 or 366) or (367 or 368)))) or ((((369 or 370) or (371 or 372)) or ((373 or 374) or (375 or 376))) or (((377 or 378) or (379 or 380)) or ((381 or 382) or (383 or 384))))))) or (((((((385 or 386) or (387 or 388)) or ((389 or 390) or (391 or 392))) or (((393 or 394) or (395 or 396)) or ((397 or 398) or (399 or 400)))) or ((((401 or 402) or (403 or 404)) or ((405 or 406) or (407 or 408))) or (((409 or 410) or (411 or 412)) or ((413 or 414) or (415 or 416))))) or (((((417 or 418) or (419 or 420)) or ((421 or 422) or (423 or 424))) or (((425 or 426) or (427 or 428)) or ((429 or 430) or (431 or 432)))) or ((((433 or 434) or (435 or 436)) or ((437 or 438) or (439 or 440))) or (((441 or 442) or (443 or 444)) or ((445 or 446) or (447 or 448)))))) or ((((((449 or 450) or (451 or 452)) or ((453 or 454) or (455 or 456))) or (((457 or 458) or (459 or 460)) or ((461 or 462) or (463 or 464)))) or ((((465 or 466) or (467 or 468)) or ((469 or 470) or (471 or 472))) or (((473 or 474) or (475 or 476)) or ((477 or 478) or (479 or 480))))) or (((((481 or 482) or (483 or 484)) or ((485 or 486) or (487 or 488))) or (((489 or 490) or (491 or 492)) or ((493 or 494) or (495 or 496)))) or ((((497 or 498) or (499 or 500)) or ((501 or 502) or (503 or 504))) or (((505 or 506) or (507 or 508)) or ((509 or 510) or (511 or 512)))))))))
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T5896
Differential Revision: https://secure.phabricator.com/D12549
Summary:
Ref T6755. In Git and Subversion, running `git clone http://google.com/` or `svn checkout http://google.com/` does not echo the response body.
In Mercurial, it does. Censor it from the output of `hg pull` and `hg clone`. This prevents an attacker from:
- Creating a Mercurial remote repository with URI `http://10.0.0.1/secrets/`; and
- reading the secrets out of the error message after the clone fails.
Test Plan: Set a Mercurial remote URI to a non-Mercurial repository, ran `repository update`, saw censored error message.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6755
Differential Revision: https://secure.phabricator.com/D12170
Summary:
Ref T7522. This is mostly useful in the cluster, but could be useful for external installs too.
If you want to import an instance into a test/dry-run state (in the cluster, to test an import; in the general case, to do something like test new hardware or configuration), you currently risk spamming users with a lot of duplicate notifications. In particular, if Phabricator tracks remotes, both instances will continue importing commits and sending email about them. Both instances will try to publish to mirrors, too, which could be bad news, and both instances will try to update linked services.
Instead, provide a flag to let an instance run in "silent mode", which disables all outbound messaging and data.
We need to remember to support this flag on any new outbound channels, but we add about one of those per year so I think that's reasonable.
Test Plan:
- Flipped config.
- Saw it void email, feed and mirroring.
- Didn't test SMS since it's not really in use yet and not convenient to test.
- (Can you think of any publishing I missed?)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7522
Differential Revision: https://secure.phabricator.com/D12109
Summary:
Fixes T7019. In a cluster environment, pushes currently fail because the commit hook can't identify the instance.
For web processes, the hostname identifies the instance -- but we don't have a hostname in the hook.
For CLI processes, the environment identifies the instance -- but we don't have an environment in the hook under SVN.
Promote the instance identifier into the upstream and pack/unpack it explicitly for hooks. This is probably not useful for anyone but us, but the amount of special-purpose code we're introducing is very small.
I poked at trying to do this in a more general way, but:
- We MUST know this BEFORE we run code, so the normal subclassing stuff is useless.
- I couldn't come up with any other parameter which might ever be useful to pass in.
Test Plan: Used `git push` to push code through proxied HTTP, got a clean push.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7019
Differential Revision: https://secure.phabricator.com/D11495
Summary: Fix a few minor lint issues.
Test Plan: Ran `arc lint`.
Reviewers: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11059
Summary: I don't have `hg` yet on my new laptop; we should just skip tests if the user is missing binaries. Add a convenience method to do this.
Test Plan: Got clean `arc unit --everything` with no `hg` installed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11051
Summary: Fixes T4946. Theoretically.
Test Plan:
iiam
also unit tests.
also
```
cd /var/repo/X
git remote remove origin # simulates origin-missing clone under 1.7.1
cd /path/to/phabricator
./bin/repository pull X
```
and observed no errors
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4946, T5938
Differential Revision: https://secure.phabricator.com/D10855
Summary:
Fixes T5839. If a repository has been force pushed and garbage collected, we might have a ref cursor in the database which still points at the old commit (which no longer exists).
We'll then run a command like `git log <new hash> --not <old hash>` to figure out which commits are newly pushed, and this will bomb out because `<old hash>` is invalid.
Instead, validate all the `<old hash>` values before we try to make use of them.
Test Plan:
- Forced a repository into a bad state by mucking with the datbase, generating a reproducible failure similar to the one in T5839.
- Applied patch.
- `bin/repository update <callsign> --trace` filtered the bad commit and put the repository into the right state.
- Saw new commits recognized correctly.
- Ran `bin/repository update <callsign>` for a Mercurial and SVN repo as a sanity check.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5839
Differential Revision: https://secure.phabricator.com/D10226
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: 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: 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 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
