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

Conflict resolve suggestion doesn't fetch commits to cherry-pick #424

Open GuyAv46 opened 4 months ago

GuyAv46 commented 4 months ago

Describe the bug

When backporting a PR that was merged with the "squash and merge" method, and the backporting fails (due to conflicts), the suggestion message doesn't work, because it does not fetch the original PR target branch, and therefore the squashed commit sha is unknown locally.

To Reproduce

  1. Add a label to backport a PR to a branch that will cause conflicts
  2. Squash and merge the PR
  3. Copy the suggestion to the local work env
  4. See that it fails to cherry-pick

Expected behavior If the "squash and merge" method was detected, and a failure occurred, include the original PR's base branch or the squashed commit sha at the beginning of the suggestion:

      git fetch origin ${target} ${original target branch}
      git worktree add -d .worktree/${branchname} origin/${target}
      cd .worktree/${branchname}
      git switch --create ${branchname}
      git cherry-pick -x ${commitShasToCherryPick.join(" ")}

or

      git fetch origin ${target} ${squashed commit sha}
      git worktree add -d .worktree/${branchname} origin/${target}
      cd .worktree/${branchname}
      git switch --create ${branchname}
      git cherry-pick -x ${commitShasToCherryPick.join(" ")}
AlexVermette-Eaton commented 4 months ago

@GuyAv46 Can you provide the version you are using for this workflow?

GuyAv46 commented 4 months ago

I used the new v3 tag. From the run logs

Download action repository 'korthout/backport-action@v3' 
(SHA:bd410d37cdcae80be6d969823ff5a225fe5c833f)

after more thinking this problem might happen when the action detects a “rebase and merge” method. Maybe it is useful to fetch the head branch in this case? Or all the about-to-be-cherry-picked commits?

GuyAv46 commented 4 months ago

Why not just call git fetch with no specific branch for all the cases?

korthout commented 4 months ago

Thanks for reporting @GuyAv46

You're right that the suggestion currently doesn't fetch all the required references. The commits to cherry-pick may not be known locally.

Reading the suggestions again, I believe this issue appears for all merge methods (including merge commit), regardless of the new experimental conflict_resolution setting.

By default:

https://github.com/korthout/backport-action/blob/bd410d37cdcae80be6d969823ff5a225fe5c833f/src/backport.ts#L628-L634

With conflict_resolution: draft_commit_conflicts:

https://github.com/korthout/backport-action/blob/bd410d37cdcae80be6d969823ff5a225fe5c833f/src/backport.ts#L616-L623

In both cases, the commits to cherry-pick may be unreachable after the suggested git fetch.

Example:

We must add an additional fetch based on the merge method to resolve this. The action already does this, so we should also suggest to do this locally: On squash: https://github.com/korthout/backport-action/blob/bd410d37cdcae80be6d969823ff5a225fe5c833f/src/backport.ts#L147-L155 On rebase: https://github.com/korthout/backport-action/blob/bd410d37cdcae80be6d969823ff5a225fe5c833f/src/backport.ts#L158-L166 Otherwise: https://github.com/korthout/backport-action/blob/bd410d37cdcae80be6d969823ff5a225fe5c833f/src/backport.ts#L132-L136

korthout commented 4 months ago

As a quick fix, we can add a git fetch without any references (it performs less). We must keep the git fetch target for the default behavior because the branch of the merged pull request may have already been deleted and not passed along with a regular git fetch. My feeling is that even this solution doesn't cover all cases correctly. For example, what if this concerns a pull request from a fork, or if the remotes are named differently locally.

Eventually, I think we'll need to switch to explicit github refs and commit shas in the fetch.

Otherwise, we could simply add a note about this bug to the suggestion. I'm also open to other ideas.

@GuyAv46 @AlexVermette-Eaton, I'd welcome new contributions to resolve this 🙇. Let me know if you're interested in giving it a try. Otherwise, I'll pick this up eventually, but I don't have time at the moment. While annoying, I don't see this as a critical bug (using git fetch will in most cases resolve the problem as a workaround).