cli/cli

`gh repo fork` no longer respects the git protocol of the original repo when `git_protocol` is not configured

williammartin opened this issue · 2 comments

williammartin commented

Describe the bug

➜ gh version
gh version 2.49.0 (2024-04-30)
https://github.com/cli/cli/releases/tag/v2.49.0

In #1056, it was requested that gh repo fork respected the protocol of the existing remote i.e. if the remote was using ssh then the fork would as well. Later in #2711, it was requested that the configured git_protocol would take precedence over the remote, and only if no git_protocol was configured, we would use the protocol of the existing remote.

In #8246 the repo fork command was modified to call config.GitProtocol rather than directly accessing the git_protocol config key. The difference here is that the GitProtocol method always returns the configured value or the default of https. However, this means that the following code branch could never be exercised:

cli/pkg/cmd/repo/fork/fork.go

Lines 257 to 272 in 4896546

if protocol == "" { // user has no set preference
if remote, err := remotes.FindByRepo(repoToFork.RepoOwner(), repoToFork.RepoName()); err == nil {
scheme := ""
if remote.FetchURL != nil {
scheme = remote.FetchURL.Scheme
}
if remote.PushURL != nil {
scheme = remote.PushURL.Scheme
}
if scheme != "" {
protocol = scheme
} else {
protocol = "https"
}
}
}

Steps to reproduce the behavior

  1. Make a temp directory: cd $(mktemp -d)
  2. Clone a repository via ssh: gh repo clone git@github.com:cli/cli.git
  3. Navigate into the repository: cd cli
  4. Fork the repository: gh repo fork --remote=true
  5. List all remotes: git remote -v

Expected behaviour

The new fork uses the ssh protocol because the repo was originally cloned through that.

Actual behaviour

The fork was added using the https protocol:

❯ git remote -v
origin	https://github.com/williammartin/cli.git (fetch)
origin	https://github.com/williammartin/cli.git (push)
upstream	git@github.com:cli/cli.git (fetch)
upstream	git@github.com:cli/cli.git (push)

Extra Details

It's worth knowing that the git_protocol will not be configured in the following cases:

  1. After logging in using --with-token and not providing --git-protocol e.g. echo $TOKEN | gh auth login --with-token
  2. After logging in using the web flow with prompting disabled e.g. GH_PROMPT_DISABLED=true gh auth login
  3. Running a command that uses git while using GH_TOKEN or another env var for authentication (and no entry in the hosts file)
williammartin commented

Why didn't the tests catch this?

This branch is covered by tests but they didn't fail when the behaviour changed because the test setup doesn't accurately reflect reality. In reality, the git_protocol config value can only be https or ssh but the test sets it to the empty string on line 257:

cli/pkg/cmd/repo/fork/fork_test.go

Lines 243 to 264 in 4896546

name: "implicit match, no configured protocol",
tty: true,
opts: &ForkOptions{
Remote: true,
RemoteName: "fork",
},
remotes: []*context.Remote{
{
Remote: &git.Remote{Name: "origin", PushURL: &url.URL{
Scheme: "ssh",
}},
Repo: ghrepo.New("OWNER", "REPO"),
},
},
cfgStubs: func(_ *testing.T, c config.Config) {
c.Set("", "git_protocol", "")
},
httpStubs: forkPost,
execStubs: func(cs *run.CommandStubber) {
cs.Register(`git remote add fork git@github\.com:someone/REPO\.git`, 0, "")
},
wantErrOut: "✓ Created fork someone/REPO\n✓ Added remote fork\n",

The consequence for this is that our config code treats this as a valid configuration with the value of empty string rather than returning https as it would when exercised through the CLI.

Removing this configuration stub fails as expected, with an attempt to add an https remote failing because the expectation is that we are adding an ssh remote:

			execStubs: func(cs *run.CommandStubber) {
				cs.Register(`git remote add fork git@github\.com:someone/REPO\.git`, 0, "")
			},

--- FAIL: TestRepoFork (0.01s)
    --- FAIL: TestRepoFork/implicit_match,_no_configured_protocol (0.00s)
        /Users/williammartin/workspace/cli/pkg/cmd/repo/fork/stub.go:27: unmatched stubs (1): git remote add fork git@github\.com:someone/REPO\.git
panic: no exec stub for `git remote add fork https://github.com/someone/REPO.git` [recovered]
	panic: no exec stub for `git remote add fork https://github.com/someone/REPO.git`
williammartin commented

What should we do about this?

Although no one has complained that we've broken them, the previous behaviour seems reasonable and we did have a regression. I'm inclined to return this behaviour to it's previous state.

I have an additional reason to believe this is the right thing to do and that is because gh auth refresh offers configuration of the Git Credential Manager in cases where the git_protocol was not set and gh is not the credential manager e.g.:

➜  cat ~/.config/gh/hosts.yml
github.com:
    users:
        williammartin:
    user: williammartin

➜  gh auth refresh
? Authenticate Git with your GitHub credentials? (Y/n)

This is a departure from gh auth login which only prompts for this if the user selected the https protocol.