Agree that it's not ideal 😄 Seems really strange for the ranges returned from the API to not actually be directly applicable to the repo names. How difficult would it be to change the ranges when we trim the code host in the client?

How difficult would it be to change the ranges when we trim the code host in the client?

I don't know, I thought about it briefly, but it seemed like it would be potentially more intrusive than just doing this. 🙈 I don't expect that we're going to change how we display repo names anytime soon which made me feel better about going this route

I don't think this should be a blocker, but I do think it's worth looking into. If you're not going to dig into this immediately, could you open a github issue for it? This feels like it's happening on the wrong layer, and that becomes more important if we ever have other consumers like Code Insights using this. We're running into issues like this left and right with code insights right now, so I'd rather not add more 🙂

Opened this issue https://github.com/sourcegraph/sourcegraph/issues/41476

Is this condition correct? Consider repo:contains.path(abc) and repo:def. Don't we still want to highlight def?

I think it is correct, for something like repo:contains.path(abc) and repo:def there will only be one RepoSearchJob and path will be appended to RepoOptions.HasFileContent for that job. So there shouldn't need to be merging of results since that repo will only be found once

oh wait, maybe not. stand by

Okay never mind, this condition should be right, but there are some other places in the search backend outside of RepoSearchJob where we return a RepoMatch and if one of those paths is hit, that repo match will not be highlighted. Will follow up to address that

for something like repo:contains.path(abc) and repo:def there will only be one RepoSearchJob

Yes, you're right -- bad example 😄

But what about repo:contains.path(abc) or repo:def (which also uses the merger)? I think we still want the highlights from repo:def even if the result we're merging into has no highlights.

Yes indeed, removed the if check