search: use chunk matches to render multiline streamed results#807
Conversation
0f912e2 to
019dfe4
Compare
019dfe4 to
d1c5991
Compare
|
|
||
| [38;5;2msourcegraph/sourcegraph[0m[38;5;239m ([0m[38;5;23mhttp://127.0.0.1:55128/sourcegraph/sourcegraph[0m[38;5;239m)[0m[38;5;2m (1 match)[0m | ||
| [0m [38;5;69m 5[0m[38;5;239m | [0m[38;5;0m[48;5;11mfoo[0m bar [38;5;0m[48;5;11mfoo[0m[38;5;239m | ||
| ------------------------------------------------------------------------------ |
There was a problem hiding this comment.
There is a minor cosmetic change, that we will put a ---- line below the last result, and is a consequence of keeping the loop logic simple
d1c5991 to
e47eaa2
Compare
|
If we version src-cli in tandem with sourcegraph this should be safe. If not, I worry that a customer is on an older version of sourcegraph and now src search -stream breaks. Either way we need to document this loudly in the changelog. I know of customers using the json output with streaming.
Sounds good to me! Additionally can we update the output of this so its less janky? I think you have done some of that, but man I really wish it didn't automatically have color output and the use of a pager. When using src-cli I am nearly always wanting to consume it via a tool and not as a human. |
|
@camdencheek nice catch, I remembered the part about overlapping ranges but naively thought it would unset and just re-enable highlighting. A test case proved otherwise. I implemented a test and the counter idea to fix this @keegancsmith we are set with in-tandem compatibility. Is bold markdown enough for "loudly" communicating this or should we go harder with
Unfortunately I have developed an allergy for drive-by requests and will have to decline 😛 I really just want to make sure we benefit from @camdencheek's hard work while it's fresh, and expedite our ability to get rid of |
|
Alright I'm going to go ahead with merging this. I made the changelog entry louder. I will triage client/customer usage issues coming from this change (so you know who to call). I suspect this is not going to be a high impact change. We really have to move forward at some point and break away from broken stuff for our own sanity. |
|
Also I checked the docs to see if this change should update anything there, and instead see we're still not guaranteeing backward compatibility anyway.
|
This switches
src search -stream 'query'to use the chunk match result type.This means:
Accurate result counts for patterns that match across newlines
Multiline/structural search highlighting is now accurate (previously structural search was highlighting was broken)
New JSON schema for
src search -stream -json 'query'new schema
{ "type": "content", "path": ".golangci.yml", "repository": "github.com/sourcegraph/sourcegraph", "branches": [ "" ], "commit": "994c2bc9fd0d5a5baaa6ed1664b3fa7bfa96e4aa", "chunkMatches": [ { "content": " - sheets.go", "contentStart": { "offset": 3092, "line": 98, "column": 0 }, "ranges": [ { "start": { "offset": 3098, "line": 98, "column": 6 }, "end": { "offset": 3107, "line": 98, "column": 15 } } ] }, { "content": " - slack.go", "contentStart": { "offset": 3108, "line": 99, "column": 0 }, "ranges": [ { "start": { "offset": 3114, "line": 99, "column": 6 }, "end": { "offset": 3122, "line": 99, "column": 14 } } ] } ] }old schema
{ "type": "content", "path": ".golangci.yml", "repository": "github.com/sourcegraph/sourcegraph", "branches": [ "" ], "commit": "477f4cf74b6deef0f78ba5ec316fd299bddc621c", "lineMatches": [ { "line": " - sheets.go", "lineNumber": 98, "offsetAndLengths": [ [ 6, 9 ] ] }, { "line": " - slack.go", "lineNumber": 99, "offsetAndLengths": [ [ 6, 8 ] ] } ] }PR details:
I am making the decision to remove line matches entirely for this option, because otherwise we can never move away from line matches/
previewto adopt chunk matches as the only source of truth, which we really should do. It is possible that this could break clients/consumers that expect line matches for the-stream -jsonoption, and the changelog is updated accordingly.Maybe this should be a major version bump, but looks like we keep src-cli releases in tandem with Sourcegraph? Or is this minor enough that we can ship this with a minor
srcrelease (to what extent have we encouraged usingsearch -stream? consumers could in theory remove-streamand revert to using GQL to regain "old" behavior). If we don't feel comfortable with removingLineMatchesin this PR, I would prefer holding out until 4.0 release so I can have the opportunity to piggy back on the "breaking change" version bump.I think the follow up we want is:
search -streamthe defaultsearchsearch -gqlinstead (only useful for clients who want to get JSON I imagine).Test plan
Updates tests