{"push_id":1124753803,"size":44,"distinct_size":44,"ref":"refs/heads/master","head":"cd3c08f742f6d7bc6f669447158d6a9086206597","before":"d9a8d7259ffe2d6d7abffa77b6735cc9621ee060","commits":[{"sha":"03a1deba23f60312dc1fca06290de47436cdd4fb","author":{"email":"c34c009102e60d77975598e1f414f8a701c22d0b@chadsdomain.com","name":"Chad Little"},"message":"Remove hard-coding of diff line height\n\nSummary: Fixes T10959. This is the smallest/simplest fix that I could come up with, and I wasn't able to break it. Basically, I removed \"line-height\" and then adjusted other rules until the defaults looked reasonable again.\n\nTest Plan:\nHere's `24px / 48px impact` or something like it:\n\n{F1310445}\n\n{F1310446}\n\nHere's normal stuff working properly without weird artifacts on the highlighting:\n\n{F1310447}\n\nAlso tested Firefox and Chrome and got similar results.\n\nReviewers: epriestley\n\nReviewed By: epriestley\n\nSubscribers: wxm20073527, Korvin\n\nManiphest Tasks: T10959\n\nDifferential Revision: https://secure.phabricator.com/D15905","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/03a1deba23f60312dc1fca06290de47436cdd4fb"},{"sha":"3a727c31e2f96665081e68ccbcd562701c26f5e3","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Modernize DifferentialRevisionSearchEngine\n\nSummary:\nRef T10939. Ref T4144. This moves the revision SearchEngine to modern code so I can add some kind of bucketing layer on top of it.\n\nThis seems to have worked pretty cleanly. One thing is that I removed the ability to search for \"pending drafts\":\n\n  - This was added in D1927 from a bootcamp task, was an indirect solution to a questionable problem, and almost certainly would not meet the bar today.\n  - Later, in D3324, we added the icons to the list. I think this is a better solution in general. In particular, it specifically addressed the query being kind of junky.\n  - At the time, Differential had a prebuilt \"Drafts\" filter. This was removed in D6347 with the move to ApplicationSearch, which simplified the large number of prebuilt filters. Although we got a lot of feedback about that, none requested that the drafts filter be restored.\n\nTest Plan: Searched for responsible users, subscribers, orders, projects, repositories.\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T4144, T10939\n\nDifferential Revision: https://secure.phabricator.com/D15921","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/3a727c31e2f96665081e68ccbcd562701c26f5e3"},{"sha":"eade206625ea1ec1e6946c379c9c24eb7c629534","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Introduce search result buckets\n\nSummary:\nRef T10939. Currently, Differential hard-codes some behaviors for the \"active\" filter. This introduces \"buckets\" to make this grouping behavior more general/flexible.\n\nThe buckets don't actually do any grouping yet, this just gets rid of the `$query === 'active'` stuff so far.\n\nThese buckets change the page size to a large value, becuase pagination won't currently work with bucketing.\n\nThe problem is that we normally paginate by selecting one more result than we need: so if we're building a page of size 10, we'll select 11 results. This is fast, and if we get 11 back, we know there's a next page with at least one result on it.\n\nWith buckets, we can't do this, since our 11 results might come back in these buckets:\n\n  - A, B, C, A, C, C, A, A, B, B, (B)\n\nSo we know there are more results, and we know that bucket B has more results, but we have no clue if bucket A and bucket C have more results or not (or if there's anything in bucket D, etc).\n\nWe might need to select a thousand more results to get the first (D) or the next (A).\n\nSo we could render something like \"Some buckets have more results, click here to go to the next page\", but users would normally expect to be able to see \"This specific bucket, A, has more results.\", and we can't do that without a lot more work.\n\nIt doesn't really matter for revisions, because almost no one has 1K of them, but this may need to be resolved eventually.\n\n(I have some OK-ish ideas for resolving it but nothing I'm particularly happy with.)\n\nTest Plan: {F1376542}\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T10939\n\nDifferential Revision: https://secure.phabricator.com/D15923","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/eade206625ea1ec1e6946c379c9c24eb7c629534"},{"sha":"42d49be47bfcbad59ce0634f06a99ba43a40d389","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Change Differential revision buckets to focus on \"next required action\"\n\nSummary:\nRef T10939. Ref T4144. This splits the existing buckets (\"Blocking Others\", \"Action Required\", \"Waiting on Others\") into 6-7 buckets with a stronger focus on what the next action you need to take is.\n\nSee T10939#175423 for some discussion.\n\nOverall, I think some of the root problems here are caused by reviewer laziness and shotgun review workflows (where a ton of people get automatically added to everything, probably unnecessarily), but these buckets haven't been updated since the introduction of blocking reviewers or project/package reviewers and I think splitting the 3 buckets into 6 buckets isn't unreasonable, even though it's kind of a lot of buckets and the root problem here is approximately \"I want to ignore a bunch of stuff on my dashboard\".\n\nI didn't remove the old bucketing code yet since it's still in use on the default homepage.\n\nThis also isn't quite right until I fix the tokenizer to work properly, since it won't bucket project/package reviewers accurately.\n\nTest Plan: {F1395972}\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T4144, T10939\n\nDifferential Revision: https://secure.phabricator.com/D15924","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/42d49be47bfcbad59ce0634f06a99ba43a40d389"},{"sha":"d46378df208f2fe7b8f9df0586122f154d7bee65","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Modernize \"Responsible Users\" tokenizer and add \"exact(user)\" token\n\nSummary:\nRef T10939. Fixes T9263. Ref T4144.\n\nFirst, this resolves users (converting users into all packages and projects they are responsible for) earlier, so bucketing can act on that data correctly. Previously, your own blocking reviews would appear in \"Must Review\" but your packages/projects' would not. Now, all of them will.\n\nSecond, this adds `exact(username)` to mean \"just me, not my packages/projects\". You can use this along with \"Bucket: By Required Action\" to create a personal view of \"Active Revisions\" if you'd like, and ignore all your project/package reviews.\n\nTest Plan: Queried by \"me\" and \"exact(me)\", got reasonable looking results.\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T4144, T9263, T10939\n\nDifferential Revision: https://secure.phabricator.com/D15925","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/d46378df208f2fe7b8f9df0586122f154d7bee65"},{"sha":"8a98868bfbc44ef887b81638dad6a8b5a0d3ede5","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Remove \"days fresh\" / \"days stale\" indictator in Differential revision list\n\nSummary:\nRef T10939. I'm not //totally// opposed to the existence of this element, but I think it's the kind of thing that would never make it upstream today. I think this should just be a T418 custom sort of thing in the long run, not a mainline upstream feature.\n\nOverall, I think this thing is nearly useless and just adds visual clutter. My dashboard is about 100% red. This also sort of teaches users that it's fine to let revisions sit for a couple of days, which isn't what I'd like the UI to teach. Finally, removing it helps the UI feel a little less cluttered after the visually busy changes in D15926.\n\nTest Plan: Grepped for removed config. Viewed revision list.\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T10939\n\nDifferential Revision: https://secure.phabricator.com/D15927","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/8a98868bfbc44ef887b81638dad6a8b5a0d3ede5"},{"sha":"6e9828c743a7876f538037a6dfbd59ae6e10bbf0","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Fix a dashboard bucketing bug\n\nSummary: Ref T10939. For various historical reasons, revision status is a numeric string. This comparison fails because it's `(string) !== (int)`. Just use `!=` so this will still work if we turn it into a real string in the future.\n\nTest Plan: Tried a more specific test case locally, got better looking results in \"Must Review\" and \"Should Review\".\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T10939\n\nDifferential Revision: https://secure.phabricator.com/D15928","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/6e9828c743a7876f538037a6dfbd59ae6e10bbf0"},{"sha":"c5853b4f4820560fc385ee49de8c9fc22f2ad47f","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Put revisions you're a reviewer on which need review and which you've commented on in \"Should Review\"\n\nSummary: Ref T10939. These poor stragglers got left out in the rain. Didn't catch any issues otherwise.\n\nTest Plan: {F1426604}\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T10939\n\nDifferential Revision: https://secure.phabricator.com/D15929","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/c5853b4f4820560fc385ee49de8c9fc22f2ad47f"},{"sha":"bf5437212cc85bf2079c4bcfd796af26a0bba6ae","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"When a revision is accepted but has open dependencies, show a note in the list UI\n\nSummary:\nRef T10939. I don't think this is hugely important, but it doesn't clutter things up much and it's nice as a hint.\n\nT4055 was the original request specifically asking for this. It wanted a separate bucket, but I think this use case isn't common/strong enough to justify that.\n\nI would like to improve Differential's \"X depends on Y\" feature in the long term. We don't tend to use/need it much, but it could easily do a better and more automatic job of supporting review of a group of revisions.\n\nTest Plan: {F1426636}\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T10939\n\nDifferential Revision: https://secure.phabricator.com/D15930","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/bf5437212cc85bf2079c4bcfd796af26a0bba6ae"},{"sha":"174f3f6d2366967b459876572438f519d556555b","author":{"email":"6847c34346fa72e5a886a3c4baab59942beebe92@gmail.com","name":"Luka Kladaric"},"message":"docs: fix setup instructions for Mailgun and S3\n\nSummary: The S3 fields are mandatory and if you only enter the ones in the docs you will immediately encounter a \"Amazon S3 is Only Partially Configured\" error. For Mailgun the error is more difficult to figure out - emails get stuck on Mailgun's side but the error is illegible. On Phabricator's side you have to go trawling through nginx logs to find \"Mail signature is not valid. Check your Mailgun API key.\"\n\nTest Plan: Deploy a new standalone instance, follow old instructions, fail. Deploy another one, follow updated instructions, win.\n\nReviewers: epriestley, #blessed_reviewers\n\nReviewed By: epriestley, #blessed_reviewers\n\nSubscribers: Korvin\n\nDifferential Revision: https://secure.phabricator.com/D15931","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/174f3f6d2366967b459876572438f519d556555b"},{"sha":"f930a43f91548d20ba1974b9b16d7c00723359da","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Remove \"Used By\" from Passphrase\n\nSummary: Fixes T10972. Nothing actually updates this anymore, and only repositories ever did (e.g., Harbormaster and Drydock have never tracked it). Keeping track of this is more trouble than it's worth.\n\nTest Plan: Grepped for constants, viewed a passphrase credential.\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T10972\n\nDifferential Revision: https://secure.phabricator.com/D15932","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/f930a43f91548d20ba1974b9b16d7c00723359da"},{"sha":"875b86671566a994f899777a06f401ba1d56297d","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Add missing \"oauth_server_edge\" tables\n\nSummary: Fixes T10975. The \"scramble attached file permissions when an object is saved\" code is misfiring here too. See T10778 + D15803 for prior work.\n\nTest Plan:\n  - Ran `bin/storage upgrade -f`.\n  - Edited the view policy of an OAuth server (prepatch: fatal; postpatch: worked great).\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T10975\n\nDifferential Revision: https://secure.phabricator.com/D15938","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/875b86671566a994f899777a06f401ba1d56297d"},{"sha":"afec01129a09122aebb97c39409ab4606a9d3740","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Allow blocking reviewers to be added via the web UI\n\nSummary:\nRef T10939. Adds a `blocking(...)` token.\n\nThis code is pretty iffy and going to get worse before it gets better, but the fix (T10967 + EditEngine) is going to be a fair chunk of work down the road.\n\nTest Plan: {F1426966}\n\nReviewers: chad\n\nReviewed By: chad\n\nSubscribers: scode\n\nManiphest Tasks: T10939\n\nDifferential Revision: https://secure.phabricator.com/D15933","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/afec01129a09122aebb97c39409ab4606a9d3740"},{"sha":"29a060d7f13fa12510772feb6a9c0978b6c924c7","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Allow blocking reviewers to be added via the CLI\n\nSummary: Ref T10939. Fixes T4887. Supports \"username!\" to add a reviewer as blocking.\n\nTest Plan: Added and removed blocking and non-blocking reviewers via CLI.\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T4887, T10939\n\nDifferential Revision: https://secure.phabricator.com/D15934","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/29a060d7f13fa12510772feb6a9c0978b6c924c7"},{"sha":"6cb2bde48d413e7daba2bf66128982920931d69e","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Add \"Dominion\" rules for Owners Packages\n\nSummary:\nRef T10939. This supports two settings for packages (although they can't be configured yet):\n\n  - **Strong Dominion**: If the package owns `a/`, it always owns every subpath, even if another package also owns the subpath. For example, if I own `src/differential/`, I always own it even if someone else claims `src/differential/js/` as part of the \"Javascript\" package. This is the current behavior, and the default.\n  - **Weak Dominion**: If the package owns `a/`, but another package owns `a/b/`, the package gives up control of those paths and no longer owns paths in `a/b/`. This is a new behavior which can make defining some types of packages easier.\n\nIn the next change, I'll allow users to switch these modes and document what they mean.\n\nTest Plan:\n  - Ran existing unit tests.\n  - Added new unit tests.\n\nReviewers: chad\n\nReviewed By: chad\n\nSubscribers: joel\n\nManiphest Tasks: T10939\n\nDifferential Revision: https://secure.phabricator.com/D15935","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/6cb2bde48d413e7daba2bf66128982920931d69e"},{"sha":"809c7bf996afe41597b242d626bdeab5a2645379","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Allow users to manage package dominion rules\n\nSummary: Ref T10939. This adds UI, transactions, etc, to adjust dominion rules.\n\nTest Plan:\n  - Read documentation.\n  - Changed dominion rules.\n  - Created packages on `/` (\"A\") and `/x` (\"B\") with \"Auto Review: Review\".\n  - Touched `/x`.\n  - Verified that A and B were added with strong dominion.\n  - Verified that only B was added when A was set to weak dominion.\n  - Viewed file in Diffusion, saw correct ownership with strong/weak dominion rules.\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T10939\n\nDifferential Revision: https://secure.phabricator.com/D15936","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/809c7bf996afe41597b242d626bdeab5a2645379"},{"sha":"9c24798e64c952ad5b350ff06e34c5c051cbc795","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Update Owners auditing rules for multiple reviewers\n\nSummary:\nRef T10939. Fixes T10181. This slightly simplifies, then documents the auditing rules, which haven't been updated for a while. In particular:\n\n  - If an owner authored the change, never audit.\n  - Examine all reviewers to determine reviewer audit status, not just the first reviewer.\n  - Simplify some of the loading code a bit.\n\nTest Plan:\n  - Ran `bin/repository reparse --owners <commit> --force` to trigger this stuff.\n  - Verified that the web UI did reasonable things with resulting audits.\n  - Read documentation.\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T10181, T10939\n\nDifferential Revision: https://secure.phabricator.com/D15939","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/9c24798e64c952ad5b350ff06e34c5c051cbc795"},{"sha":"de1a30efc7404dd02f14b6afd98cf5ba3cb701f0","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Improve audit behavior for \"uninteresting\" auditors\n\nSummary:\nRef T10939. Fixes T10174. We can currently trigger \"uninteresting\" auditors in two ways:\n\n  - Packages with auditing disabled (\"NONE\" audits).\n  - Packages with auditing enabled, but they don't need an audit (e.g., author is a pacakge owner; \"NOT REQUIRED\" audits).\n\nThese audits aren't interesting (we only write them so we can list \"commits in this package\" from other UIs) but right now they take up the audit slot. In particular:\n\n  - They show in the UI, but are generally useless/confusing nowadays. The actual table of contents does a better job of just showing \"which packages do these paths belong to\" now, and shows all packages for each path.\n  - They block Herald from adding real auditors.\n\nChange this:\n\n  - Don't show uninteresting auditors.\n  - Let Herald upgrade uninteresting auditors into real auditors.\n\nTest Plan:\n  - Ran `bin/repository reparse --owners <commit> --force`, and `--herald` to trigger Owners and Herald rules.\n  - With a package with auditing disabled, triggered a \"None\" audit and saw it no longer appear in the UI with the patch applied.\n  - With a package with auditing disabled, added a Herald rule to trigger an audit. With the patch, saw it go through and upgrade the audit to \"Audit Required\".\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T10174, T10939\n\nDifferential Revision: https://secure.phabricator.com/D15940","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/de1a30efc7404dd02f14b6afd98cf5ba3cb701f0"},{"sha":"3aed39b8b072c8bfa89b3f45183dfa126600ff1d","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Fix an issue with serializing reviewers over the wire\n\nFixes T10981. Ref T10939. `arc` currently has some odd, hard-coded checks\n(missing reviewers, all reviewers away) that depend on the field value being\nin a certain format.\n\nThe recent changes swapped the field value from scalars (PHIDs) to\ndictionaries and broke this workflow. It worked fine in testing because we\napply these checks very inconsistently (not on update or `--edit`).\n\nTo get around this for now, serialize into \"PHID!\" and then unserialize on\nthe other side. This is icky but keeps us from needing to require an `arc`\nupgrade.\n\nThese checks are generally bad news and should move to the server side in the\nlong run (T4631).\n\n(This probably prevents clean `arc diff`, so I'm just cowboy committing it.)\n\nAuditors: chad","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/3aed39b8b072c8bfa89b3f45183dfa126600ff1d"},{"sha":"7b50eef27a5600aacdb371749717977c27f986a3","author":{"email":"46f1a0bd5592a2f9244ca321b129902a06b53e03@epriestley.com","name":"epriestley"},"message":"Special case the \"added projects\" transaction in mail when creating objects\n\nSummary: Fixes T10493. See that task and inline comments for discussion.\n\nTest Plan:\nCreated an object with some projects, saw the transaction in resulting mail:\n\n{F1600496}\n\nReviewers: chad\n\nReviewed By: chad\n\nManiphest Tasks: T10493\n\nDifferential Revision: https://secure.phabricator.com/D15942","distinct":true,"url":"https://api.github.com/repos/Khan/phabricator/commits/7b50eef27a5600aacdb371749717977c27f986a3"}]}
{"actor":{"display_login":"csilvers"}}