"Rebase then FF" merged PR is misidentified as Squash-Merged

[BtbN]

This is happening on our Forgejo instance, but I believe the exact same issue would happen on GitHub with its Rebase-Merge style, which is basically identical.

In https://github.com/kiegroup/git-backporting/blob/main/src/service/git/github/github-client.ts#L49 and then https://github.com/kiegroup/git-backporting/blob/main/src/service/git/git-util.ts#L51 what the logical basically does is:

If the PR is:

• closed
• its merge_commit_sha commit only has one parent
• that merge_commit_sha is different than the HEAD commit sha of the PR

Then ultimately inferSquash() returns true, and as a result, only the HEAD commit of the PR is cherry-picked.

The only problem is: All those conditions are also true for a pull request that was "Rebase then ff" merged.
It's closed, the merge commit only has one parent, and unless the PR was fully up to date, it will be different than the PR head sha.

I'm not immediately sure how else to properly detect a squash-merged PR vs. a rebase merged one, but the current logic misidentifies rebase+ff merged ones, and misses potentially all but one commit from them.

[lampajr]

Hi @BtbN, thanks for opening this issue! 🚀

Sounds interesting, do you have any concrete example to share with the expected outcome vs the actual one?

I might find some time to look at this later this week, but if you want to contribute feel free to propose any change and I will be more than happy to double check

[BtbN]

I deleted the broken PR it created, but you can still see the log of the misidentification happening here:
https://code.ffmpeg.org/FFmpeg/FFmpeg/actions/runs/67398/jobs/0/attempt/3
Since it will eventually expire, here's the full log:

[DEBUG] Setting up codeberg client: apiUrl=https://code.ffmpeg.org/api/v1, token=****
[DEBUG] Parsing configs..
[DEBUG] Fetching pull request FFmpeg/FFmpeg/23742
(node:20) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:20) [DEP0169] DeprecationWarning: `url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.
[DEBUG] cherry-pick the squashed commit a633df56a455ffb75cd97af784cb1f2d2ec2bca9
[DEBUG] Extracting branches from [avcodec,backport/release/9.0] using ^backport/(?<target>(.+))$
[DEBUG] [release/9.0] Cloning repo..
[INFO ] [release/9.0] Cloning repository https://code.ffmpeg.org/FFmpeg/FFmpeg.git to /workspace/FFmpeg/FFmpeg/bp
[DEBUG] [release/9.0] Creating local branch..
[INFO ] [release/9.0] Creating branch bp-release/9.0-a633df5
[DEBUG] [release/9.0] Fetching pull request remote..
[INFO ] [release/9.0] Fetching origin pull/23742/head:pr/23742
[DEBUG] [release/9.0] Cherry picking commits..
[INFO ] [release/9.0] Cherry picking a633df56a455ffb75cd97af784cb1f2d2ec2bca9
[DEBUG] [release/9.0] Cherry picking command git cherry-pick,-m,1,--strategy=ort,--strategy-option=find-renames,-x,a633df56a455ffb75cd97af784cb1f2d2ec2bca9
[INFO ] [release/9.0] Pushing bp-release/9.0-a633df5 to https://code.ffmpeg.org/ffmpeg-devel/FFmpeg.git
[INFO ] [release/9.0] Creating pull request bp-release/9.0-a633df5 -> release/9.0
[INFO ] [release/9.0] {
  "owner": "FFmpeg",
  "repo": "FFmpeg",
  "head": "bp-release/9.0-a633df5",
  "headOwner": "ffmpeg-devel",
  "headRepo": "FFmpeg",
  "pushRemoteUrl": "https://code.ffmpeg.org/ffmpeg-devel/FFmpeg.git",
  "base": "release/9.0",
  "title": "[release/9.0] Fix d3d12va bitstream size checking",
  "body": "**Backport:** https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/23742\r\n\r\nD3D12VA fully trusts the return value of ff_d3d12va_get_suitable_max_bitstream_size(), which just returns av_image_get_buffer_size(frames_ctx->sw_format, avctx->coded_width, avctx->coded_height, 1).\r\nNo size checking was done.\r\nSo a file specifically crafted to bust that limit is able to overflow that buffer.\r\nAt least I wasn't able to find any size checking done anywhere, so this is what this series adds.\r\n\r\nThis was originally found by depthfirst, but only the instance the first commit fixes.\r\nWhen I looked into it more, I found the same issue in all other d3d12va decoders, as well as the other instance inside of the AV1 one.",
  "reviewers": [
    "BtbN"
  ],
  "assignees": [],
  "labels": [],
  "comments": []
}
[INFO ] [release/9.0] Pull request created: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/23846
[INFO ] Process succeeded

It's logging "cherry-pick the squashed commit", so it thinks the PR was squash merged, while it in fact was rebase+ff merged.
The original PR https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/23742 has 7 commits, but it then only picks the HEAD commit, and creates a single-commit PR from it.

I have just short-circuited the squash-detection in our local fork of the action for now, since we never squash merge anyway: https://code.ffmpeg.org/actions/git-backporting/commit/627509584cceec97af8d01aaa85a57929006c823

No matter how I think about this, I don't have an immediate idea how to distinguish a squash-merge from a rebase+ff one.
So I'm unsure what the proper fix would look like.

[lampajr]

Thanks for all the info @BtbN , I will check it out!

I have just short-circuited the squash-detection in our local fork of the action for now, since we never squash merge anyway: https://code.ffmpeg.org/actions/git-backporting/commit/627509584cceec97af8d01aaa85a57929006c823

I believe you can achieve the same by passing --no-squash (or the equivalent in the action) which will force squash set to false, not sure if you tried this already

[lampajr]

Ok I might have an idea, but I need to validate it before 😄

In the meantime a couple of things:

• You might not need the workaround, you can theoretically simply pass squash: false in the action and you should achieve the same thing
• You don't need to explicitly hardcode code.ffpmeg.org as url, you can simply set gitClient: codeberg in the action

[BtbN]

Yes, those workarounds are simply put there since merging commits into FFmpeg is a whole multi-day to week process, while I can push to the fork of the action more freely.
Some of the commits in there are thus really just option changes.

I do intend to PR the patch for opening the PRs from a forked repo, since I saw there's an open issue for that already.

[lampajr]

@BtbN I was able to quickly draft a possible fix - looks working but I'd like to get another pair of eyes if you don't mind 👉🏻 #215 - it fix is mainly relying on the commit messages, not 100% sure this is going to cover all possible edge cases though.

I do intend to PR the patch for opening the PRs from a forked repo, since I saw there's an open issue for that already.

BTW that would be great, thank you!