Skip untracked symlinks-to-directories in the review diff - #63
Merged
Conversation
`git status --porcelain -uall` reports a symlink to a directory as a single `?? link` entry — it does not recurse. `git diff --no-index -- /dev/null link` then walks into the link and pairs `/dev/null` with a path inside it, exiting 1 with `error: Could not access 'link/null'`. `untrackedFileDiff`'s predicate correctly reads that as a failure rather than "the sides differ", so it threw, and one untracked symlink-to-directory anywhere in the working tree took down `reviewDiff()` and with it the whole review loop. Guard the call instead: a symlink resolving to a directory is named in the payload rather than handed to `--no-index`. Symlinks to a file or to nothing render fine — as their own mode-120000 new-file diff carrying the link target — so they still go through unchanged.
The skip's `&& os.isDir` conjunct survived mutation: widening it to every symlink would silently replace each file-symlink's mode-120000 diff with a skip line, and nothing would go red. This test guards how narrow the filter is, rather than the abort it fixes.
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
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.
An untracked symlink pointing at a directory aborted the whole review loop.
git status --porcelain -uallreports such a symlink as a single?? linkdirentry — git does not recurse into it.
git diff --no-index -- /dev/null linkdirthen follows the link, finds a directory on one side and a file on the other,
and tries to pair
/dev/nullwithlinkdir/null:exit 1, message on stderr.
OsGitTool.untrackedFileDiffclassifies thatcorrectly — its
exit 1 && empty stderrpredicate means "the sides differ", andthis is exit 1 with stderr — so it calls
fail, which throwsOrcaFlowException. One such symlink anywhere in the working tree took downreviewDiff()and with it the review loop.The fix
The predicate is untouched. It was tightened deliberately in #46, and it is
correct: before that, exit 1 was unconditionally read as "the sides differ",
which swallowed a real failure — from a subdirectory,
git diff --no-index -- /dev/null sub/newfile.txtalso exits 1 with an access error, and every newfile's contents were vanishing while
reviewDiff()reported success. Looseningit would restore that bug. The symlink case is the same signal from a different
cause, so the fix is to stop asking git to render the path at all.
untrackedFileDiffnow short-circuits a symlink that resolves to a directoryand emits a line naming it instead. It is the single shared path — both
reviewDiff()andpendingChanges()reach it throughwithNewFileContents—so both are fixed once.
What was measured, not assumed
Against git 2.53.0, for each kind of untracked symlink:
error: Could not access 'linkdir/null'new file mode 120000, link target as contentnew file mode 120000, link target as contentOnly the directory case is broken. git never follows a symlink to a file — it
renders the link itself as a mode-120000 blob holding the target path.
Only directory symlinks are held back, not all symlinks: a symlink to a file
or to nothing already renders correctly and is a real change the reviewer should
see, so dropping those would trade one silent omission for another.
Skipped symlinks are announced, not silently omitted, as a
# skipped <path>: symlink to a directoryline. A reviewer who believes it saweverything is worse than one told something was skipped — the same reason
CommitDiffmarks a truncated diff. It also keeps the reviewer able to flag anewly created symlink at all, which matters given this repo already treats
symlinks in the working tree as a surface worth refusing (
OrcaDir,ProgressScan).untrackedPaths()is deliberately left alone, sopendingChanges().newFilesstill lists the symlink — verified that
git add -Adoes commit it, as amode-120000 blob, which is exactly what that list promises.
Tests
Two in
OsGitToolTest, each mutation-checked to fail alone.The abort. A repo with an untracked symlink-to-directory alongside an
ordinary new file. Replacing the guard condition with
falsefails exactly thistest, with
orca.OrcaFlowException: git diff --no-index -- /dev/null linkdir failed (exit 1): error: Could not access 'linkdir/null'— the reported errorverbatim.
How narrow the filter is — deliberately not a test of the fix, and not
redundant with the one above. It pins that a symlink to a file still renders
as its own mode-120000 diff. Widening the guard to
os.isLink(path)alone failsexactly this test and nothing else; without it, that widening would silently
swallow every file-symlink's diff — the same "reviewer told nothing was there"
failure this PR exists to prevent. Please don't delete it as a duplicate of the
directory test; it guards the decision, not the bug.
Sequencing vs #59
Branched off
master, not off #59. #59 (review-change-set-since-stage-base)adds a
sinceparameter and touchesreviewDiff,withNewFileContentsand thetrait scaladoc in the same file, but does not touch
untrackedFileDifforuntrackedPaths— the two changes are independent. Whichever merges secondpicks up a small conflict in the
reviewDiffscaladoc block and inwithNewFileContents's signature; both resolve by keeping both edits. Noordering is required either way.
Known adjacent defect, not fixed here
The identical abort fires for an untracked nested git repository:
git status -ualldoes not recurse into one either, reporting?? nested/(with atrailing slash), and
git diff --no-index -- /dev/null nested/fails the sameway with
error: Could not access 'nested/null'. Verified on git 2.53.0. It isa different cause with a different fix and a different message, so it is left
for its own change rather than widened into this one.