{"thread":{"id":"44646","subject":"[PATCH 4/5] Make sequencer abort safer","startedAt":"2016-12-07T21:52:07Z","lastAt":"2016-12-10T20:21:08Z","messageCount":18,"participants":["Stephan Beyer","Paul Tan","Johannes Schindelin","Junio C Hamano","Christian Couder","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"307169","messageId":"20161207215133.13433-4-s-beyer@gmx.net","threadId":"44646","inReplyTo":"20161207215133.13433-1-s-beyer@gmx.net","subject":"[PATCH 4/5] Make sequencer abort safer","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2016-12-07T21:51:32Z","receivedAt":"2016-12-07T21:52:07Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"In contrast to \"git am --abort\", a sequencer abort did not check\nwhether the current HEAD is the one that is expected. This can\nlead to loss of work (when not spotted and resolved using reflog\nbefore the garbage collector chimes in).\n\nThis behavior is now changed by mimicking \"git am --abort\":\nthe abortion is done but HEAD is not changed when the current HEAD\nis not the expected HEAD.\n\nA new file \"sequencer/current\" is added to save the expected HEAD.\n\nThe new behavior is only active when --abort is invoked on multiple\npicks. The problem does not occur for the single-pick case because\nit is handled differently.\n\nSigned-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\n sequencer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 49 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 30b10ba14..c9b560ac1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n static GIT_PATH_FUNC(git_path_todo_file, \"sequencer/todo\")\n static GIT_PATH_FUNC(git_path_opts_file, \"sequencer/opts\")\n static GIT_PATH_FUNC(git_path_head_file, \"sequencer/head\")\n+static GIT_PATH_FUNC(git_path_curr_file, \"sequencer/current\")\n \n /*\n  * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and\n@@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts)\n \treturn -1;\n }\n \n+static void update_curr_file()\n+{\n+\tstruct object_id head;\n+\n+\t/* Do nothing on a single-pick */\n+\tif (!file_exists(git_path_seq_dir()))\n+\t\treturn;\n+\n+\tif (!get_oid(\"HEAD\", &head))\n+\t\twrite_file(git_path_curr_file(), \"%s\", oid_to_hex(&head));\n+\telse\n+\t\twrite_file(git_path_curr_file(), \"%s\", \"\");\n+}\n+\n static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \t\t\tint unborn, struct replay_opts *opts)\n {\n@@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \tstrbuf_release(&sb);\n \tstrbuf_release(&err);\n \tref_transaction_free(transaction);\n+\tupdate_curr_file();\n \treturn 0;\n }\n \n@@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \n leave:\n \tfree_message(commit, &msg);\n+\tupdate_curr_file();\n \n \treturn res;\n }\n@@ -1132,9 +1149,34 @@ static int save_head(const char *head)\n \treturn 0;\n }\n \n+static int rollback_is_safe()\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct object_id expected_head, actual_head;\n+\n+\tif (strbuf_read_file(&sb, git_path_curr_file(), 0) >= 0) {\n+\t\tstrbuf_trim(&sb);\n+\t\tif (get_oid_hex(sb.buf, &expected_head)) {\n+\t\t\tstrbuf_release(&sb);\n+\t\t\tdie(_(\"could not parse %s\"), git_path_curr_file());\n+\t\t}\n+\t\tstrbuf_release(&sb);\n+\t}\n+\telse if (errno == ENOENT)\n+\t\toidclr(&expected_head);\n+\telse\n+\t\tdie_errno(_(\"could not read '%s'\"), git_path_curr_file());\n+\n+\tif (get_oid(\"HEAD\", &actual_head))\n+\t\toidclr(&actual_head);\n+\n+\treturn !oidcmp(&actual_head, &expected_head);\n+}\n+\n static int reset_for_rollback(const unsigned char *sha1)\n {\n \tconst char *argv[4];\t/* reset --merge <arg> + NULL */\n+\n \targv[0] = \"reset\";\n \targv[1] = \"--merge\";\n \targv[2] = sha1_to_hex(sha1);\n@@ -1189,6 +1231,12 @@ int sequencer_rollback(struct replay_opts *opts)\n \t\terror(_(\"cannot abort from a branch yet to be born\"));\n \t\tgoto fail;\n \t}\n+\n+\tif (!rollback_is_safe()) {\n+\t\t/* Do not error, just do not rollback */\n+\t\twarning(_(\"You seem to have moved HEAD. \"\n+\t\t\t  \"Not rewinding, check your HEAD!\"));\n+\t} else\n \tif (reset_for_rollback(sha1))\n \t\tgoto fail;\n \tstrbuf_release(&buf);\n@@ -1393,6 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\treturn -1;\n \tif (save_opts(opts))\n \t\treturn -1;\n+\tupdate_curr_file();\n \tres = pick_commits(&todo_list, opts);\n \ttodo_list_release(&todo_list);\n \treturn res;\n-- \n2.11.0.27.g4eed97c\n\n"},{"id":"307170","messageId":"20161207215133.13433-2-s-beyer@gmx.net","threadId":"44646","inReplyTo":"20161207215133.13433-1-s-beyer@gmx.net","subject":"[PATCH 2/5] am: Change safe_to_abort()'s not rewinding error into a warning","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2016-12-07T21:51:30Z","receivedAt":"2016-12-07T21:52:12Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"The error message tells the user that something went terribly wrong\nand the --abort could not be performed. But the --abort is performed,\nonly without rewinding. By simply changing the error into a warning,\nwe indicate the user that she must not try something like\n\"git am --abort --force\", instead she just has to check the HEAD.\n\nSigned-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\n builtin/am.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 7cf40e6f2..826f18ba1 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2134,7 +2134,7 @@ static int safe_to_abort(const struct am_state *state)\n \tif (!oidcmp(&head, &abort_safety))\n \t\treturn 1;\n \n-\terror(_(\"You seem to have moved HEAD since the last 'am' failure.\\n\"\n+\twarning(_(\"You seem to have moved HEAD since the last 'am' failure.\\n\"\n \t\t\"Not rewinding to ORIG_HEAD\"));\n \n \treturn 0;\n-- \n2.11.0.27.g4eed97c\n\n"},{"id":"307171","messageId":"20161207215133.13433-1-s-beyer@gmx.net","threadId":"44646","inReplyTo":"xmqqlgvs28bh.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 1/5] am: Fix filename in safe_to_abort() error message","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2016-12-07T21:51:29Z","receivedAt":"2016-12-07T21:52:16Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"Signed-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\n Okay let's give it a try. Some minor things that I found\n are also in this patchset (patch 01, 02 and 05).\n The branch can also be found on\n   https://github.com/sbeyer/git/commits/sequencer-abort-safety\n\n builtin/am.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 6981f42ce..7cf40e6f2 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)\n \n \tif (read_state_file(&sb, state, \"abort-safety\", 1) > 0) {\n \t\tif (get_oid_hex(sb.buf, &abort_safety))\n-\t\t\tdie(_(\"could not parse %s\"), am_path(state, \"abort_safety\"));\n+\t\t\tdie(_(\"could not parse %s\"), am_path(state, \"abort-safety\"));\n \t} else\n \t\toidclr(&abort_safety);\n \n-- \n2.11.0.27.g4eed97c\n\n"},{"id":"307172","messageId":"20161207215133.13433-3-s-beyer@gmx.net","threadId":"44646","inReplyTo":"20161207215133.13433-1-s-beyer@gmx.net","subject":"[PATCH 3/5] Add test that cherry-pick --abort does not unsafely change HEAD","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2016-12-07T21:51:31Z","receivedAt":"2016-12-07T21:52:18Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"Signed-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\n t/t3510-cherry-pick-sequence.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 7b7a89dbd..372307c21 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -147,6 +147,16 @@ test_expect_success '--abort to cancel single cherry-pick' '\n \tgit diff-index --exit-code HEAD\n '\n \n+test_expect_success '--abort does not unsafely change HEAD' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick picked anotherpick &&\n+\tgit reset --hard base &&\n+\ttest_must_fail git cherry-pick picked anotherpick &&\n+\tgit cherry-pick --abort 2>actual &&\n+\ttest_i18ngrep \"You seem to have moved HEAD\" actual &&\n+\ttest_cmp_rev base HEAD\n+'\n+\n test_expect_success 'cherry-pick --abort to cancel multiple revert' '\n \tpristine_detach anotherpick &&\n \ttest_expect_code 1 git revert base..picked &&\n-- \n2.11.0.27.g4eed97c\n\n"},{"id":"307173","messageId":"20161207215133.13433-5-s-beyer@gmx.net","threadId":"44646","inReplyTo":"20161207215133.13433-1-s-beyer@gmx.net","subject":"[PATCH 5/5] sequencer: Remove useless get_dir() function","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2016-12-07T21:51:33Z","receivedAt":"2016-12-07T21:52:21Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"This function is used only once, for the removal of the\ndirectory. It is not used for the creation of the directory\nnor anywhere else.\n\nSigned-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\n sequencer.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex c9b560ac1..689cfa5f1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -47,11 +47,6 @@ static inline int is_rebase_i(const struct replay_opts *opts)\n \treturn 0;\n }\n \n-static const char *get_dir(const struct replay_opts *opts)\n-{\n-\treturn git_path_seq_dir();\n-}\n-\n static const char *get_todo_path(const struct replay_opts *opts)\n {\n \treturn git_path_todo_file();\n@@ -160,7 +155,7 @@ int sequencer_remove_state(struct replay_opts *opts)\n \t\tfree(opts->xopts[i]);\n \tfree(opts->xopts);\n \n-\tstrbuf_addf(&dir, \"%s\", get_dir(opts));\n+\tstrbuf_addf(&dir, \"%s\", git_path_seq_dir());\n \tremove_dir_recursively(&dir, 0);\n \tstrbuf_release(&dir);\n \n-- \n2.11.0.27.g4eed97c\n\n"},{"id":"307229","messageId":"CACRoPnSzHo2wqZyP6nfMaZBCy9-m6E72vTGRTjGcuSdo-qJ4dw@mail.gmail.com","threadId":"44646","inReplyTo":"20161207215133.13433-1-s-beyer@gmx.net","subject":"Re: [PATCH 1/5] am: Fix filename in safe_to_abort() error message","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2016-12-08T10:21:23Z","receivedAt":"2016-12-08T10:21:29Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi Stephan,\n\nOn Thu, Dec 8, 2016 at 5:51 AM, Stephan Beyer <s-beyer@gmx.net> wrote:\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 6981f42ce..7cf40e6f2 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)\n>\n>         if (read_state_file(&sb, state, \"abort-safety\", 1) > 0) {\n>                 if (get_oid_hex(sb.buf, &abort_safety))\n> -                       die(_(\"could not parse %s\"), am_path(state, \"abort_safety\"));\n> +                       die(_(\"could not parse %s\"), am_path(state, \"abort-safety\"));\n\nAh, this is obviously correct. Sorry for the oversight.\n\n>         } else\n>                 oidclr(&abort_safety);\n>\n> --\n> 2.11.0.27.g4eed97c\n\nThanks,\nPaul\n"},{"id":"307253","messageId":"alpine.DEB.2.20.1612081627290.23160@virtualbox","threadId":"44646","inReplyTo":"20161207215133.13433-4-s-beyer@gmx.net","subject":"Re: [PATCH 4/5] Make sequencer abort safer","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-08T15:28:06Z","receivedAt":"2016-12-08T15:28:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 7 Dec 2016, Stephan Beyer wrote:\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 30b10ba14..c9b560ac1 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n>  static GIT_PATH_FUNC(git_path_todo_file, \"sequencer/todo\")\n>  static GIT_PATH_FUNC(git_path_opts_file, \"sequencer/opts\")\n>  static GIT_PATH_FUNC(git_path_head_file, \"sequencer/head\")\n> +static GIT_PATH_FUNC(git_path_curr_file, \"sequencer/current\")\n\nIs it required by law to have a four-letter infix, or can we have a nicer\nvariable name (e.g. git_path_current_file)?\n\nCiao,\nDscho\n"},{"id":"307266","messageId":"xmqqr35itjor.fsf@gitster.mtv.corp.google.com","threadId":"44646","inReplyTo":"alpine.DEB.2.20.1612081627290.23160@virtualbox","subject":"Re: [PATCH 4/5] Make sequencer abort safer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-08T17:27:48Z","receivedAt":"2016-12-08T17:27:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Wed, 7 Dec 2016, Stephan Beyer wrote:\n>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 30b10ba14..c9b560ac1 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n>>  static GIT_PATH_FUNC(git_path_todo_file, \"sequencer/todo\")\n>>  static GIT_PATH_FUNC(git_path_opts_file, \"sequencer/opts\")\n>>  static GIT_PATH_FUNC(git_path_head_file, \"sequencer/head\")\n>> +static GIT_PATH_FUNC(git_path_curr_file, \"sequencer/current\")\n>\n> Is it required by law to have a four-letter infix, or can we have a nicer\n> variable name (e.g. git_path_current_file)?\n\nI agree with you that, as other git_path_*_file variables match the\nactual name on the filesystem, this one should too, together with\nthe update_curr_file() function.\n\nBy the way, this step seems to be a fix to an existing problem, and\nthe new test added in 3/5 seems to be a demonstration of the issue.\nIf that is the case, shouldn't the new test initially expect failure\nand updated by this step to expect success?\n\nI'll queue this on top of step 4/5 as \"SQUASH???\" as usual.  The\nother SQUASH??? that must come after 3/5 for t3510 should be trivial\n(the reverse of what appears here).\n\nThanks.\n\n\n sequencer.c                     | 22 +++++++++++-----------\n t/t3510-cherry-pick-sequence.sh |  2 +-\n 2 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex c9b560ac15..ce04377f8e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -27,7 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n static GIT_PATH_FUNC(git_path_todo_file, \"sequencer/todo\")\n static GIT_PATH_FUNC(git_path_opts_file, \"sequencer/opts\")\n static GIT_PATH_FUNC(git_path_head_file, \"sequencer/head\")\n-static GIT_PATH_FUNC(git_path_curr_file, \"sequencer/current\")\n+static GIT_PATH_FUNC(git_path_current_file, \"sequencer/current\")\n \n /*\n  * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and\n@@ -311,7 +311,7 @@ static int error_dirty_index(struct replay_opts *opts)\n \treturn -1;\n }\n \n-static void update_curr_file()\n+static void update_current_file(void)\n {\n \tstruct object_id head;\n \n@@ -320,9 +320,9 @@ static void update_curr_file()\n \t\treturn;\n \n \tif (!get_oid(\"HEAD\", &head))\n-\t\twrite_file(git_path_curr_file(), \"%s\", oid_to_hex(&head));\n+\t\twrite_file(git_path_current_file(), \"%s\", oid_to_hex(&head));\n \telse\n-\t\twrite_file(git_path_curr_file(), \"%s\", \"\");\n+\t\twrite_file(git_path_current_file(), \"%s\", \"\");\n }\n \n static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n@@ -354,7 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \tstrbuf_release(&sb);\n \tstrbuf_release(&err);\n \tref_transaction_free(transaction);\n-\tupdate_curr_file();\n+\tupdate_current_file();\n \treturn 0;\n }\n \n@@ -829,7 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \n leave:\n \tfree_message(commit, &msg);\n-\tupdate_curr_file();\n+\tupdate_current_file();\n \n \treturn res;\n }\n@@ -1149,23 +1149,23 @@ static int save_head(const char *head)\n \treturn 0;\n }\n \n-static int rollback_is_safe()\n+static int rollback_is_safe(void)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct object_id expected_head, actual_head;\n \n-\tif (strbuf_read_file(&sb, git_path_curr_file(), 0) >= 0) {\n+\tif (strbuf_read_file(&sb, git_path_current_file(), 0) >= 0) {\n \t\tstrbuf_trim(&sb);\n \t\tif (get_oid_hex(sb.buf, &expected_head)) {\n \t\t\tstrbuf_release(&sb);\n-\t\t\tdie(_(\"could not parse %s\"), git_path_curr_file());\n+\t\t\tdie(_(\"could not parse %s\"), git_path_current_file());\n \t\t}\n \t\tstrbuf_release(&sb);\n \t}\n \telse if (errno == ENOENT)\n \t\toidclr(&expected_head);\n \telse\n-\t\tdie_errno(_(\"could not read '%s'\"), git_path_curr_file());\n+\t\tdie_errno(_(\"could not read '%s'\"), git_path_current_file());\n \n \tif (get_oid(\"HEAD\", &actual_head))\n \t\toidclr(&actual_head);\n@@ -1441,7 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\treturn -1;\n \tif (save_opts(opts))\n \t\treturn -1;\n-\tupdate_curr_file();\n+\tupdate_current_file();\n \tres = pick_commits(&todo_list, opts);\n \ttodo_list_release(&todo_list);\n \treturn res;\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex efcd4fc485..372307c21b 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' '\n \tgit diff-index --exit-code HEAD\n '\n \n-test_expect_failure '--abort does not unsafely change HEAD' '\n+test_expect_success '--abort does not unsafely change HEAD' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick picked anotherpick &&\n \tgit reset --hard base &&\n"},{"id":"307300","messageId":"c02708de-8b47-e490-4a1e-77f5727b1156@gmx.net","threadId":"44646","inReplyTo":"xmqqr35itjor.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/5] Make sequencer abort safer","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2016-12-08T19:17:29Z","receivedAt":"2016-12-08T19:17:43Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"Hi,\n\nI'm a little afraid of feeding Parkinson's law of triviality here, but... ;)\n\nOn 12/08/2016 06:27 PM, Junio C Hamano wrote:\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n>> On Wed, 7 Dec 2016, Stephan Beyer wrote:\n>>\n>>> diff --git a/sequencer.c b/sequencer.c\n>>> index 30b10ba14..c9b560ac1 100644\n>>> --- a/sequencer.c\n>>> +++ b/sequencer.c\n>>> @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n>>>  static GIT_PATH_FUNC(git_path_todo_file, \"sequencer/todo\")\n>>>  static GIT_PATH_FUNC(git_path_opts_file, \"sequencer/opts\")\n>>>  static GIT_PATH_FUNC(git_path_head_file, \"sequencer/head\")\n>>> +static GIT_PATH_FUNC(git_path_curr_file, \"sequencer/current\")\n>>\n>> Is it required by law to have a four-letter infix, or can we have a nicer\n>> variable name (e.g. git_path_current_file)?\n> \n> I agree with you that, as other git_path_*_file variables match the\n> actual name on the filesystem, this one should too, together with\n> the update_curr_file() function.\n\nI totally agree with that (and I don't know why I used \"curr\", probably\njust because it looked consistent and good...).\n\nHowever:\n\n> -static void update_curr_file()\n> +static void update_current_file(void)\n\nThis function name could lead to the impression that there is some\ncurrent file (defined by a global state or whatever) that is updated.\n\nSo I'd rather rename the *file* to one of\n\n * sequencer/abort-safety (consistent to am, describes its purpose)\n * sequencer/safety (shorter, still describes the purpose)\n * sequencer/current-head (describes what it contains)\n * sequencer/last (a four-letter word, not totally unambiguous though)\n\n> By the way, this step seems to be a fix to an existing problem, and\n> the new test added in 3/5 seems to be a demonstration of the issue.\n> If that is the case, shouldn't the new test initially expect failure\n> and updated by this step to expect success?\n\nThat's usually a matter of taste that I sometimes also discuss with\ncolleagues in other projects... However, for the git test suite with its\n\"known breakage\" behavior, your recommendation is surely the best way to\ndo it (aside from introducing the test and the fix in one commit... but\nthat does not show in the history that there actually was that breakage)\n\n~Stephan\n"},{"id":"307375","messageId":"xmqq4m2drlys.fsf@gitster.mtv.corp.google.com","threadId":"44646","inReplyTo":"c02708de-8b47-e490-4a1e-77f5727b1156@gmx.net","subject":"Re: [PATCH 4/5] Make sequencer abort safer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-09T18:33:47Z","receivedAt":"2016-12-09T18:33:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephan Beyer <s-beyer@gmx.net> writes:\n\n> However:\n>\n>> -static void update_curr_file()\n>> +static void update_current_file(void)\n>\n> This function name could lead to the impression that there is some\n> current file (defined by a global state or whatever) that is updated.\n>\n> So I'd rather rename the *file* to one of\n>\n>  * sequencer/abort-safety (consistent to am, describes its purpose)\n>  * sequencer/safety (shorter, still describes the purpose)\n>  * sequencer/current-head (describes what it contains)\n>  * sequencer/last (a four-letter word, not totally unambiguous though)\n\nOK, so here is a patch that needs to be squashed further on top of\n4/5.  I just picked the first one on your list ;-)\n\nThanks.\n\n sequencer.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 874aaa4cd4..3ac4cb8d3b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -27,7 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n static GIT_PATH_FUNC(git_path_todo_file, \"sequencer/todo\")\n static GIT_PATH_FUNC(git_path_opts_file, \"sequencer/opts\")\n static GIT_PATH_FUNC(git_path_head_file, \"sequencer/head\")\n-static GIT_PATH_FUNC(git_path_current_file, \"sequencer/current\")\n+static GIT_PATH_FUNC(git_path_abort_safety_file, \"sequencer/abort-safety\")\n \n /*\n  * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and\n@@ -306,7 +306,7 @@ static int error_dirty_index(struct replay_opts *opts)\n \treturn -1;\n }\n \n-static void update_current_file(void)\n+static void update_abort_safety_file(void)\n {\n \tstruct object_id head;\n \n@@ -315,9 +315,9 @@ static void update_current_file(void)\n \t\treturn;\n \n \tif (!get_oid(\"HEAD\", &head))\n-\t\twrite_file(git_path_current_file(), \"%s\", oid_to_hex(&head));\n+\t\twrite_file(git_path_abort_safety_file(), \"%s\", oid_to_hex(&head));\n \telse\n-\t\twrite_file(git_path_current_file(), \"%s\", \"\");\n+\t\twrite_file(git_path_abort_safety_file(), \"%s\", \"\");\n }\n \n static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n@@ -349,7 +349,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \tstrbuf_release(&sb);\n \tstrbuf_release(&err);\n \tref_transaction_free(transaction);\n-\tupdate_current_file();\n+\tupdate_abort_safety_file();\n \treturn 0;\n }\n \n@@ -824,7 +824,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \n leave:\n \tfree_message(commit, &msg);\n-\tupdate_current_file();\n+\tupdate_abort_safety_file();\n \n \treturn res;\n }\n@@ -1149,18 +1149,18 @@ static int rollback_is_safe(void)\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct object_id expected_head, actual_head;\n \n-\tif (strbuf_read_file(&sb, git_path_current_file(), 0) >= 0) {\n+\tif (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {\n \t\tstrbuf_trim(&sb);\n \t\tif (get_oid_hex(sb.buf, &expected_head)) {\n \t\t\tstrbuf_release(&sb);\n-\t\t\tdie(_(\"could not parse %s\"), git_path_current_file());\n+\t\t\tdie(_(\"could not parse %s\"), git_path_abort_safety_file());\n \t\t}\n \t\tstrbuf_release(&sb);\n \t}\n \telse if (errno == ENOENT)\n \t\toidclr(&expected_head);\n \telse\n-\t\tdie_errno(_(\"could not read '%s'\"), git_path_current_file());\n+\t\tdie_errno(_(\"could not read '%s'\"), git_path_abort_safety_file());\n \n \tif (get_oid(\"HEAD\", &actual_head))\n \t\toidclr(&actual_head);\n@@ -1436,7 +1436,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\treturn -1;\n \tif (save_opts(opts))\n \t\treturn -1;\n-\tupdate_current_file();\n+\tupdate_abort_safety_file();\n \tres = pick_commits(&todo_list, opts);\n \ttodo_list_release(&todo_list);\n \treturn res;\n"},{"id":"307378","messageId":"20161209190111.9571-1-s-beyer@gmx.net","threadId":"44646","inReplyTo":"xmqq4m2drlys.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2 1/5] am: Fix filename in safe_to_abort() error message","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2016-12-09T19:01:07Z","receivedAt":"2016-12-09T19:01:42Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"Signed-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\n builtin/am.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 6981f42ce..7cf40e6f2 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)\n \n \tif (read_state_file(&sb, state, \"abort-safety\", 1) > 0) {\n \t\tif (get_oid_hex(sb.buf, &abort_safety))\n-\t\t\tdie(_(\"could not parse %s\"), am_path(state, \"abort_safety\"));\n+\t\t\tdie(_(\"could not parse %s\"), am_path(state, \"abort-safety\"));\n \t} else\n \t\toidclr(&abort_safety);\n \n-- \n2.11.0.27.g74d6bea\n\n"},{"id":"307379","messageId":"20161209190111.9571-3-s-beyer@gmx.net","threadId":"44646","inReplyTo":"20161209190111.9571-1-s-beyer@gmx.net","subject":"[PATCH v2 3/5] Add test that cherry-pick --abort does not unsafely change HEAD","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2016-12-09T19:01:09Z","receivedAt":"2016-12-09T19:01:47Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"The test expects failure because it is a current breakage\nreported by Junio C Hamano.\n\nSigned-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\n t/t3510-cherry-pick-sequence.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 7b7a89dbd..efcd4fc48 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -147,6 +147,16 @@ test_expect_success '--abort to cancel single cherry-pick' '\n \tgit diff-index --exit-code HEAD\n '\n \n+test_expect_failure '--abort does not unsafely change HEAD' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick picked anotherpick &&\n+\tgit reset --hard base &&\n+\ttest_must_fail git cherry-pick picked anotherpick &&\n+\tgit cherry-pick --abort 2>actual &&\n+\ttest_i18ngrep \"You seem to have moved HEAD\" actual &&\n+\ttest_cmp_rev base HEAD\n+'\n+\n test_expect_success 'cherry-pick --abort to cancel multiple revert' '\n \tpristine_detach anotherpick &&\n \ttest_expect_code 1 git revert base..picked &&\n-- \n2.11.0.27.g74d6bea\n\n"},{"id":"307380","messageId":"20161209190111.9571-2-s-beyer@gmx.net","threadId":"44646","inReplyTo":"20161209190111.9571-1-s-beyer@gmx.net","subject":"[PATCH v2 2/5] am: Change safe_to_abort()'s not rewinding error into a warning","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2016-12-09T19:01:08Z","receivedAt":"2016-12-09T19:01:49Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"The error message tells the user that something went terribly wrong\nand the --abort could not be performed. But the --abort is performed,\nonly without rewinding. By simply changing the error into a warning,\nwe indicate the user that she must not try something like\n\"git am --abort --force\", instead she just has to check the HEAD.\n\nSigned-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\n builtin/am.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 7cf40e6f2..826f18ba1 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2134,7 +2134,7 @@ static int safe_to_abort(const struct am_state *state)\n \tif (!oidcmp(&head, &abort_safety))\n \t\treturn 1;\n \n-\terror(_(\"You seem to have moved HEAD since the last 'am' failure.\\n\"\n+\twarning(_(\"You seem to have moved HEAD since the last 'am' failure.\\n\"\n \t\t\"Not rewinding to ORIG_HEAD\"));\n \n \treturn 0;\n-- \n2.11.0.27.g74d6bea\n\n"},{"id":"307381","messageId":"20161209190111.9571-4-s-beyer@gmx.net","threadId":"44646","inReplyTo":"20161209190111.9571-1-s-beyer@gmx.net","subject":"[PATCH v2 4/5] Make sequencer abort safer","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2016-12-09T19:01:10Z","receivedAt":"2016-12-09T19:01:52Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"In contrast to \"git am --abort\", a sequencer abort did not check\nwhether the current HEAD is the one that is expected. This can\nlead to loss of work (when not spotted and resolved using reflog\nbefore the garbage collector chimes in).\n\nThis behavior is now changed by mimicking \"git am --abort\":\nthe abortion is done but HEAD is not changed when the current HEAD\nis not the expected HEAD.\n\nA new file \"sequencer/current\" is added to save the expected HEAD.\n\nThe new behavior is only active when --abort is invoked on multiple\npicks. The problem does not occur for the single-pick case because\nit is handled differently.\n\nSigned-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\n sequencer.c                     | 48 +++++++++++++++++++++++++++++++++++++++++\n t/t3510-cherry-pick-sequence.sh |  2 +-\n 2 files changed, 49 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 30b10ba14..35c158471 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n static GIT_PATH_FUNC(git_path_todo_file, \"sequencer/todo\")\n static GIT_PATH_FUNC(git_path_opts_file, \"sequencer/opts\")\n static GIT_PATH_FUNC(git_path_head_file, \"sequencer/head\")\n+static GIT_PATH_FUNC(git_path_abort_safety_file, \"sequencer/abort-safety\")\n \n /*\n  * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and\n@@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts)\n \treturn -1;\n }\n \n+static void update_abort_safety_file(void)\n+{\n+\tstruct object_id head;\n+\n+\t/* Do nothing on a single-pick */\n+\tif (!file_exists(git_path_seq_dir()))\n+\t\treturn;\n+\n+\tif (!get_oid(\"HEAD\", &head))\n+\t\twrite_file(git_path_abort_safety_file(), \"%s\", oid_to_hex(&head));\n+\telse\n+\t\twrite_file(git_path_abort_safety_file(), \"%s\", \"\");\n+}\n+\n static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \t\t\tint unborn, struct replay_opts *opts)\n {\n@@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \tstrbuf_release(&sb);\n \tstrbuf_release(&err);\n \tref_transaction_free(transaction);\n+\tupdate_abort_safety_file();\n \treturn 0;\n }\n \n@@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \n leave:\n \tfree_message(commit, &msg);\n+\tupdate_abort_safety_file();\n \n \treturn res;\n }\n@@ -1132,6 +1149,30 @@ static int save_head(const char *head)\n \treturn 0;\n }\n \n+static int rollback_is_safe(void)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct object_id expected_head, actual_head;\n+\n+\tif (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {\n+\t\tstrbuf_trim(&sb);\n+\t\tif (get_oid_hex(sb.buf, &expected_head)) {\n+\t\t\tstrbuf_release(&sb);\n+\t\t\tdie(_(\"could not parse %s\"), git_path_abort_safety_file());\n+\t\t}\n+\t\tstrbuf_release(&sb);\n+\t}\n+\telse if (errno == ENOENT)\n+\t\toidclr(&expected_head);\n+\telse\n+\t\tdie_errno(_(\"could not read '%s'\"), git_path_abort_safety_file());\n+\n+\tif (get_oid(\"HEAD\", &actual_head))\n+\t\toidclr(&actual_head);\n+\n+\treturn !oidcmp(&actual_head, &expected_head);\n+}\n+\n static int reset_for_rollback(const unsigned char *sha1)\n {\n \tconst char *argv[4];\t/* reset --merge <arg> + NULL */\n@@ -1189,6 +1230,12 @@ int sequencer_rollback(struct replay_opts *opts)\n \t\terror(_(\"cannot abort from a branch yet to be born\"));\n \t\tgoto fail;\n \t}\n+\n+\tif (!rollback_is_safe()) {\n+\t\t/* Do not error, just do not rollback */\n+\t\twarning(_(\"You seem to have moved HEAD. \"\n+\t\t\t  \"Not rewinding, check your HEAD!\"));\n+\t} else\n \tif (reset_for_rollback(sha1))\n \t\tgoto fail;\n \tstrbuf_release(&buf);\n@@ -1393,6 +1440,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\treturn -1;\n \tif (save_opts(opts))\n \t\treturn -1;\n+\tupdate_abort_safety_file();\n \tres = pick_commits(&todo_list, opts);\n \ttodo_list_release(&todo_list);\n \treturn res;\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex efcd4fc48..372307c21 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' '\n \tgit diff-index --exit-code HEAD\n '\n \n-test_expect_failure '--abort does not unsafely change HEAD' '\n+test_expect_success '--abort does not unsafely change HEAD' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick picked anotherpick &&\n \tgit reset --hard base &&\n-- \n2.11.0.27.g74d6bea\n\n"},{"id":"307382","messageId":"20161209190111.9571-5-s-beyer@gmx.net","threadId":"44646","inReplyTo":"20161209190111.9571-1-s-beyer@gmx.net","subject":"[PATCH v2 5/5] sequencer: Remove useless get_dir() function","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2016-12-09T19:01:11Z","receivedAt":"2016-12-09T19:01:53Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"This function is used only once, for the removal of the\ndirectory. It is not used for the creation of the directory\nnor anywhere else.\n\nSigned-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\n sequencer.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 35c158471..aba096a0a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -47,11 +47,6 @@ static inline int is_rebase_i(const struct replay_opts *opts)\n \treturn 0;\n }\n \n-static const char *get_dir(const struct replay_opts *opts)\n-{\n-\treturn git_path_seq_dir();\n-}\n-\n static const char *get_todo_path(const struct replay_opts *opts)\n {\n \treturn git_path_todo_file();\n@@ -160,7 +155,7 @@ int sequencer_remove_state(struct replay_opts *opts)\n \t\tfree(opts->xopts[i]);\n \tfree(opts->xopts);\n \n-\tstrbuf_addf(&dir, \"%s\", get_dir(opts));\n+\tstrbuf_addf(&dir, \"%s\", git_path_seq_dir());\n \tremove_dir_recursively(&dir, 0);\n \tstrbuf_release(&dir);\n \n-- \n2.11.0.27.g74d6bea\n\n"},{"id":"307445","messageId":"CAP8UFD0hCke_W6C=gOHinpj+G3WCFKf7Cji6zREDer4RUBxKxg@mail.gmail.com","threadId":"44646","inReplyTo":"20161209190111.9571-4-s-beyer@gmx.net","subject":"Re: [PATCH v2 4/5] Make sequencer abort safer","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-12-10T19:56:26Z","receivedAt":"2016-12-10T19:56:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Dec 9, 2016 at 8:01 PM, Stephan Beyer <s-beyer@gmx.net> wrote:\n\n[...]\n\n> +static int rollback_is_safe(void)\n> +{\n> +       struct strbuf sb = STRBUF_INIT;\n> +       struct object_id expected_head, actual_head;\n> +\n> +       if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {\n> +               strbuf_trim(&sb);\n> +               if (get_oid_hex(sb.buf, &expected_head)) {\n> +                       strbuf_release(&sb);\n> +                       die(_(\"could not parse %s\"), git_path_abort_safety_file());\n> +               }\n> +               strbuf_release(&sb);\n> +       }\n\nMaybe the following is a bit simpler:\n\n       if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {\n               int res;\n               strbuf_trim(&sb);\n               res = get_oid_hex(sb.buf, &expected_head);\n               strbuf_release(&sb);\n               if (res)\n                   die(_(\"could not parse %s\"), git_path_abort_safety_file());\n       }\n\nThanks,\nChristian.\n"},{"id":"307446","messageId":"20161210200437.ijmahia6e6xifhk6@sigill.intra.peff.net","threadId":"44646","inReplyTo":"CAP8UFD0hCke_W6C=gOHinpj+G3WCFKf7Cji6zREDer4RUBxKxg@mail.gmail.com","subject":"Re: [PATCH v2 4/5] Make sequencer abort safer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-10T20:04:38Z","receivedAt":"2016-12-10T20:04:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 10, 2016 at 08:56:26PM +0100, Christian Couder wrote:\n\n> > +static int rollback_is_safe(void)\n> > +{\n> > +       struct strbuf sb = STRBUF_INIT;\n> > +       struct object_id expected_head, actual_head;\n> > +\n> > +       if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {\n> > +               strbuf_trim(&sb);\n> > +               if (get_oid_hex(sb.buf, &expected_head)) {\n> > +                       strbuf_release(&sb);\n> > +                       die(_(\"could not parse %s\"), git_path_abort_safety_file());\n> > +               }\n> > +               strbuf_release(&sb);\n> > +       }\n> \n> Maybe the following is a bit simpler:\n> \n>        if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {\n>                int res;\n>                strbuf_trim(&sb);\n>                res = get_oid_hex(sb.buf, &expected_head);\n>                strbuf_release(&sb);\n>                if (res)\n>                    die(_(\"could not parse %s\"), git_path_abort_safety_file());\n>        }\n\nIs there any point in calling strbuf_release() if we're about to die\nanyway? I could see it if it were \"return error()\", but it's normal in\nour code base for die() to be abrupt.\n\n-Peff\n"},{"id":"307447","messageId":"102c7dd1-fa70-6a47-3f13-ddafbce4b13b@gmx.net","threadId":"44646","inReplyTo":"20161210200437.ijmahia6e6xifhk6@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/5] Make sequencer abort safer","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2016-12-10T20:20:41Z","receivedAt":"2016-12-10T20:21:08Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"On 12/10/2016 09:04 PM, Jeff King wrote:\n> On Sat, Dec 10, 2016 at 08:56:26PM +0100, Christian Couder wrote:\n> \n>>> +static int rollback_is_safe(void)\n>>> +{\n>>> +       struct strbuf sb = STRBUF_INIT;\n>>> +       struct object_id expected_head, actual_head;\n>>> +\n>>> +       if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {\n>>> +               strbuf_trim(&sb);\n>>> +               if (get_oid_hex(sb.buf, &expected_head)) {\n>>> +                       strbuf_release(&sb);\n>>> +                       die(_(\"could not parse %s\"), git_path_abort_safety_file());\n>>> +               }\n>>> +               strbuf_release(&sb);\n>>> +       }\n>>\n>> Maybe the following is a bit simpler:\n>>\n>>        if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {\n>>                int res;\n>>                strbuf_trim(&sb);\n>>                res = get_oid_hex(sb.buf, &expected_head);\n>>                strbuf_release(&sb);\n>>                if (res)\n>>                    die(_(\"could not parse %s\"), git_path_abort_safety_file());\n>>        }\n> \n> Is there any point in calling strbuf_release() if we're about to die\n> anyway? I could see it if it were \"return error()\", but it's normal in\n> our code base for die() to be abrupt.\n\nThe point is that someone \"libifies\" the function some day; then \"die()\"\nbecomes \"return error()\" almost automatically. Chances are high that the\nresulting memory leak is forgotten. That's one of the reasons why I like\nbeing strict about memory leaks.\n\nHowever, I cannot tell if mine or Christian's variant is really\n\"simpler\" (with whatever measure) and I also don't care much.\n\n~Stephan\n"}]}