Fix up for the GitDiffService (#2116)

* initial pass at untracked file changes, testing next

* add final trailing \n

* unit tests for function

* fix test execution

* tweak some comments and formattting

* another comment tweak after testing

* small change to handling fully empty files

* copilot PR feedback

---------

Co-authored-by: Ian Huff <ianhuff@Ians-MacBook-Pro-2.local>
Co-authored-by: Ian Huff <ianhuff@Mac.home>

Author: 3 people

src/extension/prompt/vscode-node/gitDiffService.ts

@@ -114,22 +114,38 @@ export class GitDiffService implements IGitDiffService {
 		try {
 			const buffer = await workspace.fs.readFile(resource);
 			const relativePath = path.relative(repository.rootUri.fsPath, resource.fsPath);
+			const content = buffer.toString();
 
 			// Header
 			patch.push(`diff --git a/${relativePath} b/${relativePath}`);
-
-			// Add original/modified file paths
+			// 100644 is standard file mode for new git files. Saves us from trying to check file permissions and handling
+			// UNIX vs Windows permission differences. Skipping calculating the SHA1 hashes as well since they are not strictly necessary
+			// to apply the patch.
+			patch.push('new file mode 100644');
 			patch.push('--- /dev/null', `+++ b/${relativePath}`);
 
-			// Add range header
-			patch.push(`@@ -0,0 +1,${buffer.length} @@`);
-
-			// Add content
-			patch.push(...buffer.toString().split('\n').map(line => `+${line}`));
+			// For non-empty files, add range header and content (empty files omit this)
+			if (content.length > 0) {
+				const lines = content.split('\n');
+				if (content.endsWith('\n')) {
+					// Prevent an extra empty line at the end
+					lines.pop();
+				}
+
+				// Range header and content
+				patch.push(`@@ -0,0 +1,${lines.length} @@`);
+				patch.push(...lines.map(line => `+${line}`));
+
+				// Git standard to add this comment if the file does not end with a newline
+				if (!content.endsWith('\n')) {
+					patch.push('\\ No newline at end of file');
+				}
+			}
 		} catch (err) {
 			console.error(err, `Failed to generate patch file for untracked file: ${resource.toString()}`);
 		}
 
-		return patch.join('\n');
+		// The patch itself should always end with a newline per git patch standards
+		return patch.join('\n') + '\n';
 	}
 }

src/extension/prompt/vscode-node/test/gitDiffService.spec.ts

@@ -0,0 +1,179 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import { MockInstance, afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import * as vscode from 'vscode';
+import { IGitExtensionService } from '../../../../platform/git/common/gitExtensionService';
+import { API, Change, Repository } from '../../../../platform/git/vscode/git';
+import { ITestingServicesAccessor } from '../../../../platform/test/node/services';
+import { IInstantiationService } from '../../../../util/vs/platform/instantiation/common/instantiation';
+import { Uri } from '../../../../vscodeTypes';
+import { createExtensionUnitTestingServices } from '../../../test/node/services';
+import { GitDiffService } from '../gitDiffService';
+
+describe('GitDiffService', () => {
+	let readFileSpy: MockInstance<typeof vscode.workspace.fs.readFile>;
+	let accessor: ITestingServicesAccessor;
+	let gitDiffService: GitDiffService;
+	let mockRepository: Partial<Repository>;
+
+	beforeEach(() => {
+		// Create mock workspace.fs.readFile if it doesn't exist
+		if (!vscode.workspace?.fs?.readFile) {
+			const workspaceWithFs = vscode as unknown as { workspace: typeof vscode.workspace };
+			workspaceWithFs.workspace = {
+				...vscode.workspace,
+				fs: {
+					...vscode.workspace?.fs,
+					readFile: vi.fn()
+				}
+			};
+		}
+
+		// Spy on workspace.fs.readFile
+		readFileSpy = vi.spyOn(vscode.workspace.fs, 'readFile').mockImplementation(() => Promise.resolve(new Uint8Array()));
+
+		mockRepository = {
+			rootUri: Uri.file('/repo'),
+			diffWith: vi.fn(),
+			diffIndexWithHEAD: vi.fn(),
+			diffWithHEAD: vi.fn()
+		};
+
+		const services = createExtensionUnitTestingServices();
+
+		const mockGitExtensionService = {
+			getExtensionApi: vi.fn().mockReturnValue({
+				getRepository: vi.fn().mockReturnValue(mockRepository),
+				openRepository: vi.fn(),
+				repositories: [mockRepository as Repository]
+			} as unknown as API)
+		} as unknown as IGitExtensionService;
+		services.set(IGitExtensionService, mockGitExtensionService);
+
+		accessor = services.createTestingAccessor();
+		gitDiffService = accessor.get(IInstantiationService).createInstance(GitDiffService);
+	});
+
+	afterEach(() => {
+		readFileSpy.mockRestore();
+	});
+
+	describe('_getUntrackedChangePatch', () => {
+		it('should generate correct patch for untracked file', async () => {
+			const fileUri = Uri.file('/repo/newfile.txt');
+			const fileContent = 'line1\nline2\n';
+
+			readFileSpy.mockResolvedValue(Buffer.from(fileContent));
+
+			const changes: Change[] = [{
+				uri: fileUri,
+				originalUri: fileUri,
+				renameUri: undefined,
+				status: 7 /* UNTRACKED */
+			}];
+
+			const diffs = await gitDiffService.getChangeDiffs(mockRepository as Repository, changes);
+
+			expect(diffs).toHaveLength(1);
+			const patch = diffs[0].diff;
+
+			// Verify standard git patch headers
+			expect(patch).toContain('diff --git a/newfile.txt b/newfile.txt');
+			expect(patch).toContain('new file mode 100644');
+			expect(patch).toContain('--- /dev/null');
+			expect(patch).toContain('+++ b/newfile.txt');
+
+			// Verify range header uses line count (2 lines), not byte length
+			expect(patch).toContain('@@ -0,0 +1,2 @@');
+
+			// Verify content
+			expect(patch).toContain('+line1');
+			expect(patch).toContain('+line2');
+
+			// Verify final newline
+			expect(patch.endsWith('\n')).toBe(true);
+
+			// Verify no "No newline at end of file" warning since file ends with \n
+			expect(patch).not.toContain('\\ No newline at end of file');
+		});
+
+		it('should handle file without trailing newline', async () => {
+			const fileUri = Uri.file('/repo/no-newline.txt');
+			const fileContent = 'line1'; // No trailing \n
+
+			readFileSpy.mockResolvedValue(Buffer.from(fileContent));
+
+			const changes: Change[] = [{
+				uri: fileUri,
+				originalUri: fileUri,
+				renameUri: undefined,
+				status: 7 /* UNTRACKED */
+			}];
+
+			const diffs = await gitDiffService.getChangeDiffs(mockRepository as Repository, changes);
+			const patch = diffs[0].diff;
+
+			expect(patch).toContain('@@ -0,0 +1,1 @@');
+			expect(patch).toContain('+line1');
+			expect(patch).toContain('\\ No newline at end of file');
+			expect(patch.endsWith('\n')).toBe(true);
+		});
+
+		it('should handle empty file', async () => {
+			const fileUri = Uri.file('/repo/empty.txt');
+			const fileContent = '';
+
+			// Mock readFile to return an empty buffer
+			readFileSpy.mockResolvedValue(Buffer.from(fileContent));
+
+			const changes: Change[] = [{
+				uri: fileUri,
+				originalUri: fileUri,
+				renameUri: undefined,
+				status: 7 /* UNTRACKED */
+			}];
+
+			const diffs = await gitDiffService.getChangeDiffs(mockRepository as Repository, changes);
+
+			// Empty file case: git omits range header and content for totally empty files
+			const patch = diffs[0].diff;
+			expect(patch).toContain('diff --git a/empty.txt b/empty.txt');
+			expect(patch).toContain('new file mode 100644');
+			expect(patch).toContain('--- /dev/null');
+			expect(patch).toContain('+++ b/empty.txt');
+			// No range header for empty files
+			expect(patch).not.toContain('@@');
+			// No content lines
+			expect(patch).not.toMatch(/^\+[^+]/m);
+		});
+
+		it('should handle file with single blank line', async () => {
+			const fileUri = Uri.file('/repo/blank-line.txt');
+			const fileContent = '\n'; // Single newline
+
+			readFileSpy.mockResolvedValue(Buffer.from(fileContent));
+
+			const changes: Change[] = [{
+				uri: fileUri,
+				originalUri: fileUri,
+				renameUri: undefined,
+				status: 7 /* UNTRACKED */
+			}];
+
+			const diffs = await gitDiffService.getChangeDiffs(mockRepository as Repository, changes);
+
+			// Single blank line: should have range header and one empty line addition
+			const patch = diffs[0].diff;
+			expect(patch).toContain('diff --git a/blank-line.txt b/blank-line.txt');
+			expect(patch).toContain('new file mode 100644');
+			expect(patch).toContain('--- /dev/null');
+			expect(patch).toContain('+++ b/blank-line.txt');
+			expect(patch).toContain('@@ -0,0 +1,1 @@');
+			expect(patch).toContain('+'); // One empty line
+			expect(patch.endsWith('\n')).toBe(true);
+		});
+	});
+});