fix(param_reorder): Correct update old_filenames fields

Author: amilcarlucas

param_reorder.py

@@ -17,7 +17,6 @@
 import json
 import logging
 import os
-import re
 import shutil
 import subprocess
 from typing import Optional
@@ -113,7 +112,7 @@ def reorder_param_files(steps: dict) -> dict[str, str]:
     return renames
 
 
-def loop_relevant_files(renames: dict[str, str], steps: dict) -> list[str]:
+def loop_relevant_files(renames: dict[str, str]) -> list[str]:
     param_dirs = ["."]
     # Search all *.py, *.json and *.md files in the current directory
     # and replace all occurrences of the old names with the new names
@@ -125,76 +124,109 @@ def loop_relevant_files(renames: dict[str, str], steps: dict) -> list[str]:
                 continue
             if file == "vehicle_components.json":
                 continue
-            if file == SEQUENCE_FILENAME:
-                uplate_old_filenames(renames, steps)
             if file in PYTHON_FILES or file.endswith((".md", ".json")):
-                update_file_contents(renames, root, file, steps)
+                update_file_contents(renames, root, file)
     return param_dirs
 
 
-def uplate_old_filenames(renames: dict[str, str], steps: dict) -> None:
-    for new_name, old_name in renames.items():
-        if old_name != new_name:
-            if "old_filenames" in steps[old_name]:
-                if old_name not in steps[old_name]["old_filenames"]:
-                    steps[old_name]["old_filenames"].append(old_name)
-            else:
-                steps[old_name]["old_filenames"] = [old_name]
-
-
-def update_file_contents(renames: dict[str, str], root: str, file: str, steps: dict) -> None:
+def update_file_contents(renames: dict[str, str], root: str, file: str) -> None:
     with open(os.path.join(root, file), encoding="utf-8", newline="") as handle:
         file_content = handle.read()
-    if file.startswith("TUNING_GUIDE_") and file.endswith(".md"):
-        for old_filename in renames.values():
-            if old_filename not in file_content:
-                msg = f"The intermediate parameter file '{old_filename}' is not mentioned in the {file} file"
-                logging.error(msg)
-    for new_name, old_name in renames.items():
-        if "configuration_steps" in file and file.endswith(".json"):
-            file_content = update_configuration_steps_json_file_contents(steps, file_content, new_name, old_name)
-        else:
+    if "configuration_steps" in file and file.endswith(".json"):
+        # Use renames (current JSON key -> new key) so that historical names already
+        # stored in old_filenames are never accidentally overwritten.
+        for new_name, old_name in renames.items():
+            file_content = file_content.replace(old_name, new_name)
+    else:
+        if file.startswith("TUNING_GUIDE_") and file.endswith(".md"):
+            for old_filename in file_renames:
+                if old_filename not in file_content:
+                    msg = f"The intermediate parameter file '{old_filename}' is not mentioned in the {file} file"
+                    logging.error(msg)
+        for old_name, new_name in file_renames.items():
             file_content = file_content.replace(old_name, new_name)
     with open(os.path.join(root, file), "w", encoding="utf-8", newline="") as handle:
         handle.write(file_content)
 
 
-def update_configuration_steps_json_file_contents(steps: dict, file_content: str, new_name: str, old_name: str) -> str:
-    new_file_content = ""
-    curr_filename = ""
-    in_steps_block = False
+def _parse_old_filenames_from_line(line: str) -> list[str]:
+    """Extract filename strings from an old_filenames JSON line."""
+    bracket_start = line.index("[")
+    bracket_end = line.rindex("]")
+    text = line[bracket_start + 1 : bracket_end].strip()
+    return [v.strip().strip('"') for v in text.split(",")] if text else []
 
-    for line in file_content.splitlines(keepends=True):
-        # Track when we enter/exit the "steps" block
-        if '"steps": {' in line:
-            in_steps_block = True
-            new_file_content += line
-            continue
 
-        # Only process filenames when inside steps block
-        if in_steps_block:
-            re_search = re.search(r"^        \"(\w.+)\"", line)
-            if re_search:
-                curr_filename = re_search.group(1)
-
-        if "old_filenames" in line:
-            if curr_filename in steps and "old_filenames" in steps[curr_filename]:
-                # WARNING!!! old_filenames can only used once, so we remove it after using it
-                old_filenames = str(steps[curr_filename].pop("old_filenames")).replace("'", '"')
-                new_file_content += f'            "old_filenames": {old_filenames}'
-                if line.endswith(",\n"):
-                    new_file_content += ","
-                new_file_content += "\n"
-            else:
-                new_file_content += line
-        else:
-            new_file_content += line.replace(old_name, new_name)
+def _format_old_filenames_line(indent: str, values: list[str], trailing_comma: bool) -> str:
+    """Format an old_filenames JSON line."""
+    values_str = ", ".join(f'"{v}"' for v in values)
+    return f'{indent}"old_filenames": [{values_str}]{"," if trailing_comma else ""}\n'
 
-        # Track end of steps block
-        if in_steps_block and line.strip() == "}," and curr_filename:
-            in_steps_block = False
 
-    return new_file_content
+def update_old_filenames_in_json_file(json_path: str) -> None:
+    """
+    Update old_filenames fields in the JSON file using targeted text replacement.
+
+    Must be called after loop_relevant_files so the JSON already has the new step keys.
+    Reads the file as text to avoid reformatting. Uses brace counting to correctly
+    identify step block boundaries and deduplicates multiple old_filenames entries.
+    """
+    with open(json_path, encoding="utf-8", newline="") as f:
+        lines = f.readlines()
+
+    needs_update: dict[str, list[str]] = {}
+    for old_name, new_name in file_renames.items():
+        if old_name != new_name:
+            needs_update.setdefault(new_name, []).append(old_name)
+
+    new_lines: list[str] = []
+    i = 0
+    while i < len(lines):
+        line = lines[i]
+        # Step keys have exactly 8 spaces of indentation inside "steps": { ... }
+        if not (line.startswith('        "') and line.rstrip().endswith('": {')):
+            new_lines.append(line)
+            i += 1
+            continue
+        stripped = line.strip()
+        step_key = stripped[1 : stripped.index('"', 1)]
+        if step_key not in needs_update:
+            new_lines.append(line)
+            i += 1
+            continue
+        # Collect all lines for this step using brace counting
+        step_lines: list[str] = [line]
+        i += 1
+        depth = 1
+        while i < len(lines) and depth > 0:
+            depth += lines[i].count("{") - lines[i].count("}")
+            step_lines.append(lines[i])
+            i += 1
+        # Merge all old_filenames entries (existing + new names) into one
+        of_indices = [k for k, sl in enumerate(step_lines) if sl.strip().startswith('"old_filenames"')]
+        merged: list[str] = []
+        for k in of_indices:
+            for v in _parse_old_filenames_from_line(step_lines[k]):
+                if v not in merged:
+                    merged.append(v)
+        for name in needs_update[step_key]:
+            if name not in merged:
+                merged.append(name)
+        if of_indices:
+            first = of_indices[0]
+            first_line = step_lines[first]
+            indent = first_line[: len(first_line) - len(first_line.lstrip())]
+            step_lines[first] = _format_old_filenames_line(indent, merged, first_line.rstrip().endswith(","))
+            for k in reversed(of_indices[1:]):
+                del step_lines[k]
+        else:
+            ref = step_lines[1] if len(step_lines) > 1 else ""
+            indent = ref[: len(ref) - len(ref.lstrip())] if ref.strip() else "            "
+            step_lines.insert(1, _format_old_filenames_line(indent, merged, trailing_comma=True))
+        new_lines.extend(step_lines)
+
+    with open(json_path, "w", encoding="utf-8", newline="") as f:
+        f.writelines(new_lines)
 
 
 def _git_executable() -> Optional[str]:
@@ -238,18 +270,20 @@ def rename_file(old_name: str, new_name: str, param_dir: str) -> None:
     if git_executable and _is_git_tracked(old_name_path):
         try:
             subprocess.run(  # noqa: S603
-                [git_executable, "mv", "--", old_name_path, new_name_path],
+                [git_executable, "mv", "-f", "--", old_name_path, new_name_path],
                 check=True,
                 capture_output=True,
                 text=True,
             )
             return
         except subprocess.CalledProcessError as exc:
+            stderr = exc.stderr.strip() if exc.stderr else ""
             logging.warning(
-                "git mv failed for %s -> %s, falling back to os.rename: %s",
+                "git mv failed for %s -> %s, falling back to os.rename: %s%s",
                 old_name_path,
                 new_name_path,
                 exc,
+                f"\n  stderr: {stderr}" if stderr else "",
             )
     os.rename(old_name_path, new_name_path)
 
@@ -285,12 +319,14 @@ def change_line_endings_for_md_files() -> None:
 
 def main() -> None:
     logging.basicConfig(level="INFO", format="%(asctime)s - %(levelname)s - %(message)s")
-    with open(os.path.join("ardupilot_methodic_configurator", SEQUENCE_FILENAME), encoding="utf-8") as f:
+    json_path = os.path.join("ardupilot_methodic_configurator", SEQUENCE_FILENAME)
+    with open(json_path, encoding="utf-8") as f:
         json_content = json.load(f)
     steps = json_content["steps"]
     renames = reorder_param_files(steps)
-    param_dirs = loop_relevant_files(renames, steps)
+    param_dirs = loop_relevant_files(renames)
     reorder_actual_files(renames, param_dirs)
+    update_old_filenames_in_json_file(json_path)
     # change_line_endings_for_md_files()