From fa3088054757f8ef2fd8cea4d78fed91188b20ad Mon Sep 17 00:00:00 2001
From: Eric Amodio <eamodio@gmail.com>
Date: Mon, 20 Nov 2023 15:54:01 -0500
Subject: [PATCH] Fixes #3023 unifies file & dirty blame Adds don't prompt
again to error prompts Adds specific error handling for invalid revision vs
invalid file Adds better fallback handling of user entered custom args
---
CHANGELOG.md | 1 +
package.json | 14 +++++-
src/config.ts | 4 +-
src/env/node/git/git.ts | 96 ++++++++++++++++--------------------
src/env/node/git/localGitProvider.ts | 26 ++++++----
src/git/errors.ts | 22 +++++++--
src/messages.ts | 20 ++++++++
7 files changed, 112 insertions(+), 71 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d861372..47539dd 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p
### Fixed
+- Fixes [#3023](https://github.com/gitkraken/vscode-gitlens/issues/3023) - "Unable to show blame. Invalid or missing blame.ignoreRevsFile" with valid ignore revs file
- Fixes [#3018](https://github.com/gitkraken/vscode-gitlens/issues/3018) - Line blame overlay is broken when commit message contains a `)`
- Fixes [#2625](https://github.com/gitkraken/vscode-gitlens/issues/2625) - full issue ref has escape characters that break hover links
- Fixes stuck busy state of the _Commit Details_ Explain AI panel after canceling a request
diff --git a/package.json b/package.json
index 39adc98..abe9b4d 100644
--- a/package.json
+++ b/package.json
@@ -4031,7 +4031,9 @@
"suppressRebaseSwitchToTextWarning": false,
"suppressIntegrationDisconnectedTooManyFailedRequestsWarning": false,
"suppressIntegrationRequestFailed500Warning": false,
- "suppressIntegrationRequestTimedOutWarning": false
+ "suppressIntegrationRequestTimedOutWarning": false,
+ "suppressBlameInvalidIgnoreRevsFileWarning": false,
+ "suppressBlameInvalidIgnoreRevsFileBadRevisionWarning": false
},
"properties": {
"suppressCommitHasNoPreviousCommitWarning": {
@@ -4103,6 +4105,16 @@
"type": "boolean",
"default": false,
"description": "Integration Request Timed Out Warning"
+ },
+ "suppressBlameInvalidIgnoreRevsFileWarning": {
+ "type": "boolean",
+ "default": false,
+ "description": "Invalid Blame IgnoreRevs File Warning"
+ },
+ "suppressBlameInvalidIgnoreRevsFileBadRevisionWarning": {
+ "type": "boolean",
+ "default": false,
+ "description": "Invalid Revision in Blame IgnoreRevs File Warning"
}
},
"additionalProperties": false,
diff --git a/src/config.ts b/src/config.ts
index 3199a48..d2eca10 100644
--- a/src/config.ts
+++ b/src/config.ts
@@ -537,7 +537,9 @@ export type SuppressedMessages =
| 'suppressRebaseSwitchToTextWarning'
| 'suppressIntegrationDisconnectedTooManyFailedRequestsWarning'
| 'suppressIntegrationRequestFailed500Warning'
- | 'suppressIntegrationRequestTimedOutWarning';
+ | 'suppressIntegrationRequestTimedOutWarning'
+ | 'suppressBlameInvalidIgnoreRevsFileWarning'
+ | 'suppressBlameInvalidIgnoreRevsFileBadRevisionWarning';
export interface ViewsCommonConfig {
readonly defaultItemLimit: number;
diff --git a/src/env/node/git/git.ts b/src/env/node/git/git.ts
index 8378d14..71b7808 100644
--- a/src/env/node/git/git.ts
+++ b/src/env/node/git/git.ts
@@ -10,6 +10,7 @@ import { GlyphChars } from '../../../constants';
import type { GitCommandOptions, GitSpawnOptions } from '../../../git/commandOptions';
import { GitErrorHandling } from '../../../git/commandOptions';
import {
+ BlameIgnoreRevsFileBadRevisionError,
BlameIgnoreRevsFileError,
CherryPickError,
CherryPickErrorReason,
@@ -35,7 +36,7 @@ import { parseGitTagsDefaultFormat } from '../../../git/parsers/tagParser';
import { splitAt } from '../../../system/array';
import { configuration } from '../../../system/configuration';
import { log } from '../../../system/decorators/log';
-import { join } from '../../../system/iterable';
+import { count, join } from '../../../system/iterable';
import { Logger } from '../../../system/logger';
import { slowCallWarningThreshold } from '../../../system/logger.constants';
import { getLogScope } from '../../../system/logger.scope';
@@ -70,12 +71,13 @@ const textDecoder = new TextDecoder('utf8');
const rootSha = '4b825dc642cb6eb9a060e54bf8d69288fbee4904';
export const GitErrors = {
- badIgnoreRevsFile: /could not open object name list: (.*)\s/i,
badRevision: /bad revision '(.*?)'/i,
cantLockRef: /cannot lock ref|unable to update local ref/i,
changesWouldBeOverwritten: /Your local changes to the following files would be overwritten/i,
commitChangesFirst: /Please, commit your changes before you can/i,
conflict: /^CONFLICT \([^)]+\): \b/m,
+ invalidObjectName: /invalid object name: (.*)\s/i,
+ invalidObjectNameList: /could not open object name list: (.*)\s/i,
noFastForward: /\(non-fast-forward\)/i,
noMergeBase: /no merge base/i,
noRemoteRepositorySpecified: /No remote repository specified\./i,
@@ -386,20 +388,37 @@ export class Git {
async blame(
repoPath: string | undefined,
fileName: string,
- ref?: string,
- options: { args?: string[] | null; ignoreWhitespace?: boolean; startLine?: number; endLine?: number } = {},
+ options?: ({ ref: string | undefined; contents?: never } | { contents: string; ref?: never }) & {
+ args?: string[] | null;
+ correlationKey?: string;
+ ignoreWhitespace?: boolean;
+ startLine?: number;
+ endLine?: number;
+ },
) {
const [file, root] = splitPath(fileName, repoPath, true);
const params = ['blame', '--root', '--incremental'];
- if (options.ignoreWhitespace) {
+ if (options?.ignoreWhitespace) {
params.push('-w');
}
- if (options.startLine != null && options.endLine != null) {
+ if (options?.startLine != null && options.endLine != null) {
params.push(`-L ${options.startLine},${options.endLine}`);
}
- if (options.args != null) {
+ if (options?.args != null) {
+ // See if the args contains a value like: `--ignore-revs-file <file>` or `--ignore-revs-file=<file>` to account for user error
+ // If so split it up into two args
+ const argIndex = options.args.findIndex(
+ arg => arg !== '--ignore-revs-file' && arg.startsWith('--ignore-revs-file'),
+ );
+ if (argIndex !== -1) {
+ const match = /^--ignore-revs-file\s*=?\s*(.*)$/.exec(options.args[argIndex]);
+ if (match != null) {
+ options.args.splice(argIndex, 1, '--ignore-revs-file', match[1]);
+ }
+ }
+
params.push(...options.args);
}
@@ -444,63 +463,26 @@ export class Git {
}
let stdin;
- if (ref) {
- if (isUncommittedStaged(ref)) {
+ if (options?.contents != null) {
+ // Pipe the blame contents to stdin
+ params.push('--contents', '-');
+
+ stdin = options.contents;
+ } else if (options?.ref) {
+ if (isUncommittedStaged(options.ref)) {
// Pipe the blame contents to stdin
params.push('--contents', '-');
// Get the file contents for the staged version using `:`
stdin = await this.show<string>(repoPath, fileName, ':');
} else {
- params.push(ref);
+ params.push(options.ref);
}
}
try {
- const blame = await this.git<string>({ cwd: root, stdin: stdin }, ...params, '--', file);
- return blame;
- } catch (ex) {
- // Since `-c blame.ignoreRevsFile=` doesn't seem to work (unlike as the docs suggest), try to detect the error and throw a more helpful one
- const match = GitErrors.badIgnoreRevsFile.exec(ex.message);
- if (match != null) {
- throw new BlameIgnoreRevsFileError(match[1], ex);
- }
- throw ex;
- }
- }
-
- async blame__contents(
- repoPath: string | undefined,
- fileName: string,
- contents: string,
- options: {
- args?: string[] | null;
- correlationKey?: string;
- ignoreWhitespace?: boolean;
- startLine?: number;
- endLine?: number;
- } = {},
- ) {
- const [file, root] = splitPath(fileName, repoPath, true);
-
- const params = ['blame', '--root', '--incremental'];
-
- if (options.ignoreWhitespace) {
- params.push('-w');
- }
- if (options.startLine != null && options.endLine != null) {
- params.push(`-L ${options.startLine},${options.endLine}`);
- }
- if (options.args != null) {
- params.push(...options.args);
- }
-
- // Pipe the blame contents to stdin
- params.push('--contents', '-');
-
- try {
const blame = await this.git<string>(
- { cwd: root, stdin: contents, correlationKey: options.correlationKey },
+ { cwd: root, stdin: stdin, correlationKey: options?.correlationKey },
...params,
'--',
file,
@@ -508,10 +490,16 @@ export class Git {
return blame;
} catch (ex) {
// Since `-c blame.ignoreRevsFile=` doesn't seem to work (unlike as the docs suggest), try to detect the error and throw a more helpful one
- const match = GitErrors.badIgnoreRevsFile.exec(ex.message);
+ let match = GitErrors.invalidObjectNameList.exec(ex.message);
if (match != null) {
throw new BlameIgnoreRevsFileError(match[1], ex);
}
+
+ match = GitErrors.invalidObjectName.exec(ex.message);
+ if (match != null) {
+ throw new BlameIgnoreRevsFileBadRevisionError(match[1], ex);
+ }
+
throw ex;
}
}
diff --git a/src/env/node/git/localGitProvider.ts b/src/env/node/git/localGitProvider.ts
index 0c5c618..bafd22f 100644
--- a/src/env/node/git/localGitProvider.ts
+++ b/src/env/node/git/localGitProvider.ts
@@ -17,6 +17,7 @@ import { emojify } from '../../../emojis';
import { Features } from '../../../features';
import { GitErrorHandling } from '../../../git/commandOptions';
import {
+ BlameIgnoreRevsFileBadRevisionError,
BlameIgnoreRevsFileError,
CherryPickError,
CherryPickErrorReason,
@@ -150,6 +151,7 @@ import { getRemoteProviderMatcher, loadRemoteProviders } from '../../../git/remo
import type { GitSearch, GitSearchResultData, GitSearchResults, SearchQuery } from '../../../git/search';
import { getGitArgsFromSearchQuery, getSearchQueryComparisonKey } from '../../../git/search';
import {
+ showBlameInvalidIgnoreRevsFileWarningMessage,
showGenericErrorMessage,
showGitDisabledErrorMessage,
showGitInvalidConfigErrorMessage,
@@ -1748,7 +1750,8 @@ export class LocalGitProvider implements GitProvider, Disposable {
const [relativePath, root] = paths;
try {
- const data = await this.git.blame(root, relativePath, uri.sha, {
+ const data = await this.git.blame(root, relativePath, {
+ ref: uri.sha,
args: configuration.get('advanced.blame.customArguments'),
ignoreWhitespace: configuration.get('blame.ignoreWhitespace'),
});
@@ -1769,8 +1772,8 @@ export class LocalGitProvider implements GitProvider, Disposable {
document.state.setBlame(key, value);
document.setBlameFailure(ex);
- if (ex instanceof BlameIgnoreRevsFileError) {
- void window.showErrorMessage(ex.friendlyMessage);
+ if (ex instanceof BlameIgnoreRevsFileError || ex instanceof BlameIgnoreRevsFileBadRevisionError) {
+ void showBlameInvalidIgnoreRevsFileWarningMessage(ex);
}
return emptyPromise as Promise<GitBlame>;
@@ -1831,7 +1834,8 @@ export class LocalGitProvider implements GitProvider, Disposable {
const [relativePath, root] = paths;
try {
- const data = await this.git.blame__contents(root, relativePath, contents, {
+ const data = await this.git.blame(root, relativePath, {
+ contents: contents,
args: configuration.get('advanced.blame.customArguments'),
correlationKey: `:${key}`,
ignoreWhitespace: configuration.get('blame.ignoreWhitespace'),
@@ -1853,8 +1857,8 @@ export class LocalGitProvider implements GitProvider, Disposable {
document.state.setBlame(key, value);
document.setBlameFailure(ex);
- if (ex instanceof BlameIgnoreRevsFileError) {
- void window.showErrorMessage(ex.friendlyMessage);
+ if (ex instanceof BlameIgnoreRevsFileError || ex instanceof BlameIgnoreRevsFileBadRevisionError) {
+ void showBlameInvalidIgnoreRevsFileWarningMessage(ex);
}
return emptyPromise as Promise<GitBlame>;
@@ -1903,7 +1907,8 @@ export class LocalGitProvider implements GitProvider, Disposable {
const [relativePath, root] = splitPath(uri, uri.repoPath);
try {
- const data = await this.git.blame(root, relativePath, uri.sha, {
+ const data = await this.git.blame(root, relativePath, {
+ ref: uri.sha,
args: configuration.get('advanced.blame.customArguments'),
ignoreWhitespace: configuration.get('blame.ignoreWhitespace'),
startLine: lineToBlame,
@@ -1919,8 +1924,8 @@ export class LocalGitProvider implements GitProvider, Disposable {
};
} catch (ex) {
Logger.error(ex, scope);
- if (ex instanceof BlameIgnoreRevsFileError) {
- void window.showErrorMessage(ex.friendlyMessage);
+ if (ex instanceof BlameIgnoreRevsFileError || ex instanceof BlameIgnoreRevsFileBadRevisionError) {
+ void showBlameInvalidIgnoreRevsFileWarningMessage(ex);
}
return undefined;
@@ -1959,7 +1964,8 @@ export class LocalGitProvider implements GitProvider, Disposable {
const [relativePath, root] = splitPath(uri, uri.repoPath);
try {
- const data = await this.git.blame__contents(root, relativePath, contents, {
+ const data = await this.git.blame(root, relativePath, {
+ contents: contents,
args: configuration.get('advanced.blame.customArguments'),
ignoreWhitespace: configuration.get('blame.ignoreWhitespace'),
startLine: lineToBlame,
diff --git a/src/git/errors.ts b/src/git/errors.ts
index dc01aa1..ba9696b 100644
--- a/src/git/errors.ts
+++ b/src/git/errors.ts
@@ -11,19 +11,31 @@ export class BlameIgnoreRevsFileError extends Error {
return ex instanceof BlameIgnoreRevsFileError;
}
- readonly friendlyMessage: string;
-
constructor(
- filename: string,
+ public readonly fileName: string,
public readonly original?: Error,
) {
- super(`Invalid blame.ignoreRevsFile: '${filename}'`);
+ super(`Invalid blame.ignoreRevsFile: '${fileName}'`);
- this.friendlyMessage = `Unable to show blame. Invalid or missing blame.ignoreRevsFile (${filename}) specified in your Git config.`;
Error.captureStackTrace?.(this, BlameIgnoreRevsFileError);
}
}
+export class BlameIgnoreRevsFileBadRevisionError extends Error {
+ static is(ex: unknown): ex is BlameIgnoreRevsFileBadRevisionError {
+ return ex instanceof BlameIgnoreRevsFileBadRevisionError;
+ }
+
+ constructor(
+ public readonly revision: string,
+ public readonly original?: Error,
+ ) {
+ super(`Invalid revision in blame.ignoreRevsFile: '${revision}'`);
+
+ Error.captureStackTrace?.(this, BlameIgnoreRevsFileBadRevisionError);
+ }
+}
+
export const enum StashApplyErrorReason {
WorkingChanges = 1,
}
diff --git a/src/messages.ts b/src/messages.ts
index 70b6140..38736fb 100644
--- a/src/messages.ts
+++ b/src/messages.ts
@@ -2,11 +2,31 @@ import type { MessageItem } from 'vscode';
import { ConfigurationTarget, window } from 'vscode';
import type { SuppressedMessages } from './config';
import { Commands } from './constants';
+import type { BlameIgnoreRevsFileError } from './git/errors';
+import { BlameIgnoreRevsFileBadRevisionError } from './git/errors';
import type { GitCommit } from './git/models/commit';
import { executeCommand } from './system/command';
import { configuration } from './system/configuration';
import { Logger } from './system/logger';
+export function showBlameInvalidIgnoreRevsFileWarningMessage(
+ ex: BlameIgnoreRevsFileError | BlameIgnoreRevsFileBadRevisionError,
+): Promise<MessageItem | undefined> {
+ if (ex instanceof BlameIgnoreRevsFileBadRevisionError) {
+ return showMessage(
+ 'error',
+ `Unable to show blame. Invalid revision (${ex.revision}) specified in the blame.ignoreRevsFile in your Git config.`,
+ 'suppressBlameInvalidIgnoreRevsFileBadRevisionWarning',
+ );
+ }
+
+ return showMessage(
+ 'error',
+ `Unable to show blame. Invalid or missing blame.ignoreRevsFile (${ex.fileName}) specified in your Git config.`,
+ 'suppressBlameInvalidIgnoreRevsFileWarning',
+ );
+}
+
export function showCommitHasNoPreviousCommitWarningMessage(commit?: GitCommit): Promise<MessageItem | undefined> {
if (commit == null) {
return showMessage('info', 'There is no previous commit.', 'suppressCommitHasNoPreviousCommitWarning');