[git] Fixed the git diff parser issue.

Multiple empty lines were not handled gracefully in the commit subject.

Signed-off-by: Akos Kitta <kittaakos@gmail.com>

packages/git/src/browser/history/git-history-widget.ts

@@ -14,7 +14,7 @@ import URI from "@theia/core/lib/common/uri";
 import { GIT_HISTORY } from './git-history-contribution';
 import { LabelProvider } from '@theia/core/lib/browser/label-provider';
 import { GitRepositoryProvider } from '../git-repository-provider';
-import { GitFileStatus, Git, CommitFragment } from '../../common';
+import { GitFileStatus, Git, CommitWithChanges } from '../../common';
 import { GitBaseWidget } from "../git-base-widget";
 import { GitFileChangeNode } from "../git-widget";
 
@@ -51,13 +51,13 @@ export class GitHistoryWidget extends GitBaseWidget implements StatefulWidget {
         this.options = options;
         const repository = this.repositoryProvider.selectedRepository;
         if (repository) {
-            const commitFragments: CommitFragment[] = await this.git.log(repository, {
+            const changes: CommitWithChanges[] = await this.git.log(repository, {
                 uri: options.uri
             });
             const commits: GitCommitNode[] = [];
-            for (const commitFragment of commitFragments) {
+            for (const change of changes) {
                 const fileChangeNodes: GitFileChangeNode[] = [];
-                for (const fileChange of commitFragment.fileChanges) {
+                for (const fileChange of change.fileChanges) {
                     const fileChangeUri = new URI(fileChange.uri);
                     const [icon, label, description] = await Promise.all([
                         this.labelProvider.getIcon(fileChangeUri),
@@ -70,12 +70,12 @@ export class GitHistoryWidget extends GitBaseWidget implements StatefulWidget {
                     });
                 }
                 commits.push({
-                    authorName: commitFragment.author.name,
-                    authorDate: commitFragment.author.date,
-                    authorEmail: commitFragment.author.email,
-                    authorDateRelative: commitFragment.authorDateRelative,
-                    commitSha: commitFragment.sha,
-                    commitMessage: commitFragment.summary,
+                    authorName: change.author.name,
+                    authorDate: change.author.date,
+                    authorEmail: change.author.email,
+                    authorDateRelative: change.authorDateRelative,
+                    commitSha: change.sha,
+                    commitMessage: change.summary,
                     fileChangeNodes,
                     expanded: false
                 });

packages/git/src/common/git-model.ts

@@ -100,19 +100,6 @@ export interface GitFileChange {
 
 }
 
-export interface CommitFragment extends Commit {
-
-    /**
-     * The date when the commit was authored.
-     */
-    readonly authorDateRelative: string;
-
-    /**
-     * The number of file changes per commit.
-     */
-    readonly fileChanges: GitFileChange[];
-}
-
 /**
  * An object encapsulating the changes to a committed file.
  */
@@ -252,6 +239,22 @@ export interface Commit {
 
 }
 
+/**
+ * Representation of a Git commit, plus the changes that were performed in that particular commit.
+ */
+export interface CommitWithChanges extends Commit {
+
+    /**
+     * The date when the commit was authored.
+     */
+    readonly authorDateRelative: string;
+
+    /**
+     * The number of file changes per commit.
+     */
+    readonly fileChanges: GitFileChange[];
+}
+
 /**
  * A tuple of name, email, and a date for the author or commit info in a commit.
  */

packages/git/src/common/git.ts

@@ -7,7 +7,7 @@
 
 import { ChildProcess } from 'child_process';
 import { Disposable } from '@theia/core';
-import { Repository, WorkingDirectoryStatus, Branch, GitResult, GitError, GitFileStatus, GitFileChange, CommitFragment } from './git-model';
+import { Repository, WorkingDirectoryStatus, Branch, GitResult, GitError, GitFileStatus, GitFileChange, CommitWithChanges } from './git-model';
 
 /**
  * The WS endpoint path to the Git service.
@@ -443,6 +443,7 @@ export namespace Git {
          * Optional configuration for the `git log` command.
          */
         export interface Log extends Diff {
+
             /**
              * The name of the branch to run the `git log` command. If not specified, then the currently active branch will be used.
              */
@@ -631,7 +632,7 @@ export interface Git extends Disposable {
      * @param repository the repository where the log has to be calculated.
      * @param options optional configuration for further refining the `git log` command execution.
      */
-    log(repository: Repository, options?: Git.Options.Log): Promise<CommitFragment[]>
+    log(repository: Repository, options?: Git.Options.Log): Promise<CommitWithChanges[]>
 
 }
 

packages/git/src/node/dugite-git.ts

@@ -26,20 +26,41 @@ import { IStatusResult, IAheadBehind, AppFileStatus, WorkingDirectoryStatus as D
 import { Branch as DugiteBranch } from 'dugite-extra/lib/model/branch';
 import { Commit as DugiteCommit, CommitIdentity as DugiteCommitIdentity } from 'dugite-extra/lib/model/commit';
 import { ILogger } from '@theia/core';
-import { Git, GitUtils, Repository, WorkingDirectoryStatus, GitFileChange, GitFileStatus, Branch, Commit, CommitIdentity, GitResult, CommitFragment } from '../common';
+import { Git, GitUtils, Repository, WorkingDirectoryStatus, GitFileChange, GitFileStatus, Branch, Commit, CommitIdentity, GitResult, CommitWithChanges } from '../common';
 import { GitRepositoryManager } from './git-repository-manager';
 import { GitLocator } from './git-locator/git-locator-protocol';
 
+/**
+ * Parsing and converting raw Git output into Git model instances.
+ */
+@injectable()
+export abstract class OutputParser<T> {
+
+    /** This is the `NUL` delimiter. Equals wih `%x00`. */
+    static LINE_DELIMITER = '\0';
+
+    abstract parse(repositoryUri: string, raw: string, delimiter?: string): T[];
+    abstract parse(repositoryUri: string, items: string[]): T[];
+    abstract parse(repositoryUri: string, input: string | string[], delimiter?: string): T[];
+
+    protected toUri(repositoryUri: string, pathSegment: string): string {
+        return FileUri.create(Path.join(FileUri.fsPath(repositoryUri), pathSegment)).toString();
+    }
+
+    protected split(input: string | string[], delimiter: string): string[] {
+        return (Array.isArray(input) ? input : input.split(delimiter)).filter(item => item && item.length > 0);
+    }
+
+}
+
 /**
  * Status parser for converting raw Git `--name-status` output into file change objects.
  */
 @injectable()
-export class NameStatusParser {
+export class NameStatusParser extends OutputParser<GitFileChange> {
 
-    parse(repositoryUri: string, raw: string, delimiter?: string): GitFileChange[];
-    parse(repositoryUri: string, items: string[]): GitFileChange[];
-    parse(repositoryUri: string, input: string | string[], delimiter?: string): GitFileChange[] {
-        const items = Array.isArray(input) ? input : input.split(delimiter === undefined ? '\0' : delimiter);
+    parse(repositoryUri: string, input: string | string[], delimiter: string = OutputParser.LINE_DELIMITER): GitFileChange[] {
+        const items = this.split(input, delimiter);
         const changes: GitFileChange[] = [];
         let index = 0;
         while (index < items.length) {
@@ -66,8 +87,69 @@ export class NameStatusParser {
         return changes;
     }
 
-    protected toUri(repositoryUri: string, pathSegment: string): string {
-        return FileUri.create(Path.join(FileUri.fsPath(repositoryUri), pathSegment)).toString();
+}
+
+/**
+ * Built-in Git placeholders for tuning the `--format` option for `git diff` or `git log`.
+ */
+export enum CommitPlaceholders {
+    HASH = '%H',
+    AUTHOR_EMAIL = '%aE',
+    AUTHOR_NAME = '%aN',
+    AUTHOR_DATE = '%ad',
+    AUTHOR_RELATIVE_DATE = '%ar',
+    SUBJECT = '%s'
+}
+
+/**
+ * Parser for converting raw, Git commit details into `CommitWithChanges` instances.
+ */
+@injectable()
+export class CommitDetailsParser extends OutputParser<CommitWithChanges> {
+
+    static ENTRY_DELIMITER = '\x01';
+    static COMMIT_CHUNK_DELIMITER = '\x02';
+    static DEFAULT_PLACEHOLDERS = [
+        CommitPlaceholders.HASH,
+        CommitPlaceholders.AUTHOR_EMAIL,
+        CommitPlaceholders.AUTHOR_NAME,
+        CommitPlaceholders.AUTHOR_DATE,
+        CommitPlaceholders.AUTHOR_RELATIVE_DATE,
+        CommitPlaceholders.SUBJECT];
+
+    @inject(NameStatusParser)
+    protected readonly nameStatusParser: NameStatusParser;
+
+    parse(repositoryUri: string, input: string | string[], delimiter: string = CommitDetailsParser.COMMIT_CHUNK_DELIMITER): CommitWithChanges[] {
+        const chunks = this.split(input, delimiter);
+        const changes: CommitWithChanges[] = [];
+        for (const chunk of chunks) {
+            const [sha, email, name, timestamp, authorDateRelative, summary, rawChanges] = chunk.trim().split(CommitDetailsParser.ENTRY_DELIMITER);
+            const date = this.toDate(timestamp);
+            const fileChanges = this.nameStatusParser.parse(repositoryUri, (rawChanges || '').trim());
+            changes.push({
+                sha,
+                author: {
+                    date, email, name
+                },
+                authorDateRelative,
+                summary,
+                fileChanges
+            });
+        }
+        return changes;
+    }
+
+    getFormat(...placeholders: CommitPlaceholders[]): string {
+        return '%x02' + placeholders.join('%x01') + '%x01';
+    }
+
+    protected toDate(epochSeconds: string | undefined): Date {
+        const date = new Date(0);
+        if (epochSeconds) {
+            date.setUTCSeconds(Number.parseInt(epochSeconds));
+        }
+        return date;
     }
 
 }
@@ -90,6 +172,9 @@ export class DugiteGit implements Git {
     @inject(NameStatusParser)
     protected readonly nameStatusParser: NameStatusParser;
 
+    @inject(CommitDetailsParser)
+    protected readonly commitDetailsParser: CommitDetailsParser;
+
     dispose(): void {
         this.locator.dispose();
     }
@@ -272,65 +357,36 @@ export class DugiteGit implements Git {
     }
 
     async diff(repository: Repository, options?: Git.Options.Diff): Promise<GitFileChange[]> {
-        const args = [
-            'diff',
-            '--name-status',
-            '-C',
-            '-M',
-            '-z'
-        ];
+        const args = ['diff', '--name-status', '-C', '-M', '-z'];
         args.push(this.mapRange((options || {}).range));
         if (options && options.uri) {
-            args.push(...['--', Path.relative(FileUri.fsPath(repository.localUri), FileUri.fsPath(options.uri))]);
+            args.push(...['--', Path.relative(this.getFsPath(repository), this.getFsPath(options.uri))]);
         }
         const result = await this.exec(repository, args);
-        return this.nameStatusParser.parse(repository.localUri, result.stdout.trim().split('\0').filter(item => item && item.length > 0));
+        return this.nameStatusParser.parse(repository.localUri, result.stdout.trim());
     }
 
-    async log(repository: Repository, options?: Git.Options.Log): Promise<CommitFragment[]> {
-        const commits: CommitFragment[] = [];
+    async log(repository: Repository, options?: Git.Options.Log): Promise<CommitWithChanges[]> {
         // If remaining commits should be calculated by the backend, then run `git rev-list --count ${fromRevision | HEAD~fromRevision}`.
         // How to use `mailmap` to map authors: https://www.kernel.org/pub/software/scm/git/docs/git-shortlog.html.
         // (Probably, this would the responsibility of the `GitHub` extension.)
-        const args = [
-            'log'
-        ];
+        const args = ['log'];
         if (options && options.branch) {
             args.push(options.branch);
         }
-        if (options) {
-            const range = this.mapRange(options.range);
-            args.push(...[range, '-C', '-M', '-m']);
-        }
+        const range = this.mapRange((options || {}).range);
+        args.push(...[range, '-C', '-M', '-m']);
         const maxCount = options && options.maxCount ? options.maxCount : 0;
         if (Number.isInteger(maxCount) && maxCount > 0) {
             args.push(...['-n', `${maxCount}`]);
         }
-        args.push(...['--name-status', '--date=unix', `--format=%n%n%H%n%aE%n%aN%n%ad%n%ar%n%s`, '-z', '--']);
+        args.push(...['--name-status', '--date=unix', `--format=${this.commitDetailsParser.getFormat(...CommitDetailsParser.DEFAULT_PLACEHOLDERS)}`, '-z', '--']);
         if (options && options.uri) {
-            const file = Path.relative(repository.localUri, options.uri);
+            const file = Path.relative(this.getFsPath(repository), this.getFsPath(options.uri));
             args.push(...[file]);
         }
-        const blocks = (await this.exec(repository, args)).stdout.split('\n\n').slice(1);
-        blocks.map(block => block.split('\n')).forEach(lines => {
-            const sha = lines.shift() || '';
-            const email = lines.shift() || '';
-            const name = lines.shift() || '';
-            const date = this.toDate(lines.shift());
-            const authorDateRelative = lines.shift() || '';
-            const summary = this.toCommitMessage(lines.shift());
-            const fileChanges = this.nameStatusParser.parse(repository.localUri, (lines.shift() || '').split('\0').map(line => line.trim()).filter(line => line.length > 0));
-            commits.push({
-                sha,
-                author: {
-                    date, email, name
-                },
-                authorDateRelative,
-                summary,
-                fileChanges
-            });
-        });
-        return commits;
+        const result = await this.exec(repository, args);
+        return this.commitDetailsParser.parse(repository.localUri, result.stdout.trim().split(CommitDetailsParser.COMMIT_CHUNK_DELIMITER).filter(item => item && item.length > 0));
     }
 
     private getCommitish(options?: Git.Options.Show): string {
@@ -492,18 +548,6 @@ export class DugiteGit implements Git {
         return range;
     }
 
-    private toCommitMessage(raw: string | undefined): string {
-        return (raw || '').split('\0').shift() || '';
-    }
-
-    private toDate(epochSeconds: string | undefined): Date {
-        const date = new Date(0);
-        if (epochSeconds) {
-            date.setUTCSeconds(Number.parseInt(epochSeconds));
-        }
-        return date;
-    }
-
     private getFsPath(repository: Repository | string): string {
         const uri = typeof repository === 'string' ? repository : repository.localUri;
         return FileUri.fsPath(uri);

packages/git/src/node/git-backend-module.ts

@@ -9,7 +9,7 @@ import * as cluster from 'cluster';
 import { ContainerModule, Container, interfaces } from 'inversify';
 import { Git, GitPath } from '../common/git';
 import { GitWatcherPath, GitWatcherClient, GitWatcherServer } from '../common/git-watcher';
-import { DugiteGit, NameStatusParser } from './dugite-git';
+import { DugiteGit, OutputParser, NameStatusParser, CommitDetailsParser } from './dugite-git';
 import { DugiteGitWatcherServer } from './dugite-git-watcher';
 import { ConnectionHandler, JsonRpcConnectionHandler, ILogger } from "@theia/core/lib/common";
 import { GitRepositoryManager } from './git-repository-manager';
@@ -51,7 +51,9 @@ export function bindGit(bind: interfaces.Bind, bindingOptions: GitBindingOptions
         bind(GitLocator).to(GitLocatorClient);
     }
     bind(DugiteGit).toSelf();
+    bind(OutputParser).toSelf();
     bind(NameStatusParser).toSelf();
+    bind(CommitDetailsParser).toSelf();
     bind(Git).toDynamicValue(ctx => ctx.container.get(DugiteGit));
 }