Improve repository base and head resolution #1706
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,33 +1,27 @@ | ||
| // TODO: rename this package to avoid clash with stdlib | ||
| package context | ||
|
|
||
| import ( | ||
| "errors" | ||
| "sort" | ||
|
|
||
| "github.com/AlecAivazis/survey/v2" | ||
| "github.com/cli/cli/api" | ||
| "github.com/cli/cli/git" | ||
| "github.com/cli/cli/internal/ghrepo" | ||
| "github.com/cli/cli/pkg/iostreams" | ||
| "github.com/cli/cli/pkg/prompt" | ||
| ) | ||
|
|
||
| // cap the number of git remotes looked up, since the user might have an | ||
| // unusually large number of git remotes | ||
| const maxRemotesForLookup = 5 | ||
|
|
||
| func ResolveRemotesToRepos(remotes Remotes, client *api.Client, base string) (*ResolvedRemotes, error) { | ||
| sort.Stable(remotes) | ||
|
|
||
| result := &ResolvedRemotes{ | ||
| remotes: remotes, | ||
| apiClient: client, | ||
| } | ||
|
|
||
|
|
@@ -38,138 +32,136 @@ func ResolveRemotesToRepos(remotes Remotes, client *api.Client, base string) (Re | |
| if err != nil { | ||
| return result, err | ||
| } | ||
| result.baseOverride = baseOverride | ||
| } | ||
|
|
||
| return result, nil | ||
| } | ||
|
|
||
| func resolveNetwork(result *ResolvedRemotes) error { | ||
| var repos []ghrepo.Interface | ||
| for _, r := range result.remotes { | ||
| repos = append(repos, r) | ||
| if len(repos) == maxRemotesForLookup { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| networkResult, err := api.RepoNetwork(result.apiClient, repos) | ||
| result.network = &networkResult | ||
| return err | ||
| } | ||
|
|
||
| type ResolvedRemotes struct { | ||
| baseOverride ghrepo.Interface | ||
| remotes Remotes | ||
| network *api.RepoNetworkResult | ||
| apiClient *api.Client | ||
| } | ||
|
|
||
| func (r *ResolvedRemotes) BaseRepo(io *iostreams.IOStreams) (ghrepo.Interface, error) { | ||
| if r.baseOverride != nil { | ||
| return r.baseOverride, nil | ||
| } | ||
|
|
||
| // if any of the remotes already has a resolution, respect that | ||
|
Contributor
There was a problem hiding this comment. Ideally, it seems that there should only be one resolved remote for any repo, but do we need any safeguards/messaging in place for the case when there are more than one?
Contributor
There was a problem hiding this comment. Possibly we should sort the remotes list so that if there are multiple resolved remotes we will deterministically select the same one every time.
Contributor
Author
There was a problem hiding this comment. @samcoe Good catch! The remote list is already guaranteed to be sorted: first Good question about what if multiple remotes have the resolution bit. Right now I would say that this is not possible, since the resolution bit is only written when no git remote already has a bit. |
||
| for _, r := range r.remotes { | ||
| if r.Resolved == "base" { | ||
| return r, nil | ||
| } else if r.Resolved != "" { | ||
| repo, err := ghrepo.FromFullName(r.Resolved) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return ghrepo.NewWithHost(repo.RepoOwner(), repo.RepoName(), r.RepoHost()), nil | ||
| } | ||
| } | ||
|
|
||
| if !io.CanPrompt() { | ||
| // we cannot prompt, so just resort to the 1st remote | ||
| return r.remotes[0], nil | ||
| } | ||
|
|
||
| // from here on, consult the API | ||
| if r.network == nil { | ||
| err := resolveNetwork(r) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| var repoNames []string | ||
| repoMap := map[string]*api.Repository{} | ||
| add := func(r *api.Repository) { | ||
| fn := ghrepo.FullName(r) | ||
| if _, ok := repoMap[fn]; !ok { | ||
| repoMap[fn] = r | ||
| repoNames = append(repoNames, fn) | ||
| } | ||
| } | ||
|
|
||
| for _, repo := range r.network.Repositories { | ||
| if repo == nil { | ||
| continue | ||
| } | ||
| if repo.IsFork() { | ||
| add(repo.Parent) | ||
| } | ||
| add(repo) | ||
| } | ||
|
|
||
| if len(repoNames) == 0 { | ||
| return r.remotes[0], nil | ||
| } | ||
|
|
||
| baseName := repoNames[0] | ||
| if len(repoNames) > 1 { | ||
| err := prompt.SurveyAskOne(&survey.Select{ | ||
| Message: "Which should be the base repository (used for e.g. querying issues) for this directory?", | ||
| Options: repoNames, | ||
| }, &baseName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| // determine corresponding git remote | ||
| selectedRepo := repoMap[baseName] | ||
| resolution := "base" | ||
| remote, _ := r.RemoteForRepo(selectedRepo) | ||
| if remote == nil { | ||
| remote = r.remotes[0] | ||
| resolution = ghrepo.FullName(selectedRepo) | ||
| } | ||
|
|
||
| // cache the result to git config | ||
| err := git.SetRemoteResolution(remote.Name, resolution) | ||
| return selectedRepo, err | ||
| } | ||
|
|
||
| func (r *ResolvedRemotes) HeadRepos() ([]*api.Repository, error) { | ||
| if r.network == nil { | ||
| err := resolveNetwork(r) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| var results []*api.Repository | ||
| for _, repo := range r.network.Repositories { | ||
| if repo != nil && repo.ViewerCanPush() { | ||
| results = append(results, repo) | ||
| } | ||
| } | ||
| return results, nil | ||
| } | ||
|
|
||
| // RemoteForRepo finds the git remote that points to a repository | ||
| func (r *ResolvedRemotes) RemoteForRepo(repo ghrepo.Interface) (*Remote, error) { | ||
| for _, remote := range r.remotes { | ||
| if ghrepo.IsSame(remote, repo) { | ||
| return remote, nil | ||
| } | ||
| } | ||
| return nil, errors.New("not found") | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts on changing this method to take in
canPrompt boolargument instead of theio *iostreams.IOStreamsargument? It seems that is all we are doing withiohere, and might make the method easier to test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(fwiw, iostreams is easy to test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samcoe That's an excellent point! It's usually better to pass primitives (such as boolean) than complex types such as IOStreams. Here, however, passing in IOStreams semantically draws attention that there will be prompt functionality inside. Right now, the Survey library isn't configured with IOStreams, but we are hoping to one day hook it up. After that, any part of functionality that needs access to stdin/stdout/stderr should take an IOStreams so it's easier to stub out in tests.