Improve repository base and head resolution #1706
Conversation
On first run in a git repository, `BaseRepo()` will now prompt the user which repository should be queried as base repository if there are multiple git remotes or when we are in the context of a fork. In non-interactive mode, the prompt is skipped and we default to the first git remote instead. After the base repo is resolved, the result is cached in the local repository using `git config` so that RepositoryNetwork API lookups can be avoided in the future.
| return r.baseOverride, nil | ||
| } | ||
|
|
||
| // if any of the remotes already has a resolution, respect that |
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?
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.
There was a problem hiding this comment.
@samcoe Good catch! The remote list is already guaranteed to be sorted: first upstream, then github, then origin, then alphabetical order. This makes me feel confident that the resolution will be deterministic.
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 _, repo := range r.Network.Repositories { | ||
| if repo != nil && ghrepo.IsSame(repo, r.BaseOverride) { | ||
| return repo, nil | ||
| func (r *ResolvedRemotes) BaseRepo(io *iostreams.IOStreams) (ghrepo.Interface, error) { |
There was a problem hiding this comment.
What are your thoughts on changing this method to take in canPrompt bool argument instead of the io *iostreams.IOStreams argument? It seems that is all we are doing with io here, and might make the method easier to test.
There was a problem hiding this comment.
(fwiw, iostreams is easy to test)
There was a problem hiding this comment.
@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.
We no longer guess the head repository using heuristics; instead, we present the user with the choice of pushable repositories and an additional option to create a new fork. The new `pr create --head` flag is available for the user to specify the head branch in `branch` or `owner:branch` format and completely skip any forking or auto-pushing checks.
51101d6 to
f923966
Compare
|
Glad to see something safer. Still would like to be able to somehow configure an automatic default. In my case, if there is a remote named ¹Using the same remote branch name as the local branch, like if I had typed git push forkand then copied and pasted the suggested variant with |
|
@jglick since the resolved remote is persisted in git-config, it would appear that one could probably set their preferred remote via git-config manually. This would allow you to set it for your entire system, or even a collection of repos (using git's conditional-includes). https://github.com/cli/cli/pull/1706/files#diff-5b52564f62704260aa7d42e182fd1a5dR139 (However, this would increase the chance of the multiple-remote configuration scenario that was discussed. Unsure of the ramifications of having this configured manually above the repo-level git-config.) I recognize this doesn't provide first-class support for configuring a default; instead leveraging the cli's own persistence to side-step the normal lookup. |
First, this changes how "base" repository is determined when in a git repository. This affects all gh commands that operate on the current repo (i.e. when the
-R, --repoflag wasn't passed).On first run in a git repository, if there are multiple git remotes or when we are in the context of a fork,
BaseRepo()will now prompt the user which repository should be queried as base repository:In non-interactive mode, the prompt is skipped and we default to the first git remote instead.
After the base repo is resolved, the result is cached in the local repository using
git configso that RepositoryNetwork API lookups can be avoided in the future.Fixes #924, fixes #458, fixes #867, fixes #317
Second, this changes how the "head" repository is determined during
gh pr create.We no longer try to guess which is a suitable push target for the "head" branch. Instead, unless the user has already fully pushed their branch, we always present the user with a prompt:
Thus, the prompt also acts as a confirmation step for pushing/forking. We never fork nor push without explicit user agreement anymore.
In non-interactive mode, we error out, instructing the user to fully push the branch to a remote before starting
pr create, or to use the new--head [<owner>:]<branch>flag to explicitly set which head branch should be used when creating the PR.Fixes #1486, fixes #1330, fixes #1098, fixes #1032, fixes #800, fixes #684
Ref. #1645
TODO:
pr create