Fixes #756 - use --first-parent for merge commits

Fixes issue with --numstat changes
Fixes issue with walking back too far

src/commands/diffLineWithPrevious.ts

@@ -62,7 +62,7 @@ export class DiffLineWithPreviousCommand extends ActiveEditorCommand {
             Logger.error(
                 ex,
                 'DiffLineWithPreviousCommand',
-                `getPreviousDiffUris(${gitUri.repoPath}, ${gitUri.fsPath}, ${gitUri.sha})`
+                `getPreviousLineDiffUris(${gitUri.repoPath}, ${gitUri.fsPath}, ${gitUri.sha})`
             );
             return Messages.showGenericErrorMessage('Unable to open compare');
         }

src/commands/diffWithPrevious.ts

@@ -1,7 +1,7 @@
 'use strict';
 import { commands, TextDocumentShowOptions, TextEditor, Uri } from 'vscode';
 import { Container } from '../container';
-import { GitCommit, GitService, GitUri } from '../git/gitService';
+import { GitCommit, GitLogCommit, GitService, GitUri } from '../git/gitService';
 import { Logger } from '../logger';
 import { Messages } from '../messages';
 import { ActiveEditorCommand, command, CommandContext, Commands, getCommandUri, openEditor } from './common';
@@ -57,7 +57,9 @@ export class DiffWithPreviousCommand extends ActiveEditorCommand {
                 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
+                args.inDiffEditor ? 1 : 0,
+                // If this is a merge commit, walk back using the first parent only
+                args.commit && (args.commit as GitLogCommit).isMerge
             );
 
             if (diffUris === undefined || diffUris.previous === undefined) {

src/git/git.ts

@@ -667,25 +667,27 @@ export class Git {
         ref: string | undefined,
         {
             filters,
-            format = GitLogParser.defaultFormat,
             maxCount,
+            firstParent = false,
             renames = true,
             reverse = false,
+            simple = false,
             startLine,
             endLine
         }: {
             filters?: GitLogDiffFilter[];
-            format?: string;
             maxCount?: number;
+            firstParent?: boolean;
             renames?: boolean;
             reverse?: boolean;
+            simple?: boolean;
             startLine?: number;
             endLine?: number;
         } = {}
     ) {
         const [file, root] = Git.splitPath(fileName, repoPath);
 
-        const params = ['log', `--format=${format}`];
+        const params = ['log', `--format=${simple ? GitLogParser.simpleFormat : GitLogParser.defaultFormat}`];
 
         if (maxCount && !reverse) {
             params.push(`-n${maxCount}`);
@@ -696,8 +698,17 @@ export class Git {
             params.push(`--diff-filter=${filters.join(emptyStr)}`);
         }
 
+        if (firstParent) {
+            params.push('--first-parent');
+        }
+
         if (startLine == null) {
-            params.push('--numstat', '--summary');
+            if (simple) {
+                params.push('--name-status');
+            }
+            else {
+                params.push('--numstat', '--summary');
+            }
         }
         else {
             // Don't include --name-status or -s because Git won't honor it

src/git/gitService.ts

@@ -1678,10 +1678,10 @@ export class GitService implements Disposable {
         const fileName = GitUri.relativeTo(uri, repoPath);
         let data = await Git.log__file(repoPath, fileName, ref, {
             filters: filters,
-            format: GitLogParser.simpleFormat,
             maxCount: skip + 1,
             // startLine: editorLine !== undefined ? editorLine + 1 : undefined,
-            reverse: true
+            reverse: true,
+            simple: true
         });
         if (data == null || data.length === 0) return undefined;
 
@@ -1690,9 +1690,9 @@ export class GitService implements Disposable {
         if (status === 'D') {
             data = await Git.log__file(repoPath, '.', nextRef, {
                 filters: ['R'],
-                format: GitLogParser.simpleFormat,
-                maxCount: 1
+                maxCount: 1,
                 // startLine: editorLine !== undefined ? editorLine + 1 : undefined
+                simple: true
             });
             if (data == null || data.length === 0) {
                 return GitUri.fromFile(file || fileName, repoPath, nextRef);
@@ -1714,7 +1714,8 @@ export class GitService implements Disposable {
         repoPath: string,
         uri: Uri,
         ref: string | undefined,
-        skip: number = 0
+        skip: number = 0,
+        firstParent: boolean = false
     ): Promise<{ current: GitUri; previous: GitUri | undefined } | undefined> {
         if (ref === GitService.deletedOrMissingSha) return undefined;
 
@@ -1744,14 +1745,14 @@ export class GitService implements Disposable {
                     return {
                         // Diff staged with HEAD (or prior if more skips)
                         current: GitUri.fromFile(fileName, repoPath, GitService.uncommittedStagedSha),
-                        previous: await this.getPreviousUri(repoPath, uri, ref, skip - 1)
+                        previous: await this.getPreviousUri(repoPath, uri, ref, skip - 1, undefined, firstParent)
                     };
                 }
                 else if (status.workingTreeStatus !== undefined) {
                     if (skip === 0) {
                         return {
                             current: GitUri.fromFile(fileName, repoPath, undefined),
-                            previous: await this.getPreviousUri(repoPath, uri, undefined, skip)
+                            previous: await this.getPreviousUri(repoPath, uri, undefined, skip, undefined, firstParent)
                         };
                     }
                 }
@@ -1765,25 +1766,25 @@ export class GitService implements Disposable {
             const current =
                 skip === 0
                     ? GitUri.fromFile(fileName, repoPath, ref)
-                    : (await this.getPreviousUri(repoPath, uri, undefined, skip - 1))!;
+                    : (await this.getPreviousUri(repoPath, uri, undefined, skip - 1, undefined, firstParent))!;
             if (current === undefined || current.sha === GitService.deletedOrMissingSha) return undefined;
 
             return {
                 current: current,
-                previous: await this.getPreviousUri(repoPath, uri, undefined, skip)
+                previous: await this.getPreviousUri(repoPath, uri, undefined, skip, undefined, firstParent)
             };
         }
 
         // If we are at a commit, diff commit with previous
         const current =
             skip === 0
                 ? GitUri.fromFile(fileName, repoPath, ref)
-                : (await this.getPreviousUri(repoPath, uri, ref, skip - 1))!;
+                : (await this.getPreviousUri(repoPath, uri, ref, skip - 1, undefined, firstParent))!;
         if (current === undefined || current.sha === GitService.deletedOrMissingSha) return undefined;
 
         return {
             current: current,
-            previous: await this.getPreviousUri(repoPath, uri, ref, skip)
+            previous: await this.getPreviousUri(repoPath, uri, ref, skip, undefined, firstParent)
         };
     }
 
@@ -1904,7 +1905,8 @@ export class GitService implements Disposable {
         uri: Uri,
         ref?: string,
         skip: number = 0,
-        editorLine?: number
+        editorLine?: number,
+        firstParent: boolean = false
     ): Promise<GitUri | undefined> {
         if (ref === GitService.deletedOrMissingSha) return undefined;
 
@@ -1914,17 +1916,14 @@ export class GitService implements Disposable {
             ref = undefined;
         }
 
-        if (ref !== undefined) {
-            skip++;
-        }
-
         const fileName = GitUri.relativeTo(uri, repoPath);
         // TODO: Add caching
         let data;
         try {
             data = await Git.log__file(repoPath, fileName, ref, {
-                format: GitLogParser.simpleFormat,
-                maxCount: skip + 1,
+                maxCount: skip + 2,
+                firstParent: firstParent,
+                simple: true,
                 startLine: editorLine !== undefined ? editorLine + 1 : undefined
             });
         }
@@ -1950,7 +1949,7 @@ export class GitService implements Disposable {
         }
         if (data == null || data.length === 0) return undefined;
 
-        const [previousRef, file] = GitLogParser.parseSimple(data, skip, editorLine !== undefined ? ref : undefined);
+        const [previousRef, file] = GitLogParser.parseSimple(data, skip, ref);
         // If the previous ref matches the ref we asked for assume we are at the end of the history
         if (ref !== undefined && ref === previousRef) return undefined;
 
@@ -2343,8 +2342,8 @@ export class GitService implements Disposable {
             // Now check if that commit had any renames
             data = await Git.log__file(repoPath, '.', ref, {
                 filters: ['R'],
-                format: GitLogParser.simpleFormat,
-                maxCount: 1
+                maxCount: 1,
+                simple: true
             });
             if (data == null || data.length === 0) {
                 return GitUri.resolveToUri(fileName, repoPath);

src/git/parsers/logParser.ts

@@ -460,13 +460,8 @@ export class GitLogParser {
     static parseSimple(
         data: string,
         skip: number,
-        lineRef?: string
+        skipRef?: string
     ): [string | undefined, string | undefined, GitFileStatus | undefined] {
-        // Don't skip 1 extra for line-based previous, as we will be skipping the line ref as needed
-        if (lineRef !== undefined) {
-            skip--;
-        }
-
         let ref;
         let diffFile;
         let diffRenamed;
@@ -479,16 +474,11 @@ export class GitLogParser {
             match = logFileSimpleRegex.exec(data);
             if (match == null) break;
 
+            if (match[1] === skipRef) continue;
             if (skip-- > 0) continue;
 
             [, ref, diffFile, diffRenamed, status, file, renamed] = match;
 
-            if (lineRef === ref) {
-                skip++;
-
-                continue;
-            }
-
             // Stops excessive memory usage -- https://bugs.chromium.org/p/v8/issues/detail?id=2869
             file = ` ${diffRenamed || diffFile || renamed || file}`.substr(1);
             // Stops excessive memory usage -- https://bugs.chromium.org/p/v8/issues/detail?id=2869
@@ -499,7 +489,11 @@ export class GitLogParser {
         logFileSimpleRegex.lastIndex = 0;
 
         // Stops excessive memory usage -- https://bugs.chromium.org/p/v8/issues/detail?id=2869
-        return [` ${ref}`.substr(1), file, status as GitFileStatus | undefined];
+        return [
+            ref == null || ref.length === 0 ? undefined : ` ${ref}`.substr(1),
+            file,
+            status as GitFileStatus | undefined
+        ];
     }
 
     @debug({ args: false })
@@ -536,6 +530,10 @@ export class GitLogParser {
         logFileSimpleRenamedFilesRegex.lastIndex = 0;
 
         // Stops excessive memory usage -- https://bugs.chromium.org/p/v8/issues/detail?id=2869
-        return [` ${ref}`.substr(1), file, status as GitFileStatus | undefined];
+        return [
+            ref == null || ref.length === 0 ? undefined : ` ${ref}`.substr(1),
+            file,
+            status as GitFileStatus | undefined
+        ];
     }
 }