Summary: The recent change to the field causes us to render "http://junk.com/D"
in some cases, just null the field if there's no data.
Test Plan: Ran "arc diff --create".
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1321
Summary: Preview of Add Reviewers looks silly without actually showing them
Test Plan:
Go to any diff
Leap into action: Add Reviewers
Add some reviewers
Write some comment
Preview including Added reviewers should be displayed
Change action to Comment
Added reviewers should disappear
Repeat with Add CCs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1276
Summary:
See D1295. $unit_messages may be undefined.
I'll see if I can improve the visibility of warnings, the red dot in DarkConsole
is easy to miss right now. See T734.
Test Plan: Loaded a revision with no unit failures, didn't receive a warning.
Reviewers: nh, btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1306
Summary:
Some installs use Git as the backbone of a CI framework or use a Git remote to
share patches. The tracker scripts currently recognize associated revisions as
"Committed" when they appear in any branch, even if that branch is
"alincoln-personal-development_test_hack" or whatever.
To address the broadest need here, allow Git repositories to be configured to
track only certain branches instead of all branches.
This doesn't allow you to import a branch into Diffusion but ignore it in
Differential. Supporting that is somewhat technically complicated because the
parser currently goes like this:
- Look at HEAD of all branches.
- For any commits we haven't seen before, follow them back to something we
have seen (or the root).
- "Discover" everything new.
Since this doesn't track <branch, commit> pairs, we currently don't have enough
information to tell when a commit appears in a branch for the first time, so we
don't have anywhere we can put a test for whether that branch is tracked and do
the Differential hook only if it is.
However, I think this cruder patch satisfies most of the need and is simple and
obvious in its implementation.
See also D1263.
Test Plan:
- Updated a Git repository with various filters: "", "master, remote", "derp",
" ,,, master ,,,,,"
- Edited SVN and Mercurial repositories to verify they didn't get caught in
the crossfire.
- Ran daemon in debug mode on libphutil with filter "derp", got exception
about no tracked branches. Ran with filter "master", got tracking. Ran with no
filter, got tracking.
- Looked at Diffusion with "derp" and "master", saw no branches and "master"
respectively.
- Added unit tests to cover filtering logic.
Reviewers: btrahan, jungejason, nh, fratrik
Reviewed By: fratrik
CC: aran, fratrik, epriestley
Maniphest Tasks: T270
Differential Revision: https://secure.phabricator.com/D1290
Summary:
- We have a few places where we do some kind of ad-hoc comma list tokenizing,
and I'm adding another one in D1290. Add a helper to the request object.
- Add some unit tests.
Test Plan:
- Ran unit tests.
- Used PHID manager, Maniphest custom view, and Repository project editor.
Reviewers: btrahan, fratrik, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1302
Summary: This diffs adds support for marking up unittest result messages.
Test Plan: Verified that links in unittest results were markup'd.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D1298
Summary:
D1293 adds support for a literal block in remarkup. This diff enables
it in phabricator with a few basic rules (for line breaks, escaping HTML,
and linkifying URLs).
Test Plan: Tested in sandbox
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1297
Summary:
When all unit tests pass, a box appears between the unit test results and lint
status (for test failures to go in). This checks if there's anything to put
in that div/ul before putting it on the page.
Test Plan:
Loaded a revision with unit tests OK and saw no box. Loaded a revision with
failing unittests, and saw the same box from before.
Reviewers: tuomaspelkonen, epriestley
Reviewed By: epriestley
CC: jungejason, aran, nh, epriestley
Differential Revision: https://secure.phabricator.com/D1295
Summary: Makes it easier to discover the list of all revisions for a user.
Test Plan:
Opened up /differential/filter/revisions/, and saw that it defaulted
to status of all. Clicked between tabs, and it stayed on all. Selected
open, it only displayed open revisions, including as I switched between
tabs.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1278
Summary:
We have a debug interface for sending various sorts of email, but normal users
don't really need to use it. In particular, they can:
- Send arbitrary email to other users;
- Discover other users' email addresses fairly easily (CC everyone);
- Send arbitrary email to arbitrary addresses in conjunction with "Mailing
Lists"
In fact, normal users don't need to get to the MetaMTA web interface at all and
it has some somewhat-sensitive things beacuse it has a lot of detailed
information about mail. For instance, users can look at mail records to discover
things like password reset links and per-user object email addresses.
We should smooth out the UI here but I think I can do something about T21 fairly
soon and cover it then.
Test Plan:
Went to /mail/ with a non-admin, got 404'd. Went to /mail/ with an
admin, everything works, got a red admin header.
Reviewers: jungejason, btrahan
Reviewed By: btrahan
CC: aran, btrahan, jungejason
Maniphest Tasks: T718
Differential Revision: https://secure.phabricator.com/D1292
Summary: makes a nice side filter for most UI elements. only place this getds a
little funky is on the test console; a second, inner filter list appears for the
"affected" filters.
Test Plan: viewed each side filter and verified ui. for each filter, interacted
with the ui and made sure things looked right and there were no errors
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T631
Differential Revision: https://secure.phabricator.com/D1289
Summary:
See T624. I originally wrote this to require an explicit remote, but this
creates an ugly "origin:" in all the URIs and makes T270 more difficult.
Treat all branch names as implying 'origin/'.
Test Plan:
- Pulled and imported a fresh copy of libphutil without issues.
- Browsed various git repositories.
- Browsed Javelin's various branches.
- Ran upgrade script, got a bunch of clean 'origin/master' -> 'master'
conversions.
- Tried to specify an explicit remote in a default branch name.
- Unit tests.
Reviewers: nh, jungejason, btrahan
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T624
Differential Revision: https://secure.phabricator.com/D1269
Summary:
- When changing auxiliary field values, use transactions.
- Clean up some of the load/save logic for auxiliary fields so it's a little
more performant.
NOTE: The transaction display of auxiliary fields is incredibly hacky, I'll
follow up with a more nuanced approach but wanted to limit scope here.
Test Plan: Created and edited tasks with custom fields configured; created and
edited tasks without custom fields configured.
Reviewers: btrahan, jungejason, zeeg
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D1283
Summary: Allow paths to match even if they differ by trailing slashes and
".git".
Test Plan: Ran unit tests.
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T710
Differential Revision: https://secure.phabricator.com/D1286
Summary:
- These never actually did anything.
- I don't even really remember why I built them, maybe the Open Source team
was pushing for more GitHub integration or something? I really have no idea.
- Anyway, repository tailers do everything these could do (and much more).
Test Plan:
- Ran tailers off GitHub for many months without needing post-receive hooks.
- Grepped for relevant strings, couldn't find any references.
- Used "Repository" edit interface for a Git repository.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T706
Differential Revision: https://secure.phabricator.com/D1273
Summary:
- On the edit view, this is represented as a checkbox.
- On the detail view, it renders with a user-selectable string.
Test Plan: Added a bool field to my local install, checked and unchecked it.
Reviewers: zeeg, jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1277
Summary:
Git accepts either "git@x:/path" or "ssh://git@x.com/path" URIs to mean the
exact same thing, which is causing some false positives and confusion,
particularly because we sometimes mutate URIs.
Since this is just a sanity check, we don't really care about the username,
domain or credentials -- matching the paths is good enough. We're just trying to
make it hard to shoot yourself in the foot by copy-pasting the same local path
into two repositories and forgetting to change one, like I did. :P
Relax the check to only verify the paths are the same.
Test Plan:
- Ran unit tests, which should fully cover things.
- Ran commit discovery daemon in debug mode on incorrectly and correctly
configured repositories.
Reviewers: ajtrichards, jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T710
Differential Revision: https://secure.phabricator.com/D1279
layout
Summary:
- Use new less-horrible layout.
- Organize information more completely and sensibly.
Test Plan: Looked at some profiles.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1281
Summary:
It used to be more useful for daemons to spew random debugging information, but
features like "phd debug" and some fixes to error reporting like D1101 provide
better ways to debug, test, develop and diagnose daemons.
- Stop writing "." every time MetaMTA sends a message.
- Stop spewing the entire IRC protocol from the IRC bot unless in debug mode.
- Stop writing GC daemon log entries about collecting daemon logs (DURRR)
unless in debug mode.
Test Plan: Ran daemons in debug and non-debug modes, got expected level of
noisiness.
Reviewers: jungejason, nh, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1268
Summary: There can be Dxxx, rXXXxxx or even full URL in //Blame Revision// field
so just highlighting it as normal text would work probably best
Test Plan:
Go to https://secure.phabricator.com/D277
You should see a link from //Blame Revision// (if it would be displayed)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1274
Summary:
when a path is '/' in defining a package, D1251 is generating
an extra '//'.
Test Plan: veryfied adding path '/', '/src' and '/src/' all worked.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1266
Summary: I think we only hit this because I mucked around with the database to
recover from the runaway parse of the Diviner repository (now prevented by
D1253), but be more robust against missing data in this interface.
Test Plan: After applying this patch, no longer received a fatal on the commit
history page for users linked to nonexistant/bogus commits.
Reviewers: jack, btrahan, jungejason, aran
Reviewed By: aran
CC: aran
Maniphest Tasks: T701
Differential Revision: https://secure.phabricator.com/D1264
- Use the computed remote URI (which may have an explicit 'ssh://' under Git in some cases).
- Use '$id' correctly rather than casting the URI to an int in the message parser.
Summary: I want to add a command like "where is ArcanistUnitTestEngine" to
phabot. I also want to add a symbol typeahead to Diffusion and generally finish
up that feature since it's useful but only half-implemented. Consolidate the
query logic and expose the data over Conduit.
Test Plan: Used /symbol/ and Conduit to lookup symbols.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T315
Differential Revision: https://secure.phabricator.com/D1260
Summary: See D1257. Also make the error message more friendly, and remove a very
very old Facebook-specific error.
Test Plan:
- Tried to diff with an older arc.
- Tried to diff with a newer arc.
- Diffed with the right arc.
Reviewers: btrahan, jungejason, aran
Reviewed By: aran
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1258
Summary:
- Previously, used IDs like "33" to match a commit to a Differential revision.
This has a namespacing problem because we now have an arbitrarily large number
of Phabricator installs in the world, and they may want to track commits from
other installs.
- In Differential, parse raw IDs or full URIs. Emit only full URIs.
- In Repositories, parse only full URIs.
- This might cause a few commits to not be picked up in rare circumstances.
Users can fix them with "arc mark-committed". This should be exceedingly rare
because of hash matching.
- There are some caveats for reparsing older repositories, see comments
inline. I don't think there's much broad impact here.
Test Plan:
- Created a new revision, got a full URI.
- Updated revision, worked correctly.
- Ran unit tests.
- Monkeyed with "Differential Revision" field.
- Reviewers: btrahan, jungejason
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Maniphest Tasks: T54, T692
Differential Revision: 1250
Summary:
Although I couldn't repro the issue in T692, I did manage to point the "Diviner"
repository at the "Phabricator" working copy and screw some stuff up on
secure.phabricator.com.
Before discovering commits in a repository, ensure the 'origin' remote points at
the configured URI. This prevents issues where the working copy gets configured
to point at an existing (but incorrect) checkout.
Test Plan:
- Ran gitcommitdiscovery daemon normally under "phd debug", saw it execute the
"remote show -n" command and then start working.
- Intentionally botched the config, got an exception:
(Exception) Working copy '/INSECURE/repos/phabricator' has origin URL
'ssh://git@github.com/facebook/phabricator.git', but the configured URL
'git://github.com/facebook/diviner.git' is expected. Refusing to proceed.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T692
Differential Revision: 1253
Summary:
Allow entry of "CC: alincoln" to match user "ALincoln".
Put both variations in the map and try the exact case version first since we'll
also match email addresses and mailables, and theoretically some mailable might
have the same name as a user, as we're effectively abandoning restriction of
which characters can appear in usernames.
Test Plan: Created a local revision with a reviewer in CrAzY CaPs.
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T697
Differential Revision: 1255
Summary:
Paths in owners packages when referring to a directory should always end with
a trailing slash. (Otherwise, some things break, like loading the owning
packages for a path.) With this change, PhabricatorOwnersPackage now requires
that the path provided for a package is valid, and if the path is for a
directory, it adds a trailing slash if one was not provided.
Test Plan:
Edited a path in a package and left off the trailing slash. Saw that the slash
was added. Tried again with the trailing slash, and checked that another slash
was not added. Did this with a path in both a git and svn repository.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1251
Summary:
- Old page was useless and dumb.
- New page looks a little less bad, functions a little less poorly.
- Still lots of work to be done.
Test Plan:
- Viewed a project.
- Clicked all the links on the left nav.
- Here is a screenshot:
https://secure.phabricator.com/file/view/PHID-FILE-4buzquotb3fo4dhlicrw/
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T681
Differential Revision: 1246
Summary: Share more code; reduce the number of ad-hoc versions of this rendering
loop.
Test Plan: Clicked all the filters.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1247
Summary:
Provide tighter integration between Projects and Phriction. Partly, I have most
of a rewrite for the Projects homepage ready but it's not currently possible to
publish feed stories about a project so all the feeds are empty/boring. This
partly makes them more useful and partly just provides a tool integration point.
- When you create a project, all the wiki pages in projects/<project_name>/*
are associated with it.
- Publish updates to those pages as being related to the project so they'll
show up in project feeds.
- Show a project link on those pages.
This is very "convention over configuration" but I think it's the right
approach. We could provide some sort of, like, "@project=derp" tag to let you
associated arbitrary pages to projects later, but just letting you move pages is
probably far better.
Test Plan:
- Ran upgrade scripts against stupidly named projects ("der", " der", " der
", "der (2)", " der (2) (2)", etc). Ended up with uniquely named projects.
- Ran unit tests.
- Created /projects/ wiki documents and made sure they displayed correctly.
- Verified feed stories publish as project-related.
- Edited projects, including perfomring a name-colliding edit.
- Created projects, including performing a name-colliding create.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T681
Differential Revision: 1231
Summary:
add basic auditing functionalities. For the related commits for a
package, we detect the following conditions which might be suspicious to the
owners of the package:
* no revision specified
* revision not found
* author not match
* reviewedby not match
* owners not involved
* commit author not recognized
The owners of the package can change the status of the audit entries by
accepting it or specify concern.
The owner can turn on/off the auditing for a package.
Test Plan:
* verified that non-owner cannot see the details of the audit and cannot modify
it
* verified that all the audit reasons can be detected
* tested dropdown filtering and package search
* verified really normal change not detected
* verified accept/concern a commit
* tested enable/disable a package for auditing
* verified one audit applies to all <commit, packages> to the packages the
auditor owns
* verified that re-parsing a commit won't have effect if there exists a
relationship for <commit, package> already
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, benmathews, btrahan, mpodobnik, prithvi, TomL, epriestley
Differential Revision: 1242
Summary: This was well-intentioned but has not actually proven to be useful.
Test Plan:
- No list tab shows up anymore.
- Looked up a PHID.
Reviewers: btrahan, jungejason, Girish
Reviewed By: Girish
CC: aran, jungejason, edward, emiraga, Girish, nh, tuomaspelkonen, epriestley
Maniphest Tasks: T631
Differential Revision: 1234
Summary: I didn't get around to this earlier; add Feed/Maniphest integration.
This is partly motivated by wanting Projects to not be terrible. Pretty
straightforward.
Test Plan:
- Created, updated, reassigned and closed a task.
- Verified feed stories render reasonably.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1232
Summary: These are "local" commands, but need remote credentials. If the daemon
runs as a user who does not have credentials, the initial clone will work but
subsequent updates will fail.
Test Plan:
- Nuked a local copy of a Git repo.
- Ran "phd debug fetch <phid>" as root (or any other user with no natural SSH
keys). Verified initial clone worked (since it passes credentials to the command
correctly).
- Killed daemon, re-ran, verified "fetch" failed (no credentials passed).
- Applied this patch.
- Re-ran "phd debug fetch <phid>", verified it passed credentials and
succeeded.
- Did all these steps for a Mercurial repo.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T686
Differential Revision: 1236
Summary:
I pretty much copy/pasted this code; rather than do that again now that I want
to add feeds to projects, share the code.
This "Builder" is a little weird -- I don't want to call it a "View" because it
does data access. "Builder" seemed okay. We don't really have much code that
does this sort of thing right now, elsewhere.
Test Plan:
- Viewed public feed.
- Viewed private feed.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1233
Summary:
- For context, see T547. This is the last (maybe?) in a series of diffs that
moves us off raw sha1() calls in order to make it easier to audit the codebase
for correct use of hash functions.
- This breaks CSRF tokens. Any open forms will generate an error when
submitted, so maybe upgrade off-peak.
- We now generate HMAC mail keys but accept MAC or HMAC. In a few months, we
can remove the MAC version.
- The only remaining callsite is Conduit. We can't use HMAC since Arcanist
would need to know the key. {T550} provides a better solution to this, anyway.
Test Plan:
- Verified CSRF tokens generate properly.
- Manually changed CSRF to an incorrect value and got an error.
- Verified mail generates with a new mail hash.
- Verified Phabricator accepts both old and new mail hashes.
- Verified Phabricator rejects bad mail hashes.
- Checked user log, things look OK.
Reviewers: btrahan, jungejason, benmathews
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T547
Differential Revision: 1237
Summary:
Outlook wraps a message in 5 dashes on each side when doing replies.
This strips english and danish versions.
Test Plan:
Tried parsing emails with different messages and saw the
expected behaviour with patch applied. Ran arc unit, and saw test
passed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1239
Summary:
When changing attachments, the removed part is squished together
with the added parts, making it hard to read. This changes the output
so it looks like other changes, seperating each action by a semicolon.
Test Plan:
Viewed a task where i had attached and deleted revisions, and
saw the output look as other changes of same type.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1238
Summary:
- Add a "delete" operation. Delete is just a special edit which removes the
page from indexes and shows a notice that the document has been deleted.
- When a user deletes all the content on a page, treat it as a delete.
- When a conduit call deletes all the content on a page, treat it as a delete.
- Add page status to Conduit.
- Add change type field to history.
- Added a couple of constants to support a future 'move' change, which would
move content from one document to another.
Test Plan:
- Verified deleted pages vanish from the document index (and restoring them
puts them back).
- Verified deleted pages show "This page has been deleted...".
- Created, edited and deleted a document via Conduit.
- Deleted pages via "delete" button.
- Deleted pages via editing content to nothing.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: skrul, aran, btrahan, epriestley
Maniphest Tasks: T680
Differential Revision: 1230
Summary: Remove a bunch of relatively useless stuff from the Project list
interfaces.
Test Plan: Looked at project lists, less random busy junk.
Reviewers: btrahan, jungejason, zeeg
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1229
