Skip to content

fix(config): round-trip the fetchRecurse config key correctly#63

Merged
bashandbone merged 1 commit into
mainfrom
fix/config-camelcase-keys-silent-drop
Jun 29, 2026
Merged

fix(config): round-trip the fetchRecurse config key correctly#63
bashandbone merged 1 commit into
mainfrom
fix/config-camelcase-keys-silent-drop

Conversation

@bashandbone

Copy link
Copy Markdown
Owner

What & why

Fixes the P0-3 finding from the test audit (#62): submod silently dropped the documented fetchRecurse config key and could not read back the fetch-recurse setting it wrote.

Two distinct defects, both real and round-trip-breaking:

  1. Read side — the serde field is fetch_recurse with no rename, while the README (and the test suite, and the bundled sample) document the key as fetchRecurse. So a config written per the docs deserialized to None — the value was silently dropped.
  2. Write side — both config writers (save_config and write_full_config) emitted fetch = "<to_gitmodules>". That's the wrong key (fetch, neither documented fetchRecurse nor field fetch_recurse) and the wrong value form (to_gitmodules() yields git's true/false, but the deserializer expects the serde always/never). So submod could not parse a config it had just written.

The existing test_change_global_sets_update_and_fetch actually enshrined the bug — it asserted the output contained fetch = "true", with a comment rationalizing it.

Approach

  • Standardize on the documented fetchRecurse key via #[serde(rename = "fetchRecurse")] on all three structs (SubmoduleGitOptions, SubmoduleDefaults, SubmoduleEntry). rename (not alias) is required because Config::load merges layers through figment at the raw-dict level — an alias would leave the file layer (fetchRecurse) and the CLI-provider layer (fetch_recurse) as two different keys, which serde then rejects as a duplicate field. A single canonical spelling matches how ignore/update already behave.
  • Add SerializableFetchRecurse::as_config_value() returning the serde kebab form (on-demand/always/never), explicitly distinct from to_gitmodules() (git's true/false), and use it in both writers.
  • Update the writer key to fetchRecurse and add it to the known-key lists.
  • Update the bundled sample_config/submod.toml comments.

Tests (TDD — each watched fail first)

  • test_config_toml_fetch_recurse_camelcase_key_is_parsed — deserializes the documented fetchRecurse key and asserts the parsed value (not file text).
  • test_write_full_config_fetch_recurse_round_trips — writes config via write_full_config, reloads, asserts Always/Never survive — the core regression test.
  • test_fetch_recurse_as_config_value_uses_serde_form_not_git_bools — pins the TOML value form against regressing to the git encoding.
  • Corrects test_change_global_sets_update_and_fetch to assert the round-trippable fetchRecurse = "always" and that a subsequent check succeeds.

Full suite: 518 tests pass.

Notes

  • config.rs and git_manager.rs also pick up rustfmt (1.8.0) normalization of pre-existing unformatted lines — required to pass the cargo fmt pre-commit gate, which currently fails on ~154 pre-existing diffs across the repo. Reformatting was confined to the two files this PR already touches.
  • Follow-ups (not in this PR): the JSON schemas (schemas/v1.0.0, schemas/v1.1.0) still define the key as fetch with additionalProperties: false, so they'd reject the documented fetchRecurse; and schemas/v1.1.0/...json is currently malformed JSON (a stray ""). The repo-wide rustfmt drift (~154 diffs across 13 files) is also worth a standalone style: commit.

Closes the P0-3 item in #62.

🤖 Generated with Claude Code

submod silently dropped the documented `fetchRecurse` TOML key on load and
could not read back the fetch-recurse setting it wrote:

- The serde field was `fetch_recurse` with no rename, so the documented
  `fetchRecurse` key (per README) deserialized to nothing.
- Both config writers emitted `fetch = "<to_gitmodules>"` — the wrong TOML
  key *and* git's `true`/`false` value encoding instead of the serde
  `always`/`never` form — so a written value never parsed back.

Standardize on the documented `fetchRecurse` key via `#[serde(rename)]`
(matching how `ignore`/`update` already behave, and avoiding the figment
layered-merge duplicate-key error that `alias` would trigger), add
`SerializableFetchRecurse::as_config_value()` for the TOML value form, and
fix both writers plus the known-key lists. Update the sample config comments.

Tests: a deserialize test for the documented key, an as_config_value unit
test, and a write->read round-trip regression test through write_full_config.
Corrects test_change_global_sets_update_and_fetch, which previously asserted
the broken `fetch = "true"` output.

config.rs and git_manager.rs also pick up rustfmt (1.8.0) normalization of
pre-existing unformatted lines, required to pass the cargo fmt gate.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T8D5ZK1473YCiZkbueAY2X
@codecov

codecov Bot commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.09091% with 28 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/config.rs 73.23% 19 Missing ⚠️
src/git_manager.rs 89.88% 9 Missing ⚠️
Files with missing lines Coverage Δ
src/options.rs 94.76% <100.00%> (+0.11%) ⬆️
src/git_manager.rs 90.36% <89.88%> (-0.17%) ⬇️
src/config.rs 81.97% <73.23%> (+0.25%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bashandbone bashandbone merged commit 441f41b into main Jun 29, 2026
4 of 8 checks passed
@bashandbone bashandbone deleted the fix/config-camelcase-keys-silent-drop branch June 29, 2026 20:29
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