refactor: reduce smallvec dep from git-date#2593
Conversation
There was a problem hiding this comment.
💡 Codex Review
gitoxide/gix-date/src/parse/mod.rs
Line 34 in 0a38780
For every Time::to_str() call where the serialized timestamp is shorter than Time::MAX.size(), as_str() now returns the entire fixed-size backing array, including trailing \0 bytes left after clear(). That changes the public string from e.g. "0 +0000" to the same text plus NUL padding, so callers comparing, formatting, or writing commit headers get invalid extra bytes; this should use the tracked idx length when creating time_bytes.
ℹ️ 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 (@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 (@codex) address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR refactors gix-date’s time serialization buffer to remove the smallvec dependency by replacing SmallVec-backed storage with a fixed-size stack buffer and a custom std::io::Write implementation.
Changes:
- Replace
TimeBuf’sSmallVec<[u8; N]>backing with[u8; N]plus a write index and implementio::Writedirectly. - Remove
smallvecfromgix-datedependencies and updateCargo.lockaccordingly.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| gix-date/src/parse/mod.rs | Refactors TimeBuf storage and implements io::Write to eliminate SmallVec. |
| gix-date/Cargo.toml | Drops the smallvec dependency from gix-date. |
| Cargo.lock | Removes smallvec from the resolved dependency list for gix-date. |
Sebastian Thiel (Byron)
left a comment
There was a problem hiding this comment.
Thanks for tackling this.
I left a note that should help steer this to completion.
Sebastian Thiel (Byron)
left a comment
There was a problem hiding this comment.
Putting it to draft while the comment isn't addressed.
This reminds me of my rule to do these things myself, but here I am far enough down the wrong side of the road so might as well keep passive aggressively repeating myself here 😁.
Sebastian Thiel (Byron)
left a comment
There was a problem hiding this comment.
Ah, right, sorry for that.
To me it seemed like nothing changed.
It would have helped if there would have been a summary of what you did along with "I think it can be merged now".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efaeb714e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
Signed-off-by: tison <wander4096@gmail.com>
efaeb71 to
df23312
Compare
|
Sebastian Thiel (@Byron) Thanks for your review and merging. I enjoy this kind of collaboration :D
It's actually included in #2593 (comment), I quote:
But for sure I can repeat it in the next comment to make it clear :P |
Resolve TODO when encountering it. Comments inline.
cc Sebastian Thiel (@Byron)