From 6e523d969d8fd94d9b0b0077100ae4836ef8777e Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 18 Oct 2026 17:38:53 +0200 Subject: [PATCH] fix: address verified review findings (repo-mutations-sync-1) Signed-off-by: bircni --- docs/migration-1.0.md | 7 +++ internal/app/app_test.go | 8 +++ internal/app/clone_test.go | 18 ++++--- internal/app/help_topics.go | 5 +- internal/app/repo_mutations.go | 78 +++++++++++++++++++++++----- internal/app/repo_mutations_test.go | 33 ++++++++++++ internal/app/repository_edit_test.go | 55 +++++++++++++++++++- internal/app/repository_sync.go | 7 +++ internal/app/repository_sync_test.go | 16 ++++++ internal/git/sync.go | 60 ++++++++++++++++++--- internal/git/sync_test.go | 26 ++++++++++ 11 files changed, 283 insertions(+), 30 deletions(-) diff --git a/docs/migration-1.0.md b/docs/migration-1.0.md index 83e5ac26..6d8c5044 100644 --- a/docs/migration-1.0.md +++ b/docs/migration-1.0.md @@ -135,6 +135,10 @@ remove the old entry with is only needed for plain `git` commands. Add `--local` to configure only the current repository. +tea 0.16 cloned over SSH when the login had an SSH key; tea 1.0 follows +`git_protocol`, which defaults to `https`. To keep cloning over SSH, run +`tea config set git_protocol ssh` (add `--host ` for one instance). + ## Global flags | Old | tea 1.0 | @@ -240,6 +244,9 @@ Renamed flags that appear on many commands: | `--page` / `-p`, `--limit` / `--lm` | `--page`, `--limit` / `-L` | | `--style` / `-s` (pr merge) | `--merge`, `--squash` or `--rebase`, or `--method` / `-s`; without one, the repository's default merge style | | `--owner` / `-O`, `--init`, `--labels`, `--trustmodel`, `--branch`, `--template` (repo create) | `--org`, `--auto-init`, `--label-template`, `--trust-model`, `--default-branch`, `--as-template` | +| `--desc` (repo edit) | `--description` / `-d` | +| `--template true\|false`, `--private true\|false`, `--archived true\|false` (repo edit) | `--as-template`, `--private`, `tea repo archive` / `unarchive`; these are on/off switches, so write `--private=false` (`--private false` names a repository `false`) | +| `--owner` / `-O` (repo fork) | `--org` | | `--inactive` (webhook) | `--active=false` | | `--out` (man) | `--output-file` / `-o` | diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 304934c0..b7a45da7 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -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 [flags]\n\nAVAILABLE COMMANDS\n attachment: ") require.NotContains(t, out.String(), "JSON FIELDS") diff --git a/internal/app/clone_test.go b/internal/app/clone_test.go index f0fc342b..b49a5507 100644 --- a/internal/app/clone_test.go +++ b/internal/app/clone_test.go @@ -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"})) diff --git a/internal/app/help_topics.go b/internal/app/help_topics.go index 78a4a7fa..7005d8ca 100644 --- a/internal/app/help_topics.go +++ b/internal/app/help_topics.go @@ -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] { diff --git a/internal/app/repo_mutations.go b/internal/app/repo_mutations.go index d96465f0..4d44b5c6 100644 --- a/internal/app/repo_mutations.go +++ b/internal/app/repo_mutations.go @@ -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 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 ", 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) }, } diff --git a/internal/app/repo_mutations_test.go b/internal/app/repo_mutations_test.go index 7bf63c1a..6a5514ed 100644 --- a/internal/app/repo_mutations_test.go +++ b/internal/app/repo_mutations_test.go @@ -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()) +} diff --git a/internal/app/repository_edit_test.go b/internal/app/repository_edit_test.go index 16483883..6e9c7964 100644 --- a/internal/app/repository_edit_test.go +++ b/internal/app/repository_edit_test.go @@ -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) +} diff --git a/internal/app/repository_sync.go b/internal/app/repository_sync.go index 4aa68f2a..77f6f50e 100644 --- a/internal/app/repository_sync.go +++ b/internal/app/repository_sync.go @@ -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: diff --git a/internal/app/repository_sync_test.go b/internal/app/repository_sync_test.go index 0c4f82ac..31cca31e 100644 --- a/internal/app/repository_sync_test.go +++ b/internal/app/repository_sync_test.go @@ -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) +} diff --git a/internal/git/sync.go b/internal/git/sync.go index 8278a876..0cf37994 100644 --- a/internal/git/sync.go +++ b/internal/git/sync.go @@ -7,6 +7,8 @@ import ( "context" "errors" "fmt" + "os" + "path/filepath" "strings" ) @@ -31,8 +33,8 @@ type SyncResult struct { // when it is missing, and never changes which branch is checked out. The checked-out branch merges // with --ff-only and needs a clean working tree; another branch moves with update-ref, which fails // if the branch changed meanwhile. A branch that already contains the remote history is kept. A -// branch checked out in another linked worktree is refused, since moving it would leave that -// worktree's files behind its HEAD. +// branch checked out in another linked worktree, or being rebased in any worktree, is refused, +// since moving it would leave that worktree's files behind its HEAD or break the rebase. func SyncBranch(ctx context.Context, runner Runner, opts SyncOptions) (SyncResult, error) { run := func(args ...string) (string, error) { return runner.Output(ctx, opts.Directory, opts.Prompt, args...) } if opts.Remote == "" || opts.Branch == "" { @@ -58,6 +60,9 @@ func SyncBranch(ctx context.Context, runner Runner, opts SyncOptions) (SyncResul if path := worktreeWithBranch(worktrees, ref); path != "" { return SyncResult{}, fmt.Errorf("%s is checked out in the worktree %s; sync it from there", opts.Branch, path) } + if path := rebasingWorktree(ctx, runner, worktrees, ref); path != "" { + return SyncResult{}, fmt.Errorf("%s is being rebased in %s; finish or abort the rebase first", opts.Branch, path) + } } if checkedOut { status, err := run("status", "--porcelain=v1", "--untracked-files=normal") @@ -91,27 +96,34 @@ func SyncBranch(ctx context.Context, runner Runner, opts SyncOptions) (SyncResul return SyncResult{}, err } if _, err := run("merge-base", "--is-ancestor", commit, previous); err != nil { - return SyncResult{}, fmt.Errorf("%s and %s/%s have diverged; merge or rebase before syncing: %w", opts.Branch, opts.Remote, opts.Branch, err) + return SyncResult{}, fmt.Errorf("%s and %s/%s have diverged; merge or rebase before syncing", opts.Branch, opts.Remote, opts.Branch) } result.Commit = previous return result, nil } } + track := func() error { + for _, setting := range [][2]string{{"remote", opts.Remote}, {"merge", ref}} { + if _, err := run("config", "branch."+opts.Branch+"."+setting[0], setting[1]); err != nil { + return fmt.Errorf("created %s, but could not set it to track %s: %w", opts.Branch, opts.Remote, err) + } + } + return nil + } if !checkedOut { // The old value makes update-ref fail if the branch moved, or appeared, while fetching. if _, err := run("update-ref", "-m", "tea repo sync", ref, commit, previous); err != nil { return SyncResult{}, fmt.Errorf("%s changed while fetching; retry sync: %w", opts.Branch, err) } if previous == "" { - for _, setting := range [][2]string{{"remote", opts.Remote}, {"merge", ref}} { - if _, err := run("config", "branch."+opts.Branch+"."+setting[0], setting[1]); err != nil { - return SyncResult{}, fmt.Errorf("created %s, but could not set it to track %s: %w", opts.Branch, opts.Remote, err) - } + if err := track(); err != nil { + return SyncResult{}, err } } return result, nil } - current, err := run("rev-parse", "--verify", "HEAD^{commit}") + // A checked-out branch without commits has no HEAD to compare; merge fast-forwards it. + current, err := optional(run("rev-parse", "--verify", "--quiet", "HEAD^{commit}")) if err != nil { return SyncResult{}, err } @@ -124,6 +136,11 @@ func SyncBranch(ctx context.Context, runner Runner, opts SyncOptions) (SyncResul if actual, err := run("rev-parse", "--verify", "HEAD^{commit}"); err != nil || actual != commit { return SyncResult{}, errors.New("HEAD changed during sync; inspect the working tree before retrying") } + if previous == "" { + if err := track(); err != nil { + return SyncResult{}, err + } + } return result, nil } @@ -142,6 +159,33 @@ func worktreeWithBranch(porcelain, ref string) string { return "" } +// rebasingWorktree returns the path of the worktree in worktree list --porcelain output whose +// detached HEAD is the result of rebasing ref, or "" when none is. Git records the branch being +// rebased in rebase-merge/head-name or rebase-apply/head-name of the worktree's Git directory. +func rebasingWorktree(ctx context.Context, runner Runner, porcelain, ref string) string { + path := "" + for line := range strings.Lines(porcelain) { + line = strings.TrimRight(line, "\r\n") + if value, ok := strings.CutPrefix(line, "worktree "); ok { + path = value + } else if line == "detached" { + for _, name := range []string{"rebase-merge/head-name", "rebase-apply/head-name"} { + file, err := runner.Output(ctx, path, false, "rev-parse", "--git-path", name) + if err != nil { + continue + } + if !filepath.IsAbs(file) { + file = filepath.Join(path, file) + } + if head, err := os.ReadFile(file); err == nil && strings.TrimSpace(string(head)) == ref { + return path + } + } + } + } + return "" +} + // optional returns "" without an error for a Git command that exits with status 1 to report // that what it looked for does not exist, such as rev-parse --verify --quiet. func optional(output string, err error) (string, error) { diff --git a/internal/git/sync_test.go b/internal/git/sync_test.go index 59836a15..c2e6d208 100644 --- a/internal/git/sync_test.go +++ b/internal/git/sync_test.go @@ -125,6 +125,18 @@ func TestSyncBranchKeepsTheCheckedOutBranch(t *testing.T) { } } +func TestSyncBranchFastForwardsABranchWithoutCommits(t *testing.T) { + f := newSyncFixture(t) + empty := t.TempDir() + f.git(empty, "init", "-b", "main") + f.git(empty, "remote", "add", "origin", f.git(f.work, "remote", "get-url", "origin")) + result, err := SyncBranch(context.Background(), f.runner, SyncOptions{Directory: empty, Remote: "origin", Branch: "main"}) + require.NoError(t, err) + require.Equal(t, SyncResult{"main", f.ahead, "", "origin"}, result) + require.Equal(t, f.ahead, f.git(empty, "rev-parse", "HEAD")) + require.Equal(t, "origin", f.git(empty, "config", "branch.main.remote")) +} + func TestSyncBranchRefusesWhatItCannotFastForward(t *testing.T) { for _, tc := range []struct { name, message string @@ -149,6 +161,19 @@ func TestSyncBranchRefusesWhatItCannotFastForward(t *testing.T) { name: "checked out in another worktree", message: "main is checked out in the worktree", prepare: func(f syncFixture) { f.git(f.work, "worktree", "add", filepath.Join(t.TempDir(), "linked"), "main") }, }, + { + name: "being rebased", message: "main is being rebased in", + prepare: func(f syncFixture) { + f.git(f.work, "switch", "main") + require.NoError(t, os.WriteFile(filepath.Join(f.work, "local.txt"), []byte("local"), 0o600)) + f.git(f.work, "add", "local.txt") + f.git(f.work, "commit", "-m", "local") + f.git(f.work, "fetch", "origin") + // The failing exec step stops the rebase with main detached. + _, _ = f.runner.Output(context.Background(), f.work, false, "rebase", "--exec", "false", "origin/main") + require.Empty(t, f.git(f.work, "branch", "--show-current")) + }, + }, } { t.Run(tc.name, func(t *testing.T) { f := newSyncFixture(t) @@ -156,6 +181,7 @@ func TestSyncBranchRefusesWhatItCannotFastForward(t *testing.T) { branch, before := f.git(f.work, "branch", "--show-current"), f.git(f.work, "rev-parse", "main") _, err := f.sync("main") require.ErrorContains(t, err, tc.message) + require.NotContains(t, err.Error(), "credentials") require.Equal(t, branch, f.git(f.work, "branch", "--show-current")) require.Equal(t, before, f.git(f.work, "rev-parse", "main")) })