Ensure that cherry-picker doesn't exit with bad state on branch cleanup

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

Ensure that cherry-picker doesn't exit with bad state on branch cleanup #61

Closed Jackenmen closed 2 years ago

Jackenmen commented 2 years ago

Fixes #17 and fixes #6

Does 2 things:

tries to checkout the previously checked out branch rather than the default branch (if it fails, it tries the default branch)
fixes cherry-picker exiting with a bad state when it fails to checkout a branch during cleanup
- this is done because regardless of what branch we choose to check out to, there's a chance that it won't be able to be checked out (either due to being checked in a different worktree or because it no longer exists)
- judging by the existing code in cleanup_branch() method, I assume that this was already the intention anyway

Jackenmen commented 2 years ago

There is also a problem that git worktrees share the git config (without the use of special configuration) so trying to cherry-pick in a worktree while there's a paused cherry-pick in another worktree is bound to fail. I consider it out of scope for this PR though and regardless, I'm not really sure what would be a good solution for that problem. If someone runs into that, they should probably just make a separate issue as #17 was reporting a different issue and it's somewhat of a coincidence that we also found the issue with shared git config.

Jackenmen commented 2 years ago

While I was fixing conflicts for #39, I noticed this has caused a small regression in the error message (checked_out_branch variable should have been used instead of branch_name).

I fixed this in #39 (https://github.com/python/cherry-picker/pull/39/commits/dc9bb668785c148b2061e62045165b758d9cf13d) since it caused tests to fail but I can split it if necessary. Note that #39 also adds a test that should ensure this doesn't happen in the future so it would save me some time from writing a dedicated regression test if we keep the fix in #39.