Page MenuHomePhabricator

Implement "arc audit"
Closed, WontfixPublic

Description

Implement arc audit, which triggers audits. Rough plan is:

  • Do a future-proofing check:
    • have the workflow check for diffusion.createcommit using conduit.query;
    • or just give up and accept the X.createcommit APIs indefinitely.
  • Look up users with user.query to get their PHIDs.
  • Look up commit PHIDs with diffusion.querycommits or similar. This needs to be implemented, and will deprecate diffusion.getcommit.
  • Call diffusion.createcomment to create the audit.
    • Needs a tweak to accept auditor PHIDs.

Event Timeline

epriestley assigned this task to talshiri.
epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added a project: Diffusion.
epriestley added subscribers: epriestley, talshiri.
epriestley edited this Maniphest Task.Jan 24 2014, 7:46 PM

Actually, if you have project_id, you can do this without T4344:

  • Call arcanist.projectinfo to look up the project.
  • That gives you the callsign.
  • Construct the rXnnn string from the callsign and commit hash.
  • Call diffusion.getcommit to get its PHID.

Cool.

How would it work with T4344, though? Also query with a project id?

Yeah -- arcanist.projectinfo will let you convert a Project ID into a repository callsign.

epriestley edited this Maniphest Task.Jan 26 2014, 7:40 PM
epriestley edited this Maniphest Task.Jan 26 2014, 11:31 PM

(diffusion.querycommits is in HEAD now, too.)

Hey Evan!
Some updates on this (we've already discusses some of these, but I'm just summarizing them here for reference sake):

I wanted to use arc audit as a replacement for post-push CRs. Sadly this doesn't work terribly well, as:

  • The interface for the "CRs" is now split across two places
  • Some users here (myself included) much prefer the existing arc diff workflow
  • arc audit naturally won't work if you haven't pushed.

I did implement a basic arc audit shortly after your diffusion.querycommits addition. I can submit it, but I'm not sure if it's very useful at it's current state (there is no parsing of the messages a-la arc diff, so if you say, wanted to pull more people into the audit, you'd have to rely on command line args).

I'm currently thinking of maybe hacking around arc diff + diffusion so you can submit CRs without changing the git commits (so if you wanted to update a CR, you'll have to specify the revision manually).
I haven't looked at the code yet, so I'm not sure if it's possible to do that without breaking some base assumptions.

What's your take on this?

I'm currently thinking of maybe hacking around arc diff + diffusion so you can submit CRs without changing the git commits

With history.immutable = true, arc won't change the base commits. Differential Revisions will be linked to commits using commit and tree hashes, and won't amend commits. Additinoally, arc land will use a --no-ff merge strategy instead of a rebase/squash strategy.

So I think that's reasonable, and the hack you need to (have users who want to use this workflow) apply is:

arc set-config --local history.immutable true

Ha! That is awesome.
How does autoclose work in those cases? Does it wait for it to be accepted before switching it over to closed?

(wait = don't do anything on push, and as soon as it's accepted it queries diffusion for the commit to see if it's there)

If it's pushed to a non-autoclose branch, the merge commit which brings it to an autoclose branch will cause it to "appear" there for the first time and trigger the close.

That is, the sequence of actions is:

  • Accept
  • arc land, creating a merge commit which merges tal-dev1 into master
  • master is autoclose, so when the commit appears on master it triggers close actions.
talshiri added a comment.EditedFeb 21 2014, 8:23 PM

The issue here is that some users want to push directly to master, and have the code reviewed post-push (so there's no arc land).

Would it be sensible to make differential only autoclose diffs that have been accepted?

That plan is terrible, but you can set differential.close-on-accept.

That would close CRs that haven't been pushed, though.
What about having it check with diffusion first? If it's there, close. Otherwise it will get closed normally when diffusion sees it.

epriestley moved this task from Backlog to Future on the Audit board.Jan 10 2017, 5:20 PM
epriestley closed this task as Wontfix.Jan 14 2017, 5:05 PM

I'm going to close this since the original use case seems to have not panned out. We're still open to building this, but would like to see a modern use case driving it -- feel free to file a new request (or leave a comment here) if you have one.

After changes related to T10978 we now have modern diffusion.commit.edit and diffusion.commit.search endpoints so most of the technical blockers are cleared here. There's some remaining setup work in arc (roughly, T11429) but we're much better positioned to pursue this if there's a real use case for it.