From 229f4aae67495c8ddc4b577125b585167a44b20a Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Wed, 18 Mar 2026 14:53:29 +0100 Subject: [PATCH 1/2] fix(private-org-sync): batch ls-remote calls per repo instead of per branch Previously, mirror() called ls-remote for both source and destination on every branch, resulting in 2*N network calls per repo where N is the number of branches. Since ls-remote --heads returns ALL branches at once, we can call it once per repo and pass the results to mirror(). This moves git init, remote setup, and ls-remote calls from mirror() into the main loop where repos are already grouped, reducing network calls from 2*N to 2 per repo. Co-Authored-By: Claude Opus 4.6 --- cmd/private-org-sync/main.go | 94 ++++++++++-------- cmd/private-org-sync/main_test.go | 158 +++++++++--------------------- 2 files changed, 99 insertions(+), 153 deletions(-) diff --git a/cmd/private-org-sync/main.go b/cmd/private-org-sync/main.go index af788c6b0f9..c8a05b5a0bb 100644 --- a/cmd/private-org-sync/main.go +++ b/cmd/private-org-sync/main.go @@ -343,13 +343,13 @@ func (g gitSyncer) initRepo(repoDir, org, repo string) error { return nil } -// mirror syncs content from source location to destination one, using a local -// repository in the given path. The `repoDir` must have been previously -// initialized via initRepo(). The git content from the `src` location will -// be fetched to this local repository and then pushed to the `dst` location. +// mirror syncs a single branch from source to destination, using pre-fetched +// branch head information. The `repoDir` must have been previously initialized +// with git init and remote setup. The `srcHeads` and `dstHeads` must have been +// obtained from ls-remote calls against the source and destination repos. // Multiple `mirror` calls over the same `repoDir` will reuse the content // fetched in previous calls, acting like a cache. -func (g gitSyncer) mirror(repoDir string, src, dst location) error { +func (g gitSyncer) mirror(repoDir string, src, dst location, srcHeads, dstHeads RemoteBranchHeads, destUrl *url.URL) error { mirrorFields := logrus.Fields{ "source": src.String(), "destination": dst.String(), @@ -358,40 +358,10 @@ func (g gitSyncer) mirror(repoDir string, src, dst location) error { logger := g.logger.WithFields(mirrorFields) logger.Info("Syncing content between locations") - // We ls-remote destination first thing because when it does not exist - // we do not need to do any of the remaining operations. - logger.Debug("Determining HEAD of destination branch") - destUrlRaw := fmt.Sprintf("%s/%s/%s", g.prefix, dst.org, dst.repo) - destUrl, err := url.Parse(destUrlRaw) - if err != nil { - logger.WithField("remote-url", destUrlRaw).WithError(err).Error("Failed to construct URL for the destination remote") - return fmt.Errorf("failed to construct URL for the destination remote") - } - if g.token != "" { - destUrl.User = url.User(g.token) - } - - dstHeads, err := getRemoteBranchHeads(logger, g.git, repoDir, destUrl.String()) - if err != nil { - message := "destination repository does not exist or we cannot access it" - if g.failOnNonexistentDst { - logger.Errorf("%s", message) - return fmt.Errorf("%s", message) - } - - logger.Warn(message) - return nil - } dstCommitHash := dstHeads[dst.branch] srcRemote := fmt.Sprintf("%s-%s", src.org, src.repo) - logger.Debug("Determining HEAD of source branch") - srcHeads, err := getRemoteBranchHeads(logger, withRetryOnNonzero(g.git, 5), repoDir, srcRemote) - if err != nil { - logger.WithError(err).Error("Failed to determine branch HEADs in source") - return fmt.Errorf("failed to determine branch HEADs in source") - } srcCommitHash, ok := srcHeads[src.branch] if !ok { if api.FlavorForBranch(src.branch) == "misc" { @@ -664,6 +634,11 @@ func main() { } for key, branches := range grouped { + repoLogger := logrus.WithFields(logrus.Fields{"org": key.org, "repo": key.repo}) + syncer.logger = repoLogger + + dstRepo := privateorg.MirroredRepoName(key.org, key.repo, flattenedOrgs) + gitDir, err := syncer.makeGitDir(key.org, key.repo) if err != nil { for _, source := range branches { @@ -672,10 +647,6 @@ func main() { continue } - syncer.logger = logrus.WithFields(logrus.Fields{ - "org": key.org, - "repo": key.repo, - }) if err := syncer.initRepo(gitDir, key.org, key.repo); err != nil { for _, source := range branches { errs = append(errs, fmt.Errorf("%s: %w", source.String(), err)) @@ -683,6 +654,45 @@ func main() { continue } + // ls-remote destination once per repo + destUrlRaw := fmt.Sprintf("%s/%s/%s", syncer.prefix, o.targetOrg, dstRepo) + destUrl, err := url.Parse(destUrlRaw) + if err != nil { + repoLogger.WithField("remote-url", destUrlRaw).WithError(err).Error("Failed to construct URL for the destination remote") + for _, source := range branches { + errs = append(errs, fmt.Errorf("%s: failed to construct URL for the destination remote", source.String())) + } + continue + } + if syncer.token != "" { + destUrl.User = url.User(syncer.token) + } + + dstHeads, err := getRemoteBranchHeads(repoLogger, syncer.git, gitDir, destUrl.String()) + if err != nil { + message := "destination repository does not exist or we cannot access it" + if syncer.failOnNonexistentDst { + repoLogger.Errorf("%s", message) + for _, source := range branches { + errs = append(errs, fmt.Errorf("%s: %s", source.String(), message)) + } + } else { + repoLogger.Warn(message) + } + continue + } + + // ls-remote source once per repo + srcRemote := fmt.Sprintf("%s-%s", key.org, key.repo) + srcHeads, err := getRemoteBranchHeads(repoLogger, withRetryOnNonzero(syncer.git, 5), gitDir, srcRemote) + if err != nil { + repoLogger.WithError(err).Error("Failed to determine branch HEADs in source") + for _, source := range branches { + errs = append(errs, fmt.Errorf("%s: failed to determine branch HEADs in source", source.String())) + } + continue + } + for _, source := range branches { syncer.logger = config.LoggerForInfo(config.Info{ Metadata: api.Metadata{ @@ -692,11 +702,9 @@ func main() { }, }) - destination := source - destination.org = o.targetOrg - destination.repo = privateorg.MirroredRepoName(source.org, source.repo, flattenedOrgs) + destination := location{org: o.targetOrg, repo: dstRepo, branch: source.branch} - if err := syncer.mirror(gitDir, source, destination); err != nil { + if err := syncer.mirror(gitDir, source, destination, srcHeads, dstHeads, destUrl); err != nil { errs = append(errs, fmt.Errorf("%s->%s: %w", source.String(), destination.String(), err)) } } diff --git a/cmd/private-org-sync/main_test.go b/cmd/private-org-sync/main_test.go index ad11187849f..11fa13016b8 100644 --- a/cmd/private-org-sync/main_test.go +++ b/cmd/private-org-sync/main_test.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "net/url" "strings" "testing" "time" @@ -255,13 +256,16 @@ func TestMirror(t *testing.T) { token := "TOKEN" org, repo, branch := "org", "repo", "branch" destOrg := "dest" + destUrl, _ := url.Parse(fmt.Sprintf("https://%s@github.com/%s/%s", token, destOrg, repo)) testCases := []struct { description string - src location - dst location - failOnNonexistentDst bool - confirm bool + src location + dst location + confirm bool + + srcHeads RemoteBranchHeads + dstHeads RemoteBranchHeads expectedGitCalls []mockGitCall expectError bool @@ -271,9 +275,9 @@ func TestMirror(t *testing.T) { src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, confirm: true, + srcHeads: RemoteBranchHeads{branch: "source-sha"}, + dstHeads: RemoteBranchHeads{branch: "dest-sha"}, expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "dest-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, {call: "fetch --tags org-repo branch --depth=2"}, {call: "push --tags https://TOKEN@github.com/dest/repo FETCH_HEAD:refs/heads/branch"}, }, @@ -282,20 +286,9 @@ func TestMirror(t *testing.T) { description: "no confirm, success -> push with dry run", src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, + srcHeads: RemoteBranchHeads{branch: "source-sha"}, + dstHeads: RemoteBranchHeads{branch: "dest-sha"}, expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "dest-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, - {call: "fetch --tags org-repo branch --depth=2"}, - {call: "push --tags --dry-run https://TOKEN@github.com/dest/repo FETCH_HEAD:refs/heads/branch"}, - }, - }, - { - description: "no confirm, source has more branches -> push with dry run", - src: location{org: org, repo: repo, branch: branch}, - dst: location{org: destOrg, repo: repo, branch: branch}, - expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "dest-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch\nanother-sha refs/heads/another-branch"}, {call: "fetch --tags org-repo branch --depth=2"}, {call: "push --tags --dry-run https://TOKEN@github.com/dest/repo FETCH_HEAD:refs/heads/branch"}, }, @@ -304,9 +297,9 @@ func TestMirror(t *testing.T) { description: "fails to fetch -> error", src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, + srcHeads: RemoteBranchHeads{branch: "source-sha"}, + dstHeads: RemoteBranchHeads{branch: "dest-sha"}, expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "dest-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, {call: "fetch --tags org-repo branch --depth=2", exitCode: 1}, }, expectError: true, @@ -315,9 +308,9 @@ func TestMirror(t *testing.T) { description: "fetch fails with shallow file changed -> retries and succeeds", src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, + srcHeads: RemoteBranchHeads{branch: "source-sha"}, + dstHeads: RemoteBranchHeads{branch: "dest-sha"}, expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "dest-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, {call: "fetch --tags org-repo branch --depth=2", exitCode: 128, output: "fatal: shallow file has changed since we read it\n"}, {call: "fetch --tags org-repo branch --depth=2"}, {call: "push --tags --dry-run https://TOKEN@github.com/dest/repo FETCH_HEAD:refs/heads/branch"}, @@ -327,9 +320,9 @@ func TestMirror(t *testing.T) { description: "fetch fails with shallow file changed repeatedly -> error after retries exhausted", src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, + srcHeads: RemoteBranchHeads{branch: "source-sha"}, + dstHeads: RemoteBranchHeads{branch: "dest-sha"}, expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "dest-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, {call: "fetch --tags org-repo branch --depth=2", exitCode: 128, output: "fatal: shallow file has changed since we read it\n"}, {call: "fetch --tags org-repo branch --depth=2", exitCode: 128, output: "fatal: shallow file has changed since we read it\n"}, {call: "fetch --tags org-repo branch --depth=2", exitCode: 128, output: "fatal: shallow file has changed since we read it\n"}, @@ -340,9 +333,9 @@ func TestMirror(t *testing.T) { description: "no confirm, fails to push -> error", src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, + srcHeads: RemoteBranchHeads{branch: "source-sha"}, + dstHeads: RemoteBranchHeads{branch: "dest-sha"}, expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "dest-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, {call: "fetch --tags org-repo branch --depth=2"}, {call: "push --tags --dry-run https://TOKEN@github.com/dest/repo FETCH_HEAD:refs/heads/branch", exitCode: 1}, }, @@ -352,93 +345,39 @@ func TestMirror(t *testing.T) { description: "branches are in sync -> no fetch, no push", src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, - expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "source-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, - }, - }, - { - description: "ls-remote source fails with retries -> error", - src: location{org: org, repo: repo, branch: branch}, - dst: location{org: destOrg, repo: repo, branch: branch}, - expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "source-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", exitCode: 1}, - {call: "ls-remote --heads org-repo", exitCode: 1}, - {call: "ls-remote --heads org-repo", exitCode: 1}, - {call: "ls-remote --heads org-repo", exitCode: 1}, - {call: "ls-remote --heads org-repo", exitCode: 1}, - }, - expectError: true, - }, - { - description: "ls-remote source succeeds after retries -> success", - src: location{org: org, repo: repo, branch: branch}, - dst: location{org: destOrg, repo: repo, branch: branch}, - expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "source-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", exitCode: 1}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, - }, + srcHeads: RemoteBranchHeads{branch: "same-sha"}, + dstHeads: RemoteBranchHeads{branch: "same-sha"}, }, { description: "non-release source branch does not exist -> no error, skip with warning", src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, - expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "source-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", output: "some-sha refs/heads/not-the-branch"}, - }, + srcHeads: RemoteBranchHeads{"not-the-branch": "some-sha"}, + dstHeads: RemoteBranchHeads{branch: "dest-sha"}, }, { description: "release source branch does not exist -> error", src: location{org: org, repo: repo, branch: "release-4.21"}, dst: location{org: destOrg, repo: repo, branch: "release-4.21"}, - expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "source-sha refs/heads/release-4.21"}, - {call: "ls-remote --heads org-repo", output: "some-sha refs/heads/not-the-branch"}, - }, + srcHeads: RemoteBranchHeads{"not-the-branch": "some-sha"}, + dstHeads: RemoteBranchHeads{"release-4.21": "dest-sha"}, expectError: true, }, { description: "main source branch does not exist -> error", src: location{org: org, repo: repo, branch: "main"}, dst: location{org: destOrg, repo: repo, branch: "main"}, - expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "source-sha refs/heads/main"}, - {call: "ls-remote --heads org-repo", output: "some-sha refs/heads/not-the-branch"}, - }, + srcHeads: RemoteBranchHeads{"not-the-branch": "some-sha"}, + dstHeads: RemoteBranchHeads{"main": "dest-sha"}, expectError: true, }, { - // If git ls-remote fails, destination repository does not exist - // This is not an error unless failOnNonexistentDst is set - description: "warm cache, ls-remote destination fails on git -> no error when configured", - src: location{org: org, repo: repo, branch: branch}, - dst: location{org: destOrg, repo: repo, branch: branch}, - expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", exitCode: 1}, - }, - }, - { - // If git ls-remote fails, destination repository does not exist - // This is an error when failOnNonexistentDst is set - description: "warm cache, ls-remote destination fails on git -> error when configured", - src: location{org: org, repo: repo, branch: branch}, - dst: location{org: destOrg, repo: repo, branch: branch}, - expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", exitCode: 1}, - }, - failOnNonexistentDst: true, - expectError: true, - }, - { - description: "destination is empty repo, needs many commits -> full fetch then success", + description: "destination is empty repo -> full fetch then success", src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, + srcHeads: RemoteBranchHeads{branch: "source-sha"}, + dstHeads: RemoteBranchHeads{}, expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, {call: "fetch --tags org-repo branch"}, {call: "push --tags --dry-run https://TOKEN@github.com/dest/repo FETCH_HEAD:refs/heads/branch"}, }, @@ -447,9 +386,9 @@ func TestMirror(t *testing.T) { description: "destination needs 50 commits -> retries deepening fetches, then success", src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, + srcHeads: RemoteBranchHeads{branch: "source-sha"}, + dstHeads: RemoteBranchHeads{branch: "dest-sha"}, expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "dest-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, {call: "fetch --tags org-repo branch --depth=2"}, { call: "push --tags --dry-run https://TOKEN@github.com/dest/repo FETCH_HEAD:refs/heads/branch", @@ -493,9 +432,9 @@ func TestMirror(t *testing.T) { description: "destination needs to merge with source -> retries exceeded, then perform merge after fetching --unshallow", src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, + srcHeads: RemoteBranchHeads{branch: "source-sha"}, + dstHeads: RemoteBranchHeads{branch: "dest-sha"}, expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "dest-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, {call: "fetch --tags org-repo branch --depth=2"}, { call: "push --tags --dry-run https://TOKEN@github.com/dest/repo FETCH_HEAD:refs/heads/branch", @@ -554,9 +493,9 @@ func TestMirror(t *testing.T) { description: "destination needs to merge with source -> retries exceeded, merge fails and performs merge --abort", src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, + srcHeads: RemoteBranchHeads{branch: "source-sha"}, + dstHeads: RemoteBranchHeads{branch: "dest-sha"}, expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "dest-sha refs/heads/branch"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, {call: "fetch --tags org-repo branch --depth=2"}, { call: "push --tags --dry-run https://TOKEN@github.com/dest/repo FETCH_HEAD:refs/heads/branch", @@ -618,9 +557,9 @@ func TestMirror(t *testing.T) { description: "conflicting histories after a force-push result in an error", src: location{org: org, repo: repo, branch: branch}, dst: location{org: destOrg, repo: repo, branch: branch}, + srcHeads: RemoteBranchHeads{branch: "source-sha"}, + dstHeads: RemoteBranchHeads{}, expectedGitCalls: []mockGitCall{ - {call: "ls-remote --heads https://TOKEN@github.com/dest/repo"}, - {call: "ls-remote --heads org-repo", output: "source-sha refs/heads/branch"}, {call: "fetch --tags org-repo branch"}, { call: "push --tags --dry-run https://TOKEN@github.com/dest/repo FETCH_HEAD:refs/heads/branch", @@ -645,17 +584,16 @@ hint: See the 'Note about fast-forwards' in 'git push --help' for details. t: t, } m := gitSyncer{ - logger: logrus.WithField("test", tc.description), - prefix: defaultPrefix, - token: token, - confirm: tc.confirm, - root: "git-dir", - git: git.exec, - gitName: "openshift-bot", - gitEmail: "openshift-bot@redhat.com", - failOnNonexistentDst: tc.failOnNonexistentDst, + logger: logrus.WithField("test", tc.description), + prefix: defaultPrefix, + token: token, + confirm: tc.confirm, + root: "git-dir", + git: git.exec, + gitName: "openshift-bot", + gitEmail: "openshift-bot@redhat.com", } - err := m.mirror("repo-dir", tc.src, tc.dst) + err := m.mirror("repo-dir", tc.src, tc.dst, tc.srcHeads, tc.dstHeads, destUrl) if err == nil && tc.expectError { t.Error("expected error, got nil") } From fa7af8511faddfef1185d90a393fa725e324a153 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Mon, 13 Apr 2026 20:38:39 +0200 Subject: [PATCH 2/2] refactor(private-org-sync): extract syncRepo method for testability Extract syncRepo from the main loop to group per-repo operations (git init, ls-remote, branch iteration) into a testable method. Simplify mirror's signature by hoisting branch-existence checks and empty-destination logic into syncRepo, and extract a remoteURL helper to eliminate the redundant destUrl parameter. Co-Authored-By: Claude Opus 4.6 --- cmd/private-org-sync/main.go | 210 ++++++++++++++++-------------- cmd/private-org-sync/main_test.go | 188 ++++++++++++++++++++++---- 2 files changed, 275 insertions(+), 123 deletions(-) diff --git a/cmd/private-org-sync/main.go b/cmd/private-org-sync/main.go index c8a05b5a0bb..6d40cc797fd 100644 --- a/cmd/private-org-sync/main.go +++ b/cmd/private-org-sync/main.go @@ -307,6 +307,22 @@ func (l location) String() string { return fmt.Sprintf("%s/%s@%s", l.org, l.repo, l.branch) } +func (l location) remoteName() string { + return fmt.Sprintf("%s-%s", l.org, l.repo) +} + +func (g gitSyncer) remoteURL(l location) (string, error) { + raw := fmt.Sprintf("%s/%s/%s", g.prefix, l.org, l.repo) + parsed, err := url.Parse(raw) + if err != nil { + return "", fmt.Errorf("failed to parse remote URL %q: %w", raw, err) + } + if g.token != "" { + parsed.User = url.User(g.token) + } + return parsed.String(), nil +} + // makeGitDir creates a directory for a local git repo used for fetching content // from the given location and pushing it to any other git repo func (g gitSyncer) makeGitDir(org, repo string) (string, error) { @@ -343,13 +359,90 @@ func (g gitSyncer) initRepo(repoDir, org, repo string) error { return nil } -// mirror syncs a single branch from source to destination, using pre-fetched -// branch head information. The `repoDir` must have been previously initialized -// with git init and remote setup. The `srcHeads` and `dstHeads` must have been -// obtained from ls-remote calls against the source and destination repos. -// Multiple `mirror` calls over the same `repoDir` will reuse the content -// fetched in previous calls, acting like a cache. -func (g gitSyncer) mirror(repoDir string, src, dst location, srcHeads, dstHeads RemoteBranchHeads, destUrl *url.URL) error { +// syncRepo initializes a local git repo, fetches branch heads from both source +// and destination via ls-remote, and mirrors each branch that needs syncing. +func (g gitSyncer) syncRepo(org, repo, targetOrg, dstRepo string, branches []location) error { + repoLogger := g.logger + + gitDir, err := g.makeGitDir(org, repo) + if err != nil { + return err + } + + if err := g.initRepo(gitDir, org, repo); err != nil { + return err + } + + // ls-remote destination once per repo + // branches is guaranteed non-empty: the caller groups by (org, repo) via append + dstLocation := location{org: targetOrg, repo: dstRepo, branch: branches[0].branch} + destUrl, err := g.remoteURL(dstLocation) + if err != nil { + repoLogger.WithError(err).Error("Failed to construct URL for the destination remote") + return err + } + + dstHeads, err := getRemoteBranchHeads(repoLogger, g.git, gitDir, destUrl) + if err != nil { + message := "destination repository does not exist or we cannot access it" + if g.failOnNonexistentDst { + repoLogger.Errorf("%s", message) + return fmt.Errorf("%s", message) + } + repoLogger.Warn(message) + return nil + } + + // ls-remote source once per repo + srcRemote := branches[0].remoteName() + srcHeads, err := getRemoteBranchHeads(repoLogger, withRetryOnNonzero(g.git, 5), gitDir, srcRemote) + if err != nil { + repoLogger.WithError(err).Error("Failed to determine branch HEADs in source") + return fmt.Errorf("failed to determine branch HEADs in source: %w", err) + } + + initialDepth := startDepth + if len(dstHeads) == 0 { + repoLogger.Info("Destination is an empty repo: will do a full fetch right away") + initialDepth = fullFetch + } + + var errs []error + for _, source := range branches { + branchSyncer := g + branchSyncer.logger = config.LoggerForInfo(config.Info{ + Metadata: api.Metadata{ + Org: source.org, + Repo: source.repo, + Branch: source.branch, + }, + }) + + destination := location{org: targetOrg, repo: dstRepo, branch: source.branch} + srcCommitHash, srcFound := srcHeads[source.branch] + if !srcFound { + if api.FlavorForBranch(source.branch) == "misc" { + branchSyncer.logger.Warn("Non-release branch does not exist in source remote, likely deleted; skipping") + continue + } + branchSyncer.logger.Error("Release/main branch does not exist in source remote; this may indicate the branch was deleted") + errs = append(errs, fmt.Errorf("%s: branch does not exist in source remote", source.String())) + continue + } + + if err := branchSyncer.mirror(gitDir, source, destination, srcCommitHash, dstHeads[source.branch], initialDepth); err != nil { + errs = append(errs, fmt.Errorf("%s->%s: %w", source.String(), destination.String(), err)) + } + } + + return utilerrors.NewAggregate(errs) +} + +// mirror syncs a single branch from source to destination. The `repoDir` must +// have been previously initialized with git init and remote setup. Multiple +// `mirror` calls over the same `repoDir` will reuse the content fetched in +// previous calls, acting like a cache. +func (g gitSyncer) mirror(repoDir string, src, dst location, srcCommitHash string, dstCommitHash string, initialDepth int) error { mirrorFields := logrus.Fields{ "source": src.String(), "destination": dst.String(), @@ -358,30 +451,20 @@ func (g gitSyncer) mirror(repoDir string, src, dst location, srcHeads, dstHeads logger := g.logger.WithFields(mirrorFields) logger.Info("Syncing content between locations") - dstCommitHash := dstHeads[dst.branch] - - srcRemote := fmt.Sprintf("%s-%s", src.org, src.repo) - - srcCommitHash, ok := srcHeads[src.branch] - if !ok { - if api.FlavorForBranch(src.branch) == "misc" { - logger.Warn("Non-release branch does not exist in source remote, likely deleted; skipping") - return nil - } - logger.Error("Release/main branch does not exist in source remote; this may indicate the branch was deleted") - return fmt.Errorf("branch does not exist in source remote") + destUrl, err := g.remoteURL(dst) + if err != nil { + logger.WithError(err).Error("Failed to construct URL for the destination remote") + return err } + srcRemote := src.remoteName() + if srcCommitHash == dstCommitHash { logger.Info("Branches are already in sync") return nil } - depth := startDepth - if len(dstHeads) == 0 { - logger.Info("Destination is an empty repo: will do a full fetch right away") - depth = fullFetch - } + depth := initialDepth push := func() (retry func() error, err error) { cmd := []string{"push", "--tags"} @@ -390,7 +473,7 @@ func (g gitSyncer) mirror(repoDir string, src, dst location, srcHeads, dstHeads cmd = append(cmd, "--dry-run") logDryRun = " (dry-run)" } - cmd = append(cmd, destUrl.String(), fmt.Sprintf("FETCH_HEAD:refs/heads/%s", dst.branch)) + cmd = append(cmd, destUrl, fmt.Sprintf("FETCH_HEAD:refs/heads/%s", dst.branch)) logger.Infof("Pushing to destination%s", logDryRun) out, exitCode, err := g.git(logger, repoDir, cmd...) @@ -407,7 +490,7 @@ func (g gitSyncer) mirror(repoDir string, src, dst location, srcHeads, dstHeads if depth == unshallow { logger.Info("Trying to fetch source and destination full history and perform a merge") - if err := mergeRemotesAndPush(logger, g.git, repoDir, srcRemote, dst.branch, destUrl.String(), g.confirm, g.gitName, g.gitEmail); err != nil { + if err := mergeRemotesAndPush(logger, g.git, repoDir, srcRemote, dst.branch, destUrl, g.confirm, g.gitName, g.gitEmail); err != nil { return nil, fmt.Errorf("failed to fetch remote and merge: %w", err) } return nil, nil @@ -423,7 +506,7 @@ func (g gitSyncer) mirror(repoDir string, src, dst location, srcHeads, dstHeads switch strings.TrimSpace(shallowOut) { case "false": logger.Info("Trying to fetch source and destination full history and perform a merge") - if err := mergeRemotesAndPush(logger, g.git, repoDir, srcRemote, dst.branch, destUrl.String(), g.confirm, g.gitName, g.gitEmail); err != nil { + if err := mergeRemotesAndPush(logger, g.git, repoDir, srcRemote, dst.branch, destUrl, g.confirm, g.gitName, g.gitEmail); err != nil { return nil, fmt.Errorf("failed to fetch remote and merge: %w", err) } return nil, nil @@ -634,79 +717,12 @@ func main() { } for key, branches := range grouped { - repoLogger := logrus.WithFields(logrus.Fields{"org": key.org, "repo": key.repo}) - syncer.logger = repoLogger + syncer.logger = logrus.WithFields(logrus.Fields{"org": key.org, "repo": key.repo}) dstRepo := privateorg.MirroredRepoName(key.org, key.repo, flattenedOrgs) - gitDir, err := syncer.makeGitDir(key.org, key.repo) - if err != nil { - for _, source := range branches { - errs = append(errs, fmt.Errorf("%s: %w", source.String(), err)) - } - continue - } - - if err := syncer.initRepo(gitDir, key.org, key.repo); err != nil { - for _, source := range branches { - errs = append(errs, fmt.Errorf("%s: %w", source.String(), err)) - } - continue - } - - // ls-remote destination once per repo - destUrlRaw := fmt.Sprintf("%s/%s/%s", syncer.prefix, o.targetOrg, dstRepo) - destUrl, err := url.Parse(destUrlRaw) - if err != nil { - repoLogger.WithField("remote-url", destUrlRaw).WithError(err).Error("Failed to construct URL for the destination remote") - for _, source := range branches { - errs = append(errs, fmt.Errorf("%s: failed to construct URL for the destination remote", source.String())) - } - continue - } - if syncer.token != "" { - destUrl.User = url.User(syncer.token) - } - - dstHeads, err := getRemoteBranchHeads(repoLogger, syncer.git, gitDir, destUrl.String()) - if err != nil { - message := "destination repository does not exist or we cannot access it" - if syncer.failOnNonexistentDst { - repoLogger.Errorf("%s", message) - for _, source := range branches { - errs = append(errs, fmt.Errorf("%s: %s", source.String(), message)) - } - } else { - repoLogger.Warn(message) - } - continue - } - - // ls-remote source once per repo - srcRemote := fmt.Sprintf("%s-%s", key.org, key.repo) - srcHeads, err := getRemoteBranchHeads(repoLogger, withRetryOnNonzero(syncer.git, 5), gitDir, srcRemote) - if err != nil { - repoLogger.WithError(err).Error("Failed to determine branch HEADs in source") - for _, source := range branches { - errs = append(errs, fmt.Errorf("%s: failed to determine branch HEADs in source", source.String())) - } - continue - } - - for _, source := range branches { - syncer.logger = config.LoggerForInfo(config.Info{ - Metadata: api.Metadata{ - Org: source.org, - Repo: source.repo, - Branch: source.branch, - }, - }) - - destination := location{org: o.targetOrg, repo: dstRepo, branch: source.branch} - - if err := syncer.mirror(gitDir, source, destination, srcHeads, dstHeads, destUrl); err != nil { - errs = append(errs, fmt.Errorf("%s->%s: %w", source.String(), destination.String(), err)) - } + if err := syncer.syncRepo(key.org, key.repo, o.targetOrg, dstRepo, branches); err != nil { + errs = append(errs, err) } } diff --git a/cmd/private-org-sync/main_test.go b/cmd/private-org-sync/main_test.go index 11fa13016b8..16038a4e6dd 100644 --- a/cmd/private-org-sync/main_test.go +++ b/cmd/private-org-sync/main_test.go @@ -2,7 +2,6 @@ package main import ( "fmt" - "net/url" "strings" "testing" "time" @@ -256,7 +255,6 @@ func TestMirror(t *testing.T) { token := "TOKEN" org, repo, branch := "org", "repo", "branch" destOrg := "dest" - destUrl, _ := url.Parse(fmt.Sprintf("https://%s@github.com/%s/%s", token, destOrg, repo)) testCases := []struct { description string @@ -348,29 +346,6 @@ func TestMirror(t *testing.T) { srcHeads: RemoteBranchHeads{branch: "same-sha"}, dstHeads: RemoteBranchHeads{branch: "same-sha"}, }, - { - description: "non-release source branch does not exist -> no error, skip with warning", - src: location{org: org, repo: repo, branch: branch}, - dst: location{org: destOrg, repo: repo, branch: branch}, - srcHeads: RemoteBranchHeads{"not-the-branch": "some-sha"}, - dstHeads: RemoteBranchHeads{branch: "dest-sha"}, - }, - { - description: "release source branch does not exist -> error", - src: location{org: org, repo: repo, branch: "release-4.21"}, - dst: location{org: destOrg, repo: repo, branch: "release-4.21"}, - srcHeads: RemoteBranchHeads{"not-the-branch": "some-sha"}, - dstHeads: RemoteBranchHeads{"release-4.21": "dest-sha"}, - expectError: true, - }, - { - description: "main source branch does not exist -> error", - src: location{org: org, repo: repo, branch: "main"}, - dst: location{org: destOrg, repo: repo, branch: "main"}, - srcHeads: RemoteBranchHeads{"not-the-branch": "some-sha"}, - dstHeads: RemoteBranchHeads{"main": "dest-sha"}, - expectError: true, - }, { description: "destination is empty repo -> full fetch then success", src: location{org: org, repo: repo, branch: branch}, @@ -593,7 +568,11 @@ hint: See the 'Note about fast-forwards' in 'git push --help' for details. gitName: "openshift-bot", gitEmail: "openshift-bot@redhat.com", } - err := m.mirror("repo-dir", tc.src, tc.dst, tc.srcHeads, tc.dstHeads, destUrl) + initialDepth := startDepth + if len(tc.dstHeads) == 0 { + initialDepth = fullFetch + } + err := m.mirror("repo-dir", tc.src, tc.dst, tc.srcHeads[tc.src.branch], tc.dstHeads[tc.dst.branch], initialDepth) if err == nil && tc.expectError { t.Error("expected error, got nil") } @@ -682,6 +661,163 @@ func TestInitRepo(t *testing.T) { } } +func TestSyncRepo(t *testing.T) { + second = time.Millisecond + token := "TOKEN" + org, repo := "org", "repo" + targetOrg := "dest" + branches := []location{ + {org: org, repo: repo, branch: "main"}, + {org: org, repo: repo, branch: "release-4.18"}, + } + + testCases := []struct { + description string + branches []location + failOnNonexistentDst bool + expectedGitCalls []mockGitCall + expectError bool + }{ + { + description: "all branches in sync -> init, ls-remote, no fetch/push", + branches: branches, + expectedGitCalls: []mockGitCall{ + {call: "init"}, + {call: "remote get-url org-repo", exitCode: 1}, + {call: "remote add org-repo https://TOKEN@github.com/org/repo"}, + {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "aaa refs/heads/main\naaa refs/heads/release-4.18\n"}, + {call: "ls-remote --heads org-repo", output: "aaa refs/heads/main\naaa refs/heads/release-4.18\n"}, + }, + }, + { + description: "one branch needs sync -> fetches and pushes that branch only", + branches: branches, + expectedGitCalls: []mockGitCall{ + {call: "init"}, + {call: "remote get-url org-repo", exitCode: 1}, + {call: "remote add org-repo https://TOKEN@github.com/org/repo"}, + {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "aaa refs/heads/main\nbbb refs/heads/release-4.18\n"}, + {call: "ls-remote --heads org-repo", output: "aaa refs/heads/main\naaa refs/heads/release-4.18\n"}, + // only release-4.18 needs sync (bbb != aaa) + {call: "fetch --tags org-repo release-4.18 --depth=2"}, + {call: "push --tags --dry-run https://TOKEN@github.com/dest/repo FETCH_HEAD:refs/heads/release-4.18"}, + }, + }, + { + description: "dst ls-remote fails, failOnNonexistentDst=true -> error", + branches: branches, + failOnNonexistentDst: true, + expectedGitCalls: []mockGitCall{ + {call: "init"}, + {call: "remote get-url org-repo", exitCode: 1}, + {call: "remote add org-repo https://TOKEN@github.com/org/repo"}, + {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", exitCode: 1}, + }, + expectError: true, + }, + { + description: "dst ls-remote fails, failOnNonexistentDst=false -> no error (skip)", + branches: branches, + failOnNonexistentDst: false, + expectedGitCalls: []mockGitCall{ + {call: "init"}, + {call: "remote get-url org-repo", exitCode: 1}, + {call: "remote add org-repo https://TOKEN@github.com/org/repo"}, + {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", exitCode: 1}, + }, + }, + { + description: "src ls-remote fails -> error", + branches: branches, + expectedGitCalls: []mockGitCall{ + {call: "init"}, + {call: "remote get-url org-repo", exitCode: 1}, + {call: "remote add org-repo https://TOKEN@github.com/org/repo"}, + {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "aaa refs/heads/main\n"}, + // src ls-remote fails with retries (withRetryOnNonzero does 5 retries) + {call: "ls-remote --heads org-repo", exitCode: 1}, + {call: "ls-remote --heads org-repo", exitCode: 1}, + {call: "ls-remote --heads org-repo", exitCode: 1}, + {call: "ls-remote --heads org-repo", exitCode: 1}, + {call: "ls-remote --heads org-repo", exitCode: 1}, + }, + expectError: true, + }, + { + description: "init fails -> error", + branches: branches, + expectedGitCalls: []mockGitCall{ + {call: "init", exitCode: 1}, + }, + expectError: true, + }, + { + description: "non-release source branch does not exist -> skip with warning, no error", + branches: []location{{org: org, repo: repo, branch: "some-feature"}}, + expectedGitCalls: []mockGitCall{ + {call: "init"}, + {call: "remote get-url org-repo", exitCode: 1}, + {call: "remote add org-repo https://TOKEN@github.com/org/repo"}, + {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "aaa refs/heads/some-feature\n"}, + {call: "ls-remote --heads org-repo", output: "aaa refs/heads/other-branch\n"}, + }, + }, + { + description: "release source branch does not exist -> error", + branches: []location{{org: org, repo: repo, branch: "release-4.21"}}, + expectedGitCalls: []mockGitCall{ + {call: "init"}, + {call: "remote get-url org-repo", exitCode: 1}, + {call: "remote add org-repo https://TOKEN@github.com/org/repo"}, + {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "aaa refs/heads/release-4.21\n"}, + {call: "ls-remote --heads org-repo", output: "aaa refs/heads/other-branch\n"}, + }, + expectError: true, + }, + { + description: "main source branch does not exist -> error", + branches: []location{{org: org, repo: repo, branch: "main"}}, + expectedGitCalls: []mockGitCall{ + {call: "init"}, + {call: "remote get-url org-repo", exitCode: 1}, + {call: "remote add org-repo https://TOKEN@github.com/org/repo"}, + {call: "ls-remote --heads https://TOKEN@github.com/dest/repo", output: "aaa refs/heads/main\n"}, + {call: "ls-remote --heads org-repo", output: "aaa refs/heads/other-branch\n"}, + }, + expectError: true, + }, + } + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + git := mockGit{ + expected: tc.expectedGitCalls, + t: t, + } + s := gitSyncer{ + logger: logrus.WithField("test", tc.description), + prefix: defaultPrefix, + token: token, + root: "git-dir", + git: git.exec, + confirm: false, + failOnNonexistentDst: tc.failOnNonexistentDst, + gitName: "openshift-bot", + gitEmail: "openshift-bot@redhat.com", + } + err := s.syncRepo(org, repo, targetOrg, repo, tc.branches) + if err == nil && tc.expectError { + t.Error("expected error, got nil") + } + if err != nil && !tc.expectError { + t.Errorf("unexpected error: %v", err) + } + if err := git.check(); err != nil { + t.Errorf("bad git operation: %v", err) + } + }) + } +} + func TestDestinationNaming(t *testing.T) { testCases := []struct { name string