From 7372eaeb8b47ba96bad08777cd1330561ada8592 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Tue, 14 May 2019 19:03:46 +0100 Subject: rebase: fix a memory leak buf was never freed. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- builtin/rebase.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/rebase.c b/builtin/rebase.c index cd233c6118..a9062c672b 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -2165,6 +2165,7 @@ run_rebase: ret = !!run_specific_rebase(&options, action); cleanup: + strbuf_release(&buf); strbuf_release(&revisions); free(options.head_name); free(options.gpg_sign_opt); -- cgit v1.2.3 From d3fce47d2df868287de1a15d769e78db2c3283a9 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Tue, 14 May 2019 19:03:47 +0100 Subject: rebase: warn if state directory cannot be removed If rebase --quit cannot remove the state directory then it dies. However when rebase finishes normally or the user runs rebase --abort any errors that occur when removing the state directory are ignored. That is fixed by this commit. All of the callers of finish_rebase() except the code that handles --abort are careful to make sure they get a postive return value, do the same for --abort. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- builtin/rebase.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index a9062c672b..b5d2e2311c 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -760,6 +760,7 @@ static int finish_rebase(struct rebase_options *opts) { struct strbuf dir = STRBUF_INIT; const char *argv_gc_auto[] = { "gc", "--auto", NULL }; + int ret = 0; delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF); apply_autostash(opts); @@ -770,10 +771,11 @@ static int finish_rebase(struct rebase_options *opts) */ run_command_v_opt(argv_gc_auto, RUN_GIT_CMD); strbuf_addstr(&dir, opts->state_dir); - remove_dir_recursively(&dir, 0); + if (remove_dir_recursively(&dir, 0)) + ret = error(_("could not remove '%s'"), opts->state_dir); strbuf_release(&dir); - return 0; + return ret; } static struct commit *peel_committish(const char *name) @@ -1645,7 +1647,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) die(_("could not move back to %s"), oid_to_hex(&options.orig_head)); remove_branch_state(the_repository); - ret = finish_rebase(&options); + ret = !!finish_rebase(&options); goto cleanup; } case ACTION_QUIT: { -- cgit v1.2.3 From 37e9ee5cb90db0831d5d58bed82149ba67917d73 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Tue, 14 May 2019 19:03:48 +0100 Subject: sequencer: return errors from sequencer_remove_state() If there is an error when removing the state directory then we should report it. This matches what the non-interactive rebase does. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 610b7ece14..258e583156 100644 --- a/sequencer.c +++ b/sequencer.c @@ -274,7 +274,7 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts) int sequencer_remove_state(struct replay_opts *opts) { struct strbuf buf = STRBUF_INIT; - int i; + int i, ret = 0; if (is_rebase_i(opts) && strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) { @@ -283,8 +283,10 @@ int sequencer_remove_state(struct replay_opts *opts) char *eol = strchr(p, '\n'); if (eol) *eol = '\0'; - if (delete_ref("(rebase -i) cleanup", p, NULL, 0) < 0) + if (delete_ref("(rebase -i) cleanup", p, NULL, 0) < 0) { warning(_("could not delete '%s'"), p); + ret = -1; + } if (!eol) break; p = eol + 1; @@ -300,10 +302,11 @@ int sequencer_remove_state(struct replay_opts *opts) strbuf_reset(&buf); strbuf_addstr(&buf, get_dir(opts)); - remove_dir_recursively(&buf, 0); + if (remove_dir_recursively(&buf, 0)) + ret = error(_("could not remove '%s'"), buf.buf); strbuf_release(&buf); - return 0; + return ret; } static const char *action_name(const struct replay_opts *opts) -- cgit v1.2.3 From d559f502c59658ea1f360fd28d8909894e40a4fe Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Tue, 14 May 2019 19:03:49 +0100 Subject: rebase --abort/--quit: cleanup refs/rewritten When `rebase -r` finishes it removes any refs under refs/rewritten that it has created. However if the user aborts or quits the rebase refs are not removed. This can cause problems for future rebases. For example I recently wanted to merge a updated version of a topic branch into an integration branch so ran `rebase -ir` and removed the picks and label for the topic branch from the todo list so that merge -C topic would pick up the new version of topic. Unfortunately refs/rewritten/topic already existed from a previous rebase that had been aborted so the rebase just used the old topic, not the new one. The logic for the non-interactive quit case is changed to ensure `buf` is always freed. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- builtin/rebase.c | 34 +++++++++++++++++++++++++--------- t/t3430-rebase-merges.sh | 18 +++++++++++++++++- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index b5d2e2311c..bb347203ba 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -770,10 +770,18 @@ static int finish_rebase(struct rebase_options *opts) * user should see them. */ run_command_v_opt(argv_gc_auto, RUN_GIT_CMD); - strbuf_addstr(&dir, opts->state_dir); - if (remove_dir_recursively(&dir, 0)) - ret = error(_("could not remove '%s'"), opts->state_dir); - strbuf_release(&dir); + if (opts->type == REBASE_INTERACTIVE) { + struct replay_opts replay = REPLAY_OPTS_INIT; + + replay.action = REPLAY_INTERACTIVE_REBASE; + ret = sequencer_remove_state(&replay); + } else { + strbuf_addstr(&dir, opts->state_dir); + if (remove_dir_recursively(&dir, 0)) + ret = error(_("could not remove '%s'"), + opts->state_dir); + strbuf_release(&dir); + } return ret; } @@ -1651,11 +1659,19 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) goto cleanup; } case ACTION_QUIT: { - strbuf_reset(&buf); - strbuf_addstr(&buf, options.state_dir); - ret = !!remove_dir_recursively(&buf, 0); - if (ret) - die(_("could not remove '%s'"), options.state_dir); + if (options.type == REBASE_INTERACTIVE) { + struct replay_opts replay = REPLAY_OPTS_INIT; + + replay.action = REPLAY_INTERACTIVE_REBASE; + ret = !!sequencer_remove_state(&replay); + } else { + strbuf_reset(&buf); + strbuf_addstr(&buf, options.state_dir); + ret = !!remove_dir_recursively(&buf, 0); + if (ret) + error(_("could not remove '%s'"), + options.state_dir); + } goto cleanup; } case ACTION_EDIT_TODO: diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 4c69255ee6..e63576d334 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -224,8 +224,24 @@ test_expect_success 'refs/rewritten/* is worktree-local' ' test_cmp_rev HEAD "$(cat wt/b)" ' +test_expect_success '--abort cleans up refs/rewritten' ' + git checkout -b abort-cleans-refs-rewritten H && + GIT_SEQUENCE_EDITOR="echo break >>" git rebase -ir @^ && + git rev-parse --verify refs/rewritten/onto && + git rebase --abort && + test_must_fail git rev-parse --verify refs/rewritten/onto +' + +test_expect_success '--quit cleans up refs/rewritten' ' + git checkout -b quit-cleans-refs-rewritten H && + GIT_SEQUENCE_EDITOR="echo break >>" git rebase -ir @^ && + git rev-parse --verify refs/rewritten/onto && + git rebase --quit && + test_must_fail git rev-parse --verify refs/rewritten/onto +' + test_expect_success 'post-rewrite hook and fixups work for merges' ' - git checkout -b post-rewrite && + git checkout -b post-rewrite H && test_commit same1 && git reset --hard HEAD^ && test_commit same2 && -- cgit v1.2.3