Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix incorrect author display You(#457) #460

Merged
merged 4 commits into from Jul 28, 2018
Merged

Fix incorrect author display You(#457) #460

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
784285e
Fix incorrect displayed username `You`(#457)
prism4time Jul 24, 2018
02e5ce7
Fix incorrect cache refer for previous commit
prism4time Jul 24, 2018
787ad5c
Make userName and email combined together
prism4time Jul 25, 2018
2a5260e
Add check for multiple user info and incomplete info
prism4time Jul 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/git/git.ts
View file
Open in desktop
Expand Up @@ -395,6 +395,16 @@ export class Git {
}
}

static async config_getRegex(pattern: string, repoPath?: string) {
try {
const data = await gitCommandCore({ cwd: repoPath || '' }, 'config', '--get-regex', pattern);
return data.trim();
}
catch {
return undefined;
}
}

static diff(repoPath: string, fileName: string, sha1?: string, sha2?: string, options: { encoding?: string } = {}) {
const params = ['-c', 'color.diff=false', 'diff', '--diff-filter=M', '-M', '--no-ext-diff', '--minimal'];
if (sha1) {
Expand Down
14 changes: 8 additions & 6 deletions src/git/parsers/blameParser.ts
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@ export class GitBlameParser {
data: string,
repoPath: string | undefined,
fileName: string,
currentUser: string | undefined
currentUser: { name?: string, email?: string } | undefined
): GitBlame | undefined {
if (!data) return undefined;

Expand Down Expand Up @@ -69,9 +69,6 @@ export class GitBlameParser {
.slice(1)
.join(' ')
.trim();
if (currentUser !== undefined && currentUser === entry.author) {
entry.author = 'You';
}
}
break;

Expand Down Expand Up @@ -125,7 +122,7 @@ export class GitBlameParser {
}
first = false;

GitBlameParser.parseEntry(entry, repoPath, relativeFileName, commits, authors, lines);
GitBlameParser.parseEntry(entry, repoPath, relativeFileName, commits, authors, lines, currentUser);

entry = undefined;
break;
Expand Down Expand Up @@ -160,11 +157,16 @@ export class GitBlameParser {
fileName: string | undefined,
commits: Map<string, GitBlameCommit>,
authors: Map<string, GitAuthor>,
lines: GitCommitLine[]
lines: GitCommitLine[],
currentUser: {name?: string, email?: string} | undefined
) {
let commit = commits.get(entry.sha);
if (commit === undefined) {
if (entry.author !== undefined) {
if (currentUser !== undefined &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this if should look like

if (
    currentUser !== undefined &&
    // Name or e-mail is configured
    (currentUser.name !== undefined || currentUser.email !== undefined) &&
    // Match on name if configured
    (currentUser.name === undefined || currentUser.name === entry.author) &&
    // Match on email if configured
    (currentUser.email === undefined || currentUser.email === entry.authorEmail)
) {
    entry.author = 'You';
}

currentUser.name === entry.author && currentUser.email === entry.authorEmail) {
entry.author = 'You';
}
let author = authors.get(entry.author);
if (author === undefined) {
author = {
Expand Down
15 changes: 9 additions & 6 deletions src/git/parsers/logParser.ts
View file
Open in desktop
Expand Up @@ -32,7 +32,7 @@ export class GitLogParser {
repoPath: string | undefined,
fileName: string | undefined,
sha: string | undefined,
currentUser: string | undefined,
currentUser: { name?: string; email?: string } | undefined,
maxCount: number | undefined,
reverse: boolean,
range: Range | undefined
Expand Down Expand Up @@ -87,9 +87,6 @@ export class GitLogParser {
}
else {
entry.author = line.substring(4);
if (currentUser !== undefined && currentUser === entry.author) {
entry.author = 'You';
}
}
break;

Expand Down Expand Up @@ -211,7 +208,8 @@ export class GitLogParser {
relativeFileName,
commits,
authors,
recentCommit
recentCommit,
currentUser
);

break;
Expand All @@ -238,10 +236,15 @@ export class GitLogParser {
relativeFileName: string,
commits: Map<string, GitLogCommit>,
authors: Map<string, GitAuthor>,
recentCommit: GitLogCommit | undefined
recentCommit: GitLogCommit | undefined,
currentUser: {name?: string, email?: string} | undefined
): GitLogCommit | undefined {
if (commit === undefined) {
if (entry.author !== undefined) {
if (currentUser !== undefined &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

if (
    currentUser !== undefined &&
    // Name or e-mail is configured
    (currentUser.name !== undefined || currentUser.email !== undefined) &&
    // Match on name if configured
    (currentUser.name === undefined || currentUser.name === entry.author) &&
    // Match on email if configured
    (currentUser.email === undefined || currentUser.email === entry.authorEmail)
) {
    entry.author = 'You';
}

currentUser.name === entry.author && currentUser.email === entry.email) {
entry.author = 'You';
}
let author = authors.get(entry.author);
if (author === undefined) {
author = {
Expand Down
28 changes: 16 additions & 12 deletions src/gitService.ts
View file
Open in desktop
Expand Up @@ -612,7 +612,7 @@ export class GitService extends Disposable {
args: Container.config.advanced.blame.customArguments,
ignoreWhitespace: Container.config.blame.ignoreWhitespace
});
const blame = GitBlameParser.parse(data, root, file, await this.getCurrentUsername(root));
const blame = GitBlameParser.parse(data, root, file, await this.getCurrentUser(root));
return blame;
}
catch (ex) {
Expand Down Expand Up @@ -692,7 +692,7 @@ export class GitService extends Disposable {
correlationKey: `:${key}`,
ignoreWhitespace: Container.config.blame.ignoreWhitespace
});
const blame = GitBlameParser.parse(data, root, file, await this.getCurrentUsername(root));
const blame = GitBlameParser.parse(data, root, file, await this.getCurrentUser(root));
return blame;
}
catch (ex) {
Expand Down Expand Up @@ -755,7 +755,7 @@ export class GitService extends Disposable {
data,
uri.repoPath,
fileName,
await this.getCurrentUsername(uri.repoPath!)
await this.getCurrentUser(uri.repoPath!)
);
if (blame === undefined) return undefined;

Expand Down Expand Up @@ -808,7 +808,7 @@ export class GitService extends Disposable {
startLine: lineToBlame,
endLine: lineToBlame
});
const currentUser = await this.getCurrentUsername(uri.repoPath!);
const currentUser = await this.getCurrentUser(uri.repoPath!);
const blame = GitBlameParser.parse(data, uri.repoPath, fileName, currentUser);
if (blame === undefined) return undefined;

Expand Down Expand Up @@ -925,13 +925,17 @@ export class GitService extends Disposable {
}

// TODO: Clear cache when git config changes
private _userNameMapCache: Map<string, string | undefined> = new Map();
private _userMapCache: Map<string, { name: string, email: string } | undefined> = new Map();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be adapted like so

    private _userMapCache = new Map<string, { name?: string; email?: string } | null>();

    async getCurrentUser(repoPath: string) {
        let user = this._userMapCache.get(repoPath);
        if (user != null) return user;
        // If we found the repo, but no user data was found just return
        if (user === null) return undefined;

        const data = await Git.config_getRegex('user.(name|email)', repoPath);
        if (!data) {
            // If we found no user data, mark it so we won't bother trying again
            this._userMapCache.set(repoPath, null);
            return undefined;
        }

        user = { name: undefined, email: undefined };

        let match: RegExpExecArray | null = null;
        do {
            match = userConfigRegex.exec(data);
            if (match == null) break;

            // Stops excessive memory usage -- https://bugs.chromium.org/p/v8/issues/detail?id=2869
            user[match[1] as 'name' | 'email'] = (' ' + match[2]).substr(1);
        } while (match != null);

        this._userMapCache.set(repoPath, user);
        return user;
    }

Allows user name or email to be missing, and avoids calling config each time if we don't find data. It also allows for the last entry to win (since that is "closest" to the repo)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex

const userConfigRegex = /^user\.(name|email) (.*)$/gm;

Can be placed on line 70 in gitService.ts


async getCurrentUsername(repoPath: string) {
let user = this._userNameMapCache.get(repoPath);
async getCurrentUser(repoPath: string) {
const user = this._userMapCache.get(repoPath);
if (user === undefined) {
user = await Git.config_get('user.name', repoPath);
this._userNameMapCache.set(repoPath, user);
const userName = await Git.config_get('user.name', repoPath);
const userEmail = await Git.config_get('user.email', repoPath);
if (userName !== undefined && userEmail !== undefined) {
this._userMapCache.set(repoPath, {name: userName, email: userEmail});
return {name: userName, email: userEmail};
}
}
return user;
}
Expand Down Expand Up @@ -1120,7 +1124,7 @@ export class GitService extends Disposable {
repoPath,
undefined,
options.ref,
await this.getCurrentUsername(repoPath),
await this.getCurrentUser(repoPath),
maxCount,
options.reverse!,
undefined
Expand Down Expand Up @@ -1179,7 +1183,7 @@ export class GitService extends Disposable {
repoPath,
undefined,
undefined,
await this.getCurrentUsername(repoPath),
await this.getCurrentUser(repoPath),
maxCount,
false,
undefined
Expand Down Expand Up @@ -1334,7 +1338,7 @@ export class GitService extends Disposable {
root,
file,
opts.ref,
await this.getCurrentUsername(root),
await this.getCurrentUser(root),
maxCount,
opts.reverse!,
range
Expand Down