Add conflict_resolution input

[vermz99]

Summary

The goal of this PR is to allow creating draft PR for backport that resulted in merge conflict.

Similar to #386

Consideration

• New input was added in experimental since side effect are not fully well-know. I did some testing and it seems to work great, but I might have overlooked something.
• cherry-pick is stopped at first commit conflict. The commit in conflict is not committed, but all the previous ones are.
• PR is created as Draft to ensure the user is aware that it is an incomplete PR
• Suggestion resolution message is added both in original PR and new PR to ensure the user take action and do not merge unresolved PR.
• Might need to add label to emphasize even more that PR need to solve some conflict?

Dev Notes

• Did some refactoring in the messages to try to make this as centralized as possible. I am open to suggestion here if this does not align with maintainers vision.

TODO

• add to ReadMe
• add unit test (open to suggestion here)
• Determine if backport_on_conflicts remains in experimental
• Determine if label is needed
• Add commit conflict logic

[vermz99]

Issue identified

I overlooked that GH does not allow to create PRs for 2 identical branches.

Important

This PR needs rework

I probably will commit the conflict like initially suggested in #386, but i'll provide instruction in the suggestion to reset last commit (i.e., git reset --hard HEAD^) and restart cherry-pick from the conflict followed by git push --force.

[korthout]

Thanks for the amazing contribution @vermz99. It looks very promising! I'll provide some more detailed comments in the coming week, but wanted to already share some early thoughts.

I probably will commit the conflict like initially suggested in #386, but i'll provide instruction in the suggestion to reset last commit (i.e., git reset --hard HEAD^) and restart cherry-pick from the conflict followed by git push --force.

🤔 This is worsening the DX for the cases where some of the commits could be cherry-picked without conflicts. Or it introduces a special case where users need to interact differently with the created draft PR in relation to those cases I mentioned. Different interactions may be hard to get used to when using backport-action in a repo actively like our team does.

My current thought is that we should go with your first idea instead. We can make the feature flag a bit more flexible, for example by calling it conflict_resolution taking one of a few values fail (fails the backport) or partial_draft (your first idea with partial backport of as much it can do). Then, we can later expand this with other modes like partial_draft_allowing_empty (special case dealing with the first commit leading to conflicts) or draft_commit_conflicts (with the behavior described in #386).

With that in mind, we can keep the initial version simple. It will improve the situation for pull requests with multiple commits. What do you think?

---

Regarding labeling, I think an output may be more flexible. It would allow anyone to add labeling afterward, or do anything else they want.

[vermz99]

Thank you for the initial feedback!

🤔 This is worsening the DX for the cases where some of the commits could be cherry-picked without conflicts. Or it introduces a special case where users need to interact differently with the created draft PR in relation to those cases I mentioned. Different interactions may be hard to get used to when using backport-action in a repo actively like our team does.

Regarding this, just to clarify my idea, I was thinking of always giving the git reset --hard HEAD^ set of instructions when a conflict is encountered. This will rollback only the conflict commit, not all the previous ones correctly cherry-picked. So it would always look like that, in cases where conflicts occur:

1. cherry-pick all up until, and including, the first conflict encountered
2. Create draft PR
3. Provide the following set of instructions on both PR (original and new one)

git fetch origin
git worktree add
cd .worktree
git switch
git reset --hard HEAD^ #To rollback only the first commit, which should always have a conflict
git cherry-pick sha1 sha2 sha3 ... shaN
git push --force

I can see how it might add another level of complexity if we handle all the cases differently (i.e., first commit conflict has a different behavior than conflict on subsequent commits), that's why I had in mind to always have the above behavior, regardless of the position of the conflict.

For now while we think a bit about it and to give other contributors a chance to see this, I'll rework the code to allow different flags for different behavior.

---

Note

If we indeed decide to have a partial_draft_allowing_empty option, I made some tests and we could amend nothing to HEAD to create a new commit, but that would for sure dirty the history a bit and may be problematic for some people. Something to think about...

[korthout]

So, if only a single commit abcdef12 is being backported and it conflicts, then the instructions have the same structure as a backport of many commits where any of them conflicts.

git fetch origin
git worktree add
cd .worktree
git switch
git reset --hard HEAD^ #To rollback only the first commit, which should always have a conflict
git cherry-pick abcdef12
git push --force

So, the DX differs for backports with conflicts, but it already does anyway (PR is draft, manual effort is required). I like it 👍

[AlexVermette-Eaton]

@korthout Ready for review.

What I did

• Added the conflict_resolution flag, as suggested. I only implemented fail and draft_commit_conflicts for now. We can discuss if other cases are needed for first version of this feature. As this is a significant changes, I don't want to introduce too many things that can lead to bugs.
• Added unit tests. I tried to cover as much as possible the new feature. Tell me if you need more.
• Refactored a bit everything around suggestions. I think I might need Improve manual instructions #305 relatively soon so I tried to keep that in mind when I designed this part.

What I did not do

• I did not add an output as I don't really know how it should be represented. I'm open to suggestion if we want this for the first version.
• I did not implement partial_draft and partial_draft_allowing_empty.
• Test all possible scenarios. This is pretty hard and time consuming to test so if you want to help me test it on your side, that would be awesome. I'm also open to deploy it in prod on my side (20+ devs) to test it before merging, if we think it's necessary.

[korthout]

Thanks @AlexVermette-Eaton 🙇 My apology for not having found the time to review it.

Please note that I'm currently moving internationally, and I'll review it as soon as I can, but I can't make promises now. If you need this functionality immediately, I suggest your team temporarily switches to the version in your fork until we merge this.

I hope to be back at full capacity in a few weeks, and who knows, I may still find a moment to sit down for this soon enough. Sorry for the inconvenience.

[korthout]

Apology for the long wait @AlexVermette-Eaton @vermz99, but I'm fully back again now! After my move, I first reviewed and merged #405. And today, I found time to review this great addition!

🙌 First off, thanks for the changes! I really like how this has evolved.

🙇 Sadly, some conflicts were introduced by merging #405. Let me know if you need some help to resolve these.

💭 As your changes refer to origin in some places, it will not function correctly together with the new downstream_repo feature (from #405). That's totally fine for now. We can improve that in a separate PR later, as both these features are experimental. Let's ship first, and then improve.

🔧 I also have a few suggestions, but nothing about the functionality itself. Please have a look.

👍 I'm happy how you've managed to separate the experimental parts from the existing code, this makes it easy for me to approve and merge.

👍 Regarding tests, I'm happy to see that you've added some unit tests. That's really enough for me. We can consider adding an E2E case in the future. As this is experimental, I'd rather ship it in an early form than to have comprehensive tests in place. The same goes for outputs, labeling, other input options, etc.

So, please have a look at my suggestions and resolve the conflicts. Then we should be good to ship it

[korthout]

Is a conflict the only reason for 1 as exit code? If not, we could consider matching the stdout or stderr to be sure.

[AlexVermette-Eaton]

Based on what I read, a conflict should always return 1, but the other way around is not true. 1 might now always indicate a proper conflict. For example, I believe an "empty" cherry-pick will also return 1. I assumed here that 1 would equal a conflict since it's very hard to be sure that a proper conflict occured that would make sense with our resolution suggestion. There are so many different ways that a conflict can occur based on git unit test: https://github.com/git/git/blob/master/t/t3507-cherry-pick-conflict.sh. I am open to suggestion, I just don't know in what way I should parse stdout. Also there might be a solution using the output of git status, but i'm not too familiar with this.

[korthout]

When there is a conflict, the stdout will contain hint: After resolving the conflicts, mark them with, which we could use as an indication of a conflict. It's not so pretty, but it should work and it's unlikely that this will change anytime soon.

However, I'm also okay with just using exitCode === 1 for now. It's just as unlikely that the exit code changes, and it doesn't need to be perfect at this time.

[vermz99]

Thanks a lot for the review! 🚀

New Changes

• I merged the new feature from main. I added some things to try and support it, but I think it still needs some work to adjust correctly the messages printed in the original PR and draft PR.
• I added your suggestion for the while loop
• I added the read me
• I fixed the default value in the .yml file

Let me know if you want anything else added! Also, I would appreciate it if you could try it on your side and see if it behaves as it should 🙏.

Also, apologies for the 2 accounts shuffling, work forces us to use a different account.

[korthout]

That's great! Thanks for the fast changes! @vermz99 @AlexVermette-Eaton 🚀

✅ I ran a successful test here (backporting to a branch called conflict): korthout/backport-action-test#371

• It first ran with defaults, producing this comment
• Then it ran with "conflict_resolution": "draft_commit_conflicts", resulting in this comment, and this pr

❌ I noticed the following that we should change before merging:

• We should suggest the safer git push --force-with-lease rather than git push --force
• The commit with the conflicts has double quotes surrounding its commit message

🙇 Please adjust this still, and then I'll merge this soon. Or if I find some time during the weekend, I might just pick up these last two items. Anyways, thanks for all the hard work. This looks great!

Also, apologies for the 2 accounts shuffling, work forces us to use a different account.

No worries, I'll just tag both :)

[AlexVermette-Eaton]

Thanks for the review and thank you very much for the testing! 🔥🚀

New Changes

• Replace --force with --force-with-lease
• Fixed "BACKPORT-CONFLICT" to BACKPORT-CONFLICT

Again don't hesitate if you need additional changes.

[korthout]

🚀 I've tested the feature again, and it works well!

💭 I hope we get feedback from users about it.

💭 I feel improvements remain in the suggestions code, but this will change with #415 anyway. There is no need to spend more time on it now.

LGTM 👍