{"thread":{"id":"49995","subject":"[PATCH 0/3] rebase: offer to reschedule failed exec commands automatically","startedAt":"2018-12-10T19:05:00Z","lastAt":"2021-04-15T15:24:42Z","messageCount":28,"participants":["Johannes Schindelin via GitGitGadget","Johannes Sixt","Stefan Beller","Elijah Newren","Junio C Hamano","Johannes Schindelin","Ævar Arnfjörð Bjarmason","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"364969","messageId":"pull.90.git.gitgitgadget@gmail.com","threadId":"49995","inReplyTo":null,"subject":"[PATCH 0/3] rebase: offer to reschedule failed exec commands automatically","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-12-10T19:04:57Z","receivedAt":"2018-12-10T19:05:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The idea was brought up by Paul Morelle.\n\nTo be honest, this idea of rescheduling a failed exec makes so much sense\nthat I wish we had done this from the get-go.\n\nSo let's do the next best thing: implement a command-line option and a\nconfig setting to make it so.\n\nThe obvious intent behind that config setting is to not only give users a\nway to opt into that behavior already, but also to make it possible to flip\nthe default at a later date, after the feature has been battle-tested and\nafter deprecating the non-rescheduling behavior for a couple of Git\nversions.\n\nIf the team agrees with my reasoning, then the 3rd patch (introducing -y\n as a shortcut for --reschedule-failed-exec -x ) might not make\nmuch sense, as it would introduce a short option that would become obsolete\nsoon.\n\nJohannes Schindelin (3):\n rebase: introduce --reschedule-failed-exec\n rebase: add a config option to default to --reschedule-failed-exec\n rebase: introduce a shortcut for --reschedule-failed-exec\n\n Documentation/config/rebase.txt | 5 ++++\n Documentation/git-rebase.txt | 11 +++++++++\n builtin/rebase--interactive.c | 2 ++\n builtin/rebase.c | 42 ++++++++++++++++++++++++++++++++-\n git-legacy-rebase.sh | 24 ++++++++++++++++++-\n git-rebase--common.sh | 1 +\n sequencer.c | 13 +++++++---\n sequencer.h | 1 +\n t/t3418-rebase-continue.sh | 14 +++++++++++\n 9 files changed, 108 insertions(+), 5 deletions(-)\n\n\nbase-commit: a1598010f775d82b5adf12c29d0f5bc9b41434c6\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-90%2Fdscho%2Freschedule-failed-exec-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-90/dscho/reschedule-failed-exec-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/90\n-- \ngitgitgadget\n"},{"id":"364970","messageId":"873d1fda948855510b07f9cb75d374c03d60a94e.1544468695.git.gitgitgadget@gmail.com","threadId":"49995","inReplyTo":"pull.90.git.gitgitgadget@gmail.com","subject":"[PATCH 2/3] rebase: add a config option to default to --reschedule-failed-exec","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-12-10T19:04:59Z","receivedAt":"2018-12-10T19:05:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt would be cumbersome to type out that option all the time, so let's\noffer the convenience of a config setting: rebase.rescheduleFailedExec.\n\nBesides, this opens the door to changing the default in a future version\nof Git: it does make some sense to reschedule failed `exec` commands by\ndefault (and if we could go back in time when the `exec` command was\ninvented, we probably would change that default right from the start).\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/config/rebase.txt | 5 +++++\n builtin/rebase.c | 5 +++++\n git-legacy-rebase.sh | 2 ++\n t/t3418-rebase-continue.sh | 7 ++++++-\n 4 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt\nindex f079bf6b7e..331d250e04 100644\n--- a/Documentation/config/rebase.txt\n+++ b/Documentation/config/rebase.txt\n@@ -64,3 +64,8 @@ instead of:\n -------------------------------------------\n +\n Defaults to false.\n+\n+rebase.rescheduleFailedExec::\n+\tAutomatically reschedule `exec` commands that failed. This only makes\n+\tsense in interactive mode (or when an `--exec` option was provided).\n+\tThis is the same as specifying the `--reschedule-failed-exec` option.\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 6d556fc6c8..06e450b537 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -677,6 +677,11 @@ static int rebase_config(const char *var, const char *value, void *data)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"rebase.reschedulefailedexec\")) {\n+\t\topts->reschedule_failed_exec = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(var, value, data);\n }\n \ndiff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh\nindex 5f0f0c5ab8..1b268a5fcc 100755\n--- a/git-legacy-rebase.sh\n+++ b/git-legacy-rebase.sh\n@@ -99,6 +99,8 @@ case \"$(git config --bool commit.gpgsign)\" in\n true)\tgpg_sign_opt=-S ;;\n *)\tgpg_sign_opt= ;;\n esac\n+test \"$(git config --bool rebase.reschedulefailedexec)\" = \"true\" &&\n+reschedule_failed_exec=--reschedule-failed-exec\n . git-rebase--common\n \n read_basic_state () {\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 54b26a9284..bdaa511bb0 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -257,7 +257,12 @@ test_expect_success 'the todo command \"break\" works' '\n test_expect_success '--reschedule-failed-exec' '\n \ttest_when_finished \"git rebase --abort\" &&\n \ttest_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&\n-\tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo\n+\tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo &&\n+\tgit rebase --abort &&\n+\ttest_must_fail git -c rebase.rescheduleFailedExec=true \\\n+\t\trebase -x false HEAD^ 2>err &&\n+\tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo &&\n+\ttest_i18ngrep \"has been rescheduled\" err\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"364971","messageId":"05d8792d12e692eeefa0021e8686b7211a055593.1544468695.git.gitgitgadget@gmail.com","threadId":"49995","inReplyTo":"pull.90.git.gitgitgadget@gmail.com","subject":"[PATCH 1/3] rebase: introduce --reschedule-failed-exec","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-12-10T19:04:58Z","receivedAt":"2018-12-10T19:05:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nA common use case for the `--exec` option is to verify that each commit\nin a topic branch compiles cleanly, via `git rebase -x make `.\n\nHowever, when an `exec` in such a rebase fails, it is not re-scheduled,\nwhich in this instance is not particularly helpful.\n\nLet's offer a flag to reschedule failed `exec` commands.\n\nBased on an idea by Paul Morelle.\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/git-rebase.txt | 5 +++++\n builtin/rebase--interactive.c | 2 ++\n builtin/rebase.c | 16 +++++++++++++++-\n git-legacy-rebase.sh | 16 +++++++++++++++-\n git-rebase--common.sh | 1 +\n sequencer.c | 13 ++++++++++---\n sequencer.h | 1 +\n t/t3418-rebase-continue.sh | 6 ++++++\n 8 files changed, 55 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 80793bad8d..9dd68f77f6 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -501,6 +501,11 @@ See also INCOMPATIBLE OPTIONS below.\n \twith care: the final stash application after a successful\n \trebase might result in non-trivial conflicts.\n \n+--reschedule-failed-exec::\n+--no-reschedule-failed-exec::\n+\tAutomatically reschedule `exec` commands that failed. This only makes\n+\tsense in interactive mode (or when an `--exec` option was provided).\n+\n INCOMPATIBLE OPTIONS\n --------------------\n \ndiff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c\nindex a2ab68ed06..a9ab009fdb 100644\n--- a/builtin/rebase--interactive.c\n+++ b/builtin/rebase--interactive.c\n@@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"onto-name\", &onto_name, N_(\"onto-name\"), N_(\"onto name\")),\n \t\tOPT_STRING(0, \"cmd\", &cmd, N_(\"cmd\"), N_(\"the command to run\")),\n \t\tOPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),\n+\t\tOPT_BOOL(0, \"reschedule-failed-exec\", &opts.reschedule_failed_exec,\n+\t\t\t N_(\"automatically re-schedule any `exec` that fails\")),\n \t\tOPT_END()\n \t};\n \ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 5b3e5baec8..6d556fc6c8 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -104,6 +104,7 @@ struct rebase_options {\n \tint rebase_merges, rebase_cousins;\n \tchar *strategy, *strategy_opts;\n \tstruct strbuf git_format_patch_opt;\n+\tint reschedule_failed_exec;\n };\n \n static int is_interactive(struct rebase_options *opts)\n@@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options *opts)\n \t\t\targv_array_push(&child.args, opts->gpg_sign_opt);\n \t\tif (opts->signoff)\n \t\t\targv_array_push(&child.args, \"--signoff\");\n+\t\tif (opts->reschedule_failed_exec)\n+\t\t\targv_array_push(&child.args, \"--reschedule-failed-exec\");\n \n \t\tstatus = run_command(&child);\n \t\tgoto finished_rebase;\n@@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t \"strategy\")),\n \t\tOPT_BOOL(0, \"root\", &options.root,\n \t\t\t N_(\"rebase all reachable commits up to the root(s)\")),\n+\t\tOPT_BOOL(0, \"reschedule-failed-exec\",\n+\t\t\t &options.reschedule_failed_exec,\n+\t\t\t N_(\"automatically re-schedule any `exec` that fails\")),\n \t\tOPT_END(),\n \t};\n \tint i;\n@@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \t}\n \n+\tif (options.reschedule_failed_exec && !is_interactive(&options))\n+\t\tdie(_(\"--reschedule-failed-exec requires an interactive rebase\"));\n+\n \tif (options.git_am_opts.argc) {\n \t\t/* all am options except -q are compatible only with --am */\n \t\tfor (i = options.git_am_opts.argc - 1; i >= 0; i--)\n@@ -1220,7 +1229,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\toptions.flags |= REBASE_FORCE;\n \t}\n \n-\tif (options.type == REBASE_PRESERVE_MERGES)\n+\tif (options.type == REBASE_PRESERVE_MERGES) {\n \t\t/*\n \t\t * Note: incompatibility with --signoff handled in signoff block above\n \t\t * Note: incompatibility with --interactive is just a strong warning;\n@@ -1230,6 +1239,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"error: cannot combine '--preserve-merges' with \"\n \t\t\t \"'--rebase-merges'\"));\n \n+\t\tif (options.reschedule_failed_exec)\n+\t\t\tdie(_(\"error: cannot combine '--preserve-merges' with \"\n+\t\t\t \"'--reschedule-failed-exec'\"));\n+\t}\n+\n \tif (options.rebase_merges) {\n \t\tif (strategy_options.nr)\n \t\t\tdie(_(\"error: cannot combine '--rebase-merges' with \"\ndiff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh\nindex b97ffdc9dd..5f0f0c5ab8 100755\n--- a/git-legacy-rebase.sh\n+++ b/git-legacy-rebase.sh\n@@ -48,6 +48,7 @@ skip! skip current patch and continue\n edit-todo! edit the todo list during an interactive rebase\n quit! abort but keep HEAD where it is\n show-current-patch! show the patch file being applied or merged\n+reschedule-failed-exec automatically reschedule failed exec commands\n \"\n . git-sh-setup\n set_reflog_action rebase\n@@ -92,6 +93,7 @@ autosquash=\n keep_empty=\n allow_empty_message=--allow-empty-message\n signoff=\n+reschedule_failed_exec=\n test \"$(git config --bool rebase.autosquash)\" = \"true\" && autosquash=t\n case \"$(git config --bool commit.gpgsign)\" in\n true)\tgpg_sign_opt=-S ;;\n@@ -126,6 +128,8 @@ read_basic_state () {\n \t\tsignoff=\"$(cat \"$state_dir\"/signoff)\"\n \t\tforce_rebase=t\n \t}\n+\ttest -f \"$state_dir\"/reschedule-failed-exec &&\n+\t\treschedule_failed_exec=t\n }\n \n finish_rebase () {\n@@ -163,7 +167,8 @@ run_interactive () {\n \t\t\"$allow_empty_message\" \"$autosquash\" \"$verbose\" \\\n \t\t\"$force_rebase\" \"$onto_name\" \"$head_name\" \"$strategy\" \\\n \t\t\"$strategy_opts\" \"$cmd\" \"$switch_to\" \\\n-\t\t\"$allow_rerere_autoupdate\" \"$gpg_sign_opt\" \"$signoff\"\n+\t\t\"$allow_rerere_autoupdate\" \"$gpg_sign_opt\" \"$signoff\" \\\n+\t\t\"$reschedule_failed_exec\"\n }\n \n run_specific_rebase () {\n@@ -378,6 +383,12 @@ do\n \t--gpg-sign=*)\n \t\tgpg_sign_opt=\"-S${1#--gpg-sign=}\"\n \t\t;;\n+\t--reschedule-failed-exec)\n+\t\treschedule_failed_exec=--reschedule-failed-exec\n+\t\t;;\n+\t--no-reschedule-failed-exec)\n+\t\treschedule_failed_exec=\n+\t\t;;\n \t--)\n \t\tshift\n \t\tbreak\n@@ -534,6 +545,9 @@ then\n \t# git-rebase.txt caveats with \"unless you know what you are doing\"\n \ttest -n \"$rebase_merges\" &&\n \t\tdie \"$(gettext \"error: cannot combine '--preserve-merges' with '--rebase-merges'\")\"\n+\n+\ttest -n \"$reschedule_failed_exec\" &&\n+\t\tdie \"$(gettext \"error: cannot combine '--preserve-merges' with '--reschedule-failed-exec'\")\"\n fi\n \n if test -n \"$rebase_merges\"\ndiff --git a/git-rebase--common.sh b/git-rebase--common.sh\nindex 7e39d22871..a8a44608e0 100644\n--- a/git-rebase--common.sh\n+++ b/git-rebase--common.sh\n@@ -19,6 +19,7 @@ write_basic_state () {\n \t\t\"$state_dir\"/allow_rerere_autoupdate\n \ttest -n \"$gpg_sign_opt\" && echo \"$gpg_sign_opt\" > \"$state_dir\"/gpg_sign_opt\n \ttest -n \"$signoff\" && echo \"$signoff\" >\"$state_dir\"/signoff\n+\ttest -n \"$reschedule_failed_exec\" && : > \"$state_dir\"/reschedule-failed-exec\n }\n \n apply_autostash () {\ndiff --git a/sequencer.c b/sequencer.c\nindex e1a4dd15f1..69bee63e11 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, \"rebase-merge/strategy\")\n static GIT_PATH_FUNC(rebase_path_strategy_opts, \"rebase-merge/strategy_opts\")\n static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, \"rebase-merge/allow_rerere_autoupdate\")\n static GIT_PATH_FUNC(rebase_path_quiet, \"rebase-merge/quiet\")\n+static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, \"rebase-merge/reschedule-failed-exec\")\n \n static int git_sequencer_config(const char *k, const char *v, void *cb)\n {\n@@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts)\n \t\t\topts->signoff = 1;\n \t\t}\n \n+\t\tif (file_exists(rebase_path_reschedule_failed_exec()))\n+\t\t\topts->reschedule_failed_exec = 1;\n+\n \t\tread_strategy_opts(opts, &buf);\n \t\tstrbuf_release(&buf);\n \n@@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\twrite_file(rebase_path_gpg_sign_opt(), \"-S%s\\n\", opts->gpg_sign);\n \tif (opts->signoff)\n \t\twrite_file(rebase_path_signoff(), \"--signoff\\n\");\n+\tif (opts->reschedule_failed_exec)\n+\t\twrite_file(rebase_path_reschedule_failed_exec(), \"%s\", \"\");\n \n \treturn 0;\n }\n@@ -3586,9 +3592,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)\n \t\t\t*end_of_arg = saved;\n \n \t\t\t/* Reread the todo file if it has changed. */\n-\t\t\tif (res)\n-\t\t\t\t; /* fall through */\n-\t\t\telse if (stat(get_todo_path(opts), &st))\n+\t\t\tif (res) {\n+\t\t\t\tif (opts->reschedule_failed_exec)\n+\t\t\t\t\treschedule = 1;\n+\t\t\t} else if (stat(get_todo_path(opts), &st))\n \t\t\t\tres = error_errno(_(\"could not stat '%s'\"),\n \t\t\t\t\t\t get_todo_path(opts));\n \t\t\telse if (match_stat_data(&todo_list->stat, &st)) {\ndiff --git a/sequencer.h b/sequencer.h\nindex 5071a73563..1f865dae26 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -39,6 +39,7 @@ struct replay_opts {\n \tint allow_empty_message;\n \tint keep_redundant_commits;\n \tint verbose;\n+\tint reschedule_failed_exec;\n \n \tint mainline;\n \ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 0210b2ac6f..54b26a9284 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -254,4 +254,10 @@ test_expect_success 'the todo command \"break\" works' '\n \ttest_path_is_file execed\n '\n \n+test_expect_success '--reschedule-failed-exec' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\ttest_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&\n+\tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"364972","messageId":"86ed094426bd7d18f24ede841e08756efcf30caa.1544468696.git.gitgitgadget@gmail.com","threadId":"49995","inReplyTo":"pull.90.git.gitgitgadget@gmail.com","subject":"[PATCH 3/3] rebase: introduce a shortcut for --reschedule-failed-exec","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-12-10T19:05:01Z","receivedAt":"2018-12-10T19:05:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt is a bit cumbersome to write out the `--reschedule-failed-exec`\noption before `-x ` all the time; let's introduce a convenient\noption to do both at the same time: `-y `.\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/git-rebase.txt | 6 ++++++\n builtin/rebase.c | 21 +++++++++++++++++++++\n git-legacy-rebase.sh | 6 ++++++\n t/t3418-rebase-continue.sh | 3 +++\n 4 files changed, 36 insertions(+)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 9dd68f77f6..99ca589ffc 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -462,6 +462,12 @@ without an explicit `--interactive`.\n +\n See also INCOMPATIBLE OPTIONS below.\n \n+-y ::\n+\tThis is the same as passing `--reschedule-failed-exec` before\n+\t`-x `, i.e. it appends the specified `exec` command and\n+\tturns on the mode where failed `exec` commands are automatically\n+\trescheduled.\n+\n --root::\n \tRebase all commits reachable from , instead of\n \tlimiting them with an . This allows you to rebase\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 06e450b537..b707ccf00f 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -754,6 +754,23 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n+struct opt_y {\n+\tstruct string_list *list;\n+\tstruct rebase_options *options;\n+};\n+\n+static int parse_opt_y(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct opt_y *o = opt->value;\n+\n+\tif (unset || !arg)\n+\t\treturn -1;\n+\n+\to->options->reschedule_failed_exec = 1;\n+\tstring_list_append(o->list, arg);\n+\treturn 0;\n+}\n+\n static void NORETURN error_on_missing_default_upstream(void)\n {\n \tstruct branch *current_branch = branch_get(NULL);\n@@ -817,6 +834,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tstruct string_list strategy_options = STRING_LIST_INIT_NODUP;\n \tstruct object_id squash_onto;\n \tchar *squash_onto_name = NULL;\n+\tstruct opt_y opt_y = { .list = &exec, .options = &options };\n \tstruct option builtin_rebase_options[] = {\n \t\tOPT_STRING(0, \"onto\", &options.onto_name,\n \t\t\t N_(\"revision\"),\n@@ -894,6 +912,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING_LIST('x', \"exec\", &exec, N_(\"exec\"),\n \t\t\t\tN_(\"add exec lines after each commit of the \"\n \t\t\t\t \"editable list\")),\n+\t\t{ OPTION_CALLBACK, 'y', NULL, &opt_y, N_(\"\"),\n+\t\t\tN_(\"same as --reschedule-failed-exec -x \"),\n+\t\t\tPARSE_OPT_NONEG, parse_opt_y },\n \t\tOPT_BOOL(0, \"allow-empty-message\",\n \t\t\t &options.allow_empty_message,\n \t\t\t N_(\"allow rebasing commits with empty messages\")),\ndiff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh\nindex 1b268a5fcc..8048891fed 100755\n--- a/git-legacy-rebase.sh\n+++ b/git-legacy-rebase.sh\n@@ -26,6 +26,7 @@ f,force-rebase! cherry-pick all commits, even if unchanged\n m,merge! use merging strategies to rebase\n i,interactive! let the user edit the list of commits to rebase\n x,exec=! add exec lines after each commit of the editable list\n+y=! same as --reschedule-failed-exec -x\n k,keep-empty\t preserve empty commits during rebase\n allow-empty-message allow rebasing commits with empty messages\n stat! display a diffstat of what changed upstream\n@@ -262,6 +263,11 @@ do\n \t\tcmd=\"${cmd}exec ${1#--exec=}${LF}\"\n \t\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\n \t\t;;\n+\t-y*)\n+\t\treschedule_failed_exec=--reschedule-failed-exec\n+\t\tcmd=\"${cmd}exec ${1#-y}${LF}\"\n+\t\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\n+\t\t;;\n \t--interactive)\n \t\tinteractive_rebase=explicit\n \t\t;;\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex bdaa511bb0..25aaacacfc 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -262,6 +262,9 @@ test_expect_success '--reschedule-failed-exec' '\n \ttest_must_fail git -c rebase.rescheduleFailedExec=true \\\n \t\trebase -x false HEAD^ 2>err &&\n \tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo &&\n+\ttest_i18ngrep \"has been rescheduled\" err &&\n+\tgit rebase --abort &&\n+\ttest_must_fail git rebase -y false HEAD^ 2>err &&\n \ttest_i18ngrep \"has been rescheduled\" err\n '\n \n-- \ngitgitgadget\n"},{"id":"364983","messageId":"d2c317fd-d10a-19c1-8b4f-a7311c69a52f@kdbg.org","threadId":"49995","inReplyTo":"pull.90.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-12-10T22:08:06Z","receivedAt":"2018-12-10T22:08:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.12.18 um 20:04 schrieb Johannes Schindelin via GitGitGadget:\n> The idea was brought up by Paul Morelle.\n> \n> To be honest, this idea of rescheduling a failed exec makes so much sense\n> that I wish we had done this from the get-go.\n\nThe status quo was actually not that bad a decision, because it made 'x \nfalse' as a substitute for 'break' very convenient.\n\nBut now that we have a real 'break', I'm very much in favor of flipping \nthe behavior over to rescheduling. (I'm actually not a user of the \nfeature, but the proposed behavior is so compellingly logical.)\n\n-- Hannes\n"},{"id":"364987","messageId":"CAGZ79kY18SCaCBvkWyeVd+oeJ4EhoJK4=QxGhJ9a77iX2sR8ew@mail.gmail.com","threadId":"49995","inReplyTo":"d2c317fd-d10a-19c1-8b4f-a7311c69a52f@kdbg.org","subject":"Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-10T22:56:47Z","receivedAt":"2018-12-10T22:57:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 10, 2018 at 2:08 PM Johannes Sixt wrote:\n>\n> Am 10.12.18 um 20:04 schrieb Johannes Schindelin via GitGitGadget:\n> > The idea was brought up by Paul Morelle.\n> >\n> > To be honest, this idea of rescheduling a failed exec makes so much sense\n> > that I wish we had done this from the get-go.\n>\n> The status quo was actually not that bad a decision, because it made 'x\n> false' as a substitute for 'break' very convenient.\n>\n> But now that we have a real 'break', I'm very much in favor of flipping\n> the behavior over to rescheduling. (I'm actually not a user of the\n> feature, but the proposed behavior is so compellingly logical.)\n\nIn rare cases I had commands that may be dangerous if rerun,\nbut I'd just not run them with -y but with -x.\n\nThat brings me to some confusion I had in the last patch:\nTo catch a flaky test I surely would be tempted to\n git rebase -x make -y \"make test\"\nbut that might reschedule a compile failure as well,\nas a single -y has implications on all other -x's.\n\nI wonder if it might be better to push this mechanism\none layer down: Instead of having a flag that changes\nthe behavior of the \"exec\" instructions and having a\nhandy '-y' short cut for the new mode, we'd rather have\na new type of command that executes&retries a command\n(\"exnrt\", 'n'), which can still get the '-y' command line flag,\nbut more importantly by having 2 separate sets of\ncommands we'd have one set that is a one-shot, and the\nother that is retried. Then we can teach the user which\nis safe and which isn't for rescheduling.\n\nBy having two classes, I would anticipate fewer compatibility\nissues ('\"Exec\" behaves differently, and I forgot I had turned\non the rescheduling').\n\nTalking about rescheduling: In the above example the flaky\ntest can flake more than once, so I'd be stuck with keeping\n'git rebase --continue'ing after I see the test flaked once again.\n\nMy workflow with interactive rebase and fixing up things as I go\nalways involves a manual final \"make test\" to check \"for real\",\nwhich I could lose now, which is nice.\n\nThanks,\nStefan\n"},{"id":"364990","messageId":"CABPp-BGoJxKFFu+JE9n52t8Fygzf0+mpPSOo8rftR2v0_i+eZw@mail.gmail.com","threadId":"49995","inReplyTo":"05d8792d12e692eeefa0021e8686b7211a055593.1544468695.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] rebase: introduce --reschedule-failed-exec","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-12-10T23:18:06Z","receivedAt":"2018-12-10T23:18:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Dec 10, 2018 at 1:18 PM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> A common use case for the `--exec` option is to verify that each commit\n> in a topic branch compiles cleanly, via `git rebase -x make `.\n>\n> However, when an `exec` in such a rebase fails, it is not re-scheduled,\n> which in this instance is not particularly helpful.\n>\n> Let's offer a flag to reschedule failed `exec` commands.\n>\n> Based on an idea by Paul Morelle.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> Documentation/git-rebase.txt | 5 +++++\n> builtin/rebase--interactive.c | 2 ++\n> builtin/rebase.c | 16 +++++++++++++++-\n> git-legacy-rebase.sh | 16 +++++++++++++++-\n> git-rebase--common.sh | 1 +\n> sequencer.c | 13 ++++++++++---\n> sequencer.h | 1 +\n> t/t3418-rebase-continue.sh | 6 ++++++\n> 8 files changed, 55 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 80793bad8d..9dd68f77f6 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -501,6 +501,11 @@ See also INCOMPATIBLE OPTIONS below.\n> with care: the final stash application after a successful\n> rebase might result in non-trivial conflicts.\n>\n> +--reschedule-failed-exec::\n> +--no-reschedule-failed-exec::\n> + Automatically reschedule `exec` commands that failed. This only makes\n> + sense in interactive mode (or when an `--exec` option was provided).\n> +\n> INCOMPATIBLE OPTIONS\n> --------------------\n>\n> diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c\n> index a2ab68ed06..a9ab009fdb 100644\n> --- a/builtin/rebase--interactive.c\n> +++ b/builtin/rebase--interactive.c\n> @@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)\n> OPT_STRING(0, \"onto-name\", &onto_name, N_(\"onto-name\"), N_(\"onto name\")),\n> OPT_STRING(0, \"cmd\", &cmd, N_(\"cmd\"), N_(\"the command to run\")),\n> OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),\n> + OPT_BOOL(0, \"reschedule-failed-exec\", &opts.reschedule_failed_exec,\n> + N_(\"automatically re-schedule any `exec` that fails\")),\n> OPT_END()\n> };\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 5b3e5baec8..6d556fc6c8 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -104,6 +104,7 @@ struct rebase_options {\n> int rebase_merges, rebase_cousins;\n> char *strategy, *strategy_opts;\n> struct strbuf git_format_patch_opt;\n> + int reschedule_failed_exec;\n> };\n>\n> static int is_interactive(struct rebase_options *opts)\n> @@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options *opts)\n> argv_array_push(&child.args, opts->gpg_sign_opt);\n> if (opts->signoff)\n> argv_array_push(&child.args, \"--signoff\");\n> + if (opts->reschedule_failed_exec)\n> + argv_array_push(&child.args, \"--reschedule-failed-exec\");\n>\n> status = run_command(&child);\n> goto finished_rebase;\n> @@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \"strategy\")),\n> OPT_BOOL(0, \"root\", &options.root,\n> N_(\"rebase all reachable commits up to the root(s)\")),\n> + OPT_BOOL(0, \"reschedule-failed-exec\",\n> + &options.reschedule_failed_exec,\n> + N_(\"automatically re-schedule any `exec` that fails\")),\n> OPT_END(),\n> };\n> int i;\n> @@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> break;\n> }\n>\n> + if (options.reschedule_failed_exec && !is_interactive(&options))\n> + die(_(\"--reschedule-failed-exec requires an interactive rebase\"));\n> +\n\nI was surprised at first that you checked is_interactive() rather than\nchecking for --exec being specified. But I guess this is because\nusers can manually specify 'exec' lines.\n\nWhat if the user specifies an implicitly interactive rebase (i.e. no\nediting of the todo list, such as with --rebase-merges or\n--keep-empty, or soon --strategy or --strategy-option) and also\ndoesn't specify --exec?\n\n> if (options.git_am_opts.argc) {\n> /* all am options except -q are compatible only with --am */\n> for (i = options.git_am_opts.argc - 1; i >= 0; i--)\n> @@ -1220,7 +1229,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> options.flags |= REBASE_FORCE;\n> }\n>\n> - if (options.type == REBASE_PRESERVE_MERGES)\n> + if (options.type == REBASE_PRESERVE_MERGES) {\n> /*\n> * Note: incompatibility with --signoff handled in signoff block above\n> * Note: incompatibility with --interactive is just a strong warning;\n> @@ -1230,6 +1239,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> die(_(\"error: cannot combine '--preserve-merges' with \"\n> \"'--rebase-merges'\"));\n>\n> + if (options.reschedule_failed_exec)\n> + die(_(\"error: cannot combine '--preserve-merges' with \"\n> + \"'--reschedule-failed-exec'\"));\n> + }\n> +\n> if (options.rebase_merges) {\n> if (strategy_options.nr)\n> die(_(\"error: cannot combine '--rebase-merges' with \"\n> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh\n> index b97ffdc9dd..5f0f0c5ab8 100755\n> --- a/git-legacy-rebase.sh\n> +++ b/git-legacy-rebase.sh\n> @@ -48,6 +48,7 @@ skip! skip current patch and continue\n> edit-todo! edit the todo list during an interactive rebase\n> quit! abort but keep HEAD where it is\n> show-current-patch! show the patch file being applied or merged\n> +reschedule-failed-exec automatically reschedule failed exec commands\n> \"\n> . git-sh-setup\n> set_reflog_action rebase\n> @@ -92,6 +93,7 @@ autosquash=\n> keep_empty=\n> allow_empty_message=--allow-empty-message\n> signoff=\n> +reschedule_failed_exec=\n> test \"$(git config --bool rebase.autosquash)\" = \"true\" && autosquash=t\n> case \"$(git config --bool commit.gpgsign)\" in\n> true) gpg_sign_opt=-S ;;\n> @@ -126,6 +128,8 @@ read_basic_state () {\n> signoff=\"$(cat \"$state_dir\"/signoff)\"\n> force_rebase=t\n> }\n> + test -f \"$state_dir\"/reschedule-failed-exec &&\n> + reschedule_failed_exec=t\n> }\n>\n> finish_rebase () {\n> @@ -163,7 +167,8 @@ run_interactive () {\n> \"$allow_empty_message\" \"$autosquash\" \"$verbose\" \\\n> \"$force_rebase\" \"$onto_name\" \"$head_name\" \"$strategy\" \\\n> \"$strategy_opts\" \"$cmd\" \"$switch_to\" \\\n> - \"$allow_rerere_autoupdate\" \"$gpg_sign_opt\" \"$signoff\"\n> + \"$allow_rerere_autoupdate\" \"$gpg_sign_opt\" \"$signoff\" \\\n> + \"$reschedule_failed_exec\"\n> }\n>\n> run_specific_rebase () {\n> @@ -378,6 +383,12 @@ do\n> --gpg-sign=*)\n> gpg_sign_opt=\"-S${1#--gpg-sign=}\"\n> ;;\n> + --reschedule-failed-exec)\n> + reschedule_failed_exec=--reschedule-failed-exec\n> + ;;\n> + --no-reschedule-failed-exec)\n> + reschedule_failed_exec=\n> + ;;\n> --)\n> shift\n> break\n> @@ -534,6 +545,9 @@ then\n> # git-rebase.txt caveats with \"unless you know what you are doing\"\n> test -n \"$rebase_merges\" &&\n> die \"$(gettext \"error: cannot combine '--preserve-merges' with '--rebase-merges'\")\"\n> +\n> + test -n \"$reschedule_failed_exec\" &&\n> + die \"$(gettext \"error: cannot combine '--preserve-merges' with '--reschedule-failed-exec'\")\"\n> fi\n>\n> if test -n \"$rebase_merges\"\n\nIn the builtin rebase, you checked that --reschedule-failed-exec had\nto be used with an interactive rebase. Here in the legacy rebase you\nhave no such check at all.\n\nNot sure if that's an oversight, or if we're at the point where we\njust start intentionally allowing legacy rebase to lag and soon throw\nit out. (When do we get to that point?)\n\n> diff --git a/git-rebase--common.sh b/git-rebase--common.sh\n> index 7e39d22871..a8a44608e0 100644\n> --- a/git-rebase--common.sh\n> +++ b/git-rebase--common.sh\n> @@ -19,6 +19,7 @@ write_basic_state () {\n> \"$state_dir\"/allow_rerere_autoupdate\n> test -n \"$gpg_sign_opt\" && echo \"$gpg_sign_opt\" > \"$state_dir\"/gpg_sign_opt\n> test -n \"$signoff\" && echo \"$signoff\" >\"$state_dir\"/signoff\n> + test -n \"$reschedule_failed_exec\" && : > \"$state_dir\"/reschedule-failed-exec\n> }\n>\n> apply_autostash () {\n> diff --git a/sequencer.c b/sequencer.c\n> index e1a4dd15f1..69bee63e11 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, \"rebase-merge/strategy\")\n> static GIT_PATH_FUNC(rebase_path_strategy_opts, \"rebase-merge/strategy_opts\")\n> static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, \"rebase-merge/allow_rerere_autoupdate\")\n> static GIT_PATH_FUNC(rebase_path_quiet, \"rebase-merge/quiet\")\n> +static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, \"rebase-merge/reschedule-failed-exec\")\n>\n> static int git_sequencer_config(const char *k, const char *v, void *cb)\n> {\n> @@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts)\n> opts->signoff = 1;\n> }\n>\n> + if (file_exists(rebase_path_reschedule_failed_exec()))\n> + opts->reschedule_failed_exec = 1;\n> +\n> read_strategy_opts(opts, &buf);\n> strbuf_release(&buf);\n>\n> @@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n> write_file(rebase_path_gpg_sign_opt(), \"-S%s\\n\", opts->gpg_sign);\n> if (opts->signoff)\n> write_file(rebase_path_signoff(), \"--signoff\\n\");\n> + if (opts->reschedule_failed_exec)\n> + write_file(rebase_path_reschedule_failed_exec(), \"%s\", \"\");\n>\n> return 0;\n> }\n> @@ -3586,9 +3592,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)\n> *end_of_arg = saved;\n>\n> /* Reread the todo file if it has changed. */\n> - if (res)\n> - ; /* fall through */\n> - else if (stat(get_todo_path(opts), &st))\n> + if (res) {\n> + if (opts->reschedule_failed_exec)\n> + reschedule = 1;\n> + } else if (stat(get_todo_path(opts), &st))\n> res = error_errno(_(\"could not stat '%s'\"),\n> get_todo_path(opts));\n> else if (match_stat_data(&todo_list->stat, &st)) {\n> diff --git a/sequencer.h b/sequencer.h\n> index 5071a73563..1f865dae26 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -39,6 +39,7 @@ struct replay_opts {\n> int allow_empty_message;\n> int keep_redundant_commits;\n> int verbose;\n> + int reschedule_failed_exec;\n>\n> int mainline;\n>\n> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n> index 0210b2ac6f..54b26a9284 100755\n> --- a/t/t3418-rebase-continue.sh\n> +++ b/t/t3418-rebase-continue.sh\n> @@ -254,4 +254,10 @@ test_expect_success 'the todo command \"break\" works' '\n> test_path_is_file execed\n> '\n>\n> +test_expect_success '--reschedule-failed-exec' '\n> + test_when_finished \"git rebase --abort\" &&\n> + test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&\n> + grep \"^exec false\" .git/rebase-merge/git-rebase-todo\n> +'\n> +\n> test_done\n\nThe rest of the patch looks good to me.\n"},{"id":"364991","messageId":"CABPp-BGeDA=Cm6MYkrCK=pN94y9AKRHgknjyXN1oMfnSsTCnzw@mail.gmail.com","threadId":"49995","inReplyTo":"pull.90.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-12-10T23:20:43Z","receivedAt":"2018-12-10T23:20:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Dec 10, 2018 at 3:13 PM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> The idea was brought up by Paul Morelle.\n>\n> To be honest, this idea of rescheduling a failed exec makes so much sense\n> that I wish we had done this from the get-go.\n>\n> So let's do the next best thing: implement a command-line option and a\n> config setting to make it so.\n>\n> The obvious intent behind that config setting is to not only give users a\n> way to opt into that behavior already, but also to make it possible to flip\n> the default at a later date, after the feature has been battle-tested and\n> after deprecating the non-rescheduling behavior for a couple of Git\n> versions.\n>\n> If the team agrees with my reasoning, then the 3rd patch (introducing -y\n> as a shortcut for --reschedule-failed-exec -x ) might not make\n> much sense, as it would introduce a short option that would become obsolete\n> soon.\n>\n\nComplete side-track: This email showed up for me just five minutes\nago, whereas the rest of the series showed up four hours ago, making\nme think this email had disappeared and trying to figure out how to\nrespond when I didn't have the original. Any ideas why there might be\nthat level of lag?\n"},{"id":"365021","messageId":"xmqqpnu8lp2p.fsf@gitster-ct.c.googlers.com","threadId":"49995","inReplyTo":"CAGZ79kY18SCaCBvkWyeVd+oeJ4EhoJK4=QxGhJ9a77iX2sR8ew@mail.gmail.com","subject":"Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-11T03:28:14Z","receivedAt":"2018-12-11T03:28:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I wonder if it might be better to push this mechanism\n> one layer down: Instead of having a flag that changes\n> the behavior of the \"exec\" instructions and having a\n> handy '-y' short cut for the new mode, we'd rather have\n> a new type of command that executes&retries a command\n> ...\n> By having two classes, I would anticipate fewer compatibility\n> issues ('\"Exec\" behaves differently, and I forgot I had turned\n> on the rescheduling').\n\nIt takes us back to the original proposal that started this whole\nthing.\n\n cf. <3fb5a7ff-a63a-6fac-1456-4dbc9135d088@gmail.com>\n\nAfter re-reading the thread, I still do not quite follow why it was\nconsidered better to change the way \"exec\" is run with the command\nline option than to implement this as a new insn [*1*], but that is\nwhere this series fit in the larger picture, I think.\n\n\n[Footnote]\n\n*1* The original proposal called it \"test\" which I do not think was\n a great name.\n"},{"id":"365052","messageId":"nycvar.QRO.7.76.6.1812111104561.43@tvgsbejvaqbjf.bet","threadId":"49995","inReplyTo":"CABPp-BGoJxKFFu+JE9n52t8Fygzf0+mpPSOo8rftR2v0_i+eZw@mail.gmail.com","subject":"Re: [PATCH 1/3] rebase: introduce --reschedule-failed-exec","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-11T10:14:39Z","receivedAt":"2018-12-11T10:14:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Mon, 10 Dec 2018, Elijah Newren wrote:\n\n> On Mon, Dec 10, 2018 at 1:18 PM Johannes Schindelin via GitGitGadget\n> wrote:\n> >\n> > @@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > break;\n> > }\n> >\n> > + if (options.reschedule_failed_exec && !is_interactive(&options))\n> > + die(_(\"--reschedule-failed-exec requires an interactive rebase\"));\n> > +\n> \n> I was surprised at first that you checked is_interactive() rather than\n> checking for --exec being specified. But I guess this is because users\n> can manually specify 'exec' lines.\n\nIndeed, that is exactly the reason.\n\n> What if the user specifies an implicitly interactive rebase (i.e. no\n> editing of the todo list, such as with --rebase-merges or\n> --keep-empty, or soon --strategy or --strategy-option) and also\n> doesn't specify --exec?\n\nThen the todo list won't have any `exec` lines, and the flag is irrelevant\n(but does not do any harm).\n\n... except in the case that the rebase fails at some stage, the user edits\nthe todo list with `git rebase --edit-todo` and inserts an `exec` line.\n\nSo I would contend that it still makes sense to allow that flag in those\ncases, i.e. whenever the user asked for the interactive backend.\n\n> > @@ -534,6 +545,9 @@ then\n> > # git-rebase.txt caveats with \"unless you know what you are doing\"\n> > test -n \"$rebase_merges\" &&\n> > die \"$(gettext \"error: cannot combine '--preserve-merges' with '--rebase-merges'\")\"\n> > +\n> > + test -n \"$reschedule_failed_exec\" &&\n> > + die \"$(gettext \"error: cannot combine '--preserve-merges' with '--reschedule-failed-exec'\")\"\n> > fi\n> >\n> > if test -n \"$rebase_merges\"\n> \n> In the builtin rebase, you checked that --reschedule-failed-exec had\n> to be used with an interactive rebase. Here in the legacy rebase you\n> have no such check at all.\n> \n> Not sure if that's an oversight, or if we're at the point where we\n> just start intentionally allowing legacy rebase to lag and soon throw\n> it out. (When do we get to that point?)\n\nGood point. My thinking was that the legacy rebase does not matter all\nthat much anymore, I would expect that we get rid of it in v2.21.0.\n\nBut you're right, I should not intentionally diverge the functionality out\nof sheer laziness.\n\nWill fix.\n\n> The rest of the patch looks good to me.\n\nThanks!\nDscho\n"},{"id":"365054","messageId":"nycvar.QRO.7.76.6.1812111117280.43@tvgsbejvaqbjf.bet","threadId":"49995","inReplyTo":"CABPp-BGeDA=Cm6MYkrCK=pN94y9AKRHgknjyXN1oMfnSsTCnzw@mail.gmail.com","subject":"email lags, was Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-11T10:19:25Z","receivedAt":"2018-12-11T10:19:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Mon, 10 Dec 2018, Elijah Newren wrote:\n\n> On Mon, Dec 10, 2018 at 3:13 PM Johannes Schindelin via GitGitGadget\n> wrote:\n> >\n> > The idea was brought up by Paul Morelle.\n> >\n> > To be honest, this idea of rescheduling a failed exec makes so much sense\n> > that I wish we had done this from the get-go.\n> >\n> > So let's do the next best thing: implement a command-line option and a\n> > config setting to make it so.\n> >\n> > The obvious intent behind that config setting is to not only give users a\n> > way to opt into that behavior already, but also to make it possible to flip\n> > the default at a later date, after the feature has been battle-tested and\n> > after deprecating the non-rescheduling behavior for a couple of Git\n> > versions.\n> >\n> > If the team agrees with my reasoning, then the 3rd patch (introducing -y\n> > as a shortcut for --reschedule-failed-exec -x ) might not make\n> > much sense, as it would introduce a short option that would become obsolete\n> > soon.\n> >\n> \n> Complete side-track: This email showed up for me just five minutes\n> ago, whereas the rest of the series showed up four hours ago, making\n> me think this email had disappeared and trying to figure out how to\n> respond when I didn't have the original. Any ideas why there might be\n> that level of lag?\n\nI have such email woes for roughly half a year now. No idea where this\ncomes from, whether this is some graylisting at work, or whether the\n` via GitGitGadget` marks gitgitgadget@gmail.com as suspect with\nsome mail providers and/or central lists of dubious email addresses.\n\nAt first, I thought it was only GMX, but yes, I also see it with GMail\nnow.\n\nCiao,\nDscho\n"},{"id":"365056","messageId":"nycvar.QRO.7.76.6.1812111119560.43@tvgsbejvaqbjf.bet","threadId":"49995","inReplyTo":"CAGZ79kY18SCaCBvkWyeVd+oeJ4EhoJK4=QxGhJ9a77iX2sR8ew@mail.gmail.com","subject":"Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-11T10:31:16Z","receivedAt":"2018-12-11T10:31:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Mon, 10 Dec 2018, Stefan Beller wrote:\n\n> On Mon, Dec 10, 2018 at 2:08 PM Johannes Sixt wrote:\n> >\n> > Am 10.12.18 um 20:04 schrieb Johannes Schindelin via GitGitGadget:\n> > > The idea was brought up by Paul Morelle.\n> > >\n> > > To be honest, this idea of rescheduling a failed exec makes so much sense\n> > > that I wish we had done this from the get-go.\n> >\n> > The status quo was actually not that bad a decision, because it made 'x\n> > false' as a substitute for 'break' very convenient.\n> >\n> > But now that we have a real 'break', I'm very much in favor of flipping\n> > the behavior over to rescheduling. (I'm actually not a user of the\n> > feature, but the proposed behavior is so compellingly logical.)\n> \n> In rare cases I had commands that may be dangerous if rerun,\n> but I'd just not run them with -y but with -x.\n\nPlease note that 3/3 (i.e. the `-y` option) is *really* only intended as a\n\"I could do this if anybody *really* wanted to\" patch. I actually would\nstrongly prefer not to have this patch in git.git at all.\n\n> That brings me to some confusion I had in the last patch:\n> To catch a flaky test I surely would be tempted to\n> git rebase -x make -y \"make test\"\n> but that might reschedule a compile failure as well,\n> as a single -y has implications on all other -x's.\n> \n> I wonder if it might be better to push this mechanism\n> one layer down: Instead of having a flag that changes\n> the behavior of the \"exec\" instructions and having a\n> handy '-y' short cut for the new mode, we'd rather have\n> a new type of command that executes&retries a command\n> (\"exnrt\", 'n'), which can still get the '-y' command line flag,\n> but more importantly by having 2 separate sets of\n> commands we'd have one set that is a one-shot, and the\n> other that is retried. Then we can teach the user which\n> is safe and which isn't for rescheduling.\n> \n> By having two classes, I would anticipate fewer compatibility\n> issues ('\"Exec\" behaves differently, and I forgot I had turned\n> on the rescheduling').\n\nAs Junio points out, this brings us back to the proposal to have two\ndifferent `exec` commands.\n\nI am really unenthusiastic about this idea, as I find it \"hard to sell\":\nwhat little benefit it would bring to have two commands that pretty much\ndo the same thing almost all the time would be outweighed *by far* by the\nconfusion we would sow by that.\n\nIt is amazing to me how much my perspective changed when I actually had to\nteach Git to new users. Things that I live with easily all of a sudden\nbecome these unnecessarily confusing road blocks that make it *so hard* to\nactually use Git.\n\n> Talking about rescheduling: In the above example the flaky\n> test can flake more than once, so I'd be stuck with keeping\n> 'git rebase --continue'ing after I see the test flaked once again.\n\nNo, you would not be stuck with that.\n\nYou would read the error message carefully (we do that all the time, yes?)\nand then run `git rebase --edit-todo` to remove that line before\ncontinuing.\n\n:-)\n\nIf you feel very strongly about this, we could introduce a new option for\n`git rebase --skip`, say, `git rebase --skip --skip-next-commands-too=1`.\n(I specifically used a too-long option name, as you and I are both\nGermans, known to use too-long names for everything. I do not really\nintend to use such an awkward option name, if we decide that such an\noption would be a good idea. Probably more like `git rebase\n--skip-next[=]`.)\n\n> My workflow with interactive rebase and fixing up things as I go\n> always involves a manual final \"make test\" to check \"for real\",\n> which I could lose now, which is nice.\n\nMy workflow with `git rebase -r --exec \"make test\"` is pretty similar to\nyours. With the difference that I would want those commands to be\nrescheduled *even if* the command is flakey. Actually, *in particular*\nwhen it is flakey.\n\nI really see myself calling\n\n\tgit config --global rebase.rescheduleFailedExec true\n\nin all my setups.\n\nCiao,\nDscho\n"},{"id":"365098","messageId":"CABPp-BHENhW=uz6=_gzELNOd15JWfH1ymtxVRFMD284FLBFtKg@mail.gmail.com","threadId":"49995","inReplyTo":"nycvar.QRO.7.76.6.1812111104561.43@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/3] rebase: introduce --reschedule-failed-exec","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-12-11T16:16:43Z","receivedAt":"2018-12-11T16:17:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Dscho,\n\nOn Tue, Dec 11, 2018 at 2:14 AM Johannes Schindelin\n wrote:\n>\n> Hi Elijah,\n>\n> On Mon, 10 Dec 2018, Elijah Newren wrote:\n>\n> > On Mon, Dec 10, 2018 at 1:18 PM Johannes Schindelin via GitGitGadget\n> > wrote:\n> > >\n> > > @@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > > break;\n> > > }\n> > >\n> > > + if (options.reschedule_failed_exec && !is_interactive(&options))\n> > > + die(_(\"--reschedule-failed-exec requires an interactive rebase\"));\n> > > +\n> >\n> > I was surprised at first that you checked is_interactive() rather than\n> > checking for --exec being specified. But I guess this is because users\n> > can manually specify 'exec' lines.\n>\n> Indeed, that is exactly the reason.\n>\n> > What if the user specifies an implicitly interactive rebase (i.e. no\n> > editing of the todo list, such as with --rebase-merges or\n> > --keep-empty, or soon --strategy or --strategy-option) and also\n> > doesn't specify --exec?\n>\n> Then the todo list won't have any `exec` lines, and the flag is irrelevant\n> (but does not do any harm).\n>\n> ... except in the case that the rebase fails at some stage, the user edits\n> the todo list with `git rebase --edit-todo` and inserts an `exec` line.\n\nAh, good point; hadn't thought of that case. Thanks for explaining.\n"},{"id":"365106","messageId":"CAGZ79kZL5CRqCDRb6B-EedUm8Z_i4JuSF2=UtwwdRXMitrrOBw@mail.gmail.com","threadId":"49995","inReplyTo":"nycvar.QRO.7.76.6.1812111119560.43@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-11T17:36:46Z","receivedAt":"2018-12-11T17:37:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> It is amazing to me how much my perspective changed when I actually had to\n> teach Git to new users. Things that I live with easily all of a sudden\n> become these unnecessarily confusing road blocks that make it *so hard* to\n> actually use Git.\n\nI see. Without the -y patch, this series looks good to me.\n\n> My workflow with `git rebase -r --exec \"make test\"` is pretty similar to\n> yours. With the difference that I would want those commands to be\n> rescheduled *even if* the command is flakey. Actually, *in particular*\n> when it is flakey.\n>\n> I really see myself calling\n>\n> git config --global rebase.rescheduleFailedExec true\n\nMe, too.\n\nThanks,\nStefan\n"},{"id":"419933","messageId":"002dc72ee72dc31a6d756c762f9fc1150ab88142.1616411973.git.avarab@gmail.com","threadId":"49995","inReplyTo":"cover.1616411973.git.avarab@gmail.com","subject":"[PATCH 1/3] rebase tests: camel-case rebase.rescheduleFailedExec consistently","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-22T11:48:23Z","receivedAt":"2021-03-22T11:50:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a test added in 906b63942ac (rebase --am: ignore\nrebase.rescheduleFailedExec, 2019-07-01) to camel-case the\nconfiguration variable. This doesn't change the behavior of the test,\nit's merely to help its human readers.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t3418-rebase-continue.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 0838f4e798a..fe407e63cf1 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -282,8 +282,8 @@ test_expect_success '--reschedule-failed-exec' '\n \ttest_i18ngrep \"has been rescheduled\" err\n '\n \n-test_expect_success 'rebase.reschedulefailedexec only affects `rebase -i`' '\n-\ttest_config rebase.reschedulefailedexec true &&\n+test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '\n+\ttest_config rebase.rescheduleFailedExec true &&\n \ttest_must_fail git rebase -x false HEAD^ &&\n \tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo &&\n \tgit rebase --abort &&\n-- \n2.31.0.285.gb40d23e604f\n\n"},{"id":"419934","messageId":"cover.1616411973.git.avarab@gmail.com","threadId":"49995","inReplyTo":"873d1fda948855510b07f9cb75d374c03d60a94e.1544468695.git.gitgitgadget@gmail.com","subject":"[PATCH 0/3] rebase: don't override --no-reschedule-failed-exec with config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-22T11:48:22Z","receivedAt":"2021-03-22T11:50:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I recently started using rebase.rescheduleFailedExec=true and noticed\nthis bug in its implementation. It's conceptually a relatively simple\nfix, but as noted in 3/3 rebase being a \"start this operation, run\nother command verbs later\" has an unintuitive interaction with our\nusual \"command-line options override config\".\n\n!README FIRST!\nEverthing after this line has no relevance to this series, it's just a\nside musing on another (mis-)feature of --reschedule-failed-exec.\n!/README FIRST!\n\nThere's another bug/misfeature I've noticed with\nrebase.rescheduleFailedExec=true (although maybe it'll be argued by\nsomeone that it's a feature). Let's say you run:\n\n git rebase -x false --reschedule-failed-exec HEAD~2\n\nYou'll now land in a state of (according to our helpful PS1 code, but\nalso the rebase state) of 2/4. Aside: Because you're rebasing 2\ncommits, we have 4 because of the 2x exec. So far so good and all of\nthat's expected.\n\nBut now when you do \"git rebase --continue\" we'll fail as expected,\nbut not as expected (at least I find it funny) bump the count to 3/5,\nthen 4/6, 5/7 etc.\n\nWith my \"I just read the code\" hat on this makes perfect sense. Every\ntime we process a TODO item we bump the count of processed items, and\nunder --reschedule-failed-exec we simply push the command that just\nfailed onto the list, hence the increasing done/TODO count when a\ncommand fails.\n\nBut I don't see how it makes any sense to expose this as UI via \"git\nstatus\" and __git_ps1. I asked git to process this item of 4 sequencer\nTODO items. If I fail an item it makes more sense that I just don't\nget past it, not that under the hood a new identical item is\nrescheduled for me. Just don't advance past the current item!\n\nNow if I've tried X times to make the \"make test\" pass for each commit\nin my 3-commit series I'm going to be on item 10/12 or\nwhatever. There's no way to just look at that and see where I'm at in\nthe sequence.\n\nAs an aside it would arguably make more sense to report 1/3 instead of\n2/6 for the first commit of 3 with a failing -x \"make test\", but you\ncan have X number of \"exec\" items and a manually edited list etc. \n\nSo that's probably a no-go but at least once I'm used to it I know if\ni'm on 4/6 I'm on commit 2/3, with --reschedule-failed-exec you'll\nhave no idea what 12/14 or whatever means for where you are in your\n3-patch sequence, it has no relation to the TODO list you edited, just\nrebase-merge's own internal state.\n\nGetting back on topic: This just seems like needlessly exposing an\nimplementation detail, I also asked to \"pick\" a commit, but if that\nitem \"fails\" e.g. due to:\n \n $ git rebase --continue\n You must edit all merge conflicts and then\n mark them as resolved using git add\n\nWe don't push a new \"pick\" item on the list and inflate the count, so\nwhy would we do that for \"exec\"? Just say the command failed, return\nits non-zero status from \"git rebase --continue\", and don't advance.\n\nMaybe it is useful to keep track of the N number of failures, and\ne.g. report in __git_ps1:\n\n master|REBASE 2/6\n master|REBASE 2(tries: 1)/6\n master|REBASE 2(tries: 2)/6\n\nInstead of the current:\n\n master|REBASE 2/6\n master|REBASE 3/7\n master|REBASE 4/8\n\nTo say I'm on 2/6, but that I've tried 3 times already and failed to\nadvance past it.\n\nAnyway, I don't have patches for this side-report/rant. Looking at the\nimplementation it seemed more non-trivial than I was willing to\nquickly fix.\n\nWe bump the count fairly early before we even get to there being an\n\"exec\" item, to implement this proposed view of the world we'd need to\ndefer that (or go back and edit it once we see \"failed exec\" and that\nwe're using --reschedule-failed-exec).\n\nI'm not familiar enough with the sequencer internals to know if trying\nthat would lead us down a path of e.g. having inconsistent or bad\nstate if we'd die in the middle of all of that.\n\nÆvar Arnfjörð Bjarmason (3):\n rebase tests: camel-case rebase.rescheduleFailedExec consistently\n rebase tests: use test_unconfig after test_config\n rebase: don't override --no-reschedule-failed-exec with config\n\n Documentation/git-rebase.txt | 8 ++++++++\n sequencer.c | 5 +++++\n t/t3418-rebase-continue.sh | 30 ++++++++++++++++++++++++++++--\n 3 files changed, 41 insertions(+), 2 deletions(-)\n\n-- \n2.31.0.285.gb40d23e604f\n\n"},{"id":"419935","messageId":"330b33e7a8e87e528aec43c306753cdf573ed8af.1616411973.git.avarab@gmail.com","threadId":"49995","inReplyTo":"cover.1616411973.git.avarab@gmail.com","subject":"[PATCH 2/3] rebase tests: use test_unconfig after test_config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-22T11:48:24Z","receivedAt":"2021-03-22T11:50:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a test added in 906b63942ac (rebase --am: ignore\nrebase.rescheduleFailedExec, 2019-07-01) to reset its config after it\nruns. This doesn't matter now since it's the last test in the file,\nbut will in a subsequent commit where I'll add new tests after this\none.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t3418-rebase-continue.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex fe407e63cf1..ea14ef496cb 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -283,6 +283,7 @@ test_expect_success '--reschedule-failed-exec' '\n '\n \n test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '\n+\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n \ttest_config rebase.rescheduleFailedExec true &&\n \ttest_must_fail git rebase -x false HEAD^ &&\n \tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo &&\n-- \n2.31.0.285.gb40d23e604f\n\n"},{"id":"419936","messageId":"e00300d58d4de4a6b440446a0054d34ad5a092f3.1616411973.git.avarab@gmail.com","threadId":"49995","inReplyTo":"cover.1616411973.git.avarab@gmail.com","subject":"[PATCH 3/3] rebase: don't override --no-reschedule-failed-exec with config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-22T11:48:25Z","receivedAt":"2021-03-22T11:50:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bug in how --no-reschedule-failed-exec interacts with\nrebase.rescheduleFailedExec=true being set in the config. Before this\nchange the --no-reschedule-failed-exec config option would be\noverridden by the config.\n\nThis bug happened because of the particulars of how \"rebase\" works\nv.s. most other git commands when it comes to parsing options and\nconfig:\n\nWhen we read the config and parse the CLI options we correctly prefer\nthe --no-reschedule-failed-exec option over\nrebase.rescheduleFailedExec=true in the config. So far so good.\n\nHowever the --reschedule-failed-exec option doesn't take effect when\nthe rebase starts (we'd just create a\n\".git/rebase-merge/reschedule-failed-exec\" file if it was true). It\nonly takes effect when the exec command fails, and the user wants to\nrun \"rebase --continue\".\n\nAt that point we'll have forgotten that we asked for\n--no-reschedule-failed-exec at the start, and will happily re-read the\nconfig.\n\nWe'll then see that rebase.rescheduleFailedExec=true is set. At that\npoint we have no record of having set --no-reschedule-failed-exec\nearlier. So the config will effectively override the user having\nexplicitly disabled the option on the command-line.\n\nEven more confusingly: Since rebase accepts different options based on\nits state there wasn't even a way to get around this with \"rebase\n--continue --no-reschedule-failed-exec\" (but you could of course set\nthe config with \"rebase -c ...\").\n\nI think the least bad way out of this is to declare that for such\noptions and config whatever we decide at the beginning of the rebase\ngoes. So we'll now always create either a \"reschedule-failed-exec\" or\na \"no-reschedule-failed-exec file at the start, not just the former if\nwe decided we wanted the feature.\n\nWith this new worldview you can no longer change the setting once a\nrebase has started except by manually removing the state files\ndiscussed above. I think making it work like that is the the least\nconfusing thing we can do.\n\nIn the future we might want to learn to change the setting in the\nmiddle by combining \"--edit-todo\" with\n\"--[no-]reschedule-failed-exec\", we currently don't support combining\nthose options, or any other way to change the state in the middle of\nthe rebase short of manually editing the files in\n\".git/rebase-merge/*\".\n\nThe bug being fixed here originally came about because of a\ncombination of the behavior of the code added in d421afa0c66 (rebase:\nintroduce --reschedule-failed-exec, 2018-12-10) and the addition of\nthe config variable in 969de3ff0e0 (rebase: add a config option to\ndefault to --reschedule-failed-exec, 2018-12-10).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-rebase.txt | 8 ++++++++\n sequencer.c | 5 +++++\n t/t3418-rebase-continue.sh | 25 +++++++++++++++++++++++++\n 3 files changed, 38 insertions(+)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex a0487b5cc58..b48e6225769 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -622,6 +622,14 @@ See also INCOMPATIBLE OPTIONS below.\n --no-reschedule-failed-exec::\n \tAutomatically reschedule `exec` commands that failed. This only makes\n \tsense in interactive mode (or when an `--exec` option was provided).\n++\n+Even though this option applies once a rebase is started, it's set for\n+the whole rebase at the start based on either the\n+`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]\n+or \"CONFIGURATION\" below) or whether this option is\n+provided. Otherwise an explicit `--no-reschedule-failed-exec` at the\n+start would be overridden by the presence of\n+`rebase.rescheduleFailedExec=true` configuration.\n \n INCOMPATIBLE OPTIONS\n --------------------\ndiff --git a/sequencer.c b/sequencer.c\nindex 848204d3dc3..59735fdff62 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -164,6 +164,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, \"rebase-merge/strategy\")\n static GIT_PATH_FUNC(rebase_path_strategy_opts, \"rebase-merge/strategy_opts\")\n static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, \"rebase-merge/allow_rerere_autoupdate\")\n static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, \"rebase-merge/reschedule-failed-exec\")\n+static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, \"rebase-merge/no-reschedule-failed-exec\")\n static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, \"rebase-merge/drop_redundant_commits\")\n static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, \"rebase-merge/keep_redundant_commits\")\n \n@@ -2672,6 +2673,8 @@ static int read_populate_opts(struct replay_opts *opts)\n \n \t\tif (file_exists(rebase_path_reschedule_failed_exec()))\n \t\t\topts->reschedule_failed_exec = 1;\n+\t\telse if (file_exists(rebase_path_no_reschedule_failed_exec()))\n+\t\t\topts->reschedule_failed_exec = 0;\n \n \t\tif (file_exists(rebase_path_drop_redundant_commits()))\n \t\t\topts->drop_redundant_commits = 1;\n@@ -2772,6 +2775,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\twrite_file(rebase_path_ignore_date(), \"%s\", \"\");\n \tif (opts->reschedule_failed_exec)\n \t\twrite_file(rebase_path_reschedule_failed_exec(), \"%s\", \"\");\n+\telse\n+\t\twrite_file(rebase_path_no_reschedule_failed_exec(), \"%s\", \"\");\n \n \treturn 0;\n }\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex ea14ef496cb..9553d969646 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -291,4 +291,29 @@ test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '\n \tgit rebase HEAD^\n '\n \n+test_expect_success 'rebase.rescheduleFailedExec=true & --no-reschedule-failed-exec' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n+\ttest_config rebase.rescheduleFailedExec true &&\n+\ttest_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&\n+\ttest_must_fail git rebase --continue 2>err &&\n+\t! grep \"has been rescheduled\" err\n+'\n+\n+test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\ttest_must_fail git rebase -x false HEAD~2 &&\n+\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n+\ttest_config rebase.rescheduleFailedExec true &&\n+\ttest_must_fail git rebase --continue 2>err &&\n+\t! grep \"has been rescheduled\" err\n+'\n+\n+test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebase' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\ttest_must_fail git rebase -x false HEAD~2 &&\n+\ttest_expect_code 129 git rebase --continue --no-reschedule-failed-exec &&\n+\ttest_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec\n+'\n+\n test_done\n-- \n2.31.0.285.gb40d23e604f\n\n"},{"id":"420116","messageId":"nycvar.QRO.7.76.6.2103241243000.50@tvgsbejvaqbjf.bet","threadId":"49995","inReplyTo":"cover.1616411973.git.avarab@gmail.com","subject":"Re: [PATCH 0/3] rebase: don't override --no-reschedule-failed-exec with config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-03-24T11:50:53Z","receivedAt":"2021-03-24T13:39:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Mon, 22 Mar 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> I recently started using rebase.rescheduleFailedExec=true and noticed\n> this bug in its implementation.\n\nOkay, there is a bug. \"This bug\". Let's read on to find out what it is.\n\n> It's conceptually a relatively simple fix,\n\nWait, hold on, there is a fix, but what is the bug?\n\n> but as noted in 3/3 rebase being a \"start this operation, run other\n> command verbs later\" has an unintuitive interaction with our usual\n> \"command-line options override config\".\n\nOh, so there is a detailed analysis of the bug, whatever it is, in the\nthird patch in this patch series.\n\nBut what is the bug.\n\n> !README FIRST!\n> Everthing after this line has no relevance to this series, it's just a\n> side musing on another (mis-)feature of --reschedule-failed-exec.\n> !/README FIRST!\n\nHold on! I should read this first? Why is this not on top of the cover\nletter, then?\n\nOh, and wait, everything after that has no relevance to this series? Then\nwhere is the high-level description of the bug, a motivator to read this\npatch series?\n\n;-)\n\nI guess I will find out when I have set aside some time to read the patch\nseries, which is on my back burner because nothing I read so far makes\nthis more urgent than other tasks I had planned on addressing this week.\n\nOr maybe you could enhance the cover letter by skipping the rant, and by\ngiving a very rough overview of the bug at the top of the cover letter?\nThink of the cover letter as an elevator pitch to make me want to spend\ntime on reviewing your patch series.\n\nCiao,\nDscho\n\n> [... snip rant about the odd progress counting when rescheduling todo\n> commands ...]\n"},{"id":"420479","messageId":"fc7e3d13-8761-6d79-0fc9-734d6942a8e6@gmail.com","threadId":"49995","inReplyTo":"e00300d58d4de4a6b440446a0054d34ad5a092f3.1616411973.git.avarab@gmail.com","subject":"Re: [PATCH 3/3] rebase: don't override --no-reschedule-failed-exec with config","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-03-29T14:49:50Z","receivedAt":"2021-03-29T14:50:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 22/03/2021 11:48, Ævar Arnfjörð Bjarmason wrote:\n> Fix a bug in how --no-reschedule-failed-exec interacts with\n> rebase.rescheduleFailedExec=true being set in the config. Before this\n> change the --no-reschedule-failed-exec config option would be\n> overridden by the config.\n> \n> This bug happened because of the particulars of how \"rebase\" works\n> v.s. most other git commands when it comes to parsing options and\n> config:\n> \n> When we read the config and parse the CLI options we correctly prefer\n> the --no-reschedule-failed-exec option over\n> rebase.rescheduleFailedExec=true in the config. So far so good.\n> \n> However the --reschedule-failed-exec option doesn't take effect when\n> the rebase starts (we'd just create a\n> \".git/rebase-merge/reschedule-failed-exec\" file if it was true). It\n> only takes effect when the exec command fails, and the user wants to\n> run \"rebase --continue\".\n\nThe exec command is rescheduled in the todo file as soon as it fails, we \ndo not wait for the user to run 'rebase --continue' to reschedule it. \nHowever if it still fails after restarting or a later exec fails we have \nthe problem you describe.\n\n> At that point we'll have forgotten that we asked for\n> --no-reschedule-failed-exec at the start, and will happily re-read the\n> config.\n> \n> We'll then see that rebase.rescheduleFailedExec=true is set. At that\n> point we have no record of having set --no-reschedule-failed-exec\n> earlier. So the config will effectively override the user having\n> explicitly disabled the option on the command-line.\n> \n> Even more confusingly: Since rebase accepts different options based on\n> its state there wasn't even a way to get around this with \"rebase\n> --continue --no-reschedule-failed-exec\" (but you could of course set\n> the config with \"rebase -c ...\").\n> \n> I think the least bad way out of this is to declare that for such\n> options and config whatever we decide at the beginning of the rebase\n> goes. So we'll now always create either a \"reschedule-failed-exec\" or\n> a \"no-reschedule-failed-exec file at the start, not just the former if\n> we decided we wanted the feature.\n\nThanks for working on this and for the detailed commit message. I'm not \nentirely convinced we want yet another state file in .git/rebase-merge. \nWe could we start writing the setting to the file rather than having \ndifferent files for whether the option is on or off. If we use the \ncontents of the file it could be -1 for 'use config', 0 'off', 1 'on'. \nThe downside is that starting 'rebase --no-reschedule-failed-exec' with \na new version of git and then continuing with an old version would do \nthe wrong thing.\n\nBest Wishes\n\nPhillip\n\n> With this new worldview you can no longer change the setting once a\n> rebase has started except by manually removing the state files\n> discussed above. I think making it work like that is the the least\n> confusing thing we can do.\n> \n> In the future we might want to learn to change the setting in the\n> middle by combining \"--edit-todo\" with\n> \"--[no-]reschedule-failed-exec\", we currently don't support combining\n> those options, or any other way to change the state in the middle of\n> the rebase short of manually editing the files in\n> \".git/rebase-merge/*\".\n> \n> The bug being fixed here originally came about because of a\n> combination of the behavior of the code added in d421afa0c66 (rebase:\n> introduce --reschedule-failed-exec, 2018-12-10) and the addition of\n> the config variable in 969de3ff0e0 (rebase: add a config option to\n> default to --reschedule-failed-exec, 2018-12-10).\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/git-rebase.txt | 8 ++++++++\n> sequencer.c | 5 +++++\n> t/t3418-rebase-continue.sh | 25 +++++++++++++++++++++++++\n> 3 files changed, 38 insertions(+)\n> \n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index a0487b5cc58..b48e6225769 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -622,6 +622,14 @@ See also INCOMPATIBLE OPTIONS below.\n> --no-reschedule-failed-exec::\n> \tAutomatically reschedule `exec` commands that failed. This only makes\n> \tsense in interactive mode (or when an `--exec` option was provided).\n> ++\n> +Even though this option applies once a rebase is started, it's set for\n> +the whole rebase at the start based on either the\n> +`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]\n> +or \"CONFIGURATION\" below) or whether this option is\n> +provided. Otherwise an explicit `--no-reschedule-failed-exec` at the\n> +start would be overridden by the presence of\n> +`rebase.rescheduleFailedExec=true` configuration.\n> \n> INCOMPATIBLE OPTIONS\n> --------------------\n> diff --git a/sequencer.c b/sequencer.c\n> index 848204d3dc3..59735fdff62 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -164,6 +164,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, \"rebase-merge/strategy\")\n> static GIT_PATH_FUNC(rebase_path_strategy_opts, \"rebase-merge/strategy_opts\")\n> static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, \"rebase-merge/allow_rerere_autoupdate\")\n> static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, \"rebase-merge/reschedule-failed-exec\")\n> +static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, \"rebase-merge/no-reschedule-failed-exec\")\n> static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, \"rebase-merge/drop_redundant_commits\")\n> static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, \"rebase-merge/keep_redundant_commits\")\n> \n> @@ -2672,6 +2673,8 @@ static int read_populate_opts(struct replay_opts *opts)\n> \n> \t\tif (file_exists(rebase_path_reschedule_failed_exec()))\n> \t\t\topts->reschedule_failed_exec = 1;\n> +\t\telse if (file_exists(rebase_path_no_reschedule_failed_exec()))\n> +\t\t\topts->reschedule_failed_exec = 0;\n> \n> \t\tif (file_exists(rebase_path_drop_redundant_commits()))\n> \t\t\topts->drop_redundant_commits = 1;\n> @@ -2772,6 +2775,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n> \t\twrite_file(rebase_path_ignore_date(), \"%s\", \"\");\n> \tif (opts->reschedule_failed_exec)\n> \t\twrite_file(rebase_path_reschedule_failed_exec(), \"%s\", \"\");\n> +\telse\n> +\t\twrite_file(rebase_path_no_reschedule_failed_exec(), \"%s\", \"\");\n> \n> \treturn 0;\n> }\n> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n> index ea14ef496cb..9553d969646 100755\n> --- a/t/t3418-rebase-continue.sh\n> +++ b/t/t3418-rebase-continue.sh\n> @@ -291,4 +291,29 @@ test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '\n> \tgit rebase HEAD^\n> '\n> \n> +test_expect_success 'rebase.rescheduleFailedExec=true & --no-reschedule-failed-exec' '\n> +\ttest_when_finished \"git rebase --abort\" &&\n> +\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n> +\ttest_config rebase.rescheduleFailedExec true &&\n> +\ttest_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&\n> +\ttest_must_fail git rebase --continue 2>err &&\n> +\t! grep \"has been rescheduled\" err\n> +'\n> +\n> +test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '\n> +\ttest_when_finished \"git rebase --abort\" &&\n> +\ttest_must_fail git rebase -x false HEAD~2 &&\n> +\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n> +\ttest_config rebase.rescheduleFailedExec true &&\n> +\ttest_must_fail git rebase --continue 2>err &&\n> +\t! grep \"has been rescheduled\" err\n> +'\n> +\n> +test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebase' '\n> +\ttest_when_finished \"git rebase --abort\" &&\n> +\ttest_must_fail git rebase -x false HEAD~2 &&\n> +\ttest_expect_code 129 git rebase --continue --no-reschedule-failed-exec &&\n> +\ttest_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec\n> +'\n> +\n> test_done\n> \n"},{"id":"420482","messageId":"87lfa6lz16.fsf@evledraar.gmail.com","threadId":"49995","inReplyTo":"fc7e3d13-8761-6d79-0fc9-734d6942a8e6@gmail.com","subject":"Re: [PATCH 3/3] rebase: don't override --no-reschedule-failed-exec with config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-29T16:12:21Z","receivedAt":"2021-03-29T16:13:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 29 2021, Phillip Wood wrote:\n\n> Hi Ævar\n>\n> On 22/03/2021 11:48, Ævar Arnfjörð Bjarmason wrote:\n>> Fix a bug in how --no-reschedule-failed-exec interacts with\n>> rebase.rescheduleFailedExec=true being set in the config. Before this\n>> change the --no-reschedule-failed-exec config option would be\n>> overridden by the config.\n>> This bug happened because of the particulars of how \"rebase\" works\n>> v.s. most other git commands when it comes to parsing options and\n>> config:\n>> When we read the config and parse the CLI options we correctly\n>> prefer\n>> the --no-reschedule-failed-exec option over\n>> rebase.rescheduleFailedExec=true in the config. So far so good.\n>> However the --reschedule-failed-exec option doesn't take effect when\n>> the rebase starts (we'd just create a\n>> \".git/rebase-merge/reschedule-failed-exec\" file if it was true). It\n>> only takes effect when the exec command fails, and the user wants to\n>> run \"rebase --continue\".\n>\n> The exec command is rescheduled in the todo file as soon as it fails,\n> we do not wait for the user to run 'rebase --continue' to reschedule\n> it. However if it still fails after restarting or a later exec fails\n> we have the problem you describe.\n\nRight, as noted in [1] I grokked those internals eventually, but the\ncommit message is written from the viewpoint of a hypothetical user...\n\nB.t.w. if you've had a chance to read [1] over it would be interesting\nto get some thoughts on that rambling. So far I only managed to upset\nJohannes :)\n\n1. http://lore.kernel.org/git/cover.1616411973.git.avarab@gmail.com\n\n>> At that point we'll have forgotten that we asked for\n>> --no-reschedule-failed-exec at the start, and will happily re-read the\n>> config.\n>> We'll then see that rebase.rescheduleFailedExec=true is set. At that\n>> point we have no record of having set --no-reschedule-failed-exec\n>> earlier. So the config will effectively override the user having\n>> explicitly disabled the option on the command-line.\n>> Even more confusingly: Since rebase accepts different options based\n>> on\n>> its state there wasn't even a way to get around this with \"rebase\n>> --continue --no-reschedule-failed-exec\" (but you could of course set\n>> the config with \"rebase -c ...\").\n>> I think the least bad way out of this is to declare that for such\n>> options and config whatever we decide at the beginning of the rebase\n>> goes. So we'll now always create either a \"reschedule-failed-exec\" or\n>> a \"no-reschedule-failed-exec file at the start, not just the former if\n>> we decided we wanted the feature.\n>\n> Thanks for working on this and for the detailed commit message. I'm\n> not entirely convinced we want yet another state file in\n> .git/rebase-merge. We could we start writing the setting to the file\n> rather than having different files for whether the option is on or\n> off. If we use the contents of the file it could be -1 for 'use\n> config', 0 'off', 1 'on'. The downside is that starting 'rebase\n> --no-reschedule-failed-exec' with a new version of git and then\n> continuing with an old version would do the wrong thing.\n\nYes I suppose, but it seems much simpler indeed to just represent this\nsort of tri-state as a ENOENT/no-FOO/FOO neither exists + file pair with\nhow the current code is set up, especially because (as you note) we'd\nneed to phase-in any writing of the content across multiple versions or\nsomething, least in-progress rebases across versions subtly behave\nweirdly.\n\nWell, in this case it's not such a big deal, but I'd rather not\nestablish the pattern for something that *does* matter.\n\n> Best Wishes\n>\n> Phillip\n>\n>> With this new worldview you can no longer change the setting once a\n>> rebase has started except by manually removing the state files\n>> discussed above. I think making it work like that is the the least\n>> confusing thing we can do.\n>> In the future we might want to learn to change the setting in the\n>> middle by combining \"--edit-todo\" with\n>> \"--[no-]reschedule-failed-exec\", we currently don't support combining\n>> those options, or any other way to change the state in the middle of\n>> the rebase short of manually editing the files in\n>> \".git/rebase-merge/*\".\n>> The bug being fixed here originally came about because of a\n>> combination of the behavior of the code added in d421afa0c66 (rebase:\n>> introduce --reschedule-failed-exec, 2018-12-10) and the addition of\n>> the config variable in 969de3ff0e0 (rebase: add a config option to\n>> default to --reschedule-failed-exec, 2018-12-10).\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> Documentation/git-rebase.txt | 8 ++++++++\n>> sequencer.c | 5 +++++\n>> t/t3418-rebase-continue.sh | 25 +++++++++++++++++++++++++\n>> 3 files changed, 38 insertions(+)\n>> diff --git a/Documentation/git-rebase.txt\n>> b/Documentation/git-rebase.txt\n>> index a0487b5cc58..b48e6225769 100644\n>> --- a/Documentation/git-rebase.txt\n>> +++ b/Documentation/git-rebase.txt\n>> @@ -622,6 +622,14 @@ See also INCOMPATIBLE OPTIONS below.\n>> --no-reschedule-failed-exec::\n>> \tAutomatically reschedule `exec` commands that failed. This only makes\n>> \tsense in interactive mode (or when an `--exec` option was provided).\n>> ++\n>> +Even though this option applies once a rebase is started, it's set for\n>> +the whole rebase at the start based on either the\n>> +`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]\n>> +or \"CONFIGURATION\" below) or whether this option is\n>> +provided. Otherwise an explicit `--no-reschedule-failed-exec` at the\n>> +start would be overridden by the presence of\n>> +`rebase.rescheduleFailedExec=true` configuration.\n>> INCOMPATIBLE OPTIONS\n>> --------------------\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 848204d3dc3..59735fdff62 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -164,6 +164,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, \"rebase-merge/strategy\")\n>> static GIT_PATH_FUNC(rebase_path_strategy_opts, \"rebase-merge/strategy_opts\")\n>> static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, \"rebase-merge/allow_rerere_autoupdate\")\n>> static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, \"rebase-merge/reschedule-failed-exec\")\n>> +static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, \"rebase-merge/no-reschedule-failed-exec\")\n>> static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, \"rebase-merge/drop_redundant_commits\")\n>> static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, \"rebase-merge/keep_redundant_commits\")\n>> @@ -2672,6 +2673,8 @@ static int read_populate_opts(struct\n>> replay_opts *opts)\n>> \t\tif (file_exists(rebase_path_reschedule_failed_exec()))\n>> \t\t\topts->reschedule_failed_exec = 1;\n>> +\t\telse if (file_exists(rebase_path_no_reschedule_failed_exec()))\n>> +\t\t\topts->reschedule_failed_exec = 0;\n>> \t\tif (file_exists(rebase_path_drop_redundant_commits()))\n>> \t\t\topts->drop_redundant_commits = 1;\n>> @@ -2772,6 +2775,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n>> \t\twrite_file(rebase_path_ignore_date(), \"%s\", \"\");\n>> \tif (opts->reschedule_failed_exec)\n>> \t\twrite_file(rebase_path_reschedule_failed_exec(), \"%s\", \"\");\n>> +\telse\n>> +\t\twrite_file(rebase_path_no_reschedule_failed_exec(), \"%s\", \"\");\n>> \treturn 0;\n>> }\n>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n>> index ea14ef496cb..9553d969646 100755\n>> --- a/t/t3418-rebase-continue.sh\n>> +++ b/t/t3418-rebase-continue.sh\n>> @@ -291,4 +291,29 @@ test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '\n>> \tgit rebase HEAD^\n>> '\n>> +test_expect_success 'rebase.rescheduleFailedExec=true &\n>> --no-reschedule-failed-exec' '\n>> +\ttest_when_finished \"git rebase --abort\" &&\n>> +\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n>> +\ttest_config rebase.rescheduleFailedExec true &&\n>> +\ttest_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&\n>> +\ttest_must_fail git rebase --continue 2>err &&\n>> +\t! grep \"has been rescheduled\" err\n>> +'\n>> +\n>> +test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '\n>> +\ttest_when_finished \"git rebase --abort\" &&\n>> +\ttest_must_fail git rebase -x false HEAD~2 &&\n>> +\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n>> +\ttest_config rebase.rescheduleFailedExec true &&\n>> +\ttest_must_fail git rebase --continue 2>err &&\n>> +\t! grep \"has been rescheduled\" err\n>> +'\n>> +\n>> +test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebase' '\n>> +\ttest_when_finished \"git rebase --abort\" &&\n>> +\ttest_must_fail git rebase -x false HEAD~2 &&\n>> +\ttest_expect_code 129 git rebase --continue --no-reschedule-failed-exec &&\n>> +\ttest_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec\n>> +'\n>> +\n>> test_done\n>> \n\n"},{"id":"420497","messageId":"a5ce93ea-1e1d-a8e5-7d0e-059dd0c63fd4@gmail.com","threadId":"49995","inReplyTo":"87lfa6lz16.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/3] rebase: don't override --no-reschedule-failed-exec with config","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-03-29T17:15:56Z","receivedAt":"2021-03-29T17:16:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 29/03/2021 17:12, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Mar 29 2021, Phillip Wood wrote:\n> \n>> Hi Ævar\n>>\n>> On 22/03/2021 11:48, Ævar Arnfjörð Bjarmason wrote:\n>>> Fix a bug in how --no-reschedule-failed-exec interacts with\n>>> rebase.rescheduleFailedExec=true being set in the config. Before this\n>>> change the --no-reschedule-failed-exec config option would be\n>>> overridden by the config.\n>>> This bug happened because of the particulars of how \"rebase\" works\n>>> v.s. most other git commands when it comes to parsing options and\n>>> config:\n>>> When we read the config and parse the CLI options we correctly\n>>> prefer\n>>> the --no-reschedule-failed-exec option over\n>>> rebase.rescheduleFailedExec=true in the config. So far so good.\n>>> However the --reschedule-failed-exec option doesn't take effect when\n>>> the rebase starts (we'd just create a\n>>> \".git/rebase-merge/reschedule-failed-exec\" file if it was true). It\n>>> only takes effect when the exec command fails, and the user wants to\n>>> run \"rebase --continue\".\n>>\n>> The exec command is rescheduled in the todo file as soon as it fails,\n>> we do not wait for the user to run 'rebase --continue' to reschedule\n>> it. However if it still fails after restarting or a later exec fails\n>> we have the problem you describe.\n> \n> Right, as noted in [1] I grokked those internals eventually, but the\n> commit message is written from the viewpoint of a hypothetical user...\n\nSo user will find that the first failed exec is not rescheduled but \nsubsequent ones are which looks like the setting only takes effect after \nthe run 'git rebase --continue' - fair enough.\n\n> B.t.w. if you've had a chance to read [1] over it would be interesting\n> to get some thoughts on that rambling. So far I only managed to upset\n> Johannes :)\n\nI'll try and have a proper look at it tomorrow, I did have a glance the \nother day - I'm not that keen on the way the prompt behaves either\n\n> 1. http://lore.kernel.org/git/cover.1616411973.git.avarab@gmail.com\n\n>>> At that point we'll have forgotten that we asked for\n>>> --no-reschedule-failed-exec at the start, and will happily re-read the\n>>> config.\n>>> We'll then see that rebase.rescheduleFailedExec=true is set. At that\n>>> point we have no record of having set --no-reschedule-failed-exec\n>>> earlier. So the config will effectively override the user having\n>>> explicitly disabled the option on the command-line.\n>>> Even more confusingly: Since rebase accepts different options based\n>>> on\n>>> its state there wasn't even a way to get around this with \"rebase\n>>> --continue --no-reschedule-failed-exec\" (but you could of course set\n>>> the config with \"rebase -c ...\").\n>>> I think the least bad way out of this is to declare that for such\n>>> options and config whatever we decide at the beginning of the rebase\n>>> goes. So we'll now always create either a \"reschedule-failed-exec\" or\n>>> a \"no-reschedule-failed-exec file at the start, not just the former if\n>>> we decided we wanted the feature.\n>>\n>> Thanks for working on this and for the detailed commit message. I'm\n>> not entirely convinced we want yet another state file in\n>> .git/rebase-merge. We could we start writing the setting to the file\n>> rather than having different files for whether the option is on or\n>> off. If we use the contents of the file it could be -1 for 'use\n>> config', 0 'off', 1 'on'. The downside is that starting 'rebase\n>> --no-reschedule-failed-exec' with a new version of git and then\n>> continuing with an old version would do the wrong thing.\n> \n> Yes I suppose, but it seems much simpler indeed to just represent this\n> sort of tri-state as a ENOENT/no-FOO/FOO neither exists + file pair with\n> how the current code is set up, \n\nI can see that, I'm not a great fan of the current setup though - we're \nnot using shell scripting anymore so there's no need to add a new file \nfor each piece of state (though we need to keep the old ones for \nbackwards compatibility). Having said that I don't object to the current \napproach if it's easier and preserves backwards compatibility. If you \nfeel like adding support for ENOENT=\"use config\" that would be great so \nthat 'git -c ... rebase --continue' continues to work.\n\nBest Wishes\n\nPhillip\n\n> especially because (as you note) we'd\n> need to phase-in any writing of the content across multiple versions or\n> something, least in-progress rebases across versions subtly behave\n> weirdly.\n> \n> Well, in this case it's not such a big deal, but I'd rather not\n> establish the pattern for something that *does* matter.\n>> Best Wishes\n>>\n>> Phillip\n>>\n>>> With this new worldview you can no longer change the setting once a\n>>> rebase has started except by manually removing the state files\n>>> discussed above. I think making it work like that is the the least\n>>> confusing thing we can do.\n>>> In the future we might want to learn to change the setting in the\n>>> middle by combining \"--edit-todo\" with\n>>> \"--[no-]reschedule-failed-exec\", we currently don't support combining\n>>> those options, or any other way to change the state in the middle of\n>>> the rebase short of manually editing the files in\n>>> \".git/rebase-merge/*\".\n>>> The bug being fixed here originally came about because of a\n>>> combination of the behavior of the code added in d421afa0c66 (rebase:\n>>> introduce --reschedule-failed-exec, 2018-12-10) and the addition of\n>>> the config variable in 969de3ff0e0 (rebase: add a config option to\n>>> default to --reschedule-failed-exec, 2018-12-10).\n>>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>>> ---\n>>> Documentation/git-rebase.txt | 8 ++++++++\n>>> sequencer.c | 5 +++++\n>>> t/t3418-rebase-continue.sh | 25 +++++++++++++++++++++++++\n>>> 3 files changed, 38 insertions(+)\n>>> diff --git a/Documentation/git-rebase.txt\n>>> b/Documentation/git-rebase.txt\n>>> index a0487b5cc58..b48e6225769 100644\n>>> --- a/Documentation/git-rebase.txt\n>>> +++ b/Documentation/git-rebase.txt\n>>> @@ -622,6 +622,14 @@ See also INCOMPATIBLE OPTIONS below.\n>>> --no-reschedule-failed-exec::\n>>> \tAutomatically reschedule `exec` commands that failed. This only makes\n>>> \tsense in interactive mode (or when an `--exec` option was provided).\n>>> ++\n>>> +Even though this option applies once a rebase is started, it's set for\n>>> +the whole rebase at the start based on either the\n>>> +`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]\n>>> +or \"CONFIGURATION\" below) or whether this option is\n>>> +provided. Otherwise an explicit `--no-reschedule-failed-exec` at the\n>>> +start would be overridden by the presence of\n>>> +`rebase.rescheduleFailedExec=true` configuration.\n>>> INCOMPATIBLE OPTIONS\n>>> --------------------\n>>> diff --git a/sequencer.c b/sequencer.c\n>>> index 848204d3dc3..59735fdff62 100644\n>>> --- a/sequencer.c\n>>> +++ b/sequencer.c\n>>> @@ -164,6 +164,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, \"rebase-merge/strategy\")\n>>> static GIT_PATH_FUNC(rebase_path_strategy_opts, \"rebase-merge/strategy_opts\")\n>>> static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, \"rebase-merge/allow_rerere_autoupdate\")\n>>> static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, \"rebase-merge/reschedule-failed-exec\")\n>>> +static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, \"rebase-merge/no-reschedule-failed-exec\")\n>>> static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, \"rebase-merge/drop_redundant_commits\")\n>>> static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, \"rebase-merge/keep_redundant_commits\")\n>>> @@ -2672,6 +2673,8 @@ static int read_populate_opts(struct\n>>> replay_opts *opts)\n>>> \t\tif (file_exists(rebase_path_reschedule_failed_exec()))\n>>> \t\t\topts->reschedule_failed_exec = 1;\n>>> +\t\telse if (file_exists(rebase_path_no_reschedule_failed_exec()))\n>>> +\t\t\topts->reschedule_failed_exec = 0;\n>>> \t\tif (file_exists(rebase_path_drop_redundant_commits()))\n>>> \t\t\topts->drop_redundant_commits = 1;\n>>> @@ -2772,6 +2775,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n>>> \t\twrite_file(rebase_path_ignore_date(), \"%s\", \"\");\n>>> \tif (opts->reschedule_failed_exec)\n>>> \t\twrite_file(rebase_path_reschedule_failed_exec(), \"%s\", \"\");\n>>> +\telse\n>>> +\t\twrite_file(rebase_path_no_reschedule_failed_exec(), \"%s\", \"\");\n>>> \treturn 0;\n>>> }\n>>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n>>> index ea14ef496cb..9553d969646 100755\n>>> --- a/t/t3418-rebase-continue.sh\n>>> +++ b/t/t3418-rebase-continue.sh\n>>> @@ -291,4 +291,29 @@ test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '\n>>> \tgit rebase HEAD^\n>>> '\n>>> +test_expect_success 'rebase.rescheduleFailedExec=true &\n>>> --no-reschedule-failed-exec' '\n>>> +\ttest_when_finished \"git rebase --abort\" &&\n>>> +\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n>>> +\ttest_config rebase.rescheduleFailedExec true &&\n>>> +\ttest_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&\n>>> +\ttest_must_fail git rebase --continue 2>err &&\n>>> +\t! grep \"has been rescheduled\" err\n>>> +'\n>>> +\n>>> +test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '\n>>> +\ttest_when_finished \"git rebase --abort\" &&\n>>> +\ttest_must_fail git rebase -x false HEAD~2 &&\n>>> +\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n>>> +\ttest_config rebase.rescheduleFailedExec true &&\n>>> +\ttest_must_fail git rebase --continue 2>err &&\n>>> +\t! grep \"has been rescheduled\" err\n>>> +'\n>>> +\n>>> +test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebase' '\n>>> +\ttest_when_finished \"git rebase --abort\" &&\n>>> +\ttest_must_fail git rebase -x false HEAD~2 &&\n>>> +\ttest_expect_code 129 git rebase --continue --no-reschedule-failed-exec &&\n>>> +\ttest_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec\n>>> +'\n>>> +\n>>> test_done\n>>>\n> \n\n"},{"id":"420600","messageId":"9c11c7cf-3a37-a07b-83c3-89c24195c359@gmail.com","threadId":"49995","inReplyTo":"cover.1616411973.git.avarab@gmail.com","subject":"Re: [PATCH 0/3] rebase: don't override --no-reschedule-failed-exec with config","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-03-30T13:40:14Z","receivedAt":"2021-03-30T13:40:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 22/03/2021 11:48, Ævar Arnfjörð Bjarmason wrote:\n> I recently started using rebase.rescheduleFailedExec=true and noticed\n> this bug in its implementation. It's conceptually a relatively simple\n> fix, but as noted in 3/3 rebase being a \"start this operation, run\n> other command verbs later\" has an unintuitive interaction with our\n> usual \"command-line options override config\".\n> \n> !README FIRST!\n> Everthing after this line has no relevance to this series, it's just a\n> side musing on another (mis-)feature of --reschedule-failed-exec.\n> !/README FIRST!\n> \n> There's another bug/misfeature I've noticed with\n> rebase.rescheduleFailedExec=true (although maybe it'll be argued by\n> someone that it's a feature). Let's say you run:\n> \n> git rebase -x false --reschedule-failed-exec HEAD~2\n> \n> You'll now land in a state of (according to our helpful PS1 code, but\n> also the rebase state) of 2/4. Aside: Because you're rebasing 2\n> commits, we have 4 because of the 2x exec. So far so good and all of\n> that's expected.\n> \n> But now when you do \"git rebase --continue\" we'll fail as expected,\n> but not as expected (at least I find it funny) bump the count to 3/5,\n> then 4/6, 5/7 etc.\n> \n> With my \"I just read the code\" hat on this makes perfect sense. Every\n> time we process a TODO item we bump the count of processed items, and\n> under --reschedule-failed-exec we simply push the command that just\n> failed onto the list, hence the increasing done/TODO count when a\n> command fails.\n> \n> But I don't see how it makes any sense to expose this as UI via \"git\n> status\" and __git_ps1. I asked git to process this item of 4 sequencer\n> TODO items. If I fail an item it makes more sense that I just don't\n> get past it, not that under the hood a new identical item is\n> rescheduled for me. Just don't advance past the current item!\n> \n> Now if I've tried X times to make the \"make test\" pass for each commit\n> in my 3-commit series I'm going to be on item 10/12 or\n> whatever. There's no way to just look at that and see where I'm at in\n> the sequence.\n> \n> As an aside it would arguably make more sense to report 1/3 instead of\n> 2/6 for the first commit of 3 with a failing -x \"make test\", but you\n> can have X number of \"exec\" items and a manually edited list etc.\n\nI think counting the number of picks rather than total commands is \nprobably better especially now that we have reset and label commands as \nwell. Grouping all the execs with the previous pick doesn't seem that \nunreasonable to me (though I should note that I don't use the prompt \nfrom git.git and my prompt prints the command that we've stopped at on a \nline above the prompt so it is easy for me to see where I am in the list).\n\n> So that's probably a no-go but at least once I'm used to it I know if\n> i'm on 4/6 I'm on commit 2/3, with --reschedule-failed-exec you'll\n> have no idea what 12/14 or whatever means for where you are in your\n> 3-patch sequence, it has no relation to the TODO list you edited, just\n> rebase-merge's own internal state.\n> \n> Getting back on topic: This just seems like needlessly exposing an\n> implementation detail, I also asked to \"pick\" a commit, but if that\n> item \"fails\" e.g. due to:\n> \n> $ git rebase --continue\n> You must edit all merge conflicts and then\n> mark them as resolved using git add\n> \n> We don't push a new \"pick\" item on the list and inflate the count, so\n> why would we do that for \"exec\"?\n\nWe do push a new pick if a pick/merge/reset fails because it would \noverwrite untracked files.\n\nAs an aside I wish I could skip a rescheduled pick or exec with 'git \nrebase --skip' rather than having to edit the todo list (skipping reset \ncommands is a bad idea though)\n\n> Just say the command failed, return\n> its non-zero status from \"git rebase --continue\", and don't advance.\n> \n> Maybe it is useful to keep track of the N number of failures, and\n> e.g. report in __git_ps1:\n> \n> master|REBASE 2/6\n> master|REBASE 2(tries: 1)/6\n> master|REBASE 2(tries: 2)/6\n\nSomething like that would be nice but I think just not incrementing the \ncount when we reschedule a command would be fine.\n\n> Instead of the current:\n> \n> master|REBASE 2/6\n> master|REBASE 3/7\n> master|REBASE 4/8\n> \n> To say I'm on 2/6, but that I've tried 3 times already and failed to\n> advance past it.\n> \n> Anyway, I don't have patches for this side-report/rant. Looking at the\n> implementation it seemed more non-trivial than I was willing to\n> quickly fix.\n\nYeah, I think the implementation would need a bit of thought.\n\nIn summary I agree that it would be good to improve the count so it \nmakes more sense to the user\n\nBest Wishes\n\nPhillip\n\n\n> We bump the count fairly early before we even get to there being an\n> \"exec\" item, to implement this proposed view of the world we'd need to\n> defer that (or go back and edit it once we see \"failed exec\" and that\n> we're using --reschedule-failed-exec).\n> \n> I'm not familiar enough with the sequencer internals to know if trying\n> that would lead us down a path of e.g. having inconsistent or bad\n> state if we'd die in the middle of all of that.\n> \n> Ævar Arnfjörð Bjarmason (3):\n> rebase tests: camel-case rebase.rescheduleFailedExec consistently\n> rebase tests: use test_unconfig after test_config\n> rebase: don't override --no-reschedule-failed-exec with config\n> \n> Documentation/git-rebase.txt | 8 ++++++++\n> sequencer.c | 5 +++++\n> t/t3418-rebase-continue.sh | 30 ++++++++++++++++++++++++++++--\n> 3 files changed, 41 insertions(+), 2 deletions(-)\n> \n\n"},{"id":"420602","messageId":"53e9be89-db1e-b9ee-e93d-d8c8195480d4@gmail.com","threadId":"49995","inReplyTo":"330b33e7a8e87e528aec43c306753cdf573ed8af.1616411973.git.avarab@gmail.com","subject":"Re: [PATCH 2/3] rebase tests: use test_unconfig after test_config","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-03-30T13:53:14Z","receivedAt":"2021-03-30T13:54:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 22/03/2021 11:48, Ævar Arnfjörð Bjarmason wrote:\n> Fix a test added in 906b63942ac (rebase --am: ignore\n> rebase.rescheduleFailedExec, 2019-07-01) to reset its config after it\n> runs. This doesn't matter now since it's the last test in the file,\n> but will in a subsequent commit where I'll add new tests after this\n> one.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/t3418-rebase-continue.sh | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n> index fe407e63cf1..ea14ef496cb 100755\n> --- a/t/t3418-rebase-continue.sh\n> +++ b/t/t3418-rebase-continue.sh\n> @@ -283,6 +283,7 @@ test_expect_success '--reschedule-failed-exec' '\n> '\n> \n> test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '\n> +\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n\nI think test_config adds this test_when_finished line itself. See \ntest-lib-functions.sh:\n\n# Set git config, automatically unsetting it after the test is over.\ntest_config () {\n\tconfig_dir=\n\tif test \"$1\" = -C\n\tthen\n\t\tshift\n\t\tconfig_dir=$1\n\t\tshift\n\tfi\n\ttest_when_finished \"test_unconfig ${config_dir:+-C '$config_dir'} '$1'\" &&\n\tgit ${config_dir:+-C \"$config_dir\"} config \"$@\"\n}\n\nBest Wishes\n\nPhillip\n\n\nBest Wishes\n\nPhillip\n\n> \ttest_config rebase.rescheduleFailedExec true &&\n> \ttest_must_fail git rebase -x false HEAD^ &&\n> \tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo &&\n> \n"},{"id":"421345","messageId":"patch-1.3-e0dd2cb82a-20210409T075713Z-avarab@gmail.com","threadId":"49995","inReplyTo":"cover-0.3-0000000000-20210409T075713Z-avarab@gmail.com","subject":"[PATCH v2 1/2] rebase tests: camel-case rebase.rescheduleFailedExec consistently","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-09T08:01:37Z","receivedAt":"2021-04-09T08:01:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a test added in 906b63942ac (rebase --am: ignore\nrebase.rescheduleFailedExec, 2019-07-01) to camel-case the\nconfiguration variable. This doesn't change the behavior of the test,\nit's merely to help its human readers.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t3418-rebase-continue.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 0838f4e798..fe407e63cf 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -282,8 +282,8 @@ test_expect_success '--reschedule-failed-exec' '\n \ttest_i18ngrep \"has been rescheduled\" err\n '\n \n-test_expect_success 'rebase.reschedulefailedexec only affects `rebase -i`' '\n-\ttest_config rebase.reschedulefailedexec true &&\n+test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '\n+\ttest_config rebase.rescheduleFailedExec true &&\n \ttest_must_fail git rebase -x false HEAD^ &&\n \tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo &&\n \tgit rebase --abort &&\n-- \n2.31.1.584.gf4baedee75\n\n"},{"id":"421346","messageId":"cover-0.3-0000000000-20210409T075713Z-avarab@gmail.com","threadId":"49995","inReplyTo":"cover.1616411973.git.avarab@gmail.com","subject":"[PATCH v2 0/2] rebase: don't override --no-reschedule-failed-exec with config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-09T08:01:36Z","receivedAt":"2021-04-09T08:01:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This fixes a bug where we read config & options when \"git rebase -i -x\nmake\" starts, and will understand the \"--no-reschedule-failed-exec\"\noption, but once a command fails we'll lose track of having had a\n\"--no-reschedule-failed-exec\" and will happily re-read the\n\"rebase.rescheduleFailedExec=true\" config the user might have.\n\nThus we'll have config winning over explicit command-line\noptions. This series fixes that bug.\n\nChanges since v1:\n\n * I forgot how test_config works, and was doing a test_when_finished\n \"test_unconfig\", which isn't needed, duh! Thanks to Phillip Wood\n for that & other reviews on this series.\n\n * There was a discussion about how to add yet another rebase\n machinery state file. I think the consensus is \"let's just do it\n like this\", i.e. we could write some tri-state content to the file,\n but we'd get into back-compat issues with other git versions.\n\nThere was a discussiob about how to manage this whole state (a DB,\nJSON?) in another thread, let's kick the can of how exactly we store\nstate down the line, and just fix the bug using the existing state\nsaving convention for now.\n\nÆvar Arnfjörð Bjarmason (2):\n rebase tests: camel-case rebase.rescheduleFailedExec consistently\n rebase: don't override --no-reschedule-failed-exec with config\n\n Documentation/git-rebase.txt | 8 ++++++++\n sequencer.c | 5 +++++\n t/t3418-rebase-continue.sh | 27 +++++++++++++++++++++++++--\n 3 files changed, 38 insertions(+), 2 deletions(-)\n\nRange-diff against v1:\n1: 002dc72ee7 = 1: e0dd2cb82a rebase tests: camel-case rebase.rescheduleFailedExec consistently\n2: 330b33e7a8 < -: ---------- rebase tests: use test_unconfig after test_config\n3: e00300d58d ! 2: 7991160de3 rebase: don't override --no-reschedule-failed-exec with config\n @@ Commit message\n However the --reschedule-failed-exec option doesn't take effect when\n the rebase starts (we'd just create a\n \".git/rebase-merge/reschedule-failed-exec\" file if it was true). It\n - only takes effect when the exec command fails, and the user wants to\n - run \"rebase --continue\".\n + only takes effect when the exec command fails, at which point we'll\n + reschedule the failed \"exec\" command.\n \n - At that point we'll have forgotten that we asked for\n - --no-reschedule-failed-exec at the start, and will happily re-read the\n - config.\n + Since we only wrote out the positive\n + \".git/rebase-merge/reschedule-failed-exec\" under\n + --reschedule-failed-exec, but nothing with --no-reschedule-failed-exec\n + we'll forget that we asked not to reschedule failed \"exec\", and would\n + happily re-read the config and see that\n + rebase.rescheduleFailedExec=true is set.\n \n - We'll then see that rebase.rescheduleFailedExec=true is set. At that\n - point we have no record of having set --no-reschedule-failed-exec\n - earlier. So the config will effectively override the user having\n - explicitly disabled the option on the command-line.\n + So the config will effectively override the user having explicitly\n + disabled the option on the command-line.\n \n Even more confusingly: Since rebase accepts different options based on\n its state there wasn't even a way to get around this with \"rebase\n @@ t/t3418-rebase-continue.sh: test_expect_success 'rebase.rescheduleFailedExec onl\n \n +test_expect_success 'rebase.rescheduleFailedExec=true & --no-reschedule-failed-exec' '\n +\ttest_when_finished \"git rebase --abort\" &&\n -+\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n +\ttest_config rebase.rescheduleFailedExec true &&\n +\ttest_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&\n +\ttest_must_fail git rebase --continue 2>err &&\n @@ t/t3418-rebase-continue.sh: test_expect_success 'rebase.rescheduleFailedExec onl\n +test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '\n +\ttest_when_finished \"git rebase --abort\" &&\n +\ttest_must_fail git rebase -x false HEAD~2 &&\n -+\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n +\ttest_config rebase.rescheduleFailedExec true &&\n +\ttest_must_fail git rebase --continue 2>err &&\n +\t! grep \"has been rescheduled\" err\n-- \n2.31.1.584.gf4baedee75\n\n"},{"id":"421347","messageId":"patch-2.3-7991160de3-20210409T075713Z-avarab@gmail.com","threadId":"49995","inReplyTo":"cover-0.3-0000000000-20210409T075713Z-avarab@gmail.com","subject":"[PATCH v2 2/2] rebase: don't override --no-reschedule-failed-exec with config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-09T08:01:38Z","receivedAt":"2021-04-09T08:01:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bug in how --no-reschedule-failed-exec interacts with\nrebase.rescheduleFailedExec=true being set in the config. Before this\nchange the --no-reschedule-failed-exec config option would be\noverridden by the config.\n\nThis bug happened because of the particulars of how \"rebase\" works\nv.s. most other git commands when it comes to parsing options and\nconfig:\n\nWhen we read the config and parse the CLI options we correctly prefer\nthe --no-reschedule-failed-exec option over\nrebase.rescheduleFailedExec=true in the config. So far so good.\n\nHowever the --reschedule-failed-exec option doesn't take effect when\nthe rebase starts (we'd just create a\n\".git/rebase-merge/reschedule-failed-exec\" file if it was true). It\nonly takes effect when the exec command fails, at which point we'll\nreschedule the failed \"exec\" command.\n\nSince we only wrote out the positive\n\".git/rebase-merge/reschedule-failed-exec\" under\n--reschedule-failed-exec, but nothing with --no-reschedule-failed-exec\nwe'll forget that we asked not to reschedule failed \"exec\", and would\nhappily re-read the config and see that\nrebase.rescheduleFailedExec=true is set.\n\nSo the config will effectively override the user having explicitly\ndisabled the option on the command-line.\n\nEven more confusingly: Since rebase accepts different options based on\nits state there wasn't even a way to get around this with \"rebase\n--continue --no-reschedule-failed-exec\" (but you could of course set\nthe config with \"rebase -c ...\").\n\nI think the least bad way out of this is to declare that for such\noptions and config whatever we decide at the beginning of the rebase\ngoes. So we'll now always create either a \"reschedule-failed-exec\" or\na \"no-reschedule-failed-exec file at the start, not just the former if\nwe decided we wanted the feature.\n\nWith this new worldview you can no longer change the setting once a\nrebase has started except by manually removing the state files\ndiscussed above. I think making it work like that is the the least\nconfusing thing we can do.\n\nIn the future we might want to learn to change the setting in the\nmiddle by combining \"--edit-todo\" with\n\"--[no-]reschedule-failed-exec\", we currently don't support combining\nthose options, or any other way to change the state in the middle of\nthe rebase short of manually editing the files in\n\".git/rebase-merge/*\".\n\nThe bug being fixed here originally came about because of a\ncombination of the behavior of the code added in d421afa0c66 (rebase:\nintroduce --reschedule-failed-exec, 2018-12-10) and the addition of\nthe config variable in 969de3ff0e0 (rebase: add a config option to\ndefault to --reschedule-failed-exec, 2018-12-10).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-rebase.txt | 8 ++++++++\n sequencer.c | 5 +++++\n t/t3418-rebase-continue.sh | 23 +++++++++++++++++++++++\n 3 files changed, 36 insertions(+)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex f08ae27e2a..93afe5ce3c 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -623,6 +623,14 @@ See also INCOMPATIBLE OPTIONS below.\n --no-reschedule-failed-exec::\n \tAutomatically reschedule `exec` commands that failed. This only makes\n \tsense in interactive mode (or when an `--exec` option was provided).\n++\n+Even though this option applies once a rebase is started, it's set for\n+the whole rebase at the start based on either the\n+`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]\n+or \"CONFIGURATION\" below) or whether this option is\n+provided. Otherwise an explicit `--no-reschedule-failed-exec` at the\n+start would be overridden by the presence of\n+`rebase.rescheduleFailedExec=true` configuration.\n \n INCOMPATIBLE OPTIONS\n --------------------\ndiff --git a/sequencer.c b/sequencer.c\nindex b4c63ae207..8a7e7414eb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -164,6 +164,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, \"rebase-merge/strategy\")\n static GIT_PATH_FUNC(rebase_path_strategy_opts, \"rebase-merge/strategy_opts\")\n static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, \"rebase-merge/allow_rerere_autoupdate\")\n static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, \"rebase-merge/reschedule-failed-exec\")\n+static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, \"rebase-merge/no-reschedule-failed-exec\")\n static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, \"rebase-merge/drop_redundant_commits\")\n static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, \"rebase-merge/keep_redundant_commits\")\n \n@@ -2854,6 +2855,8 @@ static int read_populate_opts(struct replay_opts *opts)\n \n \t\tif (file_exists(rebase_path_reschedule_failed_exec()))\n \t\t\topts->reschedule_failed_exec = 1;\n+\t\telse if (file_exists(rebase_path_no_reschedule_failed_exec()))\n+\t\t\topts->reschedule_failed_exec = 0;\n \n \t\tif (file_exists(rebase_path_drop_redundant_commits()))\n \t\t\topts->drop_redundant_commits = 1;\n@@ -2954,6 +2957,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\twrite_file(rebase_path_ignore_date(), \"%s\", \"\");\n \tif (opts->reschedule_failed_exec)\n \t\twrite_file(rebase_path_reschedule_failed_exec(), \"%s\", \"\");\n+\telse\n+\t\twrite_file(rebase_path_no_reschedule_failed_exec(), \"%s\", \"\");\n \n \treturn 0;\n }\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex fe407e63cf..f4c2ee02bc 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -290,4 +290,27 @@ test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '\n \tgit rebase HEAD^\n '\n \n+test_expect_success 'rebase.rescheduleFailedExec=true & --no-reschedule-failed-exec' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\ttest_config rebase.rescheduleFailedExec true &&\n+\ttest_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&\n+\ttest_must_fail git rebase --continue 2>err &&\n+\t! grep \"has been rescheduled\" err\n+'\n+\n+test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\ttest_must_fail git rebase -x false HEAD~2 &&\n+\ttest_config rebase.rescheduleFailedExec true &&\n+\ttest_must_fail git rebase --continue 2>err &&\n+\t! grep \"has been rescheduled\" err\n+'\n+\n+test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebase' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\ttest_must_fail git rebase -x false HEAD~2 &&\n+\ttest_expect_code 129 git rebase --continue --no-reschedule-failed-exec &&\n+\ttest_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec\n+'\n+\n test_done\n-- \n2.31.1.584.gf4baedee75\n\n"},{"id":"422033","messageId":"63774b95-7de2-b160-385b-6c7d99e867ed@gmail.com","threadId":"49995","inReplyTo":"cover-0.3-0000000000-20210409T075713Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/2] rebase: don't override --no-reschedule-failed-exec with config","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-04-15T15:24:30Z","receivedAt":"2021-04-15T15:24:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 09/04/2021 09:01, Ævar Arnfjörð Bjarmason wrote:\n> This fixes a bug where we read config & options when \"git rebase -i -x\n> make\" starts, and will understand the \"--no-reschedule-failed-exec\"\n> option, but once a command fails we'll lose track of having had a\n> \"--no-reschedule-failed-exec\" and will happily re-read the\n> \"rebase.rescheduleFailedExec=true\" config the user might have.\n> \n> Thus we'll have config winning over explicit command-line\n> options. This series fixes that bug.\n> \n> Changes since v1:\n> \n> * I forgot how test_config works, and was doing a test_when_finished\n> \"test_unconfig\", which isn't needed, duh! Thanks to Phillip Wood\n> for that & other reviews on this series.\n> \n> * There was a discussion about how to add yet another rebase\n> machinery state file. I think the consensus is \"let's just do it\n> like this\", i.e. we could write some tri-state content to the file,\n> but we'd get into back-compat issues with other git versions.\n\nThese patches look fine to me now\n\nThanks\n\nPhillip\n\n> There was a discussiob about how to manage this whole state (a DB,\n> JSON?) in another thread, let's kick the can of how exactly we store\n> state down the line, and just fix the bug using the existing state\n> saving convention for now.\n> \n> Ævar Arnfjörð Bjarmason (2):\n> rebase tests: camel-case rebase.rescheduleFailedExec consistently\n> rebase: don't override --no-reschedule-failed-exec with config\n> \n> Documentation/git-rebase.txt | 8 ++++++++\n> sequencer.c | 5 +++++\n> t/t3418-rebase-continue.sh | 27 +++++++++++++++++++++++++--\n> 3 files changed, 38 insertions(+), 2 deletions(-)\n> \n> Range-diff against v1:\n> 1: 002dc72ee7 = 1: e0dd2cb82a rebase tests: camel-case rebase.rescheduleFailedExec consistently\n> 2: 330b33e7a8 < -: ---------- rebase tests: use test_unconfig after test_config\n> 3: e00300d58d ! 2: 7991160de3 rebase: don't override --no-reschedule-failed-exec with config\n> @@ Commit message\n> However the --reschedule-failed-exec option doesn't take effect when\n> the rebase starts (we'd just create a\n> \".git/rebase-merge/reschedule-failed-exec\" file if it was true). It\n> - only takes effect when the exec command fails, and the user wants to\n> - run \"rebase --continue\".\n> + only takes effect when the exec command fails, at which point we'll\n> + reschedule the failed \"exec\" command.\n> \n> - At that point we'll have forgotten that we asked for\n> - --no-reschedule-failed-exec at the start, and will happily re-read the\n> - config.\n> + Since we only wrote out the positive\n> + \".git/rebase-merge/reschedule-failed-exec\" under\n> + --reschedule-failed-exec, but nothing with --no-reschedule-failed-exec\n> + we'll forget that we asked not to reschedule failed \"exec\", and would\n> + happily re-read the config and see that\n> + rebase.rescheduleFailedExec=true is set.\n> \n> - We'll then see that rebase.rescheduleFailedExec=true is set. At that\n> - point we have no record of having set --no-reschedule-failed-exec\n> - earlier. So the config will effectively override the user having\n> - explicitly disabled the option on the command-line.\n> + So the config will effectively override the user having explicitly\n> + disabled the option on the command-line.\n> \n> Even more confusingly: Since rebase accepts different options based on\n> its state there wasn't even a way to get around this with \"rebase\n> @@ t/t3418-rebase-continue.sh: test_expect_success 'rebase.rescheduleFailedExec onl\n> \n> +test_expect_success 'rebase.rescheduleFailedExec=true & --no-reschedule-failed-exec' '\n> +\ttest_when_finished \"git rebase --abort\" &&\n> -+\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n> +\ttest_config rebase.rescheduleFailedExec true &&\n> +\ttest_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&\n> +\ttest_must_fail git rebase --continue 2>err &&\n> @@ t/t3418-rebase-continue.sh: test_expect_success 'rebase.rescheduleFailedExec onl\n> +test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '\n> +\ttest_when_finished \"git rebase --abort\" &&\n> +\ttest_must_fail git rebase -x false HEAD~2 &&\n> -+\ttest_when_finished \"test_unconfig rebase.rescheduleFailedExec\" &&\n> +\ttest_config rebase.rescheduleFailedExec true &&\n> +\ttest_must_fail git rebase --continue 2>err &&\n> +\t! grep \"has been rescheduled\" err\n> \n"}]}