Summary:
Currently, "Closed" and "Abandoned" are treated as "closed". I want to add a flag which treats "Accepted" as "Closed", too, for Asana and other companies who use an Asana-like workflow.
The background here is that their workflow is a bit weird. They basically do audits, but have a lot of things which Diffusion doesn't do well right now. This one change makes Differential fit their workflow fairly well, even though it's an audit workflow.
To prepare for this, normalize the definition of "closed" better. We have a few callsites which explicitly check for "ABANDONED || CLOSED", and normalizing this is cleaner anyway.
Also delete the very old COMMITTED status, which has been obsolete for over a year.
Test Plan: Browsed around most/all of the affected interfaces.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7653
Summary:
Ref T1049. Ref T2222. `DifferentialDiff` does not currently have a PHID, but we need it for Harbormaster and ApplicationTransactions. See some discussion in D7501.
(I split the SQL into two sections so we can't fail in the middle. At some point, I'd like to do a pass on the migration stuff and get this happening automatically, and also simplify the PatchList.)
Test Plan:
- Ran `bin/storage upgrade`.
- Checked for valid PHIDs in the database.
- Used `phid.query` to look up a diff by PHID.
- Created a new diff and verified it got a PHID.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran, vrana
Maniphest Tasks: T2222, T1049
Differential Revision: https://secure.phabricator.com/D7513
Summary: See title. Fixes T1809.
Test Plan:
verified each type that has flaggable interface still can be flagged
verified that new custom query filter works
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1809
Differential Revision: https://secure.phabricator.com/D7392
Summary:
Ref T2222. Shrink the API to make it easier to move this object's storage to ApplicationTransactions.
Fixes T3415. This moves the "Summary" and "Test Plan" into the property list, and thereby fixes all the attribution problems associated with commandeering, creating a revision from another user's diff, etc.
Test Plan: Browsed several revisions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3415, T2222
Differential Revision: https://secure.phabricator.com/D7375
Summary:
Ref T2222. This implements step (1) described there, which is moving over all the inline comments.
The old and new tables are simliar. The only real trick here is that `transactionPHID` and `legacyCommentID` mean roughly the same thing (`null` if the inline is a draft, non-null if it has been submitted) but we don't have real `transactionPHID`s yet. We just make some up -- we'll backfill them later.
Two risks here:
- I need to take a second look at the keys on this table. I think we need to tweak them a bit, and it will be less disruptive to do that before this migration than after.
- This will take a while for Facebook, and other large installs with tens of thousands of revisions. I'll communicate this.
I'm otherwise pretty satisfied with this, seems to work well and is pretty low risk / non-disruptive.
Test Plan:
- Before migrating, then after migrating:
- Made a bunch of inlines (drafts, submitted).
- Edited and deleted inlines.
- Verified inlines showed up in preview.
- Verified that inlines aren't indexed when they're drafts (`bin/search index D935`).
- Verified that inlines ARE indexed when they're not drafts.
- Verified that drafts inlines make revisions appear as "with draft" in the revision list.
- Made left, right, and draft inlines.
- Migrated (`bin/storage upgrade`).
- Verified that my inlines from before the migration still showed up.
- (Repeated all the stuff above.)
- Manually inspected the inline comment table.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D7139
Summary:
Ref T603. This cleans up an existing callsite in the policy filter, and opens up some stuff in the future.
Some policy objects don't have real PHIDs:
PhabricatorTokenGiven
PhabricatorSavedQuery
PhabricatorNamedQuery
PhrequentUserTime
PhabricatorFlag
PhabricatorDaemonLog
PhabricatorConduitMethodCallLog
ConduitAPIMethod
PhabricatorChatLogEvent
PhabricatorChatLogChannel
Although it would be reasonable to add real PHIDs to some of these (like `ChatLogChannel`), it probably doesn't make much sense for others (`DaemonLog`, `MethodCallLog`). Just let them return `null`.
Also remove some duplicate `$id` and `$phid` properties. These are declared on `PhabricatorLiskDAO` and do not need to be redeclared.
Test Plan: Ran the `testEverythingImplemented` unit test, which verifies that all classes conform to the interface.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7306
Summary:
Ref T603. Allows the Differential view policy to be configured with a default.
I've omitted "edit" because I want to wait and see how comment/comment-action policies work out. I could imagine locking "edit" down to only the owner at some point, and providing a wider "interact" capability, or something like that, which would cover accept/reject/commandeer. Users in this group could still edit indirectly by commandeering first.
Test Plan: Created new revisions from the CLI and conduit.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7269
Summary:
Ref T603. I want to let applications define new capabilities (like "can manage global rules" in Herald) and get full support for them, including reasonable error strings in the UI.
Currently, this is difficult for a couple of reasons. Partly this is just a code organization issue, which is easy to fix. The bigger thing is that we have a bunch of strings which depend on both the policy and capability, like: "You must be an administrator to view this object." "Administrator" is the policy, and "view" is the capability.
That means every new capability has to add a string for each policy, and every new policy (should we introduce any) needs to add a string for each capability. And we can't do any piecemeal "You must be a {$role} to {$action} this object" becuase it's impossible to translate.
Instead, make all the strings depend on //only// the policy, //only// the capability, or //only// the object type. This makes the dialogs read a little more strangely, but I think it's still pretty easy to understand, and it makes adding new stuff way way easier.
Also provide more context, and more useful exception messages.
Test Plan:
- See screenshots.
- Also triggered a policy exception and verified it was dramatically more useful than it used to be.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7260
Summary:
Ref T1279. We currently determine reviewers at display time, but this is bad for several reasons:
- It puts queries very close to the display layer.
- We have to query for each revision if we want to figure out authority for several.
- We need to figure it out in several places, so we'll end up with copies of this logic.
- The logic isn't trivial (exceptions for the viewer, exceptions to that rule for install configuration).
- We already do this "figure it out when we need it" stuff in Diffusion for audits and it's really bad: we have half-working copies of the logic spread all over the place.
Instead, put it in the Query. Callers query for it and get the data attached to the reviewer objects.
Test Plan:
- Looked at some revisions, verified the correct lines were highlighted.
- Looked at a revision I created and verified that projects I was a member of were not highlighted.
- With self-accept enabled, these //are// highlighted.
- Looked at a revision I did not create and verified that projects I was a member of were highlighted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7241
Summary: Ref T1279. Show separate sections for "Reviewers" and "Project Reviewers" (Differential) and for "Auditors" and "Package/Project Auditors" (Diffusion/Audit).
Test Plan:
- Looked at a commit. Saw separation.
- Looked at a revision. Saw separation.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7233
Summary:
Ref T1279. No actual logical changes, but:
- You can now add projects as reviewers from the revision view typeahead ("Add Reviewers" action).
- You can now add projects as reviewers from the revision detail typeahead.
- You can now add projects as reviewers from the CLI (`#yoloswag`).
- Generated commit messages now list project reviewers (`Reviewers: #yoloswag`).
I'll separate projects from users in the "Reviewers" tables in the next revision.
Test Plan:
- Added projects as reviewers using the web UI and CLI.
- Used `arc amend --show --revision Dnnn` to generate commit messages.
- Viewed revision with project reviewers in web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7230
Summary:
Ref T1279. No logical changes, just updates the reviewer display style.
We currently keep track of only "requested changes".
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7228
Summary:
Ref T1279. @champo did a lot of this work already; we've been doing double writes for a long time.
Add "double reads" (reading the edge table as both the "relationship" table and as the "reviewer status" table), and migrate all the data.
I'm not bothering to try to recover old reviewer status (e.g., we could infer from transactions who accepted old revisions) because it wold be very complicated and doesn't seem too valuable.
Test Plan:
- Without doing the migration, used Differential. Verified that reads and writes worked. Most of the data was there anyway since we've been double-writing.
- Performed the migration. Verified that everything was still unchanged.
- Dropped the edge table, verified all reviweer data vanished.
- Migrated again, verified the reviewer stuff was restored.
- Did various cc/reviewer/subscriber queries, got consistent results.
Reviewers: btrahan
Reviewed By: btrahan
CC: champo, aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7227
Summary:
Does what it says on the label. We already had 'Any changed file content', now we have 'Any added file content' and 'Any removed file content'.
- There is a bit of copied/pasted code here: I'm open to suggestions on how to refactor it so it's less redundant.
- The wording seems a little awkward, and as @epriestley mentioned in T3829, moved code will be detected less than ideally.
Test Plan: Created Herald Rules, verified via dry run that they were triggered in appropriate situations.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3829
Differential Revision: https://secure.phabricator.com/D7214
Summary:
Ref T603. This could probably use a little more polish, but improve the quality of policy error messages.
- Provide as much detail as possible.
- Fix all the strings for i18n.
- Explain special rules to the user.
- Allow indirect policy filters to raise policy exceptions instead of 404s.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7151
Summary:
Ref T603. Adds clarifying text which expands on policies and explains exceptions and rules. The goal is to provide an easy way for users to learn about special policy rules, like "task owners can always see a task".
This presentation might be a little aggressive. That's probably OK as we introduce policies, but something a little more tempered might be better down the road.
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7150
Summary:
Ref T603. Make Differential behaviors for logged-out and underprivleged users more similar to other apps.
I'm going to drop this "anonymous access" thing at some point, but `reviews.fb.net` actually looks like it's running semi-modern code, so leave it alive until we have a more compelling replacement in the upstream.
Test Plan: As a logged out user, browsed Differential and clicked things and such.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7148
Summary:
Ref T603. Read policies out of policy columns.
When a revision is associated with a repository (which is currently never), require view access on the repository to see the revision (or, require the viewer to be the owner). This is a blanket "do the right thing" rule which should make Differential's default policies align with user expectations.
Future diffs will populate the `repositoryPHID` when a revision is created.
Test Plan: Tooled around Differential. None of this stuff does anything yet, so nothing very exciting happened.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7134
Summary: Ref T603. Paves the way for policy controls.
Test Plan: Ran storage upgrade, bumbled around in Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7133
Summary:
`getArcanistProjectName()` has some logic which gets messy with the `self::ATTACHABLE` mechanism. This makes `differential.getdiff` and similar Conduit methods throw an exception when querying a diff which doesn't have a project. See <http://pastebin.com/Czzrd0Jz>.
Instead, unconditionally attach a project (possibly `null`) when loading diffs if they need projects.
Test Plan: Ran `differential.getdiff` against a `arc diff --raw` diff with no project, got a result instead of an exception.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, sttwister
Differential Revision: https://secure.phabricator.com/D7101
Summary: I kind of made a mess of the API doing T2784. I figure just adding this is fine but LMK if you'd prefer something like diffquery got cleaned up more to handle this. Also adds an idx() call as I was getting errors looking at old diffs. Fixes T3823.
Test Plan: used the new api via test console - great success.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3823
Differential Revision: https://secure.phabricator.com/D6966
Summary:
Ref T3599
Go through everything, grep a bit, replace some bits.
Test Plan: Navigate around a bit
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3599
Differential Revision: https://secure.phabricator.com/D6871
Summary:
We can get this out of PHIDType reasonably in all cases and simplify implementation here.
None of these translate correctly anyway so they're basically debugging/development strings.
Test Plan: `grep`, browsed some transactions
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6786
Summary: Add `getReviewerStatus` to get an array of `DifferentialReviewer` objects. The method `needReviewerStatus` in `DifferentialRevisionQuery` loads the edges into the revisions loaded.
Test Plan: Added `->needReviewerStatus(true)` to `DifferentialRevisionSearchEngine` and checked through logging that the data was being loaded correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6450
Summary:
Ref T603. This introduces a policy-aware DifferentialDiffQuery and converts most callsites.
I've left unusual callsites (mostly: hard to get the viewer, unusual query, queries related to active diffs) alone for now, so this isn't exhaustive but hits 60-80% of sites.
Test Plan: Created diff; created revision; viewed diffs and revisions; made additional conduit calls.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6338
Summary: See discussion in IRC. Not 100% sure what's going on here because of email ghost theives, but conceivably a commit with no changes will end up with `null` changesets instead of `array()` changesets, which throws. Such diffs are certianly possible (`git commit --allow-empty`) even if they aren't the issue in this specific case. See T3416. Initialize changesets to `array()` to avoid throwing.
Test Plan:
Viewed some commits?
iiam
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6339
Summary:
Ref T2222. My path forward here wasn't very good -- I was thinking I could set `transactionPHID` for the inline comments as I migrated, but it must be unique and an individual DifferentialComment may have more than one inline comment. Dropping the unique requirement just creates more issues for us, not fewer.
So the migration in D6266 isn't actually useful. Undo it -- this can't be a straight revert because some installs may already have upgraded.
Test Plan: Ran new migrations, verified the world ended up back in the same place as before (made comments, viewed reivsions).
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6269
Summary:
Ref T2222. This adds PHIDs to all Differential comments so I can migrate the inlinecommment table to transaction_comment in the next diff.
@wez, this will issue a few million queries for Facebook (roughly, one for each Differential comment ever made). It's safe to skip the `.php` half of the patch, bring Phabricator up normally, and then apply this patch with Phabricator running if that eases the migration, although the next few diffs will probably be downtime-required migrations so maybe it's easier to just schedule some downtime.
Test Plan: Ran migration locally. Verified existing comments and new comments received PHIDs.
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6266
Summary:
Ref T2222. See D6260.
Push all this junk behind a Query so I can move the storage out from underneath it.
Test Plan: Viewed home page, list view, revision. Made draft, looked at preview, submitted draft, viewed inline, replied to inline.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6262
Summary:
Ref T2222. Ref T1460. Depends on D6260.
This creates the new tables, but doesn't start using them. I added three new fields for {T1460}, to represent fixed/done/replied states.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T1460, T2222
Differential Revision: https://secure.phabricator.com/D6261
Summary:
Ref T2222.
I'm thinking about how I want to approach the Asana sync, and I want to try to do T2222 first so that we can build it cleanly on top of ApplicationTransactions. I think we can at least walk down this road a little bit and if it turns out to be scary we can take another approach.
I was generally very happy with how the auth migration turned out (seemingly, it was almost completely clean), and want to pursue a similar strategy here. Basically:
- Wrap the new objects in the old objects for reads/writes.
- Migrate all the existing data to the new table.
- Everything hard is done; move things over a piece at a time at a leisurely pace in lots of smallish, relatively-easy-to-understand changes.
This deletes or abstracts all reads of the DifferentialComment table. In particular, these things are **deleted**:
- The script `undo_commits.php`, which I haven't pointed anyone at in a very long time.
- The `differential.getrevisionfeedback` Conduit method, which has been marked deprecated for a year or more.
- The `/stats/` interface in Differential, which should be rebuilt on Fact and has never been exposed in the UI. It does a ton of joins and such which are prohibitively difficult to migrate.
This leaves a small number of reading interfaces, which I replaced with a new `DifferentialCommentQuery`. Some future change will make this actually load transactions and wrap them with DifferentialComment interfaces.
Test Plan: Viewed a revision; made revision comments
Reviewers: btrahan
Reviewed By: btrahan
CC: edward, chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6260
Summary:
getDiffDict method returns the JSON for the changesets in a diff
The JSON descriping a changeset didn't containt the changeset ID
in some scenarios, knowing the changeset ID is really useful for the
clients.(i.e. when you want to open a standalone view of the changeset,
you need to know its ID)
Test Plan: existing
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5816
Summary:
This differential implements Phrequent's time tracking
functionality for users and hooks it up to Maniphest. It
also includes a basic "Time Tracked" list for the Phrequent
application, where users can review what they've spent time
working on.
Test Plan:
Apply the patch and track some things in Maniphest. They
should appear in the "Time Tracked" view of Phrequent.
There is also a `phrequent.show-prompt` option which toggles
whether to display a prompt when tracking time. I'm unsure
of whether the prompt is useful or is more likely to cause
people to click "Track Time", go off and do the task and then
come back to the prompt still waiting for them to confirm. A
potential solution to the "accidentally clicking the button
and recording 2 seconds of time" might be to show a prompt
on stop if the total time is under 10 seconds, asking whether
the user wants to keep or discard the tracked time.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2857
Differential Revision: https://secure.phabricator.com/D5479
Summary:
- Publish feed/notification.
- I think this is too lightweight for an email?
- We don't tell them which token right now. Laziness? Or intentional aura of mystery?!
- For tasks, notify both author and current owner.
- Fixes T2562.
Test Plan: {F33187}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2562
Differential Revision: https://secure.phabricator.com/D5007
Summary:
After D4825, this information is often available to us in a safe way. Provide it explictly.
This removes or reduces functionality in some cases, but I think we can plug those holes with Conpherence addresses and/or explicit user acknowledgement/config.
Test Plan: Patched a commit with `arc patch` and got the original address out.
Reviewers: btrahan, edward, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4828
Test Plan: Ran the migration on a single revision, verified DB, called `loadUnsubscribedPHIDs()`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4786
Summary: Added date created and date modified to diff
Test Plan: Creat a new diff. Check to see that dateCreated and dateModified appear
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4756
Summary:
T2345
getConfig throws an Exception when the key does not exist.
Also removes dead code that throws an Exception.
Test Plan:
Reloaded the Phabricator home page. In the process, found
2 Exceptions thrown due to nonexistent keys. After addressing these problems,
the home page loads without Exceptions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4541
Summary: introducing a new friend called DifferentialHunkParser. Sort of like the DifferentialChangesetParser but works with hunks only. tried to grab hunk parsing type things from across the code base and move them into this new class.
Test Plan: unit tests and played around in Differential a bit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4351
Summary: Some time long in the past, this was renamed to unsavedHunks. Fixes T2249.
Test Plan: Ran `destroy_revision.php D20`, got a successful destruction.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2249
Differential Revision: https://secure.phabricator.com/D4304
Summary:
assume at least 360px for a given code pane. that's about when the comment box starts fighting back anyway. we'll use the yet-to-be-built one page render for the narrow viewport cases.
This address the cases as laid out in T2005. It fails the "MMMMM" case pretty horribly. However, if there is a space it works just fine and presumably folks are stretching out their windows on big glorious monitors at 160 characters wide or whatever.
Re-factored things just a tad but figure I'll take a nice big chunk of "renderer" to move forward T2009
Test Plan: looked at all sorts of funky diffs
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4083
Summary: Happens on the end of hunk.
Test Plan:
$ ./reparse.php
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4091
Summary: 'cuz we need it in arcanist for T479 to commit as author
Test Plan: verified the return value was correct in conduit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T479
Differential Revision: https://secure.phabricator.com/D3917
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Test Plan: Looked at diff with several different lint errors, saw correct messages in their inline comments.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3827
Summary:
See T1963 for discussion of the Facebook-specific hack.
Differential currently uses a one-stage cache (render -> postprocess -> save in cache) rather than the two-stage cache (render -> save in cache -> postprocess) offered by `PhabricatorMarkupInteface`. This breaks Differential comments coming out of cache for the lightbox, and makes various other things suboptimal (status of handles like @mentions and embeds are not displayed accurately).
Instead, use the modern stuff.
Test Plan:
- Created preview comments and inlines in Differential.
- Edited a Differential inline.
- Submitted main and inline Differential comments.
- Viewed and edited Differential summary and test plan.
- Created preview comments and inlines in Diffusion.
- Submitted comments and inlines in Diffusion.
- Verified Differential now loads and saves to the generalized markup cache (Diffusion is close, but main comments still hold a single-stage cache).
- Verified old Differential comments work correctly with the lightbox.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1963
Differential Revision: https://secure.phabricator.com/D3804
Summary: If you `git show` and copy/paste it into Differential, we die trying to save the DifferentialChangeset corresponding to the commit message (error: column "filename" can not be null). Instead, drop the message change for raw diffs.
Test Plan: Copy/pasted `git show` into Differential.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3740
Summary: DifferentialAffectedPath has no id or phid key so delete() won't work and we have to do things this other way.
Test Plan: deleted a few diffs on my test reproduction. aside from warnings about missing keys (epriestley is on it as I write this) no errors found and diff observed as deleted
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1846
Differential Revision: https://secure.phabricator.com/D3610
Test Plan: Created diff, opened the file from Differential, opened the file in Diffusion.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3538
Summary: A cursory look at DifferentialReviewer suggests the primary reviewer doesn't actually have to be among the reviewers? Uploading this so bug reporter can patch and see if it helps.
Test Plan: Nope.
Reviewers: epriestley
Reviewed By: epriestley
CC: szymon, aran, Korvin
Maniphest Tasks: T1625
Differential Revision: https://secure.phabricator.com/D3198
Summary:
Diff of diffs display changes between new versions of file.
This is bad after rebase because there can be many unrelated changes so it is hard to spot the real change.
This diff unhighlights the lines that were added or removed in rebase.
The changes are still visible (they can be sometimes relevant) but very subtle.
Test Plan:
# Add, change and delete line. Display diff.
# Add and change some lines in parent. Rebase. Display diff. Display diff of diff.
# Change and add some lines. Display diff. Display diff to first diff. Display diff to second diff.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: jungejason, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2761
Summary: This error usually doesn't occur because we have only one hunk per changeset most of the time.
Test Plan:
$hunk = new ArcanistDiffHunk();
$hunk->setAddLines(1);
$hunk->setDelLines(1);
$change = new ArcanistDiffChange();
$change->addHunk($hunk);
$change->addHunk($hunk);
$diff = DifferentialDiff::newFromRawChanges(array($change));
var_dump($diff->getLineCount());
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2878
Summary: I will also need `getRemovedLines()` so refactor this first.
Test Plan:
New test case.
Viewed uncached diff.
Verified that the only callsite of `getAddedLines()` trims lines.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2875
Summary: When the commandeerer was a reviewer, after the commandeering, he stayed as a reviewer. He can no longer amend the diff. He has to go 'Edit Revision' to remove himself/herself. The fix is to remove it automatically.
Test Plan:
comamndeereded a revision and the behavior is correct now:
- I was removed from the revision list
- the comment transaction shows one more entry that I removed myself as a reviewer
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: nh, vrana, aran, Korvin
Maniphest Tasks: T1225
Differential Revision: https://secure.phabricator.com/D2872
Summary: See T693. To do "arc branch" performantly in immutable history repositories, we need to be able to do a single query to identify revisions related by hash. Return hash information to enable this.
Test Plan: Ran `differential.query`.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T693
Differential Revision: https://secure.phabricator.com/D2859
Test Plan:
Displayed repository.
Displayed repository history.
Wondered that we actually have bunch of commits without a revision.
Displayed blame.
Didn't display merge commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2840
Summary: `array_fill()`, contrary to `range()`, doesn't accept the last element but the number of elements.
Test Plan: Reparsed commit not changed after the last diff but rebased which was previously reported as changed.
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2783
Summary: We need it.
Test Plan: Ran 'differential.getdiff', saw it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2766
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
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
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
Test Plan: Display revision list both with last reviewer and without.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2495
Summary:
When choosing a verb to show with a closed differential revision, choose the
verb based on the upstream vcs, not the vcs used to create the diff, since these
are not the same thing. I also updated the documentation for the next step for
an accepted diff for the case where the local vcs and backing vcs aren't the
same (since arc land doesn't work for those).
Test Plan:
Loaded a committed diff and an accepted diff from fbcode and www to check that
they show the correct thing.
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1118
Differential Revision: https://secure.phabricator.com/D2360
Summary:
Also reduce the memory usage a little bit (before increasing it again).
I use the same CSS class as for the copied code.
Test Plan: Parsed 100 diffs and checked about 10 of them - looks good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2339
Summary:
Required for D2321.
Deprecates D2320.
Uses algorithm described at D2320#16.
Complexity of this algorithm would be `O(N)` (`N` stands for number of lines) in most cases.
The worst case is `O(A*F)` (`A` stands for number of added lines, `F` for number of colliding lines) but it should be pretty rare. Real-world example is 100 modified files with moved license block (15 lines) in each. This will require 1500*100 comparisons because the algorithm will be trying to find the longest block in each file.
Test Plan:
`arc diff --only` on commit with copied code.
More tests on standalone algorithm.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2333
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
Summary:
These are explicit copies of implicitly-generated Lisk methods.
See brief discussion in rPdec8bac3a3af6065166d485db80fffa70dc2abe3.
Test Plan: Looked at a diff in Differential.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2283
Summary:
- We currently post-filter by branches, but should do this in SQL. See T799.
- We currently identify branch-name-matches as being in the working copy even if they belong to a different project (e.g., two different projects with commits on the branch "master"). See T1100.
- Denormalize branch and project information into DifferentialRevision.
- Expose project information in the API.
Test Plan: Ran conduit API queries with branches and arc project IDs, got reasonable results.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1100, T799
Differential Revision: https://secure.phabricator.com/D2190
Test Plan:
/rX1
Browse in Diffusion
Open in Editor
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2180
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2103
Summary: I'll mark this one up inline since it's all separate bugs.
Test Plan:
- Created a diff with eight changes: (newline absent -> newline present, newline present -> newline absent, newline present -> newline present, newline absent -> newline absent) x (short file with change near end, long file with change near middle).
- Viewed diff in Ignore All, Ignore Most, Ignore Trailing and Show All whitespace modes.
- All 32 results seemed sensible.
- Really wish this stuff was better factored and testable. Need to fix it. :(
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1030
Differential Revision: https://secure.phabricator.com/D1992
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
Summary: No big surprises here, delted the unused "DarkConsole" class.
Test Plan: Ran 'testEverythingImplemented' to verify I wasn't finalizing anything we extend.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1876
Summary:
When a comments add reviewers or CCs, we just dump that sort of nastily into the
body. Put it in the header like Maniphest instead.
Also, record the diff associated with "update" actions and link to it (T871).
Test Plan: {F8546} {F8547}
Reviewers: btrahan, davidreuss
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T871
Differential Revision: https://secure.phabricator.com/D1659
Summary:
See discussion in T838. These fields expose information which it isn't necessary
or useful to expose in the general case.
- Disable fields by default, allow them to be enabled in config (these fields
were useful for me at Facebook when I had access to all the machines).
- Remove 'sourcePath' from Conduit methods other than differential.query.
- Condition 'sourcePath' field in Conduit on the caller being the revision
author. This is a bit hacky but not so awful.
Test Plan:
- Verified fields are gone by default and restored by configuration.
- Verified Conduit no longer returns these fields other than
differential.query.
- Verified field presence/absence according to authorship in
differential.query.
- Grepped around in arcanist to make sure we aren't relying on sourcePath.
There's a workflow in "arc merge" that technically might hit it, but I think
it's unreachable, definitely irrelvant (we never use source path as a
distinguisher under git/hg, and can't 'arc merge' in SVN) and it's going away
Real Soon Now anyway.
Reviewers: btrahan, arice
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T838
Differential Revision: https://secure.phabricator.com/D1582
Summary:
This just looks silly:
{F8088, size=full}
It runs in O(N*N) but it's not a big deal because there are usually only few
comments per line.
I didn't implement it for images.
Test Plan:
View revision with compatible inline comments in two diffs.
View revision with different inline comments on same line in two diffs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1570
Summary: There are lots of callsites to $changeset->getFilename() so it seemed
easier to rename getFileName() to getFilename() even if it includes database
change. Plus I think that getFilename() is better.
Test Plan:
Alter database.
Open revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1437
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
Summary: We need some additional fields to heuristically match revisions to the
working copy in arc.
Test Plan: Executed conduit method, got correct values in fields
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1347
