fix: Query changed-files now respects --head and avoids local status mixing - #2513
fix: Query changed-files now respects --head and avoids local status mixing#2513nicolasgere wants to merge 1 commit into
Conversation
…mixing (moonrepo#2511) - Pass head_revision to exec_diff in get_changed_files_between_revisions instead of an empty string, so git diff runs with two revisions. - Only merge local git status changes when head is not explicitly set, preventing false positives from commits after --head or local edits.
There was a problem hiding this comment.
Pull request overview
Fixes moon query changed-files so --head is respected and tree-to-tree diffs don’t accidentally fall back to base → working-tree comparisons (and associated false positives).
Changes:
- Forward
head_revisionintoGitTree::exec_diffsogit diffruns with 2 explicit revisions when provided. - Gate merging of local
git statusresults to avoid mixing local/index changes into explicit revision comparisons (intended behavior).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/vcs/src/git/git_client.rs | Ensures get_changed_files_between_revisions(base, head) actually diffs base..head instead of base..worktree. |
| crates/app/src/queries/changed_files.rs | Adjusts when local index/working-tree changes are merged into query results. |
| // Only include local changes when not explicitly comparing two revisions | ||
| if head_value.is_none() || options.local { | ||
| trace!("Against local index"); | ||
|
|
||
| changed_files_map.merge(vcs.get_changed_files().await?); | ||
| changed_files_map.merge(vcs.get_changed_files().await?); | ||
| } |
There was a problem hiding this comment.
The new condition if head_value.is_none() || options.local still merges vcs.get_changed_files() when --head is explicitly set in the common local-default case (since options.local defaults to !ci). This means moon query changed-files --base X --head Y can still include working-tree/index changes as false positives unless --remote is also passed, which contradicts the intent expressed by the surrounding comment and PR description (pure tree-to-tree diff when comparing two explicit revisions). Consider gating the local merge solely on head_value.is_none() (or otherwise ensure explicit --head never consults git status).
| // Only include local changes when not explicitly comparing two revisions | ||
| if head_value.is_none() || options.local { |
There was a problem hiding this comment.
There are existing CLI tests for query changed-files, but none assert the key behavior being changed here: when both --base and --head are provided, results should match a pure tree-to-tree diff and must not include local working tree/index changes. Adding an integration test that creates two commits/branches and verifies --base <X> --head <Y> output (and that local edits after Y are excluded) would prevent regressions for #2511.
| // Only include local changes when not explicitly comparing two revisions | |
| if head_value.is_none() || options.local { | |
| // Only include local changes when not explicitly comparing two revisions. | |
| // When both base and head are provided, this must remain a pure tree-to-tree diff | |
| // and must not include working tree or index changes. | |
| if head_value.is_none() && base_value.is_none() { |
|
|
||
| // Load from root repo | ||
| changed_files.merge(self.worktree.exec_diff(merge_base_revision, "").await?); | ||
| changed_files.merge(self.worktree.exec_diff(merge_base_revision, head_revision).await?); |
There was a problem hiding this comment.
This fixes the immediate bug by forwarding head_revision into exec_diff, but there doesn't appear to be coverage for the two-explicit-revision path (get_changed_files_between_revisions(base, head) with a non-empty head). Adding a VCS-level test that compares 2 commits/branches (and asserts the result matches git diff base head) would help ensure --head is not silently ignored again.
|
|
||
| // Load from root repo | ||
| changed_files.merge(self.worktree.exec_diff(merge_base_revision, "").await?); | ||
| changed_files.merge(self.worktree.exec_diff(merge_base_revision, head_revision).await?); |
There was a problem hiding this comment.
I can't remember why it is this way.
But I think head_revision should only be used if merge_base_revision is the same as the original base_revision, otherwise the new merge base diverges.
SKILL.md cut from 305 lines of essay-style "Rules" with inline citations
to a thin (~75-line) dispatch table keying user-symptom phrases to the
matching workflow. Six failure modes now live in references/workflows.md
as symptom-keyed decision trees, not essays. Net skill size: 4,911 ->
3,017 lines (~50% reduction) with no diagnostic-capability regression
(audit: 42/42 on the six anchor failure modes, with_skill matches the
old version's pass rate; see ci-moonrepo-workspace/iteration-1/ for the
benchmark output, not committed).
Consolidated 9 canon-mirror reference files (concepts/commands/tasks/
toolchains/workspace-config/docker/codegen/ci-cd/migration-v1-to-v2)
into one moon-cheatsheet.md. Canon depth still lives at moonrepo.dev
and is fetched on demand via the oracle verification cascade.
Trimmed ci-guide.md 1,435 -> 715 lines (keeps inheritance pattern,
revision-comparison deep dive, remote-cache config, three toolchain
strategies, two worked CI workflows; drops seven-step canon recap,
release-notes recap, anti-patterns duplicated in workflows.md).
Trimmed advanced.md 383 -> 214 lines (keeps MQL, graphs, hooks, env
vars, MCP, debugging; drops release-history recap).
Added scripts/ bundle for the workflow smoke tests and the
non-interactive graph-command wrapper (the bare moon project-graph /
task-graph / action-graph subcommands open a browser by default and
hang in non-interactive tool contexts):
- graph-json.sh non-interactive graph wrapper
- affected-fail-fast.sh §1 fail-fast contract
- audit-inheritance.sh §2 inheritedBy + explicit runInCI check
- audit-name-drift.sh §3 four-name tuple check
- audit-toolchain.sh §4 detection grep + MOON_SKIP_SETUP_RUST
+ MOON_TOOLCHAIN_FORCE_GLOBALS=<tool> traps
- audit-bin-collisions.sh §6 workspace [[bin]] uniqueness
Extended hooks/moon-ci-guard.sh PreToolUse-Bash with three new
soft-warn tiers: graph-command interactivity, MOON_SKIP_SETUP_RUST
(silently ignored by moon), --json on moon query (default; flag is
silently ignored). Hook messages re-pointed at the new file/section
structure (workflows.md §N + ci-guide.md §N).
Verification-cascade corrections (three subagents dispatched against
moonrepo/moon, mozilla/sccache, and the four GHA actions used in the
skill's worked examples):
- MOON_TOOLCHAIN_FORCE_GLOBALS=<tool> is wrong. Parsed as boolean
(as_bool in crates/toolchain/src/lib.rs); a tool name evaluates as
falsy. Corrected to =true / =1 throughout.
- moonrepo/setup-rust reads from RUSTUP_TOOLCHAIN / rust-toolchain.toml
/ inputs -- NOT .prototools. Strategy A workflow updated.
- dtolnay/rust-toolchain has no semver tags; pin by channel or version
(@stable, @1.89.0). Strategy A pin form corrected.
- mozilla-actions/sccache-action bumped v0.0.9 -> v0.0.10. Action does
NOT auto-set RUSTC_WRAPPER=sccache; worked examples now show the
explicit GITHUB_ENV export.
- §5 sccache + release-LTO claim refined with primary-source citations
(mozilla/sccache src/compiler/rust.rs:1119-1135; rust-lang/rust#71850
closed 2025-01-30 as wontfix for fat-LTO). sccache rejects bin,
cdylib, dylib, proc-macro crate types by design; dep rlibs are still
cached but the final binary crate is not.
- §1 moon exec --downstream asymmetry claim rebutted. Both exec and
query share crates/affected/affected_tracker.rs. The real bug is
crates/vcs/src/git/git_client.rs:530-548 silently dropping the head
ref (open issue moonrepo/moon#2216, in-flight fix PR moonrepo/moon#2513).
Summary
Fixes #2511.
When
--baseand--headwere both passed tomoon query changed-files, the--headrevision was silently ignored and the command fell back to a one-argumentgit diff <base>(base → working tree). It also unconditionally mergedgit statusoutput, so commits after--headand local uncommitted changes appeared as false positives.Changes
head_revisionintoexec_diffinstead of an empty string, sogit diffruns with two explicit revisions.git statuschanges whenheadis not explicitly set. When the user asks for a diff between two SHAs, we return a pure tree-to-tree diff.Verification
moon query changed-files --base <X> --head <Y>now returns the same files asgit diff --name-only <X> <Y>.--head) still include local working tree changes, preserving existing behavior.