From 39a7ce21a02d8f8a6a9d438e0a2260fe9d64456d Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 30 Sep 2026 08:36:51 -0700 Subject: [PATCH 1/2] fix(xcstrings): keep conflict resolutions valid JSON Attach separators to the non-empty sides of per-key conflicts, including the preceding separator when a deleted key was last in its object. Pin the ours conflict skeleton and resolution behavior in the merge-driver tests. Co-Authored-By: Claude Opus 5 --- scripts/merge-xcstrings.py | 27 ++++++++++++- tests/test_merge_xcstrings.py | 75 +++++++++++++++++++++++++++++++---- 2 files changed, 92 insertions(+), 10 deletions(-) diff --git a/scripts/merge-xcstrings.py b/scripts/merge-xcstrings.py index 97e73cae2223..046650cfb699 100755 --- a/scripts/merge-xcstrings.py +++ b/scripts/merge-xcstrings.py @@ -263,15 +263,38 @@ def materialize_catalog_conflicts( theirs_block = sources["theirs"].blocks.get(key, "") start, _, end = target.spans[key] line_start = _line_start(merged_text, start) + # Deliberately inspect only the text before the key; trailing text on + # its line remains lossless and resolves to valid JSON on either side. if merged_text[line_start:start].strip(): raise ValueError(f"conflict key {name!r} shares a line with other text") suffix = merged_text[end:] comma = "," if suffix.startswith(",") else "" + replacement_start = line_start + replacement_prefix = "" + leading_comma = "" + if not comma: + member_index = next(index for index, member in enumerate(target.members) if member[0] == key) + if member_index: + previous_end = target.members[member_index - 1][3] + separator = merged_text[previous_end:line_start] + comma_index = separator.find(",") + if comma_index < 0 or separator[:comma_index].strip(): + raise ValueError(f"cannot consume separator before conflict key {name!r}") + replacement_start = previous_end + replacement_prefix = separator[:comma_index] + separator[comma_index + 1 :] + leading_comma = "," + with_comma = lambda block: (leading_comma + block + comma) if block else block replacements.append( ( - line_start, + replacement_start, end + len(comma), - conflict_text(base_block, ours_block, theirs_block, width) + comma, + replacement_prefix + + conflict_text( + with_comma(base_block), + with_comma(ours_block), + with_comma(theirs_block), + width, + ), ) ) ranges = sorted(replacements) diff --git a/tests/test_merge_xcstrings.py b/tests/test_merge_xcstrings.py index 8571812ed3e6..616c3950d48b 100644 --- a/tests/test_merge_xcstrings.py +++ b/tests/test_merge_xcstrings.py @@ -88,6 +88,29 @@ def conflict_regions(text): return ["\n".join(lines[start : end + 1]) for start, end in zip(starts, ends)] +def resolve_conflict(text, side): + resolved = [] + active_side = None + for line in text.splitlines(keepends=True): + marker = line.rstrip("\r\n") + if marker.startswith("<<<<<<<"): + assert active_side is None + active_side = "ours" + elif marker.startswith("|||||||"): + assert active_side == "ours" + active_side = "base" + elif marker == "=======": + assert active_side == "base" + active_side = "theirs" + elif marker.startswith(">>>>>>>"): + assert active_side == "theirs" + active_side = None + elif active_side is None or active_side == side: + resolved.append(line) + assert active_side is None + return "".join(resolved) + + def run_with_patched_merge(result): driver = load_driver() with tempfile.TemporaryDirectory() as directory: @@ -184,7 +207,7 @@ def test_multiple_conflict_hunks_keep_untouched_keys_outside_conflicts(): assert merged.count('"value": "untouched"') == 1 -def test_each_reported_key_is_inside_a_conflict_region(): +def test_each_conflict_region_stays_within_its_reported_key_span(): base = catalog( { "shared": localized_unit( @@ -245,9 +268,23 @@ def test_each_reported_key_is_inside_a_conflict_region(): assert code == 1, stderr report = stderr.split("materializing a conflict: ", 1)[1].strip() regions = conflict_regions(merged) - for name in report.split(", "): - key = name.split(".", 1)[1] - assert any(f'"{key}"' in region for region in regions), (name, merged) + catalog_keys = set(base["strings"]) | set(ours["strings"]) | set(theirs["strings"]) + assert len(regions) == len(report.split(", ")) + for region in regions: + for key in catalog_keys: + if key != "shared": + assert f'"{key}"' not in region, (key, region) + + +def test_conflicting_key_skeleton_uses_ours_text(): + driver = load_driver() + base = render(catalog({"a": {"v": "base"}})) + ours = json.dumps(catalog({"a": {"v": "ours"}}), separators=(",", ":")) + theirs = render(catalog({"a": {"v": "theirs"}})) + merged, conflicts, _ = driver.merge_catalog_text(base, ours, theirs) + assert conflicts == ["strings.a"] + assert '"a":{"v":"ours"}' in merged + assert '"a": {\n' not in merged def test_key_conflicts_preserve_clean_key_merges_and_formatting(): @@ -310,6 +347,28 @@ def test_delete_versus_modify_conflicts(): assert '"a"' in merged +def test_delete_versus_modify_resolves_to_each_side_as_valid_json(): + base = catalog({"a": unit("A"), "b": unit("B")}) + ours = catalog({"a": unit("changed"), "b": unit("B")}) + theirs = catalog({"b": unit("B")}) + code, merged, stderr = run(base, ours, theirs) + assert code == 1, stderr + for side in ("ours", "theirs"): + resolved = json.loads(resolve_conflict(merged, side)) + assert set(resolved["strings"]) == ({"a", "b"} if side == "ours" else {"b"}) + + +def test_delete_versus_modify_last_key_resolves_to_each_side_as_valid_json(): + base = catalog({"a": unit("A"), "b": unit("B")}) + ours = catalog({"a": unit("A"), "b": unit("changed")}) + theirs = catalog({"a": unit("A")}) + code, merged, stderr = run(base, ours, theirs) + assert code == 1, stderr + for side in ("ours", "theirs"): + resolved = json.loads(resolve_conflict(merged, side)) + assert set(resolved["strings"]) == ({"a", "b"} if side == "ours" else {"a"}) + + def test_non_canonical_input_merges_without_reformatting(): """Branches routinely carry a different catalog style from main. The driver must merge them anyway, and must not rewrite either side's formatting.""" @@ -581,7 +640,7 @@ def test_a_conflict_key_sharing_a_line_keeps_every_side_intact(): def test_two_conflict_keys_on_one_line_stay_balanced_and_lossless(): - """Overlapping per-key ranges must not clobber one another's text.""" + """Two conflicts sharing one line fall back without losing either key.""" base = ( '{\n "sourceLanguage" : "en",\n "strings" : {\n' ' "a": { "v": "B1" }, "b": { "v": "B2" }\n' @@ -592,10 +651,10 @@ def test_two_conflict_keys_on_one_line_stay_balanced_and_lossless(): code, merged, _ = run(base, ours, theirs) assert code == 1 - # conflict_regions() asserts the markers balance; an overlapping - # replacement produced a stray '||||||| base' with no opening marker. + # The shares-a-line refusal keeps this compacted input in one lossless + # whole-file conflict instead of attempting per-key replacements. assert len(conflict_regions(merged)) == 1, merged - # Ours' value for the second key was truncated to ',: "O2" }'. + # A per-key replacement would have truncated the second key to ',: "O2" }'. assert '"b": { "v": "O2" }' in merged, merged assert_conflict_preserves(merged, "O1", "O2", "T1", "T2", "B1", "B2") From 06ab9f5ab8ed41c199d3e74b902141abebe94d67 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 30 Sep 2026 12:12:09 -0700 Subject: [PATCH 2/2] test(xcstrings): retain conflict containment assertions Keep both the reported-key coverage and unrelated-key exclusion assertions in the renamed conflict-region test. Co-Authored-By: Claude Opus 5 --- tests/test_merge_xcstrings.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_merge_xcstrings.py b/tests/test_merge_xcstrings.py index 616c3950d48b..531f6cd08ed2 100644 --- a/tests/test_merge_xcstrings.py +++ b/tests/test_merge_xcstrings.py @@ -268,6 +268,9 @@ def test_each_conflict_region_stays_within_its_reported_key_span(): assert code == 1, stderr report = stderr.split("materializing a conflict: ", 1)[1].strip() regions = conflict_regions(merged) + for name in report.split(", "): + key = name.split(".", 1)[1] + assert any(f'"{key}"' in region for region in regions), (name, merged) catalog_keys = set(base["strings"]) | set(ours["strings"]) | set(theirs["strings"]) assert len(regions) == len(report.split(", ")) for region in regions: