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.

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

Fix incorrect author display `You`(#457) #460

Merged
merged 4 commits into from Jul 28, 2018

Conversation

Projects
None yet
2 participants
@qzyse2017
Copy link
Contributor

qzyse2017 commented Jul 24, 2018

add email for further check

@qzyse2017 qzyse2017 changed the title Fix incorrect author display Fix incorrect author display(#457) Jul 24, 2018

@qzyse2017 qzyse2017 changed the title Fix incorrect author display(#457) Fix incorrect author display `You`(#457) Jul 24, 2018

@eamodio
Copy link
Owner

eamodio left a comment

Thank you so much for the contribution. I have outlined a set of changes that should be made to keep this performant and consistent. Please let me know what you think. Thanks again!

@@ -28,7 +28,8 @@ export class GitBlameParser {
data: string,
repoPath: string | undefined,
fileName: string,
currentUser: string | undefined
currentUser: string | undefined,

This comment has been minimized.

@eamodio

eamodio Jul 25, 2018

Owner

Rather than separating currentUser and currentEmail, let's combine them into currentUser: { name?: string; email?: string } | undefined

This comment has been minimized.

@eamodio

eamodio Jul 25, 2018

Owner

This applies to the other parsers as well

This comment has been minimized.

@qzyse2017

qzyse2017 Jul 25, 2018

Author Contributor

Thanks a lot, using combination is better! ! !

@@ -69,7 +70,8 @@ export class GitBlameParser {
.slice(1)
.join(' ')
.trim();
if (currentUser !== undefined && currentUser === entry.author) {
if (currentUser !== undefined && currentUser === entry.author &&

This comment has been minimized.

@eamodio

eamodio Jul 25, 2018

Owner

Rather than have these checks here, I think they should be moved to be at the end of the entry parsing -- probably in parseEntry https://github.com/eamodio/vscode-gitlens/blob/develop/src/git/parsers/blameParser.ts#L157

That way we don't need to have the check in 2 places, since we don't want to rely on the email being parsed before the author (which should actually be the other way around given the current structure)

This comment has been minimized.

@eamodio

eamodio Jul 25, 2018

Owner

This applies to the other parsers as well

// TODO: Clear cache when git config changes
private _userEmailMapCache: Map<string, string | undefined> = new Map();

async getCurrentUserEmail(repoPath: string) {

This comment has been minimized.

@eamodio

eamodio Jul 25, 2018

Owner

Instead of adding getCurrentUserEmail, let's rename getCurrentUsername to getCurrentUser and have it return { name?: string; email?: string } | undefined. Then we can share the same cache too

This comment has been minimized.

@eamodio

eamodio Jul 25, 2018

Owner

Probably would also be best to add a new method in git.ts:

    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;
        }
    }

That new method can be called from getCurrentUser using the pattern user.(name|email).
It will then return output that looks like:

> git config --get-regex "user.(name|email)"
user.name Foo Bar
user.email foo@bar.com

which could then be easily parsed and returned

This comment has been minimized.

@qzyse2017

qzyse2017 Jul 25, 2018

Author Contributor

It seems the returned results in config_getRegex is got by executing a command in command line and returning its results, but I tried in a test repo, and when the repo's username or email is reset, it may return multiple lines and it will make the judegment rather ambiguous. It seems using config_get would be a better choice

@qzyse2017

This comment has been minimized.

Copy link
Contributor Author

qzyse2017 commented Jul 25, 2018

image

image
these screenshots showed the conditions talked above
the first two config is global, and the last ones are local to the test repo

Make userName and email combined together
- make username and email combined together
- move check for 'You' to parseEntry
- add method `config_getRegex`
@qzyse2017

This comment has been minimized.

Copy link
Contributor Author

qzyse2017 commented Jul 25, 2018

I noticed that the previous method for getCurrentUsername also has a comments which implied some changes needed to be done to check whether the config of the repo's user may has changed, and I think it may be done by checking the changed time and contents config file in .git directory,What is your opinions on that, I'm rather curious about how to get it done 😃
also thanks a lot for your kind suggestions on my pull request and your great work for gitlens, I use it every day.

@eamodio eamodio changed the base branch from master to develop Jul 26, 2018

@eamodio
Copy link
Owner

eamodio left a comment

Looking great -- just some more changes to finish/polish things up

) {
let commit = commits.get(entry.sha);
if (commit === undefined) {
if (entry.author !== undefined) {
if (currentUser !== undefined &&

This comment has been minimized.

@eamodio

eamodio Jul 26, 2018

Owner

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';
}
): GitLogCommit | undefined {
if (commit === undefined) {
if (entry.author !== undefined) {
if (currentUser !== undefined &&

This comment has been minimized.

@eamodio

eamodio Jul 26, 2018

Owner

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';
}
@@ -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();

This comment has been minimized.

@eamodio

eamodio Jul 26, 2018

Owner

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)

This comment has been minimized.

@eamodio

eamodio Jul 26, 2018

Owner

The regex

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

Can be placed on line 70 in gitService.ts

@eamodio

This comment has been minimized.

Copy link
Owner

eamodio commented Jul 26, 2018

As for your question about clearing the cache, I think the easiest solution is to add

        if (reason === RepositoryChange.Config) {
            this._userMapCache.delete(repo.path);
        }

Here: https://github.com/eamodio/vscode-gitlens/blob/develop/src/gitService.ts#L128

That should take care of any repo-level changes to the config. Now if the user changes the global config all bets are off -- but I'm ok not bothering with that at this point 😄

@eamodio

This comment has been minimized.

Copy link
Owner

eamodio commented Jul 26, 2018

Almost there! Looking forward to getting this merged in!

@qzyse2017 qzyse2017 force-pushed the qzyse2017:fixIncorrectAuthorDisplay branch from 3d54a79 to 1d10413 Jul 26, 2018

@qzyse2017 qzyse2017 force-pushed the qzyse2017:fixIncorrectAuthorDisplay branch from 1d10413 to 2a5260e Jul 26, 2018

@eamodio eamodio merged commit 6122550 into eamodio:develop Jul 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.