chore: Refactor and document GitTreeTranslator#63390
Conversation
Previously, the code had several problems: - There was code which seemed like it was handling path renames across commits, but it was just a no-op. It was not documented why it was implemented that way. - The Position adjustment API and Range adjustment APIs were inconsistent. - The Position and Range adjustment APIs suggested that they could handle file renames, but they don't/cannot. - The mocking logic was just returning wrong outputs (commits instead of paths in one place). This patch documents the rationale behind the lack of rename handling, as well as adds a slow-but-correct form of Document checking.
There was a problem hiding this comment.
Because commits and paths are both strings, this function was meant to return a path as the first return value, and this was just returning a commit, and this was not caught by any of our existing tests.
There was a problem hiding this comment.
This is certainly non-ideal, but in the typical case, we don't expect there to be lots of visible uploads (because of the shadowing semantics in the commitgraph logic), and I didn't want to get side-tracked into writing DB queries, this PR has been enough of a distraction already.
There was a problem hiding this comment.
Let's make an issue to fix this near-term. Writing a query to do an "EXISTS" rather than fetching the document should (hopefully?) not be too much work.
There was a problem hiding this comment.
There was a problem hiding this comment.
In this case and elsewhere, I'm not 100% sure if we should keep chugging along and return 0 results, but if I were the client, I would likely consider this an error (and wouldn't make requests if I didn't know that precise code intel already existed)
Maybe it's worth having a dedicated error type here?
There was a problem hiding this comment.
Let's make a note to go over these during our next pairing session? Feels like I need to read a lot of code to give a good answer here right now.
1dec2cb to
30c1767
Compare
kritzcreek
left a comment
There was a problem hiding this comment.
Approving, because I think this is a net-positive over the status-quo.
|
|
||
| // NewGitTreeTranslator creates a new GitTreeTranslator with the given repository and source commit. | ||
| func NewGitTreeTranslator(client gitserver.Client, args *requestArgs, hunkCache HunkCache) GitTreeTranslator { | ||
| func NewGitTreeTranslator(client gitserver.Client, args *translationBase, hunkCache HunkCache) GitTreeTranslator { |
There was a problem hiding this comment.
Rename args to base here?
There was a problem hiding this comment.
Let's make an issue to fix this near-term. Writing a query to do an "EXISTS" rather than fetching the document should (hopefully?) not be too much work.
There was a problem hiding this comment.
Let's make a note to go over these during our next pairing session? Feels like I need to read a lot of code to give a good answer here right now.
30c1767 to
a9381b5
Compare
Previously, the code had several problems:
commits, but it was just a no-op. It was not documented why it was
implemented that way.
handle file renames, but they don't/cannot.
instead of paths in one place).
This patch documents the rationale behind the lack of rename handling,
as well as adds a slow-but-correct form of Document checking.
Test plan
Covered by existing tests? 🤷🏽
Changelog