This is really nice, and it nukes soooo much sketchy code.

The only thing I'd sort of like to see is to have PhabricatorAuditSearchEngine be PhabricatorCommitSearchEngine instead, which wraps a DiffusionCommitQuery instead of a PhabricatorAuditQuery. Are there reasons this isn't actually a good idea?

It would probably mean adding a few more methods to DiffusionCommitQuery (and some of them will be a bit thorny to implement), but I think it would give us a better result overall. For example, if something has a package auditor and a user auditor, I can get output like this with the query-by-audit, where I have the same commit listed twice:

My claim is that this is useless and no one ever wants to do it, and it only works this way because it was easier to do originally and/or no one thought about it when implementing this interface.

The current approach also means "All Commits" isn't all commits, it's all commits with an auditor. So I can't just do something like "show me all of bob's commits across every repository" (which seems useful), and I have the same duplication problem, where everything is listed once for each auditor:

Having the "Audit" application query commits instead of audits is a little surprising, but I think it would generally be more powerful/useful, be what people actually want, and maybe we eventually merge it into Diffusion.

Does that make some approximation of sense? Are there technical / product reasons why it's a bad call?

Woah.

For the big comment, I can do it that way I guess. A few things had me do it this way "technically" -

• when I first got started, it made more sense to me to start from audit as I learned / remembered how these tables relate
• when I first got started, I hit an iceberg or two technically trying to start from commits. most memorably, "attachAudits" on a PhabricatorRepositoryCommit expects an audit inline comment and not an actual audit request, so I felt like I probably needed to re-name a bunch of calls and then make sure those calls still made sense in context... I forget what else seemed iceberg-y; probably wasn't much.
• adding "withCommitAuthorPHIDs" felt simpler than adding "withAuditorPHIDs", "withAuditStatus", "withAuditAwaitingUser", "needAudits" and more.
• somewhere in the middle, I was using my own display code in the render result list that didn't have the multiple, display bug (though possibly other display bugs for duplicate audits) because it was iterating through *commits* and not through the audits. (I upgraded to the AuditListView late-ish when I realized there was more functionality in there than I initially thought.)
• the home page stuff / alert counts also use commits now for the counts, so I thought I wisely beat the bug end to end.

From a product perspective, I thought the "all commits" thing is weird in Audit anyway and if I hadn't been silly and realized it was actually "all audits" I would have just named it that. :) I'm not sure how valuable it is, but if we want to display information about all N audits for a single commit, we need all N audits anyway, and if so we need to be smart about managing the 1 : N'ness here eventually?

So the two options I see

• do it as you say
• change the AuditListView to iterate through the *commits* and change the "all commits" to "all audits"

...and in either case, I'd like to show the 1 commit but the N auditors.

I gotta do the dinner thing, etc. so I'll default to the former of the two options (do it the @epriestley suggested way) when I get back to it if I don't hear from you.

most memorably, "attachAudits" on a PhabricatorRepositoryCommit

Yeah, there's probably some junky cleanup here to be done.

adding "withCommitAuthorPHIDs" felt simpler than adding "withAuditorPHIDs", "withAuditStatus", "withAuditAwaitingUser", "needAudits" and more

This will definitely be a bit messy, but I think it's worth doing since we'll get a better and more useful result.

I'm not sure how valuable [showing all audits] is, but if we want to display information about all N audits for a single commit

Although I might be missing use cases, my claim is basically that we never want to do this, and that it's not valuable at all, and that no one ever cares about this. (You care about it for a single commit, but you get the info on the commit detail page, and you care about querying by it, but we can query by it and still return commits.) I can't come up with any situation where I ever care about the audits or would want to see them instead of commits.

In contrast, I can come up with tons of situations where I want to see commits regardless of audit status (show all user X's commits across repositories, show all recent commits across repositories, show all commits across repositories tagged with #yolo, etc).

change the AuditListView to iterate through the *commits* and change the "all commits" to "all audits"

I think this will get really messy with paging: you'll get 100 results back, but fewer than 100 distinct commits, and have a uneven page sizes and general funkiness. And if page 1 ends on an audit for "rXaaaa" and page 2 starts with an audit for "rXaaaa", I'd expect you to have a duplicate entry.

Sure. I thought maybe the product was cleaner or more clear this way but had a 51/49 stance so I'll change it.

I'm actually probably 51/49 with you on this as is, but if we switch to commits I think we're going to end up with more fields in the future (repository, projects) and that the clarity starts to get lost if we have like 8 other fields instead of 3.

Perhaps as an analog, there's no interface in Differential for querying reviewers and getting back a list of your pending reviews, and no one has ever asked for this and I can't really imagine any cases where it would be useful. The fact that there's no need for it in Differential helps me feel confident that it's also not useful in Diffusion.