fix: cherry-pick order on GitLab by reversing commmit list only once (#…

…137)

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

@@ -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);
         }

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);
         }

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}`);
       }

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);
     }

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",

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");