gitserver: Add CommitLog API to replace client-side Commits #62606
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced May 10, 2024
github-actions
bot
added
team/product-platform
team/source
This was referenced May 10, 2024
eseliger
force-pushed
the
es/05-10-gitserverfixdurationreprotedforchangedfiles
branch
from
58c7993
to
d6d0969
Compare
eseliger
force-pushed
the
es/05-11-gitserveraddcommitlogapitoreplaceclient-sidecommits
branch
from
0c868de
to
223176c
Compare
eseliger
force-pushed
the
es/05-10-gitserverfixdurationreprotedforchangedfiles
branch
from
d6d0969
to
245f9d6
Compare
eseliger
force-pushed
the
es/05-11-gitserveraddcommitlogapitoreplaceclient-sidecommits
branch
from
223176c
to
75decf8
Compare
eseliger
force-pushed
the
es/05-10-gitserverfixdurationreprotedforchangedfiles
branch
from
245f9d6
to
8d42c6f
Compare
eseliger
force-pushed
the
es/05-11-gitserveraddcommitlogapitoreplaceclient-sidecommits
branch
from
75decf8
to
218c861
Compare
eseliger
force-pushed
the
es/05-10-gitserverfixdurationreprotedforchangedfiles
branch
from
8d42c6f
to
683aa34
Compare
eseliger
force-pushed
the
es/05-11-gitserveraddcommitlogapitoreplaceclient-sidecommits
branch
from
218c861
to
6a12a69
Compare
eseliger
force-pushed
the
es/05-10-gitserverfixdurationreprotedforchangedfiles
branch
from
683aa34
to
dfd8c89
Compare
eseliger
force-pushed
the
es/05-11-gitserveraddcommitlogapitoreplaceclient-sidecommits
branch
from
6a12a69
to
8d6dc0c
Compare
Base automatically changed from
es/05-10-gitserverfixdurationreprotedforchangedfiles
to
main
May 13, 2024 12:38
eseliger
force-pushed
the
es/05-11-gitserveraddcommitlogapitoreplaceclient-sidecommits
branch
5 times, most recently
from
4af6a15
to
b3a07ae
Compare
eseliger
commented
May 17, 2024
Comment on lines
+19
to
+25
| // TODO: | ||
| // Add tests for: | ||
| // - AllRefs | ||
| // - FollowOnlyFirstParent | ||
| // - Order | ||
| // - FollowPathRenames | ||
|
|
There was a problem hiding this comment.
These didn't exist previously, I added tests for a few more settings here than we had before, but these are a bit more involved and don't impact the goal of finalizing the migration - so I decided to leave that for a rainy day.
eseliger
commented
May 17, 2024
Comment on lines
+407
to
409
| message CommitLogResponse { | ||
| repeated GetCommitResponse commits = 1; | ||
| } |
There was a problem hiding this comment.
Thoughts on this? It's basically a batched GetCommit, and that response already carries the optional modified_files slice that we need for subrepo perms checks.
Alternatives I considered:
- Making a separate type, but otherwise same contents
- Add ModifiedFiles to
message GitCommitdirectly, and use field masks to not compute it (sadly they're purely additive so I'd need to always send a mask for the default case of don't include them) - Use a
viewfield like Google has in their gRPC APIs (I dislike that kinda, because there's no good tooling to know which view of a resource you got back -> easy to use a field on the client side that wasn't actually sent
eseliger
force-pushed
the
es/05-11-gitserveraddcommitlogapitoreplaceclient-sidecommits
branch
4 times, most recently
from
413fc7e
to
535cfbc
Compare
This PR moves the Commits API to the server side and is the last gRPC API to migrate. Test plan: Existing tests are still passing, moved the ~extensive unit test coverage this had on the client side to the server side.
eseliger
force-pushed
the
es/05-11-gitserveraddcommitlogapitoreplaceclient-sidecommits
branch
from
535cfbc
to
2e8df76
Compare
This was referenced May 17, 2024
ggilmore
reviewed
May 20, 2024
| } | ||
|
|
||
| // Safety check: ensure we are always starting with a record separator | ||
| if data[0] != '\x1e' { |
There was a problem hiding this comment.
nit: can we extract that into a constant?
Suggested change
| if data[0] != '\x1e' { | |
| const recordSeparator = '\x1e' | |
| if data[0] != recordSeparator { |
ggilmore
approved these changes
May 20, 2024
eseliger
deleted the
es/05-11-gitserveraddcommitlogapitoreplaceclient-sidecommits
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves the Commits API to the server side and is the last gRPC API to migrate (considering LogReverseEach is already out for review).
Yet again (lack of integration tests), I had to reimplement some logic for the rockskip tests. We should investigate ways to run tests against actual gitservers, ideally without having to compile the whole server image and running that.
Closes #61690
Test plan:
Existing tests are still passing, moved the ~extensive unit test coverage this had on the client side to the server side, and reimplemented the subrepoperms tests for the new backend.