Summary: Searched for `AphrontFormView` and then for `appendChild()`.
Test Plan: /login/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4855
Summary: Done by searching for `AphrontErrorView` and then `appendChild()`.
Test Plan:
Looked at Commit Detail.
Looked at Revision Detail.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4843
Summary:
I wasn't able to reproduce the "recursion detected" in real web request but I saw lots of 1073741824 refcounts in `debug_zval_dump()` of $object.
I'm not sure how that happens.
Test Plan: D4807#4
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4839
Summary:
Convert a final few `render_tag()` calls to `tag()` calls. This leaves us with 36 calls:
- 9 are in Conpherence, and will be converted after merging. Using Views makes the most sense here, to get access to renderHTMLView() and such.
- 2 are the definition and its library map entry.
- 3 are in the documentation.
- I believe the remaining 22 are too difficult to convert pre-merge. About half are in code which is slated for destruction; the other half are in the base implementations of very common classes (like PhabricatorStandardPageView) and can only be converted by converting the entire codebase.
My plan at this point is:
- Test the branch thoroughly.
- Merge to master.
- Over time, resolve the remaining issues: lint means things shouldn't get any worse.
Test Plan: See inlines.
Reviewers: vrana
Reviewed By: vrana
CC: aran, edward
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4802
Summary:
The other `true` is correct (and I think we can fix the scaling issues) but this one should be an indirect change. This prevents the branch from appearing in the history of every file.
(I misread this code and gave @vrana some bad advice originally. This is //actually// consistent with Mercurial and Git.)
Test Plan: Partial revert. I'll make this stuff testable.
Reviewers: nh, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4742
Summary:
- Implements `javelin_tag()`, which is `javelin_render_tag()` on top of `phutil_tag()` instead of `phutil_render_tag()`.
- Manually converts all or almost all of the trivial callsites.
Test Plan:
- Site does not seem any more broken than before.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4639
Summary:
- Grepped for phutil_render_tag().
- Fixed some easy ones.
Test Plan:
- Browsed around; site didn't seem more broken than it was before.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4638
Summary:
Created with spatch:
lang=diff
- phutil_render_tag
+ phutil_tag
(X, Y, '...')
Then searched for `&` and `<` in the output and replaced them.
Test Plan: Loaded homepage.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4503
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: Ref T2298. This seems like the least complicated reasonable order to implement.
Test Plan: Looked at repositories, saw them ordered by name.
Reviewers: vrana, btrahan, brennantaylor
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2298
Differential Revision: https://secure.phabricator.com/D4395
Summary: Simple change to stop exceptions from being thrown based on the diff in D4146.
Test Plan: arc call-conduit repository.create
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: vrana, aran, Korvin
Maniphest Tasks: T2268
Differential Revision: https://secure.phabricator.com/D4342
Test Plan: HHVM currently core dumps so I didn't test it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4343
Summary:
The search indexing API has several problems right now:
- Always runs in-process.
- It would be nice to push this into the task queue for performance. However, the API currently passses an object all the way through (and some indexers depend on preloaded object attributes), so it can't be dumped into the task queue at any stage since we can't serialize it.
- Being able to use the task queue will also make rebuilding indexes faster.
- Instead, make the API phid-oriented.
- No uniform indexing API.
- Each "Editor" currently calls SomeCustomIndexer::indexThing(). This won't work with AbstractTransactions. The API is also just weird.
- Instead, provide a uniform API.
- No uniform CLI.
- We have `scripts/search/reindex_everything.php`, but it doesn't actually index everything. Each new document type needs to be separately added to it, leading to stuff like D3839. Third-party applications can't provide indexers.
- Instead, let indexers expose documents for indexing.
- Not application-oriented.
- All the indexers live in search/ right now, which isn't the right organization in an application-orietned view of the world.
- Instead, move indexers to applications and load them with SymbolLoader.
Test Plan:
- `bin/search index`
- Indexed one revision, one task.
- Indexed `--type TASK`, `--type DREV`, etc., for all types.
- Indexed `--all`.
- Added the word "saboteur" to a revision, task, wiki page, and question and then searched for it.
- Creating users is a pain; searched for a user after indexing.
- Creating commits is a pain; searched for a commit after indexing.
- Mocks aren't currently loadable in the result view, so their indexing is moot.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: 20after4, aran
Maniphest Tasks: T1991, T2104
Differential Revision: https://secure.phabricator.com/D4261
Summary:
Because of the way PhabricatorOwnersPackage works, the code to save package
changes when parsing commit changes was raising a few undefined index errors,
and was throwing an exception trying to call a method on null (because not
all of the phids related to the package had their handles loaded). This fix
doesn't load the missing handles, it just avoids trying to use them.
Test Plan:
./scripts/repository/reparse.php on a commit with path changes that triggered
a package change
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4236
Summary:
Load the data for daemon worker tasks when viewing them, and present
the information in a useful way. This defaults to printing the json data,
but for some classes of worker it will also link to the corresponding
object, to make debugging problems with workers easier.
Test Plan:
load /daemon/task/NNN for a CommitParserWorker and a MetaMTAWorker, and
see the addition of a data field with useful content and link.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4226
Summary:
This adds a configuration option for repositories to be marked as managed
by phabricator, which is then used by the pullLocal daemon to try to recover
from some errors it runs into.
Test Plan: Set a bogus uri for a repo, saw that the pullLocal daemon fixed it.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3680
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: we were catching a specific exception; just catch all exceptions
Test Plan: viewed repository tool home page
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2155
Differential Revision: https://secure.phabricator.com/D4118
Summary:
Issues here:
- Need an application-sized "eye", or a "home" icon for "Phabricator Home".
- Some of the "apps_lb_2x" sliced images are the "_dark_" versions, not the light versions.
- If you slice an application-sized "logout" (power off) icon and application-sized "help" (questionmark in circle) icon I can replace the current menu icons and nearly get rid of "autosprite".
- To replace the icons on /applications/, the non-retina size is "4x", so we'd need "8x" for retina. Alternatively I can reduce the icon sizes by 50%.
- The "Help", "Settings" and "Logout" items currently have a "glowing" hover state, which needs a variant (or we can drop it).
- The /applications/ icons have a white hover state (or we can drop it).
- The 1x application (14x14) icons aren't used anywhere right now, should they be? Maybe in the feed in the future, etc?
- The "apps-2x" and "apps-large" sheets are the same image, but getting them to actually use the same file is a bit tricky, so I just left them separate for now.
Test Plan:
{F26698}
{F26699}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4108
Summary:
AphrontSideNavView is an old class which required you to do a lot of work; it was obsoleted by AphrontSideNavFilterView. Remove all direct callsites so I can clean it up.
This is a precursor to letting me render a filter menu as a dropdown menu for T1960.
Test Plan:
Examined each interface for correct filter construction and selection:
- Browsed Diffusion
- Browsed Differential
- Browsed Files
- Browsed Slowvote
- Browsed Phriction
- Browsed repo edit interface
Grepped for `AphrontSideNavView`. The only remaining instances are in `AphrontSideNavView` itself and `AphrontSideNavFilterView` (which currently uses it).
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4034
Summary: See D3977, a terrible diff where I made a huge messs.
Test Plan: Ran `reparse.php --message --trace <revision>`, verified correct identification of author.
Reviewers: edward, vrana
Reviewed By: edward
CC: aran
Differential Revision: https://secure.phabricator.com/D4104
Summary: See discussion in T1544. This has been obsoleted by simpler/better mechanisms.
Test Plan: Edited a repository; ran a parse task.
Reviewers: edward
Reviewed By: edward
CC: aran
Maniphest Tasks: T1544
Differential Revision: https://secure.phabricator.com/D3977
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:
Currently, when taskmasters complete a task it is immediately deleted. This prevents us from doing some general things, like:
- Supporting the idea of permanent failure (e.g., after N failures just stop trying).
- Showing the user how fast taskmasters are completing tasks.
- Showing the user how long tasks took to complete.
Having better visibility into this is important to Drydock, which builds on the task system. Also, generally buff debug output for task execution.
Test Plan: Ran `bin/phd debug taskmaster`. Ran `bin/phd debug garbage`. Queued some tasks via various systems.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3852
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: When we receive an email, figure out if any of the other tos and ccs are users. If they are, pass their phids through the stach as "exclude phids" and exclude them from getting the email.
Test Plan: used the various applications (audit, differential, maniphest) and noted emails were sent as expected.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin, vrana
Maniphest Tasks: T1676
Differential Revision: https://secure.phabricator.com/D3645
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:
D03646 works, I don't want it to work.
Theoretically, it can cause us some troubles if we use this string in JS number context where 030 is 24.
Test Plan: D03646, D3646
Reviewers: epriestley, edward
Reviewed By: edward
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3646
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
Summary:
D3555 stopped multiple commenting but it still attaches multiple diffs.
Save earlier to stop doing unnecessary work.
Test Plan: Reparsed the same commit twice at the same time.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3598
Summary: So they're maybe a little easier to deal with? I'm going to take this formally to "plz @chad plz help" land.
Test Plan: {F20329}
Reviewers: btrahan, vrana, chad
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3609
Summary:
Replace executing 'ps aux' with usage of available api to
control daemons PhabricatorDaemonControl.
This fixes isPullDaemonRunningOnThisMachine returning wrong status
under Fedora & lighttpd & SELinux because SELinux with default
settings blocked getting all processes in 'ps aux'.
Test Plan: start stop daemon and check repository app
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3557
Summary:
If attaching a commit or checking if there are any changes takes nonzero time then the revision may be closed by someone else.
Cleaner solution would be to do it inside a transaction and mark the SELECT as FOR UPDATE but it would be blocking.
Test Plan: Patched `$should_close` to be true, reparsed an already closed commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3555
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:
In the commit message parser worker, it tries to get relationships for a
revision without first loading them. (D3444 introduced the change.) This
is where the get happens, so I'm adding the load here - maybe it should be
added in the CommitMessageParserWorker instead?
Test Plan: Made this change, and my taskmaster daemons stopped failing.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3448
Summary:
This is mostly ripped from D2721, but doesn't implement the T945 part.
After we parse a commit message, give DifferentialFieldSpecifications an opportunity to react to the message as well (e.g., by updating related objects).
Test Plan: Impelmented a var_dump() body, ran `reparse.php` on a commit.
Reviewers: vrana, 20after4, btrahan, edward
Reviewed By: 20after4
CC: aran
Maniphest Tasks: T945, T1544
Differential Revision: https://secure.phabricator.com/D3444
Summary: If the commit has a known author but that author is different from the revision author or the revision doesn't exist, we'll throw away the commit author and then raise an audit for "Owners Not Involved". Instead, we should just use the commit author in all cases.
Test Plan:
Debugged this with Zor in IRC, he reported it fixed his issue.
Before:
http://pastie.org/4574680
("Author" is empty.)
After:
http://pastie.org/4574712
("Author" is correctly recognized.)
Reviewers: jungejason, nh, vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3374
Summary:
Adds a flexible navigation menu to diffs that shows you your current position in the diff.
Anticipating some "this is the best thing ever" and some "this is the wosrt thing ever" on this, but let's see how much pushback we get? It seems pretty good to me.
Test Plan: Will attach screenshots.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1633, T1591
Differential Revision: https://secure.phabricator.com/D3355
Summary: We should probably do this also in `differential.creatediff` but it's not a big deal because we later call `differential.updaterevision` which does this using `DifferentialRevisionEditor`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3263
Summary: See D3252.
Test Plan: This one is nasty to test, I'm going to make some coffee first.
Reviewers: davidreuss, vrana, btrahan
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T452
Differential Revision: https://secure.phabricator.com/D3254
Summary:
I ran into a case where a commit isn't "new" but hasn't been closed. I
think the check on the status of the differential revision should be
enough and this check isn't needed.
Test Plan:
used the reparse.php script to close a revision that previously wouldn't
close.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3232
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
Summary: If a change copies some file `A` to `B` and also edits `A`, we currently record this as an indirect change and don't show the edits to `A` in the diff. Instead, record these as direct changes.
Test Plan: Created two commits, one which copied `A` to `B` without modifying `A` and one which copied `A` to `B` and modified A. Viewed both commits in Diffusion. The unmodified commit did not show `A`, and the modified commit did (with the correct changes).
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: champo, aran
Differential Revision: https://secure.phabricator.com/D3120
Summary:
We have some false positives on commit changes checker.
I'm not sure if the reason is a difference between `git diff` and `svn diff` or something else but making this more robust doesn't harm anything.
We couldn't make the files from the whole changeset because I want to ignore context bigger than `$num_lines` to reduce rebase noise.
Test Plan:
Ran the method on diff which had false positive previously.
Ran the method on a diff with real change.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3107
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:
Currently, we have this cumbersome `PhabricatorRepositoryCommitMessageDetailParser` hook. This is really old and outdated; I want to just use the Differential custom field parser. See T945 for a specific application.
However, it allows installs to override author/committer association. Instead, provide an event hook for doing this.
Test Plan: Added a listener, made every commit resolve to "turtle", parsed some commits, verified the events looked sane and they now correctly were all attributed to "turtle".
Reviewers: btrahan, vrana, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1337
Differential Revision: https://secure.phabricator.com/D3040
Summary: We currently omit email from Git author/committer lookups, which gives us some bad results when identify commit authors. Include email. Also simplify this block a little bit.
Test Plan: Ran "reparse.php --message" on several commits, verified that the author/committer seemed reasonable with var_dump()s.
Reviewers: btrahan, vrana, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1337
Differential Revision: https://secure.phabricator.com/D3039
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: This is a minor quality-of-life improvement to prevent D2968 from being as nasty as it is.
Test Plan: Ran unit tests; generated Differential, Maniphest and Diffusion emails and verified the bodies looked sensible.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T931
Differential Revision: https://secure.phabricator.com/D2986
Summary:
See D818 for an older attempt at this. Support code has matured to the point where the patch is pretty straightforward.
@tido, this was a long-standing request from Aditya back in the day.
Test Plan: Used `reparse.php --herald` to send myself a bunch of emails with various patch configurations. Confirmed that limits are respected, reasonable errors arise when they're violated, etc. (Timeout is a little funky but that's out of scope here, I think.)
Reviewers: btrahan
Reviewed By: btrahan
CC: tido, aran
Maniphest Tasks: T456
Differential Revision: https://secure.phabricator.com/D2967
Summary:
The locks held by read-only pullLocal daemons were causing our discovery instance
to not get the lock and fail at discovery. We don't need to hold the lock while
pulling (only while discovering), so this moves the lock to the appropriate place.
Test Plan: tested in production
Reviewers: jungejason, epriestley, vrana
Reviewed By: jungejason
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2890
Summary:
Even though `--encoding` is passed to the command, git still fails
in some cases to correctly convert the output. Attempt the conversion
ourselves if it's non UTF-8.
Test Plan: Reparsed message in a repository with ISO-8859-1 encoded commit messages.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T452
Differential Revision: https://secure.phabricator.com/D2888
Summary:
We have a race condition right now, where we may insert a commit without `seenOnBranches`. This means shouldAutocloseCommit will return false (since it's not on any autoclose branches) if the message parser runs fast enough, causing it to associate the commit but not close the revision. This happened for D2851.
Also prompt the user to repair broken repositories.
Test Plan: Ran discovery / repair. Ran discovery on new commits. Verified 'seenOnBranches' value. Deleted some data, verified "repair" error. Repaired repository.
Reviewers: jungejason, nh, vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2858
Summary: Allow multiple daemons to run without contention.
Test Plan: Ran multiple daemons simultaneously in "debug" mode, observed them acquiring (and sometimes failing to acquire) locks.
Reviewers: btrahan, jungejason, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1400
Differential Revision: https://secure.phabricator.com/D2877
Summary:
When the absolute path is used for load file (loadFileContent(()), it fails in git. For example:
/var/repo/page_admin_app
> git cat-file blob '4d6c03923006d6c444660f2c734fe03e10fd20bd':'/ios/PageAdminApp/Resources/splash/De fault-Portrait@2x~ipad.png'
fatal: Not a valid object name 4d6c03923006d6c444660f2c734fe03e10fd20bd:/ios/PageAdminApp/Resources/s plash/Default-Portrait@2x~ipad.png
This is breaking the auto-closing for about 8 revisions like
https://phabricator.fb.com/rPPA4d6c03923006d6c444660f2c734fe03e10fd20bd ...
https://phabricatorcator.fb.com/rPPA51acb7e482aab0c491b530ed19dddc741d50f673 ...
Test Plan:
- reparsed https://phabricator.fb.com/rPPA4d6c03923006d6c444660f2c734fe03e10fd20bd successfully with corresponding differential revision being closed.
- verified that without leading '/', loadFileContent for svn still works. Both of the following commands worked (note the double '/' right before 'tfb':
svn cat svn+ssh://svn.vip.facebook.com/svnroot//tfb/trunk/www/flib/intern/cachearchiver/regenerators/wurfl/CacheArchiveWurflRegenerator.php@579700
svn cat svn+ssh://svn.vip.facebook.com/svnroot/tfb/trunk/www/flib/intern/cachearchiver/regenerators/wurfl/CacheArchiveWurflRegenerator.php@579700
Reviewers: vrana
Reviewed By: vrana
CC: nh, aran, epriestley
Differential Revision: https://secure.phabricator.com/D2847
Summary:
Improve performance of large discovery tasks in Git by using subprocess streaming, like we do for Mercurial.
Basically, we save the cost of running many `git log` commands by running one big `git log` command but only parsing as much of it as we need to. This is pretty complicated, but we more or less need it for mercurial (which has ~100ms of 'hg' overhead instead of ~5ms of 'git' overhead) so we're already committed to most of the complexity costs. The git implementation is much simpler than the hg implementation because we don't need to handle all the weird parent rules (git gives us to them easily).
Test Plan:
Before, `discover --repair` on Phabricator took 35s:
real 0m35.324s
user 0m13.364s
sys 0m21.088s
Now 7s:
real 0m7.236s
user 0m2.436s
sys 0m3.444s
Note that most of the time is spent inserting rows after discover, the actual speedup of the git discovery part is much larger (subjectively, it runs in less than a second now, from ~28 seconds before).
Also ran discover/pull on single new commits in normal cases to verify that nothing broke in the common case.
Reviewers: jungejason, nh, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1401
Differential Revision: https://secure.phabricator.com/D2851
Summary:
If a repository is missing commits because they mysteriously vanished, there's no reasonable way to get them back right now. Provide a way to ignore the state in the database and rediscover the entire repository unconditionally.
We don't queue any reparses or anything, but when I move reparse into this script we can hook things up or something. This generally shouldn't be too important anyway.
Test Plan: Ran `repository discover --repair` on Phabricator.
Reviewers: jungejason, nh, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2850
Summary:
Nothing new or exciting here yet, just moving the random scripts/repositories/ things to bin/repository. Also add `repository list`.
(Console stuff comes from D2841.)
Test Plan: Ran `repository list`, `repository pull`, `repository discover`, `repository discover --verbose`, `repository help`.
Reviewers: jungejason, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2849
Summary: Add verbose logging. This logging is activated by setting "phd.verbose" in the config, running "phd debug", or explicitly in scripts/repository/pull.php and scripst/repository/discover.php
Test Plan:
>>> orbital ~/devtools/phabricator $ ./scripts/repository/discover.php GTEST
Discovering 'GTEST'...
<VERB> PhabricatorRepositoryPullLocalDaemon Discovering commits in repository 'GTEST'...
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch '()_+abcd$100', at a37bc285a12efa7224fe19f3df54cd90fa2b897a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch '+abcd$100', at a37bc285a12efa7224fe19f3df54cd90fa2b897a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch '_+abcd$100', at a37bc285a12efa7224fe19f3df54cd90fa2b897a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch 'abcd$100', at a37bc285a12efa7224fe19f3df54cd90fa2b897a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch 'arcpatch', at 774c7737b2d560a291697126bf4513204ccf661a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch 'arcpatch-1', at dc97539bee07293f95990d71f4638335a2531d69.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch 'arcpatch-2', at 1acfaec313c46dd3caa90448800181fb91b0270f.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2843
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: it's calling pull daemon's failure. This is actually Nick's fix.
Test Plan: Nick already manually ran it on daemon machine.
Reviewers: vrana, nh
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2828
Summary:
The autoclose logic is currently doing a little too much work. We want to parse each commit at most twice:
# When it first appears in the repository.
# When it first appears on an autoclose branch.
These two events might not be distinct (i.e., it might first appear on an autoclose branch).
Currently, to discover commits initially appearing on autoclose branches, we check each branch, determine if it's an autoclose branch or not, and determine if the HEAD is already a known commit on an autoclose branch. This is correct so far, and allows us to ignore branches which either haven't changed or have commits at HEAD which we've already examined.
However, if an autoclose branch has a new commit, we start working backward through it. Prior to this patch, we only stop when we hit commits that we've already discovered lie on this branch. If the branch is new, none of the commits will be discovered on it (they're discovered in general, and likely discovered on other autoclose branches, but not discovered on this branch), so we'll parse all the way back to the root.
Instead, we want to stop when we hit commits that we've already discovered on //any// autoclose branch.
So do that.
Test Plan: Pushed a new branch, then pushed a new commit at HEAD. Ran discovery, verified we rediscovered only 1 commit, not every commit in history.
Reviewers: vrana, jungejason, aurelijus
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1389
Differential Revision: https://secure.phabricator.com/D2798
