fix(tree): deduplicate entry-finding logic#2459
fix(tree): deduplicate entry-finding logic#2459datdenkikniet wants to merge 2 commits intoGitoxideLabs:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0859a9f00
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
gix/src/object/tree/mod.rs
Outdated
| self.id = oid; | ||
| self.repo.find(&oid, buf).map(Some) |
There was a problem hiding this comment.
Setting self.id before self.repo.find() means peel_to_entry() mutates the tree state even when loading the next component returns an error (for example, a missing/corrupt intermediate object). In that error path self.data is still from the previous tree while self.id now points at an object that was never loaded, so callers that recover from the error can observe an inconsistent tree and get misleading follow-up behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't disagree, but I'm not looking to fix bugs with this implementation at this point in time (if this even is a bug, that's not entirely clear. That's effectively the answer to #2456)
|
I just remembered that I'm keeping both commits for posterity, but if we're going to merge it I can squash them :) I will write sensible docs for |
datdenkikniet
force-pushed
the
unify-traversal
branch
4 times, most recently
from
a53061f to
e31aa6d
Compare
The logic for finding entries is duplicated 3 times. With an (unhealthy) smothering of generics, the common functionality can be extracted into a single function that can be reused in all places.
Instead of doing some messed up callback stuff, we can also use std::ops::ControlFlow. It is slightly more verbose, but definitely easier to read.
datdenkikniet
force-pushed
the
unify-traversal
branch
from
e31aa6d to
1531458
Compare
1 participant
I was poking around in the tree iteration code and saw an opportunity for some deduplication.
I'm not sure if the end result is worth it: a lot of generics and fairly opaque logic (looking at the direct implementation of
Tree::peel_to_entryis... Not very satisfying :P), but I figured I'd put this here in case it is something that can be merged.I'm not sure if the
pub fn lookup_entryadheres to the style guide, but I'm more than willing to find a better solution if that is all that is in the way of fixing this.