gitserver: port FirstEverCommit functionality from client to git cli backend#62165
Conversation
0b26aab to
2443801
Compare
01ed324 to
9147a1a
Compare
9147a1a to
732f339
Compare
2443801 to
5d7fa42
Compare
732f339 to
bf4e953
Compare
5d7fa42 to
772f91b
Compare
bf4e953 to
cf13ed9
Compare
cf13ed9 to
26d91b0
Compare
There was a problem hiding this comment.
This exit condition comes from https://sourcegraph.com/github.com/git/git@bf995e7a4f94a9388aa8042dc9e338f3fcb75496/-/blob/builtin/rev-list.c?L701-706 in the source I believe.
There was a problem hiding this comment.
strange that it'd print the usage 😬 but thanks for the link, very helpful!
eseliger
left a comment
There was a problem hiding this comment.
This is quite brittle, as any unrelated formatting change with our errors could change the semantics here. I decided to create a new sentinel error for this condition, gitdomain.RepoEmptyErr, to 1) better state our intent and 2) make this check more robust against minor formatting changes in our error libraries. When we generate the gRPC bindings, we'll need to create a new protobuf message for this error and write ToProto/FromProto functions, etc.
Strongly agree that no caller should depend on any git error message. We want to fully own the process of potentially switching to go-git, libgit2, or any self-made implementations that will yield different error strings.
One question - in alignment with gRPC saying "there should not be too many different error codes, because callers will most likely not add more than a few checks", should we try to keep the error types in check as well, and repurpose the RevisionNotFoundError here?
I think it's technically a correct error, since FirstEverCommit would try to look at HEAD, HEAD resolves to no branch yet, so HEAD is not found?
There was a problem hiding this comment.
Should we remove this, after the gRPC API PR is merged? Once the client makes use of the gRPC API there's no way this could still be returned.
There was a problem hiding this comment.
There was a problem hiding this comment.
strange that it'd print the usage 😬 but thanks for the link, very helpful!
78388ea to
1bbfc10
Compare
1bbfc10 to
521a52c
Compare
521a52c to
05a1f32
Compare
05a1f32 to
0dbd2b6
Compare
0dbd2b6 to
dd47983
Compare
0a3ffb5 to
a8d5080
Compare
a8d5080 to
df87c59
Compare
…ntation (#62169) Part of #61689 This PR builds on https://github.com/sourcegraph/sourcegraph/pull/62165, and adds the FirstEverCommit new gRPC method along with a corresponding server implementation. The client implementation will be in a future PR. ## Test plan - Unit tests
Part of #61689
Overview
This PR adapts the existing FirstEverCommit logic from the gitserver client to our new git cli backend. This is the first step as part of a set of stacked PRs to port the functionality to gRPC.
Existing
func (c *clientImplementor) FirstEverCommit()implementation for reference:https://github.com/sourcegraph/sourcegraph/blob/412654f098492856f32a6c561eda471800924d71/internal/gitserver/commands.go#L1734-L1757
Note
One of the existing tests that I ported called out that code insights depended on the formatting of a specific error string in order to detect if the repository was empty.
(Test in question):
https://github.com/sourcegraph/sourcegraph/blob/948848bca6e87e84878a9c1a9222d36734103fa2/internal/gitserver/commands_test.go#L721-L729
(String checking logic in code insights):
https://github.com/sourcegraph/sourcegraph/blob/2443801b4ea4a4d48a8badf3c34ed2761af65db4/internal/insights/gitserver/first_commit.go#L18-L38
This is quite brittle, as any unrelated formatting change with our errors could change the semantics here. I decided to create a new sentinel error for this condition,
gitdomain.RepoEmptyErr, to 1) better state our intent and 2) make this check more robust against minor formatting changes in our error libraries. When we generate the gRPC bindings, we'll need to create a new protobuf message for this error and writeToProto/FromProtofunctions, etc.Test plan
Adapted existing unit tests