Skip to content

fix(merge): repeated merges, help grouping and release test build - #196

Merged
mmolari merged 5 commits into
feat/merge-docsfrom
feat/merge-fixes
Aug 14, 2026
Merged

fix(merge): repeated merges, help grouping and release test build#196
mmolari merged 5 commits into
feat/merge-docsfrom
feat/merge-fixes

Conversation

@mmolari

@mmolari mmolari commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Follow-up fixes from reviewing #192 and #193, stacked on top of #195 so the reviewed branches stay untouched.

  • repeated merges collided on a constant relabeling salt. make_disjoint_from re-derived the right graph's ids as id((1, old_id)). Those ids survive into the merged graph whenever a block or node finds no homologue, so a second merge with that graph on the left mapped the new graph's raw ids straight back onto them, and the disjointness guard fired with a message blaming a 64-bit hash collision. The salt now comes from the path id offset, which strictly increases along a merge chain, and the bounded retry loop is back as the actual guarantee — simplify drops paths without renumbering, so the offset is not monotone under every command. Covered by a unit test and an integration test that appends to an already-merged graph.
  • integration tests called sanity_check, which is #[cfg(any(test, debug_assertions))] and therefore missing from the library when integration tests are built in release: cargo clippy --release --all-targets failed at those call sites. The calls were redundant anyway — merge_graphs and merge_run already sanity-check the same graph — so they are gone rather than cfg-guarded.
  • --circular, --verify, --no-progress-bar and --guide-tree were listed under the "Alignment" help section, because next_help_heading applies to every argument declared after the flattened GraphMergeParams. That struct is now declared last in both commands. Pre-existing for build; merge had inherited it. CLI reference regenerated.
  • merge --verify materialised every genome of both inputs before comparing. It now reconstructs and drops one genome at a time, matching what reconstruct --verify already does. Measured on a 3+1 E. coli merge the peak RSS is unchanged (~1.05 GB either way — the merge itself dominates), so this is consistency rather than a measurable win.

--all-targets in release still fails at #192's own commit; fixing that there would have meant force-pushing three branches.

@mmolari
mmolari deployed to refs/pull/196/merge August 14, 2026 16:07 — with GitHub Actions Active
@mmolari
mmolari deployed to refs/pull/196/merge August 14, 2026 16:19 — with GitHub Actions Active
@mmolari
mmolari merged commit cc30583 into feat/merge Aug 14, 2026
10 checks passed
@mmolari
mmolari deleted the feat/merge-fixes branch August 14, 2026 16:20
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