fix: address verified review findings (repo-mutations-sync-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 0d47b3f149
commit 6e523d969d
11 files changed
+283 -30

No files matched your search

+8
View File
@@ -286,6 +286,14 @@ func TestCommandHelpShowsRelevantFlagsOnly(t *testing.T) {
}
require.NotContains(t, out.String(), "--json fields", "version prints text")
out.Reset()
require.NoError(t, Execute(context.Background(), deps, []string{"repo", "clone", "--help"}))
require.NotContains(t, out.String(), "--repo")
require.NotContains(t, out.String(), "--remote ")
require.NotContains(t, out.String(), "Without an argument")
out.Reset()
require.NoError(t, Execute(context.Background(), deps, []string{"repo", "fork", "--help"}))
require.Equal(t, 1, strings.Count(out.String(), "\n --remote "), "a local --remote hides the global one")
out.Reset()
require.NoError(t, Execute(context.Background(), deps, []string{"issue", "--help"}))
require.Contains(t, out.String(), "\nUSAGE\n tea issue <command> [flags]\n\nAVAILABLE COMMANDS\n attachment: ")
require.NotContains(t, out.String(), "JSON FIELDS")
+10 -8
View File
@@ -97,23 +97,25 @@ func TestCloneAddsUpstreamRemoteForForks(t *testing.T) {
parent = server.URL + "/alice/project.git"
runner := &fakeGit{results: map[string]string{
prefix + "clone -- " + remote + " destination": "",
"remote add -- upstream " + parent: "",
"config branch.main.remote upstream": "",
"config branch.main.merge refs/heads/main": "",
"config branch.main.pushRemote origin": "",
"remote": "origin",
"remote add -- upstream " + parent: "",
"config branch.main.remote upstream": "",
"config branch.main.merge refs/heads/main": "",
"config branch.main.pushRemote origin": "",
}}
deps.Git = runner
cwd := t.TempDir()
require.NoError(t, Execute(context.Background(), deps, []string{"repo", "clone", "bob/project", "destination", "--cwd", cwd, "--no-input", "--json=id,full_name,fork"}))
require.Equal(t, []string{
prefix + "clone -- " + remote + " destination",
"remote",
"remote add -- upstream " + parent,
"config branch.main.remote upstream",
"config branch.main.merge refs/heads/main",
"config branch.main.pushRemote origin",
}, runner.calls)
clone := filepath.Join(cwd, "destination")
require.Equal(t, []string{cwd, clone, clone, clone, clone}, runner.dirs)
require.Equal(t, []string{cwd, clone, clone, clone, clone, clone}, runner.dirs)
require.JSONEq(t, `{"id":8,"full_name":"bob/project","fork":true}`, out.String())
}
@@ -188,12 +190,12 @@ func TestClonePassesGitFlagsAndNamesTheUpstreamRemote(t *testing.T) {
clone := credentialCall(t, deps, server.URL, "alice", "clone", "--branch", "dev", "--", server.URL+"/bob/project.git")
parent := server.URL + "/alice/project.git"
runner := &fakeGit{results: map[string]string{
clone: "", "remote add -- parent " + parent: "", "config branch.main.remote parent": "",
"config branch.main.merge refs/heads/main": "", "config branch.main.pushRemote origin": "",
clone: "", "remote": "fork", "remote add -- parent " + parent: "", "config branch.main.remote parent": "",
"config branch.main.merge refs/heads/main": "", "config branch.main.pushRemote fork": "",
}}
deps.Git = runner
require.NoError(t, Execute(context.Background(), deps, []string{"repo", "clone", "bob/project", "-u", "parent", "--", "--branch", "dev"}))
require.Equal(t, []string{clone, "remote add -- parent " + parent, "config branch.main.remote parent", "config branch.main.merge refs/heads/main", "config branch.main.pushRemote origin"}, runner.calls)
require.Equal(t, []string{clone, "remote", "remote add -- parent " + parent, "config branch.main.remote parent", "config branch.main.merge refs/heads/main", "config branch.main.pushRemote fork"}, runner.calls)
require.Equal(t, "✓ Cloned bob/project into project\n✓ Added remote parent for alice/project; tea commands in the clone target it (change with tea repo set-default)\n", diagnostics.String())
runner.calls = nil
require.NoError(t, Execute(context.Background(), deps, []string{"repo", "clone", "bob/project", "--no-upstream", "--", "--branch", "dev"}))
+4 -1
View File
@@ -168,7 +168,10 @@ func helpFlags(cmd *cobra.Command) (local, inherited string) {
slices.SortFunc(own, func(x, y *pflag.Flag) int { return strings.Compare(x.Name, y.Name) })
path := cmd.CommandPath()
repo := !slices.ContainsFunc(noRepoCommands, func(prefix string) bool { return path == prefix || strings.HasPrefix(path, prefix+" ") })
shown := map[string]bool{"repo": repo, "remote": repo, "cwd": repo, "json": outputsJSON(cmd), "jq": outputsJSON(cmd) || cmd.Annotations["tea.rawjson"] != "", "yes": cmd.Annotations["tea.confirm"] != ""}
// A command's own --remote, as in repo fork, hides the global flag of that name.
// repo clone takes its repository as an argument only, so --repo and --remote do nothing there.
selects := repo && path != "tea repo clone"
shown := map[string]bool{"repo": selects, "remote": selects && cmd.LocalFlags().Lookup("remote") == nil, "cwd": repo, "json": outputsJSON(cmd), "jq": outputsJSON(cmd) || cmd.Annotations["tea.rawjson"] != "", "yes": cmd.Annotations["tea.confirm"] != ""}
var shared []*pflag.Flag
for _, name := range []string{"repo", "remote", "cwd", "json", "jq", "yes"} {
if shown[name] {
+66 -12
View File
@@ -9,6 +9,7 @@ import (
"fmt"
"net/url"
"path/filepath"
"regexp"
"strconv"
"strings"
@@ -95,6 +96,9 @@ var repositorySwitches = []struct {
{"archived", "Archive the repository (=false unarchives it)", func(e *services.RepositoryEdit) **bool { return &e.Archived }},
}
// validTopic is the form Gitea accepts for a lowercased topic name.
var validTopic = regexp.MustCompile(`^[a-z0-9][-.a-z0-9]{0,34}$`)
func (a *application) repoEditCommand() *cobra.Command {
var name, website, defaultBranch, description, visibility, mergeStyle string
var addTopics, removeTopics []string
@@ -153,6 +157,11 @@ $ tea repo edit --default-merge-style squash --add-topic cli --add-topic go`,
if err := requireNames("topic", "", addTopics, removeTopics); err != nil {
return err
}
for _, topic := range addTopics {
if !validTopic.MatchString(strings.ToLower(topic)) {
return clierr.Wrap(clierr.Usage, fmt.Errorf("invalid topic %q: use up to 35 letters, digits, - or ., starting with a letter or digit", topic))
}
}
if options == (services.RepositoryEdit{}) && len(addTopics)+len(removeTopics) == 0 {
return clierr.Wrap(clierr.Usage, errors.New("supply a setting to change; see tea repo edit --help"))
}
@@ -163,13 +172,23 @@ $ tea repo edit --default-merge-style squash --add-topic cli --add-topic go`,
service := services.Repositories{Client: t.client}
if options.DefaultBranch != nil {
if _, err := (services.Branches{Client: t.client}).View(cmd.Context(), t.Repo, defaultBranch); err != nil {
return services.Classify(err, "branch", strconv.Quote(defaultBranch), t.Repo)
// An empty repository has no branches, and Gitea accepts any default branch for it.
if !services.IsNotFound(err) {
return services.Classify(err, "branch", strconv.Quote(defaultBranch), t.Repo)
}
if item, viewErr := service.View(cmd.Context(), t.Repo); viewErr != nil || !item.Empty {
return services.Classify(err, "branch", strconv.Quote(defaultBranch), t.Repo)
}
}
}
// Gitea refuses topic changes on an archived repository, so archiving comes last.
repo, archive := t.Repo, options.Archived != nil && *options.Archived
if archive {
if err := a.confirmAction(cmd, terminal.Confirmation{Prompt: "Archive " + t.Repo + "? It becomes read-only."}); err != nil {
inferred, err := a.inferredRepository(t, "archive", true)
if err != nil {
return err
}
if err := a.confirmAction(cmd, terminal.Confirmation{Prompt: "Archive " + t.Repo + inferred + "? It becomes read-only."}); err != nil {
return err
}
options.Archived = nil
@@ -187,7 +206,8 @@ $ tea repo edit --default-merge-style squash --add-topic cli --add-topic go`,
}
}
for _, topic := range removeTopics {
if err := service.RemoveTopic(cmd.Context(), repo, topic); err != nil {
// A topic the repository does not have is already removed.
if err := service.RemoveTopic(cmd.Context(), repo, topic); err != nil && !services.IsNotFound(err) {
return fmt.Errorf("remove topic %q: %w", topic, err)
}
}
@@ -233,6 +253,9 @@ $ tea repo edit --default-merge-style squash --add-topic cli --add-topic go`,
return command
}
// cloneArguments documents the <repository> argument of repo clone, which is required.
const cloneArguments = "A repository can be supplied as NAME (one of yours), OWNER/NAME, HOST/OWNER/NAME or a URL."
func (a *application) repoCloneCommand() *cobra.Command {
var opts cloneOptions
var noUpstream bool
@@ -251,7 +274,7 @@ $ tea repo clone alice/project src --depth 1
# Pass flags to git clone
$ tea repo clone alice/project -- --branch develop --recurse-submodules`,
Annotations: map[string]string{"tea.arguments": repositoryArguments},
Annotations: map[string]string{"tea.arguments": cloneArguments},
Args: argsBeforeGitFlags(1, 2),
RunE: func(cmd *cobra.Command, args []string) error {
at := argsBeforeDash(cmd, args)
@@ -337,6 +360,12 @@ $ tea repo fork alice/project --clone -- --depth 1`,
if strings.EqualFold(owner, cmp.Or(opts.Organization, t.User)) {
return clierr.Wrap(clierr.Usage, fmt.Errorf("cannot fork %s into %s: it already owns it; pass --org to fork it into an organization", t.Repo, owner))
}
rename := false
if addRemote {
if rename, err = a.checkForkRemote(cmd, t, remoteName); err != nil {
return err
}
}
fork, existed, err := (services.Repositories{Client: t.client}).Fork(cmd.Context(), t.Repo, opts)
if err != nil {
return err
@@ -352,7 +381,7 @@ $ tea repo fork alice/project --clone -- --depth 1`,
}
}
if addRemote {
if err := a.addForkRemote(cmd, t, fork, remoteName); err != nil {
if err := a.addForkRemote(cmd, t, fork, remoteName, rename); err != nil {
return err
}
}
@@ -369,17 +398,28 @@ $ tea repo fork alice/project --clone -- --depth 1`,
return command
}
// addForkRemote adds fork as the remote name of the clone in the working directory. A remote of
// that name that points to the forked repository is renamed to upstream first.
func (a *application) addForkRemote(cmd *cobra.Command, t target, fork services.Repository, name string) error {
// checkForkRemote reports, before anything is forked, whether the clone in the working directory
// can take the fork as the remote name, and whether a remote of that name already points to the
// forked repository and must be renamed to upstream first.
func (a *application) checkForkRemote(cmd *cobra.Command, t target, name string) (rename bool, err error) {
ctx := cmd.Context()
if inside, _ := a.gitProbe().Output(ctx, a.options.cwd, false, "rev-parse", "--is-inside-work-tree"); inside != "true" {
return clierr.Wrap(clierr.Usage, fmt.Errorf("forked to %s, but --remote must run inside a Git clone of %s", fork.FullName, t.Repo))
return false, clierr.Wrap(clierr.Usage, fmt.Errorf("--remote must run inside a Git clone of %s", t.Repo))
}
if existing, err := a.gitProbe().Output(ctx, a.options.cwd, false, "remote", "get-url", name); err == nil {
if !t.ownsRemote(existing) {
return clierr.Wrap(clierr.Usage, fmt.Errorf("forked to %s, but Git remote %s points elsewhere; pass --remote-name", fork.FullName, name))
return false, clierr.Wrap(clierr.Usage, fmt.Errorf("Git remote %s points elsewhere; pass --remote-name", name))
}
return true, nil
}
return false, nil
}
// addForkRemote adds fork as the remote name of the clone in the working directory, after renaming
// the remote of that name to upstream when rename is set.
func (a *application) addForkRemote(cmd *cobra.Command, t target, fork services.Repository, name string, rename bool) error {
ctx := cmd.Context()
if rename {
if _, err := a.git(ctx, "remote", "rename", name, "upstream"); err != nil {
return fmt.Errorf("forked to %s, but could not rename Git remote %s to upstream: %w", fork.FullName, name, err)
}
@@ -475,12 +515,18 @@ func (a *application) cloneRepository(cmd *cobra.Command, t target, item service
if err != nil || upstreamHost != t.Host || !strings.EqualFold(upstreamRepo, item.Parent.FullName) || (protocol == "ssh") != upstreamSSH {
return clone, errors.New("cloned the fork, but its parent clone URL does not match the selected instance")
}
// Git flags or clone.defaultRemoteName may have renamed origin.
origin, err := a.gitIn(cmd.Context(), clone, "remote")
if err != nil {
return clone, fmt.Errorf("cloned the fork, but could not list its remotes: %w", err)
}
origin, _, _ = strings.Cut(origin, "\n")
if _, err := a.gitIn(cmd.Context(), clone, "remote", "add", "--", opts.upstream, upstream); err != nil {
return clone, fmt.Errorf("cloned the fork, but could not add the %s remote: %w", opts.upstream, err)
}
// The default branch pulls from upstream, but pushes stay on the fork.
if branch := item.Parent.DefaultBranch; branch != "" {
for _, setting := range [][2]string{{"remote", opts.upstream}, {"merge", "refs/heads/" + branch}, {"pushRemote", "origin"}} {
for _, setting := range [][2]string{{"remote", opts.upstream}, {"merge", "refs/heads/" + branch}, {"pushRemote", origin}} {
if _, err := a.gitIn(cmd.Context(), clone, "config", "branch."+branch+"."+setting[0], setting[1]); err != nil {
return clone, fmt.Errorf("cloned the fork and added %s, but could not configure branch tracking: %w", opts.upstream, err)
}
@@ -581,7 +627,7 @@ func (a *application) repoRenameCommand() *cobra.Command {
return &cobra.Command{
Use: "rename <new-name>",
Short: "Rename a repository",
Long: "Rename a repository; its owner stays the same. Gitea redirects the old name to the new one.",
Long: "Rename a repository; its owner stays the same. Git remotes keep working through Gitea's redirect from the old name, but tea commands do not;\na default repository saved with tea repo set-default is updated.",
Example: `# Rename the repository of the current clone
$ tea repo rename new-name
@@ -619,6 +665,14 @@ $ tea repo rename new-name -R alice/old-name`,
if remote, ok := strings.CutPrefix(t.RepoSource, "remote "); ok {
a.warn(cmd, "Git remote %s still points to %s; update it with git remote set-url", remote, t.Repo)
}
if t.RepoSource == "git config tea.repository" {
// tea's own setting; the old name would fail with HTTP 301 from now on.
if _, err := a.git(cmd.Context(), "config", "--local", "tea.repository", t.Host+"/"+item.FullName); err != nil {
a.warn(cmd, "tea.repository still points to %s; update it with tea repo set-default %s", t.Repo, item.FullName)
} else {
a.done(cmd, "Updated the default repository of this clone to %s", item.FullName)
}
}
return a.jsonResult(cmd, item)
},
}
+33
View File
@@ -200,3 +200,36 @@ func TestRepoForkAddsTheForkAsOrigin(t *testing.T) {
require.Equal(t, "✓ Forked bob/project to alice/mine\n✓ Renamed Git remote origin to upstream\n✓ Added Git remote origin for alice/mine\n", diagnostics.String())
require.Equal(t, "https://gitea.test/alice/mine\n", out.String())
}
func TestRepoForkRemoteChecksTheCloneBeforeForking(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()
for name, results := range map[string]map[string]string{
"outside a clone": {"rev-parse --is-inside-work-tree": "false"},
"origin points elsewhere": {"rev-parse --is-inside-work-tree": "true", "remote get-url origin": server.URL + "/carol/other.git"},
} {
deps, _, _ := serverDeps(t, server.URL)
deps.Git = &fakeGit{results: results}
err := Execute(context.Background(), deps, []string{"repo", "fork", "bob/project", "--remote"})
require.Equal(t, clierr.Usage, clierr.ExitCode(err), "%s: %v", name, err)
}
}
func TestRepoRenameUpdatesTheSavedDefaultRepository(t *testing.T) {
server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
_, _ = w.Write([]byte(`{"name":"renamed","full_name":"alice/renamed"}`))
}))
defer server.Close()
deps, _, diagnostics := repoTTYDeps(t, server.URL)
saved := "config --local tea.repository " + server.URL + "/alice/renamed"
runner := &fakeGit{results: map[string]string{
"config --get tea.repository": server.URL + "/alice/project",
saved: "",
}}
deps.Git = runner
require.NoError(t, Execute(context.Background(), deps, []string{"repo", "rename", "renamed", "--yes"}))
require.Contains(t, runner.calls, saved)
require.Equal(t, "✓ Renamed repository alice/project to alice/renamed\n✓ Updated the default repository of this clone to alice/renamed\n", diagnostics.String())
}
+54 -1
View File
@@ -10,6 +10,7 @@ import (
"strings"
"testing"
"gitea.dev/tea/internal/clierr"
"github.com/stretchr/testify/require"
)
@@ -46,7 +47,7 @@ func TestRepositoryEditRejectsAMissingDefaultBranch(t *testing.T) {
deps, _, _ := serverDeps(t, server.URL)
err := Execute(context.Background(), deps, []string{"repo", "edit", "alice/project", "--default-branch", "nope"})
require.EqualError(t, err, `branch "nope" not found in alice/project`)
require.Equal(t, []string{"GET /api/v1/repos/alice/project/branches/nope"}, requests)
require.Equal(t, []string{"GET /api/v1/repos/alice/project/branches/nope", "GET /api/v1/repos/alice/project"}, requests)
}
func TestRepositoryEditReportsTheChangedSettings(t *testing.T) {
@@ -86,3 +87,55 @@ func TestRepositoryEditArchivesAfterTheOtherChanges(t *testing.T) {
"PATCH /api/v1/repos/alice/project archived",
}, requests)
}
func TestRepositoryEditSetsTheDefaultBranchOfAnEmptyRepository(t *testing.T) {
var requests []string
server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
requests = append(requests, r.Method+" "+r.URL.Path)
switch r.Method + " " + r.URL.Path {
case "GET /api/v1/repos/alice/project":
_, _ = w.Write([]byte(`{"full_name":"alice/project","empty":true}`))
case "PATCH /api/v1/repos/alice/project":
requirePayload(t, `{"default_branch":"develop"}`, r.Body)
_, _ = w.Write([]byte(`{"full_name":"alice/project"}`))
default:
w.WriteHeader(http.StatusNotFound)
}
}))
defer server.Close()
deps, _, _ := serverDeps(t, server.URL)
require.NoError(t, Execute(context.Background(), deps, []string{"repo", "edit", "alice/project", "--default-branch", "develop"}))
require.Equal(t, []string{"GET /api/v1/repos/alice/project/branches/develop", "GET /api/v1/repos/alice/project", "PATCH /api/v1/repos/alice/project"}, requests)
}
func TestRepositoryEditArchiveRequiresAnExplicitRepositoryWithYes(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, _, _ := serverDeps(t, server.URL)
deps.Git = upstreamCheckout(server)
err := Execute(context.Background(), deps, []string{"repo", "edit", "--archived", "--yes"})
require.Equal(t, clierr.Usage, clierr.ExitCode(err), "%v", err)
require.ErrorContains(t, err, "--yes requires an explicit repository, not one inferred from git remote upstream")
}
func TestRepositoryEditChecksTopicsBeforeChangingAnything(t *testing.T) {
requireUsageBeforeRequests(t, `invalid topic "Bad Topic"`, "repo", "edit", "alice/project", "-d", "new", "--add-topic", "Bad Topic")
}
func TestRepositoryEditIgnoresRemovingAMissingTopic(t *testing.T) {
var requests []string
server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
requests = append(requests, r.Method+" "+r.URL.Path)
if r.Method == http.MethodDelete {
w.WriteHeader(http.StatusNotFound)
return
}
_, _ = w.Write([]byte(`{"full_name":"alice/project"}`))
}))
defer server.Close()
deps, _, _ := serverDeps(t, server.URL)
require.NoError(t, Execute(context.Background(), deps, []string{"repo", "edit", "alice/project", "-d", "new", "--remove-topic", "legacy"}))
require.Equal(t, []string{"PATCH /api/v1/repos/alice/project", "DELETE /api/v1/repos/alice/project/topics/legacy"}, requests)
}
+7
View File
@@ -55,6 +55,13 @@ $ tea repo sync bob/project`,
remote, err := "", errNoMatchingRemote
if inside == "true" {
remote, err = a.checkoutRemote(cmd.Context(), target, snapshot)
// A remote preference only picks among the remotes of this repository: a repository
// argument that no remote here names is synced on the server, as without one.
if err != nil && len(args) > 0 && a.options.remote == "" {
if matches, matchErr := a.remotesForRepositoryURLs(cmd.Context(), target.hosts(), "", "", target.Repo); matchErr == nil && len(matches) == 0 {
err = errNoMatchingRemote
}
}
}
switch {
case errors.Is(err, errNoMatchingRemote) && len(args) > 0:
+16
View File
@@ -11,6 +11,7 @@ import (
"testing"
"gitea.dev/tea/internal/clierr"
"gitea.dev/tea/internal/settings"
"github.com/stretchr/testify/require"
)
@@ -129,3 +130,18 @@ func TestRepoSyncOutsideACloneExplainsWhatItNeeds(t *testing.T) {
require.Equal(t, clierr.Usage, clierr.ExitCode(err), "%v", err)
require.ErrorContains(t, err, "repo sync needs a Git clone of alice/project; run it inside one, or pass a fork to update it on the server")
}
func TestRepoSyncOnTheServerIgnoresARemotePreferenceForAnotherRepository(t *testing.T) {
var requests []string
server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
requests = append(requests, r.Method+" "+r.URL.Path)
_, _ = w.Write([]byte(`{"full_name":"bob/project","mirror":true,"original_url":"https://example.test/source.git"}`))
}))
defer server.Close()
deps, _, _ := serverDeps(t, server.URL)
require.NoError(t, deps.Settings.Update(settings.TargetPreferences, func(s *settings.Snapshot) error { s.Preferences.Remote = "origin"; return nil }))
// origin is a clone of alice/project, not of the repository named on the command line.
deps.Git = &syncGit{url: server.URL + "/alice/project.git", inside: "true"}
require.NoError(t, Execute(context.Background(), deps, []string{"repo", "sync", "bob/project"}))
require.Equal(t, []string{"GET /api/v1/repos/bob/project", "POST /api/v1/repos/bob/project/mirror-sync"}, requests)
}