Summary:
Ref T10923. Currently, users can disable or enable builtin URIs, but this doesn't actually do anything.
The behavior of "disable" has changed a bit over time and might need some further refinement, but it's currently meaningless for builtin URIs. Prevent adjustment of it. If users want to hide a URI, they should set "Display: Hidden" instead.
Test Plan:
- Disabled/enabled a non-builtin URI.
- Tried to disable a builtin URI, saw greyed out UI and got a helpful error message.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15899
Summary: Ref T10751. I think this mostly brings us up to date with the state of the world.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15897
Summary: Ref T10923. Fixes T10955. This was accidentally excluded when I broke the form into pages.
Test Plan: Saw edit field in panel; changed project tags for a repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923, T10955
Differential Revision: https://secure.phabricator.com/D15896
Summary: Ref T10751. These workflows have separate `getUser()` and `getViewer()` for weird legacy reasons. `getUser()` is correct.
Test Plan:
- Did a Git SSH push, verified that "Last Writer" reflected the proper user in the "Storage" UI in repository management.
- Grepped for other callsites, double-checked that they used correct users.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15893
Summary: Fixes T10952. Fixes T10930. I didn't implement this method correctly when I expanded this field for repositories.
Test Plan: Edited a paste without warnings.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T10930, T10952
Differential Revision: https://secure.phabricator.com/D15892
Summary:
Ref T10923. This makes the "Clone URI" UI a little nicer:
- Show whether each URI is read-only, read-write, or external.
- Clicking the button selects the URI.
- Add a link to manage the appropriate credentials.
Test Plan: {F1308302, size=full}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15891
Summary: Ref T10923. This links up some more related documentation about repository management.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15890
Summary: These parameters wrongly extend List.
Test Plan:
Used createdStart field for a search - didn't get error about "should be a list".
`git grep 'extends ConduitListParameterType'`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15881
Summary:
Ref T10923. We sort of dead-end new users creating repositories right now, by dumping them into the manage UI without an obvious way forward.
You can click the crumb to get to the repository, but by default it will say something like `R1` which isn't very obvious.
Add a more obvious navigational link to get to the main view.
Test Plan: {F1308196}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15889
Summary:
Ref T10923. The old behavior was to show a full command in SVN, Mercurial, and Git, like this:
- `git clone <uri>`
- `hg clone <uri>`
- `svn checkout <uri> <directory>`
In Git and Mercurial, the `<uri>` ends in something like `/nice-repository-name.git` so the default directory it creates is called `nice-repository-name/`.
In Subversion, we don't (and can't easily) do that for various reasons so we provide an explicit `<directory>` with the nice name.
In the update, I've changed things to just show the URI. I often found that I wanted the URI alone, not the whole clone command (for example, to `fetch`, `remote-add`, etc). This is also consistent with GitHub. Because we have nice URIs for Git and Mercurial, `git clone <uri>` has good behavior.
In Subversion, `svn checkout <uri>` has bad beahvior (you get a directory named `47/` or whatever). So continue showing the whole command there.
We can possibly tailor this after T4245 finishes up and we get access to `/source/nice-repository-name/` URIs.
Test Plan:
- Viewed a Subversion repository, saw a full command.
- Viewed a Git repository, saw only a clone URI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15888
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 T10941. This avoids a confusing dead end when configuring Subversion hosting, where `svnserve` will fail to execute hooks if the CWD isn't readable by the vcs-user.
Test Plan:
- Updated and committed in a hosted SVN repository.
- Ran some git operations, too.
- @dpotter confirmed this locally in T10941.
Reviewers: chad
Reviewed By: chad
Subscribers: dpotter
Maniphest Tasks: T10941
Differential Revision: https://secure.phabricator.com/D15879
Summary: Ref T10694. This setting no longer has any effect: we always show a limited amount of context now.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15886
Summary: Ref T10923. Although I'd ideally like to get rid of this eventually, keep it around for now.
Test Plan:
- Edited value for an SVN repository.
- Observed no panel present for a Git repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15883
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 T10923. Fixes T10406. This brings most of the guidance/instructions forward:
- Some remained as instructions.
- Some moved to documentation.
Test Plan: Went through all of the sections and hit the help.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10406, T10923
Differential Revision: https://secure.phabricator.com/D15873
Summary: Ref T10923. This provides a little guidance about hosted vs observed, and points at the `diffusion.ssh-*` options.
Test Plan: Poked around in the web UI, saw useful guidance.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15872
Summary: Ref T10923. This cleans up the remaining "pro" mess left by the cutover.
Test Plan: Viewed, managed, edited a repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15871
Summary:
Ref T10923. Primarily documents the process for creating repositories via the API.
Also fixes a couple of issues with `repositoryPHID` not being set yet when creating URIs via the API.
Test Plan:
- Followed all documented steps to create a new repository.
- Created and edited some new URIs from the web workflow, too.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15870
Summary: Ref T10923. This isn't complete yet, but reduces lies and increases truths.
Test Plan: Read documentation, clicked new "Documentation" nav item.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15868
Summary:
Ref T10923. Some of the dialogs ("Deactivate Repository", "Test Automation", etc.) had cancel or redirect URIs which I missed originally.
Go through them and make sure they all point to the right places.
Also removed one unused controller which I missed the first time around.
Test Plan:
- Opened all these dialogs in a new tab with Command-Click.
- Clicked every "cancel" and "submit" button on all of these dialogs.
- Got consistently sent to the place I came from.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15867
Summary:
Ref T10923.
- The "Policy" edit form currently goes "Push, View, Edit". Reorder the defaults to "View, Edit, Push".
- Editing Spaces doesn't currently work: the element appears in the UI, but isn't actually processed when handling transactions. Make that work.
Test Plan:
- Edited a repository policies, saw "View, Edit, Push".
- Moved a repository between Spaces.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15866
Summary:
Ref T10923. Paging wasn't being applied correctly when creating //new// repositories after API changes.
Also, the first redirect after creation wasn't sending users to the right place.
Test Plan:
- Created a new repostiory, got redirected properly.
- Verified that new repostiory flow has the correct fields (name, callsign, etc) and Conduit API has the correct fields (everything).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15865
Summary:
Ref T10694. General improvements:
- Remove leading empty lines from context snippets.
- Remove trailing empty lines from context snippets.
- If we removed everything, render a note.
- Try using `style` instead of `<pre>`? My thinking is that maybe Airmail has weird default rules for `<pre>`, since that's the biggest / most obvious thing that's different about this element to me.
Test Plan: Viewed normal comments locally, faked a comment on an empty line in the middle of a lot of other empty lines.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15864
Summary:
Ref T10694.
- Shift margins/padding around so inlines with multiple paragraphs get reasonable spacing.
- Add `text-decoration: none` to the "View Inline" link to kill the underline.
Test Plan: {F1265342}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15863
Summary: Ref T10694. Move the inline style more toward a mix of standard`<pre>` style and the web UI style for inlines.
Test Plan: See screenshots in comments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15857
Summary: Fixes T10928. This behavior hasn't changed at all and this is what, e.g., `git push --mirror` means, and the other behavior isn't possible, but be more explicit about the inherent destructive potential of mirroring.
Test Plan: Read documentation.
Reviewers: chad, sascha-egerer
Reviewed By: sascha-egerer
Subscribers: sascha-egerer
Maniphest Tasks: T10928
Differential Revision: https://secure.phabricator.com/D15861
Summary:
Ref T10927. Pretty sure the issue is:
- User makes an HTTPS request.
- Load balancer terminates it, but with an `X-Forwarded-Proto` header.
- `secure001` (or whatever; acting as web host) proxies it to `secure002` (or whatever; acting as a repository host). **This** connection is plain HTTP.
- Since this proxied connection is plain HTTP, we check if the repository can serve over "http", but it can't: only "https". So we fail incorrectly, even though the original user request was HTTPS.
In the long run we should probably forward the `X-Forwarded-Proto` header, but that has some weird implications and it's broadly fine to allow either protocol to serve as long as the other one is active: configuration like `security.require-https` is already stronger than these settings.
Test Plan: This is likely only observable in production, but normal cloning still works locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10927
Differential Revision: https://secure.phabricator.com/D15856
Summary:
Ref T10694. This mostly prevents us from having a degenerate case if someone leaves a 200-line inline.
- For one-line inlines, show 1 line of context above and below (3 lines total).
- For 3+ line inlines, show just the inline.
- For 7+ line inlines, show only the first part.
Test Plan: Made a bunch of weird long/short/different-sized comments, saw reasonble-appearing context in text and HTML mail output.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15853
Summary:
Ref T10694. Ref T9790. When generating inline diff context, highlight it and then mangle the highlighted output into `style="..."` so it works in HTML.
Also try to tighten up some spacing/formatting stuff.
Test Plan:
Got some output in this vein:
{F1259937}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790, T10694
Differential Revision: https://secure.phabricator.com/D15852
Summary:
Ref T10694. This is still missing some pieces, but seems to get most of the data into the mail in a plausible format:
- When an inline remarks on code, show the patch inline in the mail body.
- When an inline replies to another inline, show that other inline in the mail body.
- Apply remarkup rendering to inline content.
- Apply basic styling to mail body blocks.
Not covered yet:
- Syntax highlighting.
- Diff highlighting.
- Maybe clearer style/layout hints to connect comments to what they reply to? Current approach might get messy with inlines that have blockquotes and code blocks inside them, for example.
- I probably want to cap the amount of diff context we ever show to ~7 lines, even if you drag over 200 lines of code.
- CSS is a generally a bit rough still.
- The `unified-comment-context` option is effectively always on now, and should be removed.
- Text section is getting indented right now but probably shouldn't be.
- Spacing, etc., might be a bit off.
Test Plan:
Rigged Home to render these things, got a plausible-looking render (top is text, bottom is HTML):
{F1259052}
Sent myself some inline comment mail, got a plausible result.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15850
Summary: Ref T9790. This passes the map down so we can generate highlighted mail.
Test Plan:
Generated this relatively respectable-looking HTML mail:
{F1258558}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790
Differential Revision: https://secure.phabricator.com/D15848
Summary:
Ref T9790. This prepares the syntax color rules to be reused in mail.
This goes about halfway toward T5701 by sort-of supporting different styles but not really.
Test Plan:
- Ran `bin/celerity syntax` to regenerate syntax map.
- Viewed some highlighted code, didn't see any differences.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790
Differential Revision: https://secure.phabricator.com/D15846
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: Seems to work ok, if you give `size=wide` to an image file, we blow it out a bit in DocumentPro mode.
Test Plan:
Test in Phame and Maniphest.
{F1256717}
{F1256718}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15840
Summary: Feels a little hairy, but should be fine overall. Ups the css specificity enough that it's always displayed the same. The main problem is headers and boxes get put everywhere, and sometimes override each other. Fixes T10757
Test Plan: review a countdown in a phame post and phriction document. Check diviner, phriction for regressions.
Reviewers: epriestley, #phacility
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10757
Differential Revision: https://secure.phabricator.com/D15664
Summary:
Ref T10748. These:
- Look nice.
- Hint at panel contents / effects.
- Hint which panels have been customized.
- Allow panels with issues or errors to be highlighted with an alert/attention icon.
Test Plan: {F1256156}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15836
Summary: Ref T10748. This copies existing code in the `CreateController` which will eventually be removed.
Test Plan:
- Created a new repository with the EditPro workflow.
- Saw it come up into the cluster properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15835
Summary:
Fixes T10912. When you drag tasks within a milestone, we currently apply an overbroad, API-focused rule and add the parent board's project. This logic was added fairly recently, as part of T6027, to improve the behavior of API-originated moves.
Later on, this causes the task to toggle in and out of the parent project on every alternate drag.
This logic is also partially duplicated in the `MoveController`.
- Add test coverage for this interaction.
- Fix the logic so it accounts for subproject / milestone columns correctly.
- Put all of the logic into the TransactionEditor, so the API gets the exact same rules.
Test Plan:
- Added a failing test and made it pass.
- Dragged tasks around within a milestone column:
- Before patch: they got bogus project swaps on every other move.
- After patch: projects didn't change (correct).
- Dragged tasks around between normal and milestone columns.
- Before patch: worked properly.
- After patch: still works properly.
Here's what the bad changes look like, the task is swapping projects with every other move:
{F1255957}
The "every other" is because the logic was trying to do this:
- Add both the parent and milestone project.
- Whichever one exists already gets dropped from the change list because it would have no effect.
- The other one then applies.
- In applying, it forces removal of the first one.
- Then this process repeats in the other direction the next time through.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10912
Differential Revision: https://secure.phabricator.com/D15834
