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

codeintel: GitTreeTranslator rewrite#63938

Merged
kritzcreek merged 8 commits into
mainfrom
christoph/git-tree-translator-refactor
Jul 24, 2024
Merged

codeintel: GitTreeTranslator rewrite#63938
kritzcreek merged 8 commits into
mainfrom
christoph/git-tree-translator-refactor

Conversation

@kritzcreek

@kritzcreek kritzcreek commented Jul 19, 2024

Copy link
Copy Markdown
Contributor

Closes https://linear.app/sourcegraph/issue/GRAPH-742/simplify-gittreetranslator-implementation

  • use unified diff format and 0 context lines to allow translation by just using hunk headers
  • only request hunks per file once, and sync follow-up requests for the same file
  • don't bother LRU caching the full hunk contents, just store 4 int32's per hunk
  • replace linear search through sorted hunks with binary search

Current PR keeps the old PR and just proxies to the new implementation. Once this PR is reviewed and merged I'll remove the old API and update all callsites.
Idea is to make review easier.

I'd recommend just reviewing the new implementation, as the diff view on this is pretty useless.

Test plan

New/existing tests

@cla-bot cla-bot Bot added the cla-signed label Jul 19, 2024
@github-actions github-actions Bot added team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform labels Jul 19, 2024
@kritzcreek kritzcreek force-pushed the christoph/git-tree-translator-refactor branch 6 times, most recently from 9127720 to 5ec22a0 Compare July 22, 2024 01:55
@kritzcreek kritzcreek force-pushed the christoph/git-tree-translator-refactor branch from 5ec22a0 to d434856 Compare July 22, 2024 08:59
Christoph Hegemann added 2 commits July 23, 2024 07:35
use unified diff format and 0 context lines to allow translation by
just using hunk headers

only request hunks per file once, and sync follow-up requests for the
same file

don't bother LRU caching the full hunk contents, just store 4 int32's
per hunk

replace linear search through sorted hunks with binary search
@kritzcreek kritzcreek force-pushed the christoph/git-tree-translator-refactor branch from e84cb5f to f993ba0 Compare July 23, 2024 05:35
@kritzcreek kritzcreek changed the title WIP: git tree translator rewrite chore(codeintel): GitTreeTranslator rewrite Jul 23, 2024
@kritzcreek kritzcreek changed the title chore(codeintel): GitTreeTranslator rewrite codeintel: GitTreeTranslator rewrite Jul 23, 2024
@kritzcreek kritzcreek marked this pull request as ready for review July 23, 2024 05:37

@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.

Largely LGTM, will take a look at tests and the remaining logic after 2-3 hours.

Comment thread internal/codeintel/codenav/gittree_translator.go Outdated
Comment thread internal/codeintel/codenav/gittree_translator.go Outdated
Comment thread internal/codeintel/codenav/gittree_translator.go
Comment thread internal/codeintel/codenav/gittree_translator.go
Comment thread internal/codeintel/codenav/gittree_translator.go Outdated
Comment thread internal/codeintel/codenav/gittree_translator.go Outdated
Comment thread internal/codeintel/codenav/gittree_translator.go
@varungandhi-src

Copy link
Copy Markdown
Contributor

I'd recommend just reviewing the new implementation, as the diff view on this is pretty useless.

Thanks for flagging this. 👍🏽

Comment thread internal/codeintel/codenav/gittree_translator.go Outdated
Comment thread internal/codeintel/codenav/gittree_translator.go Outdated
@kritzcreek kritzcreek merged commit 34e40c3 into main Jul 24, 2024
@kritzcreek kritzcreek deleted the christoph/git-tree-translator-refactor branch July 24, 2024 02:13
kritzcreek pushed a commit that referenced this pull request Jul 24, 2024
Following through after #63938 

## Test plan

Existing tests continue to pass
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants