diff --git a/internal/app/doctor_test.go b/internal/app/doctor_test.go index 5dd81714..c66a086a 100644 --- a/internal/app/doctor_test.go +++ b/internal/app/doctor_test.go @@ -77,7 +77,7 @@ func TestDoctorReportsEveryCheckInsteadOfFailingFirst(t *testing.T) { { name: "no account for the instance of the remote", env: map[string]string{"TEA_HOST": server.URL}, statuses: "Config ok, Instance ok, Account failed, Repository skipped, Git warning", - account: "no account for " + server.URL + " (instance from " + strings.Trim(origin, "()") + "); run tea auth login --host " + server.URL + ", or use -R or --host to target another instance", + account: "no account for " + server.URL + " (instance from " + strings.Trim(origin, "()") + "); run tea auth login --host " + server.URL + ", or use -R to target another repository", }, { name: "token of another instance", env: map[string]string{"TEA_HOST": "https://other.example.org", "GITEA_TOKEN": "token"}, diff --git a/internal/app/help_topics.go b/internal/app/help_topics.go index 40554ad7..78a4a7fa 100644 --- a/internal/app/help_topics.go +++ b/internal/app/help_topics.go @@ -290,9 +290,10 @@ func helpTopics(globals *pflag.FlagSet) []*cobra.Command { ` + indent(flagUsages(flags)) + ` tea selects the repository from --repo, TEA_REPO, git config tea.repository or a -Git remote (--remote, else the current branch's remote, upstream or origin), and the -instance from the repository URL, --host, TEA_HOST or GITEA_INSTANCE_URL, default_host, -or the only configured instance. +Git remote (--remote, else the current branch's remote, upstream or origin, skipping +remotes on instances without an account, or not on --host or the GITEA_TOKEN instance), +and the instance from the repository URL, --host, TEA_HOST or GITEA_INSTANCE_URL, +default_host, or the only configured instance. Environment variables: diff --git a/internal/app/target.go b/internal/app/target.go index 0ecdd1c9..be4a49cc 100644 --- a/internal/app/target.go +++ b/internal/app/target.go @@ -228,21 +228,73 @@ func (a *application) repositorySource(ctx context.Context, t *target, s setting return saved, nil } remote := cmp.Or(a.options.remote, s.Preferences.Remote) - remote, err := git.DiscoverRemote(ctx, a.gitProbe(), a.options.cwd, remote) + hosts, err := a.repositoryHosts(s.Hosts.Hosts) + if err != nil { + return "", err + } + urls := map[string]string{} + // Only remotes on an instance tea can act on name a repository, so that a clone whose origin is + // on another forge still finds its Gitea remote. + repository := func(name string) string { + raw, err := probe("remote", "get-url", name) + if err != nil { + return "" + } + urls[name] = raw + instance, slug, err := settings.RepositoryURL(raw, hosts) + if err != nil || !a.canActOn(instance, hosts) { + return "" + } + return instance + "/" + strings.ToLower(slug) + } + remote, err = git.DiscoverRemote(ctx, a.gitProbe(), a.options.cwd, remote, repository) if errors.Is(err, git.ErrAmbiguousRemote) { return "", clierr.Wrap(clierr.Usage, err) } if err != nil { return "", clierr.Wrap(clierr.Usage, errRepositoryRequired) } - raw, err := probe("remote", "get-url", remote) - if err != nil { - return "", fmt.Errorf("cannot read the URL of Git remote %q: %w", remote, err) + raw, known := urls[remote] + if !known { + if raw, err = probe("remote", "get-url", remote); err != nil { + return "", fmt.Errorf("cannot read the URL of Git remote %q: %w", remote, err) + } } t.RepoSource = "remote " + remote return raw, nil } +// canActOn reports whether tea can use a discovered remote on instance: it is the --host instance, +// else the GITEA_TOKEN instance while that token is set (connect then ignores stored accounts), else +// an instance with an account. +func (a *application) canActOn(instance string, hosts map[string]settings.Host) bool { + selected := a.options.host + if selected == "" && a.deps.Getenv("GITEA_TOKEN") != "" { + selected = a.tokenInstance() + } + if selected != "" { + resolved, err := settings.ResolveHost(selected, hosts) + return err == nil && resolved == instance + } + return len(hosts[instance].Users) > 0 +} + +// repositoryHosts returns the instances a repository URL may name: with --host, only that one when +// it is configured. +func (a *application) repositoryHosts(configured map[string]settings.Host) (map[string]settings.Host, error) { + if a.options.host == "" { + return configured, nil + } + explicit, err := settings.ResolveHost(a.options.host, configured) + if err != nil { + return nil, clierr.Wrap(clierr.Usage, err) + } + if host, exists := configured[explicit]; exists { + return map[string]settings.Host{explicit: host}, nil + } + return configured, nil +} + // agreesWithRepoFlag rejects an argument naming another repository than --repo. func (a *application) agreesWithRepoFlag(req targetRequest, hosts map[string]settings.Host) error { if a.options.repo == "" { @@ -264,15 +316,9 @@ func (a *application) agreesWithRepoFlag(req targetRequest, hosts map[string]set // selectRepository parses raw into the target's repository and, for URLs and HOST/OWNER/REPO, // its instance. With --host, URLs match only that instance. func (a *application) selectRepository(ctx context.Context, t *target, s settings.Snapshot, raw string) error { - configured := s.Hosts.Hosts - if a.options.host != "" { - explicit, err := settings.ResolveHost(a.options.host, configured) - if err != nil { - return clierr.Wrap(clierr.Usage, err) - } - if host, exists := configured[explicit]; exists { - configured = map[string]settings.Host{explicit: host} - } + configured, err := a.repositoryHosts(s.Hosts.Hosts) + if err != nil { + return err } ref, err := settings.ParseRepository(raw, configured) if err != nil { @@ -386,6 +432,10 @@ func (t target) accountError(user string) error { if t.HostSource != "--host" { text += " (instance from " + describeSource(t.HostSource) + ")" } + // --host cannot override the instance that the repository names. + if t.RepoSource != "" && t.HostSource == t.RepoSource { + return errors.New(text + "; run tea auth login --host " + t.Host + ", or use -R to target another repository") + } return errors.New(text + "; run tea auth login --host " + t.Host + ", or use -R or --host to target another instance") } diff --git a/internal/app/target_test.go b/internal/app/target_test.go index 3b4efc3f..9ea85e3e 100644 --- a/internal/app/target_test.go +++ b/internal/app/target_test.go @@ -6,11 +6,13 @@ package app import ( "context" "encoding/json" + "maps" "net/http" "net/http/httptest" "os" "os/exec" "path/filepath" + "slices" "strings" "testing" @@ -233,6 +235,42 @@ func TestRepositoryResolutionPrecedence(t *testing.T) { require.False(t, runner.prompt) } +func TestRepositoryDiscoveryFindsGiteaRemoteBesideAnotherForge(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"full_name":"` + strings.TrimPrefix(r.URL.Path, "/api/v1/repos/") + `"}`)) + })) + defer server.Close() + for _, tc := range []struct { + name string + remotes map[string]string + args []string + env map[string]string + want string + }{ + {"origin on GitHub", map[string]string{"origin": "https://github.com/example/project.git", "gitea": server.URL + "/alice/project.git"}, nil, nil, "alice/project"}, + {"no origin", map[string]string{"github": "https://github.com/example/project.git", "gitea": server.URL + "/alice/project.git"}, nil, nil, "alice/project"}, + {"--host selects its remote", map[string]string{"origin": "https://gitea.com/alice/project.git", "gitea": server.URL + "/bob/tool.git"}, []string{"--host", server.URL}, nil, "bob/tool"}, + {"GITEA_TOKEN selects its instance", map[string]string{"origin": "https://gitea.com/carol/project.git", "gitea": server.URL + "/alice/project.git"}, nil, map[string]string{"GITEA_TOKEN": "token", "TEA_HOST": server.URL}, "alice/project"}, + {"remotes naming one repository", map[string]string{"origin": "https://github.com/example/project.git", "gitea": server.URL + "/alice/project.git", "gitea-ssh": "git@127.0.0.1:alice/project.git"}, nil, nil, "alice/project"}, + } { + t.Run(tc.name, func(t *testing.T) { + deps, out, _ := testDeps(t) + if tc.env != nil { + deps.Getenv = envFunc(tc.env) + } + configureAccounts(t, deps, server.URL, "alice", "alice") + configureAccounts(t, deps, "https://gitea.com", "carol", "carol") + results := map[string]string{"remote": strings.Join(slices.Sorted(maps.Keys(tc.remotes)), "\n")} + for name, url := range tc.remotes { + results["remote get-url "+name] = url + } + deps.Git = &fakeGit{results: results} + require.NoError(t, Execute(context.Background(), deps, append([]string{"repo", "view", "--no-input", "--json=full_name"}, tc.args...))) + require.JSONEq(t, `{"full_name":"`+tc.want+`"}`, out.String()) + }) + } +} + func TestRepositoryArgumentsAcceptShorthands(t *testing.T) { server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte(`{"full_name":"` + strings.TrimPrefix(r.URL.Path, "/api/v1/repos/") + `"}`)) @@ -285,7 +323,7 @@ func TestAccountErrorsNameTheInstanceAndAccounts(t *testing.T) { {"unknown --user", []string{"alice", "bob"}, "alice", []string{"-R", "alice/project", "--user", "nobody"}, nil, `account "nobody" is not logged in on https://example.org (accounts: alice, bob)`}, {"unknown --user without accounts", nil, "", []string{"-R", "alice/project", "--user", "nobody", "--host", instance}, nil, `account "nobody" is not logged in on https://example.org; run tea auth login --host https://example.org`}, {"no active account", []string{"alice", "bob"}, "", []string{"-R", "alice/project"}, nil, "no active account on https://example.org (accounts: alice, bob); pass --user or run tea auth switch"}, - {"instance from a remote", []string{"alice"}, "alice", nil, remote, `no account for https://gitea.com (instance from git remote "origin"); run tea auth login --host https://gitea.com, or use -R or --host to target another instance`}, + {"instance from a remote", []string{"alice"}, "alice", nil, remote, `no account for https://gitea.com (instance from git remote "origin"); run tea auth login --host https://gitea.com, or use -R to target another repository`}, } { t.Run(tc.name, func(t *testing.T) { deps, _, _ := testDeps(t) diff --git a/internal/git/discover.go b/internal/git/discover.go index 505fc6db..53c53089 100644 --- a/internal/git/discover.go +++ b/internal/git/discover.go @@ -4,6 +4,7 @@ package git import ( + "cmp" "context" "errors" "slices" @@ -16,27 +17,72 @@ var ErrAmbiguousRemote = errors.New("ambiguous repository: select --remote or -- // DiscoverRemote names the remote used for repository discovery: preferred when set, otherwise // the base remote pr checkout recorded for the current branch, the current branch's upstream // remote unless it is a pulls// head remote, then upstream, origin, or the only remote. -func DiscoverRemote(ctx context.Context, runner Runner, dir, preferred string) (string, error) { +// When repository is not nil, it names the repository of a remote tea can act on, or returns "" +// for one on another forge. Those steps then skip such remotes, and the other remotes decide when +// they name exactly one repository; with none of them usable the choice is the name-based one. +func DiscoverRemote(ctx context.Context, runner Runner, dir, preferred string, repository func(remote string) string) (string, error) { if preferred != "" { return preferred, nil } - branch, remote := CurrentUpstreamRemote(ctx, runner, dir) - if base := pullBaseRemote(ctx, runner, dir, branch); base != "" { - return base, nil + usable := func(remote string) bool { return repository == nil || repository(remote) != "" } + // fallback is the remote chosen by name alone, used when no remote is usable. + var fallback string + var checked []string + try := func(remote string) bool { + if remote == "" || slices.Contains(checked, remote) { + return false + } + if usable(remote) { + return true + } + fallback = cmp.Or(fallback, remote) + checked = append(checked, remote) + return false } - if remote != "" && !strings.HasPrefix(remote, "pulls/") { - return remote, nil + branch, tracking := CurrentUpstreamRemote(ctx, runner, dir) + if strings.HasPrefix(tracking, "pulls/") { + tracking = "" + } + for _, remote := range []string{pullBaseRemote(ctx, runner, dir, branch), tracking} { + if try(remote) { + return remote, nil + } } names, err := runner.Output(ctx, dir, false, "remote") if err != nil { + if fallback != "" { + return fallback, nil + } return "", err } available := strings.Fields(names) for _, name := range []string{"upstream", "origin"} { - if slices.Contains(available, name) { + if slices.Contains(available, name) && try(name) { return name, nil } } + if repository != nil { + var found, foundRepository string + for _, name := range available { + // pr checkout's head remotes point at forks; they never become the discovered repository. + if slices.Contains(checked, name) || strings.HasPrefix(name, "pulls/") { + continue + } + switch repo := repository(name); { + case repo == "": + case found == "": + found, foundRepository = name, repo + case repo != foundRepository: + return "", ErrAmbiguousRemote + } + } + if found != "" { + return found, nil + } + } + if fallback != "" { + return fallback, nil + } if len(available) == 1 { return available[0], nil } diff --git a/internal/git/discover_test.go b/internal/git/discover_test.go index b73afdbc..e5eee46b 100644 --- a/internal/git/discover_test.go +++ b/internal/git/discover_test.go @@ -45,11 +45,54 @@ func TestDiscoverRemote(t *testing.T) { {name: "no remotes", runner: discoverRunner{"remote": ""}, wantErr: ErrAmbiguousRemote}, } { t.Run(test.name, func(t *testing.T) { - remote, err := DiscoverRemote(context.Background(), test.runner, "/work", test.preferred) + remote, err := DiscoverRemote(context.Background(), test.runner, "/work", test.preferred, nil) require.ErrorIs(t, err, test.wantErr) require.Equal(t, test.want, remote) }) } - _, err := DiscoverRemote(context.Background(), discoverRunner{}, "/work", "") + _, err := DiscoverRemote(context.Background(), discoverRunner{}, "/work", "", nil) require.Error(t, err) } + +func TestDiscoverRemoteSkipsUnusableRemotes(t *testing.T) { + const head, tracking, base = "symbolic-ref --short HEAD", "config --get branch.main.remote", "config --get branch.main.tea-base-remote" + // gitea makes each named remote usable; a name=repository pair names its repository. + gitea := func(names ...string) func(string) string { + return func(remote string) string { + for _, name := range names { + if key, repo, ok := strings.Cut(name, "="); ok && key == remote { + return repo + } else if name == remote { + return "localhost/" + remote + } + } + return "" + } + } + for _, test := range []struct { + name string + runner discoverRunner + usable func(string) string + want string + wantErr error + }{ + {name: "origin on another forge", runner: discoverRunner{"remote": "gitea\norigin"}, usable: gitea("gitea"), want: "gitea"}, + {name: "no upstream or origin", runner: discoverRunner{"remote": "gitea\ngithub"}, usable: gitea("gitea"), want: "gitea"}, + {name: "usable upstream keeps priority", runner: discoverRunner{"remote": "fork\norigin\nupstream"}, usable: gitea("fork", "upstream"), want: "upstream"}, + {name: "usable origin after unusable upstream", runner: discoverRunner{"remote": "fork\norigin\nupstream"}, usable: gitea("fork", "origin"), want: "origin"}, + {name: "unusable branch upstream", runner: discoverRunner{head: "main", tracking: "github", "remote": "github\norigin"}, usable: gitea("origin"), want: "origin"}, + {name: "usable recorded pull base", runner: discoverRunner{head: "main", tracking: "fork", base: "gitea", "remote": "fork\ngitea\norigin"}, usable: gitea("fork", "gitea"), want: "gitea"}, + {name: "several usable others", runner: discoverRunner{"remote": "a-gitea\norigin\nz-fork"}, usable: gitea("a-gitea", "z-fork"), wantErr: ErrAmbiguousRemote}, + {name: "usable others naming one repository", runner: discoverRunner{"remote": "gitea\ngitea-ssh\norigin"}, usable: gitea("gitea=localhost/alice/project", "gitea-ssh=localhost/alice/project"), want: "gitea"}, + {name: "unusable branch upstream named origin", runner: discoverRunner{head: "main", tracking: "origin", "remote": "gitea\norigin"}, usable: gitea("gitea"), want: "gitea"}, + {name: "pull head remotes never win", runner: discoverRunner{"remote": "gitea\norigin\npulls/bob/project"}, usable: gitea("gitea", "pulls/bob/project"), want: "gitea"}, + {name: "nothing usable keeps the name choice", runner: discoverRunner{"remote": "fork\norigin"}, usable: gitea(), want: "origin"}, + {name: "nothing usable stays ambiguous", runner: discoverRunner{"remote": "one\ntwo"}, usable: gitea(), wantErr: ErrAmbiguousRemote}, + } { + t.Run(test.name, func(t *testing.T) { + remote, err := DiscoverRemote(context.Background(), test.runner, "/work", "", test.usable) + require.ErrorIs(t, err, test.wantErr) + require.Equal(t, test.want, remote) + }) + } +}