Skip to content

GTST-19 refactor: improve coding style with pedantic lints and shared helpers#16

Merged
MonteYin merged 1 commit intomainfrom
GTST-19.refactor/coding-style
Mar 26, 2026
Merged

GTST-19 refactor: improve coding style with pedantic lints and shared helpers#16
MonteYin merged 1 commit intomainfrom
GTST-19.refactor/coding-style

Conversation

@MonteYin
Copy link
Copy Markdown
Owner

Summary

  • Add lint/format tooling config (rustfmt.toml, clippy.toml, rust-toolchain.toml, Cargo.toml [lints]) with clippy::pedantic enabled
  • Extract shared git2 helpers into git/mod.rs (diff_opts_with_untracked, delta_path, hunk_header, is_binary_delta)
  • Add LineTag::from_origin(), PROTOCOL_VERSION constant, and dispatch() helper to reduce duplication
  • Deduplicate test setup into tests/common/mod.rs
  • Net -290 lines (348 added, 638 removed)
  • Bump version to 0.1.2

Test plan

  • cargo fmt --check passes
  • cargo clippy --all-targets — zero warnings
  • cargo test — all 82 tests pass
  • cargo build --release — binary runs correctly (--version, diff, stage, status, protocol)
  • Manual stage test on real repo verified

…lpers, and dedup

- Add rustfmt.toml, clippy.toml, rust-toolchain.toml for consistent tooling
- Enable clippy::pedantic in Cargo.toml [lints] and fix all warnings
- Extract shared git2 helpers into git/mod.rs (diff_opts, delta_path, hunk_header, is_binary_delta)
- Add LineTag::from_origin() and PROTOCOL_VERSION constant to models.rs
- Extract dispatch() helper in protocol.rs to reduce match arm duplication
- Deduplicate test helpers into tests/common/mod.rs
- Bump version to 0.1.2
@MonteYin
Copy link
Copy Markdown
Owner Author

Review -- Opus 4.6

Review Result

Overall Verdict: PASS


1. Correctness

Verdict: PASS

All refactoring changes preserve existing behavior:

  • diff_opts_with_untracked() in status.rs: The original code used DiffOptions::new() without explicit context_lines(3), relying on libgit2's default (which is 3). The shared helper now explicitly sets context_lines(3), making the implicit explicit. No behavioral change.
  • print_json() helper in output.rs: Both old and new code pass &DiffOutput (a reference) to Response::success(), creating Response<&DiffOutput>. Serde serializes references transparently, so JSON output is identical.
  • let _ = writeln!(patch, ...) pattern in reconstruct_patch(): fmt::Write for String is infallible, so discarding the Result is safe and idiomatic.
  • is_binary_delta() centralization: This helper is now consistently used across all four foreach callsites (diff.rs delta callback, stage.rs scan_hunk_metadata, collect_hunks, and the hunk-level staging scan loop). This resolves the inconsistency I noted in PR GTST-10 feat: integration tests and binary file fix #7 where only 2 of 4 sites had the check.
  • LineTag::from_origin(): Exact same match arms as the inline code it replaces. No behavioral change.
  • PROTOCOL_VERSION constant: Type u8 matches the Response.version field type. Values unchanged at 1.

Issues

Minor
  • No dedicated unit test for LineTag::from_origin(), but it is exercised transitively through integration tests that verify diff output content. Acceptable for a trivial method.

2. Architecture

Verdict: PASS

The extraction of shared helpers into git/mod.rs is well-motivated and well-scoped:

  • diff_opts_with_untracked() and diff_opts_tracked_only(): Good separation. The two variants have clearly documented use cases. The CONTEXT_LINES constant centralizes a value that was duplicated across 4 callsites.
  • delta_path(), hunk_header(), is_binary_delta(): These were copy-pasted across diff.rs and stage.rs (6+ callsites). Extracting them into the parent module is the right scope -- they're git2-specific helpers shared by submodules.
  • dispatch() in protocol.rs: Clean dedup of the Ok -> success / Err -> error pattern that was repeated 3 times.
  • tests/common/mod.rs: Correct use of mod common + use common::{...} pattern for sharing test utilities across integration test files. The #![allow(dead_code)] is standard practice here.
  • resolve_format visibility change (pub to fn): Good. It was only used within output.rs.

3. Security

Verdict: PASS

No security-relevant changes in this PR. The refactoring is purely structural -- no new I/O paths, no changes to input validation, no changes to how user input reaches git operations.


4. Performance

Verdict: PASS

No performance impact. Helper functions are trivially inlineable. The use std::fmt::Write import inside reconstruct_patch (moved from top-level) has zero runtime cost.


5. Simplicity

Verdict: PASS

Net -290 lines is a meaningful reduction. The changes follow KISS and DRY:

  • Eliminated 6+ copy-paste sites for delta_path, hunk_header, is_binary_delta, and LineTag matching.
  • Eliminated 3 duplicate setup_repo() + gitsift() + parse_json() definitions across test files.
  • Replaced verbose match + Option patterns with let ... else (idiomatic Rust 2024).
  • rustfmt.toml with max_width = 100 and use_small_heuristics = "Max" produces denser formatting that reduces vertical noise without sacrificing readability.

Issues

Minor
  • clippy.toml thresholds (too-many-arguments-threshold = 7, cognitive-complexity-threshold = 30) are generous. These are reasonable for a codebase of this size, but worth revisiting if the codebase grows. Not blocking.
  • CI change from cargo clippy --all-targets -- -D warnings to cargo clippy --all-targets: Default clippy warnings will no longer fail CI (only the three deny-level lints will). The intent is to manage lint policy via Cargo.toml [lints] rather than CI flags, which is a valid architectural choice. Worth being aware that this is a deliberate relaxation of CI strictness for non-configured lints.

Summary

Clean refactoring PR that achieves its stated goals: shared helpers eliminate copy-paste across diff.rs and stage.rs, test deduplication removes 3 identical setup_repo implementations, and pedantic lints are configured via Cargo.toml rather than ad-hoc. The is_binary_delta() centralization resolves the inconsistency flagged in PR #7. No correctness, security, or performance concerns. The CI clippy change is a deliberate policy shift (lint config in Cargo.toml over CI flags) that is reasonable for a project enabling pedantic lints.

@MonteYin MonteYin merged commit 79f820f into main Mar 26, 2026
4 checks passed
@MonteYin MonteYin deleted the GTST-19.refactor/coding-style branch March 26, 2026 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant