Merge pull request #4942 from cli/repo-sync-merge-upstream

repo sync: Use the new merge-upstream API if available

pkg/cmd/repo/sync/http.go

@@ -3,7 +3,9 @@ package sync
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"net/http"
 
 	"github.com/cli/cli/v2/api"
 	"github.com/cli/cli/v2/internal/ghrepo"
@@ -27,6 +29,39 @@ func latestCommit(client *api.Client, repo ghrepo.Interface, branch string) (com
 	return response, err
 }
 
+type upstreamMergeErr struct{ error }
+
+var upstreamMergeUnavailableErr = upstreamMergeErr{errors.New("upstream merge API is unavailable")}
+
+func triggerUpstreamMerge(client *api.Client, repo ghrepo.Interface, branch string) (string, error) {
+	var payload bytes.Buffer
+	if err := json.NewEncoder(&payload).Encode(map[string]interface{}{
+		"branch": branch,
+	}); err != nil {
+		return "", err
+	}
+
+	var response struct {
+		Message    string `json:"message"`
+		MergeType  string `json:"merge_type"`
+		BaseBranch string `json:"base_branch"`
+	}
+	path := fmt.Sprintf("repos/%s/%s/merge-upstream", repo.RepoOwner(), repo.RepoName())
+	var httpErr api.HTTPError
+	if err := client.REST(repo.RepoHost(), "POST", path, &payload, &response); err != nil {
+		if errors.As(err, &httpErr) {
+			switch httpErr.StatusCode {
+			case http.StatusUnprocessableEntity, http.StatusConflict:
+				return "", upstreamMergeErr{errors.New(httpErr.Message)}
+			case http.StatusNotFound:
+				return "", upstreamMergeUnavailableErr
+			}
+		}
+		return "", err
+	}
+	return response.BaseBranch, nil
+}
+
 func syncFork(client *api.Client, repo ghrepo.Interface, branch, SHA string, force bool) error {
 	path := fmt.Sprintf("repos/%s/%s/git/refs/heads/%s", repo.RepoOwner(), repo.RepoName(), branch)
 	body := map[string]interface{}{

pkg/cmd/repo/sync/sync.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/http"
 	"regexp"
+	"strings"
 
 	"github.com/MakeNowJust/heredoc"
 	"github.com/cli/cli/v2/api"
@@ -177,38 +178,19 @@ func syncRemoteRepo(opts *SyncOptions) error {
 		return err
 	}
 
-	if opts.SrcArg == "" {
+	if opts.SrcArg != "" {
-		opts.IO.StartProgressIndicator()
-		srcRepo, err = api.RepoParent(apiClient, destRepo)
-		opts.IO.StopProgressIndicator()
-		if err != nil {
-			return err
-		}
-		if srcRepo == nil {
-			return fmt.Errorf("can't determine source repository for %s because repository is not fork", ghrepo.FullName(destRepo))
-		}
-	} else {
 		srcRepo, err = ghrepo.FromFullName(opts.SrcArg)
 		if err != nil {
 			return err
 		}
 	}
 
-	if destRepo.RepoHost() != srcRepo.RepoHost() {
+	if srcRepo != nil && destRepo.RepoHost() != srcRepo.RepoHost() {
 		return fmt.Errorf("can't sync repositories from different hosts")
 	}
 
-	if opts.Branch == "" {
-		opts.IO.StartProgressIndicator()
-		opts.Branch, err = api.RepoDefaultBranch(apiClient, srcRepo)
-		opts.IO.StopProgressIndicator()
-		if err != nil {
-			return err
-		}
-	}
-
 	opts.IO.StartProgressIndicator()
-	err = executeRemoteRepoSync(apiClient, destRepo, srcRepo, opts)
+	baseBranchLabel, err := executeRemoteRepoSync(apiClient, destRepo, srcRepo, opts)
 	opts.IO.StopProgressIndicator()
 	if err != nil {
 		if errors.Is(err, divergingError) {
@@ -219,11 +201,15 @@ func syncRemoteRepo(opts *SyncOptions) error {
 
 	if opts.IO.IsStdoutTTY() {
 		cs := opts.IO.ColorScheme()
-		fmt.Fprintf(opts.IO.Out, "%s Synced the \"%s\" branch from %s to %s\n",
+		branchName := opts.Branch
+		if idx := strings.Index(baseBranchLabel, ":"); idx >= 0 {
+			branchName = baseBranchLabel[idx+1:]
+		}
+		fmt.Fprintf(opts.IO.Out, "%s Synced the \"%s:%s\" branch from \"%s\"\n",
 			cs.SuccessIcon(),
-			opts.Branch,
+			destRepo.RepoOwner(),
-			ghrepo.FullName(srcRepo),
+			branchName,
-			ghrepo.FullName(destRepo))
+			baseBranchLabel)
 	}
 
 	return nil
@@ -294,20 +280,47 @@ func executeLocalRepoSync(srcRepo ghrepo.Interface, remote string, opts *SyncOpt
 	return nil
 }
 
-func executeRemoteRepoSync(client *api.Client, destRepo, srcRepo ghrepo.Interface, opts *SyncOptions) error {
+func executeRemoteRepoSync(client *api.Client, destRepo, srcRepo ghrepo.Interface, opts *SyncOptions) (string, error) {
-	commit, err := latestCommit(client, srcRepo, opts.Branch)
+	branchName := opts.Branch
+	if branchName == "" {
+		var err error
+		branchName, err = api.RepoDefaultBranch(client, destRepo)
+		if err != nil {
+			return "", err
+		}
+	}
+
+	var apiErr upstreamMergeErr
+	if baseBranch, err := triggerUpstreamMerge(client, destRepo, branchName); err == nil {
+		return baseBranch, nil
+	} else if !errors.As(err, &apiErr) {
+		return "", err
+	}
+
+	if srcRepo == nil {
+		var err error
+		srcRepo, err = api.RepoParent(client, destRepo)
+		if err != nil {
+			return "", err
+		}
+		if srcRepo == nil {
+			return "", fmt.Errorf("can't determine source repository for %s because repository is not fork", ghrepo.FullName(destRepo))
+		}
+	}
+
+	commit, err := latestCommit(client, srcRepo, branchName)
 	if err != nil {
-		return err
+		return "", err
 	}
 
 	// This is not a great way to detect the error returned by the API
 	// Unfortunately API returns 422 for multiple reasons
 	notFastForwardErrorMessage := regexp.MustCompile(`^Update is not a fast forward$`)
-	err = syncFork(client, destRepo, opts.Branch, commit.Object.SHA, opts.Force)
+	err = syncFork(client, destRepo, branchName, commit.Object.SHA, opts.Force)
 	var httpErr api.HTTPError
 	if err != nil && errors.As(err, &httpErr) && notFastForwardErrorMessage.MatchString(httpErr.Message) {
-		return divergingError
+		return "", divergingError
 	}
 
-	return err
+	return fmt.Sprintf("%s:%s", srcRepo.RepoOwner(), branchName), nil
 }

pkg/cmd/repo/sync/sync_test.go

@@ -262,10 +262,26 @@ func Test_SyncRun(t *testing.T) {
 			wantStdout: "✓ Synced the \"trunk\" branch from OWNER/REPO to local repository\n",
 		},
 		{
-			name: "sync remote fork with parent - tty",
+			name: "sync remote fork with parent with new api - tty",
 			tty:  true,
 			opts: &SyncOptions{
-				DestArg: "OWNER/REPO-FORK",
+				DestArg: "FORKOWNER/REPO-FORK",
+			},
+			httpStubs: func(reg *httpmock.Registry) {
+				reg.Register(
+					httpmock.GraphQL(`query RepositoryInfo\b`),
+					httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`))
+				reg.Register(
+					httpmock.REST("POST", "repos/FORKOWNER/REPO-FORK/merge-upstream"),
+					httpmock.StatusStringResponse(200, `{"base_branch": "OWNER:trunk"}`))
+			},
+			wantStdout: "✓ Synced the \"FORKOWNER:trunk\" branch from \"OWNER:trunk\"\n",
+		},
+		{
+			name: "sync remote fork with parent using api fallback - tty",
+			tty:  true,
+			opts: &SyncOptions{
+				DestArg: "FORKOWNER/REPO-FORK",
 			},
 			httpStubs: func(reg *httpmock.Registry) {
 				reg.Register(
@@ -274,34 +290,31 @@ func Test_SyncRun(t *testing.T) {
 				reg.Register(
 					httpmock.GraphQL(`query RepositoryInfo\b`),
 					httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`))
+				reg.Register(
+					httpmock.REST("POST", "repos/FORKOWNER/REPO-FORK/merge-upstream"),
+					httpmock.StatusStringResponse(404, `{}`))
 				reg.Register(
 					httpmock.REST("GET", "repos/OWNER/REPO/git/refs/heads/trunk"),
 					httpmock.StringResponse(`{"object":{"sha":"0xDEADBEEF"}}`))
 				reg.Register(
-					httpmock.REST("PATCH", "repos/OWNER/REPO-FORK/git/refs/heads/trunk"),
+					httpmock.REST("PATCH", "repos/FORKOWNER/REPO-FORK/git/refs/heads/trunk"),
 					httpmock.StringResponse(`{}`))
 			},
-			wantStdout: "✓ Synced the \"trunk\" branch from OWNER/REPO to OWNER/REPO-FORK\n",
+			wantStdout: "✓ Synced the \"FORKOWNER:trunk\" branch from \"OWNER:trunk\"\n",
 		},
 		{
 			name: "sync remote fork with parent - notty",
 			tty:  false,
 			opts: &SyncOptions{
-				DestArg: "OWNER/REPO-FORK",
+				DestArg: "FORKOWNER/REPO-FORK",
 			},
 			httpStubs: func(reg *httpmock.Registry) {
-				reg.Register(
-					httpmock.GraphQL(`query RepositoryFindParent\b`),
-					httpmock.StringResponse(`{"data":{"repository":{"parent":{"name":"REPO","owner":{"login": "OWNER"}}}}}`))
 				reg.Register(
 					httpmock.GraphQL(`query RepositoryInfo\b`),
 					httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`))
 				reg.Register(
-					httpmock.REST("GET", "repos/OWNER/REPO/git/refs/heads/trunk"),
+					httpmock.REST("POST", "repos/FORKOWNER/REPO-FORK/merge-upstream"),
-					httpmock.StringResponse(`{"object":{"sha":"0xDEADBEEF"}}`))
+					httpmock.StatusStringResponse(200, `{"base_branch": "OWNER:trunk"}`))
-				reg.Register(
-					httpmock.REST("PATCH", "repos/OWNER/REPO-FORK/git/refs/heads/trunk"),
-					httpmock.StringResponse(`{}`))
 			},
 			wantStdout: "",
 		},
@@ -310,11 +323,15 @@ func Test_SyncRun(t *testing.T) {
 			tty:  true,
 			opts: &SyncOptions{
 				DestArg: "OWNER/REPO",
+				Branch:  "trunk",
 			},
 			httpStubs: func(reg *httpmock.Registry) {
+				reg.Register(
+					httpmock.REST("POST", "repos/OWNER/REPO/merge-upstream"),
+					httpmock.StatusStringResponse(422, `{"message": "Validation Failed"}`))
 				reg.Register(
 					httpmock.GraphQL(`query RepositoryFindParent\b`),
-					httpmock.StringResponse(`{"data":{"repository":{}}}`))
+					httpmock.StringResponse(`{"data":{"repository":{"parent":null}}}`))
 			},
 			wantErr: true,
 			errMsg:  "can't determine source repository for OWNER/REPO because repository is not fork",
@@ -325,19 +342,14 @@ func Test_SyncRun(t *testing.T) {
 			opts: &SyncOptions{
 				DestArg: "OWNER/REPO",
 				SrcArg:  "OWNER2/REPO2",
+				Branch:  "trunk",
 			},
 			httpStubs: func(reg *httpmock.Registry) {
 				reg.Register(
-					httpmock.GraphQL(`query RepositoryInfo\b`),
+					httpmock.REST("POST", "repos/OWNER/REPO/merge-upstream"),
-					httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`))
+					httpmock.StatusStringResponse(200, `{"base_branch": "OWNER2:trunk"}`))
-				reg.Register(
-					httpmock.REST("GET", "repos/OWNER2/REPO2/git/refs/heads/trunk"),
-					httpmock.StringResponse(`{"object":{"sha":"0xDEADBEEF"}}`))
-				reg.Register(
-					httpmock.REST("PATCH", "repos/OWNER/REPO/git/refs/heads/trunk"),
-					httpmock.StringResponse(`{}`))
 			},
-			wantStdout: "✓ Synced the \"trunk\" branch from OWNER2/REPO2 to OWNER/REPO\n",
+			wantStdout: "✓ Synced the \"OWNER:trunk\" branch from \"OWNER2:trunk\"\n",
 		},
 		{
 			name: "sync remote fork with parent and specified branch",
@@ -348,16 +360,10 @@ func Test_SyncRun(t *testing.T) {
 			},
 			httpStubs: func(reg *httpmock.Registry) {
 				reg.Register(
-					httpmock.GraphQL(`query RepositoryFindParent\b`),
+					httpmock.REST("POST", "repos/OWNER/REPO-FORK/merge-upstream"),
-					httpmock.StringResponse(`{"data":{"repository":{"parent":{"name":"REPO","owner":{"login": "OWNER"}}}}}`))
+					httpmock.StatusStringResponse(200, `{"base_branch": "OWNER:test"}`))
-				reg.Register(
-					httpmock.REST("GET", "repos/OWNER/REPO/git/refs/heads/test"),
-					httpmock.StringResponse(`{"object":{"sha":"0xDEADBEEF"}}`))
-				reg.Register(
-					httpmock.REST("PATCH", "repos/OWNER/REPO-FORK/git/refs/heads/test"),
-					httpmock.StringResponse(`{}`))
 			},
-			wantStdout: "✓ Synced the \"test\" branch from OWNER/REPO to OWNER/REPO-FORK\n",
+			wantStdout: "✓ Synced the \"OWNER:test\" branch from \"OWNER:test\"\n",
 		},
 		{
 			name: "sync remote fork with parent and force specified",
@@ -373,6 +379,9 @@ func Test_SyncRun(t *testing.T) {
 				reg.Register(
 					httpmock.GraphQL(`query RepositoryInfo\b`),
 					httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`))
+				reg.Register(
+					httpmock.REST("POST", "repos/OWNER/REPO-FORK/merge-upstream"),
+					httpmock.StatusStringResponse(409, `{"message": "Merge conflict"}`))
 				reg.Register(
 					httpmock.REST("GET", "repos/OWNER/REPO/git/refs/heads/trunk"),
 					httpmock.StringResponse(`{"object":{"sha":"0xDEADBEEF"}}`))
@@ -380,7 +389,7 @@ func Test_SyncRun(t *testing.T) {
 					httpmock.REST("PATCH", "repos/OWNER/REPO-FORK/git/refs/heads/trunk"),
 					httpmock.StringResponse(`{}`))
 			},
-			wantStdout: "✓ Synced the \"trunk\" branch from OWNER/REPO to OWNER/REPO-FORK\n",
+			wantStdout: "✓ Synced the \"OWNER:trunk\" branch from \"OWNER:trunk\"\n",
 		},
 		{
 			name: "sync remote fork with parent and not fast forward merge",
@@ -395,6 +404,9 @@ func Test_SyncRun(t *testing.T) {
 				reg.Register(
 					httpmock.GraphQL(`query RepositoryInfo\b`),
 					httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`))
+				reg.Register(
+					httpmock.REST("POST", "repos/OWNER/REPO-FORK/merge-upstream"),
+					httpmock.StatusStringResponse(409, `{"message": "Merge conflict"}`))
 				reg.Register(
 					httpmock.REST("GET", "repos/OWNER/REPO/git/refs/heads/trunk"),
 					httpmock.StringResponse(`{"object":{"sha":"0xDEADBEEF"}}`))
@@ -454,11 +466,11 @@ func Test_SyncRun(t *testing.T) {
 			defer reg.Verify(t)
 			err := syncRun(tt.opts)
 			if tt.wantErr {
-				assert.Error(t, err)
+				assert.EqualError(t, err, tt.errMsg)
-				assert.Equal(t, tt.errMsg, err.Error())
 				return
+			} else if err != nil {
+				t.Fatalf("syncRun() unexpected error: %v", err)
 			}
-			assert.NoError(t, err)
 			assert.Equal(t, tt.wantStdout, stdout.String())
 		})
 	}