Repository navigation
chore(config): remove keys walgit parses but never reads - #56
Conversation
PR SummaryMedium Risk Overview
Tests no longer opt into Reviewed by Cursor Bugbot for commit f7633ac. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 22 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Remove configuration fields and validate parsing crates/walgit-config/src/lib.rs, walgit.example.toml |
The configuration structs, defaults, and example TOML omit the removed options. Tests check rejection of removed settings and update environment-override cases. |
Update server tests and integrity documentation crates/walgit-server/src/middleware.rs, crates/walgit-server/tests/*, docs/INTEGRITY.md |
Server tests no longer set the removed options. Middleware and integrity documentation are updated to describe the configuration and object-want behavior. |
Git Ignore Update
| Layer / File(s) | Summary |
|---|---|
Ignore .DS_Store files .gitignore |
Adds .DS_Store to the ignored-file patterns. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Other
Suggested reviewers: tobi
Merge Risk: 
959fd
-
No actionable merge-blocking issue was identified in the configuration cleanup and documentation updates. Merge after normal checks pass.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly identifies the main change: removing unused configuration keys that walgit parses but never reads. Description check The description directly explains the removed configuration keys, the arbitrary object-want behavior, and the grouped changes. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 3 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the config keys,
Then hops past settings now removed.
The examples match the fields that stay,
While tests confirm the new defaults..DS_Store hides from Git today,
And tidy paths delight the hare.
898443e to
86995f8
Compare
## Summary ### Why? Partial-clone lazy fetches require direct object wants, while the configuration claimed they could be disabled even though both upload-pack engines always accepted them. ### What? Remove the inert `git.allow_any_sha1_in_want` option and update tests and integrity documentation to describe arbitrary object wants as part of partial-clone support. ## Test Plan⚠️ Rust tests were not run because `cargo` is unavailable in this environment; `git diff --check` passes. ## Issue Closes tobi#77 (cherry picked from commit 06a0ef3) Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Clarify that repository read access already permits fetching stored objects by ID, including unreachable objects, so removing the ineffective configuration switch does not widen authorization. (cherry picked from commit b0b18b1) Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
## Summary ### Why? The server exposed global concurrency and total request timeout settings without a safe way to apply them. Enforcing them around the whole router would queue health probes behind clones and terminate long streaming Git, LFS, and pack responses mid-transfer. ### What? Remove `server.max_concurrent_requests` and `server.request_timeout`, along with the unsafe middleware implementation and misleading documentation. Keep the existing per-repository Git concurrency limit, and reject the removed keys instead of silently accepting unsupported controls. ## Test Plan ✅ `cargo test -p walgit-config removed_global_request_limit_configuration_is_rejected` ✅ `cargo check -p walgit-server` ✅ `cargo clippy -p walgit-server --all-targets -- -D warnings` ✅ `cargo fmt --all -- --check` ✅ `git diff --check` ## Issue Closes tobi#78 (cherry picked from commit c7ab81a) Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
## Summary ### Why? `git.binary` implied that every Git subprocess could use an operator-selected executable, but the implementation consistently invokes `git` from `PATH` and never read the setting. ### What? Remove the unsupported configuration field and example entry, and verify that the obsolete key is rejected instead of silently suggesting custom-binary support. ## Test Plan ✅ `cargo test -p walgit-config removed_git_binary_configuration_is_rejected` ✅ `cargo fmt --all -- --check` ✅ `git diff --check` ## Issue Closes tobi#81 (cherry picked from commit 129fcc2) Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
`server.http2` and `store.gcs.direct_connectivity` were accepted configuration values with no runtime consumers, so changing them gave operators a false impression that transport behavior changed. Remove both unsupported switches from the configuration surface and example file, and add a regression test proving the obsolete keys are rejected. ✅ `cargo test -p walgit-config removed_transport_switches_are_rejected` ✅ `cargo fmt --all -- --check` ✅ `git diff --check` Closes tobi#83 (cherry picked from commit a200384) Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
## Summary ### Why? Finder-created `.DS_Store` files appeared as untracked changes throughout the repository and could be committed accidentally. ### What? Ignore `.DS_Store` recursively from the root ignore file. ## Test Plan ✅ `git check-ignore -v .DS_Store crates/.DS_Store crates/walgit-server/.DS_Store` ✅ `git diff --check` ## Issue Closes tobi#84 (cherry picked from commit ef1120d) Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
86995f8 to
f7633ac
Compare
git.allow_any_sha1_in_want,git.binary,server.http2,server.max_concurrent_requestsandserver.request_timeoutexpecting an effect, and nothing reads them; rejecting them stops the config from lying about what the server does.