Skip to content

fix(run): --files takes several paths, as upstream does - #50

Merged
blairham merged 1 commit into
mainfrom
fix-files-nargs
Sep 8, 2026
Merged

blairham merged 1 commit into
mainfrom
fix-files-nargs

Conversation

@blairham

@blairham blairham commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Found while doing the launch-readiness pass for #43.

The bug

$ pre-commit run trailing-whitespace --files a.txt b.txt
Error: expected at most 1 argument, got 2

Nothing runs, and no file is touched. Upstream, measured on the same repo with the same config:

$ pre-commit run trailing-whitespace --files a.txt b.txt
trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1

Fixing a.txt
Fixing b.txt
upstream 4.6.2 this tool, before
exit code 1 1
a.txt fixed yes no
b.txt fixed yes no

Why

Upstream declares --files with argparse's nargs='*', so one flag swallows every following path. go-flags has no equivalent — a []string field means repeat the flag, binding exactly one value per occurrence. The second path fell through as a stray positional, hit the at-most-one-hook-id check, and the run died before any hook ran.

This is not an exotic invocation. --files with several paths is the ordinary way to scope a run — diff-scoped CI, xargs, editor integrations — and it was broken in both commands that accept the flag, run and try-repo.

The fix

One helper, called by both commands rather than copied into try_repo.go, rewriting the greedy form into the repeated form before go-flags sees it. Each semantic was confirmed against a real 4.6.2 argparse parser rather than assumed:

invocation files positional
--files a b c [a b c] —
--files a b hook [a b hook] — (greedy really does swallow it)
--files=a b [a] b
--files a --other [a] —
--files a -- b [a] b
--files [] —

Help text said --files=FILE, which described the broken behavior accurately. It now reads --files [FILES ...], like upstream's.

Why 78/78 did not catch it

Nothing in the differential suite ran a multi-path invocation. There is now a check that runs both binaries with two paths and compares exit code, output, and the files on disk.

Comparing exit codes alone would not have caught it either — upstream exits 1 because the hook failed, this exited 1 because it could not parse, and "both non-zero" matches. The discriminating assertion is the filesystem one: both files fixed, or the bug is back.

Verified by mutation — reverting the one-line wiring in run.go turns three of the four new checks red:

[PASS] [exit code ] --files with several paths exits alike (py=1 go=1)   <- non-discriminating
[FAIL] [output    ] --files with several paths does not fail to parse
[FAIL] [filesystem] --files fixed a.txt
[FAIL] [filesystem] --files fixed b.txt

Verification

make check and make lint clean; parity suite green against upstream 4.6.2 with PARITY_REQUIRE=1.

Separately: that mutation run also showed the parity suite exits 0 with failing parity checks — nothing calls t.Error on a mismatch, so the job that ci.yml describes as failing "before review rather than after a merge to main" cannot currently fail. That is a distinct problem and gets its own PR.

Refs #43

`pre-commit run --files a.txt b.txt` failed with "expected at most 1
argument, got 2" and touched nothing. Upstream fixes both files.

Upstream declares --files with argparse's nargs='*', so one flag
swallows every following path. go-flags has no equivalent: a []string
field means "repeat the flag" and binds exactly one value per
occurrence. So the second path fell through as a stray positional, hit
the at-most-one-hook-id check, and the run died before any hook ran.

This is not an exotic invocation. `--files` with several paths is the
ordinary way to scope a run -- diff-scoped CI, xargs, editor
integrations -- and it was broken in the only two commands that accept
the flag, `run` and `try-repo`.

The fix rewrites the greedy form into the repeated form before go-flags
sees it, in one helper both commands call rather than a second copy in
try_repo.go. The semantics are argparse's, each case confirmed against
4.6.2 rather than assumed:

  --files a b c      -> [a b c]      greedy to the end
  --files a b hook   -> [a b hook]   greedy really does swallow the hook id
  --files=a b        -> [a], pos b   the joined form takes exactly one
  --files a --other  -> [a]          stops at the next option
  --files a -- b     -> [a], pos b   stops at the terminator
  --files            -> []           empty, same as omitting it

The help text said `--files=FILE`, which described the broken behavior
accurately; it now reads `--files [FILES ...]` like upstream's.

Why 78/78 did not catch this: nothing in the differential suite ran a
multi-path invocation, so the most common non-trivial form of the flag
was never compared. There is now a check that runs both binaries with
two paths and compares exit code, output and the resulting files.

Note that comparing exit codes alone would not have caught it either --
upstream exits 1 because the hook failed, this exited 1 because it could
not parse, and "both non-zero" matches. The check that discriminates is
the filesystem one: both files fixed, or the bug is back.

Refs #43
@blairham
blairham merged commit 52b02b5 into main Sep 8, 2026
5 checks passed
@blairham
blairham deleted the fix-files-nargs branch September 8, 2026 01:25
blairham added a commit that referenced this pull request Sep 8, 2026
The four --files comparisons added in #50 are checks like any other, so
the headline and the parity table have to count them. Left stale, the
number in the docs and the number the suite produces drift apart, which
is the exact failure this file exists to prevent.
blairham added a commit that referenced this pull request Sep 8, 2026
* test(parity): a divergence must fail the job, not just print

The parity suite could not fail. Every comparison was recorded with
addResult and rendered into the report, but nothing ever asserted on
one: no t.Error, and TestMain forced a non-zero exit only when *zero*
checks ran. So a real divergence printed [FAIL], lowered the percentage
in the report, and exited 0.

That makes the job in ci.yml untrue as described -- "Parity is this
project's entire claim, so a change that breaks it fails before review
rather than after a merge to main." It did not. It reported.

Measured, on a tree carrying two genuine divergences (missing config
made to exit 0 instead of 1):

  without this change   76/78, "FAILURES: 2 checks failed", exit 0
  with this change      76/78, same report,                 exit 1

and a clean tree is 78/78 exit 0 both ways, so this tightens the gate
without moving the bar.

This is not hypothetical. `pre-commit run --files a b` was broken in
every release while the headline read 78/78 -- the suite had no
multi-path case, and even if it had, only the filesystem assertion
discriminates: both binaries exit 1, one because the hook failed and
one because it could not parse.

Refs #43

* docs: 82 checks, not 78

The four --files comparisons added in #50 are checks like any other, so
the headline and the parity table have to count them. Left stale, the
number in the docs and the number the suite produces drift apart, which
is the exact failure this file exists to prevent.
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