Skip to content

Fix merge email confirmation when git config fails #6797

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

Merged
merged 3 commits into from
Apr 22, 2025

Conversation

kabel
Copy link
Contributor

@kabel kabel commented Apr 17, 2025

Centralizes the preferred email logic for a PullRequestModel in the FolderRepositoryManager.

PullRequestGitHelper.getEmail can return an empty string when it is unable to find user.email in the local or global config. This can occur when the user.email config is set in a different scope (system, worktree) or its in an include file of the requested (local, global) scope (git config is not run with the --includes argument). Ideally there would be an API method to get config from any scope, sadly this is not exposed on the Repository interface from the core git extension. If we are unable to get the email from git config, continue to default to the primary GitHub email and allow users to manually select the correct address to use.

Use this same default email logic in the Activity Bar View so that the merge buttons behave in a similar manner with regard to what email is used for merge commits.

See #6593, #6696

Centralizes the preferred email logic for a `PullRequestModel` in the `FolderRepositoryManager`.

`PullRequestGitHelper.getEmail` can return an empty string when it is unable to find `user.email` in the local or global config. This can occur when the `user.email` config is set in a different scope (system, worktree) or its in an include file of the requested (local, global) scope (`git config` is not run with the `--includes` argument). Ideally there would be an API method to get config from _any_ scope, sadly this is not exposed on the `Repository` interface from the core git extension. If we are unable to get the email from git config, continue to default to the primary GitHub email and allow users to manually select the correct address to use.

Use this same default email logic in the Activity Bar View so that the merge buttons behave in a similar manner with regard to what email is used for merge commits.

See microsoft#6593, microsoft#6696
kabel added a commit to kabel/vscode that referenced this pull request Apr 17, 2025
This allows extensions to get able to get configuration for any scope. This should prevent confusion when configuration can be excluded when requesting a specific scope.

See microsoft/vscode-pull-request-github#6797
This state will be used when the email address from git config is unavailable or doesn't match any of the GitHub user's emails.
@kabel
Copy link
Contributor Author

kabel commented Apr 18, 2025

Additional log context on why the email is blank:

[info] > git config --get --local user.email [4ms]
[warning] [Git][config] git config failed: Failed to execute git
[info] > git config --get --global user.email [5ms]
[warning] [Git][config] git config failed: Failed to execute git

So email got evaluated to '' && undefined ?? '[email protected]' which is ''

Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR

}

const gitHubEmails = await pullRequest.githubRepository.getAuthenticatedUserEmails();
const getMatch = (match: string | undefined) => match && gitHubEmails.find(email => email.toLowerCase() === match.toLowerCase());
Copy link
Member

Choose a reason for hiding this comment

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

Can use compareIgnoreCase instead of toLowerCaseing both values.

@alexr00 alexr00 enabled auto-merge (squash) April 22, 2025 09:12
@vs-code-engineering vs-code-engineering bot added this to the April 2025 milestone Apr 22, 2025
@alexr00
Copy link
Member

alexr00 commented Apr 22, 2025

/azp run

Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@alexr00 alexr00 merged commit 5738963 into microsoft:main Apr 22, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants