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
