Summary:
Ref T4195. To implement the "Author" and "Committer" rules, I need to resolve author/committer strings into Phabricator users.
The code to do this is currently buried in the daemons. Extract it into a standalone query.
I also added `bin/repository lookup-users <commit>` to test this query, both to improve confidence I'm getting this right and to provide a diagnostic command for users, since there's occasionally some confusion over how author/committer strings resolve into valid users.
Test Plan:
I tested this using `bin/repository lookup-users` and `reparse.php --message` on Git, Mercurial and SVN commits. Here's the `lookup-users` output:
>>> orbital ~/devtools/phabricator $ ./bin/repository lookup-users rINIS3
Examining commit rINIS3...
Raw author string: epriestley
Phabricator user: epriestley (Evan Priestley )
Raw committer string: null
>>> orbital ~/devtools/phabricator $ ./bin/repository lookup-users rPOEMS165b6c54f487c8
Examining commit rPOEMS165b6c54f487...
Raw author string: epriestley <git@epriestley.com>
Phabricator user: epriestley (Evan Priestley )
Raw committer string: epriestley <git@epriestley.com>
Phabricator user: epriestley (Evan Priestley )
>>> orbital ~/devtools/phabricator $ ./bin/repository lookup-users rINIH6d24c1aee7741e
Examining commit rINIH6d24c1aee774...
Raw author string: epriestley <hg@yghe.net>
Phabricator user: epriestley (Evan Priestley )
Raw committer string: null
>>> orbital ~/devtools/phabricator $
The `reparse.php` output was similar, and all VCSes resolved authors correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1731, T4195
Differential Revision: https://secure.phabricator.com/D7801
Summary: Ref T4195. Even though we use `svnlook` in the hook itself, I need this query elsewhere, so provide it and merge the classes into one which does the right thing.
Test Plan:
- Used `reparse.php` to reparse messages for Git, SVN and Mercurial commits, using `var_dump()` to examine the commit refs for sanity.
- Used `reparse.php` to reparse changes for an SVN commit.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7800
Summary: Ref T4249. Currently, a global rule can only trigger project audits. Although there probably aren't a huge number of use cases for triggering users from global rules, it works fine and it's somewhat confusing not to allow it.
Test Plan: {F90902}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4249
Differential Revision: https://secure.phabricator.com/D7803
Summary: There were a number of places that were generating nonsense queries for both hosted and non-hosted subversion repositories.
Test Plan: Attempted several activities in Diffusion with both a hosted and non-hosted subversion repository, including viewing various types of diffs and raw files.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7799
Summary: Ref T4010. I'll hold this for a bit, but we should eventually drop this table once the dust has settled.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4010
Differential Revision: https://secure.phabricator.com/D7372
Summary: If you push a large binary and the data crosses multiple data frames, we can end up in a loop in the parser.
Test Plan:
After this change, I was able to push a 95MB binary in 7s, which seems reasonable:
>>> orbital ~/repos/INIS $ svn st
A large2.bin
>>> orbital ~/repos/INIS $ ls -alh
total 390648
drwxr-xr-x 6 epriestley admin 204B Dec 18 17:14 .
drwxr-xr-x 98 epriestley admin 3.3K Dec 16 11:19 ..
drwxr-xr-x 7 epriestley admin 238B Dec 18 17:14 .svn
-rw-r--r-- 1 epriestley admin 80B Dec 18 15:07 README
-rw-r--r-- 1 epriestley admin 95M Dec 18 16:53 large.bin
-rw-r--r-- 1 epriestley admin 95M Dec 18 17:14 large2.bin
>>> orbital ~/repos/INIS $ time svn commit -m 'another large binary'
Adding (bin) large2.bin
Transmitting file data .
Committed revision 25.
real 0m7.215s
user 0m5.327s
sys 0m0.407s
>>> orbital ~/repos/INIS $
There may be room to improve this by using `PhutilRope`.
Reviewers: wrotte, btrahan, wotte
Reviewed By: wotte
CC: aran
Differential Revision: https://secure.phabricator.com/D7798
Summary: Ref T4195. Same as D7793, but for mercurial. (As usual, SVN needs some goofy nonsense instead, so the next diff will just make this field work.)
Test Plan: Ran `reparse.php` on Git and Mercurial commits, var_dump'd the output and it looked correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7795
Summary: Ref T4195. I need to issue this command from the pre-commit hook to get commit bodies for hooks.
Test Plan: Ran `reparse.php --message --trace` and dumped the $ref, which looked correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7793
Summary: We currently have a lot of calls to `addCrumb(id(new PhabricatorCrumbView())->...)` which can be expressed much more simply with a convenience method. Nearly all crumbs are only textual.
Test Plan:
- This was mostly automated, then I cleaned up a few unusual sites manually.
- Bunch of grep / randomly clicking around.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: hach-que, aran
Differential Revision: https://secure.phabricator.com/D7787
Summary: Ref T4195. Add Mercurial support to the content hook phase.
Test Plan:
Here are some `commit` push logs for a Mercurial repo:
{F90689}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7792
Summary: Ref T4195. Adds support for diff content rules.
Test Plan: Pushed SVN and Git changes through, saw them generate reasonable transcripts. Mercurial still isn't hooked up to this phase.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7791
Summary: Ref T4195. This doesn't provide any interesting fields yet (content, affected paths, commit message) but fires the hook correctly.
Test Plan: Added a blocking hook and saw it fire.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7789
Summary: Allow Herald rules to be referred to with `H123`, etc., like other object types are. Herald rules now have proper PHIDs and an increasingly prominent role in triggering application actions. Although I suspect users will rarely use `H123` in Remarkup to mention rules, this can simplify some of the interfaces which relate objects across systems.
Test Plan: Looked at various interfaces and saw `H123` names. Mentioned `H123` in remarkup.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7786
Summary:
These just got copy/pasted like crazy, the base class has the correct default implementation.
(I'm adding "H" for Herald Rules, which is why I was in this code.)
I also documented the existing prefixes at [[ Object Name Prefixes ]].
Test Plan: Verified base implementation. Typed some object names into the jump nav.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Differential Revision: https://secure.phabricator.com/D7785
Summary:
A few users have hit cases where Herald transcripts of large commits exceed the MySQL packet limit, because one of the fields in the transcript is an enomrous textual diff.
There's no value in saving these huge amounts of data. Transcripts are useful for understanding the action of Herald rules, but can be reconstructed later. Instead of saving all of the data, limit each field to 4KB of data.
For strings, we just truncate at 4KB. For arrays, we truncate after 4KB of values.
Test Plan: Ran unit tests. Artificially decreased limit and ran transcripts, saw them truncate properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: frgtn, aran
Differential Revision: https://secure.phabricator.com/D7783
Summary:
Ref T615. Ref T4237. With `--debug`, Mercurial will echo an "ignoring untrusted configuration option" warning **to stdout** if `.hgrc` has the wrong owner.
However, we need `--debug` to make `{parents}` usable, at least until the patches I got into the upstream are widely deployed. So after getting `--debug` output, strip off any leading warnings.
These warnings should always be in English, at least, since we set `LANG` explicitly.
Test Plan: Unit tests. @asherkin, maybe you can confirm this? I can't actually get the warning, but I think my `hg` in PATH is just a bit out of date.
Reviewers: asherkin, btrahan
Reviewed By: asherkin
CC: asherkin, aran
Maniphest Tasks: T615, T4237
Differential Revision: https://secure.phabricator.com/D7784
Summary: Ref T4195. Herald rules gained PHIDs only recently, propagate them to HeraldEffect to make some of the hook stuff eaiser.
Test Plan: iiam
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7781
Summary: Ref T4195. We need these in Herald, since HeraldTranscripts need to refer to a PHID which they acted upon.
Test Plan:
Ran migration, got PHIDs:
mysql> select phid from repository_pushlog limit 3;
+--------------------------------+
| phid |
+--------------------------------+
| PHID-PSHL-25jnc6cjgzw5rwqgmr7r |
| PHID-PSHL-2vrvmtslkrj5yv7nxsv2 |
| PHID-PSHL-34x262zkrwoka6mplony |
+--------------------------------+
3 rows in set (0.00 sec)
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7780
Summary: Ref T4195. SVN has no such thing as refs (I was thinking about writing a quasi-ref anyway like `HEAD: r23 -> r24`, but I'm not sure it would actually be useful). And content is very easy to build.
Test Plan: Pushed some stuff to SVN, got logs from it.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7766
Summary: Ref T4195. This doesn't actually work like I thought it did: it only fires locally, when you run `hg tag`. Mercurial tags are also weird and basically don't make any sense and everyone should use bookmarks instead. We could implement some flavor of this eventually, but I'd like to see users request it first. They can implement their own with content-based hooks once those work, anyway.
Test Plan: This code didn't do anything.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7765
Summary:
Ref T4195. This pulls the central logic of HookEngine up one level and makes all the git stuff genrate PushLogs.
In future diffs, everything will generate PushLogs and we can hand those off to Herald.
Test Plan:
Pushed a pile of valid/invalid stuff:
{F89256}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7761
Summary: Fixes T4241. Ref T4206. See T4241 for a description here. Generally, when we connect a fat pipe (`git-upload-pack`) to a narrow one (`git` over SSH) we currently read limitless data into memory. Instead, throttle reads until writes catch up. This is now possible because of the previous changes in this sequence.
Test Plan:
- Ran `git clone` and `git push` on the entire Wine repository.
- Observed CPU and memory usage.
- Memory usage was constant and low, CPU usage was high only during I/O (which is expected, since we have to actually do work, although thre might be room to further reduce this).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4241, T4206
Differential Revision: https://secure.phabricator.com/D7776
Summary: Until we implement an "enum" type for config, make this a bit harder to get wrong. A user entered "TLS", but the correct value is "tls". The documentation is consistent about this, but the behavior is sitll surprsing.
Test Plan: eyeballed it
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7778
Summary:
Fixes T4239. Currently, if you go to `/maniphest/?authors=alincoln`, operations dependent on the query key (like "Save Custom Query..." and "Export to Excel...") don't have a query key to work with. Make sure they have one.
Also remove a stray `phlog()`.
Test Plan: "Save Custom Query...", etc., now work on GET queries.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4239
Differential Revision: https://secure.phabricator.com/D7777
Summary: Ref T4189. Updates the Phabricator stuff to use the new, more sensible semantics from D7769. Basically, this works correctly now and doesn't need workarounds.
Test Plan: Pushed Wine repo in 1m13s.
Reviewers: btrahan, zeeg
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4189
Differential Revision: https://secure.phabricator.com/D7770
Summary: Not every revision belongs to a repository, so we might end up here with `$repo` still equal to `null`. Don't fatal if we do.
Test Plan: iiam
Reviewers: btrahan, hach-que, zeeg
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7771
Summary: Patches can exceed the 30 second time out in most PHP installations. This removes the patch preview from the version controller so that users can still see the information (although they may not be able to download the actual patch).
Test Plan: Viewed a version and saw that the patch didn't appear.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7767
Summary: This implements support for enforcing and setting policies in Phragment.
Test Plan: Set policies and ensured they were enforced successfully.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7751
Summary:
Ref T4107. Two issues:
- With strict MySQL settings, we try to insert `null` into the non-nullable `messageCount` field. Add an `initializeNew...` method.
- If we don't create a new conpherence (for example, because the message body is empty), we fatal on `getPHID()` right now.
Also, make this stuff a little easier to test.
Test Plan: Used `mail_handler.php` to receive empty conpherence mail, and new-thread conpherence mail.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4107
Differential Revision: https://secure.phabricator.com/D7760
Summary:
See <https://github.com/facebook/phabricator/issues/467>. @dctrwatson also ran into an issue where we were trying to `setPass()` a GitURI.
- For Git and Mercurial, properly generate credential URIs where relevant.
- Don't try to `setPass()` on Git-style URIs.
This isn't perfect but should clean things up a bit.
Test Plan: Added unit tests. Lots of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: dctrwatson, aran
Differential Revision: https://secure.phabricator.com/D7759
Summary: This is a small fix for Phortune so that policies don't prevent the user accounts from being implicitly created when they first visit Phortune.
Test Plan: Visited Phortune and it worked.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7758
Summary: Fixes T4224. If you `git merge-base A B`, and they have //no// ancestor, the command exits with an error. Assume errors mean "no ancestry" and continue.
Test Plan: Completely rewrite a repository with a `--force` push.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4224
Differential Revision: https://secure.phabricator.com/D7756
Summary: Fixes T4225. Adds the NON_EXISTS condition to Herald for "Reviewers", and adds a few more conditions which have reasonable meanings.
Test Plan: Used test console to check a revision with reviewers, and another without reviewers. Both produced the expected results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4225
Differential Revision: https://secure.phabricator.com/D7757
Summary: We run `git` on a different port than 22, so would like to reflect this change in the UI.
Test Plan: Set diffusion.ssh-port in settings, then make sure it's reflected on the Diffusion repository Clone URI.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D7755
Summary: Fixes T4223. The output of `ls-tree` is partially delimited by spaces
and partially delimited by `\t`. The code I added in D7744 to help debug the
issue in T4159 doesn't work properly for files with 7 or more bytes in their
filesize, because the internals use `%7s`.
Auditors: btrahan
Summary:
This provides a `phragment.getstate` and a `phragment.getpatch` Conduit method.
`phragment.getstate` - This returns the current state of the fragment and all of it's children.
`phragment.getpatch` - This accepts a base path and a mapping of paths to hashes. The mapping is for the caller to specify the current state of the files it has. This returns a list of patches that the caller needs to apply to it's files to get to the latest version.
Test Plan:
Ran the following script in a folder which had content matching a fragment and it's children:
```
#!/bin/bash
STATE=""
for i in $(find ./ -type f); do
HASH=$(cat $i | sha1sum | awk '{ print $1 }')
BASE=${i:2}
STATE="$STATE,\"$BASE\":\"$HASH\""
done
STATE=${STATE:1}
STATE="{$STATE}"
echo '{"path":"tychaia3.zip","state":'$STATE'}' | arc --conduit-uri=http://phabricator.local/ call-conduit phragment.getpatch
```
and I got:
```
{"error":null,"errorMessage":null,"response":[]}
```
I updated one of the child fragments with a new file and ran the script again (patch has been omitted due to it's size):
```
{"error":null,"errorMessage":null,"response":[{"path":"Content\/TitleFont.xnb","hash_old":"4a927d7b90582e50cdd330de9f4b59b0cc5eb5c7","hash_new":"25867504642a3a403102274c68fbb9b430c1980f","patch":"..."}]}
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, staticshock
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7739
Summary: This adds a build step which will block a build from continuing if there are previous builds of the build plan still running.
Test Plan: Configured a build plan with a wait of 60 seconds and a "wait for previous builds", then started a build. While that was still building, reconfigured the plan to have a wait time of 3 seconds, started it, and saw it move into the "Waiting" status. When the 60 second build finished, both builds passed.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7745
Summary:
This came up recently in a discussion with @lifeihuang, and then tangentally with @hach-que. Make it easier for users to get a sense of whether they might need to add more daemons. Although we've improved the transparency of daemons, it's not easy for non-experts to determine at a glance how close to overflowing the queue is.
This number is approximate, but should be good enough for determining if your queue is more like 25% or 95% full.
If this goes over, say, 80%, it's probably a good idea to think about adding a couple of daemons. If it's under that, you should generally be fine.
Test Plan: {F88331}
Reviewers: btrahan, hach-que, lifeihuang
Reviewed By: btrahan
CC: hach-que, lifeihuang, aran, chad
Differential Revision: https://secure.phabricator.com/D7747
Summary: Most checks were actually in place, but `ExecFuture` throws a `CommandException` which wasn't taken into account.
Test Plan: look at the first command and no longer saw an exception. Also, other commits worked as well.
Reviewers: richardvanvelzen
Reviewed By: richardvanvelzen
CC: krisbuist, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7730
Summary:
Ref T1049. See discussion in D7745. We have some specific interest in this for D7745, but generally we want to consume tasks with expired leases in roughly FIFO order, just like we consume new tasks in roughly FIFO order. Currently, when we select an expired task we order them by `id`, but this is the original insert order, not lease expiration order. Instead, order by `leaseExpires`.
This query is actually much better than the old one was, since the WHERE part is `leaseExpries < VALUE`.
Test Plan: Ran `EXPLAIN` on the query. Ran a taskmaster in debug mode and saw it lease new and expired tasks successfully.
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7746
