1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-09-22 18:28:47 +02:00
Commit graph

331 commits

Author SHA1 Message Date
Vasyl Vavrychuk
18ee51ac7b use available api to check daemon status
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
2012-09-27 22:27:51 +03:00
vrana
7ed8b6d7fa Recheck if revision hasn't been already closed in commit parser
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
2012-09-26 21:12:03 -07:00
vrana
e0e97b08b8 Open editor on first modified line
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
2012-09-24 11:07:59 -07:00
vrana
f7a38df5a5 Add TODO comment to commit attacher 2012-09-19 12:30:27 -07:00
Nick Harper
90b60bc3c9 Load relationships for revision
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
2012-09-06 15:46:51 -07:00
epriestley
0736592cff Add didParseCommit() to DifferentialFieldSpecification
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
2012-09-06 12:13:51 -07:00
vrana
b3d7ed3e8e Respect users with duplicate real names in commit parser
Test Plan:
  $parser->resolveUserPHID('Lei Zhao');
  $parser->resolveUserPHID('Jakub Vrana');

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3437
2012-09-05 12:24:35 -07:00
epriestley
ad409126b1 Fix a bug with owners analysis of commits
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
2012-08-23 10:12:41 -07:00
epriestley
93b0a501a4 Add local navigation to Differential
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
2012-08-21 15:01:20 -07:00
vrana
0f919ecd3c Update line count after attaching commit diff
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
2012-08-13 14:43:05 -07:00
epriestley
3e29921f5e Use phutil_utf8_convert() in Phabricator
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
2012-08-12 08:50:19 -07:00
Nick Harper
62b3e4aea3 Remove a check for closing revisions
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
2012-08-10 16:15:37 -07:00
vrana
1c98ca7042 Fix doc links 2012-08-06 15:58:28 -07:00
Alan Huang
bcb9de4ea1 Add a context field to symbol objects
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
2012-08-06 12:20:45 -07:00
epriestley
b8f4f7c438 Fix an issue with Diffusion's display of copied files in Git
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
2012-08-01 12:32:35 -07:00
vrana
9092994e45 Make old and new files when checking for changes by commit
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
2012-07-30 17:57:30 -07:00
pearj
f2dea87c11 Fix Exception Bad getter call: getURIObject
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.
2012-07-28 18:27:52 +10:00
epriestley
d9296638cd Fix a "user@domain:path" protocol handling bug
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
2012-07-27 17:21:33 -07:00
epriestley
514ee3526c Add an event for looking up names from repositories
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
2012-07-24 11:59:28 -07:00
epriestley
1e9120cdfb Minor, fix an issue in Diffusion/Herald with commits that have no Auditors. 2012-07-24 07:37:41 -07:00
epriestley
8cbd860e31 Include email in Git author/committer lookup
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
2012-07-23 13:59:58 -07:00
epriestley
839f3df9c2 Stop trasforming "scp-style" URIs into normal URIs in Git
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
2012-07-23 12:05:03 -07:00
epriestley
4dd707b189 Include explicit "Auditors:" in initial Diffusion email
Summary:
See https://groups.google.com/forum/?hl=en_US&fromgroups#!topic/phabricator-dev/ZonbTN-0TGw

When a commit's author explicitly specifies users as "Auditors", send them an email about the commit.

Test Plan: Made a commit with "Auditors: turtle", ran "reparse.php --herald" on it, verified user "turtle" got included in the intial email.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran, MarkChuCarroll

Differential Revision: https://secure.phabricator.com/D3032
2012-07-23 09:24:02 -07:00
epriestley
378feb3ffb Centralize rendering of application mail bodies
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
2012-07-16 19:01:43 -07:00
epriestley
9574b91e55 Add options to include patches inline or attached for Diffusion commit emails
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
2012-07-12 13:33:35 -07:00
epriestley
e2e9aed4fa Fix symbol handling in symbol query and IRC "Where is x?" handler
Summary: If a symbol's project has no linked repository, we currently explode. Instead, decline to generate a URI and fall back gracefully.

Test Plan: https://secure.phabricator.com/chatlog/channel/%23phabricator/?at=22345

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1465

Differential Revision: https://secure.phabricator.com/D2948
2012-07-09 17:51:42 -07:00
epriestley
ddf67fce58 Add an example event listener, improve documentation, and add a commit discovery event
Summary: Improve documentation around Phabricator events.

Test Plan: Generated and read documentation. Ran test script.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran

Maniphest Tasks: T1092

Differential Revision: https://secure.phabricator.com/D2917
2012-07-03 16:46:27 -07:00
epriestley
dcf3a31944 Minor, unlock after discovery exception. 2012-07-03 07:46:38 -07:00
epriestley
f5f88d8698 Minor, fix an issue where %e is empty from git log. 2012-07-02 07:29:15 -07:00
epriestley
1c1fdf0448 Minor, move the "repair" instructions to the right place. 2012-07-02 07:22:56 -07:00
Nick Harper
d70f8dad3f Change lock in PullLocalDaemon to only lock on discovery
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
2012-06-29 16:19:32 -07:00
David Reuss
aa95ef2463 Handle encoding if git tells us it's still not UTF-8
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
2012-06-29 12:38:09 -07:00
epriestley
46008ce3b3 Record initial branch when recording commits
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
2012-06-28 14:00:00 -07:00
epriestley
1bd0a1f0d9 Add a global lock for the RepositoryPullDaemon
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
2012-06-28 09:54:55 -07:00
Jason Ge
b644a2c3a7 Remove leading "/" so that "git cat-file" works
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
2012-06-26 11:58:34 -07:00
epriestley
7b50b2fbdc Use subprocess output streaming to improve performance of Git commit discovery
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
2012-06-25 15:21:48 -07:00
epriestley
ca31e3e84b Add a --repair flag to "bin/repository discover"
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
2012-06-25 12:35:47 -07:00
epriestley
13d96e6377 Introduce "bin/repository" for repository management
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
2012-06-25 12:35:37 -07:00
epriestley
a705f336a3 Add vebose logging to PhutilRepositoryPullDaemon
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
2012-06-24 15:06:40 -07:00
Jason Ge
de46a2550c Fix auto-close in a repo with missing commits in Phabricator db
Summary:
After adding more error logging: https://secure.phabricator.com/differential/diff/5295/, discover.php prints the output: https://secure.phabricator.com/P427

The problem is that there are gaps in the commits and revisions in Phabricator's DB for the repo. For some git commits, both the phabricator commits and the differential revision are missing (because of a mysterious DB data loss. nharper should be able to recover it); for some other git commits, the phabricator commits are missing (probably also because of the daba loss mentioned above). This can also happen if the git commits were too big to be parsed by phabricator.

Because of the gap, the pull daemon detects them as new autoclose commits on the branch
  (https://secure.phabricator.com/diffusion/P/browse/master/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php;382bafa271ec4962$267)
  (https://secure.phabricator.com/diffusion/P/browse/master/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php;382bafa271ec4962$592)

Then tries to update it
  (https://secure.phabricator.com/diffusion/P/browse/master/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php;382bafa271ec4962$616)
and will fail because the commit doesn't exist in the DB.
  (https://secure.phabricator.com/diffusion/P/browse/master/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php;382bafa271ec4962$341)
After the exception is thrown, all the commits just detected are not marked as 'seenOnBranches', so they won't be marked as closed by the CommitMessageParserWorker.

This also explains the weird behavior where the parsing of the commits is always paused for 15 minutes and then gets started again. The parsing pauses because the process listed at https://secure.phabricator.com/P427 takes quite some time to finish before it can record tha tasks. The parsing happens all the time because the process will be executed again and again.

A better fix might be to put all the missing commits in the repository_badcommit table and check against it.

Test Plan: will run 'discover.php FBCODE' to test it.

Reviewers: epriestley, vrana

Reviewed By: vrana

CC: nh, vrana, aran, Korvin

Differential Revision: https://secure.phabricator.com/D2846
2012-06-23 09:47:20 -07:00
Jason Ge
969dcc5bae handle svn and hg for auto-close
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
2012-06-22 17:05:06 -07:00
Jason Ge
a75da4efac Revert "run record commit if its the first time seen"
Summary: This reverts commit 22e606d49e.

Test Plan: none

Reviewers: epriestley, nh

Reviewed By: nh

CC: nh, vrana, aran, Korvin

Differential Revision: https://secure.phabricator.com/D2833
2012-06-21 20:26:07 -07:00
Jason Ge
22e606d49e run record commit if its the first time seen
Test Plan: test live.

Reviewers: nh

Reviewed By: nh

CC: epriestley, aran

Differential Revision: https://secure.phabricator.com/D2832
2012-06-21 19:47:25 -07:00
Jason Ge
06c976b738 check commit before calling its method
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
2012-06-21 18:36:31 -07:00
epriestley
8ddfdb26e9 Publish feed stories about commits
Summary: When stuff gets committed, publish feed stories about it.

Test Plan: {F13061}

Reviewers: jungejason, vrana, voldern

Reviewed By: jungejason

CC: aran

Maniphest Tasks: T1322

Differential Revision: https://secure.phabricator.com/D2805
2012-06-20 06:03:44 -07:00
epriestley
d277908842 Minor, avoid branch loop nonsense for repositories not set to autoclose. See IRC. 2012-06-19 15:05:22 -07:00
epriestley
e48830eecb Don't rediscover an entire branch when a new commit appears at HEAD
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
2012-06-19 13:47:01 -07:00
epriestley
14648d6d7a Provide more informative messages when autoclosing revisions
Summary: This has been a point of some confusion, make the messages more explicit.

Test Plan:
Added var_dump() stuff and ran on some commits:

  $ ./scripts/repository/reparse.php --message rP9fc54f4dfb61f7338cb1cfe819bc72d2a3404264
  Running 'PhabricatorRepositoryGitCommitMessageParserWorker'...
  string(58) "Closed by commit rP9fc54f4dfb61 (authored by @epriestley)."
  $ ./scripts/repository/reparse.php --message rP444c634b6c6612fc7b36ddffab8023ef67372ab9
  Running 'PhabricatorRepositoryGitCommitMessageParserWorker'...
  string(83) "Closed by commit rP444c634b6c66 (authored by Ben Rogers, committed by @epriestley)."
  $ ./scripts/repository/reparse.php --message rP22d12fe499e3ecb62392397f2ac2a91768c974aa
  Running 'PhabricatorRepositoryGitCommitMessageParserWorker'...
  string(52) "Closed by commit rP22d12fe499e3 (authored by vrana)."
  $ ./scripts/repository/reparse.php --message rPe51958159483cd0acf00adcff51edf8717b4a23b
  Running 'PhabricatorRepositoryGitCommitMessageParserWorker'...
  string(85) "Closed by commit rPe51958159483 (authored by David Fisher, committed by @epriestley)."

Reviewers: csilvers, vrana

Reviewed By: csilvers

CC: aran

Differential Revision: https://secure.phabricator.com/D2765
2012-06-15 17:00:08 -07:00
Ben Rogers
444c634b6c Allows branches to appear in diffusion with out triggering closing things from the commit message
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
2012-06-15 14:10:15 -07:00
vrana
892a2d1b61 Make Thread-Topic human readable
Summary:
Some e-mail clients display this header and it needs to be constant.

This is somehow involved but I doubt that there is a simpler solution.

Test Plan:
Applied SQL patch.
Commented on revision, commented on commit, changed package.
Verified that the `Thread-Topic` has constant and human readable value.

Reviewers: epriestley

Reviewed By: epriestley

CC: ola, aran, Korvin

Differential Revision: https://secure.phabricator.com/D2745
2012-06-14 11:36:34 -07:00
epriestley
375c921bb0 Minor, fix issue with defaults: wildcard and repeat default to array and may not have a different default. 2012-06-12 07:05:57 -07:00
vrana
2e484e257d Fix lint errors found by Nemo
Summary:
See also:

- https://github.com/tpyo/amazon-s3-php-class/pull/33
- https://github.com/stripe/stripe-php/pull/13

Test Plan: Ran a script analyzing sources by HPHP.

Reviewers: btrahan, jungejason, epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D2713
2012-06-11 19:09:42 -07:00
vrana
2793828795 Refactor setting e-mail subjects
Summary:
It seems that Outlook and Mail.app mostly ignores the threading headers and thread primarily by subject.
They are also very picky about the Re: part in the header.
I guess that's because users of these clients often hit Reply when they want to create a new message to the sender of an e-mail.

We need both of these applications to work with the same setting because we don't use multiplexing to prevent sending multiple e-mails to people in lists.
I also believe that the default behavior should just work in most setups.

I've tried several different combinations of putting "Re:" and none of them seems to always work in both clients.

This diff at least adds more abstraction to the code which should prevent copy/paste errors (two fixed by this diff!).

Test Plan: Sent several e-mails with varying subject, verified that they look as before in Outlook and Mail.app.

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D2709
2012-06-11 19:07:21 -07:00
vrana
a0b57cefb7 Declare that revision was closed by committer, not author
Summary: D2703#13 is confusing - it looks like that @allenjohnashton took the action but it was @epriestley.

Test Plan: I don't have a repro so I tested this block standalone.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D2707
2012-06-11 11:30:27 -07:00
epriestley
5fee8c50ee Fail more softly if we can't execute "ps"
Summary: If, e.g., $PATH is broken we may not be able to run "ps". We'll explode pretty hard, currently. Instead, just show a harsher warning.

Test Plan: Changed "ps auxwww" to "psq", which doesn't exist on my system. Loaded page, got warning instead of explosion.

Reviewers: nathanws, vrana, btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D2624
2012-06-02 14:05:27 -07:00
vrana
6cc196a2e5 Move files in Phabricator one level up
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
2012-06-01 12:32:44 -07:00
vrana
1ebf9186b4 Depend on class autoloading
Test Plan:
Run setup.
/differential/

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Koolvin

Maniphest Tasks: T1103

Differential Revision: https://secure.phabricator.com/D2612
2012-05-30 16:57:21 -07:00
epriestley
09c8af4de0 Upgrade phabricator to libphutil v2
Summary: Mechanical changes from D2588. No "Class.php" moves yet.

Test Plan: See D2588.

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran

Maniphest Tasks: T1103

Differential Revision: https://secure.phabricator.com/D2591
2012-05-30 14:26:29 -07:00
Jason Ge
bbd5c8c187 Add audit entry when code changed after a revision was accepted
Summary: build on top of D2530 and D2540. Add an auditing entry when the code was changed after the revision was accepted.

Test Plan: ran reparse.php manually. It worked at https://phabricator.fb.com/rPHGIT461864d2e09dad04c28505658ef75a979e44d0d3. Look at the latest auditing entry for auditor 'phgit ha directory'.

Reviewers: vrana

Reviewed By: vrana

CC: nh, epriestley, aran

Maniphest Tasks: T201

Differential Revision: https://secure.phabricator.com/D2586
2012-05-29 11:32:09 -07:00
vrana
af6238ca4a Inform about changes made between last revision and commit
Summary:
This adds a link to [Closed] e-mail if it detects some changes.
It compares added and removed lines with 3 lines context.
The subtle form of informing is permissive to false negatives and positives.
I have an e-mail filter for [Closed] e-mails so I wouldn't personally notice this change - we should probably promote this feature a little bit.

Test Plan:
Reparse a diff with a change after last update.
Reparse a diff without a change after last update.

Reviewers: jungejason, epriestley

Reviewed By: jungejason

CC: aran, Koolvin, btrahan

Maniphest Tasks: T201

Differential Revision: https://secure.phabricator.com/D2540
2012-05-25 21:39:58 -07:00
Bob Trahan
65710ee2d2 Fix repository interactions for SVN repositories using the SVN protocol with SASL
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
2012-05-23 12:37:43 -07:00
Hafsteinn Baldvinsson
a438c87c52 Show the difference between a committer and an author
Summary:
Git and hg (supposedly..) differentiate between an author (who wrote the patch)
and a committer (who applied the patch).

This patch allows Phabricator to note when a patch is committed
by someone other than the Author.

Test Plan:
Created 2 accounts,
 - U (Account with a PHID)
 - U' (Account without a PHID)
and had them create and commit commits

testing if their username/real name would be displayed correctly in Diffusion,
  - BrowserTable
  - HistoryTable
  - Code revision

Teztz,
A(uthor)/C(ommitter)
If it's A/A then Author committed

UL = User link (<a href="/p/username">username</a>)
UN = User name ("Firstname Lastname")

Tezt | Expected in table  | Got
-------------------------------------------
A/A   | UL/UL             | UL/UL
A'/C  | UN/UL             | UN/UL
A/C'  | UL/UN             | UL/UN
A'/C' | UN/UN             | UN/UN

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Koolvin

Maniphest Tasks: T688

Differential Revision: https://secure.phabricator.com/D2541
2012-05-23 08:34:54 -07:00
vrana
ccd37afab8 Attach commit diff to its revision
Summary:
This attaches commit diff to its associated revision as any other diff.
The consequence is that the revision page now shows the actual commit instead of the last diff. It may be disturbing but it is desired.
Another consequence is that lint and unit results are displayed as skipped after the revision was committed. I want to fix it somehow.

My next plan is to automatically diff against the last normal diff and include the link to this diff in commit e-mail if non-empty.

Test Plan:
  reparse.php

Diff against last normal diff.

Reviewers: epriestley, jungejason

Reviewed By: jungejason

CC: aran, Koolvin, jungejason

Maniphest Tasks: T201

Differential Revision: https://secure.phabricator.com/D2530
2012-05-22 16:08:16 -07:00
Sigurd Holsen
c2d2f6ded1 bugfix: mercurial commit worker had an array error
It tried to get $path['targetPath'] istead of $change['targetPath']
when $path was a string
2012-05-21 09:04:48 -07:00
Bob Trahan
80f479d948 Make directories with spaces in their names work OK in diffusion end to end
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
2012-05-20 15:29:36 -07:00
epriestley
3a1ee00335 Select default branches more effectively in Diffusion
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
2012-05-20 14:50:43 -07:00
Bob Trahan
7a19cf5a55 Fix lines of a large include path
Summary:
D2457 and D2459 made some changes here. This little guy just needed to be touched so it could be arc lint'd to have the proper updated include paths

Sorry to spam reviewers; Evan is in Diablo 3 somewhere. :D

Test Plan: no more fatal on test everything implemented (arc unit src/infrastructure/__tests__/)

Reviewers: epriestley, jungejason, nh, csilvers

CC: aran, Koolvin

Differential Revision: https://secure.phabricator.com/D2474
2012-05-15 12:37:34 -07:00
epriestley
820a6d407a Improve performance of repository discovery under Mercurial
Summary: Prelminary, I want to test this a bit more when I'm better rested. Provide a "bulk" import mode for Mercurial so we can do initial discovery more quickly.

Test Plan: Imported the Mercurial repository in 2m45s, blocked on MySQL I/O rather than Mercurial I/O.

Reviewers: csilvers, btrahan

Reviewed By: csilvers

CC: aran

Differential Revision: https://secure.phabricator.com/D2457
2012-05-11 18:29:14 -07:00
epriestley
61b728b19d Add a setting to blanket-disable autoclosing revisions in a repository
Summary: For git workflows where developers push personal feature branches to the origin, this is a quick workaround until T1210 is implemented properly. I'll also mention this in D2446.

Test Plan:
  - Committed a revision in an "autoclose" repository; it was autoclosed. Committed a revision in a no-autoclose repository, it was not autoclosed.
  - Edited repositories, saving the "autoclose" setting as enabled/disabled.

Reviewers: aurelijus, btrahan

Reviewed By: aurelijus

CC: aran

Maniphest Tasks: T1210

Differential Revision: https://secure.phabricator.com/D2448
2012-05-10 14:18:56 -07:00
epriestley
1bf68e06a5 Improve Diffusion error messages and UI for partially imported repositories
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
2012-05-09 17:28:57 -07:00
epriestley
7e0c06716f Minor, fix obvious typo that I missed in testing somehow.
Auditors: csilvers
2012-05-09 12:43:57 -07:00
epriestley
b800df8c1b Simplify daemon management: "phd start"
Summary:
  - Merge CommitTask daemon into PullLocal daemon. This is another artifact of past instability (and order-dependent parsers). We still publish to the timeline, although this was the last consumer. Long term we'll probably delete timeline and move to webhooks, since everyone who has asked about this stuff has been eager to trade away the durability and ordering of the timeline for the ease of use of webhooks. There's also no reason to timeline this anymore since parsing is no longer order-dependent.
  - Add `phd start` to start all the daemons you need. Add `phd restart` to restart all the daemons you need. So cool~
  - Simplify and improve phd and Diffusion daemon documentation.

Test Plan:
  - Ran `phd start`.
  - Ran `phd restart`.
  - Generated/read documentation.
  - Imported some stuff, got clean parses.

Reviewers: btrahan, csilvers

Reviewed By: csilvers

CC: aran, jungejason, nh

Differential Revision: https://secure.phabricator.com/D2433
2012-05-09 10:29:37 -07:00
epriestley
d2b01aead0 Use one daemon to discover commits in all repositories, not one per repository
Summary:
See D2418. This merges the commit discovery daemon into the same single daemon, and applies all the same rules to it.

There are relatively few implementation changes, but a few things did change:

  - I simplified/improved Mercurial importing, by finding full branch tip hashes with "--debug branches" and using "parents --template {node}" so we don't need to do separate "--debug id" calls.
  - Added a new "--not" flag to exclude repositories, since I switched to real arg parsing anyway.
  - I removed a web UI notification that you need to restart the daemons, this is no longer true.
  - I added a web UI notification that no pull daemon is running on the machine.

NOTE: @makinde, this doesn't change anything from your perspective, but it something breaks this is the likely cause.

This implicitly resolves T792, because discovery no longer runs before pulling.

Test Plan:

  - Swapped databases to a fresh install.
  - Ran "pulllocal" in debug mode. Verified it correctly does nothing (fixed a minor issue with min() on empty array).
  - Added an SVN repository. Verified it cloned and discovered correctly.
  - Added a Mercurial repository. Verified it cloned and discovered correctly.
  - Added a Git repository. Verified it cloned and discovered correctly.
  - Ran with arguments to verify behaviors: "--not MTEST --not STEST", "P --no-discovery", "P".

Reviewers: btrahan, csilvers, Makinde

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T792

Differential Revision: https://secure.phabricator.com/D2430
2012-05-08 12:53:41 -07:00
epriestley
1c62a35710 Run one daemon to pull all working copies, not one daemon per working copy
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
2012-05-07 15:01:10 -07:00
epriestley
87207b2f4e Allow users to have multiple email addresses, and verify emails
Summary:
  - Move email to a separate table.
  - Migrate existing email to new storage.
  - Allow users to add and remove email addresses.
  - Allow users to verify email addresses.
  - Allow users to change their primary email address.
  - Convert all the registration/reset/login code to understand these changes.
  - There are a few security considerations here but I think I've addressed them. Principally, it is important to never let a user acquire a verified email address they don't actually own. We ensure this by tightening the scoping of token generation rules to be (user, email) specific.
  - This should have essentially zero impact on Facebook, but may require some minor changes in the registration code -- I don't exactly remember how it is set up.

Not included here (next steps):

  - Allow configuration to restrict email to certain domains.
  - Allow configuration to require validated email.

Test Plan:
This is a fairly extensive, difficult-to-test change.

  - From "Email Addresses" interface:
    - Added new email (verified email verifications sent).
    - Changed primary email (verified old/new notificactions sent).
    - Resent verification emails (verified they sent).
    - Removed email.
    - Tried to add already-owned email.
  - Created new users with "accountadmin". Edited existing users with "accountadmin".
  - Created new users with "add_user.php".
  - Created new users with web interface.
  - Clicked welcome email link, verified it verified email.
  - Reset password.
  - Linked/unlinked oauth accounts.
  - Logged in with oauth account.
  - Logged in with email.
  - Registered with Oauth account.
  - Tried to register with OAuth account with duplicate email.
  - Verified errors for email verification with bad tokens, etc.

Reviewers: btrahan, vrana, jungejason

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1184

Differential Revision: https://secure.phabricator.com/D2393
2012-05-07 10:29:33 -07:00
epriestley
049048765d Add "repository.create" and "repository.query" methods to Conduit
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
2012-05-04 16:16:22 -07:00
vrana
38ffe45f8e Use committer date instead of author date for Git epoch
Summary:
This is somewhat controversial but push date is usually more useful than commit date (which can be for example a month before other people can see the commit).
We can also store both dates.

Test Plan:
  git log --pretty="%ct %at"

Reviewers: epriestley

Reviewed By: epriestley

CC: nh, aran, Koolvin

Differential Revision: https://secure.phabricator.com/D2319
2012-04-26 16:25:56 -07:00
epriestley
20a5c9b261 Use "closed", not "committed", in Differential
Summary: "Committed" is SVN-specific language, and confusing in Git and Mercurial. Use neutral language instead.

Test Plan: Inspection.

Reviewers: btrahan, Makinde, vrana, jungejason

Reviewed By: vrana

CC: aran

Maniphest Tasks: T909

Differential Revision: https://secure.phabricator.com/D2087
2012-04-23 17:40:57 -07:00
vrana
1f2cf78c1b Display committed date in Revision Status field
Summary:
This is slightly more complicated for this reason:

- We don't set `dateCommitted` for normal commits, only for markcommitted.
-- We need to add this date to old revisions now.

Test Plan:
Reparse a revision - commit date was set.
Conduit `markcommitted` - commit date was set.
Run SQL script.
Display closed revision.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Koolvin

Differential Revision: https://secure.phabricator.com/D2282
2012-04-19 15:05:09 -07:00
epriestley
dec8acd803 Add very basic tag support to Diffusion
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
2012-04-18 08:02:08 -07:00
vrana
d17be1d824 Fix SVN commit change parser for files moved from deleted directory
Summary:
This continues work started at D2215.
Files moved from deleted directory were marked as Copied Here instead of Moved Here.

Test Plan: Reparsed two commits which was previously wrong, now correct.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Maniphest Tasks: T1114

Differential Revision: https://secure.phabricator.com/D2229
2012-04-14 22:18:27 -07:00
vrana
ef990703fa Fix SVN commit change parser for directories copied from the same path
Summary: See [[ https://secure.phabricator.com/D2215?id=3773#inline-2451 | D2215#inline-2451 ]].

Test Plan: Reparsed commit which was wrong, now correct.

Reviewers: epriestley, jungejason

Reviewed By: jungejason

CC: aran

Differential Revision: https://secure.phabricator.com/D2232
2012-04-14 21:46:02 -07:00
vrana
17b0277ec5 Fix SVN commit change parser for files moved from deleted directory
Summary: This is not perfect. Moved files are reported as deleted but I'm happy with it.

Test Plan: Reparsed two commits which was previously wrong, now semi-correct.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Maniphest Tasks: T1114

Differential Revision: https://secure.phabricator.com/D2215
2012-04-12 17:47:19 -07:00
epriestley
c458768415 Fix various threading issues, particularly in Gmail
Summary:
  - Add an explicit multiplexing option, and enable it by default. This is necessary for Mail.app to coexist with other clients ("Re:" breaks outlook at the very least, and generally sucks in the common case), and allows users with flexible clients to enable subject variance.
  - Add an option for subject line variance. Default to not varying the subject, so mail no longer says [Committed], [Closed], etc. This is so the defaults thread correctly in Gmail (not entirely sure this actually works).
  - Add a preference to enable subject line variance.
  - Unless all mail is multiplexed, don't enable or respect the "Re" or "vary subject" preferences. These are currently shown and respected in non-multiplex cases, which creates inconsistent results.

NOTE: @jungejason @nh @vrana This changes the default behavior (from non-multiplexing to multiplexing), and might break Facebook's integration. You should be able to keep the same behavior by setting the options appropriately, although if you can get the new defaults working they're probably better.

Test Plan:
Send mail from Maniphest, Differential and Audit. Updated preferences. Enabled/disabled multiplexing. Things seem OK?

NOTE: I haven't actually been able to repro the Gmail threading issue so I'm not totally sure what's going on there, maybe it started respecting "Re:" (or always has), but @cpiro and @20after4 both reported it independently. This fixes a bunch of bugs in any case and gives us more conservative set of defaults.

I'll see if I can buff out the Gmail story a bit but every client is basically a giant black box of mystery. :/

Reviewers: btrahan, vrana, jungejason, nh

Reviewed By: btrahan

CC: cpiro, 20after4, aran

Maniphest Tasks: T1097, T847

Differential Revision: https://secure.phabricator.com/D2206
2012-04-12 09:31:03 -07:00
vrana
e87e1786a6 Fix docs links after D2181
Test Plan:
  diviner .

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2188
2012-04-10 11:33:26 -07:00
epriestley
a5903d2a53 Use head_key() and last_key() to explicitly communicate intent
Summary:
PHP arrays have an internal "current position" marker. (I think because foreach() wasn't introduced until PHP 4 and there was no way to get rid of it by then?)

A few functions affect the position of the marker, like reset(), end(), each(), next(), and prev(). A few functions read the position of the marker, like each(), next(), prev(), current() and key().

For the most part, no one uses any of this because foreach() is vastly easier and more natural. However, we sometimes want to select the first or last key from an array. Since key() returns the key //at the current position//, and you can't guarantee that no one will introduce some next() calls somewhere, the right way to do this is reset() + key(). This is cumbesome, so we introduced head_key() and last_key() (like head() and last()) in D2161.

Switch all the reset()/end() + key() (or omitted reset() since I was feeling like taking risks + key()) calls to head_key() or last_key().

Test Plan: Verified most of these by visiting the affected pages.

Reviewers: btrahan, vrana, jungejason, Koolvin

Reviewed By: jungejason

CC: aran

Differential Revision: https://secure.phabricator.com/D2169
2012-04-09 11:08:59 -07:00
Jason Ge
f6748bc190 The existing Audit code forgot to set authorPHID
Summary:
The audit tools has many false positive about Author Not
Matching with Revision. The fix is to set the authorPHID which was
missing in the existing code

Test Plan:
run reparse.php and it doesn't generate false positive result
anymore.

Reviewers: nh, epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2119
2012-04-06 14:16:42 -07:00
mkedia
591d50008f Allow projects to own packages
Summary:
- The UI is pretty straightforward, since Handle just works (tm)
- Added two methods to the owners object to handle the new layer of
  indirection. Then ran git grep PhabricatorOwnersOwner and changed
  callsites as appropriate.

Sending this to get a round of feedback before I test the non-trivial
changes in this diff.

Test Plan:
- owners tool: edit, view, list for basic functionality.
- phlog for the two new methods I added

Reviewers: epriestley, blair, jungejason

CC: aran

Differential Revision: https://secure.phabricator.com/D2079
2012-04-03 16:20:07 -07:00
vrana
8813c7be0e Use assert_instances_of() everywhere but Differential and Diffusion
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
2012-04-03 14:53:20 -07:00
epriestley
2ef3e69e58 Improve Herald commit emails
Summary:
  - Show the canonical (i.e., shorter) commit identifier in the subject.
  - For commits without a revision, put the commit summary in the subject.

Test Plan: Ran "scripts/repository/reparse.php <commit> --herald" for a number of different commits (with revision, without revision); got more useful email subjects.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T1028

Differential Revision: https://secure.phabricator.com/D2004
2012-03-23 11:08:18 -07:00
epriestley
85f19e16dc Fix the "Browse in Diffusion" URI in Differential
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
2012-03-22 13:55:35 -07:00
epriestley
6d631577f2 Detect changes in merge commits as the diff of the merge and the first parent
Summary:
Currently, we use "git log" to detect the change list for all commits, but this produces no output for merge commits.

Instead, parse them as changes against the first parent (the merge destination). This produces generally sensible/expected behavior, and is consistent with what GitHub does.

We need to special-case the first commit because it doesn't have parents.

NOTE: This is a parser change so you need to run `./scripts/repository/reparse.php --all <callsign> --change` to reparse merge commits in already-imported repositories after updating.

Test Plan: Reparsed a merge commit, a non-merge commit, and the first commit in the Phabricator repository.

Reviewers: btrahan, gschmidt

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T961

Differential Revision: https://secure.phabricator.com/D1985
2012-03-21 17:10:05 -07:00
epriestley
620e936cba Fix symbol URI generation to include default branch name for relevant repositories
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
2012-03-21 16:58:44 -07:00
epriestley
30ae22bfcf Fix many encoding and architecture problems in Diffusion request and URI handling
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
2012-03-19 19:52:14 -07:00
epriestley
ec736f9c50 Handle "hg pull" return code change between 2.1 and 2.1.1 more gracefully
Summary:
See <https://github.com/facebook/phabricator/issues/102>. Between Feb 1 and Mar 1, the hg released changed the exit code behavior of "hg pull". This broke us mildly (and a bunch of other applications more severely, which is why it was reverted).

Detect the common case of this (english) and don't fail.

Test Plan: @killermonk, can you try applying this? I'll try to do an upgrade to 2.1 and see if I can also do a proper test.

Reviewers: Makinde, btrahan, killermonk

Reviewed By: btrahan

CC: killermonk, aran, epriestley

Differential Revision: https://secure.phabricator.com/D1948
2012-03-19 19:19:48 -07:00
Hafsteinn Baldvinsson
fffc1e51d0 Inset view controller for inset elements of forms.
Summary:
T937 suggests 'inset' could have its own view controller.

It has the following methods:
 - setTitle         for title
 - setRightbutton   if you have to place something (preferably a button)
                    on the right side of the form
 - setDescription   if you want to describe what it does
 - setContent       for the main content
 - addDivAttributes REALLY not sure about this one but it had to be included
                    because of a single controller (see owners/controller/edit/PhabricatorOwnersEditController.php:238)
 - appendChild      works as usual if your form is complex but you still want to remove
                    ->appendChild('<div class..') ->appendChild('</div>');

It might be an overkill so maybe some could be dropped:
 - addDivAttributes() and just rewrite how PhabricatorOwnersEditController.php works
 - setContent() and use appendChild for the main content?

Test Plan:
 - Looked at the controllers in phabricator
 - Changed the controller
 - Opened the page in another tab
 - If something didnd't look the same I fixed it.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1926
2012-03-15 17:10:38 -07:00
epriestley
d0af617818 Add "final" to (almost) everything else
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
2012-03-13 16:21:04 -07:00
Nick Harper
9b48384415 [svn1.7] Fix matching xml tag when parsing svn commits
Summary:
svn 1.7 changed their xml format slightly, they now have a
 ##<?xml version="1.0" encoding="UTF-8"?>## tag instead of
 ##<?xml version="1.0"?>##. This relaxes matching this tag.

Test Plan: ./scripts/repository/reparse.php rE521979 --change

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1866
2012-03-12 16:32:30 -07:00
epriestley
def19bb8de Add additional protections against local repository misconfigurations
Summary: We've hit a couple of these in the wild, raise better error messages when the local repo is toast / broken / nonsense.

Test Plan: Broke my local repo in all of the different ways we test for, verified I got an error message in each case.

Reviewers: btrahan, abirchall

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T964, T924

Differential Revision: https://secure.phabricator.com/D1855
2012-03-12 10:34:37 -07:00
vrana
d5bf30bb48 Prepare database for UTF-8
Summary: D1830#8

Test Plan:
`scripts/sql/upgrade_schema.php`
Try adding duplicate SSH Public Key - failed.
Try adding new SSH Public Key - succeeded.

Reviewers: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1852
2012-03-09 18:56:22 -08:00
epriestley
b2890eeb0e Add "final" to all Phabricator "Controller" classes
Summary:
These are all unambiguously unextensible. Issues I hit:

  - Maniphest Change/Diff controllers, just consolidated them.
  - Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
  - D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.

Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T795

Differential Revision: https://secure.phabricator.com/D1843
2012-03-09 15:46:25 -08:00
Edward Speyer
226d0321af Revert "PhabricatorRepository: temporarily change the localPath"
Summary:
Reverts an unwanted commit (579941b866)
that got pushed by mistake.
2012-03-09 10:54:24 -08:00
Edward Speyer
579941b866 PhabricatorRepository: temporarily change the localPath
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
2012-03-08 15:15:13 -08:00
epriestley
89dac1cf19 When updating git repositories, use --prune to prune old branches
Summary: We'll keep deleted branches around right now because Git's behavior is not to remove them without --prune.

Test Plan: Ran "git fetch --all --prune" to make sure it at least ostensibly works.

Reviewers: btrahan, 20after4

Reviewed By: 20after4

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1833
2012-03-08 13:24:25 -08:00
epriestley
1c80a4eb58 Add a "properties" table to the Repository view in Diffusion
Summary: Notably, expose the clone URI / remote URI, after stripping credentials.

Test Plan: Looked at a repository.

Reviewers: btrahan, nh

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T91

Differential Revision: https://secure.phabricator.com/D1811
2012-03-08 12:46:19 -08:00
Edward Speyer
17d801a50e GitFetch daemon: more verbose
Summary: A smidgen more messaging about what's going on.

Test Plan:
Ran it, saw this:

  2012-03-07 5:37:30 PM [STDO] >>> [0] <exec> $ /data/phabricator/bin/list_db_services --tier_name 'cdb.phabricator'
  <<< [0] <exec> 47,101 us
  ...
  >>> [9] <exec> $ mkdir -p '/var/repo'
  <<< [9] <exec> 41,374 us
  Creating new directory /var/repo/fbcode for repo FBCode
  >>> [10] <exec> $ git clone --origin origin 'ssh://fbcode.git.vip.facebook.com/data/gitrepos/fbcode.git' '/var/repo/fbcode'

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1819
2012-03-07 19:24:06 -08:00
epriestley
8dfe8e84f0 Improve Diffusion parsing of submoudule changes
Summary: We currently parse these as directory changes and discard them. Instead, parse them as a new "SUBMODULE" type of change.

Test Plan:
  - Reparsed a commit which changes submodules and verified it parses correctly.
  - Reparsed a commit which adds submodules and verified it parses correctly.

Reviewers: btrahan, kdeggelman

Reviewed By: btrahan

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1815
2012-03-07 14:24:43 -08:00
epriestley
76fd9a2d28 Reduce laziness for "Mark Committed"
Summary:
  - Enforce proper workflow rules.
  - Fix a derp-bug with patches.

Test Plan:
  - Tried to mark a revision I didn't own.
  - Tried to mark a revision already marked committed.
  - Tried to mark a revision otherwise not accepted.
  - Verified daemon can override workflow rules and mark from arbitrary states.

Reviewers: btrahan, Makinde

Reviewed By: Makinde

CC: aran, epriestley

Maniphest Tasks: T948

Differential Revision: https://secure.phabricator.com/D1809
2012-03-07 10:20:17 -08:00
epriestley
a95c9873aa Add an "Auditors" field to commit messages which pushes audit requests when present
Summary:
Adds an optional "Auditors" field (like "Reviewers") to commit messages which gives installs a zero-config method for making audit requests.

This field does not appear on templates unless set, and is mostly ignored (but validated and preserved) by Differential.

It is then parsed by the daemons if present, and audit requests are pushed to valid users.

Test Plan: Made an "Auditors" commit and verified it was retained with "arc amend --show". Pushed it and verified the audit was triggered.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T904, T880

Differential Revision: https://secure.phabricator.com/D1793
2012-03-06 15:10:35 -08:00
epriestley
d94129b739 Migrate "PhabricatorOwnersPackageCommitRelationship" to "PhabricatorRepositoryAuditRequest"
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
2012-03-05 13:17:29 -08:00
epriestley
5590515007 Make Diffusion Herald emails thread with Audit emails
Summary:
  - Users may elect to receive an initial notification about a commit; allow it to be replied to in order to interact with the object.
  - Share thread headers between emails.
  - Add the "REPLY HANDLER ACTIONS" section to both emails.

Test Plan:
  - Used "reparse.php --herald" to trigger herald emails, verified reply-to and email body.
  - Made audit comments, verified body.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T904

Differential Revision: https://secure.phabricator.com/D1762
2012-03-05 09:54:45 -08:00
epriestley
d7bb686a47 Minor, actually update commit author field when it is parsed by the worker. 2012-02-28 21:25:15 -08:00
epriestley
8a0a00f118 Make PhabricatorRepositoryCommmit schema changes for audit
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
2012-02-28 21:06:34 -08:00
epriestley
37a1db6fe1 Use HGPLAIN for local hg commands in Phabricator
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
2012-02-27 19:21:03 -08:00
epriestley
d7a7bca85c Enable email for audits
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
2012-02-27 12:57:57 -08:00
epriestley
25fade5008 Add audits to search
Summary: Add audit information to the commit search index.

Test Plan: Updated a commit, searched for terms in its comments, got hits.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, epriestley

Maniphest Tasks: T904

Differential Revision: https://secure.phabricator.com/D1696
2012-02-27 09:51:00 -08:00
epriestley
1094527072 Allow Herald to trigger audits for users or projects
Summary:
Allows you to write a commit rule that triggers an audit by a user (personal
rules) or a project (global rules).

Mostly this is trying to make auditing more lightweight and accessible in
environments where setting up Owners packages doesn't make sense.

For instance, Disqus wants a rule like "trigger an audit for everything that
didn't have a Differential revision". While not necessarily scalable, this is a
perfectly reasonable rule for a small company, but a lot of work to implement
with Owners (and you'll get a lot of collateral damage if you don't make every
committer a project owner).

Instead, they can create a project called 'Unreviewed Commits' and write a rule
like:

	- When: Differential revision does not exist
 	- Action: Trigger an Audit for project: "Unreviewed Commits"

Then whoever cares can join that project and they'll see those audits in their
queue, and when they approve/raise on commits their actions will affect the
project audit.

Similarly, if I want to look at all commits that match some other rule (say,
XSS) but only want to do it like once a month, I can just set up an audit rule
and go through the queue when I feel like it.

NOTE: This abuses the 'packagePHID' field to also store user and project PHIDs.
Through the magic of handles, this (apparently) works fine for now; I'll do a
big schema patch soon but have several other edits I want to make at the same
time.

Also:

	- Adds an "active" fiew for /audit/, eventually this will be like the
Differential "active" view (stuff that is relevant to you right now).
	- On commits, highlight triggered audits you are responsible for.

Test Plan: Added personal and global audit triggers to Herald, reparsed some
commits with --herald, got audits. Browsed all audit interfaces to make sure
nothing exploded. Viewed a commit where I was responsible for only some audits.
Performed audits and made sure the triggers I am supposed to be responsible for
updated properly.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, epriestley

Maniphest Tasks: T904

Differential Revision: https://secure.phabricator.com/D1690
2012-02-27 09:36:30 -08:00
epriestley
a15130b47c Add a maintenance script for reconciling repositories to disk state
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
2012-02-02 16:03:50 -08:00
Jason Ge
b4053190ec Fix two issues in owners worker
Summary:
Fix two issues in PhabricatorRepositoryCommitOwnersWorker:

- if a commit was reviewed by some owner of the package, it should not be
  marked as needing audit
- do not run herald worker when it is not needed (for example, when the
  worker is executed from reparse.php)

Test Plan:
reparse a commit which is reviewed by the owner of a package
and verify that it is not marked as needing audit, and herald is not
executed.

Reviewers: nh, epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1496
2012-01-26 11:02:42 -08:00
epriestley
5b463e634c Write fewer "applied" rows and clean up excess historical rows
Summary:
  - Only write the <ruleID, phid> row if the rule is a one-time rule.
  - Delete all the rows for rules which aren't one-time.

NOTE: This is probably like several million rows for Facebook and could take a
while.

Test Plan:
Added some one-time and every-time rules, ran them against objects, verified
only relevant rows were inserted.
Ran upgrade script against a database with one-time and every-time "ruleapplied"
rows, got the irrelevant rows removed.

Reviewers: nh, btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1484
2012-01-25 11:53:39 -08:00
epriestley
3142fe4419 Remove massive "rule applied" query
Summary:
Herald rules may be marked as "one-time". We track this by writing a row with
<ruleID, phid> when we apply a rule.

However, the current test for rule application involves loading every <ruleID,
*> pair. We also always write this row even for rules which are not one-time, so
if there are 100 rules, we'll load 1,000,000 rows after processing 10,000
objects.

Instead, load only the <phid, *> pairs, which are guaranteed to be bounded to at
most the number of rules.

I'll follow up with a diff that causes us to write rows only for one-time rules,
and deletes all historic rows which are not associated with one-time rules.

Test Plan:
Grepped for callsites to loadAllByContentTypeWithFullData(). Ran
rules in test console.

Reviewers: nh, btrahan, jungejason

Reviewed By: nh

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1483
2012-01-24 19:29:54 -08:00
epriestley
b43eb5aa7c Add transaction-oriented editing to projects
Summary:
  - Make some editing operations transaction-oriented, like Maniphest. (This
seems to be a good model, particularly for extensibility.) I'll move the rest of
the editing operations to transactions in future diffs.
  - Make transaction-oriented operations publish feed stories.

Test Plan:
  - Created a new project.
  - Edited an existing project.
  - Created a new project via quick create flow from Maniphest.
  - Verified feed stories publish correctly.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T681

Differential Revision: https://secure.phabricator.com/D1477
2012-01-24 09:44:35 -08:00
epriestley
27f52efd37 Minor, fix spelling issues detected by linter. 2012-01-20 07:39:55 -08:00
jungejason
4faab06c3c Enable herald rule for commits that need auditing
Summary:
enable herald commit rules to have access to auditing info.

Note that the new herald condition I added contains info for the
packages. I thought about using a simpler herald condition like
"Requires audit is true or false" and let it work together with the
existing "Affected package contains any of the package". It doesn't work
because we need the info about the package to decide if the commit
requires audit, but the herald conditions work separately.

Test Plan:
- A commit requiring auditing was detected by a herald rule that checks
  the auditing status
- A commit not requiring auditing was not detected by a herald rule
  which checks auditing status, but was detected by a rule which doesn't
  check the auditing status

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1399
2012-01-17 09:13:07 -08:00
epriestley
b10fa8023f Support Git implicit file:// URIs
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
2012-01-11 09:00:18 -08:00
Bob Trahan
2a29a51080 Deploy new ArcanistManyWordsAboutDifferentialConstants class from D1328 into
Phabricator

Summary: ...this breaks without D1328.   Used good ole "codemod" to do this
work, with lots of manual edits around 80 chars.

Test Plan: clicked around phabricator tool suite, particular differential, a
bunch

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1351
2012-01-10 11:49:20 -08:00
epriestley
1903bb80bb Add a link from Differential to Diffusion
Summary:
Provide an easy way to jump to Diffusion from Differential if we have
the data we need to connect them.

Test Plan: Tested menu in linked and unlinked diffs. Used menu item.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley, btrahan

Maniphest Tasks: T309

Differential Revision: https://secure.phabricator.com/D1326
2012-01-05 18:03:08 -08:00
epriestley
9c5a6601d6 Remove some Mercurial and Google caveats
Summary: These seem to work relatively reasonably and don't have any known
deal-breaking failures.

Test Plan: shrug~

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1324
2012-01-05 14:14:06 -08:00
Bob Trahan
e478c0074c Add support for querying by commit hashes to DifferentialRevisionQuery class and
corresponding ConduitAPI

Summary: reasonable title...  also made this new functionality used by the
repository worker for parsing diffs

Test Plan:
- looked at the conduit console and queried for various types of hashes,
including hashes with no match.  got correct results.
- identified a reasonable diff from a local git repo.  set the revision status
to 2 (ACCEPTED) in the database.  augmented the worker parser code to var_dump
and die after finding revision id.   ran scripts/repository/reparse.php
--message rX and verified my var_dumps.  removed var_dumps and die and ran
reparse.php again with same paramters.  verified revision looked good in
diffusion and there were no errors.
- repeated the above reparse.php jonx for a mercurial repo.  note svn isn't in
this hash game so that test was particularly exciting no-op'dness i did not
bother with

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, btrahan, epriestley

Differential Revision: https://secure.phabricator.com/D1315
2012-01-05 13:51:51 -08:00
epriestley
99231ebba4 Allow Git repositories to track only some branches
Summary:
Some installs use Git as the backbone of a CI framework or use a Git remote to
share patches. The tracker scripts currently recognize associated revisions as
"Committed" when they appear in any branch, even if that branch is
"alincoln-personal-development_test_hack" or whatever.

To address the broadest need here, allow Git repositories to be configured to
track only certain branches instead of all branches.

This doesn't allow you to import a branch into Diffusion but ignore it in
Differential. Supporting that is somewhat technically complicated because the
parser currently goes like this:

  - Look at HEAD of all branches.
  - For any commits we haven't seen before, follow them back to something we
have seen (or the root).
  - "Discover" everything new.

Since this doesn't track <branch, commit> pairs, we currently don't have enough
information to tell when a commit appears in a branch for the first time, so we
don't have anywhere we can put a test for whether that branch is tracked and do
the Differential hook only if it is.

However, I think this cruder patch satisfies most of the need and is simple and
obvious in its implementation.

See also D1263.

Test Plan:
  - Updated a Git repository with various filters: "", "master, remote", "derp",
"  ,,, master   ,,,,,"
  - Edited SVN and Mercurial repositories to verify they didn't get caught in
the crossfire.
  - Ran daemon in debug mode on libphutil with filter "derp", got exception
about no tracked branches. Ran with filter "master", got tracking. Ran with no
filter, got tracking.
  - Looked at Diffusion with "derp" and "master", saw no branches and "master"
respectively.
  - Added unit tests to cover filtering logic.

Reviewers: btrahan, jungejason, nh, fratrik

Reviewed By: fratrik

CC: aran, fratrik, epriestley

Maniphest Tasks: T270

Differential Revision: https://secure.phabricator.com/D1290
2012-01-04 10:20:34 -08:00
epriestley
ec1df21bef Add getStrList() to AphrontRequest
Summary:
  - We have a few places where we do some kind of ad-hoc comma list tokenizing,
and I'm adding another one in D1290. Add a helper to the request object.
  - Add some unit tests.

Test Plan:
  - Ran unit tests.
  - Used PHID manager, Maniphest custom view, and Repository project editor.

Reviewers: btrahan, fratrik, jungejason

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Differential Revision: https://secure.phabricator.com/D1302
2012-01-04 10:18:46 -08:00
epriestley
efb0fa739f Make tracked git repositories use an implicit 'origin' remote
Summary:
See T624. I originally wrote this to require an explicit remote, but this
creates an ugly "origin:" in all the URIs and makes T270 more difficult.

Treat all branch names as implying 'origin/'.

Test Plan:
  - Pulled and imported a fresh copy of libphutil without issues.
  - Browsed various git repositories.
  - Browsed Javelin's various branches.
  - Ran upgrade script, got a bunch of clean 'origin/master' -> 'master'
conversions.
  - Tried to specify an explicit remote in a default branch name.
  - Unit tests.

Reviewers: nh, jungejason, btrahan

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T624

Differential Revision: https://secure.phabricator.com/D1269
2011-12-29 08:35:32 -08:00
epriestley
abd8efc358 Further relax same origin checks
Summary: Allow paths to match even if they differ by trailing slashes and
".git".

Test Plan: Ran unit tests.

Reviewers: jungejason, btrahan

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T710

Differential Revision: https://secure.phabricator.com/D1286
2011-12-26 12:01:59 -08:00
epriestley
bdbe9df65e Remove support for GitHub post-receive notifications
Summary:
  - These never actually did anything.
  - I don't even really remember why I built them, maybe the Open Source team
was pushing for more GitHub integration or something? I really have no idea.
  - Anyway, repository tailers do everything these could do (and much more).

Test Plan:
  - Ran tailers off GitHub for many months without needing post-receive hooks.
  - Grepped for relevant strings, couldn't find any references.
  - Used "Repository" edit interface for a Git repository.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T706

Differential Revision: https://secure.phabricator.com/D1273
2011-12-24 09:00:08 -08:00
epriestley
e786c44b6f Relax the same-origin check in the Git commit discovery daemon
Summary:
Git accepts either "git@x:/path" or "ssh://git@x.com/path" URIs to mean the
exact same thing, which is causing some false positives and confusion,
particularly because we sometimes mutate URIs.

Since this is just a sanity check, we don't really care about the username,
domain or credentials -- matching the paths is good enough. We're just trying to
make it hard to shoot yourself in the foot by copy-pasting the same local path
into two repositories and forgetting to change one, like I did. :P

Relax the check to only verify the paths are the same.

Test Plan:
  - Ran unit tests, which should fully cover things.
  - Ran commit discovery daemon in debug mode on incorrectly and correctly
configured repositories.

Reviewers: ajtrichards, jungejason, btrahan

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T710

Differential Revision: https://secure.phabricator.com/D1279
2011-12-24 08:54:44 -08:00
epriestley
3b65864ee1 Fix two minor bugs with recent patches:
- Use the computed remote URI (which may have an explicit 'ssh://' under Git in some cases).
  - Use '$id' correctly rather than casting the URI to an int in the message parser.
2011-12-22 06:57:04 -08:00
epriestley
b258095124 Expose symbol information over Conduit
Summary: I want to add a command like "where is ArcanistUnitTestEngine" to
phabot. I also want to add a symbol typeahead to Diffusion and generally finish
up that feature since it's useful but only half-implemented. Consolidate the
query logic and expose the data over Conduit.

Test Plan: Used /symbol/ and Conduit to lookup symbols.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T315

Differential Revision: https://secure.phabricator.com/D1260
2011-12-22 06:44:55 -08:00
epriestley
9d8b5481ae Emit full URIs to identify Differential revisions
Summary:
  - Previously, used IDs like "33" to match a commit to a Differential revision.
This has a namespacing problem because we now have an arbitrarily large number
of Phabricator installs in the world, and they may want to track commits from
other installs.
  - In Differential, parse raw IDs or full URIs. Emit only full URIs.
  - In Repositories, parse only full URIs.
  - This might cause a few commits to not be picked up in rare circumstances.
Users can fix them with "arc mark-committed". This should be exceedingly rare
because of hash matching.
  - There are some caveats for reparsing older repositories, see comments
inline. I don't think there's much broad impact here.

Test Plan:
  - Created a new revision, got a full URI.
  - Updated revision, worked correctly.
  - Ran unit tests.
  - Monkeyed with "Differential Revision" field.
  - Reviewers: btrahan, jungejason

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, epriestley, jungejason

Maniphest Tasks: T54, T692

Differential Revision: 1250
2011-12-20 19:58:22 -08:00
epriestley
92f9741779 Add a safety check to commit discovery daemon
Summary:
Although I couldn't repro the issue in T692, I did manage to point the "Diviner"
repository at the "Phabricator" working copy and screw some stuff up on
secure.phabricator.com.

Before discovering commits in a repository, ensure the 'origin' remote points at
the configured URI. This prevents issues where the working copy gets configured
to point at an existing (but incorrect) checkout.

Test Plan:
  - Ran gitcommitdiscovery daemon normally under "phd debug", saw it execute the
"remote show -n" command and then start working.
  - Intentionally botched the config, got an exception:

  (Exception) Working copy '/INSECURE/repos/phabricator' has origin URL
  'ssh://git@github.com/facebook/phabricator.git', but the configured URL
  'git://github.com/facebook/diviner.git' is expected. Refusing to proceed.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T692

Differential Revision: 1253
2011-12-20 19:52:08 -08:00
jungejason
c80d1480d5 Add Basic Auditing Functionalities
Summary:
add basic auditing functionalities. For the related commits for a
package, we detect the following conditions which might be suspicious to the
owners of the package:

* no revision specified
* revision not found
* author not match
* reviewedby not match
* owners not involved
* commit author not recognized

The owners of the package can change the status of the audit entries by
accepting it or specify concern.

The owner can turn on/off the auditing for a package.

Test Plan:
*  verified that non-owner cannot see the details of the audit and cannot modify
it
*  verified that all the audit reasons can be detected
*  tested dropdown filtering and package search
*  verified really normal change not detected
*  verified accept/concern a commit
*  tested enable/disable a package for auditing
*  verified one audit applies to all <commit, packages> to the packages the
auditor owns
*  verified that re-parsing a commit won't have effect if there exists a
 relationship for <commit, package> already

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, benmathews, btrahan, mpodobnik, prithvi, TomL, epriestley

Differential Revision: 1242
2011-12-20 13:36:53 -08:00
epriestley
b5c9b9d059 Use remote credentials for 'git fetch' and 'hg pull' commands
Summary: These are "local" commands, but need remote credentials. If the daemon
runs as a user who does not have credentials, the initial clone will work but
subsequent updates will fail.

Test Plan:
  - Nuked a local copy of a Git repo.
  - Ran "phd debug fetch <phid>" as root (or any other user with no natural SSH
keys). Verified initial clone worked (since it passes credentials to the command
correctly).
  - Killed daemon, re-ran, verified "fetch" failed (no credentials passed).
  - Applied this patch.
  - Re-ran "phd debug fetch <phid>", verified it passed credentials and
succeeded.
  - Did all these steps for a Mercurial repo.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T686

Differential Revision: 1236
2011-12-20 08:28:38 -08:00
jungejason
c13b7da290 Add Related Commits for Owners
Summary:
For each commit, find the affected packages, and provide a way to
search by package.

Test Plan:
create commits that touch and don't touch two packages, and verify
that they display correctly in all the UI pages.

Reviewers: epriestley, blair, nh, tuomaspelkonen

Reviewed By: epriestley

CC: benmathews, aran, epriestley, btrahan, jungejason, mpodobnik, prithvi

Maniphest Tasks: T83

Differential Revision: 1208
2011-12-14 22:48:57 -08:00
Marek Sapota
4ef18d35ac Fix error introduced with D1195.
Summary:
Not my best commit=/

Reviewers: epriestley

Test plan:
After the change commit parser started working again.

Differential Revision: 1206
2011-12-13 20:37:18 -08:00
epriestley
51b8168253 Fix fatal in commit message parser
Summary:
See D1195, which fataled this daemon.

https://secure.phabricator.com/daemon/log/2966/

Test Plan: Applied this patch to secure.phabricator.com, restarted daemon, it
picked up D1203.

Reviewers: btrahan, jungejason, mareksapota

Reviewed By: btrahan

CC: aran, btrahan, mareksapota

Differential Revision: 1204
2011-12-13 18:56:26 -08:00
Marek Sapota
9292cfd6a3 Recognise better who committed a revision.
Test Plan: Commit as not the author and see what shows up.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, mareksapota, zeeg

Differential Revision: 1195
2011-12-13 14:00:13 -08:00
Bob Trahan
588b959c03 phabricator_format_timestamp => phabricator_datetime
Summary: make the change, kill the function.   be sure to get a good $user or
$viewer variable

Test Plan:
for each controller or view, look at it in the ui.   change timezone, refresh ui
and note change.   i did not test the OAuthSettingsPanelController; not sure how
to get to that badboy and i got a bit lazy

Maniphest Tasks: T222

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, btrahan, epriestley

Maniphest Tasks: T222

Differential Revision: 1166
2011-12-04 08:51:34 -08:00
John Stockdale
501c90bb30 Use Git's encoding flag instead of MBString
Summary:
0d5b0f21ad added string conversion but MBString always needs an argument for endcoding.

It looks like we can get away with doing this in git instead, with the --encoding='UTF-8' flag. Then we should be safe to remove the test for output type, and stay UTF-8 safe.

Test Plan:
Run updaters with change. Verify commits are updated.

Reviewers: epriestley

CC:

Differential Revision: 1108
2011-11-12 01:01:41 +00:00
David Reuss
0d5b0f21ad Convert author/message encoding if not UTF-8
Test Plan:
used the reparse.php script for reparsing commit messages and saw the
correct author name (and mapped correctly as a phabricator user) in diffusion.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 1059
2011-10-28 08:06:03 -07:00
David Reuss
4e900c096f Convert "falsey" binary hunks if we have a repository encoding
Summary:
This adds an encoding detail to the repository, so we can attempt to
convert hunks previously detected as binary.

We also add the encoding information to the arcanist projectinfo
API so we can pull the information if we have it when uploading changes
via arc.

Test Plan:
Changed encoding through the edit UI, and saw "This is binary file", and
changed it back and saw the correct output from the diff.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 1009
2011-10-28 08:04:57 -07:00
Edward Speyer
6737ae4828 Run a commit discovery daemon only once
Summary:
Discover commits then return; useful when initializing new repositories
in unit tests.

By which I mean "when initializing a new repository in my unit test that
I'm working on".

Test Plan: Using this in a PhabricatorTestCase.

Reviewers: epriestley, aran

Reviewed By: epriestley

CC: aran, edward, epriestley

Differential Revision: 948
2011-10-25 14:25:40 -07:00