fix(fluid-build): Fix failures when deleting files (#22226)

We use the `git ls-files` command to enumerate files to work on in
various places within the build-tools code. Upon investigation, I
determined that _unstaged, deleted files_ are returned by our `git
ls-files` usage. This causes failures since most code is expecting all
the paths returned to exist, and they throw exceptions when they hit
those deleted paths.

If the deleted file is _staged_, then git correctly excludes it - this
inconsistency has led to a lot of confusing behavior for devs, where
things behave differently based on the state of the stage and working
directory.

Unfortunately I cannot find a single git command that returns all the
files we want it to, and none of the ones we don't. So in this change, I
updated the places where we call `ls-files` to also use `git status` to
enumerate deleted files, staged or not, and remove those from the
results of ls-files.

I have confirmed this fixes the Biome task caching, which was what I set
out to fix. It may also fix some related issues with staged files and
policy-check, since policy does use ls-files to enumerate the files to
check. However, I didn't have a good repro of that issue, so I can't say
for certain it's fixed.

I also noticed that some of the uses were checking if the path was a
file - that's guaranteed already. git ls-files will not return paths to
directories - it tracks files only. I removed that code.

There is still code duplication - build-tools and build-cli both have
their own git wrappers. The build-cli one is best, because it's built on
top of simple-git, so much of the infra that is custom code in
build-tools becomes unnecessary in build-cli. Long term we should
converge but that's part of the overall "split build-tools into better
packages" work.

Fixes
[AB#10257](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/10257),
and might also address
[AB#6588](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/6588),
but I don't have a good repro so it's not certain.

build-tools/packages/build-cli/src/commands/check/policy.ts

@@ -197,14 +197,8 @@ export class CheckPolicy extends BaseCommand<typeof CheckPolicy> {
 			}
 		} else {
 			const repo = new Repository({ baseDir: gitRoot });
-			const gitFiles = await repo.gitClient.raw(
-				"ls-files",
-				"-co",
-				"--exclude-standard",
-				"--full-name",
-			);
-
-			filePathsToCheck.push(...gitFiles.split("\n"));
+			const gitFiles = await repo.getFiles(".");
+			filePathsToCheck.push(...gitFiles);
 		}
 
 		const commandContext: CheckPolicyCommandContext = {

build-tools/packages/build-cli/src/library/git.ts

@@ -33,6 +33,7 @@ const defaultGitOptions: Partial<SimpleGitOptions> = {
  */
 export class Repository {
 	private readonly git: SimpleGit;
+	private readonly baseDir: string;
 
 	/**
 	 * A git client for the repository that can be used to call git directly.
@@ -51,6 +52,7 @@ export class Repository {
 		};
 		log?.verbose("gitOptions:");
 		log?.verbose(JSON.stringify(options));
+		this.baseDir = options.baseDir;
 		this.git = simpleGit(options);
 	}
 
@@ -207,4 +209,41 @@ export class Repository {
 
 		return mergeResult.result === "success";
 	}
+
+	/**
+	 * Returns an array containing repo repo-relative paths to all the files in the provided directory.
+	 * A given path will only be included once in the array; that is, there will be no duplicate paths.
+	 * Note that this function excludes files that are deleted locally whether the deletion is staged or not.
+	 *
+	 * @param directory - A directory to filter the results by. Only files under this directory will be returned. To
+	 * return all files in the repo use the value `"."`.
+	 */
+	public async getFiles(directory: string): Promise<string[]> {
+		const results = await this.gitClient.raw(
+			"ls-files",
+			// Includes cached (staged) files.
+			"--cached",
+			// Includes other (untracked) files that are not ignored.
+			"--others",
+			// Excludes files that are ignored by standard ignore rules.
+			"--exclude-standard",
+			// Removes duplicate entries from the output.
+			"--deduplicate",
+			// Shows the full path of the files relative to the repository root.
+			"--full-name",
+			"--",
+			directory,
+		);
+
+		// This includes paths to deleted, unstaged files, so we get the list of deleted files from git status and remove
+		// those from the full list.
+		const allFiles = new Set(results.split("\n").map((line) => line.trim()));
+		const status = await this.gitClient.status();
+		for (const deletedFile of status.deleted) {
+			allFiles.delete(deletedFile);
+		}
+
+		// Files are already repo root-relative
+		return [...allFiles];
+	}
 }

build-tools/packages/build-cli/src/library/repoPolicyCheck/npmPackages.ts

@@ -10,7 +10,6 @@ import fs from "node:fs";
 import { createRequire } from "node:module";
 import { EOL as newline } from "node:os";
 import path from "node:path";
-import * as readline from "node:readline";
 import { writeJson } from "fs-extra/esm";
 import replace from "replace-in-file";
 import sortPackageJson from "sort-package-json";
@@ -24,6 +23,7 @@ import {
 	updatePackageJsonFile,
 	updatePackageJsonFileAsync,
 } from "@fluidframework/build-tools";
+import { Repository } from "../git.js";
 import { queryTypesResolutionPathsFromPackageExports } from "../packageExports.js";
 import { Handler, readFile, writeFile } from "./common.js";
 
@@ -354,39 +354,25 @@ function getReadmeInfo(dir: string): IReadmeInfo {
 }
 
 let computedPrivatePackages: Set<string> | undefined;
-function ensurePrivatePackagesComputed(): Set<string> {
-	if (computedPrivatePackages) {
+async function ensurePrivatePackagesComputed(): Promise<Set<string>> {
+	if (computedPrivatePackages !== undefined) {
 		return computedPrivatePackages;
 	}
 
-	const newPrivatePackages = new Set<string>();
+	computedPrivatePackages = new Set();
 	const pathToGitRoot = child_process
 		.execSync("git rev-parse --show-cdup", { encoding: "utf8" })
 		.trim();
-	const p = child_process.spawn("git", [
-		"ls-files",
-		"-co",
-		"--exclude-standard",
-		"--full-name",
-		"**/package.json",
-	]);
-	const lineReader = readline.createInterface({
-		input: p.stdout,
-		terminal: false,
-	});
-
-	lineReader.on("line", (line) => {
-		const filePath = path.join(pathToGitRoot, line).trim().replace(/\\/g, "/");
-		if (fs.existsSync(filePath)) {
-			const packageJson = JSON.parse(readFile(filePath)) as PackageJson;
-			// eslint-disable-next-line @typescript-eslint/strict-boolean-expressions
-			if (packageJson.private) {
-				newPrivatePackages.add(packageJson.name);
-			}
+	const repo = new Repository({ baseDir: pathToGitRoot });
+	const packageJsons = await repo.getFiles("**/package.json");
+
+	for (const filePath of packageJsons) {
+		const packageJson = JSON.parse(readFile(filePath)) as PackageJson;
+		if (packageJson.private ?? false) {
+			computedPrivatePackages.add(packageJson.name);
 		}
-	});
+	}
 
-	computedPrivatePackages = newPrivatePackages;
 	return computedPrivatePackages;
 }
 
@@ -914,7 +900,7 @@ export const handlers: Handler[] = [
 				return `Error parsing JSON file: ${file}`;
 			}
 
-			const privatePackages = ensurePrivatePackagesComputed();
+			const privatePackages = await ensurePrivatePackagesComputed();
 			const errors: string[] = [];
 
 			// eslint-disable-next-line @typescript-eslint/strict-boolean-expressions

build-tools/packages/build-tools/src/common/gitRepo.ts

@@ -3,10 +3,8 @@
  * Licensed under the MIT License.
  */
 
-import path from "node:path";
 import { parseISO } from "date-fns";
 import registerDebug from "debug";
-import { statSync } from "fs-extra";
 import { exec, execNoError } from "./utils";
 
 const traceGitRepo = registerDebug("fluid-build:gitRepo");
@@ -217,18 +215,52 @@ export class GitRepo {
 	}
 
 	/**
-	 * Returns an array containing all the files in the the provided path.
-	 * Returned paths are rooted at the root of the repo.
+	 * Returns an array containing repo root-relative paths to files that are deleted in the working tree.
 	 */
-	public async getFiles(directory: string): Promise<string[]> {
-		const results = await this.exec(
-			`ls-files -co --exclude-standard --full-name -- ${directory}`,
-			`get files`,
-		);
+	public async getDeletedFiles(): Promise<string[]> {
+		const results = await this.exec(`status --porcelain`, `get deleted files`);
 		return results
 			.split("\n")
-			.map((line) => line.trim())
-			.filter((file) => statSync(path.resolve(this.resolvedRoot, file)).isFile());
+			.filter((t) => t.startsWith(" D "))
+			.map((t) => t.substring(3));
+	}
+
+	/**
+	 * Returns an array containing repo repo-relative paths to all the files in the provided directory.
+	 * A given path will only be included once in the array; that is, there will be no duplicate paths.
+	 * Note that this function excludes files that are deleted locally whether the deletion is staged or not.
+	 *
+	 * @param directory - A directory to filter the results by. Only files under this directory will be returned. To
+	 * return all files in the repo use the value `"."`.
+	 */
+	public async getFiles(directory: string): Promise<string[]> {
+		/**
+		 * What these git ls-files flags do:
+		 *
+		 * ```
+		 * --cached: Includes cached (staged) files.
+		 * --others: Includes other (untracked) files that are not ignored.
+		 * --exclude-standard: Excludes files that are ignored by standard ignore rules.
+		 * --deduplicate: Removes duplicate entries from the output.
+		 * --full-name: Shows the full path of the files relative to the repository root.
+		 * ```
+		 */
+		const command = `ls-files --cached --others --exclude-standard --deduplicate --full-name -- ${directory}`;
+		const [fileResults, deletedFiles] = await Promise.all([
+			this.exec(command, `get files`),
+			this.getDeletedFiles(),
+		]);
+
+		// This includes paths to deleted, unstaged files, so we get the list of deleted files from git status and remove
+		// those from the full list.
+		const allFiles = new Set(fileResults.split("\n").map((line) => line.trim()));
+
+		for (const deletedFile of deletedFiles) {
+			allFiles.delete(deletedFile);
+		}
+
+		// Files are already repo root-relative
+		return [...allFiles];
 	}
 
 	/**

build-tools/packages/build-tools/src/fluidBuild/tasks/leaf/leafTask.ts

@@ -498,8 +498,11 @@ export abstract class LeafWithDoneFileTask extends LeafTask {
 					return true;
 				}
 				this.traceTrigger(`mismatched compare file: ${doneFileFullPath}`);
-				traceTaskTrigger(doneFileExpectedContent);
-				traceTaskTrigger(doneFileContent);
+				// These log statements can be useful for debugging, but they're extremely long and completely
+				// obscure other logs.
+				// In the future we can consider logging just the diff between the input and output.
+				// this.traceTrigger(doneFileExpectedContent);
+				// this.traceTrigger(doneFileContent);
 			} else {
 				this.traceTrigger(
 					"unable to generate done file expected content (getDoneFileContent returned undefined)",