Auto-detect the value of the no-squash option #118

Closed earl-warren closed 5 months ago

The auto-no-squash option is added to:

backport all the commits when the pull/merge request has been merged
backport the squashed commit otherwise

It is equivalent to dynamically adjust the value of the no-squash option, depending on the context.

The no-squash option is kept for backward compatibility for a single use case: backporting the merged commit instead of backporting the commits of the pull/merge request request.

Detecting if a pull/merge request was squashed or not depends on the underlying forge:

Forgejo / GitHub: use the API to count the number of parents
GitLab: if the squash_commit_sha is set, the merge request was squashed

If the pull/merge request is open, always backport all the commits it contains.

Fixes: https://github.com/kiegroup/git-backporting/issues/113

Coverage report

St.^{:grey_question:}	Category	Percentage	Covered / Total
🟢	Statements	91.3% (+0.46% 🔼)	472/517
🟢	Branches	86.19% (+0.34% 🔼)	181/210
🟢	Functions	88.43% (+0.1% 🔼)	107/121
🟢	Lines	91.24% (+0.46% 🔼)	458/502

Show files with reduced coverage 🔻

|

St.^{:grey_question:}

| File | Statements | Branches | Functions | Lines | | :-: | :- | :- | :- | :- | :- | | 🟢 |

`...` / gitlab-client.ts

|

93.98% (+0.23% 🔼)

|

81.82% (-1.52% 🔻)

| 91.67% |

93.67% (+0.25% 🔼)

|

Test suite run success

177 tests passing in 16 suites.

Report generated by 🧪jest coverage report action from f591021c060ae9673ab003e4597465447c0d765f

The original approach was not good because at the time the PR is examined the clone is not available and it is not possible to run git show to know more about it.

Here is another approach:

the list of commits is always extracted from the PR
the sha (from the head or the merge_commit_sha) is kept around to help decide later on

After the clone is done, the following pseudo logic chooses the commits that should be cherry-picked:

  async getPrCommits(configs: Configs, originalPR: GitPullRequest, git: Git): Promise<string[]> {
    let squash = configs.squash
    if (originalPR.state == "open") {
      squash = false
    } else if (squash === undefined) {
      squash = !git.isMerge(originalPR.sha)
    }
    if squash {
      return [originalPR.sha]
    } else {
      return [originalPR.commits]
    }
  }

Let me know if it is a sound approach and I'll keep going.

P.S. I think it never makes sense to have squash = true when the PR is still open but maybe there is a valid use case I did not think of.

Now that I think about it... another way would simply be to clone before inspecting the PR so the existing logic can git show because the clone is already present. Probably simpler?

Or not... clone would require the config to be available but that same config is the one parsing the PR so there is a chicken & egg problem.

Both have the parents.

$ curl -sS https://code.forgejo.org/api/v1/repos/forgejo/runner/git/commits/eb89a98c6a401bc0afc6f9a897a82de0dfcb9d8d | jq .parents
[
  {
    "url": "https://code.forgejo.org/api/v1/repos/forgejo/runner/git/commits/781606388cb46645bb84a31b48e081a4d350baea",
    "sha": "781606388cb46645bb84a31b48e081a4d350baea",
    "created": "0001-01-01T00:00:00Z"
  },
  {
    "url": "https://code.forgejo.org/api/v1/repos/forgejo/runner/git/commits/4f4ec159f0256b1d393cca5fae6732754a0a181b",
    "sha": "4f4ec159f0256b1d393cca5fae6732754a0a181b",
    "created": "0001-01-01T00:00:00Z"
  }
]

Looks like the better option, at he expense of an additional request. But... since it will git clone an entire repository shortly after wards it seems futile to save this. Or am I being careless?

Here is a draft using the API endpoints to determine if the commit is a merge or not. It feels right this time.

It was semi-manually tested at https://code.forgejo.org/earl-warren/racepr with something like

git fetch origin ; git reset --hard origin/main ; branch=b$(date +%s) ; date > adsf ; git add adsf ; git commit -m 'update one' ; sleep 1 ; date > adsf ; git add adsf ; git commit -m 'update two' ; git push origin HEAD:$branch ; pr=$(forgejo-curl.sh api_json --data '{"title":"t1","head":"'$branch'","base":"main"}' https://code.forgejo.org/api/v1/repos/earl-warren/racepr/pulls | jq -r .number) && forgejo-curl.sh api_json  --data '{"Do": "merge"}' https://code.forgejo.org/api/v1/repos/earl-warren/racepr/pulls/$pr/merge

and

git fetch origin ; git reset --hard origin/main ; branch=b$(date +%s) ; date > adsf ; git add adsf ; git commit -m 'update one' ; sleep 1 ; date > adsf ; git add adsf ; git commit -m 'update two' ; git push origin HEAD:$branch ; pr=$(forgejo-curl.sh api_json --data '{"title":"t1","head":"'$branch'","base":"main"}' https://code.forgejo.org/api/v1/repos/earl-warren/racepr/pulls | jq -r .number) && forgejo-curl.sh api_json  --data '{"Do": "squash"}' https://code.forgejo.org/api/v1/repos/earl-warren/racepr/pulls/$pr/merge

and the workflow

on:
  pull_request_target:
    types:
      - closed
      - labeled

jobs:
  backporting:
    runs-on: docker
    container:
      image: 'docker.io/node:20-bookworm'
    steps:
      - uses: https://github.com/earl-warren/git-backporting@b2554a678d5ea2814f0df3abad2ac4fcdee2d81f
        with:
          target-branch: other
          cherry-pick-options: -x
          git-client: codeberg
          auth: ${{ secrets.BACKPORT_TOKEN }}
          pull-request: ${{ github.event.pull_request.url }}

that does not mean it is the right way to do it but it is not complete nonsense either :smile:

Hey @earl-warren , that is a great investigation and I was not aware of that way to determine whether a commit is a merge-commit or not, thus it looks a valid option to evaluate.

at he expense of an additional request. But... since it will git clone an entire repository shortly after wards it seems futile to save this. Or am I being careless?

No that's not a problem at all, I mean we don't have tight time restrictions here.

It was semi-manually tested at https://code.forgejo.org/earl-warren/racepr with something like

That's already a good early check, I will try to get some time today to review this approach as well! Moreover I'll try to check whether we can use a similar approach for Gitlab as I'd like to keep all git platform feature-equivalent (but, in that case, we can do that in another pr)

I'll complete this PR today and include GitLab as well. This is going to be good.

Hi @earl-warren, I had chance to take a look at this PR (at least from the generic workflow point of view) and I think it could work. I did not try yet but based on the changes you did I think the idea could work as long as this https://github.com/kiegroup/git-backporting/pull/118/files#diff-6e3804f9a1581e5155295a17bbbc71343fa375a15bf5383dd4c4049a44be09cdR54-R60 is enough/true.

There a couple of enhancement that could be done, e.g., now we could populate the commits variable directly from the the getPullRequest method since we already now the commits that need to be backported, but this is something we can do step by step.

I'll complete this PR today and include GitLab as well. This is going to be good.

I'll work on this PR today.

Side note: this commit is in production at https://codeberg.org/forgejo/forgejo/pulls a behaves as it should.

Side note: this commit is in production at https://codeberg.org/forgejo/forgejo/pulls a behaves as it should.

That's great!!!

I think this is a sane implementation. I thought it would be possible to just remove the no-squash option and rely on auto-detection for all cases. But there is one case that would create a non-backward compatible behavior (cherry-picking the merge commit). I also tried to change the no-squash option to have three state (undefined, true, false) instead of two. Adding another option is not elegant but it is easier to understand and makes for a clear implementation.

The GitLab support is made easy by the fact that the merge request has information regarding the status of the merge.

The PR is ready for review but I will leave it as a WIP: because it still lacks coverage for the logic. It would be great if you could review it before I polish it, in case there are changes needed that would require modifying the tests.

I really enjoy working on this action with you.

I think this is a sane implementation. I thought it would be possible to just remove the no-squash option and rely on auto-detection for all cases. But there is one case that would create a non-backward compatible behavior (cherry-picking the merge commit). I also tried to change the no-squash option to have three state (undefined, true, false) instead of two. Adding another option is not elegant but it is easier to understand and makes for a clear implementation.

Thanks a lot @earl-warren :heart:

The GitLab support is made easy by the fact that the merge request has information regarding the status of the merge.

That's really good to hear :smile:

The PR is ready for review but I will leave it as a WIP: because it still lacks coverage for the logic. It would be great if you could review it before I polish it, in case there are changes needed that would require modifying the tests.

Yeah I agree with you, I think I can find some time today to review this - it should not take much time as we were already on the same page.

I really enjoy working on this action with you.

+100, I am really glad there are people like you who are very proactive on contributing back, great example of community! Moreover I hope that this action will really save some of your day2day work time :crossed_fingers:

It is already busy at work saving valuable time every day. :sparkles:

Ok so if I correctly understood the logic, that can be summarized as follow (correct me if I am wrong): noSquash autoNoSquash SQUASH Behavior false false true Try to cherry-pick single commit, merge_commit_sha true false false Try to cherry-pick all commits in the original pull request false true undefined true true undefined
* When `autoNoSquash` is undefined/false the behavior is the one already existing.

* Whereas when the `autoNoSquash` is set to true (*) the value of `noSquash` is _ignored_ and the value of `squash` is auto computed by the new logic https://github.com/kiegroup/git-backporting/pull/118/files#diff-6e3804f9a1581e5155295a17bbbc71343fa375a15bf5383dd4c4049a44be09cdR48-R66
Am I right?

Yes.

If I am right I think the overall implementation looks really great! I am trying to see if we can simplify the logic but nothing came to my mind, so I think we can proceed with this approach 🚀

If it was not for the case where you want to cherry-pick a merge commit, I think both no-squash and auto-no-squash could be removed. And the default behavior would be to cherry-pick a single commit when there is a squash or all commits if not.

If that behavior is really desirable, there could be a cherry-pick-merge-commit option.

But removing no-squash and adding cherry-pick-merge-commit or something would be a breaking change and you probably do not want that.

I left a couple of comments.

PS: worth to add some table like this in the doc to help users understand how to configure the tool to enable a specific behavior and to highlight the default.

:+1:

PS: worth to add some table like this in the doc to help users understand how to configure the tool to enable a specific behavior and to highlight the default.

A table was added in the README

@lampajr does this look like a good enough test to complete this PR? The logic is simple right now. But it is pivotal to how GitLab/Forgejo/GitHub decide to squash or not and it feels right to keep it DRY, just in case it becomes more complex in the future and really requires good unit testing.

Manual tests.

auto-no-squash: true

      - uses: https://github.com/earl-warren/git-backporting@21d3fe29b9cb1b1c1a0ab0625dcb1b13f407eddc
        with:
          target-branch-pattern: "^backport/(?<target>(o.*))$"
          cherry-pick-options: -x
          git-client: codeberg
          auth: ${{ secrets.BACKPORT_TOKEN }}
          pull-request: ${{ github.event.pull_request.url }}
          auto-no-squash: true

merged commit

https://code.forgejo.org/earl-warren/racepr/actions/runs/26#jobstep-1-4

[DEBUG] Fetching pull request earl-warren/racepr/38
[DEBUG] cherry-pick the merged commit(s)
[DEBUG] Extracting branches from [backport/other] using ^backport/(?<target>(o.*))$

squashed commit

https://code.forgejo.org/earl-warren/racepr/actions/runs/27#jobstep-1-4

[DEBUG] Fetching pull request earl-warren/racepr/40
[DEBUG] cherry-pick the squashed commit 9b2eeb57e74115ee5d0456d983ce8b15ea709042
[DEBUG] Extracting branches from [backport/other] using ^backport/(?<target>(o.*))$

auto-no-squash: unset & no-squash: unset

      - uses: https://github.com/earl-warren/git-backporting@21d3fe29b9cb1b1c1a0ab0625dcb1b13f407eddc
        with:
          target-branch-pattern: "^backport/(?<target>(o.*))$"
          cherry-pick-options: -x
          git-client: codeberg
          auth: ${{ secrets.BACKPORT_TOKEN }}
          pull-request: ${{ github.event.pull_request.url }}

merge commit

https://code.forgejo.org/earl-warren/racepr/actions/runs/28#jobstep-1-10

[INFO ] [other] Creating branch bp-other-c6cc40c
[DEBUG] [other] Cherry picking commits..
[INFO ] [other] Cherry picking c6cc40ca7df7e1efd23b8c6064b674d3c17a872a
[DEBUG] [other] Cherry picking command git cherry-pick,-m,1,--strategy=recursive,--strategy-option=theirs,-x,c6cc40ca7df7e1efd23b8c6064b674d3c17a872a
[INFO ] [other] Pushing bp-other-c6cc40c to origin

squash commit

https://code.forgejo.org/earl-warren/racepr/actions/runs/29#jobstep-1-10

[INFO ] [other] Creating branch bp-other-1d1a9c8
[DEBUG] [other] Cherry picking commits..
[INFO ] [other] Cherry picking 1d1a9c8e5f0add54c4f9936a498e6b874aa88b7a
[DEBUG] [other] Cherry picking command git cherry-pick,-m,1,--strategy=recursive,--strategy-option=theirs,-x,1d1a9c8e5f0add54c4f9936a498e6b874aa88b7a
[INFO ] [other] Pushing bp-other-1d1a9c8 to origin

auto-no-squash: unset & no-squash: true

      - uses: https://github.com/earl-warren/git-backporting@21d3fe29b9cb1b1c1a0ab0625dcb1b13f407eddc
        with:
          target-branch-pattern: "^backport/(?<target>(o.*))$"
          cherry-pick-options: -x
          git-client: codeberg
          auth: ${{ secrets.BACKPORT_TOKEN }}
          pull-request: ${{ github.event.pull_request.url }}
          no-squash: true

https://code.forgejo.org/earl-warren/racepr/actions/runs/30#jobstep-1-8

[DEBUG] [other] Creating local branch..
[INFO ] [other] Creating branch bp-other-51b5105-e9a0bb1
[DEBUG] [other] Cherry picking commits..
[INFO ] [other] Cherry picking e9a0bb1fe756356fb48bf82d4c7718cd12bd71f5
[DEBUG] [other] Cherry picking command git cherry-pick,-m,1,--strategy=recursive,--strategy-option=theirs,-x,e9a0bb1fe756356fb48bf82d4c7718cd12bd71f5
[INFO ] [other] Cherry picking 51b5105cd3ca0f8da1364d24a88556ab8473dd27
[DEBUG] [other] Cherry picking command git cherry-pick,-m,1,--strategy=recursive,--strategy-option=theirs,-x,51b5105cd3ca0f8da1364d24a88556ab8473dd27
[INFO ] [other] Pushing bp-other-51b5105-e9a0bb1 to origin

Changes applied & README updated as suggested.

Changes applied & README updated as suggested.

Merged, thanks a lot for your contribution again @earl-warren :heart:

kiegroup / git-backporting