Reworks diff with prev (line) for better accuracy

Adds getDiffWithPreviousForFile & getPreviousRevisionUri

src/commands/diffLineWithPrevious.ts

@@ -1,12 +1,11 @@
 'use strict';
-import { commands, Range, TextDocumentShowOptions, TextEditor, Uri } from 'vscode';
+import { commands, TextDocumentShowOptions, TextEditor, Uri } from 'vscode';
 import { Container } from '../container';
 import { GitCommit, GitService, GitUri } from '../git/gitService';
 import { Logger } from '../logger';
 import { Messages } from '../messages';
 import { ActiveEditorCommand, command, Commands, getCommandUri } from './common';
 import { DiffWithCommandArgs } from './diffWith';
-import { Iterables } from '../system';
 
 export interface DiffLineWithPreviousCommandArgs {
     commit?: GitCommit;
@@ -25,73 +24,101 @@ export class DiffLineWithPreviousCommand extends ActiveEditorCommand {
         uri = getCommandUri(uri, editor);
         if (uri == null) return undefined;
 
-        const gitUri = await GitUri.fromUri(uri);
-
         args = { ...args };
         if (args.line === undefined) {
             args.line = editor == null ? 0 : editor.selection.active.line;
         }
 
-        if (args.commit === undefined || GitService.isUncommitted(args.commit.sha)) {
-            if (args.line < 0) return undefined;
+        const gitUri = args.commit !== undefined ? GitUri.fromCommit(args.commit) : await GitUri.fromUri(uri);
 
-            try {
-                if (!GitService.isStagedUncommitted(gitUri.sha)) {
-                    const blame =
-                        editor && editor.document && editor.document.isDirty
-                            ? await Container.git.getBlameForLineContents(gitUri, args.line, editor.document.getText())
-                            : await Container.git.getBlameForLine(gitUri, args.line);
-                    if (blame === undefined) {
-                        return Messages.showFileNotUnderSourceControlWarningMessage('Unable to open compare');
-                    }
+        if (gitUri.sha === undefined || GitService.isUncommitted(gitUri.sha)) {
+            const blame =
+                editor && editor.document.isDirty
+                    ? await Container.git.getBlameForLineContents(gitUri, args.line, editor.document.getText())
+                    : await Container.git.getBlameForLine(gitUri, args.line);
+            if (blame === undefined) {
+                return Messages.showFileNotUnderSourceControlWarningMessage('Unable to open compare');
+            }
 
-                    // If the line is uncommitted, change the previous commit
-                    if (blame.commit.isUncommitted) {
-                        const status = await Container.git.getStatusForFile(gitUri.repoPath!, gitUri.fsPath);
-                        if (status !== undefined && status.indexStatus !== undefined) {
-                            args.commit = blame.commit.with({
-                                sha: GitService.stagedUncommittedSha
-                            });
-                        }
-                    }
-                }
+            // Since there could be a change in the line number, update it
+            args.line = blame.line.originalLine - 1;
+
+            // If the line is uncommitted, change the previous commit
+            if (blame.commit.isUncommitted) {
+                try {
+                    const previous = await Container.git.getPreviousRevisionUri(
+                        gitUri.repoPath!,
+                        gitUri,
+                        gitUri.sha,
+                        0,
+                        args.line
+                    );
 
-                if (args.commit === undefined) {
-                    const log = await Container.git.getLogForFile(gitUri.repoPath, gitUri.fsPath, {
-                        maxCount: 2,
-                        range: new Range(args.line, 0, args.line, 0),
-                        ref:
-                            gitUri.sha === undefined || GitService.isStagedUncommitted(gitUri.sha)
-                                ? undefined
-                                : `${gitUri.sha}^`,
-                        renames: true
-                    });
-                    if (log === undefined) {
-                        return Messages.showFileNotUnderSourceControlWarningMessage('Unable to open compare');
+                    if (previous === undefined) {
+                        return Messages.showCommitHasNoPreviousCommitWarningMessage();
                     }
 
-                    args.commit = (gitUri.sha && log.commits.get(gitUri.sha)) || Iterables.first(log.commits.values());
+                    const diffArgs: DiffWithCommandArgs = {
+                        repoPath: gitUri.repoPath!,
+                        lhs: {
+                            sha: previous.sha || '',
+                            uri: previous.documentUri()
+                        },
+                        rhs: {
+                            sha: gitUri.sha || '',
+                            uri: gitUri.documentUri()
+                        },
+                        line: args.line,
+                        showOptions: args.showOptions
+                    };
+                    return commands.executeCommand(Commands.DiffWith, diffArgs);
+                }
+                catch (ex) {
+                    Logger.error(
+                        ex,
+                        'DiffLineWithPreviousCommand',
+                        `getPreviousRevisionUri(${gitUri.repoPath}, ${gitUri.fsPath}, ${gitUri.sha})`
+                    );
+                    return Messages.showGenericErrorMessage('Unable to open compare');
                 }
-            }
-            catch (ex) {
-                Logger.error(ex, 'DiffLineWithPreviousCommand', `getLogForFile(${args.line})`);
-                return Messages.showGenericErrorMessage('Unable to open compare');
             }
         }
 
-        const diffArgs: DiffWithCommandArgs = {
-            repoPath: args.commit.repoPath,
-            lhs: {
-                sha: args.commit.sha,
-                uri: args.commit.uri
-            },
-            rhs: {
-                sha: gitUri.sha || '',
-                uri: gitUri
-            },
-            line: args.line,
-            showOptions: args.showOptions
-        };
-        return commands.executeCommand(Commands.DiffWith, diffArgs);
+        try {
+            const diffWith = await Container.git.getDiffWithPreviousForFile(
+                gitUri.repoPath!,
+                gitUri,
+                gitUri.sha,
+                0,
+                args.line
+            );
+
+            if (diffWith === undefined || diffWith.previous === undefined) {
+                return Messages.showCommitHasNoPreviousCommitWarningMessage();
+            }
+
+            const diffArgs: DiffWithCommandArgs = {
+                repoPath: diffWith.current.repoPath,
+                lhs: {
+                    sha: diffWith.previous.sha || '',
+                    uri: diffWith.previous.documentUri()
+                },
+                rhs: {
+                    sha: diffWith.current.sha || '',
+                    uri: diffWith.current.documentUri()
+                },
+                line: args.line,
+                showOptions: args.showOptions
+            };
+            return commands.executeCommand(Commands.DiffWith, diffArgs);
+        }
+        catch (ex) {
+            Logger.error(
+                ex,
+                'DiffLineWithPreviousCommand',
+                `getDiffWithPreviousForFile(${gitUri.repoPath}, ${gitUri.fsPath}, ${gitUri.sha})`
+            );
+            return Messages.showGenericErrorMessage('Unable to open compare');
+        }
     }
 }

src/commands/diffWithPrevious.ts

@@ -1,13 +1,11 @@
 'use strict';
 import { commands, TextDocumentShowOptions, TextEditor, Uri } from 'vscode';
 import { Container } from '../container';
-import { GitCommit, GitService, GitUri } from '../git/gitService';
+import { GitCommit, GitUri } from '../git/gitService';
 import { Logger } from '../logger';
 import { Messages } from '../messages';
-import { Iterables } from '../system';
 import { ActiveEditorCommand, command, CommandContext, Commands, getCommandUri } from './common';
 import { DiffWithCommandArgs } from './diffWith';
-import { DiffWithWorkingCommandArgs } from './diffWithWorking';
 import { UriComparer } from '../comparers';
 
 export interface DiffWithPreviousCommandArgs {
@@ -52,137 +50,42 @@ export class DiffWithPreviousCommand extends ActiveEditorCommand {
             args.line = editor == null ? 0 : editor.selection.active.line;
         }
 
-        if (args.commit === undefined || !args.commit.isFile) {
-            const gitUri = await GitUri.fromUri(uri);
-
-            try {
-                let sha = args.commit === undefined ? gitUri.sha : args.commit.sha;
-                if (sha === GitService.deletedOrMissingSha) {
-                    return Messages.showCommitHasNoPreviousCommitWarningMessage();
-                }
-
-                // If we are a fake "staged" sha, remove it
-                let isStagedUncommitted = false;
-                if (GitService.isStagedUncommitted(sha!)) {
-                    gitUri.sha = sha = undefined;
-                    isStagedUncommitted = true;
-                }
-
-                // If we are in a diff editor, assume we are on the right side, and need to move back 2 revisions
-                const originalSha = sha;
-                if (args.inDiffEditor && sha !== undefined) {
-                    sha = `${sha}^`;
-                }
-
-                args.commit = undefined;
-
-                let log = await Container.git.getLogForFile(gitUri.repoPath, gitUri.fsPath, {
-                    maxCount: 2,
-                    ref: sha,
-                    renames: true
-                });
-
-                if (log !== undefined) {
-                    args.commit = (sha && log.commits.get(sha)) || Iterables.first(log.commits.values());
-                }
-                else {
-                    // Only kick out if we aren't looking for the previous sha -- since renames won't return a log above
-                    if (sha === undefined || !sha.endsWith('^')) {
-                        return Messages.showFileNotUnderSourceControlWarningMessage('Unable to open compare');
-                    }
-
-                    // Check for renames
-                    log = await Container.git.getLogForFile(gitUri.repoPath, gitUri.fsPath, {
-                        maxCount: 3,
-                        ref: originalSha,
-                        renames: true
-                    });
-
-                    if (log === undefined) {
-                        return Messages.showFileNotUnderSourceControlWarningMessage('Unable to open compare');
-                    }
-
-                    args.commit =
-                        Iterables.next(Iterables.skip(log.commits.values(), 1)) ||
-                        Iterables.first(log.commits.values());
-
-                    if (args.commit.sha === originalSha) {
-                        return Messages.showCommitHasNoPreviousCommitWarningMessage();
-                    }
-                }
-
-                // If the sha is missing (i.e. working tree), check the file status
-                // If file is uncommitted, then treat it as a DiffWithWorking
-                if (gitUri.sha === undefined) {
-                    const status = await Container.git.getStatusForFile(gitUri.repoPath!, gitUri.fsPath);
-                    if (status !== undefined) {
-                        if (isStagedUncommitted) {
-                            const diffArgs: DiffWithCommandArgs = {
-                                repoPath: args.commit.repoPath,
-                                lhs: {
-                                    sha: args.inDiffEditor
-                                        ? args.commit.previousSha || GitService.deletedOrMissingSha
-                                        : args.commit.sha,
-                                    uri: args.inDiffEditor ? args.commit.previousUri : args.commit.uri
-                                },
-                                rhs: {
-                                    sha: args.inDiffEditor ? args.commit.sha : GitService.stagedUncommittedSha,
-                                    uri: args.commit.uri
-                                },
-                                line: args.line,
-                                showOptions: args.showOptions
-                            };
-                            return commands.executeCommand(Commands.DiffWith, diffArgs);
-                        }
-
-                        // Check if the file is staged
-                        if (status.indexStatus !== undefined) {
-                            const diffArgs: DiffWithCommandArgs = {
-                                repoPath: args.commit.repoPath,
-                                lhs: {
-                                    sha: args.inDiffEditor ? args.commit.sha : GitService.stagedUncommittedSha,
-                                    uri: args.commit.uri
-                                },
-                                rhs: {
-                                    sha: args.inDiffEditor ? GitService.stagedUncommittedSha : '',
-                                    uri: args.commit.uri
-                                },
-                                line: args.line,
-                                showOptions: args.showOptions
-                            };
-
-                            return commands.executeCommand(Commands.DiffWith, diffArgs);
-                        }
-
-                        if (!args.inDiffEditor) {
-                            const commandArgs: DiffWithWorkingCommandArgs = {
-                                commit: args.commit,
-                                showOptions: args.showOptions
-                            };
-                            return commands.executeCommand(Commands.DiffWithWorking, uri, commandArgs);
-                        }
-                    }
-                }
-            }
-            catch (ex) {
-                Logger.error(ex, 'DiffWithPreviousCommand', `getLogForFile(${gitUri.repoPath}, ${gitUri.fsPath})`);
-                return Messages.showGenericErrorMessage('Unable to open compare');
+        const gitUri = args.commit !== undefined ? GitUri.fromCommit(args.commit) : await GitUri.fromUri(uri);
+        try {
+            const diffWith = await Container.git.getDiffWithPreviousForFile(
+                gitUri.repoPath!,
+                gitUri,
+                gitUri.sha,
+                // If we are in a diff editor, assume we are on the right side, and need to skip back 1 more revisions
+                args.inDiffEditor ? 1 : 0
+            );
+
+            if (diffWith === undefined || diffWith.previous === undefined) {
+                return Messages.showCommitHasNoPreviousCommitWarningMessage();
             }
-        }
 
-        const diffArgs: DiffWithCommandArgs = {
-            repoPath: args.commit.repoPath,
-            lhs: {
-                sha: args.commit.previousSha !== undefined ? args.commit.previousSha : GitService.deletedOrMissingSha,
-                uri: args.commit.previousUri
-            },
-            rhs: {
-                sha: args.commit.sha,
-                uri: args.commit.uri
-            },
-            line: args.line,
-            showOptions: args.showOptions
-        };
-        return commands.executeCommand(Commands.DiffWith, diffArgs);
+            const diffArgs: DiffWithCommandArgs = {
+                repoPath: diffWith.current.repoPath,
+                lhs: {
+                    sha: diffWith.previous.sha || '',
+                    uri: diffWith.previous.documentUri()
+                },
+                rhs: {
+                    sha: diffWith.current.sha || '',
+                    uri: diffWith.current.documentUri()
+                },
+                line: args.line,
+                showOptions: args.showOptions
+            };
+            return commands.executeCommand(Commands.DiffWith, diffArgs);
+        }
+        catch (ex) {
+            Logger.error(
+                ex,
+                'DiffWithPreviousCommand',
+                `getDiffWithPreviousForFile(${gitUri.repoPath}, ${gitUri.fsPath}, ${gitUri.sha})`
+            );
+            return Messages.showGenericErrorMessage('Unable to open compare');
+        }
     }
 }

src/git/git.ts

@@ -44,6 +44,8 @@ const logFormat = [
     `${lb}f${rb}`
 ].join('%n');
 
+const logSimpleFormat = `${lb}r${rb}${sp}%H`;
+
 const defaultLogParams = ['log', '--name-status', `--format=${logFormat}`];
 
 const stashFormat = [
@@ -682,6 +684,25 @@ export class Git {
         return git<string>({ cwd: root }, ...params, '--', file);
     }
 
+    static log_file_simple(repoPath: string, fileName: string, ref?: string, count: number = 2, line?: number) {
+        const [file, root] = Git.splitPath(fileName, repoPath);
+
+        const params = ['log', `--format=${logSimpleFormat}`, `-n${count}`, '--follow'];
+        if (ref && !Git.isStagedUncommitted(ref)) {
+            params.push(ref);
+        }
+
+        if (line != null) {
+            // Don't include --name-status or -s because Git won't honor it
+            params.push(/*'-s',*/ `-L ${line},${line}:${file}`);
+        }
+        else {
+            params.push('--name-status');
+        }
+
+        return git<string>({ cwd: root }, ...params, '--', file);
+    }
+
     static async log_recent(repoPath: string, fileName: string) {
         const data = await git<string>(
             { cwd: repoPath, errors: GitErrorHandling.Ignore },

src/git/gitService.ts

@@ -2149,6 +2149,91 @@ export class GitService implements Disposable {
     }
 
     @log()
+    async getDiffWithPreviousForFile(
+        repoPath: string,
+        uri: Uri,
+        ref?: string,
+        skip: number = 0,
+        editorLine?: number
+    ): Promise<{ current: GitUri; previous: GitUri | undefined } | undefined> {
+        if (ref === GitService.deletedOrMissingSha) return undefined;
+
+        const fileName = GitUri.getRelativePath(uri, repoPath);
+
+        // If the ref is missing (i.e. working tree), check the file status to see if there is anything staged
+        if (ref === undefined && editorLine === undefined) {
+            const status = await Container.git.getStatusForFile(repoPath, fileName);
+            if (status !== undefined) {
+                // If the file is staged, diff with the staged version
+                if (status.indexStatus !== undefined) {
+                    if (skip === 0) {
+                        return {
+                            current: GitUri.fromFile(fileName, repoPath, ref),
+                            previous: GitUri.fromFile(fileName, repoPath, GitService.stagedUncommittedSha)
+                        };
+                    }
+
+                    return {
+                        current: GitUri.fromFile(fileName, repoPath, GitService.stagedUncommittedSha),
+                        previous: await this.getPreviousRevisionUri(repoPath, uri, ref, skip - 1, editorLine)
+                    };
+                }
+            }
+        }
+        else if (GitService.isStagedUncommitted(ref)) {
+            const current =
+                skip === 0
+                    ? GitUri.fromFile(fileName, repoPath, ref)
+                    : (await this.getPreviousRevisionUri(repoPath, uri, undefined, skip - 1, editorLine))!;
+            if (current.sha === GitService.deletedOrMissingSha) return undefined;
+
+            return {
+                current: current,
+                previous: await this.getPreviousRevisionUri(repoPath, uri, undefined, skip, editorLine)
+            };
+        }
+
+        const current =
+            skip === 0
+                ? GitUri.fromFile(fileName, repoPath, ref)
+                : (await this.getPreviousRevisionUri(repoPath, uri, ref, skip - 1, editorLine))!;
+        if (current.sha === GitService.deletedOrMissingSha) return undefined;
+
+        return {
+            current: current,
+            previous: await this.getPreviousRevisionUri(repoPath, uri, ref, skip, editorLine)
+        };
+    }
+
+    @log()
+    async getPreviousRevisionUri(
+        repoPath: string,
+        uri: Uri,
+        ref?: string,
+        skip: number = 0,
+        editorLine?: number
+    ): Promise<GitUri | undefined> {
+        if (ref === GitService.deletedOrMissingSha) return undefined;
+
+        if (ref !== undefined) {
+            skip++;
+        }
+
+        const fileName = GitUri.getRelativePath(uri, repoPath);
+        const data = await Git.log_file_simple(
+            repoPath,
+            fileName,
+            ref,
+            skip + 1,
+            editorLine !== undefined ? editorLine + 1 : undefined
+        );
+        if (data == null || data.length === 0) throw new Error('File has no history');
+
+        const [previousRef, file] = GitLogParser.parseSimple(data, skip);
+        return GitUri.fromFile(file || fileName, repoPath, previousRef || GitService.deletedOrMissingSha);
+    }
+
+    @log()
     async resolveReference(repoPath: string, ref: string, uri?: Uri) {
         const resolved = Git.isSha(ref) || !Git.isShaLike(ref) || ref.endsWith('^3');
         if (uri == null) return resolved ? ref : (await Git.revparse(repoPath, ref)) || ref;

src/git/gitUri.ts

@@ -311,7 +311,7 @@ export class GitUri extends ((Uri as any) as UriEx) {
             : `${paths.basename(fileName)}${suffix}${separator}${directory}`;
     }
 
-    static getRelativePath(fileNameOrUri: string | Uri, relativeTo?: string, repoPath?: string): string {
+    static getRelativePath(fileNameOrUri: string | Uri, repoPath?: string, relativeTo?: string): string {
         let fileName: string;
         if (fileNameOrUri instanceof Uri) {
             if (fileNameOrUri instanceof GitUri) return fileNameOrUri.getRelativePath(relativeTo);
@@ -322,7 +322,7 @@ export class GitUri extends ((Uri as any) as UriEx) {
             fileName = fileNameOrUri;
         }
 
-        let relativePath = repoPath ? paths.relative(repoPath, fileName) : fileName;
+        let relativePath = repoPath && paths.isAbsolute(fileName) ? paths.relative(repoPath, fileName) : fileName;
         if (relativeTo !== undefined) {
             relativePath = paths.relative(relativeTo, relativePath);
         }

src/git/models/file.ts

@@ -41,11 +41,11 @@ export namespace GitFile {
     export function getOriginalRelativePath(file: GitFile, relativeTo?: string): string {
         if (file.originalFileName == null || file.originalFileName.length === 0) return '';
 
-        return GitUri.getRelativePath(file.originalFileName, relativeTo);
+        return GitUri.getRelativePath(file.originalFileName, undefined, relativeTo);
     }
 
     export function getRelativePath(file: GitFile, relativeTo?: string): string {
-        return GitUri.getRelativePath(file.fileName, relativeTo);
+        return GitUri.getRelativePath(file.fileName, undefined, relativeTo);
     }
 
     const statusIconsMap = {

src/git/parsers/logParser.ts

@@ -27,6 +27,8 @@ interface LogEntry {
 }
 
 const diffRegex = /diff --git a\/(.*) b\/(.*)/;
+const logFileSimpleRegex = /^<r> (.*)\s*(?:(?:diff --git a\/(.*) b\/(.*))|(?:\S+\t([^\t\n]+)(?:\t(.+))?))/gm;
+
 const emptyEntry: LogEntry = {};
 
 export class GitLogParser {
@@ -348,4 +350,24 @@ export class GitLogParser {
             }
         }
     }
+
+    static parseSimple(data: string, skip: number): [string | undefined, string | undefined] {
+        let match;
+        let ref;
+        let file;
+        do {
+            match = logFileSimpleRegex.exec(data);
+            if (match == null) break;
+
+            if (skip-- > 0) continue;
+
+            ref = ` ${match[1]}`.substr(1);
+            file = ` ${match[3] || match[2] || match[5] || match[4]}`.substr(1);
+        } while (skip >= 0);
+
+        // Ensure the regex state is reset
+        logFileSimpleRegex.lastIndex = 0;
+
+        return [ref, file];
+    }
 }