Add support for dirty worktrees #2
Conversation
| @@ -50,7 +53,6 @@ cargo install git-quickfix | |||
|
|
|||
| ## Known Issues | |||
|
|
|||
| - A dirty index or modified working directory will likely not work. | |||
There was a problem hiding this comment.
Yes, the line is removed because the issues should be fixed by this PR.
| // Create the new branch | ||
| let new_branch = repo | ||
| .branch(&opts.branch, &main_commit, opts.force) | ||
| .suggestion("Consider using --force to overwrite the existing branch")?; |
There was a problem hiding this comment.
I'd prefer to create the branch after the cherry picking. In case the cherry pick fails.
I believe that this might be a misunderstanding -- the previous version should not have ever modified the master branch.
There was a problem hiding this comment.
I'll look into it again. What I had seen was that master had been modified after running the command, but now I'm not sure anymore.
| let fix_commit = repo.head()?.peel_to_commit()?; | ||
| if fix_commit.parent_count() != 1 { | ||
| return Err(eyre!("Only works with non-merge commits")) | ||
| .suggestion("Quickfixing a merge commit is not supported. If you meant to do this please file a ticket with your usecase."); |
There was a problem hiding this comment.
Remind me: I should point to this repo for filling tickets.
| cherrypick_commit_onto_new_branch(&repo, &opts)?; | ||
|
|
||
| if !opts.keep { | ||
| remove_commit(&mut repo, &opts, is_dirty)? |
There was a problem hiding this comment.
Maybe I misunderstand this, but shouldn't it be if !opts.keep && is_dirty ? I see that it would potentially stash if is_dirty is true and --stash has been provided. That seems to be mixing responsibilities a bit.
There was a problem hiding this comment.
The logic now says that if the user does not specify --keep, the command should always try to remove the commit.
Removing the commit will raise an error if the working directory is dirty and --stash is not given.
I think the latest commit can be removed if either
- the working directory is clean, aka
!is_dirty, or is_dirtyandopts.stashis activated.
Changing the line to
if !opts.keep && is_dirty {would cause the 1. case to fail, right?
Would you prefer to inline the remove_commit function here to make the "stashing" more visible?
|
Please merge it, had lost my unstaged changes 😢 🙏 |
|
Sorry, I got sidetracked. Will respond the the review comments now |
|
Dear @dvogt23 , I am extremely sorry that this bug caused you the loss of work. I will try very hard that this does not happen again, and hope that quickfix will be valuable in the future. Please let me know if you have any ideas for improvement. Kind regards, |
Hey @siedentop,
thank you for this nice git extension!
I noticed that local changes in the repository are
resetif the--keepflag is not specified, asperforms a hard reset on the HEAD.
As this could lead to lost work, I propose a few tweaks / features:
--autostashflag is given, in which case a temporary stash is created like withrebase.autoStash.mainbranch unmodified(3. is actually independent of the other changes, but greatly helps with repeatedly running the command.)
I have also split the command into several functions, because the
stash_...functions required a&mut Repositoryand this way explicitdrop(...)invocations can be avoided.I'm happy to continue working on this, If you'd like to keep any of these features.
Cheers