Skip to content

search: fork/archived UI notifications

Administrator requested to merge rvt/new-fork-notification into master

Created by: rvantonder

This adds a notification as seen below. This shows up if a fork (resp. archive) is excluded only if the fork: (resp. archive:) option isn't explicitly specified in the query.

Screen Shot 2020-05-12 at 11 01 39 PM

I plan to still add an e2e web regression test and some backend test, though I would much prefer an integration test for it.

Notes:

  • On hover, it says to add fork:yes followed by the list of repos. There isn't a lot of freedom to format this, and it will overflow on the width of the hover box if the message is longer. I think the current message is OK.

  • A repo may be a fork and an archive, or only a fork/only an archive. It's best if we treat the labels separately as far as showing the notification

Backend implementation stuff

  • At first I thought to do a query of all repos including forks/archives, and then filter out the forks/archives. But, I discovered that the standard DB call will not populate those fields/labels for repos due to perf issues. Instead of iterating over the entire list and then 'hydrating' whether they are forks or repos, I instead take the original query and modify it to make two additional DB calls where I set fork:only and then archived:only, and put those matching repos in the 'excluded' list. My intuition is that this is faster than calling Get(...) for each repo and then checking fork/archived. The additional DB calls run in the typical case when fork/archived unspecified, but not otherwise.

  • I had to refactor tests that relied on a mocked DB List call. These tests want certain invariants to hold for the DB options when matching repos. The current/previous 'easy' way to do this is just to accept all the DB options as default and set the ones that matter. When we changed the default of fork/archive matching in a previous PRs (e.g., https://github.com/sourcegraph/sourcegraph/pull/8739), I had to change these tests to now use the new NoFork... default option. Now, these values can change since there are multiple calls to DB List. The answer is that these tests should not be relying on such strong invariants on the fork/archive inclusion/exclusion logic because that's not what they're testing. I have updated the tests to check for weaker invariants instead.

  • This will conflict with refactor #10489. I'd appreciate if we could hold off on merging #10489 until after branch cut, since I'd like to get this feature in for 3.16 without issues that might crop up due to refactor and/or adjusting to it.

Merge request reports