Summary:
Fixes T9279. Modernizes the SearchEngine and Query classes. User-facing changes:
- Added order by commit date, default to order by commit date with newest commits first.
- Added explicit "Needs Audit by".
- Added new `packages(...)` typeahead function.
- Picked up automatic subscribers, projects, and order fields.
This changes behavior a little bit: we previously attempted to exclude, e.g., commits which a package you own needs to audit, but which you have resigned from. This is difficult in general and I think it needs a more comprehensive solution. This shouldn't impact users much, anyway.
Test Plan: {F767628}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9279
Differential Revision: https://secure.phabricator.com/D14013
Summary:
Fixes T7370. Two changes:
- Make the default to show nothing, instead of showing all the data. This is a better default because the data is sometimes sensitive. Workers should have to opt in to revealing it.
- For TargetWorkers, link to the target (technically the build, for now, since there's no dedicated target detail page).
Test Plan: {F698325}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7370
Differential Revision: https://secure.phabricator.com/D13845
Summary: Ref T7149. These formalize the local path adjustment step for cluster imports, rather than relying on an ad-hoc script.
Test Plan: Used `list-paths` and `move-paths` to list and move repositories.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D13621
Summary: Fixes T7604. This is the big scary change which drops the "arcanist project" fields from the database permanently.
Test Plan:
`grep`ped for the following to ensure that I had found all remaining references:
- `/arcanistProject/i`
- `/arcanist_project/i`
- `/projectName/i`
- `/project_name/i`
- `/project_id/i`
WARNING: Wait at least one month before landing this.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12899
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: Ref T8099, functionally I prefer to be able to set anything 'table-like' with `setTable` for design consistency. This looses the restriction and did some light grepping for other missed cases.
Test Plan: Test new UI, grep for other missing cases.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13471
Summary: Fixes T8698. This isn't actually a table.
Test Plan: Viewed push log, no more fatal.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8698
Differential Revision: https://secure.phabricator.com/D13468
Summary:
Ref T8493. Diffusion is probably the strongest upstream use case we have for Spaces right now, so I want to get us on it to kick the tires a bit.
Small amount of hackiness around the multi-page form thing but it shouldn't create any problems.
Test Plan:
- Created a new repo.
- Edited a repo.
- Tried invalid edits, saw value preserved.
- Viewed edit full detail screen, saw space info.
- Viewed repo detail view, saw space.
- Viewed repo list view, saw space.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8493
Differential Revision: https://secure.phabricator.com/D13414
Summary: Fixes T6839. Sometimes, worker tasks go astray for whatever reason. This automates the step of `bin/repository importing | xargs | mangle mangle | bin/repostiory reparse`.
Test Plan: Ran various flavors of the command, got good looking results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6839
Differential Revision: https://secure.phabricator.com/D13362
Summary: Ref T8099. This adds a new class which all search engines return for layout. I thought about this a number of ways, and I think this is the cleanest path. Each Engine can return whatever UI bits they needs, and AppSearch or Dashboard picks and lays the bits out as needed. In the AppSearch case, interfaces like Notifications, Calendar, Legalpad all need more custom layouts. I think this also leaves a resonable path forward for NUX as well. Also, not sure I implemented the class correctly, but assume thats easy to fix?
Test Plan: Review and do a search in each application changed. Grep for all call sites.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13332
Summary: Fixes T8352. Associate Diviner books and atoms with a repository. This relationship is not really surfaced anywhere in the UI but provides metadata that contextualises search results. Depends on D13091.
Test Plan: Ran `diviner generate --repository ARC` and then went to `/diviner/book/arcanist/`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7703, T8352
Differential Revision: https://secure.phabricator.com/D13070
Summary:
Ref T8574. This could fail because the target is disabled (as here), or doesn't have access to the API, or the API request needs to be satisfied by a different host which isn't available.
In any of these cases, just show the failure and continue generating the mail. This field isn't important enough to block the mail, and many of these errors are permanent.
(I'll follow up on T8574 with some more permanent ideas to address this class of issue.)
Test Plan: Faked API call failure, generated mail, saw clean generation of mail with a failure message.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8574
Differential Revision: https://secure.phabricator.com/D13319
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: See also D13186.
Test Plan: Ran `arc unit --everything`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13201
Summary: Ref T8441. I want to use `PhabricatorSearchField` for a better, more useful object.
Test Plan: `grep`, `arc lint`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8441
Differential Revision: https://secure.phabricator.com/D13168
Summary:
Ref T8424. This adds a standard KeyValueCache to serve as a request cache.
In particular, I need to cache Spaces (they are frequently accessed, sometimes by multiple viewers) but not have them survive longer than the scope of one request.
This request cache is explicitly destroyed by each web request and each daemon request.
In the very long term, building this kind of construct supports reusing PHP interpreters to run web requests (see some discussion in T2312).
Test Plan:
- Added and executed unit tests.
- Ran every daemon.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13153
Summary: Ref T6367.
Test Plan:
- Added and executed unit tests.
- Sent mail to A (en_US) and B (en_A*).
- Got one mail in English and one mail in ENGLISH.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13142
Summary:
Ref T6367. Removes `multiplexMail()`!
We can't pass a single body into a function which splits it anymore: we need to split recipients first, then build bodies for each recipient list. This lets us build separate bodies for each recipient's individual translation/access levels.
The new logic does this:
- First, split recipients into groups called "targets".
- Each target corresponds to one actual mail we're going to build.
- Each target has a viewer (whose translation / access levels will be used to generate the mail).
- Each target has a to/cc list (the users who we'll ultimately send the mail to).
- For each target, build a custom mail body based on the viewer's access levels and settings (language prefs not actually implemented).
- Then, deliver the mail.
Test Plan:
- Read new config help.
Then did a bunch of testing, primarily with `bin/mail list-outbound` and `bin/mail show-outbound` (to review generated mail), `bin/phd debug taskmaster` (to run daemons freely) and `bin/worker execute --id <id>` (to repeatedly test a specific piece of code after identifying an issue).
With `one-mail-per-recipient` on (default):
- Sent mail to multiple users.
- Verified mail showed up in `mail list-outbound`.
- Examined mail with `mail show-outbound`.
- Added a project that a subscriber could not see.
- Verified it was not present in `X-Phabricator-Projects`.
- Verified it was rendered as "Restricted Project" for the non-permissioned viewer.
- Added a subscriber, then changed the object policy so they could not see it and sent mail.
- Verified I received mail but the other user did not.
- Enabled public replies and verified mail generated with public addresses.
- Disabld public replies and verified mail generated with private addresses.
With `one-mail-per-recipient` off:
- Verified that one mail is sent to all recipients.
- Verified users who can not see the object are still filtered.
- Verified that partially-visible projects are completely visible in the mail (this violates policies, as documented, as the best available compromise).
- Enabled public replies and verified the mail generated with "Reply To".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: carlsverre, epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13131
Summary: Fixes T7603. This application is no longer used.
Test Plan: Went to `/repository/` and got a 404.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T7603
Differential Revision: https://secure.phabricator.com/D12734
Summary:
Ref T7984. With this, an install can add an ExternalSymbolsSource to src/extensions, which will include whatever
source they have.
Test Plan: search for php and python builtins.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7984
Differential Revision: https://secure.phabricator.com/D13036
Summary:
Ref T8320. Fixes T8317. Fixes T2831. Fixes T8073. Fixes T7127.
There was a bug with this line:
for ($ii = 0; $ii < count($paths); $ii++) {
...because the array may be sparse if there have been deletes, so `count($paths)` might be 3, but the real keys could be `1`, `5` and `6`. I think this was the primary issue behind T7127.
The old Editor did a lot of work to try to validate paths. When a path failed to validate, it silently discarded it. This was silly and pointless: it's incredibly bad UX; and it's totally fine if users saves "invalid" paths. This was likely the cause of T8317, and probably the cause of T8073.
T2831 I'm less sure about, but I can't reproduce it and I rewrote all the logic so I suspect it's gone.
This also records and shows edits, so if stuff does keep happening it should be more clear what's going on.
I removed some adjacent stuff:
- I removed the ability to delete packages. I'll add "disable" in a future diff, plus `bin/remove destroy`, like other objects. Getting rid of this now let me get rid of all the mail stuff.
- I removed "path validation" where packages would try to automatically update in response to commits. This doesn't necessarily make sense in Git/Mercurial, is sketchy, could easily have been the source of T2831, and seems generally complicated and not very valuable. We could maybe restore it some day, but I'd like to get Owners stable before trying to do crazy stuff like that.
Test Plan: {F437687}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8317, T8073, T7127, T2831, T8320
Differential Revision: https://secure.phabricator.com/D13032
Summary:
Ref T8238. This allows configuration of a "staging area" for Git repositories, which is the URI to some Git repository (possibly the same repository).
If a staging area is configured, `arc` will push a copy of anything it creates a diff for there (see next revision). This primarily makes handoff to build systems easier.
This is a bit leaky and I intend for it to eventually be positioned as a less-preferred solution, but from the perspective of build systems it's the same as the real (virtual ref) solution that I want to build.
Test Plan: Ran `arc diff` with various flags, saw appropriate changes copied into the staging area. See also discussion in T8238.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: cburroughs, epriestley
Maniphest Tasks: T8238
Differential Revision: https://secure.phabricator.com/D13019
Summary: Ref T7603. Ref T7604. There is no need for these controllers anymore as the "Arcanist Project" data is not used anywhere. Depends on D12894 and D12898.
Test Plan: Went to `/repository/project/edit/2/` and got a 404.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7603, T7604
Differential Revision: https://secure.phabricator.com/D12897
Summary:
- Give the fields names and descriptions.
- When new, default-disabled fields are added, disable them by default even if there's already a config.
- Be a bit less hacky about `$faux_spec`.
Test Plan: {F432383}
Reviewers: joshuaspence, fabe
Reviewed By: fabe
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13006
Summary: This is vaguely related to T5791. Add a "REPOSITORY" header to audit emails so that they can be filtered in Gmail.
Test Plan: Commented on an audit and used `./bin/mail show-outbound` to inspect outbound email.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: fabe, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12997
Summary:
Includes a new Block in Herad emails which tells the user about which commits were merged in a merge commit
Otherwise the email would just say "Merge branch XYZ". Ref T8295
Test Plan: imported various commits (and merges) and watched resulting herald emails for all of them
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8295
Differential Revision: https://secure.phabricator.com/D12993
Summary: These format strings use `%d` instead of `%s`.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12996
Summary: Ref T7604. Add `encoding` information to the `repository.query` #conduit query.
Test Plan: Changed repository encoding from the UI and saw the encoding returned by the Conduit call.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12992
Summary: Fixes T8283. The code in `FileController` assumes this value is an array.
Test Plan: Loaded any file in Diffusion in unconfigured repository, no more warning.
Reviewers: avivey, joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8283
Differential Revision: https://secure.phabricator.com/D12964
Summary:
Fixes T7977.
- Move Indexed Languages and See Symbols From config to Repository
- Make symbol search skip projects
This also makes the default languages to Everything instead of Nothing.
Test Plan:
- Browse files, click symbols.
- Use quick search to find symbols
- Browse revision, click symbols
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7977
Differential Revision: https://secure.phabricator.com/D12687
Summary: Fixes T8215. When adding a revert edge, only consider commits within the same repository.
Test Plan: Imported the arcanist repository three times and inspected the reverts relationships on rARC490984936bb7ddfc589ef3d376ec2ac2365f070d.
Reviewers: hach-que, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8215
Differential Revision: https://secure.phabricator.com/D12875
Test Plan: If a repository's tracking flag is toggled while it's in queue, we may loop indefinitely.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11296
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Summary: Fixes T8105. We weren't including the parent WHERE clause fragment correctly.
Test Plan: Used `not(project)` in a query.
Reviewers: btrahan, avivey
Reviewed By: avivey
Subscribers: avivey, epriestley
Maniphest Tasks: T8105
Differential Revision: https://secure.phabricator.com/D12746
Summary: Fixes T7220. Ref T7977. Changes symbols from being bound to an Arcanist project to being bound to a repository.
Test Plan:
- Added symbols and then applied migrations, symbols seemed to be migrated successfully.
- Tested the `/diffusion/symbol/$SYMBOL_NAME` endpoint.
- Tested the `/diffusion/symbol/$SYMBOL_NAME` endpoint with the `?repositories=$REPOSITORY_PHID` parameter.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: avivey, Korvin, epriestley
Maniphest Tasks: T7977, T7220
Differential Revision: https://secure.phabricator.com/D12608
Summary:
Fixes T6958. Ref T7484.
- When we collide on a lock in `bin/repository update`, explain what that means.
- GlobalLock currently uses a "lock name" which is different from the lock's actual name. Don't do this. There's a small chance this fixes T7484, but I don't have high hopes.
Test Plan: Ran `bin/repository update X` in two windows really fast, got the new message in one of them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6958, T7484
Differential Revision: https://secure.phabricator.com/D12574
Summary: Seems reasonable? At least, it always matches however a user might think about documents (app or document). Unclear if "Diffusion" for example, are actually needed.
Test Plan: tested searching for "phriction", "wiki", "document", etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12577
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 T4100. Ref T5595.
To support a unified "Projects:" query across all applications, a future diff is going to add a set of "Edge Logic" capabilities to `PolicyAwareQuery` which write the required SELECT, JOIN, WHERE, HAVING and GROUP clauses for you.
With the addition of "Edge Logic", we'll have three systems which may need to build components of query claues: ordering/paging, customfields/applicationsearch, and edge logic.
For most clauses, queries don't currently call into the parent explicitly to get default components. I want to move more query construction logic up the class tree so it can be shared.
For most methods, this isn't a problem, but many subclasses define a `buildWhereClause()`. Make all such definitions protected and consistent.
This causes no behavioral changes.
Test Plan: Ran `arc unit --everything`, which does a pretty through job of verifying this statically.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, hach-que, epriestley
Maniphest Tasks: T4100, T5595
Differential Revision: https://secure.phabricator.com/D12453
Summary: Ref T5750. Make browsing work and improve filtering.
Test Plan:
- Used browse UI.
- Used normal search UI.
{F373752}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12434
Summary: Ref T5750. Update the Almanac service query to be browsable.
Test Plan:
- Browsed and reordered Diffusion.
- Browsed and reordered services in Almanac.
{F373735}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12433
Summary:
Ref T7803. Ref T5873. Allows Query methods to expose orderings from the underlying Query class nearly-for-free.
Callers can specify a string to use a builtin ordering, or an array to use a low-level column ordering.
Test Plan: {F368236}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5873, T7803
Differential Revision: https://secure.phabricator.com/D12381
Summary:
Ref T7803. Ref T5873. I want to drive Conduit through more shared infrastructure, but can't currently add parameters automatically.
Put a `getX()` around the `defineX()` methods so the parent can provide default behaviors.
Also like 60% of methods don't define any special error types; don't require them to implement this method. I want to move away from this in general.
Test Plan:
- Ran `arc unit --everything`.
- Called `conduit.query`.
- Browsed Conduit UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T5873, T7803
Differential Revision: https://secure.phabricator.com/D12380
Summary:
Ref T7803. Currently, available high-level orders are spread across Query and SearchEngine classes and implemented separately for each application.
Lift the concept of "builtin" (high-level, user-facing, named) orders (similar to "builtin" queries in ApplicationSearch) into the root Query class, and let it drive the SearchEngine implementation. This allows you to define a new order in one place and have it automatically work across the entire stack.
This will also let Conduit expose this information in a straightforward way.
Test Plan:
- Used ApplicationSearch in Diffusion.
- Used all result orderings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12379
Summary: Ref T7803. Remove these in favor of more generalized paging and ordering.
Test Plan: Sorted and paged results in various applications.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12378
Summary:
Ref T7803. Removes some getReversePaging().
This also fixes `null` column handling, by adding an explicit `'null'` key with possible values "head" (put NULL before other values) or "tail" (put NULL after other values).
Maniphest has some glitchiness in paging through NULLs right now, but I believe it's all pre-existing and will be resolved when it fully converts. Diffusion is fully converted and pages through NULL correctly.
Test Plan:
- Failed to identify any reason for ChangesetQuery to reverse paging.
- Paged thorugh Diffusion.
- Paged through Maniphest.
- Maniphest has some issues when paging inside a NULL section, but these issues are preexisting and will be resolved later in this change sequence.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12371
Summary: Ref T7803. Reduce the amount of code we're trusting to build SQL queries.
Test Plan:
- Paged through results in Maniphest, Differential and Diffusion.
- Some of the NULLable groups in Maniphest are a bit funky but this was preexisting.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12353
Summary:
Ref T7803. Instead of trusting subqueries to provide safe values, escape them explicitly.
(We'll probably have a few cases somewhere where this doesn't work, but can make them the exception rather than the rule.)
Test Plan: Issued all "order" queries in Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12351
Summary:
Fixes T7484. There's a bunch of spooky mystery here but the current behavior can probably cause problems in at least some situations.
Also moves a couple callsigns to monograms (see T4245).
Test Plan:
- Faked a short lock length to hit the exception.
- Updated normally.
- Grepped for other use sites, none seemed suspicious or likely to overflow the lock length.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7484
Differential Revision: https://secure.phabricator.com/D12263
Summary:
See IRC. We don't always reach `loadPage()`, particularly if `canViewerUseQueryApplication()` fails.
Perform initialization steps in `willExecute()` instead.
Trace from IRC:
```
2015/04/02 11:48:32 [error] 26979#0: *48012 FastCGI sent in stderr: "PHP message: [2015-04-02 11:48:32] EXCEPTION: (Exception) You must execute() the query before accessing the identifier map. at [<phabricator>/src/applications/repository/query/PhabricatorRepositoryQuery.php:134]
PHP message: #0 PhabricatorRepositoryQuery::getIdentifierMap() called at [<phabricator>/src/applications/diffusion/remarkup/DiffusionRepositoryRemarkupRule.php:26]
PHP message: #1 DiffusionRepositoryRemarkupRule::loadObjects(array) called at [<phabricator>/src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php:309]
PHP message: #2 PhabricatorObjectRemarkupRule::didMarkupText() called at [<phutil>/src/markup/engine/PhutilRemarkupEngine.php:292]
PHP message: #3 PhutilRemarkupEngine::postprocessText(array) called at [<phabricator>/src/infrastructure/markup/PhabricatorMarkupEngine.php:138]
PHP message: #4 PhabricatorMarkupEngine::process() called at [<phabricator>/src/infrastructure/markup/PhabricatorMarkupEngine.php:71]
PHP message: #5 PhabricatorMarkupEngine::renderOneObject(PhabricatorMarkupOneOff, string, PhabricatorUser) called at [<phabricator>/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldRemarkup.php:47]
PHP message: #6 PhabricatorStandardCustomFieldRemarkup::renderPropertyViewValue(array) called at [<phabricator>/src/infrastructure/customfield/field/PhabricatorCustomField.php:1191]
PHP message: #7 PhabricatorCustomField::renderPropertyViewValue(array) called at [<phabricator>/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php:176]
PHP message: #8 PhabricatorCustomFieldList::appendFieldsToPropertyList(PhabricatorProject, PhabricatorUser, PHUIPropertyListView) called at [<phabricator>/src/applications/project/controller/PhabricatorProjectProfileController.php:220]
PHP message: #9 PhabricatorProjectProfileController::buildPropertyListView(PhabricatorProject, PhabricatorActionListView) called at [<phabricator>/src/applications/project/controller/PhabricatorProjectPr
```
Test Plan:
- Used `rX` in remarkup.
- Used `rX` in search.
- Browed Diffusion, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12264
Summary: Fixes T7484. If the lock failed, we'd still try to unlock it, which is incorrect.
Test Plan: Ran two `bin/repository update X` in different windows, got proper LockException instead of indirect symptomatic "not locked by this process" exception.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7484
Differential Revision: https://secure.phabricator.com/D12253
Summary:
Ref T7199. These were a bad idea which got copy-pasted a bunch.
- There is zero reason to ever set these to different things.
- Unsurprisingly, I don't know of any install which has them set to different things.
Unless I've completely forgotten about it, this option was not motivated by some obscure business need, it was just a bad decision which didn't catch anyone's attention at the time.
We partially remedied the mistake at some point by introducing `metamta.reply-handler-domain`, which works as a default for all applications, but never cleaned this mess up.
Test Plan: Sent some mail from applications, verified it picked up appropraite reply handler domains.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12231
Summary:
Ref T7199. Although this is useful for discovery, it's un-useful enough that we already have an option to disable it, and most applications do not provide any meaningful instructions.
Throwing it away makes it easier to move forward and lets us get rid of a config option.
This is becoming a more advanced/power-user feature anyway, and the new syntax will be significantly more complex and hard to explain with a one-liner. I'm currently thinking that I'll maybe make the "help" menu a dropdown and give it some options like:
+---+
| O |
+---+---------------------+
| Maniphest Documentation |
| Maniphest Email Actions |
+-------------------------+
Then you click the "Email Actions" thing and get a runtime-derived list of available options. Not sure if I'll actually build that, but I think we can fairly throw the in-mail instructions away even if we don't go in that specific direction.
Test Plan: Grepped for `replyHandlerInstructions`, got no hits.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12229
Summary: Ref T7689. Ref T4100. This advances the goals of removing `loadViewerHandles()` (only 67 callsites remain!) and letting tokenizers some day take token functions like `viewer()` and `members(differential)`.
Test Plan:
- Sent a new message; used "To".
- I simplified the cancel URI construction slightly because it's moot in all normal cases.
- Edited a thread; used "Add Participants".
- Searched rooms; used "Participants".
- Searched countdowns; used "Authors".
- Created a diff; used "Repository".
- Edited a revision; edited "Projects"; edited "Reveiwers"; edited "Subscribers".
- Searched for revisions; edited "responsible users"; "authors"; "reviwers"; "subscribers"; "repositories".
- Added revision comments; edited "Add Reveiwers"; "Add Subscribers".
- Commented on a commit; edited "Add Auditors"; "Add subscribers".
- Edited a commit; edited "Projects".
- Edited a repository; edited "Projects".
- Searched feed, used "include Users"; "include Proejcts".
- Searched files, used "authors".
- Edited initiative; edited "Projects".
- Searched backers; used "Backers".
- Searched initiatives; used "Owners".
- Edited build plans; edited "Run Command".
- Searched Herald; used "Authors".
- Added signature exemption in Legalpad.
- Searhced legalpad; used "creators"; used "contributors".
- Searched signatures; used "documents"; used "signers".
- Created meme.
- Searched macros; used "Authors".
- Used "Projects" in Maniphest reports.
- Used Maniphest comment actions.
- Edited Maniphest tasks; edited "Assigned To"; edited "CC"; edited "projects".
- Used "parent" in Maniphest task creation workflow.
- Searched for projects; used "assigned to"; "in any projec"; "in all projects"; "not in projects"; "in users' projects"; "authors"; "subscribers".
- Edited Maniphest bug filing domains, used "Default Author".
- Searched for OAuth applications, used "Creators".
- Edited Owners pacakge; edited "Primary Owner"; edited "Owners".
- Searched for Owners packages; used "Owner".
- OMG this UI is OLD
- Edited a paste; edited "Projects".
- Searched for paste; used "Authors".
- Searched user activity log; used "Actors"; used "Users".
- Edited a mock; edited "Projects"; edited "CC".
- Searched for mocks; used "Authors".
- Edited Phortune account; edited "Members".
- Edited Phortune merchant account; edited "Members".
- Searched Phrequent; used "Users".
- Edited Ponder question; sued "projects".
- Searched Ponder; used "Authors"; used "Answered By".
- Added project members.
- Searched for projects; used "Members".
- Edited a Releeph product; edited "Pushers".
- Searched pull requests; searched "Requestors".
- Edited an arcanist project; used "Uses Symbols From".
- Searhced push logs; used "Repositories"; used "Pushers".
- Searched repositories; used "In nay project".
- Used global search; used Authors/owners/Subscribers/In Any Project.
- Edited a slowvote; used "Projects".
- Searched slovotes; used "Authors".
- Created a custom "Users" field; edited and searched for it.
- Made a whole lot of typos in this list. ^^^^^^
Did not test:
- Lint is nontrivial to test locally, I'll test it in production.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100, T7689
Differential Revision: https://secure.phabricator.com/D12224
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: Switch to modern components, crumbs, pht, object boxes.
Test Plan:
Test browsing a list, click edit, click new repository, click delete, verify all work as expected.
{F330864}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7427
Differential Revision: https://secure.phabricator.com/D11984
Summary: Ref T6516. We incorrectly fail to set this flag on repositories created via Conduit, which activates too many actions on old commits.
Test Plan:
- Created a new repository via Conduit, verified it was "importing" after creation.
- Created a new repostiory via web UI, verified it was "importing" after creation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6516
Differential Revision: https://secure.phabricator.com/D11964
Summary:
Ref T7298. We are currently inconsistent about when we publish feed, email, notifications, audits and Herald rules.
Specifically, there are two settings which impact these things:
- The "importing" flag, which is set when we're importing old commits.
- The "herald-disabled" flag, which was expanded in scope some time ago and now actually means "disable publishing".
Various parts of the pipeline were checking only one of these flags. Instead, all of them should check both.
(For example, we should never email users about importing repositories, nor trigger audits on them.)
Test Plan: See next revision.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7298
Differential Revision: https://secure.phabricator.com/D11826
Summary: Fixes T6840. Depends on D11822, which is a little iffy.
Test Plan:
Verified all references to `importStatus` are either:
- SQL patches creating the column;
- reads;
- writes immediately before an insert; or
- explicit updates of the column.
That is, I identified no cases of `setImportStatus(X)->save()` on a Commit which may already exist. This //would// break that.
In general, almost all writes go through `$commit->writeImportStatusFlag()`, which is an explicit update.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6840
Differential Revision: https://secure.phabricator.com/D11823
Summary:
Fixes T7310. We have a whole mechanism for surfacing update errors, but only surface actual update errors, not pull errors.
Instead, surface pull errors too.
Then format them a little more nicely.
Test Plan: {F309769}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7310
Differential Revision: https://secure.phabricator.com/D11821
Summary:
Fixes the long uptake we saw on `meta.phacility.com`. I regressed this in D11795.
We make three calls to this method, but only one actually consumes the messages. The other two are just checking to see if there are any messages.
Only move the cursor up if we're actually going to process the messages.
Test Plan: Sort of tricky to test convincingly since it's inherently race-prone, but ran `debug pulllocal` and pushed update messages and saw it pick them up.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11808
Summary:
Ref T6941. In the cluster (and in other reasonable setups) we've separated SSH load balancers from HTTP load balancers.
In particular, ELBs will not let you load balance port 22, so this is likely a reasonable/common issue in larger clusters in AWS.
Allow users to specify an alternate host for SSH traffic.
Test Plan: Set host to someting different, saw it reflected in UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6941
Differential Revision: https://secure.phabricator.com/D11800
Summary: Fixes T7106. If you have bad credentials AND you've pushed an "update this repository" message into the queue, the loop above this level ends up resetting the timer every time we go through it, so the daemon spins in a loop failing forever.
Test Plan:
- Created a repo with bad credentials.
- Clicekd "updated now" to queue an update message.
- Saw daemon run in a loop.
- Applied patch, no loop.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7106
Differential Revision: https://secure.phabricator.com/D11795
Summary: Ref T2783. This cleans up some more of the direct VCS access calls. If the repository is local, this boils down to an in-process call. If not, it uses Conduit to make an intracluster request.
Test Plan: Used `reparse.php --message <commit> --trace` to observe cluster request.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11253
Summary: I think this is safe to remove now.
Test Plan: WIP
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11717
Summary: Adds core and apps grouping to configuration options, makes it somewhat easier to browse config options.
Test Plan: Set each option, review list. Breakdown is nearly 50/50 apps/core.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11722
Summary: Fixes T7118. This does the basic "filter the list" thing, though it ends up being a little manual since I guess this hasn't come up before? There is also potential weird behavior if the user was using an app and lost access to it - they will have nothing selected on edit - but I think this is actually correct behavior in this circumstance.
Test Plan:
used a user who couldn't get access to the "quick create" apps and noted that the dropdown list on dashboard panel create was missing the expected engines
ran `arc unit --everything` to verify abstract method implemented everywhere
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7118
Differential Revision: https://secure.phabricator.com/D11687
Summary: Ref T7094#94295.
Test Plan: noted the absence of the TODO comment in the diff
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11656
Summary: Ref T7094. I guess theoretically someone could be making a commit and have just lost access to the revision and thus this could link this commit to that revision, but this all seems far fetched an weird? We also don't necessarily have the commit author's true identity since commit parsing can be a little funky to begin with. Anyhoo, functionally, this makes things no worse, but I am removing the TODO that would make us look at this in a fun way.
Test Plan: `bin/repository reparse --owners rXvalidhash` and it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11654
Summary: Ref T7094. This makes the underlying class take a $user parameter, and then the worker just hands it an omnipotent user. Said underyling class is the benefactor of a small re-factor, dropping one query per-use, though the single query that now remains is policy-based so maybe its a wash or even worse. Still, gotta love one less query.
Test Plan:
a little tricky to test so some extra thought instead
basic acceptance test with `bin/repository reparse --change rValidHashHere` -- it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11653
Summary: Ref T7094. This loadRepository() method bypassed policy unnecessarily. kill it.
Test Plan: basically un-tested since arcanist projects are deprecated and the main callsites were in releeph. conduit end point still works though!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11586
Summary: This sets an icon for each config, makes it easier to scan.
Test Plan:
Reload Config page, see all new icons
{F281089}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11619
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:
Ref T7019. Ref T7034. In both proxying cases, we want to proxy the request but can not do so over Conduit.
Split the URI resolution apart from Conduit client construction so we can just pull an SSH or HTTP/S URI out of the repository without getting an entire Conduit client.
Test Plan: Browsed around a service-hosted repository. This diff has no behavioral changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7034, T7019
Differential Revision: https://secure.phabricator.com/D11491
Summary: Select a similar or better FontAwesome icon to represent each application
Test Plan: Visual inspection
Reviewers: epriestley, btrahan
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11489
Summary:
Ref T2783. I think this served two purposes:
- Improving performance in cases where we "know" a repository is local.
- Preventing loops.
It is now obsolete:
- After D11476, refs can almost always resolve on a fast path.
- As T2783 moves forward, we can usually no longer know when a repository is local without actually looking it up -- almost everything is allowed to run anywhere.
- The cluster behavior in D11475 now prevents loops.
Test Plan: `grep`, browsed around. This didn't really do much of anything anymore.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11477
Summary:
Fixes T7020. When an external user makes a Conduit request to Diffusion but the repository isn't hosted locally, we need to proxy it.
This also adds a guard layer to prevent requests from getting infinitely proxied inside the cluster.
In "trivial" configurations (where the repository is a service repository, but the service is on the local device) I'm making us always proxy anyway. This basically makes it reasonable to test this stuff (otherwise you'd have to set up two different installs) and this configuration doesn't make much sense in real life (if you're using multiple machines, making one a dedicating daemons+repo box is almost certainly the most reasonable configuration, even for a cluster size of 2).
Test Plan:
- With a service-hosted repository, made Diffusion conduit calls and browsed the UI. Verified requests got proxied once, then resovled.
- With a non-service repository, made Diffusion conduit calls and browsed UI. Verified requests were handled in-process immediately.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7020
Differential Revision: https://secure.phabricator.com/D11475
Summary: Ref T7020. I need this elsewhere, and it's relatively internal anyway.
Test Plan: Browsed around my local, cluster-configured install and saw everything working fine.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7020
Differential Revision: https://secure.phabricator.com/D11474
Summary: Fixes T7021. When I moved around all the timeline stuff I guess I didn't find this "corner" case, which is wildly common in the post-commit review workflow that we don't use.
Test Plan: pre-patch I could reproduce the issue and post patch I could not. The reproduction case is to have a commit with inline comments and then enough subsequent comments to have a "show older" UI. clicking "show older" now works!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7021
Differential Revision: https://secure.phabricator.com/D11479
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from within `PhutilArgumentWorkflow::__construct`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11415
Summary: Ref T6822. This method is only called from `PhutilDaemon::execute()` and can be made `protected`.
Test Plan: See D11404.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11405
Summary: Ref T6822. This method is only called from within the `PhabricatorWorker::executeTask()` and `PhabricatorWorker::scheduleTask()` methods.
Test Plan: `grep`ped for `->doWork`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11406
Summary: Fixes T6922. We should allow the commit pipeline to continue on certain types of exceptions, including `PhabricatorApplicationTransactionNoEffectException`.
Test Plan:
**Before**
```lang=bash
> ./bin/repository reparse --herald rP2660b944bed4e4dde3e66303656b1d96d8b03e9b
[2015-01-10 09:38:06] EXCEPTION: (PhabricatorApplicationTransactionNoEffectException) Transactions have no effect:
- Edges already exist; transaction has no effect. at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:1635]
#0 PhabricatorApplicationTransactionEditor::filterTransactions(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:649]
#1 PhabricatorApplicationTransactionEditor::applyTransactions(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php:91]
#2 PhabricatorRepositoryCommitHeraldWorker::parseCommit(PhabricatorRepository, PhabricatorRepositoryCommit) called at [<phabricator>/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php:44]
#3 PhabricatorRepositoryCommitParserWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:91]
#4 PhabricatorWorker::executeTask() called at [<phabricator>/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php:297]
#5 PhabricatorRepositoryManagementReparseWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:396]
#6 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:292]
#7 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/repository/manage_repositories.php:22]
```
**After**
```lang=bash
> ./bin/repository reparse --herald rP2660b944bed4e4dde3e66303656b1d96d8b03e9b
Done.
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6922
Differential Revision: https://secure.phabricator.com/D11304
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `PhabricatorApplicationSearchEngine` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11242
Summary: Ref T6822.
Test Plan: Visual inspection. This method is only called from within the `PhabricatorRepositoryCommitParserWorker` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11248
Summary:
Fixes T5966. Accomplishes a few things
- see title
- adds a force-autoclose flag and the plumbing for it
- removes references to some HarborMaster thing that used to key off commits and seems long dead, but forgotten :/
Test Plan:
ran a few commands. These first three had great success:
`./repository reparse --all FIRSTREPO --message --change --herald --owners`
`./repository reparse --all FIRSTREPO --message --change --herald --owners --min-date yesterday`
`./repository reparse --all FIRSTREPO --message --change --herald --owners --min-date yesterday --force-autoclose`
...and these next two showed me some errors as expected:
`./repository reparse --all FIRSTREPO --message --change --herald --owners --min-date garbagedata`
`./repository reparse --all GARBAGEREPO --message --change --herald --owners`
Also, made a diff in a repository with autoclose disabled and commited the diff. Later, reparse the diff with force-autoclose. Verified the diff closed and that the reason "why" had the proper message text.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, epriestley, Korvin
Maniphest Tasks: T5966
Differential Revision: https://secure.phabricator.com/D10492
Summary: When updating the status of a task via commit, transaction should show responsible commit and status update if it was changed.
Test Plan: Push a commit "Fixes Txx", transaction should include status update and commit number.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6637
Differential Revision: https://secure.phabricator.com/D11230
Summary: Use `PhabricatorAuditEditor` instead of `PhabricatorEdgeEditor` when writing reverts edges. This ensures that a transaction is created in addition to the edge.
Test Plan: Reverted a commit and pushed to remote. Saw a row created in `phabricator_audit.audit_transaction_comment`. Interestingly, I can't actually see the transaction at http://phabricator.local/r${CALLSIGN}${REVERTED_COMMIT_HASH}.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11212
Summary: Fixes T6608, though I'll also clean up the comment for PhutilStringTruncator in another diff. If I understand correctly, before T1191, MySQL column length was by character count and post T1191 its by byte count. Ergo, most of these changes are going from codepoint -> bytes. See test plan for complete list of what was and was not done.
Test Plan:
Thought very carefully about each callsite and made changes as appropos. "Display" means the string is clearly used for display-only purposes and correctly uses "glyph" already.
grep -rn PhutilUTF8StringTruncator *
applications/calendar/query/PhabricatorCalendarEventSearchEngine.php:217: ->addAttribute(id(new PhutilUTF8StringTruncator()) -- display
applications/chatlog/controller/PhabricatorChatLogChannelLogController.php:111: $author = id(new PhutilUTF8StringTruncator()) -- display
applications/conduit/method/ConduitConnectConduitAPIMethod.php:62: $client_description = id(new PhutilUTF8StringTruncator()) -- was codepoint, changed to bytes
applications/conpherence/view/ConpherenceFileWidgetView.php:22: ->setFileName(id(new PhutilUTF8StringTruncator()) -- display
applications/differential/controller/DifferentialDiffViewController.php:65: id(new PhutilUTF8StringTruncator()) -- display
applications/differential/event/DifferentialHovercardEventListener.php:69: id(new PhutilUTF8StringTruncator()) -- display
applications/differential/parser/DifferentialCommitMessageParser.php:144: $short = id(new PhutilUTF8StringTruncator()) -- was glyphs, made to bytes
applications/differential/view/DifferentialLocalCommitsView.php:80: $summary = id(new PhutilUTF8StringTruncator()) -- display
applications/diffusion/controller/DiffusionBrowseFileController.php:686: id(new PhutilUTF8StringTruncator()) -- display
applications/feed/story/PhabricatorFeedStory.php:392: $text = id(new PhutilUTF8StringTruncator()) -- display, unless people are saving the results of renderSummary() somewhere...
applications/harbormaster/storage/build/HarbormasterBuild.php:216: $log_source = id(new PhutilUTF8StringTruncator()) -- was codepoints now bytes
applications/herald/storage/transcript/HeraldObjectTranscript.php:55: // NOTE: PhutilUTF8StringTruncator has huge runtime for giant strings. -- not applicable
applications/maniphest/export/ManiphestExcelDefaultFormat.php:107: id(new PhutilUTF8StringTruncator()) -- bytes
applications/metamta/storage/PhabricatorMetaMTAMail.php:587: $body = id(new PhutilUTF8StringTruncator()) -- bytes
applications/people/event/PhabricatorPeopleHovercardEventListener.php:62: id(new PhutilUTF8StringTruncator()) -- display
applications/phame/conduit/PhameCreatePostConduitAPIMethod.php:93: id(new PhutilUTF8StringTruncator()) -- was codepoints, now bytes
applications/pholio/storage/PholioTransaction.php:300: id(new PhutilUTF8StringTruncator()) -- display
applications/phortune/provider/PhortuneBalancedPaymentProvider.php:147: $charge_as = id(new PhutilUTF8StringTruncator()) -- bytes
applications/ponder/storage/PonderAnswerTransaction.php:86: id(new PhutilUTF8StringTruncator()) -- display
applications/ponder/storage/PonderQuestionTransaction.php:267: id(new PhutilUTF8StringTruncator()) -- display
applications/ponder/storage/PonderQuestionTransaction.php:276: id(new PhutilUTF8StringTruncator()) -- display
applications/repository/storage/PhabricatorRepositoryCommitData.php:43: $summary = id(new PhutilUTF8StringTruncator()) -- was codepoints, now bytes
applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php:20: $data->setAuthorName(id(new PhutilUTF8StringTruncator()) -- was codepoints, now bytes
applications/slowvote/query/PhabricatorSlowvoteSearchEngine.php:158: $item->addAttribute(id(new PhutilUTF8StringTruncator()) -- display
infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php:317: $host = id(new PhutilUTF8StringTruncator()) -- bytes
view/form/control/AphrontFormPolicyControl.php:61: $policy_short_name = id(new PhutilUTF8StringTruncator()) -- glyphs, probably display only
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6608
Differential Revision: https://secure.phabricator.com/D11219
Summary:
Ref T1751. When a commit reverts another commit:
- Add an edge linking them;
- Show the edge in Diffusion.
Next steps are:
- If the reverted commit is associated with a Differential revision, leave a comment;
- Also leave a comment on the commit (no API yet);
- Also trigger an audit by the original commit's author.
Test Plan: Used `scripts/repository/reparse.php --message ...` to parse commits with revert language. Verified they appear correctly in Diffusion, and update Differential.
Reviewers: btrahan, epriestley
Reviewed By: btrahan, epriestley
Subscribers: Korvin, epriestley, cburroughs, joshuaspence, sascha-egerer, aran
Maniphest Tasks: T4896, T1751
Differential Revision: https://secure.phabricator.com/D5846
Summary: Modernize remaining edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Browsed around and performed various actions include subscribing, unsubscribing and watching.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11116
Summary:
Ref T2783.
This updates PhabricatorRepositoryManagementLookupUsersWorkflow to use ConduitCall to retrieve information about the commit.
Test Plan:
Ran `bin/repository lookup-users rTESTe9683b64d3283f0b2d355fdbf231bc918b5ac0ab --trace` and saw the information returned (by making a request to `diffusion.querycommits` as the omnipotent user, signed with the device key).
Mucked with `cluster.addresses` and saw requests rejected.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
Subscribers: Krenair, epriestley, Korvin
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10403
Summary:
Ref T4245 Make repo callsigns optional
This is far from done and still very ugly. I'm just submitting it to check if i'm solving this in the right places.
Right now there's three places with duplicate code and building the identifierMap in the CommitQuery is very ugly.
If we only want to support this in the user frontend then i could hack it into the Markup rule itself and not touch the CommitQuery. Even uglier but more limited in scope...
Generally this approach will need a lot of "check this first and then try the other" in a few places.
I could move the Repository queries into a specialised PhabricatorRepositoryQuery method (withCallsignOrID) but i'm not sure about that.
Test Plan:
- phid.lookup works with R1 and rTEST (which is the same repo)
- R1 and rTEST euqally work in remarkup (tested in comments).
- Reviewed the following syntax also all works:
rTEST
rTESTd773137a7cb9
rTEST:d773137a7cb9
R1
R1:d773137a7cb9
d773137a7cb9
{rTEST}
{rTESTd773137a7cb9}
{rTEST:d773137a7cb9}
{R1}
{R1:d773137a7cb9}
{d773137a7cb9}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D11050
Summary: Modernize Differential edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: From previous experience, these changes are fairly trivial and safe. I poked around a little to make sure things looked reasonably okay.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, Krenair, epriestley
Differential Revision: https://secure.phabricator.com/D11074
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:
Ref T2783. When creating a new repository, test for cluster services. If cluster services are available, allocate on a random open service.
Show the service that repositories are allocated on.
Test Plan: Created a new repository, saw it allocate onto an available cluster service.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11003
Summary: Ref T5245. This is some of the associated cleanup there.
Test Plan:
foreach ManiphestTaskQuery site, I made the change (or not) and tested as follows:
=== Call sites where added needProjectPHIDs ===
- PhabricatorHomeMainController - loaded the home page
- ManiphestBatchEditController - batch edited some tasks (added a project)
- ManiphestConduitAPIMethod - tested implicitly when tested ManiphestUpdateConduitAPIMethod
- ManiphestInfoConduitAPIMethod - used the method via conduit console with input id : 1
- ManiphestQueryConduitAPIMethod - used the method via conduit console with input ids : [1, 2]
- ManiphestUpdateConduitAPIMethod - used the method via conduit with input id : 1 and comment : “asdasds"
- ManiphestReportController - viewed “By User” and “By Project”
- ManiphestSubpriorityController - changed the priority of a task via a drag on manphest home
- ManiphestTaskMailReceiver - updated Task 1 via bin/mail receive-test with a comment that is the README
- ManiphestTaskSearchEngine - loaded Manifest home page
- ManiphestTaskEditController - edited a task
- ManiphestTransactionEditor - closed a blocking task
- ManiphestTransactionSaveController - commented on a task
- PhabricatorProjectProfileController - viewed project with id of 1 that has a few tasks in it
- PhabricatorSearchAttachController - merged tasks together
- DifferentialTransactionEditor - submit a diff that references a task; commit the diff (thus closing the diff) and the task gets updated
- PhabricatorRepositoryCommitMessageParserWorker - submit a diff that references a task; commit the diff (thus closing the diff) and the task gets updated
=== Calls sites where *did not* add needProjectPHIDs (they do not appear in this revision) ===
- PhabricatorManiphestApplication - loaded the home page
- ManiphestGetTaskTransactionsConduitAPIMethod - used the method via conduit console with input ids : [1, 2] ManiphestTaskDetailController - viewed a task with and without associated projects; finished workflow creating a task with a parent
- ManiphestTransactionPreviewController - verified transaction preview showed up properly
- PhabricatorProjectBoardViewController - viewed a board
- PhabricatorProjectMoveController - moved a task around
- ManiphestRemarkupRule - made a task reference like {T123}
- ManiphestTaskQuery - executed a custom query for all tasks with page size of 2 and paginated through some tasks
- ManiphestTaskPHIDType - nothing random seems broken? =D
=== Call sites where had to do something funky ===
- ManiphestHovercardEventListener - loaded hover cards from task mentions
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D11004
Summary:
Ref T2783. This is primarily exploratory and just figuring out what we're blocked on:
- Allow a Repository to be bound to a Service. The Service may eventually define multiple read/write nodes, etc.
- There's no UI to do this binding yet, you have to touch the database manually.
- If a repository is bound to a Service, effect Conduit calls via calls to the remote service instead of executing them in-process.
- These don't actually work yet since there's no authentication (see T5955).
Test Plan:
- Made a nice Service with a nice Binding to a nice Interface on a nice Device.
- Force-associated a repository with the service using a raw MySQL query.
- Saw Phabricator try to make a remote call to the service (on localhost) and fail because of missing auth stuff.
- Also ran `almanac.queryservices`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10982
Summary: Fixes T6693.
Test Plan:
Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!
Successfully "showed older changes" in Maniphest too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6693
Differential Revision: https://secure.phabricator.com/D10931
Summary:
Ref T4712. Specifically...
- Differential
- needed getApplicationTransactionViewObject() implemented
- Audit
- needed getApplicationTransactionViewObject() implemented
- Repository
- one object needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true)
- Ponder
- BONUS BUG FIX - leaving a comment on an answer had a bad redirect URI
- both PonderQuestion and PonderAnswer needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on both "history" controllers
- left a "TODO" on buildAnswers on the question view controller, which is non-standard and should be re-written eventually
- Phortune
- BONUS BUG FIX - fix new user "createNewAccount" code to not fatal
- PhortuneAccount, PhortuneMerchant, and PhortuneCart needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on Account view, merchant view, and cart view controller
- Fund
- Legalpad
- Nuance
- NuanceSource needed PhabricatorApplicationTransactionInterface implemented
- Releeph (this product is kind of a mess...)
- HACKQUEST - had to manually create an arcanist project to even be able to make a "product" and get started...!
- BONUS BUG FIX - make sure to "setName" on product edit
- ReleephProject (should be ReleepProduct...?), ReleephBranch, and ReleepRequest needed PhabricatorApplicationTransactionInterface implemented
- Harbormaster
- HarbormasterBuildable, HarbormasterBuild, HarbormasterBuildPlan, and HarbormasterBuildStep all needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) all over the place
Test Plan: foreach application, viewed the timeline(s) and made sure they still rendered
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10925
Summary: Fixes T3189. Now if you say #projects in a commit message they will associate nicely with the commit. Also we record transactions about all this project editing fun.
Test Plan: tested migration by associating some projects with commits and verifying they still showed up post migration. tested adding / removing projects by doing so from the UI, noting transactions written nicely as well
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Projects: #projects
Maniphest Tasks: T3189
Differential Revision: https://secure.phabricator.com/D10877
Summary: Fixes T6152, T6237. This introduces a viewPolicy column to the DifferentialDiff, and re-jiggers the DifferentialDiff policy implementation such that things behave as before once associated with a revision, else use the DifferentialDiff policy column value.
Test Plan: made a diff with a non-standard view policy and noted that policy was still selected in the revision step. arc lint.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237, T6152
Differential Revision: https://secure.phabricator.com/D10875
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 T1768. This is mostly a data cleanliness issue as duplicate rows don't really do anything, but let's clear it up now.
Test Plan: made some duplicate rows by adding the same auditor multiple times. ran ./bin/storage upgrade and it worked perfectly!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T1768
Differential Revision: https://secure.phabricator.com/D10849
Summary: Fixes T6343. Grepped for all callsites and added addLinkSection where needed.
Test Plan: Tested Differential, Maniphest, Conpherence, Ponder and Macro. Inspect HTML mail for anchor tags. Inspect text mails for non-disruption.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: talshiri, Korvin, epriestley
Maniphest Tasks: T6343
Differential Revision: https://secure.phabricator.com/D10762
Summary: Fixes T6419. Also, there was a question on T6419 about whether this was in a try catch block and it is... Its not clear to me what happens in the "timeout" case though?
Test Plan: looks nice
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6419
Differential Revision: https://secure.phabricator.com/D10755
Summary: ...also truncate authorName to 255 so that we don't get database errors. Ref T6350.
Test Plan: see T6350 - mostly doing it live - but I did sanity check and commit something and it worked!
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6350
Differential Revision: https://secure.phabricator.com/D10734
Summary: Fixes T6336. Turns out that the function to update the import status updates that database and doesn't update the object. If the object doesn't get the pertinent update AND there's a herald rule that runs, then the object is later re-saved without ever getting the update flag.
Test Plan: logic in the ole sandbox and going to push it to prod and run re-parse on impacted commits
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley, chad
Maniphest Tasks: T6336
Differential Revision: https://secure.phabricator.com/D10723
Summary:
Fixes T4896, T6293.
Do most of the work in the editor, but pull the raw patch in the daemon and set that on the editor. This is somewhat of a pre-optimization but it was easy enough to do and makes sense to me.
Test Plan:
made a commit and saw it get parsed.
made a commit with "Auditors: foo" field and saw audit made for foo
turned on inline patch and attach patch and saw the patches
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6293, T4896
Differential Revision: https://secure.phabricator.com/D10705
Summary:
I am not sure how valuable this is *as is* - I think it needs different explanations for what happened in mercurial or subversion? I do not know what those explanations are.
Made an error in D10485 - the $hashes that were saved is an array of objects, so it ends up turning into garbage via the wonders of serialization and de-serialization. Fix that by explicitly saving the tree hash.
I would like to make this work for the other VCS types we support, add the "undo / nope" button and call it fixed.
Ref T3686.
Test Plan: clicked "explan why" and saw why
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5693, T3686
Differential Revision: https://secure.phabricator.com/D10489
Summary:
Ref T1191. Now that the whole database is covered, we don't need to do as much work to build expected schemata. Doing them database-by-database was helpful in converting, but is just reudndant work now.
Instead of requiring every application to build its Lisk objects, just build all Lisk objects.
I removed `harbormaster.lisk_counter` because it is unused.
It would be nice to autogenerate edge schemata, too, but that's a little trickier.
Test Plan: Database setup issues are all green.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10620
Summary:
Ref T1191. When changing the column type of an AUTO_INCREMENT column, we currently may lose the autoincrement attribute.
Instead, support it. This is a bit messy because AUTO_INCREMENT columns interact with PRIMARY KEY columns (tables may only have one AUTO_INCREMENT column, and it must be a primary key). We need to migrate in more phases to avoid this issue.
Introduce new `auto` and `auto64` types to represent autoincrement IDs.
Test Plan:
- Saw autoincrement show up correctly in web UI.
- Fixed an autoincrement issue on the XHProf storage table with `bin/storage adjust` safely.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10607
Summary:
Ref T1191. For most text columns, we either don't care if "a" and "A" are the same, or we expect them to be different (for example: keys, domains, secrets, etc). Default text columns to the `_bin` collation so they are compared by strict character value. This is safer in cases where we aren't sure.
For some text columns, we allow the user to sort by the column in the UI (like Maniphest task titles) or we do care that "A" and "a" are the same (for example: project names). Introduce a new class of virtual data types, the "sort..." types, to cover these columns. These are like the "text..." types but use sorting collations which treat "A" and "a" the same.
Test Plan:
- Made an effort to identify all columns where the UI relies on database collation.
- Ran `bin/storage adjust` and cleared all warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: beng, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10602
Summary:
Ref T1191. Ref T6203. While generating expected schemata, I ran into these columns which seem to have sketchy nullability.
- Mark most of them for later resolution (T6203). They work fine today and don't need to block T1191. Changing them can break the application, so we can't autofix them.
- Forgive a couple of them that are sort-of reasonable or going to get wiped out.
Test Plan: Saw 94 remaining warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T1191, T6203
Differential Revision: https://secure.phabricator.com/D10593
Summary: Fixes T5536. Some bonus pht in there.
Test Plan: made a diff hovered over the stars and saw my new text.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5536
Differential Revision: https://secure.phabricator.com/D10487
Summary:
Implements a new transaction - still TYPE_ACTION - but using a new DifferentialAction::ACTION_COMMIT_CLOSE. Augment rendering as necessary to display this new transaction. Saves enough information so T3686 is possible but stops short of implementing a popup to display this information. Fixes T5875. Ref T3686.
One small display oddity - this new transaction now renders at the top of the transaction group whereas when it was a comment it was on the bottom. I think this is basically okay but if not how fix? (Playing with the "strength" of these actions will mess up the email too?)
Test Plan: made a diff X that fixed task Y. committed. checked diff X, task Y, and the commit pages for proper transactions and all looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3686, T5875
Differential Revision: https://secure.phabricator.com/D10485
Summary: Fixes T4036. Now if you say something on diff X like "This reminds me of Tx and Dy and commitHashFoo and Px." each of those objects gets a little visible transaction that the mention occurred. No feed, email, or notifications.
Test Plan: made a comment like above and verified transactions. also submitted a diff that "Fixes Tx" and Tx did not get the transaction as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, epriestley, Korvin
Maniphest Tasks: T4036
Differential Revision: https://secure.phabricator.com/D10451
Summary:
Ref T2783. Fixes T6039.
- Provide `authorPHID` and `committerPHID` to resolve T6039.
- In message parser, store author/email strings.
- In cached results, emit author/email strings.
Test Plan: Called method with and without bypassCache. Used `reparse.php` to repopulate data on an old commit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783, T6039
Differential Revision: https://secure.phabricator.com/D10424
Summary: make it use the value of the revision before any post-commit magic has occurred. Fixes T4754
Test Plan: made a herald rule that said "if revision exists, and revision accept does not exists, block push". tried to push a commit that had a revision that wasn't accepted and I was blocked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: mbishopim3, epriestley, Korvin
Maniphest Tasks: T4754, T4574
Differential Revision: https://secure.phabricator.com/D10393
Summary: Ref T3307. Only one I thought was tricky was Excel; I went with bytes there like it was email.
Test Plan: played around on a few endpoints but mostly thought carefully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3307
Differential Revision: https://secure.phabricator.com/D10392
Summary: Fixes T2564. See screenshot.
Test Plan:
{F194796}
- Made a bunch of valid and invalid adjustments here and verified that the branches table showed autoclose state and branches consistent with the settings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2564
Differential Revision: https://secure.phabricator.com/D10349
Summary:
Fixes T4767. I believe 80% of this was actually caused by the author issue fixed in T5771, but this should help make the other 20% debuggable.
- Record why we didn't autoclose a commit when we process it.
- Show branch autoclose status in the main branch table.
- Show commit autoclose status on the edit screen.
- Add documentation about how to find these statuses and what they mean.
Test Plan:
- Read documentation.
- Viewed branches and hovered over the various states.
- Viewed commits in various states and checked the "Autoclose?" field.
- Pushed some commits and saw autoclose activate.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4767
Differential Revision: https://secure.phabricator.com/D10348
Summary:
Fixes T5926. Fixes T5830. Ref T4767. Users currently sometimes have a hard time understanding repository update frequencies. This is compounded by aggressive backoff and incorrect backoff while importing repositories.
- Don't back off while importing repositories. This prevents us from hanging at 99.99% for inactive repositories while waiting for the next update.
- Back off less aggressively in general, and even more gradually during the first 3 days. This should make behavior around weekends better.
- Show update frequency in the UI.
- Provide an explicit "update now" button to call `diffusion.looksoon` in a more user-friendly way.
- Document how backoff policies work and how to adjust behavior.
Test Plan:
- Ran `bin/phd debug pulllocal` and verified backoff worked correctly from debugging output.
- Clicked "Update Now" to get a hint, reloaded page to see it update.
- Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4767, T5830, T5926
Differential Revision: https://secure.phabricator.com/D10323
Summary:
Ref T5861. Currently, mail tags are hard-coded; move them into applications. Each Editor defines its own tags.
This has zero impact on the UI or behavior.
Test Plan:
- Checked/unchecked some options, saved form.
- Swapped back to `master` and saw exactly the same values.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10238
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:
Fixes T5855. Adds a `--graceful N` flag to `phd stop` and `phd restart`.
`phd` will send SIGINT, wait `N` seconds, SIGTERM, wait 15 seconds, and SIGKILL. By default, `N` is 15.
Test Plan:
- Ran `bin/phd debug ...` and used `^C` to interrupt daemons. Saw graceful shutdown behavior, and abrupt termination on multiple `^C`.
- Ran `bin/phd start`, `bin/phd stop` and `bin/phd restart` with `--graceful` set to various things, notably `0`. Saw graceful shutdowns on the CLI and in the web UI. With `0`, abrupt shutdowns.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T5855
Differential Revision: https://secure.phabricator.com/D10228
Summary:
- Fixes T5851. Currently, if a commit has `Fixes T123`, we generate an email with just that before generating the commit email. Don't send/publish transactions about a commit before it imports (this is a tiny bit hacky, but well-contained and I don't think it causes any problems).
- Fixes T4864. Currently, we try to parse Differential information even if Differential is not installed. Instead, do this only if Differential is installed.
- Fixes T5771. Currently, if we can't figure out who the committer/author of a commit is, we don't publish a `Fixes T123` transaction. Instead, fall back to acting as "Diffusion" if we can't find a better actor. Most of this diff expands the role of application actors. The existing application actors (Herald and Harbormaster) seem to be working well.
Test Plan:
- Pushed a commit with `Fixes T123` and verified it did not generate email directly. (The task half of the transaction still does, correctly.)
- Uninstalled Differential and pushed a commit, got a clean import instead of an exception.
- Commented out author/committer PHIDs and pushed stuff, saw a "Diffusion" actor.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5771, T4864, T5851
Differential Revision: https://secure.phabricator.com/D10221
Summary:
Ref T4896. Now that we have a transaction editor, we can delete a giant block of hacks.
I believe this also resolves the commit/task attachment issues @joshuaspence and @mbishopim3 mentioned.
Test Plan: Attached and detached commits and tasks.
Reviewers: btrahan, joshuaspence, mbishopim3
Reviewed By: mbishopim3
Subscribers: mbishopim3, epriestley, joshuaspence
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10138
Summary:
Ref T4896. Currently, subscriptions to commits are stored as auditors with a special "CC" type.
Instead, use normal subscriptions storage, reads and writes.
Test Plan:
- Ran migration and verified data still looked good.
- Viewed commits in UI and saw "subscribers".
- Saw "Automatically Subscribed", clicked Subscribe/Unsubscribe on a non-authored commit, saw subscriptions update.
- Pushed a commit through Herald rules and saw them trigger subscriptions and auditors.
- Used "Add CCs".
- Added CCs with mentions.
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10103
Summary:
Ref T4896. Depends on D10056. Moves search indexing to standard infrastructure.
Also, fixes a bug where inline comments would not be indexed.
Test Plan: Used `bin/search index ... --trace` to view index construction of a commit, saw all the comments and inlines get indexed.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10057
Summary: Ref T4896. Buries all direct access to the table so we can limit the surface area affected by the migration.
Test Plan:
- Grepped for `PhabricatorAuditComment`.
- Grepped for `audit_comment`.
- Viewed a bunch of comments.
- Added a comment.
- Reindexed a commit.
- Searched for unique term in new comment.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10019
Summary: Ref T4896. Move all direct accesses to the inline comment table behind a small amount of API to make it easier to migrate the table.
Test Plan:
- Grepped for `PhabricatorAuditInlineComment`.
- Grepped for `audit_inlinecomment`.
- Created a draft comment.
- Previewed a draft comment.
- Reloaded page, still saw draft.
- Viewed standalone, still saw draft.
- Made comment, inline published.
- Added a draft, saw both.
- Edited inline comment.
- Reindexed commit.
- Searched for unique word in published comment, found commit.
- Searched for unique word in draft comment, no results.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10016
Summary: Ref T5655. Rename `PhabricatorPHIDType` subclasses for clarity (see discussion in D9839). I'm not too keen on some of the resulting class names, so feel free to suggest alternatives.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9986
Summary: Provide an implementation for the `getName` method rather than automagically determining the application name.
Test Plan: Saw reasonable application names in the launcher.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10027
Summary: Ref T5655. Some discussion in D9839. Generally speaking, `Phabricator{$name}Application` is clearer than `PhabricatorApplication{$name}`.
Test Plan:
# Pinned and uninstalled some applications.
# Applied patch and performed migrations.
# Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
# Performed a sanity check on the database contents.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9982
Summary: Instead of implementing the `getTypeConstant` method in all subclasses of `PhabricatorPHIDType`, provide a `final` implementation in the base class which uses reflection. See D9837 for a similar implementation.
Test Plan: Ran `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9985
Summary: Ref T5655. The `PhabricatorDestructibleInterface` interface is misspelled as `PhabricatorDestructableInterface`. Fix the spelling mistake.
Test Plan: `grep`. Seeing as this interface is fairly recent, I don't expect that this would cause any widespread breakages.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9988
Summary: Ref T5245. Updates the project/object edge to use a modern class definition. Moves further toward real edges.
Test Plan: Added projects to some objects, viewed transactions in transaction record.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9849
Summary: Ref T5245. With work elsewhere (notably, D9839) we can remove this TODO and use real transactions.
Test Plan: Pushed a `closes Txxx` commit and got a close + transaction.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9848
Summary:
Ref T5245. These were a bad idea.
We no longer need actors for edge edits either, so remove those. Generally, edges have fit into the policy model as pure/low-level infrastructure, and they do not have any policy or capability information in and of themselves.
Test Plan: `grep`
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9840
Summary: These have been moved into libphutil.
Test Plan: Browsed Phabricator, didn't see a crash.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9907
Summary:
Ref T1493.
- When viewing an invalid branch, show a "there is no such branch" message.
- When viewing an empty repository, show a "this repository is empty" message.
Test Plan:
- Viewed empty, bad branch, and nonempty in Git.
- Viewed empty, bad branch, and nonempty in Mercurial.
- Viewed empty and nonempty in Subversion.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T1493
Differential Revision: https://secure.phabricator.com/D9912
Summary: Ref T4420. Update "projects" source.
Test Plan:
- Edited projects on a Differential revision.
- Edited projects on a commit.
- Edited projects on a repository.
- Edited projects in feed search.
- Edited projects in a Herald rule field.
- Edited projects in a Herald rule action.
- Edited projects in Maniphest batch editor.
- Edited projects on Maniphest task.
- Edited projects in "Associate Projects..." action in Maniphest.
- Edited projects on Maniphest search in "all projects", "any project" and "not projects" fields.
- Edited projects on a Paste.
- Edited projects on a Pholio mock.
- Edited projects on a custom policy rule.
- Edited projects on a Ponder question.
- Edited projects on a Diffusion search query.
- Edited projects on a global search query.
- Edited projects on a slowvote.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9884
Summary: Ref T4420. Make this modern.
Test Plan:
- Used typeahead in remarkup comment area to select macro "derpdog".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9875
Summary:
Ref T4420.
- Allow tokenizers to accept either a `Datasource` object (new style) or a URI (old style).
- Read URI and placeholder text from object, if available.
- Swap the "repositories" datasource (which seemed like the simplest one) over to the new stuff.
- Tweak/update the repo tokens a little bit.
Test Plan:
- Used tokenizer in Herald, Differential (search), Differential (edit), Push Logs.
- Grepped for other callsites.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9874
Summary: Since there's no way to set it, it defaults to an empty value. Make the conduit call set up sane default.
Test Plan: Call method, repo get's built with expected localpath.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9842
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
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
Summary:
Fixes T4677. Implements a "send an email" pre-receive action, which sends push summaries.
For use cases where features are often pushed as a large number of commits (e.g., checkpoint commits are retained), using commit emails means users get a ton of email. Instead, this allows you to get an email about a push, which summarizes what changed.
Overall, this is basically the same as commit email, but more suitable for some workflows.
Test Plan:
Wrote some rules, then made a bunch of pushes. Got email like this:
{F134929}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4677
Differential Revision: https://secure.phabricator.com/D8618
Summary:
Ref T4677. This shows a more detailed view of an entire "git push", "hg push", or "svn commit".
This is mostly to give push summary emails a reasonable, stable URI to link to for T4677.
Test Plan:
- Pushed into SVN, Git and Mercurial.
- Viewed partial and imported event records.
{F134864}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4677
Differential Revision: https://secure.phabricator.com/D8616
Summary:
Ref T4677. Currently, we record individual actions in a push as PhabricatorRepositoryPushLogs, but tie them together only loosely with a `transactionKey`.
Provide a real PushEvent object, and move some of the denormalized fields to it. This primarily just gives us more robust infrastructure for building, e.g., email about pushes, for T4677, since we can act on real PHIDs rather than passing awkward identifiers around.
Test Plan:
- Performed migration.
- Looked at database for consistency.
- Browsed/queried push logs.
- Pushed a bunch of stuff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4677
Differential Revision: https://secure.phabricator.com/D8615
Summary: Ref T4590. Ref T1049. This is primarily intended to support HTTP auth in Harbormaster.
Test Plan: Added a field, edited it, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4590, T1049
Differential Revision: https://secure.phabricator.com/D8607
Summary: Fixes T4600. If there's also a revision, the variable "$message" gets overwritten. groan~
Test Plan: Pushed a commit with "Fixes T123" and a revision, saw it parse on the first try.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chrisbolt, aran, epriestley
Maniphest Tasks: T4600
Differential Revision: https://secure.phabricator.com/D8519
Summary:
Via HackerOne. In regular expressions, "$" matches "end of input, or before terminating newline". This means that the expression `/^A$/` matches two strings: `"A"`, and `"A\n"`.
When we care about this, use `\z` instead, which matches "end of input" only.
This allowed registration of `"username\n"` and similar.
Test Plan:
- Grepped codebase for all calls to `preg_match()` / `preg_match_all()`.
- Fixed the ones where this seemed like it could have an impact.
- Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8516
Summary:
Currently, disabling Herald only disables feed, notifications and email. Historically, audits didn't really create external effects so it made sense for Herald to only partially disable itself.
With the advent of Harbormaster/Build Plans, it makes more sense for Herald to just stop doing anything. When this option is disabled, stop all audit/build/publish/feed/email actions for the repository.
Test Plan: Ran `scripts/repository/reparse.php --herald`, etc.
Reviewers: dctrwatson, btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8509
Summary:
- Fixes T4588.
- See D8501.
- Adds a "Tags" field for Herald commit emails.
- Fixes a bug in `tagsquery` when filtering by commit name.
- Make `tagsquery` just return nothing instead of fataling against Mercurial/Subversion.
Test Plan: Used `bin/repository/reparse.php --herald` to exercise this code.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4588
Differential Revision: https://secure.phabricator.com/D8502
Summary:
Ref T4588. Request from @zeeg. Adds a "BRANCHES" field to commit emails, so the branches the commit appears on are shown.
I've implemented this with CustomField, but in a very light way.
Test Plan: Used `scripts/repository/reparse.php --herald` to generate mail, got a BRANCHES section where applicable.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley, zeeg
Maniphest Tasks: T4588
Differential Revision: https://secure.phabricator.com/D8501
Summary:
Ref T2222.
- Removes `DifferentialTasksAttacher`, which has had no callsites for a very long time.
- Moves `differential.getrevisioncomments` off `DifferentialCommentQuery`.
- Moves Releeph churn field off `DifferentialCommentQuery`.
- Removes dead code in `DifferentialRevisionViewController`.
- Removes `DifferentialException` (no references).
- Removes `DifferentialRevision->loadComments()` (no callsites).
- Removes `DifferentialRevision->loadReviewedBy()` (all callsites updated).
- Removes `DifferentialCommentQuery` (all callsites updated).
Test Plan: Mostly a lot of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8476
Summary: Ref T2222. Straightforward, just breaks a needless dependency.
Test Plan: Pushed and parsed a commit with "Auditors" in it.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8473
Summary: Ref T2222. There's some magic here, just port it forward in a mostly-reasonable way. This could use some refinement eventually.
Test Plan: Pushed commits with "Fixes" and "Ref" language, used `reparse.php` to trigger the new code. Saw expected updates in Maniphest.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8471
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.
This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.
Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8460
Summary: Ref T2222. When we discover a commit associated with a revision, close it using modern transactions.
Test Plan: {F123848}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8441
Summary:
For imported SVN repositories with an "Import Only" path, we produce a `/path/to/root/` URI, but should produce `/path/to/root/then/to/import/only/`.
As it is, the URI instructs the user to check out the whole repository.
Also, don't show the "Clone As" fragment in the URI for remote repositories, and prevent it from being edited for nonhosted repositories. This is generally more consistent with user expectation.
Test Plan:
- Created a remote SVN repository with "Import Only", saw path include it.
- Verified no "Clone As" options, no "Clone As" in URI.
- Switched it to hosted, saw "Clone As" options appear and work properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, staticshock
Differential Revision: https://secure.phabricator.com/D8375
Summary:
Fixes T4414. Currently, when we discover a new repository, we do something like this:
foreach (branch) {
foreach (commit on this branch) {
do_something();
}
}
In cases where there are a lot of branches which mostly just branch `master`, this leads to us doing roughly `O(branches * commits)` work.
We have a `commitCache` to prevent this, but it has two problems:
- It only fills out of the DB, and we do this whole thing before writing to the DB, which is the entire point.
- It has a fixed size (2048) and on initial discovery we're usually dealing with way more commits than that.
Instead, also stop doing work if we hit a commit which is known already.
Test Plan:
- Added `print` on the number of discovered refs and number of unique refs.
- Ran `bin/repository discover --repair X` on a repo with several branches.
- Before the patch, got 397 refs and 135 unique refs.
- After the patch, got 135 refs and 135 unique refs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4414
Differential Revision: https://secure.phabricator.com/D8374
Summary:
See IRC. I'm having trouble figuring out what's going on with b4taylor's report, but fix two possible issues:
# The commit query is missing a `repositoryID`, which could cause issues if you import two copies of the same repository.
# I think we may try to close commits on untracked branches right now, as long as they aren't excluded by other autoclose rules.
Test Plan: Ran `bin/repository refs` on a few repos.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, brennantaylor
Differential Revision: https://secure.phabricator.com/D8373
Summary: Ref T2222. This is obsolete and no longer used. We could deduce it from transactions or commits in modern Phabricator if we wanted it. We may implement a more general mechanism for T4434.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8330
Summary:
@mbishopim3 reported an issue in IRC:
> mbishopim3: epriestley: "Error updating working copy: Commit "" has not been discovered yet! Run discovery before updating refs." any ideas?
I can't reproduce it and it went away for him, but one theory is that we're getting here and git/hg are spitting out nothing, which we incorrectly parse as `array("")` when we intend `array()`.
Test Plan:
Pushed some new commits, ran `bin/repositoy refs X`, got expected results.
I can't actually reproduce the bug, but this might fix it and appears to make the code more correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: mbishopim3, aran
Differential Revision: https://secure.phabricator.com/D8326
Summary:
Ref T1191. I believe we only have three meaningful binary fields across all applications:
- The general cache may contain gzipped content.
- The file storage blob may contain arbitrary binary content.
- The Passphrase secret can store arbitrary binary data (although it currently never does).
This adds Lisk config for binary fields, and uses `%B` where necessary.
Test Plan:
- Added and executed unit tests.
- Forced file uploads to use MySQL, uploaded binaries.
- Disabled the CONFIG_BINARY on the file storage blob and tried again, got an appropraite failure.
- Tried to register with an account containing a G-Clef, and was stopped before the insert.
Reviewers: btrahan, arice
Reviewed By: arice
CC: arice, chad, aran
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D8316
Summary:
Ref T3886. Ref T418. For fields like "Summary" and "Test Plan" where changes can't be summarized in one line, allow CustomField to provide a "(Show Details)" link and render a diff.
Also consolidate some of the existing copy/paste, and simplify this featuer slightly now that we've move to dialogs.
Test Plan:
{F115918}
- Viewed "description"-style field changes in phlux, pholio, legalpad, maniphest, differential, ponder (questions), ponder (answers), and repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T418
Differential Revision: https://secure.phabricator.com/D8284
Summary:
Error:
Fatal error: Call to a member function getRefType() on a non-object in /opt/phabricator/phabricator/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php on line 197
Test Plan: No more error in daemon.log afterwards
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8278
Summary: Fixes T4443. Plug VCS passwords into the shared key stretching. They don't use any real stretching now (I anticipated doing something like T4443 eventually) so we can just migrate them into stretching all at once.
Test Plan:
- Viewed VCS settings.
- Used VCS password after migration.
- Set VCS password.
- Upgraded VCS password by using it.
- Used VCS password some more.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8272
Summary: See <https://github.com/facebook/phabricator/pull/510>.
After the repository rework, the Create New Repository link in Repositories goes straight to creating a phabricator hosted repo(diffusion/create), rather than the chooser create/import (diffusion/new)
I updated it to point to diffusion/new the same as the New Repository link in Diffusion.
As a side note, the Repositories page could probably use the Crumbs treatment.
Reviewed by: epriestley
Summary: See <https://github.com/facebook/phabricator/issues/501>. I think the issue here is that we created a foreign stub for commit `X-1`, probably because commit `X` was created by running `svn cp y x`.
Test Plan: I'll write a separate test for this before I land it. Huge pain to test.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8133
Summary:
Ref T3102
In diffusion, add "In Any Project" to search options.
Test Plan: use it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3102
Differential Revision: https://secure.phabricator.com/D8113
Summary:
Ref T4175. This allows these URIs to all be valid for Git and Mercurial:
/diffusion/X/
/diffusion/X/anything.git
/diffusion/X/anything/
This mostly already works, it just needed a few tweaks.
Test Plan: Cloned git and hg working copies using HTTP and SSH.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4175
Differential Revision: https://secure.phabricator.com/D8098
Summary:
Ref T4175.
- Add a configurable name for the clone-as directory, so you can have "Bits & Pieces" clone as "bits~n~pieces/" or simliar.
- By default, use "reasonable" heruistics to choose such a name.
- Generate a copy/pasteable clone commmand with this directory name.
Test Plan: Looked at some repositories.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4175
Differential Revision: https://secure.phabricator.com/D8097
Summary:
Hosted repositories have muddied this distinction somewhat. In some cases, we only want to use the real remote URI, and the call is only relevant for imported repositories.
In other cases, we want the URI we'd plug into `git clone`.
Move this logic into `PhabricatorRepository` and make the distinction more clear.
Test Plan: Viewed SVN, Git, and Mercurial hosted and remote repositories, all the URIs looked reasonable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D8096
Summary:
Fixes T4357. The Mercurial commit hooks enforce dangerous change protection, but the UI doesn't actually let you configure it.
This was just an oversight (I never went back and enabled it) -- allow it to be configured in the UI.
Test Plan: Clicked "Edit Repository" on a hosted Mercurial repository, saw option to enable dangerous changes.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4357
Differential Revision: https://secure.phabricator.com/D8108
Summary:
Ref T4338. Mercurial exits with exit code 1 and "no changes found" in stdout
when there's no changes. I've split up the `pushRepositoryToMirror` to make
the code a tad more readable.
It isn't perfect, but it works for me.
Test Plan:
pushed some changes to my hosted repo. Saw them appearing in the
mirrored repo
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4338
Differential Revision: https://secure.phabricator.com/D8107
Summary:
I derped this up, and it passed my tests because the two URIs are the same for hosted repositories.
Match repositories against their remote URI (like `https://github.com/user/repo.git`), not their display URI (like `/diffusion/X/`).
Test Plan: i r dums
Reviewers: talshiri, btrahan
Reviewed By: talshiri
CC: aran
Differential Revision: https://secure.phabricator.com/D8082
Summary:
Moves away from ArcanistProjects:
- Adds storage for diffs to be directly associated with a repository (instead of indirectly, through arcanist projects). Not really populated yet.
- Drops `parentRevisionID`, which is obsoleted by the "Depends On" edge. This is not exposed in the UI anywhere and doesn't do anything. Resolves TODO.
Test Plan: Ran storage upgrades, browsed around, lots of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8072
Summary: This helps us move away from arcanist projects in normal use, by allowing us to identify repositories based on the remote URI.
Test Plan: Used `repository.query` to query some repos by remote URI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8069
Summary: Expose more options on `repository.query`. My broad goal here is to move forward on suppressing Arcanist Projects.
Test Plan: Used Conduit console to execute queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8068
Summary: Ref T4338. Currently, if you have several mirrors and the first one fails, we won't try the other mirrors (since we'll throw and that will take us out of the mirroring process). Instead, try each mirror even if one fails, and then throw an AggregateException with all the failures.
Test Plan:
- Ran `bin/repository mirror` normally.
- Faked an exception, ran again, got the AggregateException I expected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4338
Differential Revision: https://secure.phabricator.com/D8067
Summary:
Ref T4338. Currently, there's no diagnostic command to execute mirroring (so I can't give users an easy command to run), and it's roughly the last piece of real logic left in the PullLocal daemon.
Separate mirroring out, and provide `bin/repository mirror`.
Test Plan:
- Ran `bin/repository mirror` to mirror a repository.
- Ran PullLocalDaemon and verified it also continued mirroring normally.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4338
Differential Revision: https://secure.phabricator.com/D8066
Summary:
I'm not sure if this is desired functionality, but we happen to need
mirroring of our repository which is not hosted by phabricator, and as far as
I can tell the mirroring code does not depend on the hosting code.
Test Plan:
Setup mirror with SSH credentials to github, pushed changes to
elsewhere hosted repository, commits got mirrored to github.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4338
Differential Revision: https://secure.phabricator.com/D7637
Summary: See IRC. Currently, if a fetch fails, we may repair the remote URI incorrectly, replacing it with one without any credentials. Instead, retain credentials.
Test Plan: Faked it locally, got le-boom to verify on IRC.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8055
Summary: After adding the CLOSEABLE flag, `bin/repository importing` reports CLOSEABLE commits with no information. Just don't report these, for consistency with the old behavior. Adding flags to show them might be nice at some point if we run into issues where that output would be useful.
Test Plan: Ran `bin/repositroy importing` on a repository with CLOSEABLE commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8024
Summary: Fixes T3238. Ref T4327. Although the instructions are fairly clear on this, it's easy to miss them. Make sure the root the user enters matches the real root.
Test Plan: Added unit tests. Used `bin/repository discover` to hit the check explicitly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3238, T4327
Differential Revision: https://secure.phabricator.com/D8020
Summary: Ref T4327. This adds a subpath/partial repository where we import only a subdirectory, and adds tests for it.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8019
Summary:
Ref T4327. Adds additional tests:
- Property changes on the root directory (which has special handling).
- Copying a file from a previous revision (this is `svn cp svn://server.com/root/some_file@123 some_file`).
- "Multicopying" a file: this is where a file is copied to several places and moved in the same commit.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8018
Summary:
Ref T4327. Adds additional SVN tests to cover:
- replacing a file with a file;
- replacing a file with a directory;
- removing a directory with files in it;
- adding a directory with files;
- copying a directory and adding and deleting files inside it.
Test Plan: Ran unit tests. One of these has a slightly irregular parse, see inline.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8017
Summary:
Ref T4327. Adds the same basic battery of tests to the SVN parser as the other parsers have.
cowsay {{{ THIS IS AN AMAZING DIFF }}}
Test Plan:
Ran unit tests against SVN change parsing.
bwahaha
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8016
Summary: Ref T4327. This adds a set of test cases for the Mercurial parser. These tests are nearly identical to the git cases, except that the Mercurial parser can't figure out symlinks right now.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8014
Summary: Ref T4327. Adds some basic tests to the Git parser for a set of common operations (add, change, move, copy, directory, symlink, propchange).
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8012
Summary:
Ref T4327. There are a bunch of other probably-related tasks too, some linked there.
We have some rare/unusual bugs in the change parsers, mostly in Subversion, but it's terrifying to touch them because they're complicated and fragile and have no test coverage.
To fix this stuff, I want to make them more testable. In particular, they basically end with this big INSERT right now. Instead, I'm going to make them return objects representing the data to be inserted, then have the common infrastructure do the insert. This gives us two benefits:
- Reduced code duplication on the insert;
- we can stop before the insert and have unit tests examine the objects.
This swaps the Git parser over, but doesn't swap the hg/svn parsers yet. I'll do those separately, the SVN one looks a bit tricky.
Test Plan:
- Used `scripts/repository/reparse.php` to reparse a Git commit, with `--trace`. Verified it looked the same as before and the SQL that was executed seemed reasonable.
- Did the same for `hg` / `svn` commits, to make sure I didn't derp anything. These aren't expected to do anything differently.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7980
Summary:
Currently, we can sit in a sleep() for up to 15 seconds (or longer, in some cases), even if there's a parse request.
Try polling the DB every second to see if we should wake up instead. This might or might not produce significant improvements, but seems OK to try and see.
Also a small fix for logging branch names with `%` in them, etc.
Test Plan: Ran `phd debug pulllocal` and pushed commits, saw them parse within a second.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D7998
Summary:
Ref T4327. This moves the last pieces of discovery responsibility out of the PullLocal daemon and into the DiscoveryEngine.
(This makes it easier to discover repositories in unit tests in the future, since we don't need to build a PullLocal daemon and can just build a DiscoveryEngine.)
Test Plan: Ran `phd debug pulllocal`. Ran `repostory discover`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7997
Summary:
Ref T4327. Consolidates and simplifies infrastructure:
- Moves Git discovery into DiscoveryEngine.
- Collapses a bunch of the Git and Mercurial code related to stream discovery.
- Removes all cach code from PullLocal daemon (it's no longer called).
- Adds basic unit tests for Git and Mercurial discovery.
Various cleanup:
- Makes GitStream and MercurialStream extend a common base.
- Improves performance of MercurialStream in some cases, by requiring fewer commits be output and parsed.
- Makes mirroring exceptions easier to debug.
- Fixes discovery of Mercurial repositories with multiple branch heads.
- Adds some missing `pht()`.
Test Plan:
I tested this fairly throughly because I think this phase is complete:
- Made new repositories in multiple VCSes and did full imports.
- Particularly, I reimported Arcanist to make sure that TODO was resolved. I think it was related to the toposort stuff.
- Pushed commits to multiple VCSes.
- Pushed commits to a non-close branch, then pushed a merge commit. Observed commits import initially as non-close, then get flagged for close.
- Started full daemons and resolved various minor issues that showed up in the daemon log until everything ran cleanly.
- Basically spent about 30 minutes banging on this in every way I could think of to try to break it. I found and fixed some minor stuff, but it seems solid.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7987
Summary:
Ref T4327. Simplify the git discovery process so I can move it to the DiscoveryEngine, so I can make change parsing testable.
In particular:
- As an optimization, we process closeable branches ("master") first, then process uncloseable branches ("epriestley-devel"). This means that in the common case we can insert a commit as closeable immediately when it is discovered, the first pass through the pipeline will get it right, and the "ref update" step will never need to do any meaningful work.
- Commits which do not initially appear on a closeable branch, but later move to one (via merges or ref moves) will now be caught in the ref update step, have the closeable flag set, and have a message step re-queued.
- We no longer need to do a separate discovery step on closable branches.
- We no longer need to keep track of `seenOnBranches`.
Test Plan: Ran discovery on repositories after pushing commits, got reasonable results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7985
Summary:
Ref T4327. This provides a more general RefCursor-based way to identify closeable commits, and moves away from the messy `seenOnBranches` stuff. Basically:
- When a closeable ref (like the branch "master") is updated, query the VCS for all commits which are ancestors of the new ref, but not ancestors of the existing closeable heads we previously knew about. This is basically all the commits which have been merged or moved onto the closeable ref.
- Take these commits and set the "closeable" flag on the ones which don't have it yet, then queue new tasks to reprocess them.
I haven't removed the old stuff yet, but will do that shortly.
Test Plan:
- Ran `bin/repository discover` and `bin/repository refs` on a bunch of different VCSes and VCS states. The effects seemed correct (e.g., new commits were marked as closeable).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7984
Summary:
Ref T4327. I want to make change parsing testable; one thing which is blocking this is that the Git discovery process is still part of `PullLocal` daemon instead of being part of `DiscoveryEngine`. The unit test stuff which I want to use for change parsing relies on `DiscoveryEngine` to discover repositories during unit tests.
The major reason git discovery isn't part of `DiscoveryEngine` is that it relies on the messy "autoclose" logic, which we never implemented for Mercurial. Generally, I don't like how autoclose was implemented: it's complicated and gross and too hard to figure out and extend.
Instead, I want to do something more similar to what we do for pushes, which is cleaner overall. Basically this means remembering the old branch heads from the last time we parsed a repository, and figuring out what's new by comparing the old and new branch heads. This should give us several advantages:
- It should be simpler to understand than the autoclose stuff, which is pretty mind-numbing, at least for me.
- It will let us satisfy branch and tag queries cheaply (from the database) instead of having to go to the repository. We could also satisfy some ref-resolve queries from the database.
- It should be easier to extend to Mercurial.
This implements the basics -- pretty much a table to store the cursors, which we update only for Git for now.
Test Plan:
- Ran migration.
- Ran `bin/repository discover X --trace --verbose` on various repositories with branches and tags, before and after modifying pushes.
- Pushed commits to a git repo.
- Looked at database tables.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7982
Summary: Ref T4327. Moves us one small step forward toward testable change parsers by separating out this unrelated logic from the PullLocal daemon. We will also probably want to run this logic so we can do remote path lookups to limit the role of Arcanist Projects in the future, which is why I made the URI type (here, only "git") a parameter rather than calling this a `GitURINormalizer` or something.
Test Plan:
- Ran unit tests.
- Ran `repository discover` on a correctly-configured remote repository.
- Ran `repository discover` on an incorrectly-configured remote, got an error message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7981
Summary: Fixes T3857. Earlier work made this trivial and just left product questions, which I've answered by requiring the daemons to run on reasonable installs.
Test Plan: Ran `bin/search index` and `bin/search index --background`. Observed indexes write in the former case and tasks queue in the latter case. Commented with a unique string on a revision and searched for it a moment later, got exactly one result (that revision), verifying that reindexing works correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3857
Differential Revision: https://secure.phabricator.com/D7966
Test Plan: Did searches in Diffusion using all 3 Hosted values
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7927
Summary: This also cleans up some code a little bit. Most of the gymnastics are to make sure we call `needProjectPHIDs()` appropriately.
Test Plan: Created new commit and revision rules with this field. Ran commits and revisions through the test console. Field behavior seemed correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D7923
Summary:
Fixes T4189. Ref T4151. Allows repositories to have additional custom hooks for operations which can't be expressed with Herald (one such operation is lint).
This adds only local hook directories, since they're easier to use with existing hooks than global directories. I might add global directories eventually.
This doesn't support Mercurial since we have no demand for it and it's more complicated (we lose compatibility and power by just dropping a `hooks.d/` somewhere).
Test Plan:
- Pulled hosted SVN and Git repos to verify the hook directories generate correctly.
- Added a variety of hooks to the hook directories (echo + pass, fail).
- Pushed commits and verified the hooks fired (output expected info, or failed).
- Verified push log reflected the correct error code ("3", external) and detail ("nope.sh") when rejecting.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4151, T4189
Differential Revision: https://secure.phabricator.com/D7884
