diff options
| -rw-r--r-- | lib/naming/config.ml | 56 | ||||
| -rw-r--r-- | lib/naming/config.mli | 7 | ||||
| -rw-r--r-- | test/test_config.ml | 46 |
3 files changed, 88 insertions, 21 deletions
diff --git a/lib/naming/config.ml b/lib/naming/config.ml index 0fafd1f..7d87d62 100644 --- a/lib/naming/config.ml +++ b/lib/naming/config.ml @@ -33,27 +33,45 @@ let of_string text = (fun (s : OI.section) -> if s.OI.name = "defaults" then None else Some s.OI.name) sections in + (* Merge EVERY section named [defaults], not just the first: [lang.ml]'s + [of_string] was fixed this morning to fold over all matching + sections rather than take [List.find_opt]'s first match, because a + hand-edited file WILL grow duplicate headers as a user appends to it + over time. Both modules parse the same reader + ([Overlay_ini.parse_sections]) over the same file format, so they + must not disagree about what a duplicate [section] header means -- + taking only the first [defaults] block here silently discarded every + later one, with nothing pointing back at the parser. Folding a + single accumulator across every matching section, in file order, + keeps this consistent with the within-section behaviour below (last + [k]-match wins): a scalar key repeated across two blocks resolves to + the LATER value, and [overlay] keeps accumulating across every + block, not only its first. *) + let defaults_sections = + List.filter (fun (s : OI.section) -> s.OI.name = "defaults") sections + in let acc = - match List.find_opt (fun (s : OI.section) -> s.OI.name = "defaults") sections with - | None -> empty - | Some s -> - (* Cons then reverse once at the end, not `@ [v]` per line: the - latter is O(n^2) over the field count, a real hang on a - machine-generated file with many overlay lines. *) - let acc = - List.fold_left - (fun acc (k, v) -> - match k with - | "lang" -> { acc with lang = Some v } - | "template" -> { acc with template = Some v } - | "format" -> { acc with format = Some v } - (* accumulates: a user has more than one overlay *) - | "overlay" -> { acc with overlays = v :: acc.overlays } - | other -> { acc with unknown_keys = other :: acc.unknown_keys }) - empty s.OI.fields - in - { acc with overlays = List.rev acc.overlays; unknown_keys = List.rev acc.unknown_keys } + (* Cons then reverse once at the very end, not `@ [v]` per line: the + latter is O(n^2) over the field count, a real hang on a + machine-generated file with many overlay lines. Reversing only + after every section has been folded (not once per section) is + what keeps [overlay] and [unknown_keys] in file order across + block boundaries, not merely within one block. *) + List.fold_left + (fun acc (s : OI.section) -> + List.fold_left + (fun acc (k, v) -> + match k with + | "lang" -> { acc with lang = Some v } + | "template" -> { acc with template = Some v } + | "format" -> { acc with format = Some v } + (* accumulates: a user has more than one overlay *) + | "overlay" -> { acc with overlays = v :: acc.overlays } + | other -> { acc with unknown_keys = other :: acc.unknown_keys }) + acc s.OI.fields) + empty defaults_sections in + let acc = { acc with overlays = List.rev acc.overlays; unknown_keys = List.rev acc.unknown_keys } in Ok { acc with unknown_sections } let resolve ~flag ~config ~default = diff --git a/lib/naming/config.mli b/lib/naming/config.mli index 27f0c44..a84f95b 100644 --- a/lib/naming/config.mli +++ b/lib/naming/config.mli @@ -17,7 +17,12 @@ val of_string : string -> (t, string) result repeated key is LAST-WINS -- the opposite direction from {!Colitur_kernel.Overlay_ini.get}'s first-wins over the same [section] type -- because the natural reading of a config file a user edited by - hand and appended to is "the bottom line is the one that took effect". *) + hand and appended to is "the bottom line is the one that took effect". + This holds whether the repeat is within one [\[defaults\]] block or + across two of them: every section named [defaults] is merged, not only + the first, the same duplicate-section policy {!Lang.of_string} documents + for its own sections -- the two modules read the same underlying format + and must not disagree about what a repeated header means. *) val lang : t -> string option val overlays : t -> string list diff --git a/test/test_config.ml b/test/test_config.ml index 067b255..83da015 100644 --- a/test/test_config.ml +++ b/test/test_config.ml @@ -66,6 +66,42 @@ let test_misspelled_section_is_reported_not_silently_dropped () = Alcotest.(check (option string)) "lang not read from the wrong section" None (C.lang c); Alcotest.(check (list string)) "section reported" [ "deafults" ] (C.unknown_sections c) +(* THE regression test for the cross-module inconsistency: [config.ml] used + to locate [\[defaults\]] with [List.find_opt], taking only the FIRST + matching section and silently discarding every later one, while + [lang.ml]'s [of_string] folds over ALL matching sections. A scalar set in + the first block and a DIFFERENT scalar set only in the second block must + both resolve -- before the fix, [template] (second-block-only) came back + [None]. *) +let test_two_defaults_blocks_both_contribute () = + let text = "[defaults]\nlang = pl\n\n[defaults]\ntemplate = ~/my-ordo.tex\n" in + let c = ok (C.of_string text) in + Alcotest.(check (option string)) "lang from first block" (Some "pl") (C.lang c); + Alcotest.(check (option string)) "template from second block" (Some "~/my-ordo.tex") (C.template c) + +(* Consistent with the existing within-section last-wins rule (see + [config.mli]'s [lang] comment): a key repeated ACROSS two [\[defaults\]] + blocks resolves to the value from the LATER block, exactly as it would if + both lines sat in one block. *) +let test_key_repeated_across_blocks_last_wins () = + let text = "[defaults]\nlang = pl\n\n[defaults]\nlang = en\n" in + let c = ok (C.of_string text) in + Alcotest.(check (option string)) "later block's lang wins" (Some "en") (C.lang c) + +(* [overlay] must keep accumulating across block boundaries, in file order, + not merely within one block. *) +let test_overlays_accumulate_across_blocks () = + let text = "[defaults]\noverlay = ~/a.ini\n\n[defaults]\noverlay = ~/b.ini\n" in + let c = ok (C.of_string text) in + Alcotest.(check (list string)) "both overlays, in file order" [ "~/a.ini"; "~/b.ini" ] (C.overlays c) + +(* Merging a second [\[defaults\]] block is the whole point -- it must not + start being reported as an unknown section. *) +let test_two_defaults_blocks_report_no_unknown_sections () = + let text = "[defaults]\nlang = pl\n\n[defaults]\ntemplate = ~/my-ordo.tex\n" in + let c = ok (C.of_string text) in + Alcotest.(check (list string)) "no unknown sections" [] (C.unknown_sections c) + let suite = ( "Config", [ Alcotest.test_case "reads defaults" `Quick test_reads_defaults; @@ -75,4 +111,12 @@ let suite = Alcotest.test_case "malformed is error" `Quick test_malformed_is_error_not_crash; Alcotest.test_case "unknown key not fatal" `Quick test_unknown_key_is_reported_not_fatal; Alcotest.test_case "misspelled section reported" - `Quick test_misspelled_section_is_reported_not_silently_dropped ] ) + `Quick test_misspelled_section_is_reported_not_silently_dropped; + Alcotest.test_case "two defaults blocks both contribute" + `Quick test_two_defaults_blocks_both_contribute; + Alcotest.test_case "key repeated across blocks last wins" + `Quick test_key_repeated_across_blocks_last_wins; + Alcotest.test_case "overlays accumulate across blocks" + `Quick test_overlays_accumulate_across_blocks; + Alcotest.test_case "two defaults blocks report no unknown sections" + `Quick test_two_defaults_blocks_report_no_unknown_sections ] ) |