fix: address verified review findings (branches-checkout-status-1)

Signed-off-by: bircni <bircni@icloud.com>
This commit is contained in:
bircni committed 2026-10-18 17:38:53 +02:00
1 parent 6e523d969d
commit 58f0fc4bf9
27 files changed
+272 -56

No files matched your search

+1 -1
View File
@@ -179,7 +179,7 @@ func TestWorkflowRunNeedsTheCheckedOutBranchOnTheServer(t *testing.T) {
deps, _, _ := actionsDeps(t, server.URL, false)
deps.Git = &fakeGit{results: map[string]string{
"remote": "origin", "remote get-url origin": server.URL + "/alice/project.git",
"symbolic-ref --short HEAD": "local-only", "config --get branch.local-only.remote": "origin",
"symbolic-ref --quiet HEAD": "local-only", "config --get branch.local-only.remote": "origin",
}}
err := Execute(context.Background(), deps, []string{"workflow", "run", "ci.yml", "-f", "note=x"})
require.EqualError(t, err, "branch local-only is not on alice/project; push it or pass --ref main")
+1 -1
View File
@@ -85,7 +85,7 @@ func TestAPIBranchPlaceholderUsesEscapedCurrentBranch(t *testing.T) {
}))
defer server.Close()
deps, _, _ := serverDeps(t, server.URL)
deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --short HEAD": "feature/x"}}
deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --quiet HEAD": "feature/x"}}
require.NoError(t, Execute(context.Background(), deps, []string{"api", "repos/{owner}/{repo}/branches/{branch}", "-R", "alice/project"}))
require.Equal(t, 1, calls)
deps.Git = &fakeGit{}
+21 -6
View File
@@ -85,16 +85,21 @@ $ tea branch list -R alice/project --limit 100 --json name,sha`,
Columns: []terminal.Column{{Header: "Name"}, {Header: "Commit"}, {Header: "Protected"}, {Header: "Updated"}, {Header: "Message", Flex: true}},
Row: func(item services.Branch) []terminal.Cell {
name := terminal.Cell{Text: item.Name, Link: branchURL(repository, item.Name)}
// Piped output keeps the plain name and the full commit, for scripts.
sha := item.SHA
if a.deps.Streams.OutTTY {
sha = shortSHA(sha)
if item.Name == repository.DefaultBranch {
name.Text += " (default)"
name.Bold = true
}
}
protected := ""
if item.Protected {
protected = "protected"
}
message, _, _ := strings.Cut(item.Message, "\n")
return []terminal.Cell{name, {Text: shortSHA(item.SHA), Muted: true}, {Text: protected, Color: "yellow"}, {Time: terminal.ParseTime(item.UpdatedAt)}, {Text: message}}
return []terminal.Cell{name, {Text: sha, Muted: true}, {Text: protected, Color: "yellow"}, {Time: terminal.ParseTime(item.UpdatedAt)}, {Text: message}}
},
Context: "branches in " + target.Repo, Empty: "no branches in " + target.Repo,
}
@@ -294,17 +299,22 @@ $ tea branch protect --pattern 'release/*' --allow-push acme/release`,
if err := requireNames("push", "", pushers, rule.StatusChecks); err != nil {
return err
}
target, err := a.resolve(cmd.Context(), true)
if err != nil {
return err
}
owner, _, _ := strings.Cut(target.Repo, "/")
for _, pusher := range pushers {
if _, team, ok := strings.Cut(pusher, "/"); ok {
if org, team, ok := strings.Cut(pusher, "/"); ok {
// Gitea only looks teams up in the organization that owns the repository.
if !strings.EqualFold(org, owner) {
return clierr.Wrap(clierr.Usage, fmt.Errorf("team %s is not in %s, the owner of %s", pusher, owner, target.Repo))
}
rule.PushTeams = append(rule.PushTeams, team)
} else {
rule.PushUsers = append(rule.PushUsers, pusher)
}
}
target, err := a.resolve(cmd.Context(), true)
if err != nil {
return err
}
service := services.Branches{Client: target.client}
rules := args
if pattern != "" {
@@ -313,6 +323,11 @@ $ tea branch protect --pattern 'release/*' --allow-push acme/release`,
_, failures := applyBatch(cmd, rules, func(name string) string { return "protect " + name }, func(name string) (string, error) {
if pattern == "" {
if _, err := service.View(cmd.Context(), target.Repo, name); err != nil {
if services.IsNotFound(err) {
if _, repoErr := (services.Repositories{Client: target.client}).View(cmd.Context(), target.Repo); repoErr != nil {
return "", repoErr
}
}
return "", services.Classify(err, "branch", strconv.Quote(name), target.Repo)
}
}
+55 -1
View File
@@ -75,7 +75,7 @@ func TestBranchProtectChecksTheBranchAndIsIdempotent(t *testing.T) {
deps, _, diagnostics := repoTTYDeps(t, server.URL)
err := Execute(context.Background(), deps, []string{"branch", "protect", "nope", "-R", "alice/x"})
require.EqualError(t, err, `protect nope: branch "nope" not found in alice/x`)
require.Equal(t, []string{"GET /api/v1/repos/alice/x/branches/nope"}, requests)
require.Equal(t, []string{"GET /api/v1/repos/alice/x/branches/nope", "GET /api/v1/repos/alice/x"}, requests)
require.NoError(t, Execute(context.Background(), deps, []string{"branch", "protect", "main", "dev", "-R", "alice/x"}))
require.Equal(t, "! main is already protected in alice/x; change its rule on the web\n"+
"✓ Protected dev in alice/x (1 approval required, no direct pushes)\n", diagnostics.String())
@@ -112,3 +112,57 @@ func TestBranchDeleteTakesSeveralBranches(t *testing.T) {
require.Equal(t, []string{"old-a", "old-b"}, deleted)
require.Equal(t, "✓ Deleted branch old-a from alice/x\n✓ Deleted branch old-b from alice/x\n", diagnostics.String())
}
func TestBranchListPipedKeepsPlainNamesAndFullCommits(t *testing.T) {
server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/api/v1/repos/alice/x":
_, _ = w.Write([]byte(`{"full_name":"alice/x","default_branch":"main"}`))
default:
_, _ = w.Write([]byte(`[{"name":"main","commit":{"id":"0123456789abcdef"}}]`))
}
}))
defer server.Close()
deps, out, _ := serverDeps(t, server.URL)
require.NoError(t, Execute(context.Background(), deps, []string{"branch", "list", "-R", "alice/x"}))
require.Contains(t, out.String(), "main\t0123456789abcdef\t")
require.NotContains(t, out.String(), "(default)")
deps, out, _ = repoTTYDeps(t, server.URL)
require.NoError(t, Execute(context.Background(), deps, []string{"branch", "list", "-R", "alice/x"}))
require.Contains(t, out.String(), "main (default)")
require.Contains(t, out.String(), "0123456")
require.NotContains(t, out.String(), "0123456789")
}
func TestBranchCreateWithAnUnknownSourceIsNotFound(t *testing.T) {
server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusInternalServerError)
_, _ = w.Write([]byte(`{"message":"object does not exist [id: nope, rel_path: ]"}`))
}))
defer server.Close()
deps, _, _ := repoTTYDeps(t, server.URL)
err := Execute(context.Background(), deps, []string{"branch", "create", "hotfix", "--source", "nope", "-R", "alice/x"})
require.EqualError(t, err, `branch, tag or commit "nope" not found in alice/x`)
}
func TestBranchDeleteAndProtectNameAMissingRepository(t *testing.T) {
server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusNotFound)
}))
defer server.Close()
deps, _, _ := repoTTYDeps(t, server.URL)
err := Execute(context.Background(), deps, []string{"branch", "delete", "old", "-R", "alice/typo", "--yes"})
require.EqualError(t, err, "delete branch old: repository alice/typo not found")
err = Execute(context.Background(), deps, []string{"branch", "protect", "old", "-R", "alice/typo"})
require.EqualError(t, err, "protect old: repository alice/typo not found")
}
func TestBranchProtectRejectsATeamOfAnotherOrganization(t *testing.T) {
server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
t.Errorf("unexpected request %s %s", r.Method, r.URL.Path)
}))
defer server.Close()
deps, _, _ := repoTTYDeps(t, server.URL)
err := Execute(context.Background(), deps, []string{"branch", "protect", "main", "--allow-push", "other/devs", "-R", "acme/x"})
require.EqualError(t, err, "team other/devs is not in acme, the owner of acme/x")
}
+25 -1
View File
@@ -159,6 +159,7 @@ $ tea pr checkout 13 --update`
// checkoutBranchName names the local branch for a pull request: its head branch name when
// that is free or already at the head, else OWNER/BRANCH for a fork, else pr/NUMBER. A branch that
// an earlier checkout of the same pull request created is reused, so that --update can advance it.
// A fork's head name is not reused when that local branch tracks something else, such as main.
func (a *application) checkoutBranchName(ctx context.Context, t target, pull services.Pull) string {
fallback := fmt.Sprintf("pr/%d", pull.Number)
if pull.HeadRemoteDeleted || pull.Head == "" {
@@ -168,15 +169,38 @@ func (a *application) checkoutBranchName(ctx context.Context, t target, pull ser
if owner, _, _ := strings.Cut(pull.HeadRepo, "/"); owner != "" && !strings.EqualFold(pull.HeadRepo, t.Repo) {
candidates = append(candidates, owner+"/"+pull.Head)
}
sameRepo := strings.EqualFold(pull.HeadRepo, t.Repo)
for _, name := range candidates {
tip, err := a.gitProbe().Output(ctx, a.options.cwd, false, "rev-parse", "--verify", "refs/heads/"+name+"^{commit}")
if err != nil || tip == pull.HeadSHA || git.ReadBranchConfig(ctx, a.gitProbe(), a.options.cwd, name).PullNumber == pull.Number {
if err != nil {
if !a.branchPathTaken(ctx, name) {
return name
}
continue
}
config := git.ReadBranchConfig(ctx, a.gitProbe(), a.options.cwd, name)
if config.PullNumber == pull.Number || (tip == pull.HeadSHA && (sameRepo || config.Remote == "")) {
return name
}
}
return fallback
}
// branchPathTaken reports whether a branch name cannot be created because another branch is a
// directory prefix of it (feature for feature/login) or lives below it.
func (a *application) branchPathTaken(ctx context.Context, name string) bool {
for i, r := range name {
if r != '/' {
continue
}
if _, err := a.gitProbe().Output(ctx, a.options.cwd, false, "rev-parse", "--verify", "refs/heads/"+name[:i]+"^{commit}"); err == nil {
return true
}
}
below, err := a.gitProbe().Output(ctx, a.options.cwd, false, "for-each-ref", "--count=1", "--format=%(refname)", "refs/heads/"+name+"/")
return err == nil && below != ""
}
// ensureHeadRemote returns the remote holding the PR head, adding one when none matches.
// An empty name means the head repository has no usable clone URL and the base remote's PR ref should be used.
func (a *application) ensureHeadRemote(ctx context.Context, target target, pull services.Pull) (name, remoteURL string, undo func(), err error) {
+9 -2
View File
@@ -19,7 +19,7 @@ import (
func TestCheckoutRemoteUsesOnlyMatchingRepository(t *testing.T) {
deps, _, _ := testDeps(t)
deps.Git = &fakeGit{results: map[string]string{
"remote": "fork\norigin\nupstream\ntracking", "symbolic-ref --short HEAD": "main", "config --get branch.main.remote": "tracking",
"remote": "fork\norigin\nupstream\ntracking", "symbolic-ref --quiet HEAD": "main", "config --get branch.main.remote": "tracking",
"remote get-url fork": "https://example.org/bob/project.git",
"remote get-url origin": "https://example.org/alice/project.git",
"remote get-url upstream": "https://other.example.org/alice/project.git",
@@ -43,7 +43,7 @@ func TestCheckoutRemoteUsesOnlyMatchingRepository(t *testing.T) {
func TestCheckoutSelectedInstanceDisambiguatesSharedSSHRemote(t *testing.T) {
deps, _, _ := testDeps(t)
deps.Git = &fakeGit{results: map[string]string{
"remote": "local\norigin", "symbolic-ref --short HEAD": "main", "config --get branch.main.remote": "origin",
"remote": "local\norigin", "symbolic-ref --quiet HEAD": "main", "config --get branch.main.remote": "origin",
"remote get-url local": "/srv/git/project.git",
"remote get-url origin": "ssh://git@git.example.org:2222/alice/project.git",
}}
@@ -132,6 +132,13 @@ func TestCheckoutNamesTheBranchAfterTheHead(t *testing.T) {
{"fork head name taken", fork, map[string]string{"rev-parse --verify refs/heads/feature^{commit}": "other"}, "bob/feature"},
{"fork names taken", fork, map[string]string{"rev-parse --verify refs/heads/feature^{commit}": "other", "rev-parse --verify refs/heads/bob/feature^{commit}": "other"}, "pr/7"},
{"deleted head", services.Pull{Number: 7, Head: "feature", HeadRemoteDeleted: true}, nil, "pr/7"},
{"fork branch at the head without upstream", fork, map[string]string{"rev-parse --verify refs/heads/feature^{commit}": "abc123"}, "feature"},
{"fork default branch tracking origin", services.Pull{Number: 7, Head: "main", HeadSHA: "abc123", HeadRepo: "bob/project"}, map[string]string{
"rev-parse --verify refs/heads/main^{commit}": "abc123", "config --get branch.main.remote": "origin"}, "bob/main"},
{"branch above the head name", services.Pull{Number: 7, Head: "feature/login", HeadSHA: "abc123", HeadRepo: "alice/project"}, map[string]string{
"rev-parse --verify refs/heads/feature^{commit}": "other"}, "pr/7"},
{"branches below the head name", same, map[string]string{
"for-each-ref --count=1 --format=%(refname) refs/heads/feature/": "refs/heads/feature/login"}, "pr/7"},
} {
t.Run(tc.name, func(t *testing.T) {
deps, _, _ := testDeps(t)
+1 -1
View File
@@ -25,7 +25,7 @@ func (a *application) gitIn(ctx context.Context, dir string, args ...string) (st
// currentBranch names the checked-out branch without writing Git diagnostics.
func (a *application) currentBranch(ctx context.Context) (string, error) {
return a.gitProbe().Output(ctx, a.options.cwd, false, "symbolic-ref", "--short", "HEAD")
return git.CurrentBranch(ctx, a.gitProbe(), a.options.cwd)
}
// gitProbe runs exploratory Git commands whose failures are expected, without Git's diagnostics.
+6 -1
View File
@@ -52,9 +52,14 @@ func (a *application) pullCleanCommand() *cobra.Command {
if err != nil {
return err
}
// pr checkout names a fork's branch after its head, or OWNER/BRANCH when the head name is taken.
preferred := []string{fmt.Sprintf("pr/%d", n)}
if owner, _, _ := strings.Cut(pull.HeadRepo, "/"); owner != "" && !strings.EqualFold(pull.HeadRepo, target.Repo) {
preferred = append(preferred, owner+"/"+pull.Head)
}
localBranch, remote, err := git.FindCleanBranch(cmd.Context(), a.deps.Git, git.FindCleanBranchOptions{
Directory: a.options.cwd, HeadSHA: pull.HeadSHA, HeadBranch: pull.Head, IgnoreSHA: ignoreSHA, DefaultBranch: repository.DefaultBranch,
PreferredNames: []string{fmt.Sprintf("pr/%d", n)}, MatchingRemotes: matchingRemotes,
PreferredNames: preferred, MatchingRemotes: matchingRemotes, PullNumber: n,
})
if err != nil {
return clierr.Wrap(clierr.Usage, err)
+1 -1
View File
@@ -63,7 +63,7 @@ func TestPullCleanDeletesLocalAndRemoteBranches(t *testing.T) {
"status --porcelain=v1 --untracked-files=normal": "",
"rev-parse --verify refs/heads/pr/7^{commit}": "abc123",
"rev-parse --verify refs/heads/main^{commit}": "mainsha",
"symbolic-ref --short HEAD": "pr/7",
"symbolic-ref --quiet HEAD": "pr/7",
"switch --quiet -- main": "",
"branch -D -- pr/7": "",
}}
+2 -2
View File
@@ -59,7 +59,7 @@ func TestPullCreateImplicitInteractiveForm(t *testing.T) {
deps.Prompter = prompt
deps.Streams.InTTY, deps.Streams.ErrTTY = true, true
deps.Git = &fakeGit{results: map[string]string{
"symbolic-ref --short HEAD": "feature",
"symbolic-ref --quiet HEAD": "feature",
"rev-parse --is-inside-work-tree": "true",
"rev-parse --verify refs/heads/feature^{commit}": "localsha",
"for-each-ref --format=%(objectname) refs/remotes/origin/feature": "localsha",
@@ -120,7 +120,7 @@ func testPullCreateDefaultsFromForkBranch(t *testing.T, remote string) {
deps, _, _ := serverDeps(t, server.URL)
deps.PullForm = form
deps.Git = &fakeGit{results: map[string]string{
"symbolic-ref --short HEAD": "über-fix",
"symbolic-ref --quiet HEAD": "über-fix",
"config --get branch.über-fix.remote": remote,
"remote get-url " + remote: server.URL + "/bob/project.git",
}}
+3 -3
View File
@@ -74,7 +74,7 @@ func TestPullMergeUsesTheRepositoryDefaultsAndKeepsTheLocalBranch(t *testing.T)
server := newAuthenticatedServer(mergeServer(t, openPull, &payload))
defer server.Close()
deps, out, diagnostics := serverDeps(t, server.URL)
fake := &fakeGit{results: map[string]string{"symbolic-ref --short HEAD": "feature"}}
fake := &fakeGit{results: map[string]string{"symbolic-ref --quiet HEAD": "feature"}}
deps.Git = fake
require.NoError(t, Execute(context.Background(), deps, append([]string{"pr", "merge", "3", "-R", "alice/project", "--yes"}, tc.flags...)))
require.Equal(t, "squash", payload["Do"])
@@ -143,7 +143,7 @@ func TestPullMergeKeepsAHeadBranchThatGiteaKept(t *testing.T) {
}))
defer server.Close()
deps, _, diagnostics := serverDeps(t, server.URL)
deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --short HEAD": "main"}}
deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --quiet HEAD": "main"}}
require.NoError(t, Execute(context.Background(), deps, append([]string{"pr", "merge", "3", "-R", "alice/project", "--yes"}, tc.flags...)))
require.Equal(t, true, payload["delete_branch_after_merge"])
require.Equal(t, "! Kept remote branch feature: Gitea did not delete it (it is protected, the default branch, or used by another open pull request)\n", diagnostics.String())
@@ -234,7 +234,7 @@ func TestPullMergePicksAPullRequestForTheBranch(t *testing.T) {
deps.Getenv = serverEnv(server.URL, map[string]string{"TEA_REPO": "alice/project"})
deps.MergeForm, deps.Prompter = form, &scriptedPrompter{}
deps.Streams.InTTY, deps.Streams.ErrTTY = true, true
deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --short HEAD": "main"}}
deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --quiet HEAD": "main"}}
require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge"}))
require.Equal(t, "squash", payload["Do"])
}
+2 -2
View File
@@ -22,7 +22,7 @@ func TestPullViewResolvesCurrentBranch(t *testing.T) {
_, _ = fmt.Fprint(w, `[]`)
case r.URL.Path == "/api/v1/repos/alice/project/pulls":
require.Equal(t, "open", r.URL.Query().Get("state"))
_, _ = fmt.Fprint(w, `[{"number":9,"title":"Feature","html_url":"https://example.org/pr/9","state":"open","head":{"ref":"feature","sha":"abc"}}]`)
_, _ = fmt.Fprint(w, `[{"number":9,"title":"Feature","html_url":"https://example.org/pr/9","state":"open","head":{"ref":"feature","sha":"abc","repo":{"full_name":"alice/project"}}}]`)
default:
_, _ = fmt.Fprint(w, `[]`)
}
@@ -30,7 +30,7 @@ func TestPullViewResolvesCurrentBranch(t *testing.T) {
defer server.Close()
deps, out, _ := serverDeps(t, server.URL)
deps.Getenv = serverEnv(server.URL, map[string]string{"TEA_REPO": "alice/project"})
deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --short HEAD": "feature"}}
deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --quiet HEAD": "feature"}}
require.NoError(t, Execute(context.Background(), deps, []string{"pr", "view", "--json=number,title"}))
require.JSONEq(t, `{"number":9,"title":"Feature"}`, out.String())
deps.Git = &fakeGit{results: map[string]string{}}
+6 -3
View File
@@ -232,7 +232,7 @@ func (a *application) pullSelector(cmd *cobra.Command, args []string) (target, i
// currentPull finds the pull request of the checked-out branch: the one pr checkout recorded
// while it is open, else the open pull request whose head is the branch's upstream (and the
// upstream remote's owner), else one whose head has the local branch's name, else N from a pr/N
// upstream remote's owner), else one of this repository whose head has the local branch's name, else N from a pr/N
// branch, else the recorded pull request even though it is no longer open.
func (a *application) currentPull(ctx context.Context) (target, int64, error) {
branch, err := git.CurrentBranchConfig(ctx, a.gitProbe(), a.options.cwd)
@@ -263,11 +263,14 @@ func (a *application) currentPull(ctx context.Context) (target, int64, error) {
owner, _, _ = strings.Cut(repo, "/")
}
}
if n, err := pullForBranch(pulls, owner, branch.Merge, t.Repo); n > 0 || err != nil {
if n, err := pullForBranch(pulls, owner, branch.Merge, t.Repo); owner != "" && (n > 0 || err != nil) {
return t, n, err
}
}
if n, err := pullForBranch(pulls, "", branch.Name, t.Repo); n > 0 || err != nil {
// A bare branch name only identifies the repository's own pull requests: a contributor's
// fork may use main or fix as its head branch too.
own := slices.DeleteFunc(slices.Clone(pulls), func(pull services.Pull) bool { return !strings.EqualFold(pull.HeadRepo, t.Repo) })
if n, err := pullForBranch(own, "", branch.Name, t.Repo); n > 0 || err != nil {
return t, n, err
}
if number, ok := strings.CutPrefix(branch.Name, "pr/"); ok {
+6 -4
View File
@@ -77,7 +77,8 @@ func refApplication(t *testing.T, gitResults map[string]string) (*application, s
{"number":4,"head":{"ref":"feat/two","repo":{"full_name":"alice/project"}}},
{"number":17,"head":{"ref":"fix","repo":{"full_name":"bob/project"}}},
{"number":20,"head":{"ref":"dup","repo":{"full_name":"alice/project"}}},
{"number":21,"head":{"ref":"dup","repo":{"full_name":"carol/project"}}}
{"number":21,"head":{"ref":"dup","repo":{"full_name":"carol/project"}}},
{"number":22,"head":{"ref":"main","repo":{"full_name":"bob/project"}}}
]`)
}))
t.Cleanup(server.Close)
@@ -103,7 +104,7 @@ func commandContext() *cobra.Command {
}
func TestPullSelectorDefaultsToTheCheckedOutBranch(t *testing.T) {
const head = "symbolic-ref --short HEAD"
const head = "symbolic-ref --quiet HEAD"
branch := func(name string, keys map[string]string) map[string]string {
results := map[string]string{head: name}
for key, value := range keys {
@@ -119,15 +120,16 @@ func TestPullSelectorDefaultsToTheCheckedOutBranch(t *testing.T) {
}{
{"recorded by pr checkout", branch("pr/17", map[string]string{"tea-pr": "17", "tea-base-remote": "origin", "remote": "pulls/bob/project", "merge": "refs/heads/fix"}), 17, ""},
{"head branch name kept by pr checkout", branch("fix", map[string]string{"tea-pr": "17", "tea-base-remote": "origin"}), 17, ""},
{"recorded but merged, the head branch has a newer one", branch("fix", map[string]string{"tea-pr": "30", "tea-base-remote": "origin"}), 17, ""},
{"recorded but merged, the head branch has a newer one", branch("fix", map[string]string{"tea-pr": "30", "tea-base-remote": "origin", "remote": "pulls/bob/project", "merge": "refs/heads/fix"}), 17, ""},
{"recorded but merged, no open one", branch("gone", map[string]string{"tea-pr": "30", "tea-base-remote": "origin"}), 30, ""},
{"recorded for another repository", branch("pr/9", map[string]string{"tea-pr": "9", "tea-base-remote": "fork", "remote": "fork", "merge": "refs/heads/fix"}), 17, ""},
{"upstream branch and owner", branch("local", map[string]string{"remote": "origin", "merge": "refs/heads/feat/one"}), 3, ""},
{"upstream owner tells forks apart", branch("dup", map[string]string{"remote": "pulls/carol/project", "merge": "refs/heads/dup"}), 21, ""},
{"local branch name", branch("feat/two", nil), 4, ""},
{"pr/N branch", branch("pr/99", nil), 99, ""},
{"fork pull request from a branch named like the local one", branch("main", map[string]string{"remote": "origin", "merge": "refs/heads/main"}), 0, `no open pull request for branch "main" in alice/project; pass a number, URL or branch`},
{"no pull request", branch("main", nil), 0, `no open pull request for branch "main" in alice/project; pass a number, URL or branch`},
{"ambiguous branch", branch("dup", nil), 0, `branch "dup" has several open pull requests in alice/project (#20, #21); pass a number`},
{"same-named fork branch is ignored", branch("dup", nil), 20, ""},
{"detached", map[string]string{}, 0, "not on a branch; pass a pull request number, URL or branch"},
} {
t.Run(tc.name, func(t *testing.T) {
+2 -2
View File
@@ -17,7 +17,7 @@ import (
// upstreamCheckout discovers the repository from the upstream remote, as in a clone of a fork.
func upstreamCheckout(server *httptest.Server) *fakeGit {
return &fakeGit{results: map[string]string{
"symbolic-ref --short HEAD": "main",
"symbolic-ref --quiet HEAD": "main",
"config --get branch.main.remote": "upstream",
"remote get-url upstream": server.URL + "/alice/project.git",
}}
@@ -135,7 +135,7 @@ func TestRepoRenameHintsAtTheRemote(t *testing.T) {
defer server.Close()
deps, out, diagnostics := repoTTYDeps(t, server.URL)
deps.Git = &fakeGit{results: map[string]string{
"symbolic-ref --short HEAD": "main",
"symbolic-ref --quiet HEAD": "main",
"config --get branch.main.remote": "origin",
"remote get-url origin": server.URL + "/alice/project.git",
}}
+5 -5
View File
@@ -46,16 +46,16 @@ pull requests you created, those assigned to you, and those that request your re
}
return a.writeHuman(cmd, strings.TrimSuffix(currentBranchSection(d, branch, current, details)+pullStatusText(d, result, t.Repo), "\n"))
}
runsRepo, runsSource := t.Repo, t.RepoSource
runsRepo, runsSource, discovered := t.Repo, t.RepoSource, false
if runs && runsRepo == "" {
if discovered, err := a.resolveTarget(cmd.Context(), true, false); err == nil && discovered.Host == t.Host {
runsRepo, runsSource = discovered.Repo, discovered.RepoSource
if found, err := a.resolveTarget(cmd.Context(), true, false); err == nil && found.Host == t.Host {
runsRepo, runsSource, discovered = found.Repo, found.RepoSource, true
}
}
if !runs {
runsRepo = ""
}
result, err := service.Summary(cmd.Context(), limit, runsRepo)
result, err := service.Summary(cmd.Context(), limit, runsRepo, discovered)
if err != nil {
return err
}
@@ -68,7 +68,7 @@ pull requests you created, those assigned to you, and those that request your re
statusSection(d, "Review requests", result.PullRequests.ReviewRequested, t.Repo) +
statusSection(d, "Mentions", result.Mentions, t.Repo)
if result.RunsIncluded {
heading := "Failing runs in " + result.Repository
heading := "Failing runs in " + result.RunsRepository
if runsSource != "" && t.Repo == "" {
heading += " (from " + strings.Replace(runsSource, "remote ", "git remote ", 1) + ")"
}
+28 -1
View File
@@ -97,7 +97,7 @@ func TestPullStatusShowsTheCurrentBranch(t *testing.T) {
deps, out, _ := serverDeps(t, server.URL)
plainTTY(t, &deps, server.URL)
deps.Getenv = serverEnv(server.URL, map[string]string{"NO_COLOR": "1", "TEA_REPO": "alice/project"})
deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --short HEAD": "feat/one", "config --get branch.feat/one.tea-pr": "1"}}
deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --quiet HEAD": "feat/one", "config --get branch.feat/one.tea-pr": "1"}}
require.NoError(t, Execute(context.Background(), deps, []string{"pr", "status"}))
require.Equal(t, `Current branch
#1 Add one.txt [feat/one]
@@ -113,3 +113,30 @@ Requesting a code review from you
Nothing here
`, stripLinks(out.String()))
}
func TestStatusInACloneListsItsRunsWithoutFailingOnAnUnreadableRepository(t *testing.T) {
readable := true
server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch {
case r.URL.Path == "/api/v1/repos/alice/project" && readable:
_, _ = w.Write([]byte(`{"has_actions":true}`))
case r.URL.Path == "/api/v1/repos/alice/project":
w.WriteHeader(http.StatusNotFound)
case strings.HasSuffix(r.URL.Path, "/actions/runs"):
_, _ = w.Write([]byte(`{"workflow_runs":[{"id":7,"display_title":"Broken build","status":"completed","conclusion":"failure"}]}`))
default:
_, _ = w.Write([]byte(`[]`))
}
}))
defer server.Close()
deps, out, _ := serverDeps(t, server.URL)
deps.Git = &fakeGit{results: map[string]string{"config --get tea.repository": server.URL + "/alice/project"}}
require.NoError(t, Execute(context.Background(), deps, []string{"status", "--json=repository,runs_repository,runs_included"}))
require.JSONEq(t, `{"repository":"","runs_repository":"alice/project","runs_included":true}`, out.String())
out.Reset()
readable = false
require.NoError(t, Execute(context.Background(), deps, []string{"status", "--json=repository,runs_repository,runs_included"}))
require.JSONEq(t, `{"repository":"","runs_repository":"","runs_included":false}`, out.String())
out.Reset()
require.EqualError(t, Execute(context.Background(), deps, []string{"status", "-R", "alice/project", "--json=runs_included"}), "repository alice/project not found")
}
+1 -1
View File
@@ -205,7 +205,7 @@ func TestRepositoryResolutionPrecedence(t *testing.T) {
}
deps.Getenv = envFunc(map[string]string{"TEA_HOST": "https://example.org", "GITEA_TOKEN": "secret"})
runner := &fakeGit{results: map[string]string{
"symbolic-ref --short HEAD": "main",
"symbolic-ref --quiet HEAD": "main",
"config --get branch.main.remote": "feature-remote",
"remote get-url feature-remote": "https://example.org/owner/current.git",
"remote get-url upstream": "https://example.org/owner/upstream.git",
+15 -3
View File
@@ -29,13 +29,25 @@ type BranchConfig struct {
// CurrentBranchConfig reads the configuration of the checked-out branch, failing with
// ErrNotOnBranch when HEAD is detached or dir is not a checkout.
func CurrentBranchConfig(ctx context.Context, runner Runner, dir string) (BranchConfig, error) {
name, err := runner.Output(ctx, dir, false, "symbolic-ref", "--short", "HEAD")
if err != nil || name == "" {
return BranchConfig{}, ErrNotOnBranch
name, err := CurrentBranch(ctx, runner, dir)
if err != nil {
return BranchConfig{}, err
}
return ReadBranchConfig(ctx, runner, dir, name), nil
}
// CurrentBranch names the checked-out branch, failing with ErrNotOnBranch when HEAD is detached
// or dir is not a checkout. It reads the full ref because --short answers heads/NAME for a branch
// that shares its name with a tag.
func CurrentBranch(ctx context.Context, runner Runner, dir string) (string, error) {
ref, err := runner.Output(ctx, dir, false, "symbolic-ref", "--quiet", "HEAD")
name := strings.TrimPrefix(ref, "refs/heads/")
if err != nil || name == "" {
return "", ErrNotOnBranch
}
return name, nil
}
// ReadBranchConfig reads branch.<name>.remote, .merge and the keys pr checkout records.
// Missing or malformed keys stay empty.
func ReadBranchConfig(ctx context.Context, runner Runner, dir, name string) BranchConfig {
+17 -1
View File
@@ -5,13 +5,14 @@ package git
import (
"context"
"os/exec"
"testing"
"github.com/stretchr/testify/require"
)
func TestCurrentBranchConfig(t *testing.T) {
const head = "symbolic-ref --short HEAD"
const head = "symbolic-ref --quiet HEAD"
for _, test := range []struct {
name string
runner discoverRunner
@@ -39,3 +40,18 @@ func TestCurrentBranchConfig(t *testing.T) {
})
}
}
func TestCurrentBranchIgnoresATagWithTheSameName(t *testing.T) {
dir := t.TempDir()
for _, args := range [][]string{
{"init", "--quiet", "--initial-branch=rel-2"},
{"-c", "user.name=t", "-c", "user.email=t@example.test", "commit", "--quiet", "--allow-empty", "--message=x"},
{"tag", "rel-2"},
} {
out, err := exec.Command("git", append([]string{"-C", dir}, args...)...).CombinedOutput()
require.NoError(t, err, string(out))
}
name, err := CurrentBranch(context.Background(), System{}, dir)
require.NoError(t, err)
require.Equal(t, "rel-2", name)
}
+13 -5
View File
@@ -7,6 +7,7 @@ import (
"context"
"errors"
"fmt"
"slices"
"strings"
)
@@ -34,6 +35,9 @@ type FindCleanBranchOptions struct {
DefaultBranch string
PreferredNames, MatchingRemotes []string
IgnoreSHA bool
// PullNumber is the pull request being cleaned; a branch that pr checkout recorded for it
// wins over branches that merely share the head branch's name.
PullNumber int64
}
// FindCleanBranch locates one local branch for PR cleanup without mutating Git state.
@@ -50,7 +54,7 @@ func FindCleanBranch(ctx context.Context, runner Runner, opts FindCleanBranchOpt
}
type candidate struct {
name, remote string
matched bool
matched, recorded bool
}
setOf := func(names []string) map[string]bool {
out := map[string]bool{}
@@ -80,8 +84,9 @@ func FindCleanBranch(ctx context.Context, runner Runner, opts FindCleanBranchOpt
if len(parts) == 3 {
upstream = parts[2]
}
recorded := opts.PullNumber > 0 && ReadBranchConfig(ctx, runner, opts.Directory, name).PullNumber == opts.PullNumber
if opts.IgnoreSHA {
if name != opts.HeadBranch && !preferred[name] {
if name != opts.HeadBranch && !preferred[name] && !recorded {
continue
}
} else if opts.HeadSHA == "" || sha != opts.HeadSHA {
@@ -107,7 +112,7 @@ func FindCleanBranch(ctx context.Context, runner Runner, opts FindCleanBranchOpt
remoteName = remote
}
}
candidates = append(candidates, candidate{name: name, remote: remoteName, matched: matched})
candidates = append(candidates, candidate{name: name, remote: remoteName, matched: matched, recorded: recorded})
}
if len(candidates) == 0 {
if opts.IgnoreSHA {
@@ -119,13 +124,16 @@ Retry with --ignore-sha to select by branch name`, opts.HeadSHA)
}
selected := []candidate{}
for _, item := range candidates {
if item.matched || preferred[item.name] || item.name == opts.HeadBranch {
if item.matched || item.recorded || preferred[item.name] || item.name == opts.HeadBranch {
selected = append(selected, item)
}
}
if len(selected) == 0 {
selected = candidates
}
if recorded := slices.DeleteFunc(slices.Clone(selected), func(item candidate) bool { return !item.recorded }); len(recorded) > 0 {
selected = recorded
}
if len(selected) > 1 {
strong := make([]candidate, 0, len(selected))
for _, item := range selected {
@@ -183,7 +191,7 @@ func CleanPull(ctx context.Context, runner Runner, opts CleanOptions) (CleanResu
if _, err := run("rev-parse", "--verify", "refs/heads/"+opts.DefaultBranch+"^{commit}"); err != nil {
return CleanResult{}, fmt.Errorf("default branch is unavailable; create or fetch it before cleaning: %w", err)
}
current, err := run("symbolic-ref", "--short", "HEAD")
current, err := CurrentBranch(ctx, runner, opts.Directory)
if err == nil && current == opts.LocalBranch {
if _, err := run("switch", "--quiet", "--", opts.DefaultBranch); err != nil {
return CleanResult{}, fmt.Errorf("could not switch to %s before deleting %s: %w", opts.DefaultBranch, opts.LocalBranch, err)
+24 -1
View File
@@ -15,7 +15,7 @@ type cleanRunner struct {
calls []string
dirty, missingLocal, missingDefault, onFeature bool
branches string
remoteTracking map[string]string
remoteTracking, config map[string]string
}
func (r *cleanRunner) Output(_ context.Context, dir string, prompt bool, args ...string) (string, error) {
@@ -44,6 +44,11 @@ func (r *cleanRunner) Output(_ context.Context, dir string, prompt bool, args ..
}
return "", &CommandError{ExitCode: 1}
}
case "config":
if value, ok := r.config[args[2]]; ok {
return value, nil
}
return "", &CommandError{ExitCode: 1}
case "status":
if r.dirty {
return " M file", nil
@@ -81,6 +86,24 @@ func TestFindCleanBranchIgnoreSHAUsesName(t *testing.T) {
require.Equal(t, "origin", remote)
}
func TestFindCleanBranchIgnoreSHAPrefersTheBranchRecordedForThePull(t *testing.T) {
runner := &cleanRunner{
branches: "aaa111\tfeature\t\nbbb222\tbob/feature\t\n",
config: map[string]string{"branch.bob/feature.tea-pr": "1"},
}
options := FindCleanBranchOptions{
Directory: "/work", HeadSHA: "abc123", HeadBranch: "feature", IgnoreSHA: true, PullNumber: 1, MatchingRemotes: []string{"origin"},
}
branch, _, err := FindCleanBranch(context.Background(), runner, options)
require.NoError(t, err)
require.Equal(t, "bob/feature", branch)
// Without the record, OWNER/BRANCH is only preferred by name and the unrelated branch stays a candidate.
runner.config = nil
options.PreferredNames = []string{"bob/feature"}
_, _, err = FindCleanBranch(context.Background(), runner, options)
require.ErrorContains(t, err, "multiple local branches")
}
func TestFindCleanBranchRejectsAmbiguity(t *testing.T) {
runner := &cleanRunner{branches: "abc123\tone\t\nabc123\ttwo\t\n"}
_, _, err := FindCleanBranch(context.Background(), runner, FindCleanBranchOptions{
+1 -1
View File
@@ -92,7 +92,7 @@ func DiscoverRemote(ctx context.Context, runner Runner, dir, preferred string, r
// CurrentUpstreamRemote names the checked-out branch and the remote it tracks; each is "" when
// HEAD is detached or the branch tracks no remote.
func CurrentUpstreamRemote(ctx context.Context, runner Runner, dir string) (branch, remote string) {
branch, err := runner.Output(ctx, dir, false, "symbolic-ref", "--short", "HEAD")
branch, err := CurrentBranch(ctx, runner, dir)
if err != nil {
return "", ""
}
+2 -2
View File
@@ -23,7 +23,7 @@ func (r discoverRunner) Output(_ context.Context, dir string, prompt bool, args
}
func TestDiscoverRemote(t *testing.T) {
const head, tracking, base = "symbolic-ref --short HEAD", "config --get branch.main.remote", "config --get branch.main.tea-base-remote"
const head, tracking, base = "symbolic-ref --quiet HEAD", "config --get branch.main.remote", "config --get branch.main.tea-base-remote"
for _, test := range []struct {
name string
runner discoverRunner
@@ -55,7 +55,7 @@ func TestDiscoverRemote(t *testing.T) {
}
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"
const head, tracking, base = "symbolic-ref --quiet 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 {
+10
View File
@@ -7,6 +7,7 @@ import (
"context"
"errors"
"net/http"
"strconv"
"strings"
gitea "gitea.dev/sdk"
@@ -78,6 +79,10 @@ func (s Branches) Create(ctx context.Context, repo, branch, source string) (Bran
var created gitea.Branch
payload := map[string]string{"new_branch_name": branch, "old_ref_name": source}
if err := sendJSON(ctx, s.Client, http.MethodPost, "repos/"+repo+"/branches", payload, &created); err != nil {
// Gitea answers an unknown source with a 500 instead of a 404.
if apiErr, ok := errors.AsType[*transport.APIError](err); ok && apiErr.Status == http.StatusInternalServerError && strings.Contains(apiErr.Message, "object does not exist") {
return Branch{}, &NotFoundError{Kind: "branch, tag or commit", Key: strconv.Quote(source), Scope: repo, Err: err}
}
return Branch{}, err
}
return branchFrom(&created), nil
@@ -97,6 +102,11 @@ func (s Branches) Delete(ctx context.Context, repo, branch string) error {
_, resp, err := c.Repositories.DeleteRepoBranch(ctx, owner, name, branch)
return resp, err
})
if IsNotFound(err) {
if _, repoErr := (Repositories{Client: s.Client}).get(ctx, repo); repoErr != nil {
return repoErr
}
}
return Classify(err, "branch", branch, repo)
}
+14 -4
View File
@@ -25,11 +25,14 @@ type PullStatus struct {
type AccountStatus struct {
Host string `json:"host"`
User string `json:"user"`
// Repository is the repository the summary is scoped to, empty for all repositories.
Repository string `json:"repository"`
AssignedIssues []SearchItem `json:"assigned_issues"`
PullRequests PullStatus `json:"pull_requests"`
// Mentions are the open issues and pull requests that mention the account.
Mentions []SearchItem `json:"mentions"`
// RunsRepository is the repository whose failing runs are listed, when RunsIncluded.
RunsRepository string `json:"runs_repository"`
FailingRuns []Run `json:"failing_runs"`
RunsIncluded bool `json:"runs_included"`
}
@@ -88,9 +91,10 @@ func (s Status) Pulls(ctx context.Context, limit int) (PullStatus, error) {
}
// Summary reports open assigned issues and pull requests and mentions. Failing Actions runs of
// runsRepository are included when it is set and has Actions enabled.
func (s Status) Summary(ctx context.Context, limit int, runsRepository string) (AccountStatus, error) {
result := AccountStatus{Host: s.Client.BaseURL, Repository: runsRepository, AssignedIssues: []SearchItem{}, Mentions: []SearchItem{}, FailingRuns: []Run{}}
// runsRepository are included when it is set and has Actions enabled. A runsRepository that was
// only discovered from a clone is left out when the account cannot read it, instead of failing.
func (s Status) Summary(ctx context.Context, limit int, runsRepository string, discovered bool) (AccountStatus, error) {
result := AccountStatus{Host: s.Client.BaseURL, Repository: s.Repository, AssignedIssues: []SearchItem{}, Mentions: []SearchItem{}, FailingRuns: []Run{}}
if limit < 1 || limit > 100 {
return result, fmt.Errorf("status limit must be 1..100")
}
@@ -120,6 +124,9 @@ func (s Status) Summary(ctx context.Context, limit int, runsRepository string) (
if runsRepository != "" {
repository, err := (Repositories{Client: s.Client}).View(ctx, runsRepository)
if err != nil {
if discovered && (IsNotFound(err) || IsForbidden(err)) {
return result, nil
}
return result, err
}
if !repository.ActionsEnabled {
@@ -127,9 +134,12 @@ func (s Status) Summary(ctx context.Context, limit int, runsRepository string) (
}
failing, err := (Actions{Client: s.Client, Repository: runsRepository}).Runs(ctx, RunListOptions{Limit: limit, Page: 1, Statuses: []string{"failure", "timed_out"}})
if err != nil {
if discovered && (IsNotFound(err) || IsForbidden(err)) {
return result, nil
}
return result, err
}
result.FailingRuns, result.RunsIncluded = failing, true
result.RunsRepository, result.FailingRuns, result.RunsIncluded = runsRepository, failing, true
}
return result, nil
}
+1 -1
View File
@@ -71,7 +71,7 @@ func TestStatusSummarySkipsRunsWhenActionsAreDisabled(t *testing.T) {
w.WriteHeader(http.StatusTeapot)
}
}))
result, err := (Status{Client: client, Repository: "alice/project"}).Summary(context.Background(), 30, "alice/project")
result, err := (Status{Client: client, Repository: "alice/project"}).Summary(context.Background(), 30, "alice/project", false)
require.NoError(t, err)
require.False(t, result.RunsIncluded)
require.Empty(t, result.FailingRuns)