Skip to content

Include untracked files in working-tree diff reviews#123

Merged
benvinegar merged 2 commits intomodem-dev:mainfrom
ferologics:feat/untracked-working-tree-review
Mar 26, 2026
Merged

Include untracked files in working-tree diff reviews#123
benvinegar merged 2 commits intomodem-dev:mainfrom
ferologics:feat/untracked-working-tree-review

Conversation

@ferologics
Copy link
Copy Markdown
Contributor

@ferologics ferologics commented Mar 26, 2026

https://pi.dev/session/#4cb769589e158d30950cb75ef33ae439

Summary

  • include untracked files by default in working-tree hunk diff reviews
  • keep staged/range/show/stash behavior unchanged, with exclude_untracked / --exclude-untracked as the opt-out
  • load tracked and untracked files separately so watch mode can use cheap untracked signatures and parser-sensitive filenames still render correctly
  • keep repo-local --agent-context sidecars visible when they are also untracked files in the repo
  • add focused tests for subdirectory launches, pathspec filtering, watch signatures, parser-sensitive filenames, and repo-local sidecars

Closes #109.

Testing

  • bun test test/cli.test.ts test/config.test.ts test/loaders.test.ts test/watch.test.ts test/git.test.ts
  • bun run typecheck
  • bun run src/main.tsx -- diff --help

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 26, 2026

Greptile Summary

This PR makes untracked working-tree files visible in hunk diff reviews by default, with an opt-out via --exclude-untracked / exclude_untracked = true in config. Staged, range, show, and stash-show modes are untouched.\n\nKey design decisions:\n- Tracked and untracked patches are loaded separately: tracked via the existing git diff path, untracked via git diff --no-index -- /dev/null <file> (run from repo root, exit codes 0 and 1 both accepted).\n- normalizeUntrackedPatchHeaders rewrites the diff --git, +++, and binary-file header lines to use plain (C-escape–encoded) paths, bypassing git's own quoting for filenames that contain tabs, backslashes, or newlines — the original filePath is then reattached as name after parsing so the UI displays the real path.\n- Watch mode uses cheap statSignature (size + mtime + inode) for untracked files instead of embedding full patch content, avoiding unbounded signature growth for large files.\n- Agent-context sidecars that are themselves untracked repo files now appear in the review, which is intentional per the PR description.\n- The existing agent-context test fixture is correctly moved outside the repo so it no longer leaks into unrelated test assertions.\n\nOne item worth verifying: parseUntrackedPatchFile asserts that exactly one parsed file is returned for each synthetic patch. For a 0-byte untracked file, git diff --no-index emits only a header without ---/+++ lines; if parsePatchFiles yields 0 entries for this input the function will throw and crash loadGitChangeset. There is currently no test covering this edge case.

Confidence Score: 4/5

PR is safe to merge for the primary macOS/Linux audience; one untested edge case (0-byte untracked files) should be verified before broad rollout.

The implementation is architecturally clean and well-tested across the main scenarios (default inclusion, opt-out, pathspecs, parser-sensitive filenames, subdirectory launches, agent sidecars, watch signatures). Prior thread concerns about misleading variable names, redundant git rev-parse in watch mode, and Windows portability have either been addressed or are pre-existing non-blockers. The only new gap is the 0-byte untracked file edge case in parseUntrackedPatchFile, which could cause a runtime crash in repos with empty untracked files — worth a quick test to confirm the parser handles it gracefully before shipping.

src/core/loaders.ts — specifically parseUntrackedPatchFile and buildUntrackedDiffFile for the 0-byte file edge case.

Important Files Changed

Filename Overview
src/core/git.ts Adds listGitUntrackedFiles, runGitUntrackedFileDiffText, and supporting helpers; refactors internal runGitCommand to accept a set of allowed exit codes (0 and 1 for --no-index diffs). Logic is correct; the only pre-existing concern is /dev/null portability on Windows (flagged in a prior thread).
src/core/loaders.ts Adds escapeUntrackedPatchPath, normalizeUntrackedPatchHeaders, parseUntrackedPatchFile, and buildUntrackedDiffFile; loadGitChangeset now merges tracked and untracked file lists. Header normalization correctly handles parser-sensitive filenames (tabs, backslashes, newlines). No test for 0-byte untracked files, which may silently throw in parseUntrackedPatchFile if the patch parser returns 0 entries for a headerless empty-file diff.
src/core/watch.ts Adds gitWorkingTreeWatchSignature which uses cheap stat-based signatures for untracked files instead of full patch content; correctly calls resolveGitRepoRoot once per tick to build absolute paths for stat.
src/core/config.ts Adds excludeUntracked to config reading, merging, and resolution; defaults to false (include untracked) correctly.
test/loaders.test.ts Adds tests for include/exclude untracked, pathspec filtering, parser-sensitive filenames, agent-sidecar inclusion, and subdirectory launches. Moves agent fixture outside the repo (to avoid it appearing as an untracked file in unrelated tests). No test for 0-byte untracked files.
test/watch.test.ts New test file; covers signature content (no patch body embedded), change detection on stat mutation, and correct ignore of untracked files when excludeUntracked is set.
src/core/types.ts Adds optional excludeUntracked?: boolean to CommonOptions. Minimal, correct change.
src/core/cli.ts Adds --exclude-untracked / --no-exclude-untracked options using commander's addOption with hideHelp() for the negation form; wired through resolveBooleanFlag.
test/config.test.ts Adds a test that verifies default value (false), config-file override, and CLI override for excludeUntracked. Covers the three-layer merge correctly.
test/cli.test.ts Adds round-trip parsing tests for --exclude-untracked and --no-exclude-untracked.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[loadGitChangeset] --> B[resolveGitRepoRoot]
    A --> C[runGitText — git diff tracked]
    C --> D[normalizePatchChangeset]
    D --> E[trackedChangeset / trackedFiles]

    A --> F{shouldIncludeUntrackedFiles?}
    F -- "staged / range / excludeUntracked=true" --> G[return trackedChangeset]
    F -- "working tree, default" --> H[listGitUntrackedFiles\ngit status --porcelain=v1 -z]
    H --> I{untrackedFiles.length > 0?}
    I -- "no" --> G
    I -- "yes" --> J[buildUntrackedDiffFile x N]
    J --> K[runGitUntrackedFileDiffText\ngit diff --no-index /dev/null file]
    K --> L[normalizeUntrackedPatchHeaders\nescape parser-sensitive paths]
    L --> M[parseUntrackedPatchFile\noverride name = filePath]
    M --> N[buildDiffFile]
    N --> O[merge trackedFiles + untrackedFiles]
    O --> P[orderDiffFiles — agent context ordering]
    P --> Q[Changeset returned]

    subgraph Watch Mode
    W1[computeWatchSignature] --> W2[gitWorkingTreeWatchSignature]
    W2 --> W3[runGitText — git diff tracked patch]
    W2 --> W4[resolveGitRepoRoot]
    W2 --> W5[listGitUntrackedFiles]
    W5 --> W6[statSignature per untracked file\nsize + mtime + inode only]
    W3 & W6 --> W7[joined signature string]
    end
Loading

Reviews (2): Last reviewed commit: "fix untracked diff watch and parser edge..." | Re-trigger Greptile

src/core/git.ts Outdated
return trackedPatch;
}

const normalizedRepoRoot = repoRoot ?? resolveGitRepoRoot(input, { cwd, gitExecutable });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Misleading variable name normalizedRepoRoot

The variable is named normalizedRepoRoot, but at this point the path has NOT been run through realpathSync — it is either the raw value passed in by the caller (e.g. the git rev-parse --show-toplevel output from loaders.ts) or whatever resolveGitRepoRoot returns. The actual symlink resolution only happens inside excludedUntrackedPaths. Using this as the cwd for runGitCommand (line 395) works in practice because the OS resolves symlinks transparently, but the naming implies a normalization step that hasn't occurred here and could be confusing to future readers.

Consider renaming to resolvedRepoRoot to better reflect what it is at this stage:

Suggested change
const normalizedRepoRoot = repoRoot ?? resolveGitRepoRoot(input, { cwd, gitExecutable });
const resolvedRepoRoot = repoRoot ?? resolveGitRepoRoot(input, { cwd, gitExecutable });

(and updating the two subsequent uses on lines 383 and 395)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@ferologics ferologics force-pushed the feat/untracked-working-tree-review branch from 099829e to 2d46246 Compare March 26, 2026 11:09
@ferologics ferologics marked this pull request as draft March 26, 2026 11:59
@ferologics ferologics marked this pull request as ready for review March 26, 2026 12:35
@ferologics
Copy link
Copy Markdown
Contributor Author

A quick note on scope/design: I looked at broader cleanup options for working-tree diff loading, but kept this PR focused on the user-visible behavior change plus the correctness/perf fixes needed to make it safe by default.

Specifically, this PR does three things together:

  • includes untracked files by default for working-tree hunk diff
  • keeps watch-mode cheap by using untracked file signatures instead of rebuilding full untracked patches on every poll
  • handles parser-sensitive untracked filenames without collapsing the whole review

I intentionally did not fold in a broader refactor (for example, a dedicated working-tree adapter module) because that felt like extra blast radius for a feature fix.

Also, re: --agent-context: current behavior is intentionally transparent — if the sidecar is also an untracked repo file, it still appears in the working tree review. I avoided adding a hidden special-case exclusion for that path.

If this lands and we want follow-up cleanup later, the most obvious candidates seem to be:

  • a dedicated working-tree adapter module
  • moving exclude_untracked out of CommonOptions
  • adding a higher-level integration test around watch/reload

@benvinegar
Copy link
Copy Markdown
Member

Sounds good, thanks for the added explanation. I think I'll merge and just start experimenting/testing during regular usage, see where we wind up.

@benvinegar benvinegar merged commit ecdace9 into modem-dev:main Mar 26, 2026
3 checks passed
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.

Support untracked files

2 participants