I'm a few weeks behind master on this install if that's relevant, I can't see any commits between then and now which would fix this, but here are the hashes I'm at:

rP5125045
rARCc94e604
rPHU161e36f

D14243 should fix this. You can cherry-pick it safely if you don't want to upgrade yet, although cherry-picking changes to the Celerity map may be tricky.

You can probably also fix this like this:

• Check if phabricator_repository.repository_branch contains a single row (or a tiny number of rows, one for each call some user has made to diffusion.updatecoverage).
• If so, delete them.
• Things should work again?

The name of the table is misleading and this does not actually store information about branches. The repository_refcursor table is the modern branch/tag/ref store. This is an old table that supports a very old lint/unit mechanism which is probably cycling out of the codebase.

I realize it's an unstable API, serves me right, etc.

We're generally pretty sympathetic to "harmless-looking X had a bad, unrelated effect which was impossible to anticipate despite being marked as not-production-grade", just not "X said it wouldn't work and then it didn't work" or "I paged through a giant red ASCII art skull banner and explicitly confirmed the scope of the dangerous operation, and then it did what it said it would, but I regret my decision".

Do you plan on incorporating a coverage column in the UI? I have actually already created a "coverage" application which rolls up coverage information per directory and shows us that info, which is how I surfaced this bug.

I could try to move the implementation into diffusion, but it isn't refcursor based since the coverage data isn't refcursor based either.

Short answer is: yes, eventually, but it's not something I plan to prioritize in the near term.

The long answer has a large philosophical component:

On a purely personal level I hold coverage in relatively low esteem as a useful metric to improve quality or predict or assess risk. (There is some tangental discussion from a long time ago in T1630, and my thinking largely still follows those lines. I think coverage is more useful than complexity metrics, but not by a large margin.)

This is obviously entirely subjective and it's reasonable to have a different opinion on the value of coverage (or on complexity metrics), or have a project where the value of these metrics is higher than the class of consumer-facing web applications I have greatest familiarity with, but I think the value of observable indicators of quality (code coverage, test count, complexity, other statically derivable codebase statistics) is overstated far more often than understated across the industry.

Particularly, I believe that multiple authoritative encodings of the same information ("DRY violations") represent the largest quality problem with software by a large margin, and that these are impossible to detect statically in the general case (though you might have some luck detecting actual copy/paste statically). By coincidence, this bug was caused by a DRY problem (the main renderer and Ajax renderer separately encode the rules for which columns should be present in the table, and their rulesets differ subtly). If I tried to come up with a ranked list of other categorical quality issues I think the next 10 items on that list would probably also require human judgement and experience to detect, and only something like 5-10% of problems would benefit from better static tools.

Of course, you might share this worldview (or even have a more extreme one) and still want coverage information because the absolute value of having it is a net positive even if the relative impact on quality or risk is not large.

I consider exposing coverage information to be positive overall, but very low impact, so it is difficult to prioritize. I'd also like to have a better upstream narrative around the goals of showing coverage before building aggregate UI for it.

The current display of coverage at the file level (as colored bars on the right margin of diffs) serves a very narrow narrative: as an author, reviewer, or auditor, use coverage to identify conditions in the code code (for example, failure/error conditions) that might have been missed when writing tests (for example, maybe you tested the positive cases but missed the error cases). I think the UI is mostly effective in supporting that goal.

However, aggregate compilation of coverage isn't very useful in pursuing this specific goal, and I'm not really sure what goals it is useful in pursuing. Particularly, I don't think "have 100% coverage" is necessarily a noble goal in and of itself. Regardless of the goals, putting a "coverage %" column in the browse view is almost certainly not the best way to accomplish them, because I think this is mostly meaningless:

This codebase has 2.001% coverage because src/ has a billion lines of code and scripts/ has 50.

If there's some goal which we reasonably agree is sensible and getting to "have 100% coverage" is a reasonable proxy for achieving it, I'm not sure directory-by-directory aggregate coverage is even useful. Better might be a "lines covered / total lines" progress bar for the whole repository (which would accurately show progress toward the goal), and then a dedicated UI for attacking the problem (e.g., a flat list of files ordered by number of uncovered lines: act on this list to have the greatest impact on coverage in the codebase). To understand where coverage is missing, a UI which showed the repository as an area map or nested ring pie chart is probably better than attaching information to this UI -- something like this, but showing uncovered lines instead of disk usage:

Basically, I really want to make sure we have a narrative / use case / workflow / root problem in mind and are building the best UI to pursue it, not just adding data to the UI. I suspect there is no small step available between where we are now and a good, results-focused incorporation of coverage into Diffusion.

I agree with you philosophically, I think that coverage is mainly a heuristic we use to determine whether you're entering an area of the codebase that has been "well cared for". For us, typically, if you pull up a subproject in one of our repositories and there is no coverage, you will probably treat everything in there with extreme distrust. Actual coverage percentages don't matter to us so much.

As far as what you're saying about rolling up coverage averages, if you examine the tree one level at a time, yes, your coverage averages will be meaningless. If you traverse all of the coverage information at once for all levels of the tree, and collect the aggregates from that, it's quite useful.

Here's how our implementation works:

• Assume that arc unit --everything returns coverage information for everything, even files which have no coverage, but that the engine could otherwise grok (eg these all come back as NNNNNNNNNNNNNUUUUUNNNNNNUUUNUNUN)
• Copypasta diffusion.updatecoverage into another API method
• Roll up coverage per directory by just concatenating the strings as we loop over the directory, pertinent changes to method:

// build the batch insert queries using every covered file
foreach ($coverage as $path => $coverage_info) {
  // it's possible that coverage information would be generated for files not in the repository
  if(!$path_id = $path_map[$path]) {
    continue;
  }
  $sql[] = qsprintf(
    $conn,
    '(%d, %d, %d, %s)',
    $branch->getID(),
    $path_map[$path],
    $commit->getID(),
    $coverage_info);

  // add the coverage info for this file to the aggregate coverage info for all of its
  // parent directories
  $normal_path = DiffusionPathIDQuery::normalizePath($path);
  $path_parts = explode('/', $normal_path);
  array_pop($path_parts);
  $dir = '';
  foreach ($path_parts as $part) {
    $dir .= '/'.$part;
    if (isset($dir_info[$dir])) {
      $dir_info[$dir] .= $coverage_info;
    } else {
      $dir_info[$dir] = $coverage_info;
    }
  }
}

• Count the N,C,U,X's to get realistic coverage data on a per-directory basis
• Dump it to a table somewhere
• Hack together a terrible file browser
• 

For our largest repository, this takes a little bit, not forever, but it could be batched a little better I'm betting. Or maybe I haven't been clever enough in rolling up coverage data, but FWIW this provides us meaningful coverage information that we can derive from what Phabricator already has.

I have a bad habit of quickly reading things and responding without exactly comprehending what's been written, you didn't say what I thought you said about the coverage totals.

Anyways, tl;dr; that column is useful to us.

I'm tentatively OK with upstreaming that if we can find a way to present the data that communicates both the coverage percentage and total line size of each directory. In the screenshot above, the current directory might be nearly uncovered if async_diff/ has 95% of the code, or well on the way to coverage if submit_queue/ has most of the code, right?

One way to do that might be with different sized bars, like:

async_diff/    [|....................................]
submit_queue/  [||..]

The top bar would be only slightly filled (very little coverage) but very long (hypothetically, lots of code in that directory). The bottom bar would be half filled (50% coverage) but short (the directory doesn't have much code in it). Not sure if that's the best approach we could come up with, but that seems maybe-reasonable?

Or am I just misunderstanding what your percentages are communicating? I think we need to show two values somehow: both how much total code there is in a directory, and how much of it is covered. I don't think we can do that with a single percentage?

No you're correct and I'm getting what you're saying now.

I'll mess around with the UI and at least dump aggregate sloc alongside this info, although the "sloc" number we're looking for is just the total count of U and C combined yeah?

Yeah. If we did the bar UI, I think the length of the bar is just "U + C" (divided by the longest bar shown on the page, I guess? Or like log-base-something of that so the absolute length of the bar always has about the same meaning?) with something like "C / (U + C)" percent of it colored in.

Or it could just say "2% of 23,000 Coverable Lines" but I suspect that would be harder to parse at a glance than a bar if you have a bunch of subdirectories.

In T9524#139448, @epriestley wrote:

Yeah. If we did the bar UI, I think the length of the bar is just "U + C" (divided by the longest bar shown on the page, I guess? Or like log-base-something of that so the absolute length of the bar always has about the same meaning?) with something like "C / (U + C)" percent of it colored in.

Or it could just say "2% of 23,000 Coverable Lines" but I suspect that would be harder to parse at a glance than a bar if you have a bunch of subdirectories.

Big +1 for this. It makes much more sense than raw coverage

I'm ~1 week away from actually having our ducks in a row so we can merge upstream, would expect to land this right about then. Not sure if workboard implies some kind of scheduling?

Oh, sorry, I'm just moving stuff around on some boards so I can feel accomplished.

My "vNext" here is just "reasonable stuff from the backlog that we should do sooner than the other less reasonable stuff", as a starting point for whenever we do another real iteration on Diffusion. We don't have any specific plans for this right now.

Upshot: you're good, and at no risk of having the unique opportunity to implement this feature stolen away from you by the upstream before, like, early-to-mid-2016 unless circumstances change dramatically.

Per Q314, we also need to fix the coverage table. I'm not grossly disgusted with the +J{...} hack in D15286 but I think we need to add some kind of table to replace branchID like this:

<refCursorPHID, commitEpoch, commitPHID>

When coverage information is pushed, we look up all refs the commit is currently on (branches + tags), and pick an epoch for the commit (push date if possible, commit date or author date if not), then write rows into the new table.

To find the most recent commit with coverage information available, we look up the cursor for the current branch/tag, then find the most recent commit on it in the coverage table.

Then we can get rid of branchID and hopefully the whole branch table, which should be obsoleted by RefCursors.

We probably also need to have some GC/cleanup rules for this stuff but I guess you can go manually truncate the tables until then.

Ugh, the lint stuff also uses PhabricatorRepositoryBranch, although I suppose they can be moved off separately.

My planned pathway forward here is:

I'm going to leave D15286 and D15287 in limbo until we sort out the "most recent coverage" / branchID issue, since I think they'll cause more trouble than benefit until then (they're basically only useful in a repository which is so small/slow that you can regenerate coverage on every commit, and coverage is likely not useful in these repositories).

I expect to be able to fix the branchID stuff in this iteration, but it's fairly far afield from the core goal (optional callsigns, in T4245), so I'm not planning to pursue it right away. It's also entwined with some similar stuff in lint which probably merits some closer consideration, and both may interact with Harbormaster and arc (particularly, it would be good to make coverage and lint information easy to update via arc or Harbormaster).

The lint and unit stuff is probably separable so one possible path forward is fixing coverage without fixing lint.

Kicking this out of scope for now.

PHI181 has an entangling concern (T11372).