Skip to content

respect git_protocol setting#810

Merged
vilmibm merged 2 commits intomasterfrom
respect-protocol
Apr 22, 2020
Merged

respect git_protocol setting#810
vilmibm merged 2 commits intomasterfrom
respect-protocol

Conversation

@vilmibm
Copy link
Copy Markdown
Contributor

@vilmibm vilmibm commented Apr 21, 2020

Fixes #546

this adds recognition of the git_protocol setting when:

  • creating a repo
  • cloning a repo
  • forking a repo
  • forking/pushing during pr create
  • checking out a PR

additionally, it:

  • consolidates remote adding to use AddRemote; this introduces a fetch
    where there previously hadn't been one
  • changes repo clone to accept an ssh url
  • changes repo fork to accept an ssh url

i just added basic unit tests; adding new test cases for all of the
above scenarios seemed like diminishing returns.

this adds recognition of the git_protocol setting when:

- creating a repo
- cloning a repo
- forking a repo
- forking/pushing during pr create
- checking out a PR

additionally, it:

- consolidates remote adding to use AddRemote; this introduces a fetch
where there previously hadn't been one
- changes repo clone to accept an ssh url
- changes repo fork to accept an ssh url

i just added basic unit tests; adding new test cases for all of the
above scenarios seemed like diminishing returns.
@vilmibm vilmibm requested review from mislav and probablycorey April 22, 2020 19:28
@vilmibm vilmibm marked this pull request as ready for review April 22, 2020 19:28
@vilmibm vilmibm changed the title [WIP] respect git_protocol setting respect git_protocol setting Apr 22, 2020
Copy link
Copy Markdown
Contributor

@probablycorey probablycorey left a comment

Choose a reason for hiding this comment

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

This looks great 😎

baseRemote, _ := remotes.FindByRepo(baseRepo.RepoOwner(), baseRepo.RepoName())
// baseRemoteSpec is a repository URL or a remote name to be used in git fetch
baseURLOrName := fmt.Sprintf("https://github.com/%s.git", ghrepo.FullName(baseRepo))
baseURLOrName := formatRemoteURL(cmd, ghrepo.FullName(baseRepo))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 this is nice

remoteAdd.Stdout = os.Stdout
remoteAdd.Stderr = os.Stderr
err = run.PrepareCmd(remoteAdd).Run()
_, err = git.AddRemote("origin", remoteURL)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍👍this is easier to understand now

command/repo.go Outdated
if err != nil {
return fmt.Errorf("did not understand argument: %w", err)
}
toFork, err = ghrepo.FromURL(parsedURL)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You didn't introduce the toFork var in this PR. But naming it to repoToFork might make this easier to read. I was thrown off by what toFork did.

@vilmibm vilmibm merged commit 9d7590c into master Apr 22, 2020
@mislav mislav deleted the respect-protocol branch June 25, 2020 14:37
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 defaulting to SSH remotes

2 participants