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

Add support for Git Credential Manager (beta-only) #18700

Merged
merged 60 commits into from
Jun 10, 2024
Merged

Conversation

niik
Copy link
Member

@niik niik commented May 29, 2024

Closes #18654
Ref desktop/dugite-native#510
Ref desktop/dugite-native#508

Description

This adds experimental (beta only) support for Git Credential Manager (GCM) which we're now bundling inside of GitHub Desktop's custom Git environment.

When enabling GCM GitHub Desktop will forward all requests for credentials that don't match accounts stored in GitHub Desktop (in essence third party hosts such as Azure Devops, BitBucket, etc).

GCM currently supports Azure DevOps, Azure DevOps Server (formerly Team Foundation Server), Bitbucket, GitHub, GitLab, and generic username + password hosts.

The feature is currently disabled by default and limited to betadevelopment.

Note for reviewers: Hiding whitespace can should help with reviewing changes in git/core.ts.

Screenshots

Screenshot of Desktop's advanced preferences section with a highlight around the new option to enable Git Credential Manager

Release notes

Notes: [New] Option to use Git Credential Manager for repositories hosted outside of GitHub.com

@niik
Copy link
Member Author

niik commented May 29, 2024

Seems we've got some legit test failures here. One of them is related to our use of '--keep-reduntant-commits' in

// --keep-redundant-commits follows pattern of making sure someone cherry
// picked commit summaries appear in target branch history even tho they may
// be empty. This flag also results in the ability to cherry pick empty
// commits (thus, --allow-empty is not required.)
//
// -m 1 makes it so a merge commit always takes the first parent's history
// (the branch you are cherry-picking from) for the commit. It also means
// there could be multiple empty commits. I.E. If user does a range that
// includes commits from that merge.
const result = await git(
[
'cherry-pick',
...commits.map(c => c.sha),
'--keep-redundant-commits',

Error: GitError: fatal: cherry-pick: --keep-redundant-commits cannot be used with --continue

This was added in git v2.45.1: git/git@bd2f9fd and I have not dug further into it yet. @tidy-dev do you recall the details around this parameter?

The second issue seems to be that our localStorage mock isn't working as intended. Looking into that

@tidy-dev
Copy link
Contributor

--keep-rundandant-commits

Here is some context: #11717

I haven't tested but I believe if you say had commits: A B C D E on a branch and attempted to cherry-pick B C D to another branch where C would be redundant and thus result in an empty commit, it would stop progression and through and error. Apparently the other option was to skip the commits, we went with keep redundant so that commits didn't just disappear. I.E., User cherry-picks 3 but only 2 show up in target branch. A user might be confused about where it went. Possible third option that I didn't explore.. would be capture that error, and putting in the success banner "Redundant commits were skipped"

@tidy-dev
Copy link
Contributor

tidy-dev commented May 29, 2024

Looks like --keep-redundant-commits was deprecated.

https://git-scm.com/docs/git-cherry-pick#Documentation/git-cherry-pick.txt---keep-redundant-commits

Maybe we can just swap for --empty=keep - It would appear so from the documentation.

@niik
Copy link
Member Author

niik commented May 29, 2024

Maybe we can just swap for --empty=keep

I think the problem is that we can't use either one in conjunction with --continue. I presume it's enough to keep it in cherryPick but not in continueCherryPick

app/src/ui/preferences/advanced.tsx Outdated Show resolved Hide resolved
app/src/lib/git/credential.ts Outdated Show resolved Hide resolved
niik added 3 commits May 30, 2024 10:49
@niik niik force-pushed the credential-helper branch from c52fea7 to a69e025 Compare May 30, 2024 08:49
tidy-dev
tidy-dev previously approved these changes Jun 6, 2024
@niik niik dismissed stale reviews from tidy-dev and sergiou87 via 4afbf44 June 10, 2024 09:22
@niik niik merged commit 6ae7444 into development Jun 10, 2024
7 checks passed
@niik niik deleted the credential-helper branch June 10, 2024 11:52
Heyitsquoracom added a commit to Heyitsquoracom/desktop that referenced this pull request Jul 9, 2024
jaisansorachai

This comment was marked as spam.

Kavyasree221

This comment was marked as spam.

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.

Support Git Credential Helpers
6 participants