Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 80 additions & 33 deletions packages/desktop/src/features/git/StagingManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ export interface StageLinesResult {
}

export class GitStagingManager {
// Keep in sync with UI working-tree diff context (DiffManager.WORKING_DIFF_CONTEXT_LINES).
private readonly UI_WORKING_DIFF_CONTEXT_LINES = 3;

constructor(
private gitExecutor: GitExecutor,
private statusManager: GitStatusManager
Expand All @@ -93,7 +96,7 @@ export class GitStagingManager {
console.debug('[GitStagingManager] Got diff, length:', fullDiff.length);

// Check for binary files
if (fullDiff.includes('Binary files differ')) {
if (this.isBinaryDiffOutput(fullDiff)) {
return {
success: false,
error: 'Cannot stage individual lines of binary files',
Expand Down Expand Up @@ -151,23 +154,18 @@ export class GitStagingManager {
async stageHunk(options: StageHunkOptions): Promise<StageLinesResult> {
try {
const scope = options.isStaging ? 'unstaged' : 'staged';
// Keep hunk operations aligned with the UI diff (unified=0), so the hunk header matches.
const fullDiff = await this.getFileDiff(options.worktreePath, options.filePath, scope, options.sessionId, 0);

if (fullDiff.includes('Binary files differ')) {
return { success: false, error: 'Cannot stage hunks of binary files' };
}

const hunks = this.parseDiffIntoHunks(fullDiff);
if (hunks.length === 0) {
return { success: false, error: 'No changes found in diff' };
}

const normalizedHeader = options.hunkHeader.trim();
const targetHunk = hunks.find((h) => h.header.trim() === normalizedHeader);
if (!targetHunk) {
return { success: false, error: 'Target hunk not found in diff' };
const resolved = await this.resolveTargetHunkByHeader({
worktreePath: options.worktreePath,
filePath: options.filePath,
scope,
sessionId: options.sessionId,
hunkHeader: options.hunkHeader,
binaryError: 'Cannot stage hunks of binary files',
});
if ('error' in resolved) {
return { success: false, error: resolved.error };
}
const targetHunk = resolved.hunk;

const patch = this.generateHunkPatch(targetHunk, options.filePath);
return await this.applyPatch(options.worktreePath, patch, options.isStaging, options.sessionId, {
Expand All @@ -190,23 +188,18 @@ export class GitStagingManager {
async restoreHunk(options: RestoreHunkOptions): Promise<StageLinesResult> {
try {
const fullDiffScope = options.scope === 'staged' ? 'staged' : 'unstaged';
// Keep hunk operations aligned with the UI diff (unified=0), so the hunk header matches.
const fullDiff = await this.getFileDiff(options.worktreePath, options.filePath, fullDiffScope, options.sessionId, 0);

if (fullDiff.includes('Binary files differ')) {
return { success: false, error: 'Cannot restore hunks of binary files' };
}

const hunks = this.parseDiffIntoHunks(fullDiff);
if (hunks.length === 0) {
return { success: false, error: 'No changes found in diff' };
}

const normalizedHeader = options.hunkHeader.trim();
const targetHunk = hunks.find((h) => h.header.trim() === normalizedHeader);
if (!targetHunk) {
return { success: false, error: 'Target hunk not found in diff' };
const resolved = await this.resolveTargetHunkByHeader({
worktreePath: options.worktreePath,
filePath: options.filePath,
scope: fullDiffScope,
sessionId: options.sessionId,
hunkHeader: options.hunkHeader,
binaryError: 'Cannot restore hunks of binary files',
});
if ('error' in resolved) {
return { success: false, error: resolved.error };
}
const targetHunk = resolved.hunk;

const patch = this.generateHunkPatch(targetHunk, options.filePath);

Expand Down Expand Up @@ -331,6 +324,51 @@ export class GitStagingManager {
}
}

private async resolveTargetHunkByHeader(options: {
worktreePath: string;
filePath: string;
scope: 'staged' | 'unstaged';
sessionId: string;
hunkHeader: string;
binaryError: string;
}): Promise<{ hunk: Hunk } | { error: string }> {
const normalizedHeader = options.hunkHeader.trim();
if (!normalizedHeader) {
return { error: 'Invalid hunk header' };
}

let sawAnyHunks = false;
const unifiedCandidates = [0, this.UI_WORKING_DIFF_CONTEXT_LINES];
const attemptedUnified = new Set<number>();

for (const unified of unifiedCandidates) {
if (attemptedUnified.has(unified)) continue;
attemptedUnified.add(unified);

const fullDiff = await this.getFileDiff(
options.worktreePath,
options.filePath,
options.scope,
options.sessionId,
unified
);

if (this.isBinaryDiffOutput(fullDiff)) {
return { error: options.binaryError };
}

const hunks = this.parseDiffIntoHunks(fullDiff);
if (hunks.length === 0) continue;
sawAnyHunks = true;

const target = hunks.find((h) => h.header.trim() === normalizedHeader);
if (target) return { hunk: target };
}

if (!sawAnyHunks) return { error: 'No changes found in diff' };
return { error: 'Target hunk not found in diff' };
}

private async getFileDiff(
worktreePath: string,
filePath: string,
Expand Down Expand Up @@ -360,6 +398,15 @@ export class GitStagingManager {
return result.stdout;
}

private isBinaryDiffOutput(diffText: string): boolean {
for (const rawLine of diffText.split('\n')) {
const line = rawLine.trimEnd();
if (line === 'GIT binary patch') return true;
if (line.startsWith('Binary files ') && line.endsWith(' differ')) return true;
}
return false;
}

/**
* Parse diff text into hunks with line number tracking
*/
Expand Down
174 changes: 174 additions & 0 deletions packages/desktop/src/features/git/__tests__/StagingManager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,113 @@ Binary files differ`;
argv: expect.arrayContaining(['git', 'apply', '-R']),
}));
});

it('should fallback to unified=3 hunk lookup when unified=0 header does not match', async () => {
const diffUnified0 = `diff --git a/test.txt b/test.txt
--- a/test.txt
+++ b/test.txt
@@ -2 +2 @@
-old line
+new line`;

const diffUnified3 = `diff --git a/test.txt b/test.txt
--- a/test.txt
+++ b/test.txt
@@ -1,3 +1,3 @@
context-before
-old line
+new line
context-after`;

vi.mocked(mockGitExecutor.run)
.mockResolvedValueOnce({
// First lookup: --unified=0
exitCode: 0,
stdout: diffUnified0,
stderr: '',
commandDisplay: 'git diff --unified=0',
} as any)
.mockResolvedValueOnce({
// Fallback lookup: --unified=3
exitCode: 0,
stdout: diffUnified3,
stderr: '',
commandDisplay: 'git diff --unified=3',
} as any)
.mockResolvedValueOnce({
// Apply selected hunk patch
exitCode: 0,
stdout: '',
stderr: '',
commandDisplay: 'git apply --cached',
} as any);

const result = await stagingManager.stageHunk({
worktreePath: '/tmp/project',
sessionId: 'test-session',
filePath: 'test.txt',
isStaging: true,
hunkHeader: '@@ -1,3 +1,3 @@',
});

expect(result.success).toBe(true);
expect(mockGitExecutor.run).toHaveBeenCalledTimes(3);

expect(mockGitExecutor.run).toHaveBeenNthCalledWith(1, expect.objectContaining({
argv: expect.arrayContaining(['git', 'diff', '--unified=0']),
}));

expect(mockGitExecutor.run).toHaveBeenNthCalledWith(2, expect.objectContaining({
argv: expect.arrayContaining(['git', 'diff', '--unified=3']),
}));

const [, patchText] = (fs.promises.writeFile as any).mock.calls[0] as [string, string, string];
expect(patchText).toContain('@@ -1,3 +1,3 @@');
expect(patchText).toContain('context-before');
expect(patchText).toContain('context-after');
});

it('should not treat source lines containing "Binary files differ" as binary diff metadata', async () => {
const mockDiff = `diff --git a/test.txt b/test.txt
--- a/test.txt
+++ b/test.txt
@@ -1,1 +1,1 @@
-if (fullDiff.includes('Binary files differ')) {
+if (this.isBinaryDiffOutput(fullDiff)) {`;

vi.mocked(mockGitExecutor.run)
.mockResolvedValueOnce({
// First call: get diff
exitCode: 0,
stdout: mockDiff,
stderr: '',
commandDisplay: 'git diff',
} as any)
.mockResolvedValueOnce({
// Second call: apply patch
exitCode: 0,
stdout: '',
stderr: '',
commandDisplay: 'git apply',
} as any);

const result = await stagingManager.stageHunk({
worktreePath: '/tmp/project',
sessionId: 'test-session',
filePath: 'test.txt',
isStaging: true,
hunkHeader: '@@ -1,1 +1,1 @@',
});

expect(result.success).toBe(true);
expect(mockGitExecutor.run).toHaveBeenCalledTimes(2);
expect(mockGitExecutor.run).toHaveBeenNthCalledWith(1, expect.objectContaining({
argv: expect.arrayContaining(['git', 'diff', '--unified=0']),
}));
expect(mockGitExecutor.run).toHaveBeenNthCalledWith(2, expect.objectContaining({
argv: expect.arrayContaining(['git', 'apply', '--cached']),
}));
});
});

describe('restoreHunk', () => {
Expand Down Expand Up @@ -788,6 +895,73 @@ Binary files differ`;
argv: expect.arrayContaining(['git', 'apply', '-R']),
}));
});

it('should fallback to unified=3 hunk lookup when restoring unstaged hunk', async () => {
const diffUnified0 = `diff --git a/test.txt b/test.txt
--- a/test.txt
+++ b/test.txt
@@ -2 +2 @@
-old line
+new line`;

const diffUnified3 = `diff --git a/test.txt b/test.txt
--- a/test.txt
+++ b/test.txt
@@ -1,3 +1,3 @@
context-before
-old line
+new line
context-after`;

vi.mocked(mockGitExecutor.run)
.mockResolvedValueOnce({
// First lookup: --unified=0
exitCode: 0,
stdout: diffUnified0,
stderr: '',
commandDisplay: 'git diff --unified=0',
} as any)
.mockResolvedValueOnce({
// Fallback lookup: --unified=3
exitCode: 0,
stdout: diffUnified3,
stderr: '',
commandDisplay: 'git diff --unified=3',
} as any)
.mockResolvedValueOnce({
// Restore in worktree
exitCode: 0,
stdout: '',
stderr: '',
commandDisplay: 'git apply -R',
} as any);

const result = await stagingManager.restoreHunk({
worktreePath: '/tmp/project',
sessionId: 'test-session',
filePath: 'test.txt',
scope: 'unstaged',
hunkHeader: '@@ -1,3 +1,3 @@',
});

expect(result.success).toBe(true);
expect(mockGitExecutor.run).toHaveBeenCalledTimes(3);

expect(mockGitExecutor.run).toHaveBeenNthCalledWith(1, expect.objectContaining({
argv: expect.arrayContaining(['git', 'diff', '--unified=0']),
}));

expect(mockGitExecutor.run).toHaveBeenNthCalledWith(2, expect.objectContaining({
argv: expect.arrayContaining(['git', 'diff', '--unified=3']),
}));

expect(mockGitExecutor.run).toHaveBeenNthCalledWith(3, expect.objectContaining({
argv: expect.arrayContaining(['git', 'apply', '-R']),
}));
expect(mockGitExecutor.run).toHaveBeenNthCalledWith(3, expect.not.objectContaining({
argv: expect.arrayContaining(['--cached']),
}));
});
});

describe('changeAllStage', () => {
Expand Down
Loading
Loading