Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

gitserver: Add OctopusMergeBase RPC method#63842

Merged
eseliger merged 1 commit into
mainfrom
es/07-16-gitserveraddoctopusmergebaserpcmethod
Jul 19, 2024
Merged

gitserver: Add OctopusMergeBase RPC method#63842
eseliger merged 1 commit into
mainfrom
es/07-16-gitserveraddoctopusmergebaserpcmethod

Conversation

@eseliger

@eseliger eseliger commented Jul 16, 2024

Copy link
Copy Markdown
Member

This method can be used to find a common ancestor for many commit SHAs, to be used by code intel for finding visible uploads.

Closes SRC-485

Test plan: Added unit tests for the several layers (client, grpc, gitcli).

@cla-bot cla-bot Bot added the cla-signed label Jul 16, 2024

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @eseliger and the rest of your teammates on Graphite Graphite

@github-actions github-actions Bot added team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all labels Jul 16, 2024
@eseliger eseliger marked this pull request as ready for review July 16, 2024 07:32
@eseliger eseliger requested review from a team and varungandhi-src July 16, 2024 07:32
Comment thread cmd/gitserver/internal/git/gitcli/mergebase.go Outdated
Comment thread cmd/gitserver/internal/git/gitcli/mergebase.go Outdated
Comment thread cmd/gitserver/internal/git/gitcli/mergebase.go Outdated
Comment thread cmd/gitserver/internal/git/gitcli/mergebase.go Outdated
Comment thread cmd/gitserver/internal/git/gitcli/mergebase.go Outdated
Comment thread cmd/gitserver/internal/git/iface.go Outdated
Comment thread cmd/gitserver/internal/git/iface.go Outdated

@varungandhi-src varungandhi-src left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo existing review comments

Comment thread internal/gitserver/v1/gitserver.proto Outdated
Comment thread cmd/gitserver/internal/server_grpc.go Outdated
Comment thread cmd/gitserver/internal/server_grpc.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same point as elsewhere wrt potentially logging a large number of strings; is logging all of them necessary/deliberate?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the other one, this one I am keeping as-is because exhaustive logging is meant for debugging, off by default, and supposed to help us understand why we might see performance issues

This method can be used to find a common ancestor for many commit SHAs, to be used by code intel for finding visible uploads.

Closes SRC-481

Test plan:

Added unit tests for the several layers (client, grpc, gitcli).
@eseliger eseliger force-pushed the es/07-16-gitserveraddoctopusmergebaserpcmethod branch from ba24e67 to dafa858 Compare July 19, 2024 10:58
@eseliger eseliger merged commit 175667d into main Jul 19, 2024
@eseliger eseliger deleted the es/07-16-gitserveraddoctopusmergebaserpcmethod branch July 19, 2024 11:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants