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

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

Merged
merged 2 commits into from
Apr 15, 2022

Conversation

Jackenmen
Copy link
Contributor

@Jackenmen Jackenmen commented Mar 18, 2022

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 Jackenmen force-pushed the issue/17 branch 3 times, most recently from ff6ceb9 to 8e9a91f Compare March 18, 2022 14:44
@Jackenmen Jackenmen changed the title Check out previous branch rather than default branch on cleanup Ensure that cherry-picker doesn't exit with bad state on branch cleanup Mar 18, 2022
@Jackenmen Jackenmen marked this pull request as ready for review March 18, 2022 15:17
Copy link
Contributor

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing the patch using the mobile app, I may be missing something but in general the idea seems sound. I don't see any immediate problems with the implementation but it's a good idea to have an extra set of eyes on this.

This whole git-worktree use case makes me think that further improvements of the cherry-picker could make use of temporary detached worktrees for the backporting process. Maybe even check out commits by hashes in detached HEAD mode in those worktrees so that the main dev env would remain unchanged.

@Jackenmen
Copy link
Contributor Author

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.

@ambv ambv merged commit 852ffec into python:main Apr 15, 2022
@Jackenmen Jackenmen deleted the issue/17 branch April 15, 2022 13:45
Jackenmen added a commit to Jackenmen/cherry-picker that referenced this pull request Apr 15, 2022
@Jackenmen
Copy link
Contributor Author

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 (dc9bb66) 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.

Mariatta pushed a commit that referenced this pull request Oct 3, 2022
* Fix exit with bad state when backport branch already exists

* Add test

* Fix branch name used in checkout error

This was missed in #61

* Apply suggestions from code review

Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>

Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cherry_picker fails on branch cleanup in non-main worktree Cherry_picker should checkout original branch.
4 participants