Stream git blame results #44199
Stream git blame results #44199
Conversation
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits 55825b2 and c514c53 or learn more. Open explanation
|
mrnugget
force-pushed
the
mrn+jh/streaming-git-blame
branch
from
71a5a3f to
3ede673
Compare
|
@mrnugget I've tested our new code against the renderer, by just reading things on the server instead of streaming to the client, it works as intented. At this point, I'm inclined to reach out to @philipp-spiess to see if we can get started on the frontend side. |
|
@philipp-spiess in order to update the frontend to use that new feature:
You can test this for yourself by running the following As a reference, here is what we're sending back through the normal graphql call: {
"data": {
"repository": {
"commit": {
"blob": {
"blame": [
{
"startLine": 16,
"endLine": 17,
"author": {
"person": {
"email": "frenchben@docker.com",
"displayName": "French Ben",
"user": null
},
"date": "2018-01-31T14:01:02Z"
},
"message": "Update error output to be clean",
"rev": "bbca6551549492486ca1b0f8dee45553dd6aa6d7",
"commit": {
"url": "/github.com/hashicorp/go-multierror/-/commit/bbca6551549492486ca1b0f8dee45553dd6aa6d7"
}
},
// ...
]
}
}
}
}
}🤝 @philipp-spiess if you need anything from me regarding this, just reach me out, I'll make myself available so we can progress on this asap 🙏. |
| if _, err = w.Write(encoded); err != nil { | ||
| return err | ||
| } | ||
| flusher.Flush() |
There was a problem hiding this comment.
I expect that flushing here is not desirable since flushing is a blocking operation that will likely be slower than just marshaling the next chunk. The OS flushing is almost definitely good enough.
There was a problem hiding this comment.
This is copied from the streaming-search server-sent events write: https://github.com/sourcegraph/sourcegraph/blob/3033610b103e82904dfa7eabd33e1494c8520d3a/internal/search/streaming/http/writer.go#L100 They flush after every event/data.
There was a problem hiding this comment.
Yes, but we wrap that writer in a buffer, which collects events until we get to 32KB payloads
There was a problem hiding this comment.
Ah, interesting. But that's not the OS flushing then, right?
There was a problem hiding this comment.
Yes, that's right. We don't use OS flushing because we buffer JSON entries, then build the JSON array on flush, so it's not actually raw bytes we're buffering.
|
https://github.com/sourcegraph/sourcegraph/pull/44476 😎 Most of the stuff here is pretty straight forward. I decided to add an early-flush once we have received the first 50 hunks and emit those so we can have a fast initial render and then wait for the remainder of the hunks to go through before doing the final flush. This is something we can tweak later on though! |
|
It looks like the two of you already talked about this, but just so it's recorded here, here's what I sent to JH yesterday:
|
Co-authored-by: Taras Yemets <yemets.taras@gmail.com>
| if err := checkSpecArgSafety(string(opt.NewestCommit)); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
It's usually OK to check this arg for safety, but not strictly necessary because we filter potentially malicious arguments in IsAllowedGitCmd.
|
@philipp-spiess could you deal with the conflicts on the tsx files so we can merge this 🙏 ? |
| base.Path("/lsif/upload").Methods("POST").Name(LSIFUpload) | ||
| base.Path("/search/stream").Methods("GET").Name(SearchStream) | ||
| base.Path("/compute/stream").Methods("GET", "POST").Name(ComputeStream) | ||
|
|
There was a problem hiding this comment.
nit: Why the extra spaces?
There was a problem hiding this comment.
nit: forgot about it and brain didn't register when looking at my own code
| w.WriteHeader(http.StatusInternalServerError) | ||
| return | ||
| } | ||
| if gitdomain.IsRepoNotExist(err) { |
There was a problem hiding this comment.
That error implements NotFound so will be caught by errcode.IsNotFound(err) below.
| if strings.HasPrefix(requestedPath, "/") { | ||
| requestedPath = strings.TrimLeft(requestedPath, "/") | ||
| } |
There was a problem hiding this comment.
| if strings.HasPrefix(requestedPath, "/") { | |
| requestedPath = strings.TrimLeft(requestedPath, "/") | |
| } | |
| requestedPath = strings.TrimPrefix(requestedPath, "/") |
internal/gitserver/client.go
Outdated
| "golang.org/x/sync/errgroup" | ||
| "golang.org/x/sync/semaphore" | ||
|
|
||
| "github.com/sourcegraph/sourcegraph/internal/actor" |
There was a problem hiding this comment.
nit: Should be grouped at the bottom with other /sourcegraph imports
| func streamBlameFileCmd(ctx context.Context, checker authz.SubRepoPermissionChecker, repo api.RepoName, path string, opt *BlameOptions, command gitCommandFunc) (HunkReader, error) { | ||
| a := actor.FromContext(ctx) | ||
| if hasAccess, err := authz.FilterActorPath(ctx, checker, a, repo, path); err != nil || !hasAccess { | ||
| return nil, err |
There was a problem hiding this comment.
When hasAccess is false we may return nil, nil here which may break things higher up.
I'd suggest returning a specific Not Authorized error
There was a problem hiding this comment.
Fixed, thank you for catching this 🙏
| // | ||
| // Because we do not control when p.Read is called, we have to account for | ||
| // the context being cancelled, to avoid leaking the goroutine running p.parse. | ||
| func (p hunkParser) parse(ctx context.Context) { |
There was a problem hiding this comment.
I think this is safe in this case, but still strange to see a non pointer receiver on a relatively complex type. There's a chance that this leads to bugs later if new fields are added that need to be mutated.
@jhchabran done! |
…ourcegraph into mrn+jh/streaming-git-blame
Co-authored-by: Jean-Hadrien Chabran <jh@chabran.fr> Co-authored-by: Philipp Spiess <hello@philippspiess.com> Co-authored-by: Taras Yemets <yemets.taras@gmail.com>
(@jhchabran editing)
This PR adds a new endpoint,
/.api/blame/:repo+rev/stream/:pathwhich streams back the result from runninggit blamewith the--incrementalflag added.If the "enable-streaming-git-blame" feature flag is enabled, the client will use that route to fetch back the hunks instead of going through GraphQL.
To reviewers: the backend code is reviewable, for the client part, @philipp-spiess will do another quick pass on monday morning. cc @mrnugget if you want to take a look, as I can't add you as a reviewer on your own PR.
Test plan
Tested on the scaletesting instance. Will be enabled on s2 as soon as it get merged.
See the results: https://github.com/sourcegraph/sourcegraph/pull/44199#issuecomment-1319035227