Summary:
Fixes T7122. The way this query works is a little surprising:
- If executed as `withRepositoryIDs(...)`, it assumes you are passing one //or more// repository IDs, so it will never resolve ambiguous identifiers (e.g., "123" instead of "rSVN123").
- If executed as `withRepository(...)`, it knows you are passing exactly one repository and will use that to imply context and resolve these identifiers correctly.
This isn't very obvious from the API, but I'm not sure how to make it more clear.
(Making `withRepositoryIDs()` do the `withRepository(...)` thing if only one ID was passed in would mean its behavior varied if you passed 1 vs 2 repository IDs, which seems worse / morse surprising.)
Test Plan: Various subversion UIs no longer fail to look up commits.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mormegil, epriestley
Maniphest Tasks: T7122
Differential Revision: https://secure.phabricator.com/D11645
Summary: Ref T7094. The class DiffusionRequest has other public methods which use getUser() in an unguarded way. Code inspection of the call sites for loadCommit() also leads me to believe the $user is properly set.
Test Plan: clicked around diffusion a bunch and everything seemed to work okay. (happy to test any particular esoteric endpoints that come to mind)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11585
Summary:
Ref T2783. I think this served two purposes:
- Improving performance in cases where we "know" a repository is local.
- Preventing loops.
It is now obsolete:
- After D11476, refs can almost always resolve on a fast path.
- As T2783 moves forward, we can usually no longer know when a repository is local without actually looking it up -- almost everything is allowed to run anywhere.
- The cluster behavior in D11475 now prevents loops.
Test Plan: `grep`, browsed around. This didn't really do much of anything anymore.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11477
Summary:
Ref T2783. With service-oriented calls, we take a larger performacne hit than necessary resolving refs.
Instead of resolving refs over the wire, try to resolve them from the database first. This can resolve almost all refs (commit hashes, branch and tag names).
This can't resolve weird refs like `master~50`, and obviously can't resolve invalid refs. In those cases we'll go back to the old logic, call `diffusion.resolverefs`, and end up with the right result.
Test Plan:
- Browsed repositories in Diffusion.
- Verified that service repositories no longer make unnecessary `diffusion.resolverefs` calls for common refs (branch names, commit hashes).
- Resolved refs like `master~50`, saw call to underlying VCS and correct result.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11476
Summary:
Fixes T7020. When an external user makes a Conduit request to Diffusion but the repository isn't hosted locally, we need to proxy it.
This also adds a guard layer to prevent requests from getting infinitely proxied inside the cluster.
In "trivial" configurations (where the repository is a service repository, but the service is on the local device) I'm making us always proxy anyway. This basically makes it reasonable to test this stuff (otherwise you'd have to set up two different installs) and this configuration doesn't make much sense in real life (if you're using multiple machines, making one a dedicating daemons+repo box is almost certainly the most reasonable configuration, even for a cluster size of 2).
Test Plan:
- With a service-hosted repository, made Diffusion conduit calls and browsed the UI. Verified requests got proxied once, then resovled.
- With a non-service repository, made Diffusion conduit calls and browsed UI. Verified requests were handled in-process immediately.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7020
Differential Revision: https://secure.phabricator.com/D11475
Summary: Fixes T7011. Recent refactoring here caused us to begin ignoring URI parameters like `commit`. Most controllers take parameters as a `dblob`, which was still parsed properly.
Test Plan:
- Editing different commits actually edits the desired commits.
- Browsed around some `dblob` pages and verified they still work properly.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7011
Differential Revision: https://secure.phabricator.com/D11473
Summary:
Ref T1493.
- When viewing an invalid branch, show a "there is no such branch" message.
- When viewing an empty repository, show a "this repository is empty" message.
Test Plan:
- Viewed empty, bad branch, and nonempty in Git.
- Viewed empty, bad branch, and nonempty in Mercurial.
- Viewed empty and nonempty in Subversion.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T1493
Differential Revision: https://secure.phabricator.com/D9912
Summary: Applied some more linter fixes that I previously missed because my global `arc` install was out-of-date.
Test Plan: Will run `arc unit` on another host.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9443
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary: Fixes T5113. This was caught in the crossfire of cleaning up the DiffusionRequest "commit" properties.
Test Plan: Loaded `/rXnnnn` with some of the `nnn` missing.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5113
Differential Revision: https://secure.phabricator.com/D9253
Summary:
Ref T4994. This stuff works:
- You can dump a blob of coverage information into `diffusion.updatecoverage`. This wipes existing coverage information and replaces it.
- It shows up when viewing files.
- It shows up when viewing commits.
This stuff does not work:
- When viewing files, the Javascript hover interaction isn't tied in yet.
- We always show this information, even if you're behind the commit where it was generated.
- You can't do incremental updates.
- There's no aggregation at the file (this file has 90% coverage), diff (the changes in this commit are 90% covered), or directory (the code in this directory has 90% coverage) levels yet.
- This is probably not the final form of the UI, storage, or API, so you should expect occasional changes over time. I've marked the method as "Unstable" for now.
Test Plan:
- Ran `save_lint.php` to check for collateral damage; it worked fine.
- Ran `save_lint.php` on a new branch to check creation.
- Published some fake coverage information.
- Viewed an affected commit.
- Viewed an affected file.
{F151915}
{F151916}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: jhurwitz, epriestley, zeeg
Maniphest Tasks: T5044, T4994
Differential Revision: https://secure.phabricator.com/D9022
Summary:
Allows you to quickly search for files within a repository. Roughly:
- We build a big tree of everything and ship it to the client.
- The client implements a bunch of Sublime-ish magic to find paths.
Test Plan: {F154007}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D9087
Summary: Ref T2683. This field is //almost// entirely redundant with `symbolicCommit`. Improve how some of the diff query stuff works a bit, then remove it.
Test Plan: Browsed around in all interfaces, looked at a bunch of diffs, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9099
Summary:
Ref T2683. Currently, DiffusionRequest has four different "commitey" things:
- `commit`
- `rawCommit`
- `symbolicCommit`
- `stableCommit`
Of these, only two are actually distinct, useful values: `symbolicCommit` (which holds the value the request originally contained, if one existed) and `stableCommit` (which resolves that value, or the value implied by its omission, into a stable, permanent commit identifier).
- `rawCommit` is equivalent to `symbolicCommit` and can be simply removed.
- `commit` has some sketchy magic around it that needs to be pulled out before it can be jettisoned.
Test Plan: Viewed SVN, Git, and Mercurial repositories. Viewed brwose/history/change/tag/branch/etc views.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9098
Summary: Ref T2683. Throw a more tailored exception to allow callers to distinguish between bad refs (which are expected, if users try to visit garbage branches) and other types of errors.
Test Plan: Tried to view branch "alksndfklansdf". Viewed branch "master".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9094
Summary:
Ref T2683. The old name was a bit confusing because it meant "the type of the thing the symbol represents": a "commit type" should logically always be "commit".
(Currently, this is only used to detect when we're looking at a tag.)
Test Plan: Looked at a tag. Looked at some other non-tag things. Browsed around, `grep`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9092
Summary:
Ref T2683. This is closely related to "symbolicCommit", but has an inconsistent "name" on the end.
Also, `diffusion.searchquery` uses this parameter inconsistently.
Test Plan:
- `grep`ed for callsites.
- Ran searches in Git and Mercurial repositories.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9091
Summary: Ref T4327. At some point these two very similar classes got introduced. Collapse `DiffusionBranchInformation` into the nearly identical `DiffusionRepositoryRef`, which enjoys slightly more generality and support.
Test Plan: Viewed branch overview and detail pages. Ran `repository refs` and `repository discover`. Grepped for removed symbols.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8002
Summary: This is a little funky but fixes an issue with Git repos that are
non-bare needing "origin/" to resolve branches other than "master". Eventually
this should get cleaned up.
Test Plan: Reporting user verified this fixed their issue.
Auditors: btrahan
Summary:
Ref T1493. Diffusion has some garbagey behavior for things we can't resolve. Common cases are:
- Looking at a branch that doesn't exist.
- Looking at a repository with no branches.
- Looking at a commit that doesn't exist.
- Looking at an empty repository.
In these cases, we generally fatal unhelpfully. I want to untangle this mess.
This doesn't help much, but does clean things up a bit. We currently have two separate query paths, "stablecommitname" and "expandshortcommit". These are pretty much doing the same thing -- taking some ref like "master" or "default" or a tag name or part of a commit name, and turning it into a full commit name. Merge them into a single "resolverefs" method.
This simplifies the code a fair bit, and gives us better error messages. They still aren't great, but they're like this now:
Ref "7498aec194ecf2d333e0e2baddd9d5cdf922d7f1" is ambiguous or does not exist.
...instead of just:
ERR-INVALID-COMMIT
Test Plan: Looked at Git, Mercurial and Subversion repositories that were empty and non-empty. Looked at branches/heads. Tried to look at invalid commits. Looked at tags. All of this still works, and some behaviors are a bit better than they used to be.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1493
Differential Revision: https://secure.phabricator.com/D7484
Summary: Fixes the junk I broke in D7484. Before that, tag content was a side effect of resolving the ref name. Now, fetch it explicitly in `diffusion.tagsquery`.
Test Plan: Looked at a tag, saw the annotation/message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7485
Summary: If you load Diffusion between a repository being pulled and discovered, you can end up with a valid commit reference that hasn't been discovered yet. Don't fatal.
Test Plan: Saw somewhat-helpful error page instead of fatal.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7448
Summary: Fixes T4035. I removed these two "remote/" things in rP59922b7, but we need them for non-bare repositories. Without them, the commands work and run fine and the output looks OK, but the results may not reflect the correct information (e.g., the log shows the working copy's master, which may not be in the same state as origin/master). I'm going to generally clean this up, but unbreak it for now.
Test Plan: Viewed bare and non-bare repositories in Diffusion, got accurate history.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T4035
Differential Revision: https://secure.phabricator.com/D7445
Summary:
This doesn't really impact anything very much, but is a little cleaner than cloning repositories with a working copy. It's somewhat important for allowing pushes, because you can't push to a checked-out branch.
Mercurial has a similar option (`--noupdate`) but leave that alone for now.
The origin stuff was mostly for sanity/explicitness purposes -- I believe it's safe to remove in all non-ridiculous cases. Git fails with it in bare repositories (it automatically creates an `origin`, but doesn't create the local refs for it, or something).
Test Plan: Nuked a repo, re-cloned it, pulled and updated it several times. Browsed both bare and non-bare repos in Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7430
Summary:
Ref T603. I got most of this earlier, but finish it up.
- Make a couple of controllers public; pretty much everything in Diffusion has implicit policy checks as a result of building a `DiffusionRequest`.
- Add an "Edit" capability to commits.
- Swap out the comment thing for commits.
- Disable actions if the user can't take them.
Test Plan: Viewed a bunch of interfaces while logged out, got appropriate results or roadblocks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7152
Summary:
"Branch" really means "repository main screen, with some branch selected", so a branch isn't actually required since we can just take you to the default.
Fixes an issue where new crumbs would throw an exception in SVN repositories.
Test Plan: Browed an SVN repo.
Reviewers: btrahan, mbishopim3
Reviewed By: mbishopim3
CC: aran
Differential Revision: https://secure.phabricator.com/D7099
Summary:
- Kicks it out to full width.
- More useful header/crumbs/properties/actions (needs some more work).
- Works for public repositories.
- Fix a bug where the "rX" crumb would lose the branch you're on.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7063
Summary: Ref T603. Make common repository queries (in Conduit and DiffusionRequest) policy-aware. These tend to get caugh by something else anyway, but tighten them up.
Test Plan: The conduit change already provided `user` everywhere. I verified that and browsed some pages.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7060
Summary: Ref T2784. This is a lower-level one from drequest so it gets the conditional initialization treatment. Consolidated SVN as well even though SVN is issuing database queries; I felt better about the code de-duplication despite the small performance hit when we could just query the DB directly in the SVN case.
Test Plan: browsed around my Phabricator repositories in Mercurial, Git, and SVN flavors. Looked good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5956
Summary: Ref T2784. This one was a wee bit complicated. Had to add PhabricatorUser and concept of initFromConduit (or not) to DiffusionRequest.
Test Plan: foreach repo, visited CALLSIGN and clicked a commit and verified they laoded correctly. Hacked code to hit NOT via Conduit and repeated tests to great success.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5928
Summary: Show a more specific exception when the local clone cannot be read because of permission issues.
Test Plan: Create a repository in an unreadable location and check for the right exception.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2368
Differential Revision: https://secure.phabricator.com/D4868
Summary:
If you go to `/rXnnnn` in Git, we expand the hash. If you go to `/rXnnnn` in Mercurial, we give you a confusing error message.
Reconcile Mercurial behavior with Git. Fixes T2265.
Test Plan: Viewed partial hash, full hash commit in Diffusion. Viewed very short hash, got reasonable behaviors.
Reviewers: btrahan, tido
Reviewed By: tido
CC: aran
Maniphest Tasks: T2265
Differential Revision: https://secure.phabricator.com/D4330
Summary: Also remove some columns.
Test Plan: Looked at SVN dir in Diffusion.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, vsuba
Maniphest Tasks: T2038
Differential Revision: https://secure.phabricator.com/D3949
Test Plan:
/diffusion/ARC/lint/master/src/, clicked on count link.
/diffusion/ARC/browse/master/src/difference/?lint=XHP9, clicked on file name.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=, verified that all messages are displayed.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=XHP9.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=TXT3, verified that 0 messages are displayed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3929
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary:
We do this in differential. To do this in diffusion, we need to know the
arcanist project (which I do by loading all possible projects for the
repository) and the language.
Test Plan:
load a php file in diffusion to see crossreferences; load a text file and
check darkconsole that it didn't try to crossreference.
Reviewers: epriestley, vrana, jungejason
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3020
Summary:
- Old versions of Mercurial give different output for `hg log -- ''` and `hg log`. Just use `hg log`.
- Branch names with spaces can't be specified in `--rev`. I talked with hstuart in #mercurial and apparently am not crazy.
Test Plan:
- Viewed history of a repository.
- Viewed history of a file.
- Viewed branch `m m m m m 2:ffffffffffff (inactive)`
- Learned that you checkout this branch with `hg checkout ':m m m m m 2:ffffffffffff (inactive)'`
Reviewers: dschleimer, btrahan
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1268
Differential Revision: https://secure.phabricator.com/D2950
Summary: Currently, Diffusion supports highlighting of line ranges passed in the URI. It would be helpful to be able to highlight multiple line ranges.
Test Plan: Accessed directly via URL in my sandbox. Seems to work. I'm not sure what other components use this functionality, but this change should be backwards compatible.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D2921
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary:
we were parsing the git log output slightly incorrectly and over-exploding on spaces. we also needed to escape the path %20 stuff`.
Not sure if there's something fancy to do given folks should reparse their repos if they are impacted by this issue.
Test Plan:
made a directory with spaces and some dummy revisions. observed diffs wouldn't load and links broken.
with patch, ran scripts/reparse.php for pertinent revisions and diffs loaded and links weren't broken.
Reviewers: floatinglomas, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1252
Differential Revision: https://secure.phabricator.com/D2510
Summary: If you have an empty value saved in the "default branch" field, we default to empty string (or null, or whatever) instead of the correct default.
Test Plan:
- Looked at a Git repo with an empty default branch, got a default to "master" instead of an error.
- Looked at a Mercurial repo with an empty default branch, got a default to "default" instead of an error.
Reviewers: btrahan, csilvers
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1237
Differential Revision: https://secure.phabricator.com/D2512
Summary:
Sorting by last commit date
Branch view limit to 25 branches
All branches table page with pagination on Git
Test Plan:
* Check repository view for expected behavior on branch table view
* Check all branches page & test pagination
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1200
Differential Revision: https://secure.phabricator.com/D2442
Summary:
- When you have an un-cloned repository, we currently throw random-looking Git/Hg exception. Instead, throw a useful error.
- When you have a cloned but undiscovered repository, we show no commits. This is crazy confusing. Instead, show commits as "importing...".
- Fix some warnings and errors for empty path table cases, etc.
Test Plan:
- Wiped database.
- Added Mercurial repo without running daemons. Viewed in Diffusion, got a good exception.
- Pulled Mercurial repo without discovering it. Got "Importing...".
- Discovered Mercurial repo without parsing it. Got "Importing..." plus date information.
- Parsed Mercurial repo, got everything working properly.
- Added Git repo without running daemons, did all the stuff above, same results.
- This doesn't improve SVN much but that's a trickier case since we don't actually make SVN calls and rely only on the parse state.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T776
Differential Revision: https://secure.phabricator.com/D2439
Summary: We now allow symbolic commits, so let them through the pipeline.
Test Plan: {F10571}
Reviewers: davidreuss, btrahan, vrana
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2315
Summary:
- When viewing a commit, show its tags.
- For commits with many tags, show a list of all tags on the tag list interface.
- Improve some handling of symbolic references.
- When tags contain content, show it on the browse view reached by clicking the tag name.
Test Plan: Looked at commits with and without tags, clicked "More tags...", clicked tag names.
Reviewers: btrahan, vrana, davidreuss, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2290
