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:
This is slightly more complicated for this reason:
- We don't set `dateCommitted` for normal commits, only for markcommitted.
-- We need to add this date to old revisions now.
Test Plan:
Reparse a revision - commit date was set.
Conduit `markcommitted` - commit date was set.
Run SQL script.
Display closed revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2282
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: NOTE: This is starting to be too hacky.
Test Plan:
View revision with inline diffs, verify that Reply is there.
View standalone - no Reply.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2263
Summary: Also link to `D1?id=` instead of `?id=` because some IE versions linked to root in this case.
Test Plan: Click on old diff's inline comment link on large revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2260
Summary:
- Adds "Commandeer Revision", to allow you to plunder revisions from those lost to sea (e.g., interns who have left or co-workers who are dealing with a family emergency).
- Removes admin-abandon to simplify things, since you can just Commandeer + Abandon now.
- There are other workarounds available but this is the natural/expected workflow (and the one everyone always asks for) and there's no real reason not to allow it.
Test Plan: Swashbuckled.
Reviewers: cpiro, btrahan, vrana, jungejason
Reviewed By: cpiro
CC: aran, zeeg
Differential Revision: https://secure.phabricator.com/D2257
Summary:
If I have Pygments enabled in config but `pygmentize` doesn't work then unhighlighted source is stored to cache.
If I later make `pygmentize` work then the unhighlighted source is still loaded from the cache.
Test Plan:
Break `pygmentize`.
View a diff with JS files.
Fix `pygmenize`.
View the diff again.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, gatos99, Koolvin
Differential Revision: https://secure.phabricator.com/D2227
Summary:
- Add an explicit multiplexing option, and enable it by default. This is necessary for Mail.app to coexist with other clients ("Re:" breaks outlook at the very least, and generally sucks in the common case), and allows users with flexible clients to enable subject variance.
- Add an option for subject line variance. Default to not varying the subject, so mail no longer says [Committed], [Closed], etc. This is so the defaults thread correctly in Gmail (not entirely sure this actually works).
- Add a preference to enable subject line variance.
- Unless all mail is multiplexed, don't enable or respect the "Re" or "vary subject" preferences. These are currently shown and respected in non-multiplex cases, which creates inconsistent results.
NOTE: @jungejason @nh @vrana This changes the default behavior (from non-multiplexing to multiplexing), and might break Facebook's integration. You should be able to keep the same behavior by setting the options appropriately, although if you can get the new defaults working they're probably better.
Test Plan:
Send mail from Maniphest, Differential and Audit. Updated preferences. Enabled/disabled multiplexing. Things seem OK?
NOTE: I haven't actually been able to repro the Gmail threading issue so I'm not totally sure what's going on there, maybe it started respecting "Re:" (or always has), but @cpiro and @20after4 both reported it independently. This fixes a bunch of bugs in any case and gives us more conservative set of defaults.
I'll see if I can buff out the Gmail story a bit but every client is basically a giant black box of mystery. :/
Reviewers: btrahan, vrana, jungejason, nh
Reviewed By: btrahan
CC: cpiro, 20after4, aran
Maniphest Tasks: T1097, T847
Differential Revision: https://secure.phabricator.com/D2206
Summary:
Some Differential fields are not nullable; when Test Plan is switched to non-required mode we can end up trying to save a null value to a non-nullable column (see D2193).
(I should probably just alter the schema to make these fields nullable, but that might have farther-reaching effects.)
Test Plan: Reproduced error, applied patch, no more error.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2200
Summary: Partially broken by D2166.
Test Plan:
Hover line number in revision.
Hover line number in standalone view.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2196
Summary:
This is a somewhat common request, and far more difficult than necessary currently.
I think the field is useful enough to leave it default-enabled, but there's wide diversity in testing philosophy.
Test Plan: Verified "test plan" field appeared. Disabled config. Verified "test plan" field vanished.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran, asouza
Differential Revision: https://secure.phabricator.com/D2193
Summary: We'll get a typehint warning on the repository if there's no repository. Check outside the method instead.
Test Plan: Loaded page, no warning.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2194
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:
When Phabricator is configured to generate patch email, we'll fatal if the patch contains binaries and is generating to Git because ArcanistBundle can't load the binary data. Provide a callback to load the data. See D2174.
(This may cause us to generate absolutely enormous emails, but you get what you asked for...)
Test Plan: Created a diff with an image under "send git patches" email configuration.
Reviewers: Makinde, btrahan, vrana, jungejason
Reviewed By: Makinde
CC: aran
Differential Revision: https://secure.phabricator.com/D2175
Summary: See D2080. The introduction of `arc land`, defaulting to `origin/master`, and --auto enormously simplifies the documentation.
Test Plan: Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
CC: 20after4, aran
Maniphest Tasks: T894
Differential Revision: https://secure.phabricator.com/D2082
I think the issue is that we don't set the left-side changesetID correctly. This seems to work correctly locally, but I'm not sure I got a good repro. Pushing to verify the production test cases provided in T1076.
Auditors: vrana, btrahan
Summary: I've found it useful mainly on smaller screen or with lots of comments.
Test Plan: Show Diff
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2122
Summary:
I've considered that user may have set editor but not checked out Phabricator repositories.
But stack trace is useful mainly for developers.
Test Plan:
Click on path in Unhandled Exception.
Repeat with disabled editor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2107
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:
When system agent adds a comment then he is added to CC.
When I amend and update then I get message "Commit message references nonexistent ..."
Test Plan: Update revision with system agent in CC.
Reviewers: epriestley
Reviewed By: epriestley
CC: michalburger1, aran
Differential Revision: https://secure.phabricator.com/D2100
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2091
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2085
Summary:
We'll incorrectly send CCWelcome mail to users who would be added as CCs but are blocked by the new "$dont_add" stuff, for
example when a revision is updated and the user has a Herald rule which triggers them getting CC'd. See D2057.
Potentially a better fix for this would be to have "addCCs" return a list of the CCs it actually added, rather than duplicating the
logic of removing CCs in two places. However, that's not trivial since it's just a wrapper around alterRelationships() which is nasty
and would need a more complicated return type. I think this whole thing will get a refactoring pass at some point -- I want to build a
more generic "associations"-like datastore and replace some of the ad-hoc associations with it. So maybe I can clean it up when that
happens. For now, this should fix the immediate problem.
Test Plan: Updated a revision, didn't get CC welcomed.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2072
Summary:
- Differential, Maniphest and Diffusion use slightly different styles for the object detail panels.
- Instead, use the same styles and CSS.
- Add object actions to Diffusion, including "Flag".
Test Plan: Looked at revisions, tasks and commit. Flagged and unflagged commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2062
Summary:
Herald rules are adding CC also for Author and Reviewer.
See also D1397.
I was considering also just don't displaying the extra CC but this is probably better.
There are still cases where there could be reviewer in CC (e.g. by making reviewer from CC or by direct edit) but I think it's not a big problem.
Beeing both Reviewer and CC can be actually useful (e.g. if you resign than you still are in CC) but it's not that useful to justify this:
Author: vrana
Reviewers: epriestley
CCs: vrana, epriestley
Test Plan: Comment on revision where I am author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2057
Summary:
I think this feature is probably good, but Differential is also really starting to get a lot of stuff which is not the diff in it. Not sure how best to deal with that.
The mixed table styles are also pretty ugly.
So I guess this is more feedback / proof-of-concept, I think I want to try to improve it somehow before I land it.
Test Plan: Looked at some diffs, some had an awkward, ugly list of diffs affecting the same files.
Reviewers: bill, aran, btrahan
Reviewed By: aran
CC: aran, epriestley
Maniphest Tasks: T829
Differential Revision: https://secure.phabricator.com/D2027
Test Plan: Looked at /differential/ with and without flags.
Reviewers: nh, btrahan
Reviewed By: nh
CC: aran, epriestley
Maniphest Tasks: T1055
Differential Revision: https://secure.phabricator.com/D2044
Summary:
- When an inline comment preview corresponds to an inline comment on the page, link to it. Just punt in the tough case where the inline is on some other page.
- In "haunted" mode, "z" now toggles through three modes: normal, comment area only, and comment + previews.
Test Plan:
- Viewed visible and not-visible inline comment previews, clicked "View" links.
- Tapped "z" a bunch to toggle haunt modes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T517, T214
Differential Revision: https://secure.phabricator.com/D2041
Summary:
Flags are a personal collection of things you want to take a look at later. You can use several different colors and add notes.
Not really sure if this is actually a good idea or not but it was easy to build.
Planned features:
- Allow Herald rules to add flags.
- In the "edit flag" dialog, have a "[x] Subscribe Me" checkbox that CCs you.
- Support Diffusion.
- Support Phriction.
- Always show flags on an object if you have them (in every view)?
- Edit dialog feels a little heavy?
- More filtering in /flag/ tool.
- Add a top-level links somewhere?
Test Plan: Added, edited and removed flags from things. Viewed flags in flag view.
Reviewers: aran, btrahan
Reviewed By: btrahan
CC: aran, epriestley, Koolvin
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2024
Summary: See T955. We jump to an awkard place right now; jump above the comment instead.
Test Plan: Clicked inline comment anchor links.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T955
Differential Revision: https://secure.phabricator.com/D2029
Summary: This header allows recipients to distinguish between CCs generated by Herald and CCs generated by humans.
Test Plan: Created a Herald rule to add a bunch of CC's to every revision. Created a revision. Added some CCs manually. Verified that only manual CCs appeared in the "Explicit" header.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T808
Differential Revision: https://secure.phabricator.com/D2018
Summary:
In the Differential revision list views:
- Allow you to filter by mailables (notably, mailing lists).
- Allow you to filter by user (including disabled users).
Test Plan: Filtered by a mailing list.
Reviewers: btrahan, nh
CC: aran, epriestley
Maniphest Tasks: T1031
Differential Revision: https://secure.phabricator.com/D1994
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: Apparently I spent like a good month copy/pasting slightly different versions of this logic all over the codebase.
Test Plan: Selected "View Options -> Browse in Diffusion" for a chagneset, got a URI with a branch name in it under Git.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1993
Summary:
It is now possible to set config setting requiring class of certain implementation to something completely else.
The consequence is that your Phabricator may stop working after update because you didn't implement some new method.
This diff validates the class upon usage.
It throws exception which is better than fatal thrown currently after calling undefined method.
Better solution would be to validate classes when setting the config but it would be too expensive - respective class definitions would have to be loaded and checked by reflection.
I was also thinking about some check script but nobody would run it after changing config.
The same behavior should be implemented for these settings:
- metamta.mail-adapter
- metamta.maniphest.reply-handler
- metamta.differential.reply-handler
- metamta.diffusion.reply-handler
- storage.engine-selector
- search.engine-selector
- differential.field-selector
- maniphest.custom-task-extensions-class
- aphront.default-application-configuration-class
- controller.oauth-registration
Test Plan:
Send comment, verify that it pass.
Change `metamta.differential.reply-handler` to incompatible class, verify that sending comment shows nice red exception.
Set `metamta.differential.reply-handler` to empty string, verify that it throws.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1919
Summary:
1. Created a filter for user comments that are not submitted yet
2. surface this information to the user by providing a "Draft revisions" link on the differential home page.
Test Plan: tested on one of the diffs
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley, jungejason
Differential Revision: https://secure.phabricator.com/D1927
Summary:
Depends on D1921. Depends on D1899.
Add the "View Options" dropdown menu to Diffusion, with options like "show standalone", "show raw file", "show all", etc.
Test Plan: Viewed commits in Differential and Diffusion.
Reviewers: davidreuss, nh, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1932
