From ccd71b2f38f86a28db2cce811f414fe5369eaa1d Mon Sep 17 00:00:00 2001 From: Stephan Beyer Date: Wed, 7 Dec 2016 22:51:29 +0100 Subject: [PATCH 1/6] am: fix filename in safe_to_abort() error message Signed-off-by: Stephan Beyer Signed-off-by: Junio C Hamano --- builtin/am.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/am.c b/builtin/am.c index 6981f42ce9..7cf40e6f27 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state) if (read_state_file(&sb, state, "abort-safety", 1) > 0) { if (get_oid_hex(sb.buf, &abort_safety)) - die(_("could not parse %s"), am_path(state, "abort_safety")); + die(_("could not parse %s"), am_path(state, "abort-safety")); } else oidclr(&abort_safety); From 1868331f136cfda63ec3296dad66b48f7e9ffe00 Mon Sep 17 00:00:00 2001 From: Stephan Beyer Date: Wed, 7 Dec 2016 22:51:30 +0100 Subject: [PATCH 2/6] am: change safe_to_abort()'s not rewinding error into a warning The error message tells the user that something went terribly wrong and the --abort could not be performed. But the --abort is performed, only without rewinding. By simply changing the error into a warning, we indicate the user that she must not try something like "git am --abort --force", instead she just has to check the HEAD. Signed-off-by: Stephan Beyer Signed-off-by: Junio C Hamano --- builtin/am.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/am.c b/builtin/am.c index 7cf40e6f27..826f18ba12 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2134,7 +2134,7 @@ static int safe_to_abort(const struct am_state *state) if (!oidcmp(&head, &abort_safety)) return 1; - error(_("You seem to have moved HEAD since the last 'am' failure.\n" + warning(_("You seem to have moved HEAD since the last 'am' failure.\n" "Not rewinding to ORIG_HEAD")); return 0; From aeebd98ebe0fbd61f517d9da04f914c37ea0066b Mon Sep 17 00:00:00 2001 From: Stephan Beyer Date: Wed, 7 Dec 2016 22:51:31 +0100 Subject: [PATCH 3/6] t3510: test that cherry-pick --abort does not unsafely change HEAD Signed-off-by: Stephan Beyer Signed-off-by: Junio C Hamano --- t/t3510-cherry-pick-sequence.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 7b7a89dbd5..efcd4fc485 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -147,6 +147,16 @@ test_expect_success '--abort to cancel single cherry-pick' ' git diff-index --exit-code HEAD ' +test_expect_failure '--abort does not unsafely change HEAD' ' + pristine_detach initial && + test_must_fail git cherry-pick picked anotherpick && + git reset --hard base && + test_must_fail git cherry-pick picked anotherpick && + git cherry-pick --abort 2>actual && + test_i18ngrep "You seem to have moved HEAD" actual && + test_cmp_rev base HEAD +' + test_expect_success 'cherry-pick --abort to cancel multiple revert' ' pristine_detach anotherpick && test_expect_code 1 git revert base..picked && From 1e41229d962b43208e6bf79e729b400c31697cc9 Mon Sep 17 00:00:00 2001 From: Stephan Beyer Date: Wed, 7 Dec 2016 22:51:32 +0100 Subject: [PATCH 4/6] sequencer: make sequencer abort safer In contrast to "git am --abort", a sequencer abort did not check whether the current HEAD is the one that is expected. This can lead to loss of work (when not spotted and resolved using reflog before the garbage collector chimes in). This behavior is now changed by mimicking "git am --abort". The abortion is done but HEAD is not changed when the current HEAD is not the expected HEAD. A new file "sequencer/abort-safety" is added to save the expected HEAD. The new behavior is only active when --abort is invoked on multiple picks. The problem does not occur for the single-pick case because it is handled differently. Signed-off-by: Stephan Beyer Signed-off-by: Junio C Hamano --- sequencer.c | 49 +++++++++++++++++++++++++++++++++ t/t3510-cherry-pick-sequence.sh | 2 +- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 30b10ba143..0b78f3149f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") +static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety") /* * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and @@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts) return -1; } +static void update_abort_safety_file(void) +{ + struct object_id head; + + /* Do nothing on a single-pick */ + if (!file_exists(git_path_seq_dir())) + return; + + if (!get_oid("HEAD", &head)) + write_file(git_path_abort_safety_file(), "%s", oid_to_hex(&head)); + else + write_file(git_path_abort_safety_file(), "%s", ""); +} + static int fast_forward_to(const unsigned char *to, const unsigned char *from, int unborn, struct replay_opts *opts) { @@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, strbuf_release(&sb); strbuf_release(&err); ref_transaction_free(transaction); + update_abort_safety_file(); return 0; } @@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, leave: free_message(commit, &msg); + update_abort_safety_file(); return res; } @@ -1132,9 +1149,34 @@ static int save_head(const char *head) return 0; } +static int rollback_is_safe(void) +{ + struct strbuf sb = STRBUF_INIT; + struct object_id expected_head, actual_head; + + if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) { + strbuf_trim(&sb); + if (get_oid_hex(sb.buf, &expected_head)) { + strbuf_release(&sb); + die(_("could not parse %s"), git_path_abort_safety_file()); + } + strbuf_release(&sb); + } + else if (errno == ENOENT) + oidclr(&expected_head); + else + die_errno(_("could not read '%s'"), git_path_abort_safety_file()); + + if (get_oid("HEAD", &actual_head)) + oidclr(&actual_head); + + return !oidcmp(&actual_head, &expected_head); +} + static int reset_for_rollback(const unsigned char *sha1) { const char *argv[4]; /* reset --merge + NULL */ + argv[0] = "reset"; argv[1] = "--merge"; argv[2] = sha1_to_hex(sha1); @@ -1189,6 +1231,12 @@ int sequencer_rollback(struct replay_opts *opts) error(_("cannot abort from a branch yet to be born")); goto fail; } + + if (!rollback_is_safe()) { + /* Do not error, just do not rollback */ + warning(_("You seem to have moved HEAD. " + "Not rewinding, check your HEAD!")); + } else if (reset_for_rollback(sha1)) goto fail; strbuf_release(&buf); @@ -1393,6 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts) return -1; if (save_opts(opts)) return -1; + update_abort_safety_file(); res = pick_commits(&todo_list, opts); todo_list_release(&todo_list); return res; diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index efcd4fc485..372307c21b 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' ' git diff-index --exit-code HEAD ' -test_expect_failure '--abort does not unsafely change HEAD' ' +test_expect_success '--abort does not unsafely change HEAD' ' pristine_detach initial && test_must_fail git cherry-pick picked anotherpick && git reset --hard base && From 39784cd3620cc47415c9010ec58a9616f040125c Mon Sep 17 00:00:00 2001 From: Stephan Beyer Date: Wed, 7 Dec 2016 22:51:33 +0100 Subject: [PATCH 5/6] sequencer: remove useless get_dir() function This function is used only once, for the removal of the directory. It is not used for the creation of the directory nor anywhere else. Signed-off-by: Stephan Beyer Signed-off-by: Junio C Hamano --- sequencer.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/sequencer.c b/sequencer.c index 0b78f3149f..3ac4cb8d3b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -47,11 +47,6 @@ static inline int is_rebase_i(const struct replay_opts *opts) return 0; } -static const char *get_dir(const struct replay_opts *opts) -{ - return git_path_seq_dir(); -} - static const char *get_todo_path(const struct replay_opts *opts) { return git_path_todo_file(); @@ -160,7 +155,7 @@ int sequencer_remove_state(struct replay_opts *opts) free(opts->xopts[i]); free(opts->xopts); - strbuf_addf(&dir, "%s", get_dir(opts)); + strbuf_addf(&dir, "%s", git_path_seq_dir()); remove_dir_recursively(&dir, 0); strbuf_release(&dir); From ce73bb22d87abe16885b3e871a10de5a261e499f Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 14 Dec 2016 14:56:46 -0800 Subject: [PATCH 6/6] Revert "sequencer: remove useless get_dir() function" This reverts commit 39784cd3620cc47415c9010ec58a9616f040125c. The function had only one caller when the "remove useless" was written, but another topic will soon make heavy use of it and more importantly the function will return different paths depending on the value in opts. --- sequencer.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 3ac4cb8d3b..0b78f3149f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -47,6 +47,11 @@ static inline int is_rebase_i(const struct replay_opts *opts) return 0; } +static const char *get_dir(const struct replay_opts *opts) +{ + return git_path_seq_dir(); +} + static const char *get_todo_path(const struct replay_opts *opts) { return git_path_todo_file(); @@ -155,7 +160,7 @@ int sequencer_remove_state(struct replay_opts *opts) free(opts->xopts[i]); free(opts->xopts); - strbuf_addf(&dir, "%s", git_path_seq_dir()); + strbuf_addf(&dir, "%s", get_dir(opts)); remove_dir_recursively(&dir, 0); strbuf_release(&dir);