Summary:
Fixes T11309. When checking if a repository was fully imported, we incorrectly allow unreachable, un-imported commits to prevent the repository from moving to "Imported".
This can happen if you delete branches from a repository while it is importing.
Instead, ignore unreachable commits when checking for remaining imports, and when reporting status via `bin/repository importing`.
Test Plan:
- Stopped daemons.
- Created a new repository and activated it.
- Ran `bin/repository update Rxx`.
- Deleted a branch in the repository.
- Ran `bin/repository update Rxx`.
- Ran daemons to flush queue.
Now:
- Ran `bin/repository importing`. Old behavior: showed unreachable commits as importing. New behavior: does not show unreachable commits.
- Ran `bin/repository update`. Old behavior: failed to move repository to "imported" status. New behavior: correctly moves repository to "imported" status.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11309
Differential Revision: https://secure.phabricator.com/D16269
Summary:
Ref T11309. In that task, a user misunderstood two parts of this error:
- They took "exception" to mean "unexpected failure", when it was intended to mean "rare circumstance".
- They intereted the internal ID number of a commit to mean that Phabricator was malfunctioning.
Make the language of this condition more direct, explaining what the situation means in greater detail.
Additionally, we would previously re-throw this exception, which would make the daemon exit, wait a moment, and restart. This was normal and expected.
When //unexpected// failures occur, it's important do to this: it prevents a daemon failing in a loop from causing too many side effects (e.g., limit of 1 email per 5 seconds instead of thousands per second).
When expected, permanent failures occur, we do not need to do this: the task will not be retried. I just did it because it was slightly more consistent ("failures restart daemons") and we had few permanent failure types at the time.
We have more now, and restarting the daemons generates some additional logs which have the potential to confuse. Cycling the daemon also (intentionally) reduces the rate at which we process tasks, which can be bad for permanent failures like "deleted commit" because users can delete a huge number of commits and possibly clog up the queue with cycle-after-failure actions.
Test Plan:
Tried to process a deleted commit, saw a new message:
```
2016-07-11 9:30:22 AM [STDE] <VERB> PhabricatorTaskmasterDaemon Task 1428658 was cancelled: Commit "R55:6c46b7d0fb82a859ca3f87a95dc8dcceef8088c9" (with internal ID "282161") is no longer reachable from any branch, tag, or ref in this repository, so it will not be imported. This usually means that the branch the commit was on was deleted or overwritten.
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11309
Differential Revision: https://secure.phabricator.com/D16268
Summary: See Z2352#28072. Expose this flag to allow callers to take actions after an import finishes, which is generally reasonable.
Test Plan: Ran query from console, saw `isImporting` flag in results.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16247
Summary:
Fixes T11269. The basic issue is that `git log` in an empty repository exits with an error message.
Prior to recent Git (2.6?), this message reads:
> fatal: bad default revision 'HEAD'
This message was somewhat recently changed by <ce11360467>. After that, it reads:
> fatal: your current branch 'master' does not have any commits yet
This change isn't //technically// a //complete// fix because you could still hit this issue like this:
- Create an empty repository.
- Push some stuff to `master`.
- Delete `master`.
However, this is very rare and even in this case the repository will fix itself once you push something again. We can try to fix that if any users ever actually hit it.
Test Plan:
- Created a new empty Git repository.
- Ran `bin/repository update Rxx`.
- Before patch: "git log" error because of the empty repository.
- After patch: clean update.
- Also ran `repository update` on a non-empty repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11269
Differential Revision: https://secure.phabricator.com/D16234
Summary:
Ref T4788. Fixes T9232. This moves the "search for stuff to attach to this object" flow away from hard-coding and legacy constants and toward something more modular and flexible.
It also adds an "Edit Commits..." action to Maniphest, resolving T9232. The behavior of the search for commits isn't great right now, but it will improve once these use real ApplicationSearch.
Test Plan: Edited a tasks' related commits, mocks, tasks, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T9232
Differential Revision: https://secure.phabricator.com/D16189
Summary:
Ref T11208. See that task for a more detailed description of revprops.
This allows revprop changes in a hosted Subversion repository if the repository has the "allow dangerous changes" flag set.
In the future, we could expand this into real Herald support, but the only use case we have for now is letting `svnsync` work.
Test Plan:
Edited revprops with `svn propset --revprop -r 2 propkey propvalue repositoryuri`:
- Tried before patch, got a "configure a commit hook" error.
- Tried after patch, got a "dangerous change" error.
- Allowed dangerous changes.
- Did a revprop edit.
- Prevented dangerous changes.
- Got an error again.
- Made a normal commit to an SVN repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11208
Differential Revision: https://secure.phabricator.com/D16174
Summary:
Fixes T11180. In Git, it's possible to tag a tag (????). When you do, we try to log the tag-object, which automatically resolves to the commit and fails.
Just skip these. If "A" points at "B" which points at "C", it's fine to ignore "A" and "B" since we'll get the same stuff when we process "C".
Test Plan:
- Tagged a tag.
- Pushed it.
- Discovered it.
- Before patch: got exception similar to the one in T11180.
- After patch: got tag-tag skipped. Also got slightly better error messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11180
Differential Revision: https://secure.phabricator.com/D16149
Summary:
Ref T9028. Mostly, this gives them a strikethru style.
(I think this is probably the right definition of "closed" for commits. Another definition might be "audited", but I don't think completing audits really "closes" a commit.)
Test Plan: {F1689662}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16135
Summary: Ref T9028. Ref T6878. This rule should probably be refined in the long term, but for now just ignore "phabricator/diff/12424" and similar staging area tags.
Test Plan: Ran `bin/repository discover --verbose` on a repository with staging area refs, saw Phabricator ignore those refs as untracked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6878, T9028
Differential Revision: https://secure.phabricator.com/D16134
Summary:
Ref T9028. This allows us to detect when commits are unreachable:
- When a ref (tag, branch, etc) is moved or deleted, store the old thing it pointed at in a list.
- After discovery, go through the list and check if all the stuff on it is still reachable.
- If something isn't, try to follow its ancestors back until we find something that is reachable.
- Then, mark everything we found as unreachable.
- Finally, rebuild the repository summary table to correct the commit count.
Test Plan:
- Deleted a ref, ran `pull` + `refs`, saw oldref in database.
- Ran `discover`, saw it process the oldref, mark the unreachable commit, and update the summary table.
- Visited commit page, saw it properly marked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16133
Summary:
Ref T9028. This corrects the reachability of existing commits in a repository.
In particular, it can be used to mark deleted commits as unreachable.
Test Plan:
- Ran it on a bad repository, with bad args, etc.
- Ran it on a clean repo, got no changes.
- Marked a reachable commit as unreachable, ran script, got it marked reachable.
- Started deleting tags and branches from the local working copy while running the script, saw greater parts of the repository get marked unreachable.
- Pulled repository again, everything automatically revived.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16132
Summary:
Ref T9028. This improves the daemon behavior for unreachable commits. There is still no way for commits to become marked unreachable on their own.
- When a daemon encounters an unreachable commit, fail permanently.
- When we revive a commit, queue new daemons to process it (since some of the daemons might have failed permanently the first time around).
- Before doing a step on a commit, check if the step has already been done and skip it if it has. This can't happen normally, but will soon be possible if a commit is repeatedly deleted and revived very quickly.
- Steps queued with `bin/repository reparse ...` still execute normally.
Test Plan:
- Used `bin/repository reparse` to run every step, verified they all mark the commit with the proper flag.
- Faked the `reparse` exception in the "skip step" code, used `repository reparse` to skip every step.
- Marked a commit as unreachable, ran `discover`, saw daemons queue for it.
- Ran daemons with `bin/worker execute --id ...`, saw them all skip + queue the next step.
- Marked a commit as unreachable, ran `bin/repository reparse` on it, got permanent failures immediately for each step.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16131
Summary:
Ref T9028. This is the easy part of dealing with deleted commits:
- Add a flag for unreachable commits (nothing sets this flag yet).
- Ignore unreachable commits when querying for known commits during discovery, so we pretend they do not exist.
- When recording a commit, try just reviving an existing unreachable commit first. If that works, bail out.
Test Plan:
- Artificially marked a commit as unreachable with raw SQL.
- Verified it said "deleted: unreachable" in the UI.
- Ran `repository discover --trace --verbose`.
- Saw the discovery process ignore the commit when filling the cache.
- Saw the discovery process revive the commit instead of trying to record it again.
- Web UI now shows the commit as normal.
- Running `repository discover` again doesn't make any further changes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16130
Summary:
Ref T9028. Fixes T6878. Currently, we only fetch and discover branches. This is fine 99% of the time but sometimes commits are pushed to just a tag, e.g.:
```
git checkout <some hash>
nano file.c
git commit -am '...'
git tag wild-wild-west
git push origin wild-wild-west
```
Through a similar process, commits can also be pushed to some arbitrary named ref (we do this for staging areas).
With the current rules, we don't fetch tag refs and won't discover these commits.
Change the rules so:
- we fetch all refs; and
- we discover ancestors of all refs.
Autoclose rules for tags and arbitrary refs are just hard-coded for now. We might make these more flexible in the future, or we might do forks instead, or maybe we'll have to do both.
Test Plan:
Pushed a commit to a tag ONLY (`vegetable1`).
<cf508b8de6>
On `master`, prior to the change:
- Used `update` + `refs` + `discover`.
- Verified tag was not fetched with `git for-each-ref` in local working copy and the web UI.
- Verified commit was not discovered using the web UI.
With this patch applied:
- Used `update`, saw a `refs/*` fetch instead of a `refs/heads/*` fetch.
- Used `git for-each-ref` to verify that tag fetched.
- Used `repository refs`.
- Saw new tag appear in the tags list in the web UI.
- Saw new refcursor appear in refcursor table.
- Used `repository discover --verbose` and examine refs for sanity.
- Saw commit row appear in database.
- Saw commit skeleton appear in web UI.
- Ran `bin/phd debug task`.
- Saw commit fully parse.
{F1689319}
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T6878, T9028
Differential Revision: https://secure.phabricator.com/D16129
Summary:
Ref T11137. This class is removed in D16099. Depends on D16099.
`PhutilURI` now attempts to "just work" with Git-style URIs, so at least in theory we can just delete all of this code and pretend it does not exist.
(I've left "Display URI" and "Effective URI" as distinct, at least for now, because I think the distinction may be relevant in the future even though it isn't right now, and to keep this diff small, although I may go remove one after I think about this for a bit.)
Test Plan:
- Created a new Git repository with a Git URI.
- Pulled/updated it, which now works correctly and should resolve the original issue in T11137.
- Verified that daemons now align the origin to a Git-style URI with a relative path, which should resolve the original issue in T11004.
- Grepped for `PhutilGitURI`.
- Also grepped in `arcanist/`, but found no matches, so no patch for that.
- Checked display/conduit URIs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11137
Differential Revision: https://secure.phabricator.com/D16100
Summary: Ref T10227. When we perform `git` http operations (fetch, mirror) check if we should use a proxy; if we should, set `http_proxy` or `https_proxy` in the environment to make `git` have `curl` use it.
Test Plan:
- Configured a proxy extension to run stuff through a local instance of Charles.
- Ran `repository pull` and `repository mirror`.
- Saw `git` HTTP requests route through the proxy.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10227
Differential Revision: https://secure.phabricator.com/D16092
Summary:
Fixes T11082. Currently, the `/123/` and `/CALLSIGN/` versions of the URI get the same score.
Also the scores are backwards.
Test Plan:
- Added `getPublicCloneURI()` output to repository listing.
- Before patch, saw a repository with a callsign list a less-preferred ID-based URI.
- After patch, saw the repository list the more-preferred callsign-based URI.
Reviewers: chad
Reviewed By: chad
Subscribers: nikolay.metchev
Maniphest Tasks: T11082
Differential Revision: https://secure.phabricator.com/D16008
Summary:
Ref T4292. For hosted, clustered repositories we have a good way to increment the internal version of the repository: every time a user pushes something, we increment the version by 1.
We don't have a great way to do this for observed/remote repositories because when we `git fetch` we might get nothing, or we might get some changes, and we can't easily tell //what// changes we got.
For example, if we see that another node is at "version 97", and we do a fetch and see some changes, we don't know if we're in sync with them (i.e., also at "version 97") or ahead of them (at "version 98").
This implements a simple way to version an observed repository:
- Take the head of every branch/tag.
- Look them up.
- Pick the biggest internal ID number.
This will work //except// when branches are deleted, which could cause the version to go backward if the "biggest commit" is the one that was deleted. This should be OK, since it's rare and the effects are minor and the repository will "self-heal" on the next actual push.
Test Plan:
- Created an observed repository.
- Ran `bin/repository update` and observed a sensible version number appear in the version table.
- Pushed to the remote, did another update, saw a sensible update.
- Did an update with no push, saw no effect on version number.
- Toggled repository to hosted, saw the version reset.
- Simulated read traffic to out-of-sync node, saw it do a remote fetch.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15986
Summary:
Fixes T11030. Fixes T11032.
- Allow HTTP access to "Public" repositories even if `diffusion.allow-http-auth` is disabled.
- If you run Phabricator on an unusual port (???) use that port as the default when generating HTTP URIs.
Test Plan:
- Faked `phabricator.base-uri` to an unusual port, saw repository HTTP URI generate with an unusual port.
- Disabled `diffusion.allow-http-auth`, confirmed that toggling view policy between "public" and "users" activated or deactivated HTTP clone URI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11030, T11032
Differential Revision: https://secure.phabricator.com/D15973
Summary:
Ref T10939. Fixes T10174. We can currently trigger "uninteresting" auditors in two ways:
- Packages with auditing disabled ("NONE" audits).
- Packages with auditing enabled, but they don't need an audit (e.g., author is a pacakge owner; "NOT REQUIRED" audits).
These audits aren't interesting (we only write them so we can list "commits in this package" from other UIs) but right now they take up the audit slot. In particular:
- They show in the UI, but are generally useless/confusing nowadays. The actual table of contents does a better job of just showing "which packages do these paths belong to" now, and shows all packages for each path.
- They block Herald from adding real auditors.
Change this:
- Don't show uninteresting auditors.
- Let Herald upgrade uninteresting auditors into real auditors.
Test Plan:
- Ran `bin/repository reparse --owners <commit> --force`, and `--herald` to trigger Owners and Herald rules.
- With a package with auditing disabled, triggered a "None" audit and saw it no longer appear in the UI with the patch applied.
- With a package with auditing disabled, added a Herald rule to trigger an audit. With the patch, saw it go through and upgrade the audit to "Audit Required".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10174, T10939
Differential Revision: https://secure.phabricator.com/D15940
Summary:
Ref T10939. Fixes T10181. This slightly simplifies, then documents the auditing rules, which haven't been updated for a while. In particular:
- If an owner authored the change, never audit.
- Examine all reviewers to determine reviewer audit status, not just the first reviewer.
- Simplify some of the loading code a bit.
Test Plan:
- Ran `bin/repository reparse --owners <commit> --force` to trigger this stuff.
- Verified that the web UI did reasonable things with resulting audits.
- Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10181, T10939
Differential Revision: https://secure.phabricator.com/D15939
Summary:
Ref T10939. This is just a bug. I thought this was what was described in T10174 but that's actually talking about something completely different.
Also make a `<select />` slightly easier to use.
Test Plan:
- Created a package with auditing enabled.
- Pushed a change.
- Saw audit trigger.
- Disabled the package, pushed a change.
- Before patch: saw audit trigger improperly.
- After patch: restarted daemons, then saw audit correctly not trigger.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15907
Summary:
Ref T4292. Currently, we hold one big lock around the whole `bin/repository update` workflow.
When running multiple daemons on different hosts, this lock can end up being contentious. In particular, we'll hold it during `git fetch` on every host globally, even though it's only useful to hold it locally per-device (that is, it's fine/good/expected if `repo001` and `repo002` happen to be fetching from a repository they are observing at the same time).
Instead, split it into two locks:
- One lock is scoped to the current device, and held during pull (usually `git fetch`). This just keeps multiple daemons accidentally running on the same host from making a mess when trying to initialize or update a working copy.
- One lock is scoped globally, and held during discovery. This makes sure daemons on different hosts don't step on each other when updating the database.
If we fail to acquire either lock, assume some other process is legitimately doing the work and bail more quietly instead of fataling. In approximately 100% of cases where users have hit this lock contention, that was the case: some other daemon was running somewhere doing the work and the error didn't actually represent an issue.
If there's an actual problem, we still raise a diagnostically useful message if you run `bin/repository update` manually, so there are still tools to figure out that something is hung or whatever.
Test Plan:
- Ran `bin/repository update`, `pull`, `discover`.
- Added `sleep(5)`, forced processes to contend, got lock exceptions and graceful exit with diagnostic message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15903
Summary:
Fixes T10940. Two issues currently:
First, `PullLocal` deamon refuses to update non-cluster repositories on cluster devices. However, this is surprising/confusing/bad because as soon as you enroll a repository host in the cluster, most of the repositories on it stop working until you `clusterize` them. This is especially confusing because the documentation gives you a very nice, gradual walkthrough about going through things slowly and being able to check your work at every step, but we really drop you off a bit of a cliff here. The workflow implied by the documentation is a desirable one.
This operation is generally only unsafe/problematic if the daemon would be creating a //new// working copy. If a working copy already exists, we can reasonably guess that it's almost certainly because you've enrolled a previously un-clustered host into a new cluster. This allows the nice, gradual workflow the documentation describes to proceed as expected, without any weird surprises.
Instead of refusing to update these repositories, only refuse to update them if updating would create a new working copy. This should make transitioning much smoother without any meaningful reduction in safety.
Second, the lower-level `bin/repository update`, `refs`, `mirror`, etc., commands don't apply this same check. However, these commands are potentially just as dangerous. Use the same code to do a similar check there, making sure we only operate on repositories that are either expected to be on the current device, or which already exist here.
Test Plan:
- Ran `bin/phd debug pull`, saw diagnostic information choose to update most repositories (including some non-cluster repositories) but properly skip non-cluster repositories that do not exist locally.
- Ran `bin/repository update`, etc., saw the command apply consistent rules to the rules applied by `PullLocal` and refuse to update non-local repositories it would need to create.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10940
Differential Revision: https://secure.phabricator.com/D15902
Summary: Fixes T10948. Ref T10923. Make these rules a little more thorough and document their behavior.
Test Plan: Looked at Diffusion clone URIs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923, T10948
Differential Revision: https://secure.phabricator.com/D15887
Summary: Fixes T10815. We already recovered reasonably from this for cluster repositories, but not for non-cluster repositories.
Test Plan:
- Viewed cluster and non-cluster empty Git repository.
- Viewed cluster and non-cluster empty Mercurial repository.
- Viewed cluster and non-clsuter empty hosted SVN repository.
- Viewed cluster and non-cluster empty observed SVN repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10815
Differential Revision: https://secure.phabricator.com/D15878
Summary:
Ref T10923. Fixes T9554.
When hosting a repository, we currently have a heuristic that tries to detect when you're doing an initial import: if you push more than 7 commits to an empty repository, it counts as an import and we disable mail/feed/etc.
Do something similar for observed repositories: if the repository is empty and we discover more than 7 commits, switch to import mode until we catch up.
This should align behavior with user expectation more often when juggling hosted vs imported repositories.
Test Plan:
- Created a new hosted repository.
- Activated it and allowed it to fully import.
- Added an "Observe URI".
- Saw it automatically drop into "Importing" mode until the import completed.
- Swapped it back to hosted mode.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9554, T10923
Differential Revision: https://secure.phabricator.com/D15877
Summary:
Ref T10923. When regenerating the URI index for a repository, index every URI.
- Also, make the index slightly stricter (domain + path instead of just path). Excluding the domain made more sense when we were generating only first-party URIs.
- Make the index smarter about `/diffusion/123/` URIs.
- Show normalized URIs in `diffusion.repository.search` results.
Test Plan:
- Ran migration.
- Verified sensible-looking results in database.
- Searched for a repository URI by first-party clone URI.
- Searched for a repository URI by mirror URI.
- Used `diffusion.repository.search` to get information about repository URIs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15876
Summary: Ref T10923. Walk users through the "create, configure, activate" workflow a little better and set expectations more clearly.
Test Plan:
- Created a new repository, saw new UI help.
- Activated repository, saw onboarding help disappear.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15875
Summary:
Ref T10923.
- Hide "Automation", "Staging" and "Branches" in repositories where they do nothing.
- Fix SVN SSH URIs to read "svn+ssh://" and have proper paths.
Test Plan:
- Verified irrelevant sections did not appear in Subversion in Manage UI.
- Checked out a new hosted SVN repository.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15874
Summary: Ref T10748. We're returning a `PhabricatorRepositoryURI` here but the code expects an actual `PhutilURI`.
Test Plan:
This should clear this up in production:
```
Daemon 180278 STDE [Wed, 04 May 2016 23:25:16 +0000] [2016-05-04 23:25:16] EXCEPTION: (PhutilProxyException) Error while executing Task ID 1677075. {>} (RuntimeException) Object of class PhabricatorRepositoryURI could not be converted to string at [<phutil>/src/error/PhutilErrorHandler.php:205]
Daemon 180278 STDE [Wed, 04 May 2016 23:25:16 +0000] arcanist(head=master, ref.master=c58f1b9a2507), libcore(), phabricator(head=master, ref.master=29d1115037b8), phutil(head=master, ref.master=0709cd5cfc26), services(head=master, ref.master=04ae8c8f8e3b)
Daemon 180278 STDE [Wed, 04 May 2016 23:25:16 +0000] #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/repository/storage/PhabricatorRepository.php:1200]
Daemon 180278 STDE [Wed, 04 May 2016 23:25:16 +0000] #1 <#2> PhabricatorRepository::getPublicCloneURI() called at [<phabricator>/src/applications/repository/storage/PhabricatorRepositoryCommit.php:395]
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15844
Summary:
Ref T10748. This needs more extensive testing and is sure to have some rough edges, but seems to basically work so far.
Throwing this up so I can work through it more deliberately and make notes.
Test Plan:
- Ran migration.
- Used `bin/repository list` to list existing repositories.
- Used `bin/repository update <repository>` to update various repositories.
- Updated a migrated, hosted Git repository.
- Updated a migrated, observed Git repository.
- Converted an observed repository into a hosted repository by toggling the I/O mode of the URI.
- Conveted a hosted repository into an observed repository by toggling it back.
- Created and activated a new empty hosted Git repository.
- Created and activated an observed Git repository.
- Updated a mirrored repository.
- Cloned and pushed over HTTP.
- Tried to HTTP push a read-only repository.
- Cloned and pushed over SSH.
- Tried to SSH push a read-only repository.
- Updated several Mercurial repositories.
- Updated several Subversion repositories.
- Created and edited repositories via the API.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15842
Summary:
Ref T10748. This migrates and swaps mirroring to `PhabricatorRepositoryURI`, obsoleting `PhabricatorRepositoryMirror`.
This prevents you from editing, adding or disabling mirrors unless you know a secret URI (until the UI cuts over fully), but existing mirroring is not affected.
Test Plan:
- Added a mirroring URI to an old repository.
- Verified it worked with `bin/repository mirror`.
- Migrated forward.
- Verified it still worked with `bin/repository mirror`.
- Wow, mirroring: https://github.com/epriestley/locktopia-mirror
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15841
Summary:
Ref T10748. In D14250#158181, I accepted this conditional on removing it once Conduit could handle it.
Conduit can now handle it, or at least will be able to as soon as T10748 cuts over.
Test Plan: Grepped for `repository edit`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15839
Summary:
Ref T10748. This has had many problems for a long time, can't create hosted repositories, can't create cluster repositories, etc. It is obsoleted by `diffusion.repository.edit`. Remove it.
(Right now `diffusion.repository.edit` isn't a strict replacement, but it will be as soon as the URI stuff cuts over.)
Test Plan: Grepped for `repository.create`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15838
Summary:
Ref T4039. Long ago these were more freely editable and there were some security concerns around creating a repository, then setting its local path to point somewhere it shouldn't.
Local paths are no longer editable so there's no real reason we need to provide a uniqueness guarantee anymore, but you could still make a mistake with `bin/repository move-paths` by accident, and it's a little cleaner to pull them out into their own column with a key.
(We still don't -- and, largely can't -- guarantee that two paths aren't //equivalent// since one might be symlinked to the other, or symlinked only on some hosts, or whatever, but the primary value here is as a sanity check that you aren't goofing things up and pointing a bunch of repositories at the same working copy by mistake.)
Test Plan:
- Ran migrations.
- Grepped for `local-path`.
- Listed and moved paths with `bin/repository`.
- Created a new repository, verified its local path populated correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4039
Differential Revision: https://secure.phabricator.com/D15837
Summary:
Ref T10748. Ref T10366. Allows users to set credential for new URIs.
- Ref T7221. Our handling of the "git://" protocol is currently incorrect. This protocol is not authenticated, but is considered an SSH protocol. In the new UI, it is considered an anonymous/unauthenticated protocol instead.
- Ref T10241. This fixes the `PassphraseCredentialControl` so it doesn't silently edit the value if the current value is not visible to you and/or not valid.
Test Plan:
Performed a whole lot of credential edits, removals, and adjustments. I'll give this additional vetting before cutting over to it.
{F1253207}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7221, T10241, T10366, T10748
Differential Revision: https://secure.phabricator.com/D15829
Summary:
Ref T10748.
- Allow users to add new URIs by clicking a button instead of knowing a secret URI.
- Validate that URIs are actually valid URIs.
- Add enable/disable action and strings.
Test Plan:
- Created a new URI.
- Tried to create a nonsense URI, created a good URI.
- Enabled/disabled a URI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15825
Summary: Ref T10748. Adds a "uris" attachment with URI information.
Test Plan: Queried URI information via Conduit, saw reasonable looking information.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15822
Summary: Ref T10748. Brings the rest of the transactions to EditEngine, supports creating via API.
Test Plan:
- Created a URI via API.
- Created a URI via web.
- Tried to apply sneaky transactions, got rejected with good error messages. <_< >_>
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15821
Summary: Ref T10748. Ref T10366. This documents how everything is planned to work shortly.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler, scode
Maniphest Tasks: T10366, T10748
Differential Revision: https://secure.phabricator.com/D15817
Summary:
Ref T10748.
- New View page for repository URIs.
- Make display and I/O behavior (observe, mirror, read, read/write) editable.
- Add a bunch of checks to prevent you from completely screwing up a repository by making it writable from a bunch of differnet sources.
Test Plan:
{F1249866}
{F1249867}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15816
Summary:
Ref T10748. Ref T10366. This adds a new EditEngine, EditController, Editor, Query, and Transaction for RepositoryURIs.
None of these really do anything helpful yet, and these URIs are still unused in the actual application.
Test Plan: {F1249794}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10366, T10748
Differential Revision: https://secure.phabricator.com/D15815
Summary:
Ref T10748. Allow the new EditEngine workflow to create repositories by giving the user a modal repository type choice upfront.
(The rest of this flow is still confusing/weird, though.)
Test Plan:
- Created a new repository.
{F1249626}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15813
Summary: Ref T10748. This brings the "Actions" items (publish/notify + autoclose enabled) into the new UI.
Test Plan:
- Edited this stuff via EditEngine and Conduit.
- Viewed via new Manage UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15811
Summary: Ref T10748. Port this, add EditEngine support, add some type validation to the transaction.
Test Plan:
- Edited via EditEngine.
- Edited via Conduit.
- Viewed via Management UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15808
Summary: Ref T4292. This provides at least some sort of hint about how to set up cluster repositories.
Test Plan:
- Read documentation.
- Ran `bin/repository clusterize` to add + remove clusters.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15798
Summary: This gets over-escaped instead of bolded right now, but I only ever hit it when exporting/importing and never both cleaning it up.
Test Plan: Ran `bin/repository move-paths`, saw bolded "Move" instead of ANSI escape sequences.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15797
