Re: [PATCH] Fix revert --abort on Windows
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Nov 23, 2011, 10:04 UTC
- Message-ID
- <20111123100452.GA30629@elie.hsd1.il.comcast.net>
- In-Reply-To
- <4ECCB3A2.5030102@viscovery.net>
Johannes Sixt wrote:
Show 6 quoted lines
> From: Johannes Sixt <j6t@kdbg.org> > > On Windows, it is not possible to rename or remove a directory that has > open files. 'revert --abort' renamed .git/sequencer when it still had > .git/sequencer/head open. Close the file as early as possible to allow > the rename operation on Windows.
Nice catch.
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Speaking of which, it doesn't make a lot of sense for "git revert --abort" to be leaving a .git/sequencer-old directory around. How about this on top?
-- >8 -- Subject: revert --abort: do not leave behind useless sequencer-old directory
The "git cherry-pick --abort" command currently renames the .git/sequencer directory to .git/sequencer-old instead of removing it on success due to an accident. cherry-pick --abort is designed to work in three steps:
1) find which commit to roll back to 2) call "git reset --merge <commit>" to move to that commit 3) remove the .git/sequencer directory
But the careless author forgot step 3 entirely. The only reason the command worked anyway is that "git reset --merge <commit>" renames the .git/sequencer directory as a secondary effect --- after moving to <commit>, or so the logic goes, it is unlikely but possible that the caller of git reset wants to continue the series of cherry-picks that was in progress, so git renames the sequencer state to .git/sequencer-old to be helpful while allowing the cherry-pick to be resumed if the caller did not want to end the sequence after all.
By running "git cherry-pick --abort", the operator has clearly indicated that she is not planning to continue cherry-picking. Remove the (renamed) .git/sequencer directory as intended all along.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- By the way, as the length of the second-to-last paragraph above might have hinted, I am not convinced that allowing "git reset --hard" as an escape route from a cherry-pick sequence was very sensible. It _would_ be nice to have a command to return to a known state, discarding progress in all pending multiple-command guided workflows (am, rebase, bisect), but git reset is not that command.
builtin/revert.c | 1 + t/t7106-reset-sequence.sh | 8 ++++++++ 2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c index 5dedb51c..818b4abb 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -942,6 +942,7 @@ static int sequencer_rollback(struct replay_opts *opts) } if (reset_for_rollback(sha1)) goto fail; + remove_sequencer_state(1); strbuf_release(&buf); return 0; fail: diff --git a/t/t7106-reset-sequence.sh b/t/t7106-reset-sequence.sh index 3f86e8c5..83f7ea59 100755 --- a/t/t7106-reset-sequence.sh +++ b/t/t7106-reset-sequence.sh @@ -41,4 +41,12 @@ test_expect_success 'reset --hard cleans up sequencer state, providing one-level test_path_is_missing .git/sequencer-old ' +test_expect_success 'cherry-pick --abort does not leave sequencer-old dir' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + git cherry-pick --abort && + test_path_is_missing .git/sequencer && + test_path_is_missing .git/sequencer-old +' + test_done
-- 1.7.8.rc3