From aab201c862a7df5176255645b594295efd99872a Mon Sep 17 00:00:00 2001 From: Ratchanan Srirattanamet Date: Thu, 3 Oct 2024 13:59:38 +0000 Subject: [PATCH] fix: cherry-pick order on GitLab by reversing commmit list only once GitLabClient already handle commit order reversing, so re-handle it in Runner causes cherry-pick order on GitLab to be wrong. Remove commit order handling from Runner, and instead handle difference between GitHub and Codeberg inside GitHubClient. Now, since the default of --bp-branch-name takes the commit list from {GitHub,GitLab}Client directly, that means backporting branch name on Codeberg will also be changed to have commits in the correct order too (old to new, in line with GitHub and GitLab), which is IMO a nice bonus. --- dist/cli/index.js | 13 +++++-------- dist/gha/index.js | 13 +++++-------- src/service/git/github/github-client.ts | 5 +++++ src/service/runner/runner.ts | 8 -------- .../service/runner/cli-codeberg-runner.test.ts | 18 +++++++++--------- test/service/runner/cli-gitlab-runner.test.ts | 4 ++-- 6 files changed, 26 insertions(+), 35 deletions(-) diff --git a/dist/cli/index.js b/dist/cli/index.js index ae3b942..4c1b9df 100755 --- a/dist/cli/index.js +++ b/dist/cli/index.js @@ -884,6 +884,11 @@ class GitHubClient { pull_number: prNumber, }); commits.push(...data.map(c => c.sha)); + if (this.isForCodeberg) { + // For some reason, even though Codeberg advertises API compatibility + // with GitHub, it returns commits in reversed order. + commits.reverse(); + } } catch (error) { throw new Error(`Failed to retrieve commits for pull request n. ${prNumber}`); @@ -1619,14 +1624,6 @@ class Runner { } // 7. apply all changes to the new branch this.logger.debug("Cherry picking commits.."); - // Commits might be ordered in different ways based on the git service, e.g., considering top to bottom - // GITHUB --> oldest to newest - // CODEBERG --> newest to oldest - // GITLAB --> newest to oldest - if (git.gitClientType === git_types_1.GitClientType.CODEBERG || git.gitClientType === git_types_1.GitClientType.GITLAB) { - // reverse the order as we always need to process from older to newest - originalPR.commits.reverse(); - } for (const sha of originalPR.commits) { await git.gitCli.cherryPick(configs.folder, sha, configs.mergeStrategy, configs.mergeStrategyOption, configs.cherryPickOptions); } diff --git a/dist/gha/index.js b/dist/gha/index.js index f2434d6..aaa35f5 100755 --- a/dist/gha/index.js +++ b/dist/gha/index.js @@ -849,6 +849,11 @@ class GitHubClient { pull_number: prNumber, }); commits.push(...data.map(c => c.sha)); + if (this.isForCodeberg) { + // For some reason, even though Codeberg advertises API compatibility + // with GitHub, it returns commits in reversed order. + commits.reverse(); + } } catch (error) { throw new Error(`Failed to retrieve commits for pull request n. ${prNumber}`); @@ -1584,14 +1589,6 @@ class Runner { } // 7. apply all changes to the new branch this.logger.debug("Cherry picking commits.."); - // Commits might be ordered in different ways based on the git service, e.g., considering top to bottom - // GITHUB --> oldest to newest - // CODEBERG --> newest to oldest - // GITLAB --> newest to oldest - if (git.gitClientType === git_types_1.GitClientType.CODEBERG || git.gitClientType === git_types_1.GitClientType.GITLAB) { - // reverse the order as we always need to process from older to newest - originalPR.commits.reverse(); - } for (const sha of originalPR.commits) { await git.gitCli.cherryPick(configs.folder, sha, configs.mergeStrategy, configs.mergeStrategyOption, configs.cherryPickOptions); } diff --git a/src/service/git/github/github-client.ts b/src/service/git/github/github-client.ts index 6548429..9802ef5 100644 --- a/src/service/git/github/github-client.ts +++ b/src/service/git/github/github-client.ts @@ -73,6 +73,11 @@ export default class GitHubClient implements GitClient { }); commits.push(...data.map(c => c.sha)); + if (this.isForCodeberg) { + // For some reason, even though Codeberg advertises API compatibility + // with GitHub, it returns commits in reversed order. + commits.reverse(); + } } catch(error) { throw new Error(`Failed to retrieve commits for pull request n. ${prNumber}`); } diff --git a/src/service/runner/runner.ts b/src/service/runner/runner.ts index fed7a70..4df541d 100644 --- a/src/service/runner/runner.ts +++ b/src/service/runner/runner.ts @@ -152,14 +152,6 @@ export default class Runner { // 7. apply all changes to the new branch this.logger.debug("Cherry picking commits.."); - // Commits might be ordered in different ways based on the git service, e.g., considering top to bottom - // GITHUB --> oldest to newest - // CODEBERG --> newest to oldest - // GITLAB --> newest to oldest - if (git.gitClientType === GitClientType.CODEBERG || git.gitClientType === GitClientType.GITLAB) { - // reverse the order as we always need to process from older to newest - originalPR.commits.reverse(); - } for (const sha of originalPR.commits) { await git.gitCli.cherryPick(configs.folder, sha, configs.mergeStrategy, configs.mergeStrategyOption, configs.cherryPickOptions); } diff --git a/test/service/runner/cli-codeberg-runner.test.ts b/test/service/runner/cli-codeberg-runner.test.ts index 8ea6a1c..653dea5 100644 --- a/test/service/runner/cli-codeberg-runner.test.ts +++ b/test/service/runner/cli-codeberg-runner.test.ts @@ -370,7 +370,7 @@ describe("cli runner", () => { expect(GitCLIService.prototype.clone).toBeCalledWith("https://codeberg.org/owner/reponame.git", cwd, "target"); expect(GitCLIService.prototype.createLocalBranch).toBeCalledTimes(1); - expect(GitCLIService.prototype.createLocalBranch).toBeCalledWith(cwd, "bp-target-0404fb9-11da4e3"); + expect(GitCLIService.prototype.createLocalBranch).toBeCalledWith(cwd, "bp-target-11da4e3-0404fb9"); expect(GitCLIService.prototype.fetch).toBeCalledTimes(1); expect(GitCLIService.prototype.fetch).toBeCalledWith(cwd, "pull/4444/head:pr/4444"); @@ -380,13 +380,13 @@ describe("cli runner", () => { expect(GitCLIService.prototype.cherryPick).toBeCalledWith(cwd, "11da4e38aa3e577ffde6d546f1c52e53b04d3151", undefined, undefined, undefined); expect(GitCLIService.prototype.push).toBeCalledTimes(1); - expect(GitCLIService.prototype.push).toBeCalledWith(cwd, "bp-target-0404fb9-11da4e3"); + expect(GitCLIService.prototype.push).toBeCalledWith(cwd, "bp-target-11da4e3-0404fb9"); expect(GitHubClient.prototype.createPullRequest).toBeCalledTimes(1); expect(GitHubClient.prototype.createPullRequest).toBeCalledWith({ owner: "owner", repo: "reponame", - head: "bp-target-0404fb9-11da4e3", + head: "bp-target-11da4e3-0404fb9", base: "target", title: "[target] PR Title", body: "**Backport:** https://codeberg.org/owner/reponame/pulls/4444\r\n\r\nPlease review and merge", @@ -728,7 +728,7 @@ describe("cli runner", () => { expect(GitCLIService.prototype.clone).toBeCalledWith("https://codeberg.org/owner/reponame.git", cwd, "target"); expect(GitCLIService.prototype.createLocalBranch).toBeCalledTimes(1); - expect(GitCLIService.prototype.createLocalBranch).toBeCalledWith(cwd, "bp-target-0404fb9-11da4e3"); + expect(GitCLIService.prototype.createLocalBranch).toBeCalledWith(cwd, "bp-target-11da4e3-0404fb9"); expect(GitCLIService.prototype.fetch).toBeCalledTimes(0); @@ -737,13 +737,13 @@ describe("cli runner", () => { expect(GitCLIService.prototype.cherryPick).toBeCalledWith(cwd, "11da4e38aa3e577ffde6d546f1c52e53b04d3151", undefined, undefined, undefined); expect(GitCLIService.prototype.push).toBeCalledTimes(1); - expect(GitCLIService.prototype.push).toBeCalledWith(cwd, "bp-target-0404fb9-11da4e3"); + expect(GitCLIService.prototype.push).toBeCalledWith(cwd, "bp-target-11da4e3-0404fb9"); expect(GitHubClient.prototype.createPullRequest).toBeCalledTimes(1); expect(GitHubClient.prototype.createPullRequest).toBeCalledWith({ owner: "owner", repo: "reponame", - head: "bp-target-0404fb9-11da4e3", + head: "bp-target-11da4e3-0404fb9", base: "target", title: "[target] PR Title", body: "**Backport:** https://codeberg.org/owner/reponame/pulls/8632\r\n\r\nPlease review and merge", @@ -834,7 +834,7 @@ describe("cli runner", () => { expect(GitCLIService.prototype.clone).toBeCalledWith("https://codeberg.org/owner/reponame.git", cwd, "target"); expect(GitCLIService.prototype.createLocalBranch).toBeCalledTimes(1); - expect(GitCLIService.prototype.createLocalBranch).toBeCalledWith(cwd, "bp-target-0404fb9-11da4e3"); + expect(GitCLIService.prototype.createLocalBranch).toBeCalledWith(cwd, "bp-target-11da4e3-0404fb9"); expect(GitCLIService.prototype.fetch).toBeCalledTimes(0); @@ -843,13 +843,13 @@ describe("cli runner", () => { expect(GitCLIService.prototype.cherryPick).toBeCalledWith(cwd, "11da4e38aa3e577ffde6d546f1c52e53b04d3151", "ort", "find-renames", undefined); expect(GitCLIService.prototype.push).toBeCalledTimes(1); - expect(GitCLIService.prototype.push).toBeCalledWith(cwd, "bp-target-0404fb9-11da4e3"); + expect(GitCLIService.prototype.push).toBeCalledWith(cwd, "bp-target-11da4e3-0404fb9"); expect(GitHubClient.prototype.createPullRequest).toBeCalledTimes(1); expect(GitHubClient.prototype.createPullRequest).toBeCalledWith({ owner: "owner", repo: "reponame", - head: "bp-target-0404fb9-11da4e3", + head: "bp-target-11da4e3-0404fb9", base: "target", title: "[target] PR Title", body: "**Backport:** https://codeberg.org/owner/reponame/pulls/8632\r\n\r\nPlease review and merge", diff --git a/test/service/runner/cli-gitlab-runner.test.ts b/test/service/runner/cli-gitlab-runner.test.ts index 5d15911..ae26d36 100644 --- a/test/service/runner/cli-gitlab-runner.test.ts +++ b/test/service/runner/cli-gitlab-runner.test.ts @@ -582,8 +582,8 @@ describe("cli runner", () => { expect(GitCLIService.prototype.fetch).toBeCalledWith(cwd, "merge-requests/2/head:pr/2"); expect(GitCLIService.prototype.cherryPick).toBeCalledTimes(2); - expect(GitCLIService.prototype.cherryPick).toBeCalledWith(cwd, "e4dd336a4a20f394df6665994df382fb1d193a11", undefined, undefined, undefined); - expect(GitCLIService.prototype.cherryPick).toBeCalledWith(cwd, "974519f65c9e0ed65277cd71026657a09fca05e7", undefined, undefined, undefined); + expect(GitCLIService.prototype.cherryPick).toHaveBeenNthCalledWith(1, cwd, "e4dd336a4a20f394df6665994df382fb1d193a11", undefined, undefined, undefined); + expect(GitCLIService.prototype.cherryPick).toHaveBeenNthCalledWith(2, cwd, "974519f65c9e0ed65277cd71026657a09fca05e7", undefined, undefined, undefined); expect(GitCLIService.prototype.push).toBeCalledTimes(1); expect(GitCLIService.prototype.push).toBeCalledWith(cwd, "bp-target-e4dd336-974519f");