Skip to content

refactor(ci): rely on pre-commit file arguments#3201

Open
Standing-Man wants to merge 2 commits intoapache:masterfrom
Standing-Man:remove-staged-logic
Open

refactor(ci): rely on pre-commit file arguments#3201
Standing-Man wants to merge 2 commits intoapache:masterfrom
Standing-Man:remove-staged-logic

Conversation

@Standing-Man
Copy link
Copy Markdown
Contributor

@Standing-Man Standing-Man commented Apr 30, 2026

Which issue does this PR close?

Closes #3195.

Rationale

Some pre-commit hook scripts still accepted --staged and used git diff --cached to discover staged files internally.

That duplicated pre-commit's own file selection behavior. Since these hooks now use pass_filenames, pre-commit already passes the matched staged files directly to the scripts. Keeping a second staged-file discovery path made the behavior harder to reason about and easier to drift from pre-commit's filtering rules.

What changed?

  • Removed --staged argument handling from file-scoped hook scripts.
  • Removed git diff --cached based staged-file discovery.
  • Updated help text to no longer mention --staged.
  • Kept explicit file arguments as the primary input path for pre-commit.
  • Kept full-repository behavior for local/manual runs when no files are provided.
  • Preserved --ci behavior for GitHub Actions / PR-diff checks.

Local Execution

  • Passed / not passed
    Passed
  • Pre-commit hooks ran / not ran
    Ran

AI Usage

  1. Which tools? (e.g., GitHub Copilot, Claude, ChatGPT) Codex
  2. Scope of usage? (e.g., autocomplete, generated functions, entire implementation)
    Drafted changes and updated the pr-commit scripts based on human direction.
  3. How did you verify the generated code works correctly?
    🚧: Not tested locally yet.
  4. Can you explain every line of the code if asked?
    Yes. I reviewed the changes

@Standing-Man Standing-Man marked this pull request as ready for review April 30, 2026 13:55
Copy link
Copy Markdown
Contributor

@hubcio hubcio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

behavior change worth noting in the commit body: running scripts with --ci outside CI (no GITHUB_BASE_REF, no CI env) now scans the whole repo instead of staged files. not a bug since pre-commit no longer relies on it, but a one-line note in the commit message would help anyone running the scripts manually.

Comment thread scripts/ci/taplo.sh Outdated
# Fallback to staged files
git diff --cached --name-only --diff-filter=ACM -- '*.toml'
# Local fallback: check all TOML files
find . -name '*.toml' \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the local fallback here uses find . while the sibling scripts (binary-artifacts.sh, trailing-newline.sh, trailing-whitespace.sh) use git ls-files for the same fallback. inconsistent. could align to git ls-files -- '*.toml' and also collapse the duplication with the all) branch below since they're now functionally identical.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve replaced find with git ls-files in the latest commit to collect the files to be checked.

Comment thread scripts/ci/markdownlint.sh Outdated
;;
--all)
FILE_MODE="all"
shift
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--all is now a no-op (just shift, no FILE_MODE var anymore). help text on line 43 still says (default). flag is harmless but redundant - either drop it or document it as accepted-for-compat.

Comment thread scripts/ci/shellcheck.sh Outdated
;;
--all)
FILE_MODE="all"
shift
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as markdownlint.sh - --all is now a no-op after FILE_MODE removal. help text still claims it's the default. either drop the flag or keep it as a documented compat shim.

@Standing-Man Standing-Man force-pushed the remove-staged-logic branch from 7d60611 to 1d8fae2 Compare May 6, 2026 13:14
Signed-off-by: StandingMan <jmtangcs@gmail.com>
@Standing-Man Standing-Man force-pushed the remove-staged-logic branch from 1d8fae2 to d55965e Compare May 6, 2026 13:15
Signed-off-by: StandingMan <jmtangcs@gmail.com>
@Standing-Man Standing-Man force-pushed the remove-staged-logic branch from d55965e to 3f2faec Compare May 6, 2026 13:17
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.

Align pre-commit hooks to use passed filenames and remove internal git diff logic

2 participants