python / cherry-picker

๐Ÿ๐Ÿ’โ› Utility script for backporting/cherry-picking CPython changes from master into one of the maintenance branches.
Apache License 2.0
46 stars 38 forks source link

Backport branch deleted even if push fails #78

Open CAM-Gerlach opened 1 year ago

CAM-Gerlach commented 1 year ago

As @warsaw discovered the hard way on python/cpython#101024 , if pushing the branch to the selected (or default) remote fails for any reason (e.g. attempting to push to upstream instead of origin, as happened here), the branch is deleted anyway instead instead of issuing a clear error message and existing with it intact. This results in potentially loosing a large amount of hard, tedious work manually resolving backport conflicts (unless the user is a Git expert who knows how to recover it via git reflog), and is a very frustrating and unfriendly user experience.

Full error output from Barry ```console % cherry_picker --continue git switch๐Ÿ ๐Ÿ’ โ› Failed to push to origin โ˜น remote: error: GH006: Protected branch update failed for refs/heads/backport-49cae39-3.10. remote: error: You're not authorized to push to this branch. Visit https://docs.github.com/articles/about-protected-branches/ for more information. To github.com:python/cpython.git ! [remote rejected] backport-49cae39-3.10 -> backport-49cae39-3.10 (protected branch hook declined) error: failed to push some refs to 'github.com:python/cpython.git' branch backport-49cae39-3.10 has been deleted. Backport PR: [3.10] gh-101021: Document binary parameters as bytes (GH-101024). (cherry picked from commit 49cae39ef020eaf242607bb2d2d193760b9855a6) Co-authored-by: Bob Kline ```

If pushing fails, the PUSHING_TO_REMOTE_FAILED state is set https://github.com/python/cherry-picker/blob/main/cherry_picker/cherry_picker.py#L407, but then push_to_remote just returns and the branch is deleted regardless of the state https://github.com/python/cherry-picker/blob/main/cherry_picker/cherry_picker.py#L521 . As far as I can tell, setting the PUSHING_TO_REMOTE_FAILED state has no effect and cherry-picker just continues and terminates normally regardless.

I'm not sure the best way to fix this within Cherry_Picker's error handling and UX design, but the most obvious solution is to just have it raise e.g. RuntimeError and exit instead. There may be other situations where this happens as well, so it might be worth investigating any other known failure codepaths further.

As a sidenote I also discovered after much trial and error that you need to pass --no-auto-pr and --pr-remote upstream every time you call cherry picker --continue to get it to work, instead of it being stored in .gitconfig. This is very unintiuitve, and could also potentially lead to this error as well.

Also, calling --dry-run --continue in the middle of a cherry pick to see what it would do next completely borks things, and requires wiping the config and starting over to recover.