-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix up for the GitDiffService #2116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
da0f2e0
fb198b4
6219a7c
5e01cbb
c01a1eb
502810e
4b50cad
ec5ac8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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} @@`); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug with the old code was here, where it was using the buffer.length and not the lines length for the range header. This would lead to patches that looked incorrect in size. |
||
| 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'); | ||
lszomoru marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| } 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'; | ||
lszomoru marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The product code here uses workspace.fs directly and not the file service, so I added this mock here so that my changes for this test were just in this test file as opposed to trying to mess with the top level vscode test shims. |
||
| 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', () => { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GitDiffService didn't have unit tests previously. But while looking into this some of the cases (newline at end, newline not at end, empty file) were a bit quirky, so added some quick tests in for those specific cases. |
||
| 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 | ||
IanMatthewHuff marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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); | ||
| }); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.