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
Summary: This adds a build step in Harbormaster for publishing file artifacts as fragments in Phragment.
Test Plan:
Created a build plan with the following steps:
* Lease Host
* Upload Artifact
* Publish Fragment
Ran the build plan against a buildable and saw the fragment get created in Phragment. Ran the plan again and saw the fragment get updated with a new version. Modified the file that got uploaded and ran the plan again, checked the history of the fragment, and saw the differences represented as a Diff-Match-Patch patch.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7742
Summary:
Ref T4212. This implements snapshots in Phragment, which allows you to take a snapshot of a fragment at a given point in time, and download a ZIP of the snapshot as it was in this state.
There's also functionality for deleting and promoting snapshots. You can promote a snapshot to either the latest version or any other snapshot of the fragment.
Test Plan: Clicked around, took some snapshots, promoted them to different points and deleted snapshots. Also downloaded ZIPs of the snapshots and saw the right versions coming through for all the files downloaded.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205, T4212
Differential Revision: https://secure.phabricator.com/D7741
Summary:
This functionality allows users to revert a fragment to a previous version from the history page.
Reverting a version actually creates a new version pointing at the same file as the version being "reverted" to. In this sense it acts pretty much like Git and other distributed VCS where once you have published a commit the only way to undo your changes is to create a new commit that reverts those changes.
Test Plan: Reverted a fragment to a version before it was deleted, then reverted it to when it was deleted and saw the new versions have the correct file PHIDs (including null for the deletion).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7738
Summary: This updates Phragment so that fragments that are currently considered deleted have a disabled status and have an additional attribute 'Deleted'. It also places this effect on versions (in the history controller) that actually involve deleting the file.
Test Plan: Viewed deleted fragments and versions.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7737
Summary: When the code to update based on ZIP went to look up the child fragments, it explicitly used the paths provided in the ZIP. This meant that we could never detect omissions because there'd never be a scenario where a child fragment would return but not exist in the ZIP. To fix this, the query should be using `withLeadingPath` instead of `withPaths`.
Test Plan: Uploaded a ZIP that omitted a file and saw the `deleteFile` get called (by placing debugging output in the code).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7736
Summary: This logic causes an exception because getPHID() is called on a fragment that has no latest version. This fixes the code so that in this scenario, it returns an empty array (with no path to be added to the ZIP).
Test Plan: Downloaded the ZIP successfully after the patch was applied.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7735
Summary:
This adds support for viewing individual versions on a fragment as well as comparing versions and downloading diff_match_patch-based patches.
It does not use the side-by-side diff format as while it works for small changes, it quickly becomes impossible to distingush what changes have been made due to the diff_match_patch format.
Test Plan: Clicked on versions and downloaded patches.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7734
Summary: This fixes the update-from-ZIP functionality so that it will automatically detect directories in the ZIP that do not have explicit entries. Some ZIP programs do not create directory entries explicitly, so if we fail to do this then there's no way for users to access the sub-fragments (even though they exist, there is no directory fragment to click through).
Test Plan: Created and updated fragments from a ZIP that had implicit directories in it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, staticshock
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7733
Summary:
This implements support for creating and updating fragments from ZIP files. It allows you to upload a ZIP via the Files application, create a fragment from it, and have it recursively imported into Phragment. Updating that folder with another ZIP will recursively create, update and delete files as appropriate.
The logic for creating and updating fragments from files has also been centralized into the PhragmentFragment class. Directories are also now supported; a directory fragment is simply a fragment that has no patches; thus a directory fragment can be converted to a file fragment by uploading a first patch for it.
Test Plan: Uploaded ZIP files through the interface and saw all of the fragments get created and updated as expected.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7729
Summary: Depends on D7727. This adds support for downloading a fragment and all it's children as a ZIP file. Fragments that have children automatically become directories in the ZIP file.
Test Plan: Downloaded a fragment as a ZIP and was able to extract the contents successfully.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7728
Summary: Depends on D7726. This adds a history controller (for viewing a list of patches associated with a fragment) and an update controller, for creating a new patch of a fragment.
Test Plan: Updated and viewed history of fragments.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7727
Summary: Ref T4205. This is an initial implementation of Phragment. You can create and browse fragments in the system (but you can't yet view a fragment's patches / history).
Test Plan: Clicked around and created fragments.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7726
Summary:
If you do something like this:
// Missing $user->getPHID()!
$object->setUserPHID($user)->save();
...you get a very unhelpful exception:
Expected a scalar or null for %s conversion. Query: %s
This doesn't give you any hints about what's wrong. Instead, provide a more useful exception:
Unable to insert or update object of class DifferentialRevision, field 'title' has a nonscalar value.
Test Plan: {F87614}
Reviewers: hach-que, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7725
Summary:
A user sent a message to Phabricator which looked like:
On blah blah blah ?
On <date>, <user> wrote:
> blah blah blah
The current algorithm is too aggressive and thinks lines 1-3 are //all// the "On ... wrote:" string. Instead, patch only the most recent "On".
Test Plan: Added a failing test and made it pass.
Reviewers: btrahan, zeeg
Reviewed By: zeeg
CC: aran
Differential Revision: https://secure.phabricator.com/D7732
