Summary:
Depends on D18702. Ref T13008. This replaces the old hard-coded single rate limit with multiple flexible limits, and defines two types of limits:
- Rate: reject requests if a client has completed too many requests recently.
- Connection: reject requests if a client has too many more connections than disconnections recently.
The connection limit adds +1 to the score for each connection, then adds -1 for each disconnection. So the overall number is how many open connections they have, at least approximately.
Supporting multiple limits will let us do limiting by Hostname and by remote address (e.g., a specific IP can't exceed a low limit, and all requests to a hostname can't exceed a higher limit).
Configuring the new limits looks something like this:
```
PhabricatorStartup::addRateLimit(new PhabricatorClientRateLimit())
->setLimitKey('rate')
->setClientKey($_SERVER['REMOTE_ADDR'])
->setLimit(5);
PhabricatorStartup::addRateLimit(new PhabricatorClientConnectionLimit())
->setLimitKey('conn')
->setClientKey($_SERVER['REMOTE_ADDR'])
->setLimit(2);
```
Test Plan:
- Configured limits as above.
- Made a lot of requests, got cut off by the rate limit.
- Used `curl --limit-rate -F 'data=@the_letter_m.txt' ...` to upload files really slowly. Got cut off by the connection limit. With `enable_post_data_reading` off, this correctly killed the connections //before// the uploads finished.
- I'll send this stuff to `secure` before production to give it more of a chance.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13008
Differential Revision: https://secure.phabricator.com/D18703
Summary:
Ref T13008. Depends on D18701. The overall goal here is to make turning `enable_post_data_reading` off not break things, so we can run rate limiting checks before we read file uploads.
The biggest blocker for this is that turning it off stops `$_FILES` from coming into existence.
This //appears// to mostly work. Specifically:
- Skip the `max_post_size` check when POST is off, since it's meaningless.
- Don't read or scrub $_POST at startup when POST is off.
- When we rebuild REQUEST and POST before processing requests, do multipart parsing if we need to and rebuild FILES.
- Skip the `is_uploaded_file()` check if we built FILES ourselves.
This probably breaks a couple of small things, like maybe `__profile__` and other DarkConsole triggers over POST, and probably some other weird stuff. The parsers may also need more work than they've received so far.
I also need to verify that this actually works (i.e., lets us run code without reading the request body) but I'll include that in the change where I update the actual rate limiting.
Test Plan:
- Disabled `enable_post_data_reading`.
- Uploaded a file with a vanilla upload form (project profile image).
- Uploaded a file with drag and drop.
- Used DarkConsole.
- Submitted comments.
- Created a task.
- Browsed around.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13008
Differential Revision: https://secure.phabricator.com/D18702
Summary:
Ref T13000. Garbage collecting common ngrams is slow because MySQL isn't all that great at deleting rows quickly. See PHI96, where it looks like it's going to take a week to GC ngrams for a ~million objects at a relatively conservative 0.15 threshold.
In the event of a restore, we can reduce the impact by persisting this table so the ngrams just don't get built when the reindex happens.
Test Plan: Viewed schema in Config, saw common ngrams tables marked as "Data" instead of "Index".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18696
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary:
Ref PHI109. Ref T11786. We currently test elapsed time every 64 iterations (since iterations are normally very fast), but at least one install is seeing the page timeout after 30 seconds.
One reason could be that cache fills may occur, and are likely to be much slower than normal iterations. In an extreme case, we could do 64 cache fills before checking the time. Tweak thing so that we always check the time after doing a cache fill, regardless of how many iterations have elapsed since the last attempt.
Additionally, this API method currently accepts an arbitrary number of paths, but implicitly limits each cache query to 500ms. If more than 60 paths are passed, this may exceed 30s. Only let the cache churn for a maximum of 10s across all paths.
If this is more the latter issue than the former, this might replace the GraphCache timeouts with `git` timeouts, but at least our understanding of what's going on here will improve.
Test Plan: This is difficult to test convincingly locally, since I can't reproduce the original issue. It still works after these changes, but it worked fine before these changes too.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11786
Differential Revision: https://secure.phabricator.com/D18692
Summary:
Ref T13000. Currently, queries can only be executed from the web UI, which requires logging in as a user. I really want to avoid doing that wherever we can, but being able to execute queries on an instance (and, particularly, see the ngrams and timings on the underlying lookups) would have been helpful in several cases.
Improve tooling a bit in advance of the "common ngrams" stuff going out since it seems likely that it will be useful if issues arise.
Test Plan: Ran `bin/search query --query ...`, got useful minimal output. Ran with `--trace` to get internals.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18690
Summary:
Ref T13000. After an ngram is marked as "common", we can delete it from the storage table.
Currently, the only way to get ngrams marked as "common" is to manually run `bin/search ngrams`, so this has no impact on normal installs.
Test Plan: Ran `bin/garbage collect`, saw it start chewing through my local Maniphest ngrams table and removing common ngrams.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18687
Summary:
Ref T13000. This marks each table as either "data" (normal data), "cache" (automatically rebuilt, no need to ever dump) or "index" (can be manually rebuilt).
By default, `bin/storage dump` dumps data and index tables, but not cache tables.
With `--no-indexes`, it dumps only data tables. Indexes can be rebuilt after a restore with `bin/search index --all ...`.
Test Plan:
- Ran `--no-indexes` and normal dumps with `--trace`, verified that cache and index (former case) or cache only (latter case) tables were dumped with `--no-data`.
- Verified dump has the same number of `CREATE TABLE` statements as before the changes.
- Reviewed persistence tags in the web UI (note Ferret engine tables are "Index"):
{F5210886}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18682
Summary:
Ref T13000. This allows us to be more selective about which tables we dump data for, to reduce the size of backups and exports. The immediate goal is to make large `ngrams` tables more manageable in the cluster, but this generally makes all backups and exports faster and easier.
Here, tables are dumped one at a time. A followup change will sometimes add the `--no-data` flag, to skip dumping readthrough caches and (optionally) rebuildable indexes.
Test Plan: Compared a dump from `master` and from this branch, found them to be essentially identical. The new dump has a little more header information in each section. Verified each contains the same number of `CREATE TABLE` statements.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18679
Summary: See PHI118. Enables hovercards to support peeking at tags and other details if you, e.g., create numerous identical subtasks of each task.
Test Plan: {F5210816}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18681
Summary:
If `account.editable` is set to false, we try to add a `null` button and fatal:
> Argument 1 passed to PHUIHeaderView::addActionLink() must be an instance of PHUIButtonView, null given, called in /srv/phabricator/phabricator/src/applications/settings/panel/PhabricatorSettingsPanel.php on line 290
Instead, don't try to render `null` as a button.
Test Plan:
- Configured `account.editable` false.
- Viewed email address settings.
- Before: fatal.
- After: page works, no button is provided.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18677
Summary:
Depends on D18672. Ref T13000. This does an on-demand build of the common ngrams table.
Plan here is:
- Push to `secure`.
- Build the common ngrams table here.
- See if stuff breaks?
If it looks okay on this dataset, we can build out the GC support and try it in production.
Test Plan:
- Locally, my dataset has a bunch of `bin/lipsum` tasks with similar, common words.
- Verified that ipsum terms now skip ngrams. For "lorem ipsum" search performance actually IMPROVED by skipping the ngrams table (12s to 9s).
- Queried for normal terms, got very fast results using the ngram table, as normal.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18673
Summary:
Ref T13000. This adds support for tracking "common" ngrams, which occur in too many documents to be useful as part of the ngram index.
If an ngram is listed in the "common" table, it won't be written when indexing documents, or queried for when searching for them.
In this change, nothing actually writes to the "common" table. I'll start writing to the table in a followup change.
Specifically, I plan to do this:
- A new GC process updates the "common" table periodically, by writing ngrams which appear in more than X% of documents to it, for some value of X, if there are at least a minimum number of documents (maybe like 4,000).
- A new GC process deletes ngrams that have been added to the common table from the existing indexes.
Hopefully, this will pare down the ngrams index to something reasonable over time without requiring any manual tuning.
Test Plan:
- Ran some queries and indexes.
- Manually inserted ngrams `xxx` and `yyy` into the ngrams table, searched and indexed, saw them ignored as viable ngrams for search/index.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18672
Summary:
See PHI112. The install presumably wants to generate links to Diffusion commits from an external tool, but only knows the short name of the repository.
Provide a `/source/phabricator/commit/abcdef908273` URI which redirects to the canonical URI for the commit.
Test Plan:
- Visited `/source/` URI for a commit, got a redirect.
- Visited normal URI for a commit, got a commit page.
- Visited `/branches/` and `/tags/` for a `/source/` repository, got proper pages.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18676
Summary:
See PHI94. I considered this initially but wasn't sure about it. However, PHI94 brings up the good point that we already use a similar rule in Maniphest.
For consistency, only show visible columns here too.
Test Plan: Used "Move tasks to column..." on a board with visible and hidden columns, only saw visbile columns offered in the dropdown.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18668
Summary:
See <https://discourse.phabricator-community.org/t/daemons-tasks-crashing-in-a-loop-during-reindex/506/1>. Some object types (for example, Passphrase Credentials) support indexing but not commenting.
Make `withComments(...)` work properly if the transaction type does not support comments.
Test Plan:
Indexed a credential (no comments) and a revision (comments) with `bin/search index --trace ...`.
Before, credential fataled.
After, credetial succeeds, and skips the transaction query.
Before and after, the revision queries the transaction table.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18667
Summary:
Ref T5523. See PHI50. See PHI40. This isn't perfect, but should improve things.
Add a "Move tasks to column..." action to workboards which moves all visible tasks in a column to another column, either on the same board or on a different board.
This is a two-step process so that I don't have to write Javascript, and because I'm not 100% sure this is what users actually want/need. If it sticks, the UI could be refined later.
- The first dialog asks you to choose a project, defaulting ot the current project.
- The second dialog asks you to choose a column on that project's board.
Test Plan:
- Moved tasks on the same board.
- Moved tasks to a different board.
- Tried to move tasks to the starting column, got a sensible error.
- Tried to move tasks to no project, got a sensible error.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5523
Differential Revision: https://secure.phabricator.com/D18665
Summary:
Ref PHI91. When Owners (or Herald, or manual user action) adds package reviewers to a revision, later updates to the revision make some of them less relevant or irrelevant.
Provide a hint when a package reviewer doesn't own any of the paths that a diff changes. Humans can then decide if the reviewer is obsolete/irrelevant or not.
This is a rough cut to get the feature working, design could probably use some tweaking if it sticks.
Test Plan: {F5204309}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jboning
Differential Revision: https://secure.phabricator.com/D18663
Summary:
Ref PHI101. It looks like this was maybe copy/pasted by mistake in recent design refactoring.
We need to pass the full path, not the `basename()` of the path, to the search form.
Test Plan: Searched inside `scripts/test/`, found results inside `scripts/test/`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18664
Summary:
Ref T12997. Although we can't query by transaction type (since we can't easily enumerate all possible types which may have comments -- inline types may also have comments), we //can// just check if there's a comment row or not.
This reduces the amount of garbage we need to load to rebuild indexes for unusual objects with hundreds and hundreds of mentions.
Test Plan:
- Used batch editor to mention a task 700 times.
- Indexed it before and after this change, saw index time drop from {nav 1600ms > 160ms}.
- Made some new comments on it, verified that they still indexed/queried properly.
- Browsed around, made normal transactions, made inline comments.
- Added a unique word to an inline comment, indexed revision, searched for word, found revision.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12997
Differential Revision: https://secure.phabricator.com/D18660
Summary:
Ref T12997. See that task for more details. Briefly, an unusual dataset (where commits are mentioned hundreds of times by other commits) is causing some weird memory behavior in the daemons.
Forcing PHP to GC cycles explicitly after each task completes seems to help with this, by cleaning up some of the memory between tasks. A more thorough fix might be to untangle the `$xactions` structure, but that's significantly more involved.
Test Plan:
- Did this locally in a controlled environment, saw an immediate collection of a 500MB `$xactions` cycle.
- Put a similar change in production, memory usage seemed less to improve. It's hard to tell for sure that this does anything, but it shouldn't hurt.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12997
Differential Revision: https://secure.phabricator.com/D18657
Summary:
See PHI87. Ref T12974. The `array_slice()` method of splitting the string apart can perform poorly for large input strings. I think this is mostly just the large number of calls plus building and returning an array being not entirely trivial.
We can just use `substr()` instead, as long as we're a little bit careful about keeping track of where we're slicing the string if it has UTF8 characters.
Test Plan:
- Created a task with a single, unbroken blob of base64 encoded data as the description, roughly 100KB long.
- Saw indexing performance improve from ~6s to ~1.5s after patch.
- Before: https://secure.phabricator.com/xhprof/profile/PHID-FILE-nrxs4lwdvupbve5lhl6u/
- After: https://secure.phabricator.com/xhprof/profile/PHID-FILE-6vs2akgjj5nbqt7yo7ul/
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12974
Differential Revision: https://secure.phabricator.com/D18649
Summary:
See PHI87. Ref T12974. Currently, we do a lot more work here than we need to: we call `phutil_utf8_strtolower()` on each token, but can do it once at the beginning on the whole block.
Additionally, since ngrams don't care about order, we only need to convert unique tokens into ngrams. This saves us some `phutil_utf8v()`. These calls can be slow for large inputs.
Test Plan:
- Created a ~4MB task description.
- Ran `bin/search index Txxx --profile ...` to profile indexing performance before and after the change.
- Saw total runtime drop form 38s to 9s.
- Before: <https://secure.phabricator.com/xhprof/profile/PHID-FILE-wiht5d7lkyazaywwxovw/>
- After: <https://secure.phabricator.com/xhprof/profile/PHID-FILE-efxv56q2hulr6kjrxbx6/>
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12974
Differential Revision: https://secure.phabricator.com/D18647
Summary: Hide navbar, and make curtain behave like on a phone, when printing.
Test Plan: {F5197340}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18583
Summary:
Fixes T12995. Currently, the result highlighter (which shows //where// terms matched) only works in "term" mode, not in "substring" mode.
Provide better feedback and behvaior:
- When a term is a substring term, color it a little differently and add a tooltip. (This is partly to make it easier to debug/diagnose things, probably not enormously valuable to users.)
- When a term is a substring term, highlight it anywhere in the results.
Test Plan:
Queried for latin and CJK terms.
Here is CJK being highlighted:
{F5192195}
Here is substring vs non-substring implicit behavior:
{F5192196}
Here's ONLY terms being highlighted:
{F5192198}
Here's terms and substrings, since the query now has a substring:
{F5192201}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12995
Differential Revision: https://secure.phabricator.com/D18635
Summary:
Ref T2543. This doesn't stand alone since mail still goes out normally, but gets this piece working: new revisions start as "Draft", then after updates if there are no builds they go into "Needs Review".
This should work in general because builds update revisions when they complete, to publish a "Harbormaster finished build yada yada" transaction. So either we'll un-draft immediately, or un-draft after the last build finishes.
I'll hold this until the mail and some other stuff (like UI hints) are in slightly better shape since I think it's probably too rough on its own.
Test Plan: Created revisions locally, saw them un-draft after builds.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18628
Summary:
Ref T2543. Currently, we always do some special things when a revision is created, mostly adding more stuff to the mail.
With drafts, we want to suppress initial mail and send this big, rich mail only when the revision actually moves out of "draft".
Prepare the code for this, with the actual methods hard-coded to the current behavior. This will probably take some tweaking but I think I got most of it.
Test Plan: Banged around in Differential so it sent some mail, saw normal mail without anything new.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18627
Summary:
Ref T2543. Most actions are not available for drafts.
Authors can "Request Review" (move out of draft to become a normal revision) or "Abandon".
Non-authors can't do anything (maybe we'll let them do something later -- like "Commandeer"? -- if there's a good reason).
Test Plan: Viewed a draft revision as an author and non-author, saw fewer actions available.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18626
Summary: See PHI79. When you edit another user's SSH keys (normally, for a bot account) we currently redirect you to an older URI.
Test Plan:
- Viewed a bot's profile page.
- Clicked "Edit Settings" on the Manage page.
- Went to "SSH Keys".
- Uploaded an SSH key.
- Before: redirected to a 404 after finishing the workflow.
- After: redirected to the same page after the workflow.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18633
Summary:
See PHI78. The user was getting this message and (reasonably) interpreted it to mean "reset mail can never be sent to unverified addresses".
Reword it to be more clear, albeit an entire paragraph long. I don't really have a good solution in these cases where we'd need a whole page to explain what's happening (this, plus "we can't tell you which address you should use because an attacker could get information if we did" and "this rule defuses the risk that an opportunistic attacker may try to compromise your account after you add an email you don't own by mistake"). We could write it up separately and link to it, but I feel like that stuff tends to get out of date.
Just land somewhere in the middle.
Test Plan: {F5189105}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18630
Summary:
Ref T2543. There's no way to put revisions into this state yet, but start adding support for when there is.
Adds the status constant, plus support for bucketing them.
Test Plan:
- Manually put a revision in "Draft" state by updating the database directly.
- Verified my drafts showed up in a "Drafts" section on the bucket view.
- Verified others' drafts did not appear on the action bucket view.
- Viewed revisions, queried for "Draft" revisions, etc (stuff we get for free).
{F5186781}
{F5186782}
{F5186783}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18625
Summary:
Fixes T12986. I caught this bug in the changes from D18584: when we moved a large hunk to file storage, we would decompress it but keep the "deflated" flag. This could cause confusion when loading it later. I missed this in testing since I wasn't exhaustive enough in checking hunks and didn't run into a compressed one.
Instead of compressing on `save()`, compress during the normal workflow.
We currently never advise users to run this workflow so I didn't bother trying to clean up possible existing migrations.
Test Plan:
- Ran `bin/differential migrate-hunk` on compressed hunks, moving them to and from file storage. Saw them work correctly and remain compressed.
- Created new small (uncompressed) and large (compressed) hunks, verified they work properly and get compressed (if applicable).
- Used `bin/cache purge --caches changeset` to clear changeset caches and make sure the actual table was being hit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12986
Differential Revision: https://secure.phabricator.com/D18624
Summary:
Ref T12987. I was focused on the RefCursor table and overlooked that we need some care on this key.
It's currently possible to run `bin/storage upgrade --no-adjust`, then start Phabricator, and end up with duplicate records in this table. If you try to run `bin/storage adjust` later, it will try to add the unique key but fail. This is unusual for normal installs (they usually do not use `--no-adjust`) but we do it in the cluster and I did this exact thing on `secure`.
Normally, to avoid this, when a new table with a unique key is introduced, we also add a migration to explicitly add that key.
This is mostly harmless in this case. Fix this mistake (force the table to contain only unique rows; add the key) and try using `LOCK TABLES` to make this atomic. If this doesn't cause problems we can use this in similar situations in the future.
The "alter table may unlock things" warning comes from here:
https://dev.mysql.com/doc/refman/5.7/en/lock-tables.html
It seems like it's fine to issue `UNLOCK TABLES` even if you don't have any locks, so I think this script should always do the right thing now, regardless of ALTER TABLE unlocking or not unlocking tables.
Test Plan: Ran `bin/storage upgrade -f`, saw table end up in the right state. I'll also check this on `secure`, where the starting state is a little messier.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12987
Differential Revision: https://secure.phabricator.com/D18623
Summary: Despite how I (and everyone else?) pronounce it, it is spelled with an "a". See PHI38.
Test Plan: Googled both spellings.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18622
Summary:
See PHI71. These didn't get properly updated when we wrote Subprojects and Milestones, and should use materialized members, not raw members. Swap the query so projects you are an indirect member of (e.g., milestones you are a member of the parent for, and parent projects you are a member of a subproject of) are included in the result list.
Also fix a bad typeahead datasource.
Test Plan:
- Ran a dry run with the test console, saw project PHIDs for milestones and parent projects in the raw field value.
- Tried to set "Author's projects" to a user, no longer could.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18619
Summary:
Ref T11823. I think this is the last callsite which relies on the old data format: `bin/repository parents` rebuilds a cache which we don't currently use very heavily.
Update it to work with the new data.
Test Plan: Ran `bin/repository parents <repository> --trace`, saw successful script execution and reasonable-looking output.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18615
Summary:
Ref T11823. This is the meaty part of the change, and updates `RefEngine` to use separate RefCursor (for names) and RefPosition (for actual commit positions) tables.
I'll hold this whole series until after the release cut so it has some time to bake on `secure` to look for issues. It's also not a huge problem if there are bugs here since these tables are just caches anyway, although they do feed into some other things, and obviously it's never good to have bugs.
Test Plan:
- This logic can be invoked directly with `bin/repository refs <repository> --trace --verbose`.
- Ran that on unchanged repositories, new branches, removed branches, and modified branches. Saw appropriate output and cursor positions.
- Ran on a mercurial repository to test the close/open logic, saw it correct open/closed state of incorrect positions.
- Browed around Diffusion in various repositories.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18614
Summary:
Ref T11823. This change isn't standalone, but prepares for the more involved code change by dropping obsolete columns from the RefCursor table and adding the unique key we need to prevent the ambiguous/duplicate refs issue.
This data was moved to the RefPosition table in D18612.
Test Plan: Ran storage upgrade. See next revision for more substantial testing of this change series.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18613
Summary:
Ref T11823. This populates the new RefPosition table based on the existing RefCursor table, and deletes now-duplicate rows in the RefCursor table so the next change can add a unique key.
This change is not standalone, and there need to be separate code updates. I have a rough version of that written, but this migration needs to happen first to test it.
I'll hold this whole series of changes until after the release cut and until the code is updated.
Test Plan: Ran migration, spot-checked database tables. Saw redundant rows remove and correct-looking rows populated into the new RefPosition table.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18612
Summary:
Ref T11823. Currently, we have a "RefCursor" table which stores rows like `<branch or tag name, commit it is pointing at>` with some more data.
Because Mercurial can have a single branch pointing at several different places, this table must allow multiple rows with the same branch or tag name.
Among other things, this means there isn't a single PHID which can be used to identify a branch name in a stable way. However, we have several UIs where we want to be able to do this.
Some specific examples where we run into trouble: in Mercurial, if there are 5 heads for "default", that means there are 5 phids. And currently, if someone deletes a branch, we lose the PHID for it. Instead, we'd rather retain it so the whole world doesn't break if you accidentally delete a branch and then fix it a little later.
(I'll likely hold this until the rest of the logic is fleshed out a little more in followup changes.)
Test Plan: Ran `bin/storage upgrade`, saw the table get created without warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18602
Summary:
See <https://discourse.phabricator-community.org/t/unable-to-use-current-mercurial-on-debian-stretch/391>.
The Mercurial commit is helpful in particular: <https://www.mercurial-scm.org/repo/hg/rev/77eaf9539499>
We weren't vulnerable to the security issue (users can not control any part of the command) but pass the working directory explicitly to get past the new safety check.
I left `setCWD()` in place (a few lines below) just because it can't hurt, and in some other contexts it sometimes matter (for example, if commit hooks execute, they might inherit the parent CWD here or in other VCSes).
Test Plan:
- Cloned from a Mercurial repo locally over HTTP.
- Verified that SSH cloning already uses `-R` (it does, see `DiffusionMercurialServeSSHWorkflow`).
- Did not actually upgrade to Mercurial 4.0/4.1.3 to completely verify this, but a user in the Discourse thread asserted that a substantially similar fix worked correctly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18611
