Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions src/commands/config/show.rs
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,10 @@ fn format_show_warning(warning: &worktrunk::config::UnknownWarning) -> String {
UnknownWarning::TopLevelWrongConfig {
key,
other_description,
} => cformat!("Key <bold>{key}</> belongs in {other_description} (will be ignored)"),
} => with_scope_note(
cformat!("Key <bold>{key}</> belongs in {other_description} (will be ignored)"),
other_description,
),
UnknownWarning::TopLevelDeprecatedWrongConfig {
key,
other_description,
Expand All @@ -775,13 +778,26 @@ fn format_show_warning(warning: &worktrunk::config::UnknownWarning) -> String {
UnknownWarning::NestedWrongConfig {
path,
other_description,
} => cformat!("Key <bold>{path}</> belongs in {other_description} (will be ignored)"),
} => with_scope_note(
cformat!("Key <bold>{path}</> belongs in {other_description} (will be ignored)"),
other_description,
),
UnknownWarning::NestedUnknown { path } => {
cformat!("Unknown key <bold>{path}</> will be ignored")
}
}
}

/// Append the project-scoped-user-config note when the key belongs in user
/// config, mirroring the load-time warning path. See
/// [`scope_to_repo_note`](worktrunk::config::scope_to_repo_note).
fn with_scope_note(message: String, other_description: &str) -> String {
match worktrunk::config::scope_to_repo_note(other_description) {
Some(note) => format!("{message}; {note}"),
None => message,
}
}

fn render_project_config(out: &mut String) -> anyhow::Result<()> {
// Try to get current repository root
let repo = match Repository::current() {
Expand Down
118 changes: 114 additions & 4 deletions src/config/deprecation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1787,6 +1787,19 @@ pub fn nested_key_belongs_in<C: WorktrunkConfig>(path: &str) -> Option<&'static
.then(C::Other::description)
}

/// Note appended to a "belongs in user config" warning: a key placed in
/// project config that really lives in user config is usually an attempt to
/// scope a personal setting to one repo, which the `[projects."<id>"]` table
/// in user config does directly. Returns `None` for any other destination.
///
/// Keyed off the destination *description* rather than a config-type gate so
/// both warning formatters (load-time and `config show`) can share it — they
/// hold only the `other_description` string, not the config type.
pub fn scope_to_repo_note(other_description: &str) -> Option<&'static str> {
(other_description == crate::config::UserConfig::description())
.then_some(r#"to scope it to this repo, add it under [projects."<id>"] in user config"#)
}

/// Classification of an unknown config key for warning purposes.
pub enum UnknownKeyKind {
/// Deprecated key in its correct config type — deprecation system handles it
Expand Down Expand Up @@ -1871,8 +1884,11 @@ fn format_load_warning(label: &str, warning: &crate::config::UnknownWarning) ->
UnknownWarning::TopLevelWrongConfig {
key,
other_description,
} => cformat!(
"{label} has key <bold>{key}</> which belongs in {other_description} (will be ignored)"
} => with_scope_note(
cformat!(
"{label} has key <bold>{key}</> which belongs in {other_description} (will be ignored)"
),
other_description,
),
UnknownWarning::TopLevelDeprecatedWrongConfig {
key,
Expand All @@ -1884,15 +1900,29 @@ fn format_load_warning(label: &str, warning: &crate::config::UnknownWarning) ->
UnknownWarning::NestedWrongConfig {
path,
other_description,
} => cformat!(
"{label} has key <bold>{path}</> which belongs in {other_description} (will be ignored)"
} => with_scope_note(
cformat!(
"{label} has key <bold>{path}</> which belongs in {other_description} (will be ignored)"
),
other_description,
),
UnknownWarning::NestedUnknown { path } => {
cformat!("{label} has unknown field <bold>{path}</> (will be ignored)")
}
}
}

/// Append the project-scoped-user-config note to `message` when the key's
/// destination is user config (see [`scope_to_repo_note`]). Joined with a
/// semicolon per the house style for related clauses. The note is plain text
/// so its `[projects."<id>"]` placeholder isn't parsed as color-print markup.
fn with_scope_note(message: String, other_description: &str) -> String {
match scope_to_repo_note(other_description) {
Some(note) => format!("{message}; {note}"),
None => message,
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -4491,6 +4521,86 @@ ff = true
);
}

#[test]
fn test_nested_user_only_key_redirects_generally() {
use crate::config::{ProjectConfig, UnknownWarning, UserConfig, collect_unknown_warnings};

// `[list]` is a valid *shared* section (project config accepts `url`),
// but `columns` / `full` are user-config display settings. Placing them
// in project config redirects to user config rather than reading
// "unknown field" — the general "valid in the other config" check, not
// the hard-coded commit.generation list (#3469).
let warnings = collect_unknown_warnings::<ProjectConfig>(
"[list]\ncolumns = [\"branch\"]\nfull = true\n",
);
assert!(
warnings.iter().all(|w| matches!(
w,
UnknownWarning::NestedWrongConfig { path, other_description }
if (path == "list.columns" || path == "list.full")
&& *other_description == "user config"
)) && warnings.len() == 2,
"expected list.columns/list.full → user config, got {warnings:?}"
);

// A key unknown in *both* configs stays "unknown field".
let warnings = collect_unknown_warnings::<ProjectConfig>("[list]\nnonsense-typo = true\n");
assert!(
matches!(
warnings.as_slice(),
[UnknownWarning::NestedUnknown { path }] if path == "list.nonsense-typo"
),
"expected list.nonsense-typo → unknown, got {warnings:?}"
);

// The reverse direction: `url` is project-only, so it redirects to
// project config when found in user config — no scope-to-repo note
// there (that only applies to user-config destinations).
let warnings = collect_unknown_warnings::<UserConfig>("[list]\nurl = \"x\"\n");
assert!(
matches!(
warnings.as_slice(),
[UnknownWarning::NestedWrongConfig { path, other_description }]
if path == "list.url" && *other_description == "project config"
),
"expected list.url → project config, got {warnings:?}"
);
}

#[test]
fn test_scope_to_repo_note_only_for_user_config() {
// The note fires for user-config destinations and nothing else.
assert!(scope_to_repo_note("user config").is_some());
assert!(scope_to_repo_note("project config").is_none());

// It reaches the rendered load-warning for a user-config redirect...
let note = "to scope it to this repo";
let msg = format_load_warning(
"Project config",
&crate::config::UnknownWarning::NestedWrongConfig {
path: "list.columns".to_string(),
other_description: "user config",
},
);
assert!(
msg.contains(note),
"user-config redirect should carry note: {msg}"
);

// ...but not a project-config redirect.
let msg = format_load_warning(
"User config",
&crate::config::UnknownWarning::NestedWrongConfig {
path: "list.url".to_string(),
other_description: "project config",
},
);
assert!(
!msg.contains(note),
"project-config redirect should not carry note: {msg}"
);
}

/// Every `DEPRECATION_RULES` row's migration fires on one config, pinning
/// cross-rule interactions the per-rule tests can't see — in particular
/// the inserted `[forge]` staying in the mid-file spot where the user
Expand Down
2 changes: 1 addition & 1 deletion src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ pub use deprecation::suppress_warnings;
pub use deprecation::warnings_suppressed;
pub use deprecation::{
DEPRECATED_SECTION_KEYS, DeprecatedSection, UnknownKeyKind, classify_unknown_key,
key_belongs_in, nested_key_belongs_in, warn_unknown_fields,
key_belongs_in, nested_key_belongs_in, scope_to_repo_note, warn_unknown_fields,
};
pub use deprecation::{DeprecationKind, Deprecations};
pub use expansion::{
Expand Down
72 changes: 69 additions & 3 deletions src/config/unknown_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,51 @@ pub enum UnknownWarning {
NestedUnknown { path: String },
}

/// Position within `C::Other`'s unknown tree while walking `C`'s nested
/// unknowns. A nested key that's schema-unknown in `C` but *valid* in
/// `C::Other` (e.g. `list.columns` — a user-config display setting — placed in
/// project config) should redirect there rather than read "unknown field". We
/// answer "is this leaf valid in the other config?" by walking the other
/// config's unknown tree in lockstep: a key valid there never appears in that
/// tree, so its absence is the signal.
#[derive(Clone, Copy)]
enum OtherStatus<'a> {
/// An ancestor section is absent or wholly unknown in `C::Other`, so
/// nothing at or below this point is valid there.
UnknownSection,
/// Within a section that exists in `C::Other`. Carries the corresponding
/// node in the other tree, or `None` once no further unknowns are recorded
/// — i.e. every key at or below here is valid in the other config.
Known(Option<&'a UnknownTree>),
}

impl<'a> OtherStatus<'a> {
/// Whether `key` at the current level is a valid key in `C::Other`.
fn key_is_valid_in_other(self, key: &str) -> bool {
match self {
OtherStatus::UnknownSection => false,
OtherStatus::Known(None) => true,
OtherStatus::Known(Some(node)) => !node.keys.contains(key),
}
}

/// Descend into `key`, returning the status for its children.
fn descend(self, key: &str) -> OtherStatus<'a> {
match self {
OtherStatus::UnknownSection => OtherStatus::UnknownSection,
OtherStatus::Known(None) => OtherStatus::Known(None),
OtherStatus::Known(Some(node)) => {
if node.keys.contains(key) {
// Whole subtree is unknown in the other config too.
OtherStatus::UnknownSection
} else {
OtherStatus::Known(node.nested.get(key))
}
}
}
}
}

/// Collect structured warnings for `raw_contents` under config type `C`.
///
/// Top-level classification reads the *raw* tree (so deprecated top-level
Expand All @@ -185,6 +230,14 @@ pub enum UnknownWarning {
/// so patterns the deprecation system already warns about (e.g.,
/// `switch.no-cd`, `merge.no-ff`) don't double-warn here.
///
/// A misplaced *nested* key is redirected to `C::Other` when it's valid there
/// — determined by walking `C::Other`'s own unknown tree for the same content
/// (see the private `OtherStatus` helper). If that other-config analysis is
/// unreliable, the walk falls back to treating nested keys as unknown, so only
/// the hard-coded
/// [`nested_key_belongs_in`](crate::config::nested_key_belongs_in) redirects
/// still fire.
///
/// Returns an empty vec if either analysis is unreliable — the load path
/// surfaces parse/type errors elsewhere.
pub fn collect_unknown_warnings<C: WorktrunkConfig>(raw_contents: &str) -> Vec<UnknownWarning> {
Expand All @@ -197,6 +250,13 @@ pub fn collect_unknown_warnings<C: WorktrunkConfig>(raw_contents: &str) -> Vec<U
UnknownAnalysis::Parsed(t) => t,
UnknownAnalysis::Unreliable(_) => return Vec::new(),
};
// The same content viewed as the *other* config type: a nested key absent
// from this tree is valid there. Unreliable → no generalized redirect.
let other_analysis = compute_unknown_tree::<C::Other>(&migrated);
let other_root = match other_analysis.warn_tree() {
Some(t) => OtherStatus::Known(Some(t)),
None => OtherStatus::UnknownSection,
};

let mut out = Vec::new();
for key in &raw_tree.keys {
Expand Down Expand Up @@ -225,19 +285,25 @@ pub fn collect_unknown_warnings<C: WorktrunkConfig>(raw_contents: &str) -> Vec<U
if !C::is_valid_key(key) {
continue; // top-level unknowns were classified above against raw
}
walk_nested::<C>(sub, key, &mut out);
walk_nested::<C>(sub, key, other_root.descend(key), &mut out);
}
out
}

fn walk_nested<C: WorktrunkConfig>(
tree: &UnknownTree,
prefix: &str,
other: OtherStatus,
out: &mut Vec<UnknownWarning>,
) {
for key in &tree.keys {
let path = format!("{prefix}.{key}");
out.push(match crate::config::nested_key_belongs_in::<C>(&path) {
// Hard-coded redirects (commit.generation leaves) take precedence and
// fire even when the other-config analysis is unreliable; otherwise
// fall back to the general "valid in the other config" check.
let belongs = crate::config::nested_key_belongs_in::<C>(&path)
.or_else(|| other.key_is_valid_in_other(key).then(C::Other::description));
out.push(match belongs {
Some(other_description) => UnknownWarning::NestedWrongConfig {
path,
other_description,
Expand All @@ -250,7 +316,7 @@ fn walk_nested<C: WorktrunkConfig>(
continue;
}
let path = format!("{prefix}.{key}");
walk_nested::<C>(sub, &path, out);
walk_nested::<C>(sub, &path, other.descend(key), out);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ exit_code: 0
  \ No newline at end of file
  +[commit.generation]
  +command = "claude"
▲ Key commit.generation.command belongs in user config (will be ignored)
▲ Key commit.generation.command belongs in user config (will be ignored); to scope it to this repo, add it under [projects."<id>"] in user config

SHELL INTEGRATION
▲ Shell integration not configured
Expand Down
Loading