Page MenuHomePhabricator

Please provide diff ids as an attachment in
Open, Needs TriagePublic


We need to be able to correlate differentials with their diffs for a variety of metrics-gathering and operational concerns. Right now we need to use to get the differential ids, then use differential.query to get the revision IDs and then use differential.querydiff to get details about the revisions. Ideally we'd be able to skip this middle step.

Event Timeline

eadler added a project: Restricted Project.
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.

(Is this a performance concern or do you just want like 6 fewer lines of code to maintain?)

Performance concern. At least one use case requires us to scour *all diffs and all diff ids*. Also a desire to avoid having deprecated calls in our codebase.

What do you need on differential.querydiff? We could just give you a modern with a revisionPHIDs constraint.

I'm open to a diff attachment, but I don't want it to return everything without any limits because someone will attach a million diffs to a revision some day and the whole API will break if we do that. If we do something like the subscribers attachment (list a sample, provide a total count, you run a different query if you actually need everything) it will scale, but it won't simplify your use case because you'll have to go make some other call for revisions with more than 10 or 25 or whatever diffs.

For that matter, can't you just use revisionIDs in differential.querydiff right now? That is:

  • to find revisions;
  • differential.querydiffs with revisionIDs of those results to find diffs.

That should be ~the same cost as having return IDs, with the same number of older calls?

This has come up again with a slightly different use case. We want to intelligently purge tags from our staging areas. To do this, we examine refs/tags/* in the staging area, and parse out the diff IDs. It would be great if we could do this from, but the attachments/constraints don't support it. The current working approach is differential.querydiffs with the IDs, parse the revision IDs out, then differential.query to get the revision statuses and their associated diffs list. This approach allows us to make significantly less queries than examining *all* closed/abandoned revisions, then calling the frozen/deprecated differential.query for the diff list.