Skip to content

Commit c18e20a

Browse files
mhutchiemhutchie
committed
mhutchie#426 Improved error logging when launching external diff tools.
1 parent a80945b commit c18e20a

File tree

2 files changed

+38
-13
lines changed

2 files changed

+38
-13
lines changed

‎src/dataSource.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { AskpassEnvironment, AskpassManager } from './askpass/askpassManager';
77
import { getConfig } from './config';
88
import { Logger } from './logger';
99
import { CommitOrdering, DateType, DeepWriteable, ErrorInfo, GitCommit, GitCommitDetails, GitCommitStash, GitConfigLocation, GitFileChange, GitFileStatus, GitPushBranchMode, GitRepoConfig, GitRepoConfigBranches, GitResetMode, GitSignatureStatus, GitStash, MergeActionOn, RebaseActionOn, SquashMessageFormat, TagType, Writeable } from './types';
10-
import { GitExecutable, UNABLE_TO_FIND_GIT_MSG, UNCOMMITTED, abbrevCommit, constructIncompatibleGitVersionMessage, getPathFromStr, getPathFromUri, isGitAtLeastVersion, openGitTerminal, pathWithTrailingSlash, realpath, resolveSpawnOutput } from './utils';
10+
import { GitExecutable, UNABLE_TO_FIND_GIT_MSG, UNCOMMITTED, abbrevCommit, constructIncompatibleGitVersionMessage, getPathFromStr, getPathFromUri, isGitAtLeastVersion, openGitTerminal, pathWithTrailingSlash, realpath, resolveSpawnOutput, showErrorMessage } from './utils';
1111
import { Disposable } from './utils/disposable';
1212
import { Event } from './utils/event';
1313

@@ -1236,9 +1236,14 @@ export class DataSource extends Disposable {
12361236
}
12371237
}
12381238
if (isGui) {
1239-
this.logger.log('External difftool for ' + args[args.length - 1] + ' is being opened');
1240-
this.runGitCommand(args, repo).then(() => {
1241-
this.logger.log('External difftool for ' + args[args.length - 1] + ' has been closed');
1239+
this.logger.log('External diff tool is being opened (' + args[args.length - 1] + ')');
1240+
this.runGitCommand(args, repo).then((errorInfo) => {
1241+
this.logger.log('External diff tool has exited (' + args[args.length - 1] + ')');
1242+
if (errorInfo !== null) {
1243+
const errorMessage = errorInfo.replace(EOL_REGEX, ' ');
1244+
this.logger.logError(errorMessage);
1245+
showErrorMessage(errorMessage);
1246+
}
12421247
});
12431248
} else {
12441249
openGitTerminal(repo, this.gitExecutable.path, args.join(' '), 'Open External Directory Diff');

‎tests/dataSource.test.ts‎

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ const workspaceConfiguration = vscode.mocks.workspaceConfiguration;
2222
let onDidChangeConfiguration: EventEmitter<ConfigurationChangeEvent>;
2323
let onDidChangeGitExecutable: EventEmitter<utils.GitExecutable>;
2424
let logger: Logger;
25-
let spyOnSpawn: jest.SpyInstance, spyOnLog: jest.SpyInstance;
25+
let spyOnSpawn: jest.SpyInstance, spyOnLog: jest.SpyInstance, spyOnLogError: jest.SpyInstance;
2626

2727
beforeAll(() => {
2828
onDidChangeConfiguration = new EventEmitter<ConfigurationChangeEvent>();
@@ -31,6 +31,7 @@ beforeAll(() => {
3131
jest.spyOn(path, 'normalize').mockImplementation((p) => p);
3232
spyOnSpawn = jest.spyOn(cp, 'spawn');
3333
spyOnLog = jest.spyOn(logger, 'log');
34+
spyOnLogError = jest.spyOn(logger, 'logError');
3435
});
3536

3637
afterAll(() => {
@@ -6138,8 +6139,8 @@ describe('DataSource', () => {
61386139
// Assert
61396140
expect(result).toBe(null);
61406141
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['difftool', '--dir-diff', '-g', '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b^..1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b'], expect.objectContaining({ cwd: '/path/to/repo' }));
6141-
expect(spyOnLog).toHaveBeenCalledWith('External difftool for 1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b^..1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b is being opened');
6142-
await waitForExpect(() => expect(spyOnLog).toHaveBeenCalledWith('External difftool for 1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b^..1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b has been closed'));
6142+
expect(spyOnLog).toHaveBeenCalledWith('External diff tool is being opened (1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b^..1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b)');
6143+
await waitForExpect(() => expect(spyOnLog).toHaveBeenCalledWith('External diff tool has exited (1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b^..1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b)'));
61436144
});
61446145

61456146
it('Should launch a gui directory diff (between two commits)', async () => {
@@ -6152,8 +6153,8 @@ describe('DataSource', () => {
61526153
// Assert
61536154
expect(result).toBe(null);
61546155
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['difftool', '--dir-diff', '-g', '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b..2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c'], expect.objectContaining({ cwd: '/path/to/repo' }));
6155-
expect(spyOnLog).toHaveBeenCalledWith('External difftool for 1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b..2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c is being opened');
6156-
await waitForExpect(() => expect(spyOnLog).toHaveBeenCalledWith('External difftool for 1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b..2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c has been closed'));
6156+
expect(spyOnLog).toHaveBeenCalledWith('External diff tool is being opened (1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b..2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c)');
6157+
await waitForExpect(() => expect(spyOnLog).toHaveBeenCalledWith('External diff tool has exited (1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b..2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c)'));
61576158
});
61586159

61596160
it('Should launch a gui directory diff (for uncommitted changes)', async () => {
@@ -6166,8 +6167,8 @@ describe('DataSource', () => {
61666167
// Assert
61676168
expect(result).toBe(null);
61686169
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['difftool', '--dir-diff', '-g', 'HEAD'], expect.objectContaining({ cwd: '/path/to/repo' }));
6169-
expect(spyOnLog).toHaveBeenCalledWith('External difftool for HEAD is being opened');
6170-
await waitForExpect(() => expect(spyOnLog).toHaveBeenCalledWith('External difftool for HEAD has been closed'));
6170+
expect(spyOnLog).toHaveBeenCalledWith('External diff tool is being opened (HEAD)');
6171+
await waitForExpect(() => expect(spyOnLog).toHaveBeenCalledWith('External diff tool has exited (HEAD)'));
61716172
});
61726173

61736174
it('Should launch a gui directory diff (between a commit and the uncommitted changes)', async () => {
@@ -6180,8 +6181,8 @@ describe('DataSource', () => {
61806181
// Assert
61816182
expect(result).toBe(null);
61826183
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['difftool', '--dir-diff', '-g', '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b'], expect.objectContaining({ cwd: '/path/to/repo' }));
6183-
expect(spyOnLog).toHaveBeenCalledWith('External difftool for 1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b is being opened');
6184-
await waitForExpect(() => expect(spyOnLog).toHaveBeenCalledWith('External difftool for 1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b has been closed'));
6184+
expect(spyOnLog).toHaveBeenCalledWith('External diff tool is being opened (1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b)');
6185+
await waitForExpect(() => expect(spyOnLog).toHaveBeenCalledWith('External diff tool has exited (1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b)'));
61856186
});
61866187

61876188
it('Should launch a directory diff in a terminal (between two commits)', async () => {
@@ -6208,6 +6209,25 @@ describe('DataSource', () => {
62086209
// Assert
62096210
expect(result).toBe('Unable to find a Git executable. Either: Set the Visual Studio Code Setting "git.path" to the path and filename of an existing Git executable, or install Git and restart Visual Studio Code.');
62106211
});
6212+
6213+
it('Should display the error message when the diff tool doesn\'t exit successfully', async () => {
6214+
// Setup
6215+
mockGitThrowingErrorOnce('line1\nline2\nline3');
6216+
vscode.window.showErrorMessage.mockResolvedValueOnce(null);
6217+
6218+
// Run
6219+
const result = await dataSource.openExternalDirDiff('/path/to/repo', '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', utils.UNCOMMITTED, true);
6220+
6221+
// Assert
6222+
expect(result).toBe(null);
6223+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['difftool', '--dir-diff', '-g', '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b'], expect.objectContaining({ cwd: '/path/to/repo' }));
6224+
expect(spyOnLog).toHaveBeenCalledWith('External diff tool is being opened (1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b)');
6225+
await waitForExpect(() => {
6226+
expect(spyOnLog).toHaveBeenCalledWith('External diff tool has exited (1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b)');
6227+
expect(spyOnLogError).toBeCalledWith('line1 line2 line3');
6228+
expect(vscode.window.showErrorMessage).toBeCalledWith('line1 line2 line3');
6229+
});
6230+
});
62116231
});
62126232

62136233
describe('onDidChangeConfiguration', () => {

0 commit comments

Comments
 (0)