Skip to content

git diff reports a phantom added file for a path staged then removed from the working tree #72

Description

@Neal006

Describe the bug

git diff reports a file that exists in neither endpoint it is comparing.

When a path is staged and then removed from the working tree, git diff HEAD
compares HEAD against the working tree, and the path is absent from both. Real
git prints nothing. diffWith prints a header-only patch with no hunks, and
diffSummaryWith reports the path as an addition, so git diff --stat and
git diff --name-status list a file that was never added and does not exist.

The two surfaces also disagree with each other in a subtler way. diffWith
happens to emit a patch body, so a consumer diffing text sees a stray file
header. diffSummaryWith reports status: "A" with insertions: 0 and
deletions: 0, which reads as "an empty file was added".

Expected behavior

Nothing on every diff surface, matching git:

$ git init -b main . && echo base > base.txt && git add base.txt && git commit -m init
$ echo hello > added.txt && git add added.txt && rm added.txt

$ git diff HEAD
$ git diff HEAD --stat
$ git diff HEAD --name-status
$ git status --short
AD added.txt

git status is right to mention the path, because the index holds a version of
it. git diff HEAD is right to stay silent, because neither side it compares
contains it.

Steps to reproduce

Against 76d9e75. The failing test below is written in the style of the
existing packages/computer/src/git/diff.test.ts, using memfs with the real
isomorphic-git and the real diff package, so nothing in the path under test
is stubbed.

packages/computer/src/git/diff.repro.test.ts:

import { createPatch } from "diff";
import git from "isomorphic-git";
import { fs as memfs, vol } from "memfs";
import { beforeEach, describe, expect, it } from "vitest";

import { diffSummaryWith, diffWith, type IsomorphicGitDiffClient } from "./diff.js";

const DIR = "/repo";
const AUTHOR = { name: "test", email: "test@example.test" };
const isomorphicGit = git as unknown as IsomorphicGitDiffClient;

function deps() {
  return {
    git: isomorphicGit,
    fs: memfs,
    createPatch,
    readFile: (path: string) => memfs.promises.readFile(path) as Promise<Uint8Array | string>,
    dir: DIR,
  };
}

// Commit a base file, stage a second file, then remove it from the
// working tree. Leaves the status matrix row [path, 0, 0, 3].
async function stageThenRemove(): Promise<void> {
  await memfs.promises.mkdir(DIR, { recursive: true });
  await git.init({ fs: memfs, dir: DIR, defaultBranch: "main" });
  await memfs.promises.writeFile(`${DIR}/base.txt`, "base\n");
  await git.add({ fs: memfs, dir: DIR, filepath: "base.txt" });
  await git.commit({ fs: memfs, dir: DIR, message: "init", author: AUTHOR });

  await memfs.promises.writeFile(`${DIR}/added.txt`, "hello\n");
  await git.add({ fs: memfs, dir: DIR, filepath: "added.txt" });
  await memfs.promises.unlink(`${DIR}/added.txt`);
}

describe("diff with a staged addition removed from the working tree", () => {
  beforeEach(() => {
    vol.reset();
  });

  it("reports the path as absent from both endpoints", async () => {
    await stageThenRemove();
    const rows = await git.statusMatrix({ fs: memfs, dir: DIR });
    expect(rows).toContainEqual(["added.txt", 0, 0, 3]);
  });

  it("emits no patch, the way `git diff HEAD` does", async () => {
    await stageThenRemove();
    expect(await diffWith(deps())).toBe("");
  });

  it("omits the path from the summary, the way `git diff HEAD --stat` does", async () => {
    await stageThenRemove();
    expect(await diffSummaryWith(deps())).toEqual([]);
  });
});

Run it:

npx vitest run src/git/diff.repro.test.ts --root packages/computer

The first assertion passes, which pins the input. The other two fail:

FAIL  emits no patch, the way `git diff HEAD` does
  expected 'Index: added.txt\n===============…' to be ''

FAIL  omits the path from the summary, the way `git diff HEAD --stat` does
  - []
  + [ { path: "added.txt", status: "A", insertions: 0, deletions: 0 } ]

Diagnosis

collectDiffEntries in packages/computer/src/git/diff.ts is the shared
traversal behind both diffWith and diffSummaryWith. For this state
statusMatrix returns ["added.txt", 0, 0, 3]:

Column Value Effect in the collector
headStatus 0 oldText stays "" at diff.ts:178-181
workdirStatus 0 not caught by the === 1 skip at diff.ts:175, so newText stays "" at diff.ts:182-183
stageStatus 3 the index holds a version, which is why the row exists at all

So the entry is pushed with oldText === newText === "" and status: "A".

diffWith's only guard is patch.trim().length > 0 at diff.ts:124. For two
identical strings, createPatch still returns a 106 character header:

Index: added.txt
===================================================================
--- added.txt
+++ added.txt

That is non-empty after trimming, so the guard passes and the phantom patch is
emitted. diffSummaryWith has no such guard at all and maps every collected
entry.

The ref-to-ref collector one function down already handles this correctly, at
diff.ts:224:

if (a === b) continue;

The working-tree collector is missing the same guard. One omission, two broken
surfaces.

Proposed fix

Add the guard to the shared collector so both surfaces inherit it. Guarding
inside diffWith alone would leave diffSummaryWith still reporting the
phantom entry, and guarding each caller separately would be two copies of a
check that belongs in the traversal.

--- a/packages/computer/src/git/diff.ts
+++ b/packages/computer/src/git/diff.ts
@@ -181,6 +181,13 @@ async function collectDiffEntries(opts: DiffWithDeps): Promise<DiffEntry[]> {
         : "";
     const newText =
       workdirStatus === 2 ? await readWorkdirAsText(opts.readFile, dir, filepath) : "";
+    // Both endpoints agree, so there is nothing to render. A path
+    // staged and then removed from the working tree lands here: the
+    // status matrix still reports it because the index holds a
+    // version, but `git diff HEAD` compares HEAD to the working
+    // tree and the path is absent from both. Matches the same guard
+    // in the ref-to-ref collector below.
+    if (oldText === newText) continue;
     // headStatus 0 -> not in the base -> added. workdirStatus 0
     // -> gone from the working tree -> deleted. Otherwise it's a
     // content change.

What this proves

  • Before the change, the two behavior assertions above fail and the input
    assertion passes.
  • After the change, npx vitest run src/git/ --root packages/computer reports
    128 passing tests with no regressions across the existing git suite.
  • npx biome check is clean on both changed files.

src/git/cli.test.ts fails to collect in my environment with
Cannot find package '@cloudflare/dofs/testing'. That reproduces on a clean
checkout of main without a prior npm run build, so it is unrelated to this
change.

Compatibility

No public API change. DiffSummaryEntry and the patch string shape are
untouched, and no exported signature moves. The only behavior difference is
that output real git never produced stops appearing. Any consumer parsing
--stat or --name-status gets strictly fewer, and more accurate, rows.

The change touches one package and needs no changeset beyond a patch bump if
you want the fix in a release.

Environment

  • @cloudflare/computer at 0.1.0-alpha.1, repo at 76d9e75
  • Node 22, npm workspaces
  • Windows 11, though the reproduction is filesystem-independent: it runs
    entirely on memfs and does not touch FUSE, Docker, or a container backend

The patch and the test are on a branch if that is easier to take directly:
https://github.com/Neal006/computer/tree/fix/diff-phantom-entry

CONTRIBUTING.md says to start from the problem rather than a pull request, so
I have not opened one. Happy to send it if you would like it, or to leave it
here for you to take, whichever is less work for you.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions