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
