korthout / backport-action

Fast and flexible GitHub action to cherry-pick merged pull requests to selected branches
MIT License
61 stars 26 forks source link

PR commits are cherry-picked instead of the squashed commit #342

Closed Lawls91 closed 10 months ago

Lawls91 commented 1 year ago

Hey just testing this for our repo and I am seeing that squashed PRs cherrypick using the commits on the PR branch, not the final squash commit. I saw this ticket but it has been closed

https://github.com/korthout/backport-action/issues/46

I've not really dug into it yet but is it possible on a squash commit to grab that commit and cherrypick using that?

korthout commented 1 year ago

Hi @Lawls91 👋 Thanks for reporting this.

It is currently not possible to cherry-pick the squashed commit. The action only cherry-picks the commits that exist on the merged pull request.

Previously the action did not work at all for pull requests that were merged using rebase or squash strategies, but I closed #46 when that was fixed and the action could cherry-pick the pull request commits for those cases.

Do you have a specific need to cherry-pick the squashed commit instead of the commits on the pull request?

My personal thoughts on this have always been that this way the reviewer of the backport pull request can review it in the same way that the original pull request was reviewed. I.e., it has all the individual commits and their messages.

Lawls91 commented 1 year ago

I must admit its been a week so I cant fully remember, but I feel like I was having issues with the backport picking up merge commits where main/master had been merged into the PR, but they shouldn't be part of the backport

I'll try and revisit this this week if I can and take another look

korthout commented 1 year ago

Thanks @Lawls91! That's a great point!

There's another feature request that will help resolve that problem, which I plan to implement once I've completed #340:

Still, even with #341 implemented, there would be cases left where cherry-picking the resulting commit(s) (i.e. the squashed commit or the rebased commits) are preferable. So, eventually I hope to support this as well.

jrhemstad commented 11 months ago

I just ran into this as I started using this action. The problem if you squash PRs into main and then backport only the cherry-picked commits to another branch is that the commits will no longer be the same, i.e., the release branch will have different commits from the same changes.

I don't see a great way around this problem though because if you just simply backported the squashed commit, then you run the risk of picking up merge commits from main and end up merging changes you didn't intend to into your release branch...

korthout commented 11 months ago

@jrhemstad Yes, the action only cherry-picks the commits that exist on the merged pull request, at this time.

you run the risk of picking up merge commits from main and end up merging changes you didn't intend to into your release branch

This would not be a risk if this action would support cherry-picking the squashed commit instead of the commits that exist on the merged pull request. However, it does not support this yet.

It seems you have a specific need for cherry-picking only the squashed commit. Can you elaborate on why you need this specifically? It may help me prioritize this over other issues.

jrhemstad commented 11 months ago

Can you elaborate on why you need this specifically?

For example, we recently had a PR where the backport action failed when trying to cherry-pick the individual commits: https://github.com/NVIDIA/cccl/pull/574#issuecomment-1793117712

(fyi, the instructions it prints for manually cherry-picking don't include the logic for skipping merge commits)

Trying to manually cherry-pick all of the PR commits was a bit of a pain. In contrast, when we just manually cherry-picked the squashed commit, it applied cleanly without any conflicts or issues: https://github.com/NVIDIA/cccl/pull/663. It would be nice if the action had a way to automate this for us as we never care about preserving the individual commits in the PR we're backporting.

korthout commented 11 months ago

Ah yes, reduced chance of conflicts is definitely a benefit of cherry-picking a squashed commit. I see this feature (cherry-picking the resulting commits rather than the pull request's commits) as useful, but there are a few issues I'll prioritize over this at this time, like:

korthout commented 10 months ago

@jrhemstad @Lawls91 v2.2.0 is out and adds experimental support to detect_merge_method. I believe it resolves this issue for you. Please give it a try and let me know what you think.