Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Lawls91 opened this issue May 17, 2023 · 8 comments · Fixed by #399
Closed

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

Lawls91 opened this issue May 17, 2023 · 8 comments · Fixed by #399

Comments

@Lawls91
Copy link

Lawls91 commented May 17, 2023

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

#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
Copy link
Owner

korthout commented May 17, 2023

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
Copy link
Author

Lawls91 commented May 22, 2023

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
Copy link
Owner

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
Copy link

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
Copy link
Owner

korthout commented Nov 4, 2023

@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
Copy link

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: NVIDIA/cccl#574 (comment)

(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: NVIDIA/cccl#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
Copy link
Owner

korthout commented Nov 7, 2023

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 korthout linked a pull request Nov 26, 2023 that will close this issue
@korthout
Copy link
Owner

korthout commented Dec 4, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants