exit code was always 0 even on error - #67
Merged
Merged
Conversation
nh13
added a commit
to nh13/minibwa
that referenced
this pull request
Aug 8, 2026
The one-line CLI fix -- main() returning each subcommand's status instead of 0 -- rode inside feat/issue-14-meth-sam-tags, so dist shipped a behaviour change that no manifest entry described. The byte-identity gate could not catch it either, because exit status is not SAM output, and the same change also existed a second time as a standalone branch listed nowhere. Extract it: the commit is dropped from the meth branch (which now touches main.c not at all) and fix/exit-status becomes a [[feature]] in its own right, first in the order since one line in main.c is the most disjoint change here. It is also now upstream as lh3#67, so the reconciler can retire it if that merges. Verified: all 15 features assemble with zero drops and no rerere retraining, the gate passes with 15 features covered, and the assembled binary still exits 1 on a bad option and 0 on success.
nh13
added a commit
to nh13/minibwa
that referenced
this pull request
Aug 22, 2026
… --alt Two gaps in the preceding two commits, both found by self-review. Moving .alt resolution out of mb_idx_load() and into the caller updated one caller and missed the other: main_mem() loads the index at map-main.c:709 and never resolved .alt, so the bwa-compat `mem` subcommand silently stopped being ALT-aware. On the ALT fixture it went from MAPQ 60 with two supplementaries to a single MAPQ 0 record. The suite did not catch it because 6ca77f2 moved the ALT tests off `mem` and onto `map` on purpose, leaving that entry point with no coverage at all. Of the seven mb_idx_load*() call sites, this was the only remaining behaviour-bearing one: fastmap is SMEM-only and ex-one/ex-batch read no ALT fields. Separately, mb_idx_set_alt() returned void and dropped l2b_set_alt()'s -1, so a mistyped `--alt /path/typo.alt` loaded nothing and aligned on as if ALT-awareness were off -- no diagnostic even at -v 3, and output indistinguishable from a correct non-ALT run. It now returns the record count like l2b_set_alt(), and an explicitly named file that will not load is an error. The auto-detect branch still ignores its status: an absent adjacent .alt is the normal case, not a mistake, and that asymmetry is the point. test-noalt.sh grows a `mem`-vs-`map` parity check (both with and without the .alt, so it cannot pass for an unrelated reason) and a check that an unreadable --alt reports the failure and emits no alignments. Both were confirmed to fail when their fix is reverted. The bad-path check asserts on stderr rather than exit status because this branch predates r422 (lh3#67), where main() still discarded every subcommand's return value. Also corrects l2b_set_alt()'s header doc, which said it returns the number of ALT contigs; l2bit.c counts records there and keeps the contig count separately in n_alt_ctg.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
main()collects each subcommand's return value intoretand already uses it — the verbose stats block a few lines above is guarded byret == 0— but then returns 0 unconditionally, so every failure exits successfully.This matters for scripts and pipelines: a mistyped option prints
[ERROR] unknown option in "...", maps nothing, and still looks like success toset -eor a workflow engine.Only the propagation is fixed here. Detection is unchanged, and two nearby gaps are left alone:
memdoesn't diagnose unknown long options at all (it passes no long-option table toketopt), and a missing index still aborts with SIGABRT rather than exiting.