Your comments seem accurate, I'm heading out shortly but will fix this up later. I will probably bounce back to you for review since I haven't touched some of this code before.

After D13091 it will be possible to add repositories from the UI as well.

Should I attempt to automatically determine the repository from the working copy?

I probably wouldn't bother for now. I think the code Arcanist uses to do this is nontrivial, and we might get it free later if more of this ends up in Arcanist.

Should repositoryPHID be an attribute of DivinerLiveAtom or DivinerLiveSymbol?

It should be a property of DivinerLiveSymbol (so we can query for it -- all the user-facing query UI operates on Symbols). For technical/internal reasons, it might also end up somewhere on LiveAtom (either as a property or in a JSON blob somewhere), but that would just be to make getting it onto Symbol easier.

Did I do the automatic capabilities properly? Not sure how to test this.

It's correct as written, or you could just remove it for now. Lengthy reasoning and then relevant discussion:

"Automatic Capabilities" are a formal concept which allow users with a strong connection to an object (like a task owner, or revision author) to always have a capability, even if the policy says they don't. For example, a revision with "Edit Policy: No One" really means "Edit Policy: No One (Except the Author)". Automatic capabilities always weaken policies, and let users who do not pass policy checks take actions because of a strong relationship to the object.

"Implicit Policies" aren't really formal right now, although there's a reasonable chance that they'll become formal in order to fix T7703 (maybe as "Extended Policies"). For some kinds of objects, you need to be able to see or edit some related objects in order to see or edit a target object. For example, in Differential, you must be able to see the repository a revision is associated with in order to see the revision. In Phriction, you need view/edit capabilities on all parents of a document. In Phame, you (probably?) need to be able to see a blog in order to see posts on it. In a few other cases there are more infrastructure-level implicit capabilities: for example, you must be able to see an event in order to see its invitees.

Currently, these are implemented as (a) a "load, attach, discard records with no corresponding object" step in the Query, and (b) a human-readable note in describeAutomaticCapability(). This is somewhat confusing because the note isn't describing an automatic capability at all, but it puts the information in the right place in the UI and it wasn't a case I was thinking about when I wrote the method. A better name for this method might just be describeCapability() or describeSpecialRulesForCapability().

After T7703, they will probably also be implemented as an explicit step in PolicyFilter. Specifically, we would probably add some method like this after T7703:

public function getExtendedPolicyRequirements($capability) {
  return array(
    // Some datastructure which means "you must be able to
    // see the repository to see the book".
  );
}

These implicit policies always strengthen policies: they prevent users who would otherwise pass the policy check from taking actions.

After T3820, Spaces will also essentially put an implicit policy layer on top of all objects. So policy evaluation will roughly be:

(<Viewer Can See Space> AND <Viewer Can See Implicit Related Objects> AND (<Viewer Satisfies Policy> OR <Viewer Has Automatic Capability>))

So, in this specific case, the automatic capability implementation is correct, but it's also moot, and it would be equally correct if removed: repositories do not have any automatic capabilities, because no users have an exceptionally strong relationship to repositories: they aren't currently "owned" objects. If we gave some users a strong relationship to repositories in the future, it might or might not make sense for that relationship to also imply an automatic capability on books.

For example, if the relationship was "ownership", it might be reasonable to say that repository owners can always see books in their repository.

If the relationship was something like "permission to build/mirror" to support some kind of Harbormaster thing (I'm just making this up as an example, I can't really think of a good one), it might not make sense for that relationship to extend to books.

I'd maybe slightly favor just removing the code. Although "ownership" is by far the most likely automatic capability we'd add in the future and the code would be correct/necessary if we did, it does nothing today, I don't currently plan to add "ownership", and the code might cause us to apply too liberal a policy if we implement some other kind of automatic capability. Even if we implement "ownership", an explicit test to see if the viewer is a repository owner might be a better fix.

(There's also a third kind of implicit/indirect policy relationship, where completely-dependent sub-objects just directly reflect the policies of their parent objects. An example of this is Changesets in Differential. It never makes sense for a Changeset to have any policy other than the policy of its parent Diff, so the Changeset has no viewPolicy/editPolicy fields and just proxies Diff for the purposes of policy checks. This kind of object is straightforward and not affected by T7703, because it isn't adding any policy checks of its own. In this case, it's appropriate to just call through to the parent/container object.)

I think it makes sense to leave the automatic capability... it seems logical that access to view the repository implies access to view the documentation.

I had a thought... we could use the phabricator_repository.repository_path and phabricator_repository.repository_pathchange tables in order to determine the repository based on the path to the book. Thoughts?

We could, but that seems like a lot of effort for not much benefit to me. I think the existing mechanisms in arc (primarily, remote URI) are much better.

If you wanted to hack something in now, it's probably a comparable amount of effort to instantiate an Arcanist WorkingCopy object and then directly use the code to resolve the repository than try to do the path stuff (although this might be more complicated than I believe).

I'll defer until a later diff... I'm just going to move it to an argument (diviner generate --repository $callsign) for now.

In D13070#142166, @joshuaspence wrote:

Almost completely working. The only issue at the moment is that running diviner generate --repository X does not update the repository for the Diviner atoms if they had already previously been published.

@epriestley, do you have any thoughts on how to go about this? I was thinking of just adding queryfx($conn_w, 'UPDATE %T SET repositoryPHID = %s WHERE bookPHID = %s', id(new DivinerLiveSymbol())->getTableName(), $book->getRepositoryPHID(), $book->getPHID()) to DivinerLivePublisher::loadBook(), but that's pretty gross.

For the atom stuff, I thiiink there's probably some global/book cachekey we can put the repo PHID into to automatically invalidate everything, but we might need to add that. Hack fix seems fine for now.