Page MenuHomePhabricator

Running "arc diff" on this instance, with a restricted owners package fails (after "arc patch")
Open, Needs TriagePublic

Description

Description:

I'm trying to "arc diff" against D16324 to update it, and there's an Owners package on it which is restricted from view. When I run "arc diff", I get:

git@ip-10-124-2-40:~/phabricator> EDITOR=vim ~/arcanist/bin/arc diff 9da15fd7ab7071e3b7361463229abd4e918d9d94 --update D16324 --trace
 ARGV  '/srv/phabricator/arcanist/bin/../scripts/arcanist.php' 'diff' '9da15fd7ab7071e3b7361463229abd4e918d9d94' '--update' 'D16324' '--trace'
 LOAD  Loaded "phutil" from "/srv/phabricator/libphutil/src".
 LOAD  Loaded "arcanist" from "/srv/phabricator/arcanist/src".
Config: Reading user configuration file "/srv/phabricator/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "/srv/phabricator/phabricator/.arcconfig".
Working Copy: Path "/srv/phabricator/phabricator" is part of `git` working copy "/srv/phabricator/phabricator".
Working Copy: Project root is at "/srv/phabricator/phabricator".
Config: Did not find local configuration at "/srv/phabricator/phabricator/.git/arc/config".
Loading phutil library from '/srv/phabricator/phabricator/src'...
>>> [0] <conduit> user.whoami() <bytes = 117>
>>> [1] <http> https://secure.phabricator.com/api/user.whoami
<<< [1] <http> 770,281 us
<<< [0] <conduit> 771,102 us
>>> [2] <exec> $ git diff --no-ext-diff --no-textconv --raw 'HEAD' --
>>> [3] <exec> $ git ls-files --others --exclude-standard
<<< [3] <exec> 41,954 us
<<< [2] <exec> 43,256 us
>>> [4] <exec> $ git diff-files --name-only
<<< [4] <exec> 19,677 us
>>> [5] <event> diff.didCollectChanges <listeners = 0>
<<< [5] <event> 114 us
>>> [6] <conduit> differential.query() <bytes = 149>
>>> [7] <http> https://secure.phabricator.com/api/differential.query
<<< [7] <http> 243,793 us
<<< [6] <conduit> 244,106 us
>>> [8] <conduit> differential.getcommitmessage() <bytes = 172>
>>> [9] <http> https://secure.phabricator.com/api/differential.getcommitmessage
<<< [9] <http> 363,029 us
<<< [8] <conduit> 363,368 us
>>> [10] <conduit> differential.parsecommitmessage() <bytes = 1,526>
>>> [11] <http> https://secure.phabricator.com/api/differential.parsecommitmessage
<<< [11] <http> 251,126 us
<<< [10] <conduit> 251,504 us

[2016-07-28 04:17:12] EXCEPTION: (ArcanistDifferentialCommitMessageParserException) Error parsing field "Subscribers": The objects you have listed include objects which do not exist (PHID-OPKG-gm6ozazyms6q6i22gyam). at [<arcanist>/src/differential/ArcanistDifferentialCommitMessage.php:54]
arcanist(head=f1c45a3323ae20eefe29c0a22c7923fe8b151bbf, ref.master=e5946ed1a5fa), phabricator(head=arcpatch-D16324, ref.master=7d87781f0bcf, ref.arcpatch-D16324=079d5b56c1c2), phutil(head=5fd2cf9d5ddd38424a54a8fba02398d527639970, ref.master=ecba5f543f25)
  #0 ArcanistDifferentialCommitMessage::pullDataFromConduit(ConduitClient) called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:1795]
  #1 ArcanistDiffWorkflow::getCommitMessageFromRevision(string) called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:1527]
  #2 ArcanistDiffWorkflow::buildCommitMessage() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:469]
  #3 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]

Replication Steps:

  1. Presumably create an account on this instance which doesn't have access to whatever that owners package is.
  2. Create a diff initially, and see the package get added by the Owners application after diff creation.
  3. Delete the branch locally.
  4. Some time later, use "arc patch Dxxxx" to get the branch back.
  5. Attempt to run "arc diff" again to update the diff and see Arcanist crash.

It should be noted that I ran "arc patch D16324" to get my branch back so I could work on it. That means that rather than the commit message I'm working off is actually:

commit e9cfb22ccf05718d2e3f3fb8e9acb528a6ae25f8
Author: June Rhodes <juner@pageuppeople.com>
Date:   Thu Jul 28 04:12:03 2016 +0000

    Handle crash when build plan has mix of valid and invalid build steps

    Summary:
    Resolves T11373.  Currently Harbormaster gracefully handles a scenario where the build step implenementation is missing for a step.  However, dependency calculations on valid build steps may cause them to attempt to access the implementations of other invalid build steps (in particular, when attempting to validate whether their input artifacts are available from other build steps).

    When this happens, you get a blunt page crash like this:

    {F1737168}

    Because of this page crash, there's no way to fix the invalid configuration from the web UI, and the user has to go and remove things from the database manually.

    Instead, after this change, the page doesn't crash, but shows "Unable to calculate dependencies for this build step" when this issue occurs.

    Test Plan: Had a page with a mix of valid and invalid build steps, where the valid build step had an input artifact.  Observed the crash.  Applied this patch and saw the crash go away.

    Reviewers: #blessed_reviewers, epriestley

    Subscribers: yelirekim, Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

    Maniphest Tasks: T11373

    Differential Revision: https://secure.phabricator.com/D16324

Expected Result:

Arcanist doesn't crash.

Actual Result:

Arcanist crashes.

Event Timeline

A subproblem here is perhaps that Owners should have a "Can Create Packages" policy.

Oops -- I assumed that package was someone testing things since every other object that has ever been hidden has been and ran bin/policy unlock without examining it, but it's actually a legitimate package. I think I put it back the way it was.

I think hidden stuff here is OK for now (and I'll be more careful in the future), but we may make some kind of "no secret stuff on this install" rule eventually. This particular issue is probably quasi-legit in the general case (and I think T11344 is pretty definitely legit) but, e.g., T10745 is probably sort of unique to users hiding things on this install and there was at least one case in the past where a user was confused by corporate users hiding things and I had to explain what was going on to them (I can't immediately dig it up).

Not a big concern for now, and sort of good if it uncovers bugs, but if the balance shifts too heavily toward support costs with no bug-finding value this may need to change.

I'm a corporate user and I love hiding things :(

Yeah I think the need for corporate users to hide things from their enemies is probably outside of our control. Just try to find lots of bugs with it.