{"thread":{"id":"55389","subject":"[PATCH] sequencer: fix edit handling for cherry-pick and revert messages","startedAt":"2021-03-26T07:17:13Z","lastAt":"2021-04-09T13:53:36Z","messageCount":28,"participants":["Elijah Newren via GitGitGadget","Philip Oakley","Elijah Newren","Junio C Hamano","Phillip Wood","Johannes Schindelin","Ævar Arnfjörð Bjarmason","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"420237","messageId":"pull.988.git.git.1616742969145.gitgitgadget@gmail.com","threadId":"55389","inReplyTo":null,"subject":"[PATCH] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-26T07:16:08Z","receivedAt":"2021-03-26T07:17:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nsave_opts() should save any non-default values. It was intended to do\nthis, but since most options in struct replay_opts default to 0, it only\nsaved non-zero values. Unfortunatley, this does not always work for\noptions.edit. Roughly speaking, options.edit had a default value of 0\nfor cherry-pick but a default value of 1 for revert. Make save_opts()\nrecord a value whenever it differs from the default.\n\noptions.edit was also overly simplistic; we had more than two cases.\nThe behavior that previously existed was as follows:\n\n Non-conflict commits Right after Conflict\n revert Edit iff isatty(0) Edit (ignore isatty(0))\n cherry-pick No edit See above\n Specify --edit Edit (ignore isatty(0)) See above\n Specify --no-edit (*) See above\n\n (*) Before stopping for conflicts, No edit is the behavior. After\n stopping for conflicts, the --no-edit flag is not saved so see\n the first two rows.\n\nHowever, the expected behavior is:\n\n Non-conflict commits Right after Conflict\n revert Edit iff isatty(0) Edit iff isatty(0)\n cherry-pick No edit Edit iff isatty(0)\n Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0))\n Specify --no-edit No edit No edit\n\nIn order to get the expected behavior, we need to change options.edit\nto a tri-state: unspecified, false, or true. When specified, we follow\nwhat it says. When unspecified, we need to check whether the current\ncommit being created is resolving a conflict as well as consulting\noptions.action and isatty(0). While at it, add a should_edit() utility\nfunction that compresses options.edit down to a boolean based on the\nadditional information for the non-conflict case.\n\nMake continue_single_pick() (which is the function responsible for\nresuming after conflict cases) stop assuming edit behavior in all cases,\nso that it can correctly handle !isatty(0) and specific requests to not\nedit the commit message.\n\nReported-by: Renato Botelho \nSigned-off-by: Elijah Newren \n---\n sequencer: fix edit handling for cherry-pick and revert messages\n \n save_opts() should save any non-default values. It was intended to do\n this, but since most options in struct replay_opts default to 0, it only\n saved non-zero values. Unfortunatley, this does not always work for\n options.edit. Roughly speaking, options.edit had a default value of 0\n for cherry-pick but a default value of 1 for revert. Make save_opts()\n record a value whenever it differs from the default.\n \n options.edit was also overly simplistic; we had more than two cases. The\n behavior that previously existed was as follows:\n \n Non-conflict commits Right after Conflict\n revert Edit iff isatty(0) Edit (ignore isatty(0))\n cherry-pick No edit See above\n Specify --edit Edit (ignore isatty(0)) See above\n Specify --no-edit (*) See above\n \n (*) Before stopping for conflicts, No edit is the behavior. After\n stopping for conflicts, the --no-edit flag is not saved so see\n the first two rows.\n \n \n However, the expected behavior is:\n \n Non-conflict commits Right after Conflict\n revert Edit iff isatty(0) Edit iff isatty(0)\n cherry-pick No edit Edit iff isatty(0)\n Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0))\n Specify --no-edit No edit No edit\n \n \n In order to get the expected behavior, we need to change options.edit to\n a tri-state: unspecified, false, or true. When specified, we follow what\n it says. When unspecified, we need to check whether the current commit\n being created is resolving a conflict as well as consulting\n options.action and isatty(0). While at it, add a should_edit() utility\n function that compresses options.edit down to a boolean based on the\n additional information for the non-conflict case.\n \n Make continue_single_pick() (which is the function responsible for\n resuming after conflict cases) stop assuming edit behavior in all cases,\n so that it can correctly handle !isatty(0) and specific requests to not\n edit the commit message.\n \n Reported-by: Renato Botelho garga@freebsd.org Signed-off-by: Elijah\n Newren newren@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-988%2Fnewren%2Ffix-sequencer-no-edit-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-988/newren/fix-sequencer-no-edit-v1\nPull-Request: https://github.com/git/git/pull/988\n\n builtin/revert.c | 4 +--\n sequencer.c | 55 ++++++++++++++++++++++++++-------\n sequencer.h | 6 ++--\n t/t3510-cherry-pick-sequence.sh | 32 ++++++++++++++++++-\n 4 files changed, 80 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 314a86c5621b..81441020231a 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"--signoff\", opts->signoff,\n \t\t\t\t\"--no-commit\", opts->no_commit,\n \t\t\t\t\"-x\", opts->record_origin,\n-\t\t\t\t\"--edit\", opts->edit,\n+\t\t\t\t\"--edit\", opts->edit == 1,\n \t\t\t\tNULL);\n \n \tif (cmd) {\n@@ -230,8 +230,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n \tint res;\n \n-\tif (isatty(0))\n-\t\topts.edit = 1;\n \topts.action = REPLAY_REVERT;\n \tsequencer_init_config(&opts);\n \tres = run_sequencer(argc, argv, &opts);\ndiff --git a/sequencer.c b/sequencer.c\nindex 848204d3dc3f..2fe0e0eff7e6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1860,14 +1860,26 @@ static void record_in_rewritten(struct object_id *oid,\n \t\tflush_rewritten_pending();\n }\n \n+static int should_edit(struct replay_opts *opts) {\n+\tassert(opts->edit >= -1 && opts->edit <= 1);\n+\tif (opts->edit == -1)\n+\t\t/*\n+\t\t * Note the we only handle the case of non-conflicted\n+\t\t * commits; continue_single_pick() handles the conflicted\n+\t\t * commits itself instead of calling this function.\n+\t\t */\n+\t\treturn (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;\n+\treturn opts->edit;\n+}\n+\n static int do_pick_commit(struct repository *r,\n \t\t\t enum todo_command command,\n \t\t\t struct commit *commit,\n \t\t\t struct replay_opts *opts,\n \t\t\t int final_fixup, int *check_todo)\n {\n-\tunsigned int flags = opts->edit ? EDIT_MSG : 0;\n-\tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg(r);\n+\tunsigned int flags = should_edit(opts) ? EDIT_MSG : 0;\n+\tconst char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);\n \tstruct object_id head;\n \tstruct commit *base, *next, *parent;\n \tconst char *base_label, *next_label;\n@@ -3101,9 +3113,9 @@ static int save_opts(struct replay_opts *opts)\n \tif (opts->no_commit)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.no-commit\", \"true\");\n-\tif (opts->edit)\n-\t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.edit\", \"true\");\n+\tif (opts->edit != -1)\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\",\n+\t\t\t\t\t\t opts->edit ? \"true\" : \"false\");\n \tif (opts->allow_empty)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.allow-empty\", \"true\");\n@@ -4077,7 +4089,7 @@ static int pick_commits(struct repository *r,\n \tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n-\t\t\t opts->record_origin || opts->edit ||\n+\t\t\t opts->record_origin || should_edit(opts) ||\n \t\t\t opts->committer_date_is_author_date ||\n \t\t\t opts->ignore_date));\n \tif (read_and_refresh_cache(r, opts))\n@@ -4370,14 +4382,35 @@ static int pick_commits(struct repository *r,\n \treturn sequencer_remove_state(opts);\n }\n \n-static int continue_single_pick(struct repository *r)\n+static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n {\n-\tconst char *argv[] = { \"commit\", NULL };\n+\tstruct strvec argv = STRVEC_INIT;\n+\tint want_edit;\n+\tint ret;\n \n \tif (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\") &&\n \t !refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\"))\n \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n-\treturn run_command_v_opt(argv, RUN_GIT_CMD);\n+\n+\tstrvec_push(&argv, \"commit\");\n+\n+\t/*\n+\t * continue_single_pick() handles the case of recovering from a\n+\t * conflict. should_edit() doesn't handle that case; for a conflict,\n+\t * we want to edit if the user asked for it, or if they didn't specify\n+\t * and stdin is a tty.\n+\t */\n+\twant_edit = (opts->edit == 1) || ((opts->edit == -1) && isatty(0));\n+\tif (!want_edit)\n+\t\t/*\n+\t\t * Include --cleanup=strip as well because we don't want the\n+\t\t * \"# Conflicts:\" messages.\n+\t\t */\n+\t\tstrvec_pushl(&argv, \"--no-edit\", \"--cleanup=strip\", NULL);\n+\n+\tret = run_command_v_opt(argv.v, RUN_GIT_CMD);\n+\tstrvec_clear(&argv);\n+\treturn ret;\n }\n \n static int commit_staged_changes(struct repository *r,\n@@ -4547,7 +4580,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\t\tgoto release_todo_list;\n \t\t}\n \t} else if (!file_exists(get_todo_path(opts)))\n-\t\treturn continue_single_pick(r);\n+\t\treturn continue_single_pick(r, opts);\n \telse if ((res = read_populate_todo(r, &todo_list, opts)))\n \t\tgoto release_todo_list;\n \n@@ -4556,7 +4589,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\tif (refs_ref_exists(get_main_ref_store(r),\n \t\t\t\t \"CHERRY_PICK_HEAD\") ||\n \t\t refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\")) {\n-\t\t\tres = continue_single_pick(r);\n+\t\t\tres = continue_single_pick(r, opts);\n \t\t\tif (res)\n \t\t\t\tgoto release_todo_list;\n \t\t}\ndiff --git a/sequencer.h b/sequencer.h\nindex f8b2e4ab8527..d57d8ea23d7a 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -31,8 +31,10 @@ enum commit_msg_cleanup_mode {\n struct replay_opts {\n \tenum replay_action action;\n \n-\t/* Boolean options */\n+\t/* Tri-state options: unspecified, false, or true */\n \tint edit;\n+\n+\t/* Boolean options */\n \tint record_origin;\n \tint no_commit;\n \tint signoff;\n@@ -71,7 +73,7 @@ struct replay_opts {\n \t/* Only used by REPLAY_NONE */\n \tstruct rev_info *revs;\n };\n-#define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }\n+#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }\n \n /*\n * Note that ordering matters in this enum. Not only must it match the mapping\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex b76cb6de91d0..49010aa9469d 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -65,7 +65,7 @@ test_expect_success 'cherry-pick persists opts correctly' '\n \t# gets interrupted, use a high-enough number that is larger\n \t# than the number of parents of any commit we have created\n \tmainline=4 &&\n-\ttest_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&\n+\ttest_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours --edit initial..anotherpick &&\n \ttest_path_is_dir .git/sequencer &&\n \ttest_path_is_file .git/sequencer/head &&\n \ttest_path_is_file .git/sequencer/todo &&\n@@ -84,6 +84,36 @@ test_expect_success 'cherry-pick persists opts correctly' '\n \tours\n \tEOF\n \tgit config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"true\" >expect &&\n+\tgit config --file=.git/sequencer/opts --get-all options.edit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'revert persists opts correctly' '\n+\tpristine_detach initial &&\n+\t# to make sure that the session to revert a sequence\n+\t# gets interrupted, revert commits that are not in the history\n+\t# of HEAD.\n+\ttest_expect_code 1 git revert -s --strategy=recursive -X patience -X ours --no-edit picked yetanotherpick &&\n+\ttest_path_is_dir .git/sequencer &&\n+\ttest_path_is_file .git/sequencer/head &&\n+\ttest_path_is_file .git/sequencer/todo &&\n+\ttest_path_is_file .git/sequencer/opts &&\n+\techo \"true\" >expect &&\n+\tgit config --file=.git/sequencer/opts --get-all options.signoff >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"recursive\" >expect &&\n+\tgit config --file=.git/sequencer/opts --get-all options.strategy >actual &&\n+\ttest_cmp expect actual &&\n+\tcat >expect <<-\\EOF &&\n+\tpatience\n+\tours\n+\tEOF\n+\tgit config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"false\" >expect &&\n+\tgit config --file=.git/sequencer/opts --get-all options.edit >actual &&\n \ttest_cmp expect actual\n '\n \n\nbase-commit: 98164e9585e02e31dcf1377a553efe076c15f8c6\n-- \ngitgitgadget\n"},{"id":"420259","messageId":"eda509dd-d2d5-0856-0c75-094d67257d54@iee.email","threadId":"55389","inReplyTo":"pull.988.git.git.1616742969145.gitgitgadget@gmail.com","subject":"Re: [PATCH] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2021-03-26T12:27:54Z","receivedAt":"2021-03-26T12:45:56Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"minor nit on a code comment\n\nOn 26/03/2021 07:16, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n>\n> save_opts() should save any non-default values. It was intended to do\n> this, but since most options in struct replay_opts default to 0, it only\n> saved non-zero values. Unfortunatley, this does not always work for\n> options.edit. Roughly speaking, options.edit had a default value of 0\n> for cherry-pick but a default value of 1 for revert. Make save_opts()\n> record a value whenever it differs from the default.\n>\n> options.edit was also overly simplistic; we had more than two cases.\n> The behavior that previously existed was as follows:\n>\n> Non-conflict commits Right after Conflict\n> revert Edit iff isatty(0) Edit (ignore isatty(0))\n> cherry-pick No edit See above\n> Specify --edit Edit (ignore isatty(0)) See above\n> Specify --no-edit (*) See above\n>\n> (*) Before stopping for conflicts, No edit is the behavior. After\n> stopping for conflicts, the --no-edit flag is not saved so see\n> the first two rows.\n>\n> However, the expected behavior is:\n>\n> Non-conflict commits Right after Conflict\n> revert Edit iff isatty(0) Edit iff isatty(0)\n> cherry-pick No edit Edit iff isatty(0)\n> Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0))\n> Specify --no-edit No edit No edit\n>\n> In order to get the expected behavior, we need to change options.edit\n> to a tri-state: unspecified, false, or true. When specified, we follow\n> what it says. When unspecified, we need to check whether the current\n> commit being created is resolving a conflict as well as consulting\n> options.action and isatty(0). While at it, add a should_edit() utility\n> function that compresses options.edit down to a boolean based on the\n> additional information for the non-conflict case.\n>\n> Make continue_single_pick() (which is the function responsible for\n> resuming after conflict cases) stop assuming edit behavior in all cases,\n> so that it can correctly handle !isatty(0) and specific requests to not\n> edit the commit message.\n>\n> Reported-by: Renato Botelho \n> Signed-off-by: Elijah Newren \n> ---\n> sequencer: fix edit handling for cherry-pick and revert messages\n> \n> save_opts() should save any non-default values. It was intended to do\n> this, but since most options in struct replay_opts default to 0, it only\n> saved non-zero values. Unfortunatley, this does not always work for\n> options.edit. Roughly speaking, options.edit had a default value of 0\n> for cherry-pick but a default value of 1 for revert. Make save_opts()\n> record a value whenever it differs from the default.\n> \n> options.edit was also overly simplistic; we had more than two cases. The\n> behavior that previously existed was as follows:\n> \n> Non-conflict commits Right after Conflict\n> revert Edit iff isatty(0) Edit (ignore isatty(0))\n> cherry-pick No edit See above\n> Specify --edit Edit (ignore isatty(0)) See above\n> Specify --no-edit (*) See above\n> \n> (*) Before stopping for conflicts, No edit is the behavior. After\n> stopping for conflicts, the --no-edit flag is not saved so see\n> the first two rows.\n> \n> \n> However, the expected behavior is:\n> \n> Non-conflict commits Right after Conflict\n> revert Edit iff isatty(0) Edit iff isatty(0)\n> cherry-pick No edit Edit iff isatty(0)\n> Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0))\n> Specify --no-edit No edit No edit\n> \n> \n> In order to get the expected behavior, we need to change options.edit to\n> a tri-state: unspecified, false, or true. When specified, we follow what\n> it says. When unspecified, we need to check whether the current commit\n> being created is resolving a conflict as well as consulting\n> options.action and isatty(0). While at it, add a should_edit() utility\n> function that compresses options.edit down to a boolean based on the\n> additional information for the non-conflict case.\n> \n> Make continue_single_pick() (which is the function responsible for\n> resuming after conflict cases) stop assuming edit behavior in all cases,\n> so that it can correctly handle !isatty(0) and specific requests to not\n> edit the commit message.\n> \n> Reported-by: Renato Botelho garga@freebsd.org Signed-off-by: Elijah\n> Newren newren@gmail.com\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-988%2Fnewren%2Ffix-sequencer-no-edit-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-988/newren/fix-sequencer-no-edit-v1\n> Pull-Request: https://github.com/git/git/pull/988\n>\n> builtin/revert.c | 4 +--\n> sequencer.c | 55 ++++++++++++++++++++++++++-------\n> sequencer.h | 6 ++--\n> t/t3510-cherry-pick-sequence.sh | 32 ++++++++++++++++++-\n> 4 files changed, 80 insertions(+), 17 deletions(-)\n>\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 314a86c5621b..81441020231a 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \t\t\t\t\"--signoff\", opts->signoff,\n> \t\t\t\t\"--no-commit\", opts->no_commit,\n> \t\t\t\t\"-x\", opts->record_origin,\n> -\t\t\t\t\"--edit\", opts->edit,\n> +\t\t\t\t\"--edit\", opts->edit == 1,\n> \t\t\t\tNULL);\n> \n> \tif (cmd) {\n> @@ -230,8 +230,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n> \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n> \tint res;\n> \n> -\tif (isatty(0))\n> -\t\topts.edit = 1;\n> \topts.action = REPLAY_REVERT;\n> \tsequencer_init_config(&opts);\n> \tres = run_sequencer(argc, argv, &opts);\n> diff --git a/sequencer.c b/sequencer.c\n> index 848204d3dc3f..2fe0e0eff7e6 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1860,14 +1860,26 @@ static void record_in_rewritten(struct object_id *oid,\n> \t\tflush_rewritten_pending();\n> }\n> \n> +static int should_edit(struct replay_opts *opts) {\n> +\tassert(opts->edit >= -1 && opts->edit <= 1);\n> +\tif (opts->edit == -1)\n> +\t\t/*\n> +\t\t * Note the we only handle the case of non-conflicted\n\nThis 'Note the we' doesn't parse for me.\n--\nPhilip\n> +\t\t * commits; continue_single_pick() handles the conflicted\n> +\t\t * commits itself instead of calling this function.\n> +\t\t */\n> +\t\treturn (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;\n> +\treturn opts->edit;\n> +}\n> +\n> static int do_pick_commit(struct repository *r,\n> \t\t\t enum todo_command command,\n> \t\t\t struct commit *commit,\n> \t\t\t struct replay_opts *opts,\n> \t\t\t int final_fixup, int *check_todo)\n> {\n> -\tunsigned int flags = opts->edit ? EDIT_MSG : 0;\n> -\tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg(r);\n> +\tunsigned int flags = should_edit(opts) ? EDIT_MSG : 0;\n> +\tconst char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);\n> \tstruct object_id head;\n> \tstruct commit *base, *next, *parent;\n> \tconst char *base_label, *next_label;\n> @@ -3101,9 +3113,9 @@ static int save_opts(struct replay_opts *opts)\n> \tif (opts->no_commit)\n> \t\tres |= git_config_set_in_file_gently(opts_file,\n> \t\t\t\t\t\"options.no-commit\", \"true\");\n> -\tif (opts->edit)\n> -\t\tres |= git_config_set_in_file_gently(opts_file,\n> -\t\t\t\t\t\"options.edit\", \"true\");\n> +\tif (opts->edit != -1)\n> +\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\",\n> +\t\t\t\t\t\t opts->edit ? \"true\" : \"false\");\n> \tif (opts->allow_empty)\n> \t\tres |= git_config_set_in_file_gently(opts_file,\n> \t\t\t\t\t\"options.allow-empty\", \"true\");\n> @@ -4077,7 +4089,7 @@ static int pick_commits(struct repository *r,\n> \tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n> \tif (opts->allow_ff)\n> \t\tassert(!(opts->signoff || opts->no_commit ||\n> -\t\t\t opts->record_origin || opts->edit ||\n> +\t\t\t opts->record_origin || should_edit(opts) ||\n> \t\t\t opts->committer_date_is_author_date ||\n> \t\t\t opts->ignore_date));\n> \tif (read_and_refresh_cache(r, opts))\n> @@ -4370,14 +4382,35 @@ static int pick_commits(struct repository *r,\n> \treturn sequencer_remove_state(opts);\n> }\n> \n> -static int continue_single_pick(struct repository *r)\n> +static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n> {\n> -\tconst char *argv[] = { \"commit\", NULL };\n> +\tstruct strvec argv = STRVEC_INIT;\n> +\tint want_edit;\n> +\tint ret;\n> \n> \tif (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\") &&\n> \t !refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\"))\n> \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n> -\treturn run_command_v_opt(argv, RUN_GIT_CMD);\n> +\n> +\tstrvec_push(&argv, \"commit\");\n> +\n> +\t/*\n> +\t * continue_single_pick() handles the case of recovering from a\n> +\t * conflict. should_edit() doesn't handle that case; for a conflict,\n> +\t * we want to edit if the user asked for it, or if they didn't specify\n> +\t * and stdin is a tty.\n> +\t */\n> +\twant_edit = (opts->edit == 1) || ((opts->edit == -1) && isatty(0));\n> +\tif (!want_edit)\n> +\t\t/*\n> +\t\t * Include --cleanup=strip as well because we don't want the\n> +\t\t * \"# Conflicts:\" messages.\n> +\t\t */\n> +\t\tstrvec_pushl(&argv, \"--no-edit\", \"--cleanup=strip\", NULL);\n> +\n> +\tret = run_command_v_opt(argv.v, RUN_GIT_CMD);\n> +\tstrvec_clear(&argv);\n> +\treturn ret;\n> }\n> \n> static int commit_staged_changes(struct repository *r,\n> @@ -4547,7 +4580,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n> \t\t\tgoto release_todo_list;\n> \t\t}\n> \t} else if (!file_exists(get_todo_path(opts)))\n> -\t\treturn continue_single_pick(r);\n> +\t\treturn continue_single_pick(r, opts);\n> \telse if ((res = read_populate_todo(r, &todo_list, opts)))\n> \t\tgoto release_todo_list;\n> \n> @@ -4556,7 +4589,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n> \t\tif (refs_ref_exists(get_main_ref_store(r),\n> \t\t\t\t \"CHERRY_PICK_HEAD\") ||\n> \t\t refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\")) {\n> -\t\t\tres = continue_single_pick(r);\n> +\t\t\tres = continue_single_pick(r, opts);\n> \t\t\tif (res)\n> \t\t\t\tgoto release_todo_list;\n> \t\t}\n> diff --git a/sequencer.h b/sequencer.h\n> index f8b2e4ab8527..d57d8ea23d7a 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -31,8 +31,10 @@ enum commit_msg_cleanup_mode {\n> struct replay_opts {\n> \tenum replay_action action;\n> \n> -\t/* Boolean options */\n> +\t/* Tri-state options: unspecified, false, or true */\n> \tint edit;\n> +\n> +\t/* Boolean options */\n> \tint record_origin;\n> \tint no_commit;\n> \tint signoff;\n> @@ -71,7 +73,7 @@ struct replay_opts {\n> \t/* Only used by REPLAY_NONE */\n> \tstruct rev_info *revs;\n> };\n> -#define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }\n> +#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }\n> \n> /*\n> * Note that ordering matters in this enum. Not only must it match the mapping\n> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\n> index b76cb6de91d0..49010aa9469d 100755\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -65,7 +65,7 @@ test_expect_success 'cherry-pick persists opts correctly' '\n> \t# gets interrupted, use a high-enough number that is larger\n> \t# than the number of parents of any commit we have created\n> \tmainline=4 &&\n> -\ttest_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&\n> +\ttest_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours --edit initial..anotherpick &&\n> \ttest_path_is_dir .git/sequencer &&\n> \ttest_path_is_file .git/sequencer/head &&\n> \ttest_path_is_file .git/sequencer/todo &&\n> @@ -84,6 +84,36 @@ test_expect_success 'cherry-pick persists opts correctly' '\n> \tours\n> \tEOF\n> \tgit config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&\n> +\ttest_cmp expect actual &&\n> +\techo \"true\" >expect &&\n> +\tgit config --file=.git/sequencer/opts --get-all options.edit >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'revert persists opts correctly' '\n> +\tpristine_detach initial &&\n> +\t# to make sure that the session to revert a sequence\n> +\t# gets interrupted, revert commits that are not in the history\n> +\t# of HEAD.\n> +\ttest_expect_code 1 git revert -s --strategy=recursive -X patience -X ours --no-edit picked yetanotherpick &&\n> +\ttest_path_is_dir .git/sequencer &&\n> +\ttest_path_is_file .git/sequencer/head &&\n> +\ttest_path_is_file .git/sequencer/todo &&\n> +\ttest_path_is_file .git/sequencer/opts &&\n> +\techo \"true\" >expect &&\n> +\tgit config --file=.git/sequencer/opts --get-all options.signoff >actual &&\n> +\ttest_cmp expect actual &&\n> +\techo \"recursive\" >expect &&\n> +\tgit config --file=.git/sequencer/opts --get-all options.strategy >actual &&\n> +\ttest_cmp expect actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tpatience\n> +\tours\n> +\tEOF\n> +\tgit config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&\n> +\ttest_cmp expect actual &&\n> +\techo \"false\" >expect &&\n> +\tgit config --file=.git/sequencer/opts --get-all options.edit >actual &&\n> \ttest_cmp expect actual\n> '\n> \n>\n> base-commit: 98164e9585e02e31dcf1377a553efe076c15f8c6\n\n"},{"id":"420263","messageId":"CABPp-BHgQGYLobmYvyj4u=KLePosoCq2Y_JY6aUJOQcP_PB1iw@mail.gmail.com","threadId":"55389","inReplyTo":"eda509dd-d2d5-0856-0c75-094d67257d54@iee.email","subject":"Re: [PATCH] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-26T15:12:37Z","receivedAt":"2021-03-26T15:14:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Fri, Mar 26, 2021 at 5:27 AM Philip Oakley wrote:\n>\n> minor nit on a code comment\n>\n> On 26/03/2021 07:16, Elijah Newren via GitGitGadget wrote:\n...\n> > diff --git a/sequencer.c b/sequencer.c\n> > index 848204d3dc3f..2fe0e0eff7e6 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -1860,14 +1860,26 @@ static void record_in_rewritten(struct object_id *oid,\n> > flush_rewritten_pending();\n> > }\n> >\n> > +static int should_edit(struct replay_opts *opts) {\n> > + assert(opts->edit >= -1 && opts->edit <= 1);\n> > + if (opts->edit == -1)\n> > + /*\n> > + * Note the we only handle the case of non-conflicted\n>\n> This 'Note the we' doesn't parse for me.\n\nOops; should have been \"Note that we\". Thanks for pointing it out;\nI'll fix it up with any other feedback that comes in.\n\n> > + * commits; continue_single_pick() handles the conflicted\n> > + * commits itself instead of calling this function.\n> > + */\n> > + return (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;\n> > + return opts->edit;\n> > +}\n> > +\n"},{"id":"420325","messageId":"xmqqsg4goyim.fsf@gitster.g","threadId":"55389","inReplyTo":"CABPp-BHgQGYLobmYvyj4u=KLePosoCq2Y_JY6aUJOQcP_PB1iw@mail.gmail.com","subject":"Re: [PATCH] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-28T01:30:41Z","receivedAt":"2021-03-28T01:31:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> This 'Note the we' doesn't parse for me.\n>\n> Oops; should have been \"Note that we\". Thanks for pointing it out;\n> I'll fix it up with any other feedback that comes in.\n\nIn the meantime I'll locally tweak while queuing.\n"},{"id":"420437","messageId":"440980f5-81c9-4201-61a4-29acb0346c65@gmail.com","threadId":"55389","inReplyTo":"pull.988.git.git.1616742969145.gitgitgadget@gmail.com","subject":"Re: [PATCH] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-03-29T09:23:08Z","receivedAt":"2021-03-29T09:23:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/03/2021 07:16, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> save_opts() should save any non-default values. It was intended to do\n> this, but since most options in struct replay_opts default to 0, it only\n> saved non-zero values. Unfortunatley,\n\ns/Unfortunatley/Unfortunately/ also s/iff/if/ in a few places below.\n\n> this does not always work for\n> options.edit. Roughly speaking, options.edit had a default value of 0\n> for cherry-pick but a default value of 1 for revert. Make save_opts()\n> record a value whenever it differs from the default.\n> \n> options.edit was also overly simplistic; we had more than two cases.\n> The behavior that previously existed was as follows:\n> \n> Non-conflict commits Right after Conflict\n> revert Edit iff isatty(0) Edit (ignore isatty(0))\n> cherry-pick No edit See above\n> Specify --edit Edit (ignore isatty(0)) See above\n> Specify --no-edit (*) See above\n> \n> (*) Before stopping for conflicts, No edit is the behavior. After\n> stopping for conflicts, the --no-edit flag is not saved so see\n> the first two rows.\n> \n> However, the expected behavior is:\n> \n> Non-conflict commits Right after Conflict\n> revert Edit iff isatty(0) Edit iff isatty(0)\n> cherry-pick No edit Edit iff isatty(0)\n> Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0))\n> Specify --no-edit No edit No edit\n> \n> In order to get the expected behavior, we need to change options.edit\n> to a tri-state: unspecified, false, or true. When specified, we follow\n> what it says. When unspecified, we need to check whether the current\n> commit being created is resolving a conflict as well as consulting\n> options.action and isatty(0). While at it, add a should_edit() utility\n> function that compresses options.edit down to a boolean based on the\n> additional information for the non-conflict case.\n> \n> Make continue_single_pick() (which is the function responsible for\n> resuming after conflict cases) \n\nIt might be worth emphasizing that despite its name \ncontinue_single_pick() is used to commit conflict resolutions regardless \nof the number of picks - I had to check the code to see what it was \ndoing in the multi-pick case.\n\nstop assuming edit behavior in all cases,\n> so that it can correctly handle !isatty(0) and specific requests to not\n> edit the commit message.\n\nThanks for the thorough description.\n\n> Reported-by: Renato Botelho \n> Signed-off-by: Elijah Newren \n> ---\n> sequencer: fix edit handling for cherry-pick and revert messages\n> \n> save_opts() should save any non-default values. It was intended to do\n> this, but since most options in struct replay_opts default to 0, it only\n> saved non-zero values. Unfortunatley, this does not always work for\n> options.edit. Roughly speaking, options.edit had a default value of 0\n> for cherry-pick but a default value of 1 for revert. Make save_opts()\n> record a value whenever it differs from the default.\n> \n> options.edit was also overly simplistic; we had more than two cases. The\n> behavior that previously existed was as follows:\n> \n> Non-conflict commits Right after Conflict\n> revert Edit iff isatty(0) Edit (ignore isatty(0))\n> cherry-pick No edit See above\n> Specify --edit Edit (ignore isatty(0)) See above\n> Specify --no-edit (*) See above\n> \n> (*) Before stopping for conflicts, No edit is the behavior. After\n> stopping for conflicts, the --no-edit flag is not saved so see\n> the first two rows.\n> \n> \n> However, the expected behavior is:\n> \n> Non-conflict commits Right after Conflict\n> revert Edit iff isatty(0) Edit iff isatty(0)\n> cherry-pick No edit Edit iff isatty(0)\n> Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0))\n> Specify --no-edit No edit No edit\n> \n> \n> In order to get the expected behavior, we need to change options.edit to\n> a tri-state: unspecified, false, or true. When specified, we follow what\n> it says. When unspecified, we need to check whether the current commit\n> being created is resolving a conflict as well as consulting\n> options.action and isatty(0). While at it, add a should_edit() utility\n> function that compresses options.edit down to a boolean based on the\n> additional information for the non-conflict case.\n> \n> Make continue_single_pick() (which is the function responsible for\n> resuming after conflict cases) stop assuming edit behavior in all cases,\n> so that it can correctly handle !isatty(0) and specific requests to not\n> edit the commit message.\n> \n> Reported-by: Renato Botelho garga@freebsd.org Signed-off-by: Elijah\n> Newren newren@gmail.com\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-988%2Fnewren%2Ffix-sequencer-no-edit-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-988/newren/fix-sequencer-no-edit-v1\n> Pull-Request: https://github.com/git/git/pull/988\n> \n> builtin/revert.c | 4 +--\n> sequencer.c | 55 ++++++++++++++++++++++++++-------\n> sequencer.h | 6 ++--\n> t/t3510-cherry-pick-sequence.sh | 32 ++++++++++++++++++-\n> 4 files changed, 80 insertions(+), 17 deletions(-)\n> \n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 314a86c5621b..81441020231a 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \t\t\t\t\"--signoff\", opts->signoff,\n> \t\t\t\t\"--no-commit\", opts->no_commit,\n> \t\t\t\t\"-x\", opts->record_origin,\n> -\t\t\t\t\"--edit\", opts->edit,\n> +\t\t\t\t\"--edit\", opts->edit == 1,\n> \t\t\t\tNULL);\n> \n> \tif (cmd) {\n> @@ -230,8 +230,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n> \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n> \tint res;\n> \n> -\tif (isatty(0))\n> -\t\topts.edit = 1;\n> \topts.action = REPLAY_REVERT;\n> \tsequencer_init_config(&opts);\n> \tres = run_sequencer(argc, argv, &opts);\n> diff --git a/sequencer.c b/sequencer.c\n> index 848204d3dc3f..2fe0e0eff7e6 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1860,14 +1860,26 @@ static void record_in_rewritten(struct object_id *oid,\n> \t\tflush_rewritten_pending();\n> }\n> \n> +static int should_edit(struct replay_opts *opts) {\n> +\tassert(opts->edit >= -1 && opts->edit <= 1);\n> +\tif (opts->edit == -1)\n> +\t\t/*\n> +\t\t * Note the we only handle the case of non-conflicted\n> +\t\t * commits; continue_single_pick() handles the conflicted\n> +\t\t * commits itself instead of calling this function.\n> +\t\t */\n> +\t\treturn (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;\n> +\treturn opts->edit;\n> +}\n> +\n> static int do_pick_commit(struct repository *r,\n> \t\t\t enum todo_command command,\n> \t\t\t struct commit *commit,\n> \t\t\t struct replay_opts *opts,\n> \t\t\t int final_fixup, int *check_todo)\n> {\n> -\tunsigned int flags = opts->edit ? EDIT_MSG : 0;\n> -\tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg(r);\n> +\tunsigned int flags = should_edit(opts) ? EDIT_MSG : 0;\n> +\tconst char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);\n> \tstruct object_id head;\n> \tstruct commit *base, *next, *parent;\n> \tconst char *base_label, *next_label;\n> @@ -3101,9 +3113,9 @@ static int save_opts(struct replay_opts *opts)\n> \tif (opts->no_commit)\n> \t\tres |= git_config_set_in_file_gently(opts_file,\n> \t\t\t\t\t\"options.no-commit\", \"true\");\n> -\tif (opts->edit)\n> -\t\tres |= git_config_set_in_file_gently(opts_file,\n> -\t\t\t\t\t\"options.edit\", \"true\");\n> +\tif (opts->edit != -1)\n> +\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\",\n> +\t\t\t\t\t\t opts->edit ? \"true\" : \"false\");\n> \tif (opts->allow_empty)\n> \t\tres |= git_config_set_in_file_gently(opts_file,\n> \t\t\t\t\t\"options.allow-empty\", \"true\");\n> @@ -4077,7 +4089,7 @@ static int pick_commits(struct repository *r,\n> \tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n> \tif (opts->allow_ff)\n> \t\tassert(!(opts->signoff || opts->no_commit ||\n> -\t\t\t opts->record_origin || opts->edit ||\n> +\t\t\t opts->record_origin || should_edit(opts) ||\n> \t\t\t opts->committer_date_is_author_date ||\n> \t\t\t opts->ignore_date));\n> \tif (read_and_refresh_cache(r, opts))\n> @@ -4370,14 +4382,35 @@ static int pick_commits(struct repository *r,\n> \treturn sequencer_remove_state(opts);\n> }\n> \n> -static int continue_single_pick(struct repository *r)\n> +static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n> {\n> -\tconst char *argv[] = { \"commit\", NULL };\n> +\tstruct strvec argv = STRVEC_INIT;\n> +\tint want_edit;\n> +\tint ret;\n> \n> \tif (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\") &&\n> \t !refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\"))\n> \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n> -\treturn run_command_v_opt(argv, RUN_GIT_CMD);\n> +\n> +\tstrvec_push(&argv, \"commit\");\n> +\n> +\t/*\n> +\t * continue_single_pick() handles the case of recovering from a\n> +\t * conflict. should_edit() doesn't handle that case; for a conflict,\n> +\t * we want to edit if the user asked for it, or if they didn't specify\n> +\t * and stdin is a tty.\n> +\t */\n> +\twant_edit = (opts->edit == 1) || ((opts->edit == -1) && isatty(0));\n> +\tif (!want_edit)\n> +\t\t/*\n> +\t\t * Include --cleanup=strip as well because we don't want the\n> +\t\t * \"# Conflicts:\" messages.\n> +\t\t */\n> +\t\tstrvec_pushl(&argv, \"--no-edit\", \"--cleanup=strip\", NULL);\n> +\n> +\tret = run_command_v_opt(argv.v, RUN_GIT_CMD);\n\nI'm not sure why the original was not using run_git_commit(), changing \nit here is probably outside the scope of this patch.\n\n> +\tstrvec_clear(&argv);\n> +\treturn ret;\n> }\n> \n> static int commit_staged_changes(struct repository *r,\n> @@ -4547,7 +4580,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n> \t\t\tgoto release_todo_list;\n> \t\t}\n> \t} else if (!file_exists(get_todo_path(opts)))\n> -\t\treturn continue_single_pick(r);\n> +\t\treturn continue_single_pick(r, opts);\n> \telse if ((res = read_populate_todo(r, &todo_list, opts)))\n> \t\tgoto release_todo_list;\n> \n> @@ -4556,7 +4589,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n> \t\tif (refs_ref_exists(get_main_ref_store(r),\n> \t\t\t\t \"CHERRY_PICK_HEAD\") ||\n> \t\t refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\")) {\n> -\t\t\tres = continue_single_pick(r);\n> +\t\t\tres = continue_single_pick(r, opts);\n> \t\t\tif (res)\n> \t\t\t\tgoto release_todo_list;\n> \t\t}\n> diff --git a/sequencer.h b/sequencer.h\n> index f8b2e4ab8527..d57d8ea23d7a 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -31,8 +31,10 @@ enum commit_msg_cleanup_mode {\n> struct replay_opts {\n> \tenum replay_action action;\n> \n> -\t/* Boolean options */\n> +\t/* Tri-state options: unspecified, false, or true */\n> \tint edit;\n> +\n> +\t/* Boolean options */\n> \tint record_origin;\n> \tint no_commit;\n> \tint signoff;\n> @@ -71,7 +73,7 @@ struct replay_opts {\n> \t/* Only used by REPLAY_NONE */\n> \tstruct rev_info *revs;\n> };\n> -#define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }\n> +#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }\n> \n> /*\n> * Note that ordering matters in this enum. Not only must it match the mapping\n> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\n> index b76cb6de91d0..49010aa9469d 100755\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -65,7 +65,7 @@ test_expect_success 'cherry-pick persists opts correctly' '\n> \t# gets interrupted, use a high-enough number that is larger\n> \t# than the number of parents of any commit we have created\n> \tmainline=4 &&\n> -\ttest_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&\n> +\ttest_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours --edit initial..anotherpick &&\n> \ttest_path_is_dir .git/sequencer &&\n> \ttest_path_is_file .git/sequencer/head &&\n> \ttest_path_is_file .git/sequencer/todo &&\n> @@ -84,6 +84,36 @@ test_expect_success 'cherry-pick persists opts correctly' '\n> \tours\n> \tEOF\n> \tgit config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&\n> +\ttest_cmp expect actual &&\n> +\techo \"true\" >expect &&\n> +\tgit config --file=.git/sequencer/opts --get-all options.edit >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'revert persists opts correctly' '\n> +\tpristine_detach initial &&\n> +\t# to make sure that the session to revert a sequence\n> +\t# gets interrupted, revert commits that are not in the history\n> +\t# of HEAD.\n> +\ttest_expect_code 1 git revert -s --strategy=recursive -X patience -X ours --no-edit picked yetanotherpick &&\n> +\ttest_path_is_dir .git/sequencer &&\n> +\ttest_path_is_file .git/sequencer/head &&\n> +\ttest_path_is_file .git/sequencer/todo &&\n> +\ttest_path_is_file .git/sequencer/opts &&\n> +\techo \"true\" >expect &&\n> +\tgit config --file=.git/sequencer/opts --get-all options.signoff >actual &&\n> +\ttest_cmp expect actual &&\n> +\techo \"recursive\" >expect &&\n> +\tgit config --file=.git/sequencer/opts --get-all options.strategy >actual &&\n> +\ttest_cmp expect actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tpatience\n> +\tours\n> +\tEOF\n> +\tgit config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&\n> +\ttest_cmp expect actual &&\n> +\techo \"false\" >expect &&\n> +\tgit config --file=.git/sequencer/opts --get-all options.edit >actual &&\n> \ttest_cmp expect actual\n> '\n\nThese tests check that the options are saved but do not check what we do \nwith them. It would be good to have a test that checked we actually open \nthe editor when we should to test the new code in \ncontinue_single_pick(), however as that code calls isatty() that may be \ntricky\n\nI was surprised how big a change was required to the existing code but \nit seems this is surprising tricky to get right - I cannot think of any \nsimplifications.\n\nThanks and Best Wishes\n\nPhillip\n\n> \n> \n> base-commit: 98164e9585e02e31dcf1377a553efe076c15f8c6\n> \n\n"},{"id":"420510","messageId":"xmqqwntpiswm.fsf@gitster.g","threadId":"55389","inReplyTo":"440980f5-81c9-4201-61a4-29acb0346c65@gmail.com","subject":"Re: [PATCH] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-29T20:52:57Z","receivedAt":"2021-03-29T20:54:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 26/03/2021 07:16, Elijah Newren via GitGitGadget wrote:\n>> From: Elijah Newren \n>> save_opts() should save any non-default values. It was intended to\n>> do\n>> this, but since most options in struct replay_opts default to 0, it only\n>> saved non-zero values. Unfortunatley,\n>\n> s/Unfortunatley/Unfortunately/ also s/iff/if/ in a few places below.\n\nI think the latter is delibrate use of common abbreviation of \"if\nand only if\".\n\n> It might be worth emphasizing that despite its name\n> continue_single_pick() is used to commit conflict resolutions\n> regardless of the number of picks - I had to check the code to see\n> what it was doing in the multi-pick case.\n\nmetoo ;-)\n\n> I was surprised how big a change was required to the existing code but\n> it seems this is surprising tricky to get right - I cannot think of\n> any simplifications.\n\nThanks for a review.\n"},{"id":"420516","messageId":"CABPp-BF5fWanFcgOc1Dq0uT8cYCFjiaSO6i4PF9a4Xpg3vEf7Q@mail.gmail.com","threadId":"55389","inReplyTo":"440980f5-81c9-4201-61a4-29acb0346c65@gmail.com","subject":"Re: [PATCH] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-29T21:25:02Z","receivedAt":"2021-03-29T21:25:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Mar 29, 2021 at 2:23 AM Phillip Wood wrote:\n>\n> On 26/03/2021 07:16, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > save_opts() should save any non-default values. It was intended to do\n> > this, but since most options in struct replay_opts default to 0, it only\n> > saved non-zero values. Unfortunatley,\n>\n> s/Unfortunatley/Unfortunately/\n\nThanks, will fix.\n\n> also s/iff/if/ in a few places below.\n\nI want behavior for both isatty(0) and !isatty(0) to be specified by\nthe table, so iff is correct; see\nhttps://www.lexico.com/definition/iff\n\n[...]\n> > Make continue_single_pick() (which is the function responsible for\n> > resuming after conflict cases)\n>\n> It might be worth emphasizing that despite its name\n> continue_single_pick() is used to commit conflict resolutions regardless\n> of the number of picks - I had to check the code to see what it was\n> doing in the multi-pick case.\n\nI'll edit the description to make this more clear.\n\n[...]\n> These tests check that the options are saved but do not check what we do\n> with them. It would be good to have a test that checked we actually open\n> the editor when we should to test the new code in\n> continue_single_pick(), however as that code calls isatty() that may be\n> tricky\n>\n>\n> I was surprised how big a change was required to the existing code but\n> it seems this is surprising tricky to get right - I cannot think of any\n> simplifications.\n\nThanks for taking a look. :-)\n"},{"id":"420552","messageId":"pull.988.v2.git.git.1617070174458.gitgitgadget@gmail.com","threadId":"55389","inReplyTo":"pull.988.git.git.1616742969145.gitgitgadget@gmail.com","subject":"[PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-30T02:09:33Z","receivedAt":"2021-03-30T02:10:23Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nsave_opts() should save any non-default values. It was intended to do\nthis, but since most options in struct replay_opts default to 0, it only\nsaved non-zero values. Unfortunately, this does not always work for\noptions.edit. Roughly speaking, options.edit had a default value of 0\nfor cherry-pick but a default value of 1 for revert. Make save_opts()\nrecord a value whenever it differs from the default.\n\noptions.edit was also overly simplistic; we had more than two cases.\nThe behavior that previously existed was as follows:\n\n Non-conflict commits Right after Conflict\n revert Edit iff isatty(0) Edit (ignore isatty(0))\n cherry-pick No edit See above\n Specify --edit Edit (ignore isatty(0)) See above\n Specify --no-edit (*) See above\n\n (*) Before stopping for conflicts, No edit is the behavior. After\n stopping for conflicts, the --no-edit flag is not saved so see\n the first two rows.\n\nHowever, the expected behavior is:\n\n Non-conflict commits Right after Conflict\n revert Edit iff isatty(0) Edit iff isatty(0)\n cherry-pick No edit Edit iff isatty(0)\n Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0))\n Specify --no-edit No edit No edit\n\nIn order to get the expected behavior, we need to change options.edit\nto a tri-state: unspecified, false, or true. When specified, we follow\nwhat it says. When unspecified, we need to check whether the current\ncommit being created is resolving a conflict as well as consulting\noptions.action and isatty(0). While at it, add a should_edit() utility\nfunction that compresses options.edit down to a boolean based on the\nadditional information for the non-conflict case.\n\ncontinue_single_pick() is the function responsible for resuming after\nconflict cases, regardless of whether there is one commit being picked\nor many. Make this function stop assuming edit behavior in all cases,\nso that it can correctly handle !isatty(0) and specific requests to not\nedit the commit message.\n\nReported-by: Renato Botelho \nSigned-off-by: Elijah Newren \n---\n sequencer: fix edit handling for cherry-pick and revert messages\n \n save_opts() should save any non-default values. It was intended to do\n this, but since most options in struct replay_opts default to 0, it only\n saved non-zero values...\n \n Changes since v1:\n \n * fixed a few typos\n * Clarified that continue_single_pick() is used for resuming from\n conflict regardless of the number of commits being picked.\n \n Reported-by: Renato Botelho garga@freebsd.org Signed-off-by: Elijah\n Newren newren@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-988%2Fnewren%2Ffix-sequencer-no-edit-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-988/newren/fix-sequencer-no-edit-v2\nPull-Request: https://github.com/git/git/pull/988\n\nRange-diff vs v1:\n\n 1: 30cdc3ce215e ! 1: 376799bd3d99 sequencer: fix edit handling for cherry-pick and revert messages\n @@ Commit message\n \n save_opts() should save any non-default values. It was intended to do\n this, but since most options in struct replay_opts default to 0, it only\n - saved non-zero values. Unfortunatley, this does not always work for\n + saved non-zero values. Unfortunately, this does not always work for\n options.edit. Roughly speaking, options.edit had a default value of 0\n for cherry-pick but a default value of 1 for revert. Make save_opts()\n record a value whenever it differs from the default.\n @@ Commit message\n function that compresses options.edit down to a boolean based on the\n additional information for the non-conflict case.\n \n - Make continue_single_pick() (which is the function responsible for\n - resuming after conflict cases) stop assuming edit behavior in all cases,\n + continue_single_pick() is the function responsible for resuming after\n + conflict cases, regardless of whether there is one commit being picked\n + or many. Make this function stop assuming edit behavior in all cases,\n so that it can correctly handle !isatty(0) and specific requests to not\n edit the commit message.\n \n @@ sequencer.c: static void record_in_rewritten(struct object_id *oid,\n +\tassert(opts->edit >= -1 && opts->edit <= 1);\n +\tif (opts->edit == -1)\n +\t\t/*\n -+\t\t * Note the we only handle the case of non-conflicted\n ++\t\t * Note that we only handle the case of non-conflicted\n +\t\t * commits; continue_single_pick() handles the conflicted\n +\t\t * commits itself instead of calling this function.\n +\t\t */\n\n\n builtin/revert.c | 4 +--\n sequencer.c | 55 ++++++++++++++++++++++++++-------\n sequencer.h | 6 ++--\n t/t3510-cherry-pick-sequence.sh | 32 ++++++++++++++++++-\n 4 files changed, 80 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 314a86c5621b..81441020231a 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"--signoff\", opts->signoff,\n \t\t\t\t\"--no-commit\", opts->no_commit,\n \t\t\t\t\"-x\", opts->record_origin,\n-\t\t\t\t\"--edit\", opts->edit,\n+\t\t\t\t\"--edit\", opts->edit == 1,\n \t\t\t\tNULL);\n \n \tif (cmd) {\n@@ -230,8 +230,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n \tint res;\n \n-\tif (isatty(0))\n-\t\topts.edit = 1;\n \topts.action = REPLAY_REVERT;\n \tsequencer_init_config(&opts);\n \tres = run_sequencer(argc, argv, &opts);\ndiff --git a/sequencer.c b/sequencer.c\nindex 848204d3dc3f..d444c778a097 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1860,14 +1860,26 @@ static void record_in_rewritten(struct object_id *oid,\n \t\tflush_rewritten_pending();\n }\n \n+static int should_edit(struct replay_opts *opts) {\n+\tassert(opts->edit >= -1 && opts->edit <= 1);\n+\tif (opts->edit == -1)\n+\t\t/*\n+\t\t * Note that we only handle the case of non-conflicted\n+\t\t * commits; continue_single_pick() handles the conflicted\n+\t\t * commits itself instead of calling this function.\n+\t\t */\n+\t\treturn (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;\n+\treturn opts->edit;\n+}\n+\n static int do_pick_commit(struct repository *r,\n \t\t\t enum todo_command command,\n \t\t\t struct commit *commit,\n \t\t\t struct replay_opts *opts,\n \t\t\t int final_fixup, int *check_todo)\n {\n-\tunsigned int flags = opts->edit ? EDIT_MSG : 0;\n-\tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg(r);\n+\tunsigned int flags = should_edit(opts) ? EDIT_MSG : 0;\n+\tconst char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);\n \tstruct object_id head;\n \tstruct commit *base, *next, *parent;\n \tconst char *base_label, *next_label;\n@@ -3101,9 +3113,9 @@ static int save_opts(struct replay_opts *opts)\n \tif (opts->no_commit)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.no-commit\", \"true\");\n-\tif (opts->edit)\n-\t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.edit\", \"true\");\n+\tif (opts->edit != -1)\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\",\n+\t\t\t\t\t\t opts->edit ? \"true\" : \"false\");\n \tif (opts->allow_empty)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.allow-empty\", \"true\");\n@@ -4077,7 +4089,7 @@ static int pick_commits(struct repository *r,\n \tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n-\t\t\t opts->record_origin || opts->edit ||\n+\t\t\t opts->record_origin || should_edit(opts) ||\n \t\t\t opts->committer_date_is_author_date ||\n \t\t\t opts->ignore_date));\n \tif (read_and_refresh_cache(r, opts))\n@@ -4370,14 +4382,35 @@ static int pick_commits(struct repository *r,\n \treturn sequencer_remove_state(opts);\n }\n \n-static int continue_single_pick(struct repository *r)\n+static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n {\n-\tconst char *argv[] = { \"commit\", NULL };\n+\tstruct strvec argv = STRVEC_INIT;\n+\tint want_edit;\n+\tint ret;\n \n \tif (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\") &&\n \t !refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\"))\n \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n-\treturn run_command_v_opt(argv, RUN_GIT_CMD);\n+\n+\tstrvec_push(&argv, \"commit\");\n+\n+\t/*\n+\t * continue_single_pick() handles the case of recovering from a\n+\t * conflict. should_edit() doesn't handle that case; for a conflict,\n+\t * we want to edit if the user asked for it, or if they didn't specify\n+\t * and stdin is a tty.\n+\t */\n+\twant_edit = (opts->edit == 1) || ((opts->edit == -1) && isatty(0));\n+\tif (!want_edit)\n+\t\t/*\n+\t\t * Include --cleanup=strip as well because we don't want the\n+\t\t * \"# Conflicts:\" messages.\n+\t\t */\n+\t\tstrvec_pushl(&argv, \"--no-edit\", \"--cleanup=strip\", NULL);\n+\n+\tret = run_command_v_opt(argv.v, RUN_GIT_CMD);\n+\tstrvec_clear(&argv);\n+\treturn ret;\n }\n \n static int commit_staged_changes(struct repository *r,\n@@ -4547,7 +4580,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\t\tgoto release_todo_list;\n \t\t}\n \t} else if (!file_exists(get_todo_path(opts)))\n-\t\treturn continue_single_pick(r);\n+\t\treturn continue_single_pick(r, opts);\n \telse if ((res = read_populate_todo(r, &todo_list, opts)))\n \t\tgoto release_todo_list;\n \n@@ -4556,7 +4589,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\tif (refs_ref_exists(get_main_ref_store(r),\n \t\t\t\t \"CHERRY_PICK_HEAD\") ||\n \t\t refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\")) {\n-\t\t\tres = continue_single_pick(r);\n+\t\t\tres = continue_single_pick(r, opts);\n \t\t\tif (res)\n \t\t\t\tgoto release_todo_list;\n \t\t}\ndiff --git a/sequencer.h b/sequencer.h\nindex f8b2e4ab8527..d57d8ea23d7a 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -31,8 +31,10 @@ enum commit_msg_cleanup_mode {\n struct replay_opts {\n \tenum replay_action action;\n \n-\t/* Boolean options */\n+\t/* Tri-state options: unspecified, false, or true */\n \tint edit;\n+\n+\t/* Boolean options */\n \tint record_origin;\n \tint no_commit;\n \tint signoff;\n@@ -71,7 +73,7 @@ struct replay_opts {\n \t/* Only used by REPLAY_NONE */\n \tstruct rev_info *revs;\n };\n-#define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }\n+#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }\n \n /*\n * Note that ordering matters in this enum. Not only must it match the mapping\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex b76cb6de91d0..49010aa9469d 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -65,7 +65,7 @@ test_expect_success 'cherry-pick persists opts correctly' '\n \t# gets interrupted, use a high-enough number that is larger\n \t# than the number of parents of any commit we have created\n \tmainline=4 &&\n-\ttest_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&\n+\ttest_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours --edit initial..anotherpick &&\n \ttest_path_is_dir .git/sequencer &&\n \ttest_path_is_file .git/sequencer/head &&\n \ttest_path_is_file .git/sequencer/todo &&\n@@ -84,6 +84,36 @@ test_expect_success 'cherry-pick persists opts correctly' '\n \tours\n \tEOF\n \tgit config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"true\" >expect &&\n+\tgit config --file=.git/sequencer/opts --get-all options.edit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'revert persists opts correctly' '\n+\tpristine_detach initial &&\n+\t# to make sure that the session to revert a sequence\n+\t# gets interrupted, revert commits that are not in the history\n+\t# of HEAD.\n+\ttest_expect_code 1 git revert -s --strategy=recursive -X patience -X ours --no-edit picked yetanotherpick &&\n+\ttest_path_is_dir .git/sequencer &&\n+\ttest_path_is_file .git/sequencer/head &&\n+\ttest_path_is_file .git/sequencer/todo &&\n+\ttest_path_is_file .git/sequencer/opts &&\n+\techo \"true\" >expect &&\n+\tgit config --file=.git/sequencer/opts --get-all options.signoff >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"recursive\" >expect &&\n+\tgit config --file=.git/sequencer/opts --get-all options.strategy >actual &&\n+\ttest_cmp expect actual &&\n+\tcat >expect <<-\\EOF &&\n+\tpatience\n+\tours\n+\tEOF\n+\tgit config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"false\" >expect &&\n+\tgit config --file=.git/sequencer/opts --get-all options.edit >actual &&\n \ttest_cmp expect actual\n '\n \n\nbase-commit: 98164e9585e02e31dcf1377a553efe076c15f8c6\n-- \ngitgitgadget\n"},{"id":"420568","messageId":"nycvar.QRO.7.76.6.2103301200020.52@tvgsbejvaqbjf.bet","threadId":"55389","inReplyTo":"pull.988.v2.git.git.1617070174458.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-03-30T10:13:33Z","receivedAt":"2021-03-30T10:14:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Tue, 30 Mar 2021, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> save_opts() should save any non-default values. It was intended to do\n> this, but since most options in struct replay_opts default to 0, it only\n> saved non-zero values. Unfortunately, this does not always work for\n> options.edit. Roughly speaking, options.edit had a default value of 0\n> for cherry-pick but a default value of 1 for revert. Make save_opts()\n> record a value whenever it differs from the default.\n>\n> options.edit was also overly simplistic; we had more than two cases.\n> The behavior that previously existed was as follows:\n>\n> Non-conflict commits Right after Conflict\n> revert Edit iff isatty(0) Edit (ignore isatty(0))\n> cherry-pick No edit See above\n> Specify --edit Edit (ignore isatty(0)) See above\n> Specify --no-edit (*) See above\n>\n> (*) Before stopping for conflicts, No edit is the behavior. After\n> stopping for conflicts, the --no-edit flag is not saved so see\n> the first two rows.\n>\n> However, the expected behavior is:\n>\n> Non-conflict commits Right after Conflict\n> revert Edit iff isatty(0) Edit iff isatty(0)\n> cherry-pick No edit Edit iff isatty(0)\n> Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0))\n> Specify --no-edit No edit No edit\n>\n> In order to get the expected behavior, we need to change options.edit\n> to a tri-state: unspecified, false, or true. When specified, we follow\n> what it says. When unspecified, we need to check whether the current\n> commit being created is resolving a conflict as well as consulting\n> options.action and isatty(0). While at it, add a should_edit() utility\n> function that compresses options.edit down to a boolean based on the\n> additional information for the non-conflict case.\n>\n> continue_single_pick() is the function responsible for resuming after\n> conflict cases, regardless of whether there is one commit being picked\n> or many. Make this function stop assuming edit behavior in all cases,\n> so that it can correctly handle !isatty(0) and specific requests to not\n> edit the commit message.\n\nNicely explained!\n\nI'll allow myself one tangent: the subject of the sequencer's Unix shell\nscript heritage seems to come up with an increasing frequency, in\nparticular the awful \"let's write out one file per setting\" strategy.\n\nI would _love_ for `save_opts()` to write a JSON instead (or an INI via\nthe `git_config_*()` family of functions, as is done already by the\ncherry-pick/revert stuff), now that we no longer have any shell script\nbackend (apart from `--preserve-merges`, but that one is on its way out\nanyway).\n\nThe one thing that concerns me with this idea is that I know for a fact\nthat some enterprisey users play games with those files inside\n`/rebase-merge` that should be considered internal implementation\ndetails. Not sure how to deprecate that properly, I don't think we have a\nsane way to detect whether users rely on these implementation details\nother than breaking their expectations, which is not really a gentle way\nto ask them to update their scripts.\n\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 314a86c5621b..81441020231a 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \t\t\t\t\"--signoff\", opts->signoff,\n> \t\t\t\t\"--no-commit\", opts->no_commit,\n> \t\t\t\t\"-x\", opts->record_origin,\n> -\t\t\t\t\"--edit\", opts->edit,\n> +\t\t\t\t\"--edit\", opts->edit == 1,\n\nHonestly, I'd prefer `> 0` here.\n\n> \t\t\t\tNULL);\n>\n> \tif (cmd) {\n> @@ -230,8 +230,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n> \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n> \tint res;\n>\n> -\tif (isatty(0))\n> -\t\topts.edit = 1;\n> \topts.action = REPLAY_REVERT;\n> \tsequencer_init_config(&opts);\n> \tres = run_sequencer(argc, argv, &opts);\n> diff --git a/sequencer.c b/sequencer.c\n> index 848204d3dc3f..d444c778a097 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1860,14 +1860,26 @@ static void record_in_rewritten(struct object_id *oid,\n> \t\tflush_rewritten_pending();\n> }\n>\n> +static int should_edit(struct replay_opts *opts) {\n> +\tassert(opts->edit >= -1 && opts->edit <= 1);\n\nDo we really want to introduce more of these useless `assert()`s? I know\nthat we stopped converting them to `BUG()`, but I really dislike\nintroducing new ones: they have very little effect, being no-ops by\ndefault in most setups.\n\n> +\tif (opts->edit == -1)\n\nMaybe `< 0`, as we do elsewhere for \"not specified\"?\n\n> +\t\t/*\n> +\t\t * Note that we only handle the case of non-conflicted\n> +\t\t * commits; continue_single_pick() handles the conflicted\n> +\t\t * commits itself instead of calling this function.\n> +\t\t */\n> +\t\treturn (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;\n\nApart from the extra parentheses, that makes sense to me.\n\n> +\treturn opts->edit;\n> +}\n> +\n> static int do_pick_commit(struct repository *r,\n> \t\t\t enum todo_command command,\n> \t\t\t struct commit *commit,\n> \t\t\t struct replay_opts *opts,\n> \t\t\t int final_fixup, int *check_todo)\n> {\n> -\tunsigned int flags = opts->edit ? EDIT_MSG : 0;\n> -\tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg(r);\n> +\tunsigned int flags = should_edit(opts) ? EDIT_MSG : 0;\n> +\tconst char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);\n> \tstruct object_id head;\n> \tstruct commit *base, *next, *parent;\n> \tconst char *base_label, *next_label;\n> @@ -3101,9 +3113,9 @@ static int save_opts(struct replay_opts *opts)\n> \tif (opts->no_commit)\n> \t\tres |= git_config_set_in_file_gently(opts_file,\n> \t\t\t\t\t\"options.no-commit\", \"true\");\n> -\tif (opts->edit)\n> -\t\tres |= git_config_set_in_file_gently(opts_file,\n> -\t\t\t\t\t\"options.edit\", \"true\");\n> +\tif (opts->edit != -1)\n\ns/!= -1/>= 0/\n\n> +\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\",\n> +\t\t\t\t\t\t opts->edit ? \"true\" : \"false\");\n> \tif (opts->allow_empty)\n> \t\tres |= git_config_set_in_file_gently(opts_file,\n> \t\t\t\t\t\"options.allow-empty\", \"true\");\n> @@ -4077,7 +4089,7 @@ static int pick_commits(struct repository *r,\n> \tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n> \tif (opts->allow_ff)\n> \t\tassert(!(opts->signoff || opts->no_commit ||\n> -\t\t\t opts->record_origin || opts->edit ||\n> +\t\t\t opts->record_origin || should_edit(opts) ||\n> \t\t\t opts->committer_date_is_author_date ||\n> \t\t\t opts->ignore_date));\n> \tif (read_and_refresh_cache(r, opts))\n> @@ -4370,14 +4382,35 @@ static int pick_commits(struct repository *r,\n> \treturn sequencer_remove_state(opts);\n> }\n>\n> -static int continue_single_pick(struct repository *r)\n> +static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n> {\n> -\tconst char *argv[] = { \"commit\", NULL };\n> +\tstruct strvec argv = STRVEC_INIT;\n> +\tint want_edit;\n\nDo we really want that extra `want_edit` variable? I think the code would\nbe easier to read without it, and still be obvious enough.\n\n> +\tint ret;\n>\n> \tif (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\") &&\n> \t !refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\"))\n> \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n> -\treturn run_command_v_opt(argv, RUN_GIT_CMD);\n> +\n> +\tstrvec_push(&argv, \"commit\");\n> +\n> +\t/*\n> +\t * continue_single_pick() handles the case of recovering from a\n> +\t * conflict. should_edit() doesn't handle that case; for a conflict,\n> +\t * we want to edit if the user asked for it, or if they didn't specify\n> +\t * and stdin is a tty.\n> +\t */\n> +\twant_edit = (opts->edit == 1) || ((opts->edit == -1) && isatty(0));\n> +\tif (!want_edit)\n\nHere is what I would prefer:\n\n\tif (!opts->edit || (opts->edit < 0 && !isatty(0)))\n\nThe rest looks good, and the comments are _really_ helpful.\n\nAnd the remainder of the patch also looks good, so I will spare readers\ntime by not even quoting it.\n\nThank you!\nDscho\n"},{"id":"420637","messageId":"xmqq7dloeawf.fsf@gitster.g","threadId":"55389","inReplyTo":"nycvar.QRO.7.76.6.2103301200020.52@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-30T18:47:44Z","receivedAt":"2021-03-30T18:48:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> @@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n>> \t\t\t\t\"--signoff\", opts->signoff,\n>> \t\t\t\t\"--no-commit\", opts->no_commit,\n>> \t\t\t\t\"-x\", opts->record_origin,\n>> -\t\t\t\t\"--edit\", opts->edit,\n>> +\t\t\t\t\"--edit\", opts->edit == 1,\n>\n> Honestly, I'd prefer `> 0` here.\n\nUnless somebody (including Elijah) is trying to soon introduce yet\nanother value to .edit member, I'd agree 100%. If it is a tristate\n(unspecified, no, yes), I think \"is it positive\" should be the way\nto ask \"does the user definitely wants it?\", \"is it zero\" should be\nthe way to ask \"does the user definitely declines it?\" and \"is it\nnon-negative\" (and \"is it negative\") the way to ask \"does the user\ncare (or not care)?\". Using that consistently is good.\n\n>> +static int should_edit(struct replay_opts *opts) {\n>> +\tassert(opts->edit >= -1 && opts->edit <= 1);\n>\n> Do we really want to introduce more of these useless `assert()`s? I know\n> that we stopped converting them to `BUG()`, but I really dislike\n> introducing new ones: they have very little effect, being no-ops by\n> default in most setups.\n\nYeah, in a new code in flux where programmers can easily make\nerrors, \"if (...) BUG()\" may not be a bad thing to add (but then we\nmay want to see if we can make the codepaths involved less error\nprone), but I agree with your view that assert() is mostly useless.\nA comment that explains the expectation and why that expectation is\nthere would be more useful.\n\n\n>> +\tif (opts->edit == -1)\n>\n> Maybe `< 0`, as we do elsewhere for \"not specified\"?\n\nYup.\n\n>> +\t\t/*\n>> +\t\t * Note that we only handle the case of non-conflicted\n>> +\t\t * commits; continue_single_pick() handles the conflicted\n>> +\t\t * commits itself instead of calling this function.\n>> +\t\t */\n>> +\t\treturn (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;\n>\n> Apart from the extra parentheses, that makes sense to me.\n\nI can take it either way (but personally I think this particular one\nis easier to see as written---this is subjective).\n\n> ...\n> The rest looks good, and the comments are _really_ helpful.\n\nYup, I agree.\n\nThanks for a review.\n"},{"id":"420645","messageId":"CABPp-BGwAtpsQJ8U5N1q21PMkideptY2MB2PNgbPqvya+XuyHg@mail.gmail.com","threadId":"55389","inReplyTo":"nycvar.QRO.7.76.6.2103301200020.52@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-30T19:37:47Z","receivedAt":"2021-03-30T19:38:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Mar 30, 2021 at 3:13 AM Johannes Schindelin\n wrote:\n>\n> Hi Elijah,\n>\n> On Tue, 30 Mar 2021, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren \n> >\n> > save_opts() should save any non-default values. It was intended to do\n> > this, but since most options in struct replay_opts default to 0, it only\n> > saved non-zero values. Unfortunately, this does not always work for\n> > options.edit. Roughly speaking, options.edit had a default value of 0\n> > for cherry-pick but a default value of 1 for revert. Make save_opts()\n> > record a value whenever it differs from the default.\n> >\n> > options.edit was also overly simplistic; we had more than two cases.\n> > The behavior that previously existed was as follows:\n> >\n> > Non-conflict commits Right after Conflict\n> > revert Edit iff isatty(0) Edit (ignore isatty(0))\n> > cherry-pick No edit See above\n> > Specify --edit Edit (ignore isatty(0)) See above\n> > Specify --no-edit (*) See above\n> >\n> > (*) Before stopping for conflicts, No edit is the behavior. After\n> > stopping for conflicts, the --no-edit flag is not saved so see\n> > the first two rows.\n> >\n> > However, the expected behavior is:\n> >\n> > Non-conflict commits Right after Conflict\n> > revert Edit iff isatty(0) Edit iff isatty(0)\n> > cherry-pick No edit Edit iff isatty(0)\n> > Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0))\n> > Specify --no-edit No edit No edit\n> >\n> > In order to get the expected behavior, we need to change options.edit\n> > to a tri-state: unspecified, false, or true. When specified, we follow\n> > what it says. When unspecified, we need to check whether the current\n> > commit being created is resolving a conflict as well as consulting\n> > options.action and isatty(0). While at it, add a should_edit() utility\n> > function that compresses options.edit down to a boolean based on the\n> > additional information for the non-conflict case.\n> >\n> > continue_single_pick() is the function responsible for resuming after\n> > conflict cases, regardless of whether there is one commit being picked\n> > or many. Make this function stop assuming edit behavior in all cases,\n> > so that it can correctly handle !isatty(0) and specific requests to not\n> > edit the commit message.\n>\n> Nicely explained!\n>\n> I'll allow myself one tangent: the subject of the sequencer's Unix shell\n> script heritage seems to come up with an increasing frequency, in\n> particular the awful \"let's write out one file per setting\" strategy.\n>\n> I would _love_ for `save_opts()` to write a JSON instead (or an INI via\n> the `git_config_*()` family of functions, as is done already by the\n> cherry-pick/revert stuff), now that we no longer have any shell script\n> backend (apart from `--preserve-merges`, but that one is on its way out\n> anyway).\n>\n> The one thing that concerns me with this idea is that I know for a fact\n> that some enterprisey users play games with those files inside\n> `/rebase-merge` that should be considered internal implementation\n> details. Not sure how to deprecate that properly, I don't think we have a\n> sane way to detect whether users rely on these implementation details\n> other than breaking their expectations, which is not really a gentle way\n> to ask them to update their scripts.\n\nOoh, I'm glad you took this tangent. May I follow it for a second?\nI'd really, really like this too, for three reasons:\n\n1) I constantly get confused about the massive duplication and\ndifference in control structures and which ones affect which type of\noperations in sequencer.c. Having them both use an ini-file would\nallow us to remove lots of that duplication. I'm sure there'll still\nbe some rebase-specific or cherry-pick-specific options, but we don't\nneed a separate parallel structure for every piece of config.\n\n2) In my merge-ort optimization work, rebasing 35 commits in linux.git\nacross a massive set of 26K upstream renames has dropped rename\ndetection time from the top spot. And it also dropped several other\nthings in the merge machinery from their spots as well. Do you know\nwhat's the slowest now? Wasted time from sequencer.c: the unnecessary\nprocess forking and all the useless disk writing (which I suspect is\nmostly updating the index and working directory but also writing all\nthe individual control files under .git/rebase-merge/). And this\nstuff from sequencer.c is not just barely the slowest part, it's the\nslowest by a wide margin.\n\n3) I also want to allow cherry-picking or rebasing branches that\naren't even checked out (assuming no conflicts are triggered;\notherwise an error can be shown with the user asked to repeat with the\noperation connected to an active working directory). For such an\noperation, the difference between \"cherry-pick\" and \"rebase\" is nearly\nirrelevant so you'd expect the code to be the same; every time I look\nat the code, though, it seems that the control structures are\nseparating these two types of operations in more areas than just the\nreading and writing of the config.\n\n> > diff --git a/builtin/revert.c b/builtin/revert.c\n> > index 314a86c5621b..81441020231a 100644\n> > --- a/builtin/revert.c\n> > +++ b/builtin/revert.c\n> > @@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> > \"--signoff\", opts->signoff,\n> > \"--no-commit\", opts->no_commit,\n> > \"-x\", opts->record_origin,\n> > - \"--edit\", opts->edit,\n> > + \"--edit\", opts->edit == 1,\n>\n> Honestly, I'd prefer `> 0` here.\n\nWFM; I'll change it.\n\n>\n> > NULL);\n> >\n> > if (cmd) {\n> > @@ -230,8 +230,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n> > struct replay_opts opts = REPLAY_OPTS_INIT;\n> > int res;\n> >\n> > - if (isatty(0))\n> > - opts.edit = 1;\n> > opts.action = REPLAY_REVERT;\n> > sequencer_init_config(&opts);\n> > res = run_sequencer(argc, argv, &opts);\n> > diff --git a/sequencer.c b/sequencer.c\n> > index 848204d3dc3f..d444c778a097 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -1860,14 +1860,26 @@ static void record_in_rewritten(struct object_id *oid,\n> > flush_rewritten_pending();\n> > }\n> >\n> > +static int should_edit(struct replay_opts *opts) {\n> > + assert(opts->edit >= -1 && opts->edit <= 1);\n>\n> Do we really want to introduce more of these useless `assert()`s? I know\n> that we stopped converting them to `BUG()`, but I really dislike\n> introducing new ones: they have very little effect, being no-ops by\n> default in most setups.\n\nWhat do you mean by \"useless\"? They serve two purposes:\n * It's a very concise comment understood by readers of the code\n * Many asserts are helpful guardrails for future people attempting\nto change the code and assumptions they were written under\n\nNeither of these have anything to do with users running code in\nproduction -- if that mattered, I would have used BUG(). Since\nproduction use isn't relevant here, I could use code comments, but\nthose tend to be much more verbose, and less helpful in catching areas\nwhere assumptions were important to the code for any future folks\n(possibly myself in a few years) who want to change the code in ways\nthat might call into question previous assumptions.\n\n> > + if (opts->edit == -1)\n>\n> Maybe `< 0`, as we do elsewhere for \"not specified\"?\n\nMakes sense; will change.\n\n> > + /*\n> > + * Note that we only handle the case of non-conflicted\n> > + * commits; continue_single_pick() handles the conflicted\n> > + * commits itself instead of calling this function.\n> > + */\n> > + return (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;\n>\n> Apart from the extra parentheses, that makes sense to me.\n\nThe extra parentheses make it more readable to me; since the statement\nstill makes sense to you, I'll leave them in. :-)\n\n> > + return opts->edit;\n> > +}\n> > +\n> > static int do_pick_commit(struct repository *r,\n> > enum todo_command command,\n> > struct commit *commit,\n> > struct replay_opts *opts,\n> > int final_fixup, int *check_todo)\n> > {\n> > - unsigned int flags = opts->edit ? EDIT_MSG : 0;\n> > - const char *msg_file = opts->edit ? NULL : git_path_merge_msg(r);\n> > + unsigned int flags = should_edit(opts) ? EDIT_MSG : 0;\n> > + const char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);\n> > struct object_id head;\n> > struct commit *base, *next, *parent;\n> > const char *base_label, *next_label;\n> > @@ -3101,9 +3113,9 @@ static int save_opts(struct replay_opts *opts)\n> > if (opts->no_commit)\n> > res |= git_config_set_in_file_gently(opts_file,\n> > \"options.no-commit\", \"true\");\n> > - if (opts->edit)\n> > - res |= git_config_set_in_file_gently(opts_file,\n> > - \"options.edit\", \"true\");\n> > + if (opts->edit != -1)\n>\n> s/!= -1/>= 0/\n\nWill fix.\n\n> > + res |= git_config_set_in_file_gently(opts_file, \"options.edit\",\n> > + opts->edit ? \"true\" : \"false\");\n> > if (opts->allow_empty)\n> > res |= git_config_set_in_file_gently(opts_file,\n> > \"options.allow-empty\", \"true\");\n> > @@ -4077,7 +4089,7 @@ static int pick_commits(struct repository *r,\n> > prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n> > if (opts->allow_ff)\n> > assert(!(opts->signoff || opts->no_commit ||\n> > - opts->record_origin || opts->edit ||\n> > + opts->record_origin || should_edit(opts) ||\n> > opts->committer_date_is_author_date ||\n> > opts->ignore_date));\n> > if (read_and_refresh_cache(r, opts))\n> > @@ -4370,14 +4382,35 @@ static int pick_commits(struct repository *r,\n> > return sequencer_remove_state(opts);\n> > }\n> >\n> > -static int continue_single_pick(struct repository *r)\n> > +static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n> > {\n> > - const char *argv[] = { \"commit\", NULL };\n> > + struct strvec argv = STRVEC_INIT;\n> > + int want_edit;\n>\n> Do we really want that extra `want_edit` variable? I think the code would\n> be easier to read without it, and still be obvious enough.\n>\n> > + int ret;\n> >\n> > if (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\") &&\n> > !refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\"))\n> > return error(_(\"no cherry-pick or revert in progress\"));\n> > - return run_command_v_opt(argv, RUN_GIT_CMD);\n> > +\n> > + strvec_push(&argv, \"commit\");\n> > +\n> > + /*\n> > + * continue_single_pick() handles the case of recovering from a\n> > + * conflict. should_edit() doesn't handle that case; for a conflict,\n> > + * we want to edit if the user asked for it, or if they didn't specify\n> > + * and stdin is a tty.\n> > + */\n> > + want_edit = (opts->edit == 1) || ((opts->edit == -1) && isatty(0));\n> > + if (!want_edit)\n>\n> Here is what I would prefer:\n>\n> if (!opts->edit || (opts->edit < 0 && !isatty(0)))\n\nGiven the changes elsewhere to use >0 as true, <0 as unspecified, and\n==0 for false, this change makes perfect sense. I'll include it.\n\n> The rest looks good, and the comments are _really_ helpful.\n>\n> And the remainder of the patch also looks good, so I will spare readers\n> time by not even quoting it.\n>\n> Thank you!\n> Dscho\n"},{"id":"420648","messageId":"CABPp-BErXWkEOCyZtYP9AHd9eP2osL4s1xjMZ_BRkGSktdFnKg@mail.gmail.com","threadId":"55389","inReplyTo":"xmqq7dloeawf.fsf@gitster.g","subject":"Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-30T20:16:51Z","receivedAt":"2021-03-30T20:17:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Mar 30, 2021 at 11:47 AM Junio C Hamano wrote:\n>\n> Johannes Schindelin writes:\n>\n> >> @@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> >> \"--signoff\", opts->signoff,\n> >> \"--no-commit\", opts->no_commit,\n> >> \"-x\", opts->record_origin,\n> >> - \"--edit\", opts->edit,\n> >> + \"--edit\", opts->edit == 1,\n> >\n> > Honestly, I'd prefer `> 0` here.\n>\n> Unless somebody (including Elijah) is trying to soon introduce yet\n> another value to .edit member, I'd agree 100%. If it is a tristate\n> (unspecified, no, yes), I think \"is it positive\" should be the way\n> to ask \"does the user definitely wants it?\", \"is it zero\" should be\n> the way to ask \"does the user definitely declines it?\" and \"is it\n> non-negative\" (and \"is it negative\") the way to ask \"does the user\n> care (or not care)?\". Using that consistently is good.\n\nSounds good; I'll switch it over.\n\n> >> +static int should_edit(struct replay_opts *opts) {\n> >> + assert(opts->edit >= -1 && opts->edit <= 1);\n> >\n> > Do we really want to introduce more of these useless `assert()`s? I know\n> > that we stopped converting them to `BUG()`, but I really dislike\n> > introducing new ones: they have very little effect, being no-ops by\n> > default in most setups.\n>\n> Yeah, in a new code in flux where programmers can easily make\n> errors, \"if (...) BUG()\" may not be a bad thing to add (but then we\n> may want to see if we can make the codepaths involved less error\n> prone), but I agree with your view that assert() is mostly useless.\n> A comment that explains the expectation and why that expectation is\n> there would be more useful.\n\nSince you both don't like this assert, I'll remove it. But I strongly\ndisagree that assert is useless in general. If you two have such a\nstrong reaction to assert statements, though, would you two prefer\nthat I add a new affirm() function that is ALSO compiled out in\nproduction? Because I really want to use one of those. My operating\nassumptions with asserts are the following:\n\n1) If the check is relevant for production, assert() statements should\nNOT be used; if(...) BUG() should be used instead.\n2) assert statements will be compiled out in production, almost always\n 2a) NOTE: don't make asserts expensive, since a few production users\nwill keep them\n3) assert statements will be active for future me and some other folks\ndoing active git development\n\nDo you two disagree with any of those operating assumptions? I find\nasserts very valuable because:\n\n* It's a _concise_ code comment that is readily understood. Any\nattempt to word in English the same thing that an assert statement\nchecks always takes longer\n* It helps future folks tweaking the rules to catch additional\nlocations where assumptions were made about the old rules. In the\ndevelopment of merge-ort, for example, asserts shortened my debugging\ncycles as I found and attempted new optimizations or added new\nfeatures or changed data structures and so on. The checks were _only_\nassists while developing; once the code is right, the checks could be\nremoved. But future development might occur, so it'd be nice to have\na way to keep the checks in the code just for those future developers\nwhile production users remove them.\n\nIn particular, for merge-ort, I think the second point is very\nhelpful. What can achieve the \"remove these now-unnecessary checks\nfrom the code for production, but keep them there for future\ndevelopment\"? I thought assert() was created exactly for this\npurpose. Would you rather I created an affirm() that does essentially\nthe same thing and is compiled out unless DEVELOPER=1? That would\nallow us to declare all assert() calls in the code as buggy, but I'm\nnot sure affirm() is as readily understood by developers reading the\ncode as \"ooh, a reminder I get to assume these statements are true\nwhile I'm reading the rest of the code\".\n\n> >> + if (opts->edit == -1)\n> >\n> > Maybe `< 0`, as we do elsewhere for \"not specified\"?\n>\n> Yup.\n>\n> >> + /*\n> >> + * Note that we only handle the case of non-conflicted\n> >> + * commits; continue_single_pick() handles the conflicted\n> >> + * commits itself instead of calling this function.\n> >> + */\n> >> + return (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;\n> >\n> > Apart from the extra parentheses, that makes sense to me.\n>\n> I can take it either way (but personally I think this particular one\n> is easier to see as written---this is subjective).\n>\n> > ...\n> > The rest looks good, and the comments are _really_ helpful.\n>\n> Yup, I agree.\n>\n> Thanks for a review.\n\nIndeed; and thanks to you as well Junio for all your time reviewing.\n"},{"id":"420690","messageId":"pull.988.v3.git.git.1617173541301.gitgitgadget@gmail.com","threadId":"55389","inReplyTo":"pull.988.v2.git.git.1617070174458.gitgitgadget@gmail.com","subject":"[PATCH v3] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-31T06:52:20Z","receivedAt":"2021-03-31T06:53:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nsave_opts() should save any non-default values. It was intended to do\nthis, but since most options in struct replay_opts default to 0, it only\nsaved non-zero values. Unfortunately, this does not always work for\noptions.edit. Roughly speaking, options.edit had a default value of 0\nfor cherry-pick but a default value of 1 for revert. Make save_opts()\nrecord a value whenever it differs from the default.\n\noptions.edit was also overly simplistic; we had more than two cases.\nThe behavior that previously existed was as follows:\n\n Non-conflict commits Right after Conflict\n revert Edit iff isatty(0) Edit (ignore isatty(0))\n cherry-pick No edit See above\n Specify --edit Edit (ignore isatty(0)) See above\n Specify --no-edit (*) See above\n\n (*) Before stopping for conflicts, No edit is the behavior. After\n stopping for conflicts, the --no-edit flag is not saved so see\n the first two rows.\n\nHowever, the expected behavior is:\n\n Non-conflict commits Right after Conflict\n revert Edit iff isatty(0) Edit iff isatty(0)\n cherry-pick No edit Edit iff isatty(0)\n Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0))\n Specify --no-edit No edit No edit\n\nIn order to get the expected behavior, we need to change options.edit\nto a tri-state: unspecified, false, or true. When specified, we follow\nwhat it says. When unspecified, we need to check whether the current\ncommit being created is resolving a conflict as well as consulting\noptions.action and isatty(0). While at it, add a should_edit() utility\nfunction that compresses options.edit down to a boolean based on the\nadditional information for the non-conflict case.\n\ncontinue_single_pick() is the function responsible for resuming after\nconflict cases, regardless of whether there is one commit being picked\nor many. Make this function stop assuming edit behavior in all cases,\nso that it can correctly handle !isatty(0) and specific requests to not\nedit the commit message.\n\nReported-by: Renato Botelho \nSigned-off-by: Elijah Newren \n---\n sequencer: fix edit handling for cherry-pick and revert messages\n \n Changes since v2:\n \n * Changed to use <0 for unspecified (instead of == -1), >0 for true\n (instead of == 1).\n * Removed assert() statement.\n * Removed unnecessary \"want_edit\" local variable\n \n Reported-by: Renato Botelho garga@freebsd.org Signed-off-by: Elijah\n Newren newren@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-988%2Fnewren%2Ffix-sequencer-no-edit-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-988/newren/fix-sequencer-no-edit-v3\nPull-Request: https://github.com/git/git/pull/988\n\nRange-diff vs v2:\n\n 1: 376799bd3d99 ! 1: 8fce8c772dee sequencer: fix edit handling for cherry-pick and revert messages\n @@ builtin/revert.c: static int run_sequencer(int argc, const char **argv, struct r\n \t\t\t\t\"--no-commit\", opts->no_commit,\n \t\t\t\t\"-x\", opts->record_origin,\n -\t\t\t\t\"--edit\", opts->edit,\n -+\t\t\t\t\"--edit\", opts->edit == 1,\n ++\t\t\t\t\"--edit\", opts->edit > 0,\n \t\t\t\tNULL);\n \n \tif (cmd) {\n @@ sequencer.c: static void record_in_rewritten(struct object_id *oid,\n }\n \n +static int should_edit(struct replay_opts *opts) {\n -+\tassert(opts->edit >= -1 && opts->edit <= 1);\n -+\tif (opts->edit == -1)\n ++\tif (opts->edit < 0)\n +\t\t/*\n +\t\t * Note that we only handle the case of non-conflicted\n +\t\t * commits; continue_single_pick() handles the conflicted\n @@ sequencer.c: static int save_opts(struct replay_opts *opts)\n -\tif (opts->edit)\n -\t\tres |= git_config_set_in_file_gently(opts_file,\n -\t\t\t\t\t\"options.edit\", \"true\");\n -+\tif (opts->edit != -1)\n ++\tif (opts->edit >= 0)\n +\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\",\n +\t\t\t\t\t\t opts->edit ? \"true\" : \"false\");\n \tif (opts->allow_empty)\n @@ sequencer.c: static int pick_commits(struct repository *r,\n {\n -\tconst char *argv[] = { \"commit\", NULL };\n +\tstruct strvec argv = STRVEC_INIT;\n -+\tint want_edit;\n +\tint ret;\n \n \tif (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\") &&\n @@ sequencer.c: static int pick_commits(struct repository *r,\n +\t * we want to edit if the user asked for it, or if they didn't specify\n +\t * and stdin is a tty.\n +\t */\n -+\twant_edit = (opts->edit == 1) || ((opts->edit == -1) && isatty(0));\n -+\tif (!want_edit)\n ++\tif (!opts->edit || (opts->edit < 0 && !isatty(0)))\n +\t\t/*\n +\t\t * Include --cleanup=strip as well because we don't want the\n +\t\t * \"# Conflicts:\" messages.\n\n\n builtin/revert.c | 4 +--\n sequencer.c | 52 ++++++++++++++++++++++++++-------\n sequencer.h | 6 ++--\n t/t3510-cherry-pick-sequence.sh | 32 +++++++++++++++++++-\n 4 files changed, 77 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 314a86c5621b..237f2f18d4c0 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"--signoff\", opts->signoff,\n \t\t\t\t\"--no-commit\", opts->no_commit,\n \t\t\t\t\"-x\", opts->record_origin,\n-\t\t\t\t\"--edit\", opts->edit,\n+\t\t\t\t\"--edit\", opts->edit > 0,\n \t\t\t\tNULL);\n \n \tif (cmd) {\n@@ -230,8 +230,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n \tint res;\n \n-\tif (isatty(0))\n-\t\topts.edit = 1;\n \topts.action = REPLAY_REVERT;\n \tsequencer_init_config(&opts);\n \tres = run_sequencer(argc, argv, &opts);\ndiff --git a/sequencer.c b/sequencer.c\nindex 848204d3dc3f..8ef597ac3a9e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1860,14 +1860,25 @@ static void record_in_rewritten(struct object_id *oid,\n \t\tflush_rewritten_pending();\n }\n \n+static int should_edit(struct replay_opts *opts) {\n+\tif (opts->edit < 0)\n+\t\t/*\n+\t\t * Note that we only handle the case of non-conflicted\n+\t\t * commits; continue_single_pick() handles the conflicted\n+\t\t * commits itself instead of calling this function.\n+\t\t */\n+\t\treturn (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;\n+\treturn opts->edit;\n+}\n+\n static int do_pick_commit(struct repository *r,\n \t\t\t enum todo_command command,\n \t\t\t struct commit *commit,\n \t\t\t struct replay_opts *opts,\n \t\t\t int final_fixup, int *check_todo)\n {\n-\tunsigned int flags = opts->edit ? EDIT_MSG : 0;\n-\tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg(r);\n+\tunsigned int flags = should_edit(opts) ? EDIT_MSG : 0;\n+\tconst char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);\n \tstruct object_id head;\n \tstruct commit *base, *next, *parent;\n \tconst char *base_label, *next_label;\n@@ -3101,9 +3112,9 @@ static int save_opts(struct replay_opts *opts)\n \tif (opts->no_commit)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.no-commit\", \"true\");\n-\tif (opts->edit)\n-\t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.edit\", \"true\");\n+\tif (opts->edit >= 0)\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\",\n+\t\t\t\t\t\t opts->edit ? \"true\" : \"false\");\n \tif (opts->allow_empty)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.allow-empty\", \"true\");\n@@ -4077,7 +4088,7 @@ static int pick_commits(struct repository *r,\n \tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n-\t\t\t opts->record_origin || opts->edit ||\n+\t\t\t opts->record_origin || should_edit(opts) ||\n \t\t\t opts->committer_date_is_author_date ||\n \t\t\t opts->ignore_date));\n \tif (read_and_refresh_cache(r, opts))\n@@ -4370,14 +4381,33 @@ static int pick_commits(struct repository *r,\n \treturn sequencer_remove_state(opts);\n }\n \n-static int continue_single_pick(struct repository *r)\n+static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n {\n-\tconst char *argv[] = { \"commit\", NULL };\n+\tstruct strvec argv = STRVEC_INIT;\n+\tint ret;\n \n \tif (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\") &&\n \t !refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\"))\n \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n-\treturn run_command_v_opt(argv, RUN_GIT_CMD);\n+\n+\tstrvec_push(&argv, \"commit\");\n+\n+\t/*\n+\t * continue_single_pick() handles the case of recovering from a\n+\t * conflict. should_edit() doesn't handle that case; for a conflict,\n+\t * we want to edit if the user asked for it, or if they didn't specify\n+\t * and stdin is a tty.\n+\t */\n+\tif (!opts->edit || (opts->edit < 0 && !isatty(0)))\n+\t\t/*\n+\t\t * Include --cleanup=strip as well because we don't want the\n+\t\t * \"# Conflicts:\" messages.\n+\t\t */\n+\t\tstrvec_pushl(&argv, \"--no-edit\", \"--cleanup=strip\", NULL);\n+\n+\tret = run_command_v_opt(argv.v, RUN_GIT_CMD);\n+\tstrvec_clear(&argv);\n+\treturn ret;\n }\n \n static int commit_staged_changes(struct repository *r,\n@@ -4547,7 +4577,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\t\tgoto release_todo_list;\n \t\t}\n \t} else if (!file_exists(get_todo_path(opts)))\n-\t\treturn continue_single_pick(r);\n+\t\treturn continue_single_pick(r, opts);\n \telse if ((res = read_populate_todo(r, &todo_list, opts)))\n \t\tgoto release_todo_list;\n \n@@ -4556,7 +4586,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\tif (refs_ref_exists(get_main_ref_store(r),\n \t\t\t\t \"CHERRY_PICK_HEAD\") ||\n \t\t refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\")) {\n-\t\t\tres = continue_single_pick(r);\n+\t\t\tres = continue_single_pick(r, opts);\n \t\t\tif (res)\n \t\t\t\tgoto release_todo_list;\n \t\t}\ndiff --git a/sequencer.h b/sequencer.h\nindex f8b2e4ab8527..d57d8ea23d7a 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -31,8 +31,10 @@ enum commit_msg_cleanup_mode {\n struct replay_opts {\n \tenum replay_action action;\n \n-\t/* Boolean options */\n+\t/* Tri-state options: unspecified, false, or true */\n \tint edit;\n+\n+\t/* Boolean options */\n \tint record_origin;\n \tint no_commit;\n \tint signoff;\n@@ -71,7 +73,7 @@ struct replay_opts {\n \t/* Only used by REPLAY_NONE */\n \tstruct rev_info *revs;\n };\n-#define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }\n+#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }\n \n /*\n * Note that ordering matters in this enum. Not only must it match the mapping\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex b76cb6de91d0..49010aa9469d 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -65,7 +65,7 @@ test_expect_success 'cherry-pick persists opts correctly' '\n \t# gets interrupted, use a high-enough number that is larger\n \t# than the number of parents of any commit we have created\n \tmainline=4 &&\n-\ttest_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&\n+\ttest_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours --edit initial..anotherpick &&\n \ttest_path_is_dir .git/sequencer &&\n \ttest_path_is_file .git/sequencer/head &&\n \ttest_path_is_file .git/sequencer/todo &&\n@@ -84,6 +84,36 @@ test_expect_success 'cherry-pick persists opts correctly' '\n \tours\n \tEOF\n \tgit config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"true\" >expect &&\n+\tgit config --file=.git/sequencer/opts --get-all options.edit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'revert persists opts correctly' '\n+\tpristine_detach initial &&\n+\t# to make sure that the session to revert a sequence\n+\t# gets interrupted, revert commits that are not in the history\n+\t# of HEAD.\n+\ttest_expect_code 1 git revert -s --strategy=recursive -X patience -X ours --no-edit picked yetanotherpick &&\n+\ttest_path_is_dir .git/sequencer &&\n+\ttest_path_is_file .git/sequencer/head &&\n+\ttest_path_is_file .git/sequencer/todo &&\n+\ttest_path_is_file .git/sequencer/opts &&\n+\techo \"true\" >expect &&\n+\tgit config --file=.git/sequencer/opts --get-all options.signoff >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"recursive\" >expect &&\n+\tgit config --file=.git/sequencer/opts --get-all options.strategy >actual &&\n+\ttest_cmp expect actual &&\n+\tcat >expect <<-\\EOF &&\n+\tpatience\n+\tours\n+\tEOF\n+\tgit config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"false\" >expect &&\n+\tgit config --file=.git/sequencer/opts --get-all options.edit >actual &&\n \ttest_cmp expect actual\n '\n \n\nbase-commit: 98164e9585e02e31dcf1377a553efe076c15f8c6\n-- \ngitgitgadget\n"},{"id":"420705","messageId":"nycvar.QRO.7.76.6.2103311533340.52@tvgsbejvaqbjf.bet","threadId":"55389","inReplyTo":"CABPp-BGwAtpsQJ8U5N1q21PMkideptY2MB2PNgbPqvya+XuyHg@mail.gmail.com","subject":"unifying sequencer's options persisting, was Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-03-31T13:48:25Z","receivedAt":"2021-03-31T13:52:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 30 Mar 2021, Elijah Newren wrote:\n\n> On Tue, Mar 30, 2021 at 3:13 AM Johannes Schindelin\n> wrote:\n>\n> > I'll allow myself one tangent: the subject of the sequencer's Unix\n> > shell script heritage seems to come up with an increasing frequency,\n> > in particular the awful \"let's write out one file per setting\"\n> > strategy.\n> >\n> > I would _love_ for `save_opts()` to write a JSON instead (or an INI\n> > via the `git_config_*()` family of functions, as is done already by\n> > the cherry-pick/revert stuff), now that we no longer have any shell\n> > script backend (apart from `--preserve-merges`, but that one is on its\n> > way out anyway).\n> >\n> > The one thing that concerns me with this idea is that I know for a\n> > fact that some enterprisey users play games with those files inside\n> > `/rebase-merge` that should be considered internal\n> > implementation details. Not sure how to deprecate that properly, I\n> > don't think we have a sane way to detect whether users rely on these\n> > implementation details other than breaking their expectations, which\n> > is not really a gentle way to ask them to update their scripts.\n>\n> Ooh, I'm glad you took this tangent. May I follow it for a second?\n> I'd really, really like this too, for three reasons:\n>\n> 1) I constantly get confused about the massive duplication and\n> difference in control structures and which ones affect which type of\n> operations in sequencer.c. Having them both use an ini-file would\n> allow us to remove lots of that duplication. I'm sure there'll still\n> be some rebase-specific or cherry-pick-specific options, but we don't\n> need a separate parallel structure for every piece of config.\n>\n> 2) In my merge-ort optimization work, rebasing 35 commits in linux.git\n> across a massive set of 26K upstream renames has dropped rename\n> detection time from the top spot. And it also dropped several other\n> things in the merge machinery from their spots as well. Do you know\n> what's the slowest now? Wasted time from sequencer.c: the unnecessary\n> process forking and all the useless disk writing (which I suspect is\n> mostly updating the index and working directory but also writing all\n> the individual control files under .git/rebase-merge/). And this\n> stuff from sequencer.c is not just barely the slowest part, it's the\n> slowest by a wide margin.\n>\n> 3) I also want to allow cherry-picking or rebasing branches that\n> aren't even checked out (assuming no conflicts are triggered;\n> otherwise an error can be shown with the user asked to repeat with the\n> operation connected to an active working directory). For such an\n> operation, the difference between \"cherry-pick\" and \"rebase\" is nearly\n> irrelevant so you'd expect the code to be the same; every time I look\n> at the code, though, it seems that the control structures are\n> separating these two types of operations in more areas than just the\n> reading and writing of the config.\n\nExcellent, we're in agreement, then.\n\nThe remaining question is: how do we want to go about it? Do we just want\nto codify the notion that these are internal details that are nobody's\nbusiness, and if they are using the exact file system layout of the\ncurrent sequencer, then it's their responsibility to adapt?\n\nCiao,\nDscho\n"},{"id":"420709","messageId":"nycvar.QRO.7.76.6.2103311638280.52@tvgsbejvaqbjf.bet","threadId":"55389","inReplyTo":"pull.988.v3.git.git.1617173541301.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-03-31T14:38:59Z","receivedAt":"2021-03-31T14:42:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Wed, 31 Mar 2021, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> save_opts() should save any non-default values. It was intended to do\n> this, but since most options in struct replay_opts default to 0, it only\n> saved non-zero values. Unfortunately, this does not always work for\n> options.edit. Roughly speaking, options.edit had a default value of 0\n> for cherry-pick but a default value of 1 for revert. Make save_opts()\n> record a value whenever it differs from the default.\n>\n> options.edit was also overly simplistic; we had more than two cases.\n> The behavior that previously existed was as follows:\n>\n> Non-conflict commits Right after Conflict\n> revert Edit iff isatty(0) Edit (ignore isatty(0))\n> cherry-pick No edit See above\n> Specify --edit Edit (ignore isatty(0)) See above\n> Specify --no-edit (*) See above\n>\n> (*) Before stopping for conflicts, No edit is the behavior. After\n> stopping for conflicts, the --no-edit flag is not saved so see\n> the first two rows.\n>\n> However, the expected behavior is:\n>\n> Non-conflict commits Right after Conflict\n> revert Edit iff isatty(0) Edit iff isatty(0)\n> cherry-pick No edit Edit iff isatty(0)\n> Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0))\n> Specify --no-edit No edit No edit\n>\n> In order to get the expected behavior, we need to change options.edit\n> to a tri-state: unspecified, false, or true. When specified, we follow\n> what it says. When unspecified, we need to check whether the current\n> commit being created is resolving a conflict as well as consulting\n> options.action and isatty(0). While at it, add a should_edit() utility\n> function that compresses options.edit down to a boolean based on the\n> additional information for the non-conflict case.\n>\n> continue_single_pick() is the function responsible for resuming after\n> conflict cases, regardless of whether there is one commit being picked\n> or many. Make this function stop assuming edit behavior in all cases,\n> so that it can correctly handle !isatty(0) and specific requests to not\n> edit the commit message.\n>\n> Reported-by: Renato Botelho \n> Signed-off-by: Elijah Newren \n> ---\n> sequencer: fix edit handling for cherry-pick and revert messages\n>\n> Changes since v2:\n>\n> * Changed to use <0 for unspecified (instead of == -1), >0 for true\n> (instead of == 1).\n> * Removed assert() statement.\n> * Removed unnecessary \"want_edit\" local variable\n>\n> Reported-by: Renato Botelho garga@freebsd.org Signed-off-by: Elijah\n> Newren newren@gmail.com\n\nLooks good, thank you!\n\nReviewed-by: Johannes Schindelin \n\nThanks,\nDscho\n"},{"id":"420712","messageId":"87mtujkyxb.fsf@evledraar.gmail.com","threadId":"55389","inReplyTo":"CABPp-BErXWkEOCyZtYP9AHd9eP2osL4s1xjMZ_BRkGSktdFnKg@mail.gmail.com","subject":"Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-31T17:36:48Z","receivedAt":"2021-03-31T17:37:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 30 2021, Elijah Newren wrote:\n\n> On Tue, Mar 30, 2021 at 11:47 AM Junio C Hamano wrote:\n>>\n>> Johannes Schindelin writes:\n>>\n>> >> @@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n>> >> \"--signoff\", opts->signoff,\n>> >> \"--no-commit\", opts->no_commit,\n>> >> \"-x\", opts->record_origin,\n>> >> - \"--edit\", opts->edit,\n>> >> + \"--edit\", opts->edit == 1,\n>> >\n>> > Honestly, I'd prefer `> 0` here.\n>>\n>> Unless somebody (including Elijah) is trying to soon introduce yet\n>> another value to .edit member, I'd agree 100%. If it is a tristate\n>> (unspecified, no, yes), I think \"is it positive\" should be the way\n>> to ask \"does the user definitely wants it?\", \"is it zero\" should be\n>> the way to ask \"does the user definitely declines it?\" and \"is it\n>> non-negative\" (and \"is it negative\") the way to ask \"does the user\n>> care (or not care)?\". Using that consistently is good.\n>\n> Sounds good; I'll switch it over.\n>\n>> >> +static int should_edit(struct replay_opts *opts) {\n>> >> + assert(opts->edit >= -1 && opts->edit <= 1);\n>> >\n>> > Do we really want to introduce more of these useless `assert()`s? I know\n>> > that we stopped converting them to `BUG()`, but I really dislike\n>> > introducing new ones: they have very little effect, being no-ops by\n>> > default in most setups.\n>>\n>> Yeah, in a new code in flux where programmers can easily make\n>> errors, \"if (...) BUG()\" may not be a bad thing to add (but then we\n>> may want to see if we can make the codepaths involved less error\n>> prone), but I agree with your view that assert() is mostly useless.\n>> A comment that explains the expectation and why that expectation is\n>> there would be more useful.\n>\n> Since you both don't like this assert, I'll remove it. But I strongly\n> disagree that assert is useless in general. If you two have such a\n> strong reaction to assert statements, though, would you two prefer\n> that I add a new affirm() function that is ALSO compiled out in\n> production? Because I really want to use one of those. My operating\n> assumptions with asserts are the following:\n>\n> 1) If the check is relevant for production, assert() statements should\n> NOT be used; if(...) BUG() should be used instead.\n> 2) assert statements will be compiled out in production, almost always\n> 2a) NOTE: don't make asserts expensive, since a few production users\n> will keep them\n> 3) assert statements will be active for future me and some other folks\n> doing active git development\n>\n> Do you two disagree with any of those operating assumptions? I find\n> asserts very valuable because:\n>\n> * It's a _concise_ code comment that is readily understood. Any\n> attempt to word in English the same thing that an assert statement\n> checks always takes longer\n> * It helps future folks tweaking the rules to catch additional\n> locations where assumptions were made about the old rules. In the\n> development of merge-ort, for example, asserts shortened my debugging\n> cycles as I found and attempted new optimizations or added new\n> features or changed data structures and so on. The checks were _only_\n> assists while developing; once the code is right, the checks could be\n> removed. But future development might occur, so it'd be nice to have\n> a way to keep the checks in the code just for those future developers\n> while production users remove them.\n>\n> In particular, for merge-ort, I think the second point is very\n> helpful. What can achieve the \"remove these now-unnecessary checks\n> from the code for production, but keep them there for future\n> development\"? I thought assert() was created exactly for this\n> purpose. Would you rather I created an affirm() that does essentially\n> the same thing and is compiled out unless DEVELOPER=1? That would\n> allow us to declare all assert() calls in the code as buggy, but I'm\n> not sure affirm() is as readily understood by developers reading the\n> code as \"ooh, a reminder I get to assume these statements are true\n> while I'm reading the rest of the code\".\n\nI don't mind the asserts, or to have them in the default build.\n\nBut if you'd like to submit patches for asserts and can't otherwise get\nthem accepted, then can we please not make DEVELOPER a thing that you\ncan't turn on in production without thinking twice? Per my\nhttps://lore.kernel.org/git/87wnusj6gt.fsf@evledraar.gmail.com/\n\n>> >> + if (opts->edit == -1)\n>> >\n>> > Maybe `< 0`, as we do elsewhere for \"not specified\"?\n>>\n>> Yup.\n>>\n>> >> + /*\n>> >> + * Note that we only handle the case of non-conflicted\n>> >> + * commits; continue_single_pick() handles the conflicted\n>> >> + * commits itself instead of calling this function.\n>> >> + */\n>> >> + return (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;\n>> >\n>> > Apart from the extra parentheses, that makes sense to me.\n>>\n>> I can take it either way (but personally I think this particular one\n>> is easier to see as written---this is subjective).\n>>\n>> > ...\n>> > The rest looks good, and the comments are _really_ helpful.\n>>\n>> Yup, I agree.\n>>\n>> Thanks for a review.\n>\n> Indeed; and thanks to you as well Junio for all your time reviewing.\n\n"},{"id":"420713","messageId":"CABPp-BGXW4=8Qb6w22mNCd8fuMs93Kztzavgdu7E--YkjMgmvQ@mail.gmail.com","threadId":"55389","inReplyTo":"87mtujkyxb.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-31T17:52:22Z","receivedAt":"2021-03-31T17:53:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 31, 2021 at 10:36 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Tue, Mar 30 2021, Elijah Newren wrote:\n>\n> > In particular, for merge-ort, I think the second point is very\n> > helpful. What can achieve the \"remove these now-unnecessary checks\n> > from the code for production, but keep them there for future\n> > development\"? I thought assert() was created exactly for this\n> > purpose. Would you rather I created an affirm() that does essentially\n> > the same thing and is compiled out unless DEVELOPER=1? That would\n> > allow us to declare all assert() calls in the code as buggy, but I'm\n> > not sure affirm() is as readily understood by developers reading the\n> > code as \"ooh, a reminder I get to assume these statements are true\n> > while I'm reading the rest of the code\".\n>\n> I don't mind the asserts, or to have them in the default build.\n>\n> But if you'd like to submit patches for asserts and can't otherwise get\n> them accepted, then can we please not make DEVELOPER a thing that you\n> can't turn on in production without thinking twice? Per my\n> https://lore.kernel.org/git/87wnusj6gt.fsf@evledraar.gmail.com/\n\nFair enough; if we have to go the affirm() route, I should probably\njust make it depend on NDEBUG. :-)\n"},{"id":"420714","messageId":"xmqqmtuj8ank.fsf@gitster.g","threadId":"55389","inReplyTo":"CABPp-BErXWkEOCyZtYP9AHd9eP2osL4s1xjMZ_BRkGSktdFnKg@mail.gmail.com","subject":"Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-31T18:01:51Z","receivedAt":"2021-03-31T18:02:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> ... would you two prefer\n> that I add a new affirm() function that is ALSO compiled out in\n> production?\n\nThe reason I do not think affirm would fly is because it shares the\nmost problematic trait with assert(): it can be comipled out.\n\nThere was at least one bug we found and fixed in a piece of code\nthat came later in a codepath that had an assert() in it. The buggy\ncode (IIRC, it was added way after the assert() was written in an\nunrelated patch) was unknowingly depending on a side-effect of an\ninnocuous function call (i.e. assert(func_tion(args) == expected))\nmade in the assert condition, and the bug did not reproduce in the\ndeveloper's environment.\n"},{"id":"420813","messageId":"CABPp-BFLyYTboiGkP=sc7S+stRZvozAqnJPnza+M0q2nakvD2Q@mail.gmail.com","threadId":"55389","inReplyTo":"xmqqmtuj8ank.fsf@gitster.g","subject":"Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-04-01T16:31:26Z","receivedAt":"2021-04-01T17:44:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 31, 2021 at 11:01 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > ... would you two prefer\n> > that I add a new affirm() function that is ALSO compiled out in\n> > production?\n>\n> The reason I do not think affirm would fly is because it shares the\n> most problematic trait with assert(): it can be comipled out.\n>\n> There was at least one bug we found and fixed in a piece of code\n> that came later in a codepath that had an assert() in it. The buggy\n> code (IIRC, it was added way after the assert() was written in an\n> unrelated patch) was unknowingly depending on a side-effect of an\n> innocuous function call (i.e. assert(func_tion(args) == expected))\n> made in the assert condition, and the bug did not reproduce in the\n> developer's environment.\n\nAh, now that's a much different argument than claiming assert is\nuseless. I had to debug one of those once about a decade ago, in a\ndifferent project, put in by some other developer, and yeah it's\npainful.\n\nIf that causes assert() to be frowned upon, then I can understand\nbased on this reason. If so, perhaps a BUG_ON() function would be\nuseful? The \"if (...) BUG()\" construct causes line coverage problems\nand I find it an onerous way to signal to future code readers \"hey,\njust as a reminder, these things should be true at this point\". If\nyou need to signal something more than that or explain why the things\nbeing false is a problem or what might cause it, then the \"if (...)\nBUG()\" construct is useful and there are certainly many places for\nthat, but it just doesn't fit all the cases.\n"},{"id":"420873","messageId":"3b117e65-bf9f-af13-b093-28bbbd6f9bb3@gmail.com","threadId":"55389","inReplyTo":"nycvar.QRO.7.76.6.2103311533340.52@tvgsbejvaqbjf.bet","subject":"Re: unifying sequencer's options persisting, was Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-04-02T11:28:43Z","receivedAt":"2021-04-02T11:28:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho and Elijah\n\nOn 31/03/2021 14:48, Johannes Schindelin wrote:\n> Hi,\n> \n> On Tue, 30 Mar 2021, Elijah Newren wrote:\n> \n>> On Tue, Mar 30, 2021 at 3:13 AM Johannes Schindelin\n>> wrote:\n>>\n>>> I'll allow myself one tangent: the subject of the sequencer's Unix\n>>> shell script heritage seems to come up with an increasing frequency,\n>>> in particular the awful \"let's write out one file per setting\"\n>>> strategy.\n>>>\n>>> I would _love_ for `save_opts()` to write a JSON instead (or an INI\n>>> via the `git_config_*()` family of functions, as is done already by\n>>> the cherry-pick/revert stuff), now that we no longer have any shell\n>>> script backend (apart from `--preserve-merges`, but that one is on its\n>>> way out anyway).\n>>>\n>>> The one thing that concerns me with this idea is that I know for a\n>>> fact that some enterprisey users play games with those files inside\n>>> `/rebase-merge` that should be considered internal\n>>> implementation details. Not sure how to deprecate that properly, I\n>>> don't think we have a sane way to detect whether users rely on these\n>>> implementation details other than breaking their expectations, which\n>>> is not really a gentle way to ask them to update their scripts.\n\nI think it depends if users are just reading the files or writing to \nthem. If they are reading them (which my scripts do) then we could \ncontinue to write the important files along side the new single-file \nthat git actually reads. I think there is a distinction between the \nfiles such as git-rebase-todo which hold state and the one-line files \nwhich hold the options passed by the user. For example I have scripts \nthat read git-rebase-todo, rewritten-pending, rewritten-list, amend-head \nand check that author-script exists. If a script starts a rebase then it \nshould know what options are in effect without reading them from \n.git/rebase-merge.\n\n>> Ooh, I'm glad you took this tangent. May I follow it for a second?\n>> I'd really, really like this too, for three reasons:\n>>\n>> 1) I constantly get confused about the massive duplication and\n>> difference in control structures and which ones affect which type of\n>> operations in sequencer.c. Having them both use an ini-file would\n>> allow us to remove lots of that duplication. I'm sure there'll still\n>> be some rebase-specific or cherry-pick-specific options, but we don't\n>> need a separate parallel structure for every piece of config.\n>>\n>> 2) In my merge-ort optimization work, rebasing 35 commits in linux.git\n>> across a massive set of 26K upstream renames has dropped rename\n>> detection time from the top spot. And it also dropped several other\n>> things in the merge machinery from their spots as well. Do you know\n>> what's the slowest now? Wasted time from sequencer.c: the unnecessary\n>> process forking and all the useless disk writing (which I suspect is\n>> mostly updating the index and working directory but also writing all\n>> the individual control files under .git/rebase-merge/). And this\n>> stuff from sequencer.c is not just barely the slowest part, it's the\n>> slowest by a wide margin.\n\nI think we do a lot of needless writing which is unrelated to whether we \nwrite to one file or may files. For example from memory picking a commit \ninvolves writing the 'message', 'author-script', 'rewritten-pending' \n(which is often immediately deleted), 'rewritten-list' (we append to \nthat one) 'CHERRY_PICK_HEAD' (which is immediately deleted unless the \npick has become empty), 'git-rebase-todo' is completely rewritten, and \n'done' is appended to. None of this is necessary. For rewording and \nmerges the only thing that needs to be written is the commit message for \nthe external process to use. Fixup and squash add a couple more files \ninto the mix.\n\nI think we would save a lot by only syncing the state to disk when we \nstop or run an exec command (the state needs to be synced so exec \ncommands can alter the todo list). In those cases we need to write the \nindex and possibly run an external process so writing a couple of files \nis probably insignificant.\n\nWhere I think we can usefully consolidate is the one-line files which \nstore the options rather than state - these are read an written much \nless frequently so I don't think they have much of a performance hit but \nit would be much nicer to just serialize the options to a single file.\n\n>>\n>> 3) I also want to allow cherry-picking or rebasing branches that\n>> aren't even checked out (assuming no conflicts are triggered;\n>> otherwise an error can be shown with the user asked to repeat with the\n>> operation connected to an active working directory).\n\nExciting!\n\n>> For such an\n>> operation, the difference between \"cherry-pick\" and \"rebase\" is nearly\n>> irrelevant so you'd expect the code to be the same; every time I look\n>> at the code, though, it seems that the control structures are\n>> separating these two types of operations in more areas than just the\n>> reading and writing of the config.\n\nYes this can be confusing, for example rebase and cherry-pick handle the \ntodo list differently. Rebase removes the command before trying to pick \nthe commit and adds it back if the pick fails for a non-conflict reason, \ncherry-pick only removes the command if the pick is successful.\n\nBest Wishes\n\nPhillip\n\n> Excellent, we're in agreement, then.\n> \n> The remaining question is: how do we want to go about it? Do we just want\n> to codify the notion that these are internal details that are nobody's\n> business, and if they are using the exact file system layout of the\n> current sequencer, then it's their responsibility to adapt?\n> \n> Ciao,\n> Dscho\n> \n\n"},{"id":"420880","messageId":"b0375013-e287-06b1-2718-3736fc1b73e7@gmail.com","threadId":"55389","inReplyTo":"3b117e65-bf9f-af13-b093-28bbbd6f9bb3@gmail.com","subject":"Re: unifying sequencer's options persisting, was Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-04-02T13:10:16Z","receivedAt":"2021-04-02T13:10:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"one more thought below...\n\nOn 02/04/2021 12:28, Phillip Wood wrote:\n> Hi Dscho and Elijah\n> \n> On 31/03/2021 14:48, Johannes Schindelin wrote:\n>> Hi,\n>>\n>> On Tue, 30 Mar 2021, Elijah Newren wrote:\n>>\n>>> On Tue, Mar 30, 2021 at 3:13 AM Johannes Schindelin\n>>> wrote:\n>>>\n>>>> I'll allow myself one tangent: the subject of the sequencer's Unix\n>>>> shell script heritage seems to come up with an increasing frequency,\n>>>> in particular the awful \"let's write out one file per setting\"\n>>>> strategy.\n>>>>\n>>>> I would _love_ for `save_opts()` to write a JSON instead (or an INI\n>>>> via the `git_config_*()` family of functions, as is done already by\n>>>> the cherry-pick/revert stuff), now that we no longer have any shell\n>>>> script backend (apart from `--preserve-merges`, but that one is on its\n>>>> way out anyway).\n>>>>\n>>>> The one thing that concerns me with this idea is that I know for a\n>>>> fact that some enterprisey users play games with those files inside\n>>>> `/rebase-merge` that should be considered internal\n>>>> implementation details. Not sure how to deprecate that properly, I\n>>>> don't think we have a sane way to detect whether users rely on these\n>>>> implementation details other than breaking their expectations, which\n>>>> is not really a gentle way to ask them to update their scripts.\n> \n> I think it depends if users are just reading the files or writing to \n> them. If they are reading them (which my scripts do) then we could \n> continue to write the important files along side the new single-file \n> that git actually reads. I think there is a distinction between the \n> files such as git-rebase-todo which hold state and the one-line files \n> which hold the options passed by the user. For example I have scripts \n> that read git-rebase-todo, rewritten-pending, rewritten-list, amend-head \n> and check that author-script exists. If a script starts a rebase then it \n> should know what options are in effect without reading them from \n> .git/rebase-merge.\n> \n>>> Ooh, I'm glad you took this tangent.  May I follow it for a second?\n>>> I'd really, really like this too, for three reasons:\n>>>\n>>> 1) I constantly get confused about the massive duplication and\n>>> difference in control structures and which ones affect which type of\n>>> operations in sequencer.c.  Having them both use an ini-file would\n>>> allow us to remove lots of that duplication.  I'm sure there'll still\n>>> be some rebase-specific or cherry-pick-specific options, but we don't\n>>> need a separate parallel structure for every piece of config.\n\nOne thing to bear in mind is that you can cherry-pick or revert a \nsequence of commits while rebasing - I think that means we need to store \nthe state for rebase in a separate location to that for cherry-pick/revert\n\nBest Wishes\n\nPhillip\n\n>>> 2) In my merge-ort optimization work, rebasing 35 commits in linux.git\n>>> across a massive set of 26K upstream renames has dropped rename\n>>> detection time from the top spot.  And it also dropped several other\n>>> things in the merge machinery from their spots as well.  Do you know\n>>> what's the slowest now?  Wasted time from sequencer.c: the unnecessary\n>>> process forking and all the useless disk writing (which I suspect is\n>>> mostly updating the index and working directory but also writing all\n>>> the individual control files under .git/rebase-merge/).  And this\n>>> stuff from sequencer.c is not just barely the slowest part, it's the\n>>> slowest by a wide margin.\n> \n> I think we do a lot of needless writing which is unrelated to whether we \n> write to one file or may files. For example from memory picking a commit \n> involves writing the 'message', 'author-script', 'rewritten-pending' \n> (which is often immediately deleted), 'rewritten-list' (we append to \n> that one) 'CHERRY_PICK_HEAD' (which is immediately deleted unless the \n> pick has become empty), 'git-rebase-todo' is completely rewritten, and \n> 'done' is appended to. None of this is necessary. For rewording and \n> merges the only thing that needs to be written is the commit message for \n> the external process to use. Fixup and squash add a couple more files \n> into the mix.\n> \n> I think we would save a lot by only syncing the state to disk when we \n> stop or run an exec command (the state needs to be synced so exec \n> commands can alter the todo list). In those cases we need to write the \n> index and possibly run an external process so writing a couple of files \n> is probably insignificant.\n> \n> Where I think we can usefully consolidate is the one-line files which \n> store the options rather than state - these are read an written much \n> less frequently so I don't think they have much of a performance hit but \n> it would be much nicer to just serialize the options to a single file.\n> \n>>>\n>>> 3) I also want to allow cherry-picking or rebasing branches that\n>>> aren't even checked out (assuming no conflicts are triggered;\n>>> otherwise an error can be shown with the user asked to repeat with the\n>>> operation connected to an active working directory).\n> \n> Exciting!\n> \n>>>  For such an\n>>> operation, the difference between \"cherry-pick\" and \"rebase\" is nearly\n>>> irrelevant so you'd expect the code to be the same; every time I look\n>>> at the code, though, it seems that the control structures are\n>>> separating these two types of operations in more areas than just the\n>>> reading and writing of the config.\n> \n> Yes this can be confusing, for example rebase and cherry-pick handle the \n> todo list differently. Rebase removes the command before trying to pick \n> the commit and adds it back if the pick fails for a non-conflict reason, \n> cherry-pick only removes the command if the pick is successful.\n> \n> Best Wishes\n> \n> Phillip\n> \n>> Excellent, we're in agreement, then.\n>>\n>> The remaining question is: how do we want to go about it? Do we just want\n>> to codify the notion that these are internal details that are nobody's\n>> business, and if they are using the exact file system layout of the\n>> current sequencer, then it's their responsibility to adapt?\n>>\n>> Ciao,\n>> Dscho\n>>\n> \n\n"},{"id":"420893","messageId":"xmqqa6qg4d0i.fsf@gitster.g","threadId":"55389","inReplyTo":"3b117e65-bf9f-af13-b093-28bbbd6f9bb3@gmail.com","subject":"Re: unifying sequencer's options persisting, was Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-02T21:01:17Z","receivedAt":"2021-04-02T21:01:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> I think we would save a lot by only syncing the state to disk when we\n> stop or run an exec command (the state needs to be synced so exec \n> commands can alter the todo list). In those cases we need to write the\n> index and possibly run an external process so writing a couple of\n> files is probably insignificant.\n\nThe optimization opportunity of this may be a lot smaller than you\nwould think---you must cater to not just exec but hook scripts that\nare run while a new commit is made, which means every step you'd\nneed to write anyway.\n\n> Where I think we can usefully consolidate is the one-line files which\n> store the options rather than state - these are read an written much \n> less frequently so I don't think they have much of a performance hit\n> but it would be much nicer to just serialize the options to a single\n> file.\n\nWould that break external scripts, hooks, etc.? I am not sure if we\neven have any rough consensus for allowing other people to peek into\nthe .git/rebase-*/ directories, but I am inclined to say that it\nsounds more like a solution looking for a problem than a good idea\nto solve some concrete problem.\n\n\n\n\n"},{"id":"420906","messageId":"CABPp-BEzayZkEZ36CFNBGz9X3jv0MNUnWa2EhzKKmWcb83yRmw@mail.gmail.com","threadId":"55389","inReplyTo":"xmqqa6qg4d0i.fsf@gitster.g","subject":"Re: unifying sequencer's options persisting, was Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-04-02T22:18:09Z","receivedAt":"2021-04-02T22:18:23Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Apr 2, 2021 at 2:01 PM Junio C Hamano wrote:\n>\n> Phillip Wood writes:\n>\n> > I think we would save a lot by only syncing the state to disk when we\n> > stop or run an exec command (the state needs to be synced so exec\n> > commands can alter the todo list). In those cases we need to write the\n> > index and possibly run an external process so writing a couple of\n> > files is probably insignificant.\n>\n> The optimization opportunity of this may be a lot smaller than you\n> would think---you must cater to not just exec but hook scripts that\n> are run while a new commit is made, which means every step you'd\n> need to write anyway.\n\nFrom Documentation/git-rebase.txt, right after discussing how the\nbackends disagree on which hooks are called and how they are called:\n\n...In each case, the calling of these hooks was by accident of\nimplementation rather than by design (both backends were originally\nimplemented as shell scripts and happened to invoke other commands\nlike 'git checkout' or 'git commit' that would call the hooks). Both\nbackends should have the same behavior, though it is not entirely\nclear which, if any, is correct. We will likely make rebase stop\ncalling either of these hooks in the future.\n\n\nEven if others now disagree with the above, I know I can get a huge\nspeedup by changing sequencer to stop per-commit wasteful work (stop\nforking processes like \"git commit\", don't write control structures if\nthe rebase hasn't ended or hit a conflict, don't update the working\ncopy or index or reflog). It's enough of a speedup that if backward\ncompatibility won't allow such a method to be used by default, I'd\nstill make yet another backend that could be optionally used. And I'd\nhave the default rebase and cherry-pick start printing annoying\ndeprecation notices so that users become aware of a faster option.\nShould I go that route? t/helpers/test-fast-rebase.c already has a\ngood start...\n\n> > Where I think we can usefully consolidate is the one-line files which\n> > store the options rather than state - these are read an written much\n> > less frequently so I don't think they have much of a performance hit\n> > but it would be much nicer to just serialize the options to a single\n> > file.\n>\n> Would that break external scripts, hooks, etc.? I am not sure if we\n> even have any rough consensus for allowing other people to peek into\n> the .git/rebase-*/ directories, but I am inclined to say that it\n> sounds more like a solution looking for a problem than a good idea\n> to solve some concrete problem.\n\nTo be honest, my bigger complaint with the non-unified backend config\nis how far it bled into the code and how difficult it is to determine\nhow things are controlled and which sections of code are relevant for\nwhich types of operations. And how much of a pain I'm worried it'd\nmake the \"allow rebasing and cherry-picking in a bare repository (or\nfor an unchecked-out branch)\" functionality. Perhaps the code can be\ncleaned up without changing these on-disk structures, in which case my\nconcern for this point would lessen.\n"},{"id":"420908","messageId":"xmqq7dlk2ug9.fsf@gitster.g","threadId":"55389","inReplyTo":"CABPp-BEzayZkEZ36CFNBGz9X3jv0MNUnWa2EhzKKmWcb83yRmw@mail.gmail.com","subject":"Re: unifying sequencer's options persisting, was Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-02T22:27:34Z","receivedAt":"2021-04-02T22:27:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Even if others now disagree with the above, I know I can get a huge\n> speedup by changing sequencer to stop per-commit wasteful work (stop\n> forking processes like \"git commit\", don't write control structures if\n> the rebase hasn't ended or hit a conflict, don't update the working\n> copy or index or reflog). It's enough of a speedup that if backward\n> compatibility won't allow such a method to be used by default, I'd\n> still make yet another backend that could be optionally used. And I'd\n> have the default rebase and cherry-pick start printing annoying\n> deprecation notices so that users become aware of a faster option.\n\nA faster and less powerful interface is good; I doubt deprecation\nwould work well. If a workflow depends on things like post-commit\nhook, the affected users deserve some way to migrate to --exec or\nwhatever method to compensate the loss of functionality.\n"},{"id":"421206","messageId":"nycvar.QRO.7.76.6.2104080438080.54@tvgsbejvaqbjf.bet","threadId":"55389","inReplyTo":"xmqq7dlk2ug9.fsf@gitster.g","subject":"Re: unifying sequencer's options persisting, was Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-04-08T02:40:46Z","receivedAt":"2021-04-08T12:19:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 2 Apr 2021, Junio C Hamano wrote:\n\n> Elijah Newren writes:\n>\n> > Even if others now disagree with the above, I know I can get a huge\n> > speedup by changing sequencer to stop per-commit wasteful work (stop\n> > forking processes like \"git commit\", don't write control structures if\n> > the rebase hasn't ended or hit a conflict, don't update the working\n> > copy or index or reflog). It's enough of a speedup that if backward\n> > compatibility won't allow such a method to be used by default, I'd\n> > still make yet another backend that could be optionally used. And I'd\n> > have the default rebase and cherry-pick start printing annoying\n> > deprecation notices so that users become aware of a faster option.\n>\n> A faster and less powerful interface is good; I doubt deprecation\n> would work well. If a workflow depends on things like post-commit\n> hook, the affected users deserve some way to migrate to --exec or\n> whatever method to compensate the loss of functionality.\n\nI could imagine that there is opportunity to \"persist on disk only when\nneeded\". For example, if no `pre-commit` hook is installed that needs to\nbe run, there is no need to update the worktree nor HEAD until the rebase\nis done.\n\nAnd this type of `only write to disk when needed` functionality could\nprobably be abstracted away so much as to make the rest of the code\nlook elegant again.\n\nCiao,\nDscho\n"},{"id":"421248","messageId":"xmqqzgy8u0uy.fsf@gitster.g","threadId":"55389","inReplyTo":"nycvar.QRO.7.76.6.2104080438080.54@tvgsbejvaqbjf.bet","subject":"Re: unifying sequencer's options persisting, was Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T17:45:09Z","receivedAt":"2021-04-08T17:45:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > ... It's enough of a speedup that if backward\n>> > compatibility won't allow such a method to be used by default, I'd\n>> > still make yet another backend that could be optionally used. And I'd\n>> > have the default rebase and cherry-pick start printing annoying\n>> > deprecation notices so that users become aware of a faster option.\n>>\n>> A faster and less powerful interface is good; I doubt deprecation\n>> would work well. If a workflow depends on things like post-commit\n>> hook, the affected users deserve some way to migrate to --exec or\n>> whatever method to compensate the loss of functionality.\n>\n> I could imagine that there is opportunity to \"persist on disk only when\n> needed\". For example, if no `pre-commit` hook is installed that needs to\n> be run, there is no need to update the worktree nor HEAD until the rebase\n> is done.\n>\n> And this type of `only write to disk when needed` functionality could\n> probably be abstracted away so much as to make the rest of the code\n> look elegant again.\n\nYes, we are on the same page. What Elijah proposed as \"another\nbackend\" would unfortunately be different, and needs to be adjusted\nwith such an \"only when needed\" tweak. The check hopefully needs to\nbe done only once (e.g. do we have this hook enabled? do we have\nthat hook enabled? do we have a commit with this trait in the range\nbeing worked on? etc. etc.) upfront and for certain workflows may\nnot require any persistence.\n\nAnd until that happens, \"annoying deprecation notices\" will never be\na viable step to take.\n"},{"id":"421260","messageId":"CAP8UFD1WsjB_shHnRUcH3rnQjwh0yodrETq6zh39vnZnHQpVyw@mail.gmail.com","threadId":"55389","inReplyTo":"nycvar.QRO.7.76.6.2104080438080.54@tvgsbejvaqbjf.bet","subject":"Re: unifying sequencer's options persisting, was Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-04-08T19:58:40Z","receivedAt":"2021-04-08T19:58:57Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Thu, Apr 8, 2021 at 2:22 PM Johannes Schindelin\n wrote:\n>\n> On Fri, 2 Apr 2021, Junio C Hamano wrote:\n>\n> > Elijah Newren writes:\n> >\n> > > Even if others now disagree with the above, I know I can get a huge\n> > > speedup by changing sequencer to stop per-commit wasteful work (stop\n> > > forking processes like \"git commit\", don't write control structures if\n> > > the rebase hasn't ended or hit a conflict, don't update the working\n> > > copy or index or reflog).\n\nIt would be interesting to know which of these updates (working copy,\nindex or reflog) is the most costly. Of course the best would be to\ncompare the costs when updates are done at each step vs only at the\nend.\n\nSome years ago I worked on the split index as some tests showed that,\nfor really big repos with a big index, it could significantly speed up\nrebases. So I have the, perhaps wrong, impression that most of the\ngain would be related to the index.\n\n> > > It's enough of a speedup that if backward\n> > > compatibility won't allow such a method to be used by default, I'd\n> > > still make yet another backend that could be optionally used. And I'd\n> > > have the default rebase and cherry-pick start printing annoying\n> > > deprecation notices so that users become aware of a faster option.\n> >\n> > A faster and less powerful interface is good; I doubt deprecation\n> > would work well. If a workflow depends on things like post-commit\n> > hook, the affected users deserve some way to migrate to --exec or\n> > whatever method to compensate the loss of functionality.\n>\n> I could imagine that there is opportunity to \"persist on disk only when\n> needed\". For example, if no `pre-commit` hook is installed that needs to\n> be run, there is no need to update the worktree nor HEAD until the rebase\n> is done.\n>\n> And this type of `only write to disk when needed` functionality could\n> probably be abstracted away so much as to make the rest of the code\n> look elegant again.\n\nYeah, I agree with this approach too.\n\nIf the split index could also become the default one day, I guess we\ncould be doing pretty well even when some hooks are installed.\n"},{"id":"421395","messageId":"nycvar.QRO.7.76.6.2104091548500.54@tvgsbejvaqbjf.bet","threadId":"55389","inReplyTo":"CAP8UFD1WsjB_shHnRUcH3rnQjwh0yodrETq6zh39vnZnHQpVyw@mail.gmail.com","subject":"Re: unifying sequencer's options persisting, was Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-04-09T13:53:23Z","receivedAt":"2021-04-09T13:53:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Christian,\n\nOn Thu, 8 Apr 2021, Christian Couder wrote:\n\n> On Thu, Apr 8, 2021 at 2:22 PM Johannes Schindelin\n> wrote:\n> >\n> > On Fri, 2 Apr 2021, Junio C Hamano wrote:\n> >\n> > > Elijah Newren writes:\n> > >\n> > > > Even if others now disagree with the above, I know I can get a huge\n> > > > speedup by changing sequencer to stop per-commit wasteful work (stop\n> > > > forking processes like \"git commit\", don't write control structures if\n> > > > the rebase hasn't ended or hit a conflict, don't update the working\n> > > > copy or index or reflog).\n>\n> It would be interesting to know which of these updates (working copy,\n> index or reflog) is the most costly. Of course the best would be to\n> compare the costs when updates are done at each step vs only at the\n> end.\n\nI would bet that the worktree updates are the most costly thing.\n\nNote: there is also the step where new objects are written to the object\ndatabase, but that is a step that we can _not_ skip.\n\n> If the split index could also become the default one day, I guess we\n> could be doing pretty well even when some hooks are installed.\n\nHmm. My opinion about the split index does not match yours.\n\nI'd much rather see us tackle the challenge of allowing truly incremental\nindex updates. That \"split index\" feature feels like a bandaid that tries\nto do sort of an incremental update, and fails at it because it is not\ntruly incremental. Granted, that would look much more like a real database\nand is much, much harder than what the split index code does (think\ne.g. locking).\n\nCiao,\nDscho\n"}]}