Page MenuHomePhabricator

Why does coverage information include the "branch"?
Closed, ResolvedPublic

Asked by yelirekim on Feb 17 2016, 1:16 AM.


I'm about to be severely disillusioned about many things if someone explains to me how the commit id isn't an unambiguous identifier of the state of a repository suitable for associating coverage information with.


Updated 1,996 Days Ago

It's to support this (DiffusionRequest->loadCoverage()):

'SELECT * FROM %T WHERE branchID = %d AND pathID = %d

..which reads "select the most recent coverage information available for this file on the current branch" with a lot of hand-waving and caveats. Without branchID, this query could find coverage for a more recent commit on a different branch.

This approach is bad in general, and doesn't work, but the underlying problem is hard. For example, D15287 (and D14803 before it) only show coverage if a report has been generated for the exact commit the user is viewing. Unless you can publish coverage reports faster than users can commit, Diffusion will usually not show any coverage, even after D15287.

The actual operation we need is "find the first ancestor of the current commit which has coverage information", but there is no way to query that efficiently. We can probably drive a reasonable query out of PhabricatorRepositoryGraphCache, but probably better would be just approximating it: when coverage is written for a commit, record which branches that commit was on, then find the most recent commit-date on a commit with coverage which was on the current branch at the time it was run.

This won't work in all cases: for example, if you create a new branch off master, it won't pick up coverage until the next coverage update. Some merges, rewrites, etc., won't be immediately reflected. And commits which are deliberately back-dated won't order properly. But in all normal/reasonable cases, things should work relatively well without requiring anything gross.

New Answer


This question has been marked as closed, but you can still leave a new answer.