Test Plan:
Enable the checkbox under the Tracking options for a repository.
View the repository in Diffusion, the username should show.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2575
Differential Revision: https://secure.phabricator.com/D5269
Summary:
Move Diffusion to be hovercard-ready, and expand our ability to resolve commit references.
- Link unqualified hashes of 7 characters or more which match a commit.
- Link qualified hashes of 5 characters or more which match a commit.
- Support `{...}` syntax.
Test Plan: {F33896}
Reviewers: chad, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5121
Summary:
Fixes T2563. Instead of rendering "rPnnnnnn", render "rPnnnnnn: add feature X". Tweak Audit tables to accommodate.
@vrana / @nh, this migration might take a while. You could safely skip it when deploying and then run it after deployment.
I think I fixed all the other places where these render, but might have missed something.
Test Plan:
- Ran first schema migration, clicked around to make sure nothing broke.
- Ran `scripts/repository/reparse.php --message rXyyyyy`, verified summary populated.
- Ran second migration.
- Checked task/diffusion/audit/differential for weird rendering.
Reviewers: vrana
Reviewed By: vrana
CC: nh, aran, chrisbolt, allixsenos
Maniphest Tasks: T2563
Differential Revision: https://secure.phabricator.com/D5012
Summary: We currently try to delete symbols by ID, but the table has a multipart primary key and no `id` column.
Test Plan: Ran query locally; had @JThramer verify fix in his environment.
Reviewers: btrahan
Reviewed By: btrahan
CC: JThramer, aran
Differential Revision: https://secure.phabricator.com/D4626
Summary: This is to reduce number of calls from Arcanist.
Test Plan: Called it from web interface.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4146
Summary: This saves lint errors to the path change of current commit. It requires pushed revision. It doesn't save difference from previous commit mentioned in T2038#comment-4 - I don't plan doing it after all, everything would be much more complicated and the amount of data saved with this approach isn't that bad.
Test Plan: Applied patch, ran script, verified DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2038
Differential Revision: https://secure.phabricator.com/D3899
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 by passing the "seenOnBranches" commit data detail through the stack
Test Plan: browse in diffusion link worked for non-master checkins under git
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1949
Differential Revision: https://secure.phabricator.com/D3853
Summary: fancy title. really just make the delete() method aware of related objects and build a quick workflow which calls delete(). also make commit delete savvy about audit requests.
Test Plan: deleted a repository per the instructions given to me in the web UI
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1416, T1958, T1372
Differential Revision: https://secure.phabricator.com/D3822
Summary: For immutable objects, just use the ID as a cursor.
Test Plan:
- Analyzed commits from an empty cursor.
- Checked that cursor was good.
- Pulled some more commits.
- Analyzed commits again, verified it only hit the new ones.
- Verified the graph of "Count of CMIT" looked reasonable.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1866
Differential Revision: https://secure.phabricator.com/D3656
Summary: tricky part is purging symbols at that time too. override "delete" method to get there, use transactions, etc.
Test Plan: deleted an arcanist project - it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, mbishopim3
Maniphest Tasks: T1738
Differential Revision: https://secure.phabricator.com/D3613
Test Plan: Created diff, opened the file from Differential, opened the file in Diffusion.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3538
Summary:
See T1602.
This is just the minimal functional patch; the scripts will continue
working because of the `DEFAULT ''`.
Test Plan:
Can't fully test this until I get more code working, but
nothing broke horribly yet.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3147
After this commit: d9296638cd
I started getting this error:
Unhandled Exception ("Exception")
Bad getter call: getURIObject
It turns out that getURIObject just needed to be getRemoteURIObject and then the problem goes away.
Summary:
In D3063, we stopped converting "user@domain:path" git-style URIs, but this broke the SSH-detection code and I missed that in my test plan because my test case uses natural SSH keys so the omission of SSH flags didn't cause failures.
This code is a bit of a mess anyway. Consolidate and refactor it to be a bit simpler, and add test coverage.
Test Plan: Ran unit tests. Ran "test_connection.php" in SSH and non-SSH modes, verified SSH modes generated appropriate ssh-agent commands around the git remote commands.
Reviewers: vrana, btrahan, tberman
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1529
Differential Revision: https://secure.phabricator.com/D3093
Summary:
See D3033, T1529. We currently transform "scp-style" `user@host:path` URIs into normal `ssh://user@host/path` URIs. This is undesirable for two reasons:
- The paths aren't always equivalent. They are for GitHub, which is why I missed this originally, but in the general case the ":path" is resolved relatively and the "/path" is resolved absolutely. So this transformation can break things.
- It confuses users, who do not think of "git@host:path" URIs as SSH URIs even though the SSH protocol is implied.
So stop using them, and just use the "git@host:path" URIs instead. This is a bit messy since we have some validation built up on top of URIs. Hopefully we can get rid of more of this in the future as we simplify repository management.
Test Plan: Unit tests cover this stuff pretty well. Made a new git repository with a "git@host:path" style URI and did pull/discover on it, verified the right URI was used.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1529
Differential Revision: https://secure.phabricator.com/D3036
Summary: in svn and hg (for now), no branch used.
Test Plan: will test live
Reviewers: epriestley
CC: nh, vrana, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2839
Summary: Implemented it how it was suggested in ticket comments
Test Plan: create a revision in a branch, push that branch up, verify it's visible in diffusion and also that revision is not closed, then merge and push to master, verify that revision closed
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, 20after4
Maniphest Tasks: T1210
Differential Revision: https://secure.phabricator.com/D2706
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: also makes the UI more general for this username + password business.
Test Plan:
- configure a phabricator repository from the svn server @asherwin provided which is configured for svn protocol with SASL
- observed phabricator failing without my patch
- upgraded my SVN client to support SASL (protip for mac users - http://www.wandisco.com/subversion/download#osx)
- applied patch to phabricator
- restarted daemons
- noted daemon success - diffusion populating nicely
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1260
Differential Revision: https://secure.phabricator.com/D2549
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:
- 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:
Allow the pull daemon to take a list of repositories. By default, pull all repositories.
Make some effort to respect pull frequencies, although we'll necessarily suffer a bit if running with only one process.
NOTE: We still launch one discovery daemon per working copy, so this only cuts the daemon count in half.
Test Plan:
- Ran `phd debug pulllocal`, verified behavior.
- Ran `pull.php P MTEST SVNTEST --trace`, verified it pulled the repos and ran the right commands.
- Ran `phd repository-launch-master`, verified the right daemons launched, checked daemon console.
- Ran `phd repository-launch-readonly`, verified the right daemon launched, checked daemon console.
Reviewers: btrahan, csilvers, davidreuss
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2418
Summary: Primarily for @csilvers who has 92 million repositories or something. This is a touch hacky, but movitated by pragmatism.
Test Plan:
- Ran "repository.create" to create repositories, "repository.query" to list them.
- Tested most or maybe all of the error conditions, probably.
Reviewers: btrahan, vrana, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2396
Summary: Lists the 25 most recent tags on the "Repository" page.
Test Plan: Looked at a git repository with a tag, saw it. Looked at HG/SVN repos, they didn't break.
Reviewers: davidreuss, 20after4, btrahan, vrana, jungejason
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2255
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2091
Summary: Apparently I spent like a good month copy/pasting slightly different versions of this logic all over the codebase.
Test Plan: Selected "View Options -> Browse in Diffusion" for a chagneset, got a URI with a branch name in it under Git.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1993
Summary: We need to build a request in order to pick up an appropriate default branch name, instead of using the raw static generator.
Test Plan: Clicked a symbol link, got /master/path/blahblah
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1982
Summary:
Diffusion request/uri handling is currently a big, hastily ported mess. In particular, it has:
- Tons and tons of duplicated code.
- Bugs with handling unusual branch and file names.
- An excessively large (and yet insufficiently expressive) API on DiffusionRequest, including a nonsensical concrete base class.
- Other tools were doing hacky things like passing ":" branch names.
This diff attempts to fix these issues.
- Make the base class abstract (it was concrete ONLY for "/diffusion/").
- Move all URI generation to DiffusionRequest. Make the core static. Add unit tests.
- Delete the 300 copies of URI generation code throughout Diffusion.
- Move all URI parsing to DiffusionRequest. Make the core static. Add unit tests.
- Add an appropriate static initializer for other callers.
- Convert all code calling `newFromAphrontRequestDictionary` outside of Diffusion to the new `newFromDictionary` API.
- Refactor static initializers to be sensibly-sized.
- Refactor derived DiffusionRequest classes to remove duplicated code.
- Properly encode branch names (fixes branches with "/", see <https://github.com/facebook/phabricator/issues/100>).
- Properly encode path names (fixes issues in D1742).
- Properly escape delimiter characters ";" and "$" in path names so files like "$100" are not interpreted as "line 100".
- Fix a couple warnings.
- Fix a couple lint issues.
- Fix a bug where we would not parse filenames with spaces in them correctly in the Git browse query.
- Fix a bug where Git change queries would fail unnecessarily.
- Provide or improve some documentation.
This thing is pretty gigantic but also kind of hard to split up. If it's unreasonably difficult to review, let me know and I can take a stab at it though.
This supplants D1742.
Test Plan:
- Used home, repository, branch, browse, change, history, diff (ajax), lastmodified (ajax) views of Diffusion.
- Used Owners typeaheads and search.
- Used diffusion.getrecentcommitsbypath method.
- Pushed a change to an absurdly-named file on an absurdly-named branch, everything worked properly.
{F9185}
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1921
Summary: Last of the big final patches. Left a few debatable classes (12 out of about 400) that I'll deal with individually eventually.
Test Plan: Ran testEverythingImplemented.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1881
Summary:
I'm on a host where I have the PhabricatorRepositoryPullLocalDaemons
tracking a remote repo. In my case, these end up as local git repos in
/var/repo/$name.
I'm working on another daemon that is going to automatically make
changes and commit them back upstream. I figured it would be best to do
this in a new local repo. I'll put these in /var/repo-clones/$name.
It's nice to use the exec*() functions in PhabricatorRepository, so the
approach I thought of was to load the PhabricatorRepository object from
the database, then change its localPath to point at the
/var/repo-clones/$name directory instead.
I didn't really want to change the local-path detail with setDetail(),
as that risks committing the change upstream. It's nice to use the
repo's execLocalCommand() methods though, hence wanting to change the
local path.
Test Plan: None yet.
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1821
Summary:
- Move table to Repository, since we have no Owners joins in the application anymore but would like to do a Repository join.
- Rename "packagePHID" to "auditorPHID", since this column may contain package, project, or user PHIDs.
Test Plan:
- Browsed Owners, Audit, and Differential interfaces to the Audit tool.
- Made comments and state changes.
- Ran "reparse.php --herald --owners" on several commits.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, nh, vrana
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1787
Summary:
- Add a proper mailKey field to make these things mailable. Backfill all
existing objects.
- Denormalize authorPHID to the commit object so we can query by it
efficiently in a future diff. We currently use the search engine to drive
"commits by author" but that's not so good for audit, which needs more
constraints.
- Add an overall audit status field so we can efficiently query "commits that
needs your attention".
- Add enough code to convince myself that these fields are basically
reasonable and work correctly.
Test Plan:
- Ran schema upgrades. Checked database state afterward.
- Ran "reparse.php --owners --herald" to verify worker changes.
- Looked at a commit, altered aggregate status via audits / reparse.php,
verified it responded correctly.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley, nh
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1706
Summary: See D1707 -- just in case the Phabricator server is configured
suspiciously.
Test Plan: Cursory inspection.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T922
Differential Revision: https://secure.phabricator.com/D1708
Summary:
When users submit an audit, send email to relevant parties informing them.
Allow email to be replied to. Just basic support so far; no "!raise" stuff and
no threading with the Herald commit notification.
Test Plan: Made comments, got email. Replied to email, got comments.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1698
Summary:
@rguerin ran into an issue in his install where Phabricator appears to have
discovered commits which no longer exist, and thus is failing to proceed with
its repository import.
It's not clear how we got into this state. Previously, it was possible by, e.g.,
parsing a different repository's working copy and then switching them back, but
there are now safeguards against that.
I'm taking a three-pronged approach to try to sort this out:
- Provide a script to get out of this state (this script) and reconcile
Phabricator's view of a repository with an authoritative copy of it. This
basically "un-discovers" any discovered commits which don't actually exist (any
queued tasks to parse them will fail permanently when they fail to load the
commit object).
- Add more logging to the discovery daemon so we can figure out where commits
came from.
- Improve Diffusion's UI when stuff is partially discovered (T776).
(This script should also clean up some nonsense on secure.phabricator.com from a
botched Diviner import.)
Test Plan: Ran "reconcile.php" with bogus commits and bogus differential/commit
links, had them expunged. Will work with @rguerin to see if this resolves
things.
Reviewers: btrahan, rguerin
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1552
Summary: @s reported an issue with implicit file:// URIs in Git, see P270.
Recognize and handle URIs in this format. For URIs we don't understand, raise an
exception.
Test Plan:
- Added failing tests.
- Fixed code.
- Tests pass.
Reviewers: btrahan, jungejason, s
Reviewed By: s
CC: aran, epriestley, s
Differential Revision: https://secure.phabricator.com/D1362
