{"thread":{"id":"52266","subject":"[PATCH 0/3] rebase --edit-todo --exec","startedAt":"2019-11-14T16:35:55Z","lastAt":"2019-11-22T19:08:40Z","messageCount":13,"participants":["Andrei Rybak","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"386187","messageId":"20191114163549.7648-1-rybak.a.v@gmail.com","threadId":"52266","inReplyTo":null,"subject":"[PATCH 0/3] rebase --edit-todo --exec","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-11-14T16:35:46Z","receivedAt":"2019-11-14T16:35:55Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"I've wanted this feature for a long time, and now with rebase working without\nforking rebase--interactive (thanks to Phillip Wood for working on that), I\nfinally got around to implementing it.\n\nIt still needs validation for arguments. Right now, I have two ideas:\n\n  1. iterate over original argv and make sure its just '--exec's with its\n     arguments.\n  2. check all other vars in options[].\n\nAndrei Rybak (3):\n  rebase: prepare cmd before choosing action\n  rebase: extract add_exec()\n  rebase -i: allow --edit-todo with --exec\n\n Documentation/git-rebase.txt  |  3 +-\n builtin/rebase.c              | 63 ++++++++++++++++++++++-------------\n t/t3404-rebase-interactive.sh | 32 ++++++++++++++++++\n 3 files changed, 74 insertions(+), 24 deletions(-)\n\n-- \n2.24.0.windows.2\n\n"},{"id":"386188","messageId":"20191114163549.7648-2-rybak.a.v@gmail.com","threadId":"52266","inReplyTo":"20191114163549.7648-1-rybak.a.v@gmail.com","subject":"[PATCH 1/3] rebase: prepare cmd before choosing action","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-11-14T16:35:47Z","receivedAt":"2019-11-14T16:35:57Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"When git rebase is started with option --exec, its arguments are parsed\ninto string_list exec and then converted into options.cmd.\n\nIn following commits, action --edit-todo will be taught to use arguments\npassed with --exec option.  Prepare options.cmd before switch (action)\nto make it available for the ACTION_EDIT_TODO branch of the switch.\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n---\n builtin/rebase.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 4a20582e72..9457912f9d 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1595,6 +1595,21 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\ttrace2_cmd_mode(action_names[action]);\n \t}\n \n+\tfor (i = 0; i < exec.nr; i++)\n+\t\tif (check_exec_cmd(exec.items[i].string))\n+\t\t\texit(1);\n+\n+\tif (exec.nr) {\n+\t\tint i;\n+\n+\t\timply_interactive(&options, \"--exec\");\n+\n+\t\tstrbuf_reset(&buf);\n+\t\tfor (i = 0; i < exec.nr; i++)\n+\t\t\tstrbuf_addf(&buf, \"exec %s\\n\", exec.items[i].string);\n+\t\toptions.cmd = xstrdup(buf.buf);\n+\t}\n+\n \tswitch (action) {\n \tcase ACTION_CONTINUE: {\n \t\tstruct object_id head;\n@@ -1731,10 +1746,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tfor (i = 0; i < exec.nr; i++)\n-\t\tif (check_exec_cmd(exec.items[i].string))\n-\t\t\texit(1);\n-\n \tif (!(options.flags & REBASE_NO_QUIET))\n \t\targv_array_push(&options.git_am_opts, \"-q\");\n \n@@ -1746,17 +1757,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n \t}\n \n-\tif (exec.nr) {\n-\t\tint i;\n-\n-\t\timply_interactive(&options, \"--exec\");\n-\n-\t\tstrbuf_reset(&buf);\n-\t\tfor (i = 0; i < exec.nr; i++)\n-\t\t\tstrbuf_addf(&buf, \"exec %s\\n\", exec.items[i].string);\n-\t\toptions.cmd = xstrdup(buf.buf);\n-\t}\n-\n \tif (rebase_merges) {\n \t\tif (!*rebase_merges)\n \t\t\t; /* default mode; do nothing */\n-- \n2.24.0.windows.2\n\n"},{"id":"386189","messageId":"20191114163549.7648-3-rybak.a.v@gmail.com","threadId":"52266","inReplyTo":"20191114163549.7648-1-rybak.a.v@gmail.com","subject":"[PATCH 2/3] rebase: extract add_exec()","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-11-14T16:35:48Z","receivedAt":"2019-11-14T16:35:58Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"In following commit addition of commands to the todo file will be\nimplemented for the --edit-todo action.  So extract a function to avoid\nduplicating the code for splitting of opts->cmd into list of commands.\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n---\n builtin/rebase.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 9457912f9d..27507d3cf6 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -299,6 +299,17 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)\n \t}\n }\n \n+static int add_exec(const char *cmd)\n+{\n+\tstruct string_list commands = STRING_LIST_INIT_DUP;\n+\tint ret;\n+\n+\tsplit_exec_commands(cmd, &commands);\n+\tret = add_exec_commands(&commands);\n+\tstring_list_clear(&commands, 0);\n+\treturn ret;\n+}\n+\n static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n {\n \tint ret;\n@@ -420,11 +431,7 @@ static int run_rebase_interactive(struct rebase_options *opts,\n \t\tret = rearrange_squash_in_todo_file();\n \t\tbreak;\n \tcase ACTION_ADD_EXEC: {\n-\t\tstruct string_list commands = STRING_LIST_INIT_DUP;\n-\n-\t\tsplit_exec_commands(opts->cmd, &commands);\n-\t\tret = add_exec_commands(&commands);\n-\t\tstring_list_clear(&commands, 0);\n+\t\tret = add_exec(opts->cmd);\n \t\tbreak;\n \t}\n \tdefault:\n-- \n2.24.0.windows.2\n\n"},{"id":"386190","messageId":"20191114163549.7648-4-rybak.a.v@gmail.com","threadId":"52266","inReplyTo":"20191114163549.7648-1-rybak.a.v@gmail.com","subject":"[PATCH 3/3] rebase -i: allow --edit-todo with --exec","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-11-14T16:35:49Z","receivedAt":"2019-11-14T16:36:01Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"When using rebase, option --exec can be used, for example, to run tests\nafter every commit created by rebase.  When using interactive rebase, I\ndon't always know, if I would want to run something against each commit\nbefore I start rebasing.  Sometimes, I realize this only after doing\nsome editing in the middle of the rebase.  To do that, I have to\nmanually edit the todo file.  Additing exec command by hand or\nsemi-automatically is cumbersome and error prone.  Especially if the\nfile is big or complex, e.g. when option --rebase-merges is used.\n\nAllow using the --edit-todo action of git rebase with option --exec.\nNew test is based on test 'rebase --edit-todo can be used to modify\ntodo'.  Contents of todo file are checked using set_cat_todo_editor\nsimilarly to what test 'respects rebase.abbreviateCommands with fixup,\nsquash and exec' does.\n\nRemove unnecessary braces around call to usage_with_options, while we're\nhere.\n\nTODO: Still need better validation of options. With current\n      implementation, the following is not rejected:\n\n\tgit rebase --edit-todo -x 'git show HEAD' --autostash\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n---\n Documentation/git-rebase.txt  |  3 ++-\n builtin/rebase.c              | 16 +++++++++++++---\n t/t3404-rebase-interactive.sh | 32 ++++++++++++++++++++++++++++++++\n 3 files changed, 47 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 639a4179d1..b5db5e80d4 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -12,7 +12,8 @@ SYNOPSIS\n \t[--onto <newbase> | --keep-base] [<upstream> [<branch>]]\n 'git rebase' [-i | --interactive] [<options>] [--exec <cmd>] [--onto <newbase>]\n \t--root [<branch>]\n-'git rebase' (--continue | --skip | --abort | --quit | --edit-todo | --show-current-patch)\n+'git rebase' (--continue | --skip | --abort | --quit |\n+\t--edit-todo [--exec<cmd>] | --show-current-patch)\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 27507d3cf6..1ee55b48e7 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -33,7 +33,8 @@ static char const * const builtin_rebase_usage[] = {\n \t\t\"[--onto <newbase> | --keep-base] [<upstream> [<branch>]]\"),\n \tN_(\"git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] \"\n \t\t\"--root [<branch>]\"),\n-\tN_(\"git rebase --continue | --abort | --skip | --edit-todo\"),\n+\tN_(\"git rebase --continue | --abort | --skip | \"\n+\t\t\"--edit-todo [--exec <cmd>]\"),\n \tNULL\n };\n \n@@ -409,6 +410,11 @@ static int run_rebase_interactive(struct rebase_options *opts,\n \t\tbreak;\n \t}\n \tcase ACTION_EDIT_TODO:\n+\t\tif (opts->cmd) {\n+\t\t\tret = add_exec(opts->cmd);\n+\t\t\tif (ret)\n+\t\t\t\tbreak;\n+\t\t}\n \t\tret = edit_todo_file(flags);\n \t\tbreak;\n \tcase ACTION_SHOW_CURRENT_PATCH: {\n@@ -1565,15 +1571,19 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t     builtin_rebase_options,\n \t\t\t     builtin_rebase_usage, 0);\n \n-\tif (action != ACTION_NONE && total_argc != 2) {\n+\tif (action != ACTION_NONE && action != ACTION_EDIT_TODO &&\n+\t    total_argc != 2)\n \t\tusage_with_options(builtin_rebase_usage,\n \t\t\t\t   builtin_rebase_options);\n-\t}\n \n \tif (argc > 2)\n \t\tusage_with_options(builtin_rebase_usage,\n \t\t\t\t   builtin_rebase_options);\n \n+\tif (action == ACTION_EDIT_TODO && argc > 0)\n+\t\tusage_with_options(builtin_rebase_usage,\n+\t\t\t\t   builtin_rebase_options);\n+\n \tif (options.type == REBASE_PRESERVE_MERGES)\n \t\twarning(_(\"git rebase --preserve-merges is deprecated. \"\n \t\t\t  \"Use --rebase-merges instead.\"));\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d2dfbe46b9..5decb8570e 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1082,6 +1082,38 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' '\n \ttest L = $(git cat-file commit HEAD | sed -ne \\$p)\n '\n \n+test_expect_success 'rebase --edit-todo can be used with -x' '\n+\ttest_when_finished \"reset_rebase\" &&\n+\tgit reset --hard &&\n+\tgit checkout no-conflict-branch^0 &&\n+\tcat >expected <<-EOF &&\n+\tpick $(git rev-list --abbrev-commit -1 HEAD^) L\n+\texec git show HEAD\n+\tpick $(git rev-list --abbrev-commit -1 HEAD) M\n+\texec git show HEAD\n+\tEOF\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"1 edit 2 3 4\" git rebase -i HEAD~4 &&\n+\tset_cat_todo_editor &&\n+\ttest_must_fail git rebase --edit-todo -x \"git show HEAD\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'rebase --edit-todo -x does not allow other arguments' '\n+\ttest_when_finished \"reset_rebase\" &&\n+\tgit reset --hard &&\n+\tgit checkout no-conflict-branch^0 &&\n+\tcat >expected <<-EOF &&\n+\tpick $(git rev-list --abbrev-commit -1 HEAD^) L\n+\texec git show HEAD\n+\tpick $(git rev-list --abbrev-commit -1 HEAD) M\n+\texec git show HEAD\n+\tEOF\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"1 edit 2 3 4\" git rebase -i HEAD~4 &&\n+\ttest_must_fail git rebase --edit-todo -x \"git show HEAD\" --autostash\n+'\n+\n test_expect_success 'rebase -i produces readable reflog' '\n \tgit reset --hard &&\n \tgit branch -f branch-reflog-test H &&\n-- \n2.24.0.windows.2\n\n"},{"id":"386258","messageId":"xmqqk1817kq4.fsf@gitster-ct.c.googlers.com","threadId":"52266","inReplyTo":"20191114163549.7648-2-rybak.a.v@gmail.com","subject":"Re: [PATCH 1/3] rebase: prepare cmd before choosing action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-15T05:49:07Z","receivedAt":"2019-11-15T05:49:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak <rybak.a.v@gmail.com> writes:\n\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 4a20582e72..9457912f9d 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1595,6 +1595,21 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>  \t\t\ttrace2_cmd_mode(action_names[action]);\n>  \t}\n>  \n> +\tfor (i = 0; i < exec.nr; i++)\n> +\t\tif (check_exec_cmd(exec.items[i].string))\n> +\t\t\texit(1);\n> +\n> +\tif (exec.nr) {\n> +\t\tint i;\n\nThis masks the outer \"i\" (I am assuming such a thing exists, judging\nfrom its use in the above loop you added), but I do not see a need\nfor it.  With or without this masked local \"i\", the value of outer \"i\"\nwill be equal to exec.nr+1 after the control leaves this if block.\n\n> +\t\timply_interactive(&options, \"--exec\");\n> +\n> +\t\tstrbuf_reset(&buf);\n> +\t\tfor (i = 0; i < exec.nr; i++)\n> +\t\t\tstrbuf_addf(&buf, \"exec %s\\n\", exec.items[i].string);\n> +\t\toptions.cmd = xstrdup(buf.buf);\n> +\t}\n> +\n"},{"id":"386624","messageId":"20191120095238.4349-2-rybak.a.v@gmail.com","threadId":"52266","inReplyTo":"20191114163549.7648-1-rybak.a.v@gmail.com","subject":"[RFC PATCH v2 1/4] builtin/rebase.c: reuse loop variable","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-11-20T09:52:35Z","receivedAt":"2019-11-20T09:52:46Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Variable \"int i\" is already defined at the top of the function\ncmd_rebase, so reuse it instead of declaring other variables, which mask\nthe outer \"i\".\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n---\n builtin/rebase.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 4a20582e72..793cac1386 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1747,8 +1747,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (exec.nr) {\n-\t\tint i;\n-\n \t\timply_interactive(&options, \"--exec\");\n \n \t\tstrbuf_reset(&buf);\n@@ -1769,8 +1767,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (strategy_options.nr) {\n-\t\tint i;\n-\n \t\tif (!options.strategy)\n \t\t\toptions.strategy = \"recursive\";\n \n-- \n2.24.0.windows.2\n\n"},{"id":"386625","messageId":"20191120095238.4349-3-rybak.a.v@gmail.com","threadId":"52266","inReplyTo":"20191114163549.7648-1-rybak.a.v@gmail.com","subject":"[RFC PATCH v2 2/4] rebase: prepare cmd before choosing action","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-11-20T09:52:36Z","receivedAt":"2019-11-20T09:52:47Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"When git rebase is started with option --exec, its arguments are parsed\ninto string_list exec and then converted into options.cmd.\n\nIn following commits, action --edit-todo will be taught to use arguments\npassed with --exec option.  Prepare options.cmd before switch (action)\nto make it available for the ACTION_EDIT_TODO branch of the switch.\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n---\n builtin/rebase.c | 26 +++++++++++++-------------\n 1 file changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 793cac1386..fa27f7b439 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1595,6 +1595,19 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\ttrace2_cmd_mode(action_names[action]);\n \t}\n \n+\tfor (i = 0; i < exec.nr; i++)\n+\t\tif (check_exec_cmd(exec.items[i].string))\n+\t\t\texit(1);\n+\n+\tif (exec.nr) {\n+\t\timply_interactive(&options, \"--exec\");\n+\n+\t\tstrbuf_reset(&buf);\n+\t\tfor (i = 0; i < exec.nr; i++)\n+\t\t\tstrbuf_addf(&buf, \"exec %s\\n\", exec.items[i].string);\n+\t\toptions.cmd = xstrdup(buf.buf);\n+\t}\n+\n \tswitch (action) {\n \tcase ACTION_CONTINUE: {\n \t\tstruct object_id head;\n@@ -1731,10 +1744,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tfor (i = 0; i < exec.nr; i++)\n-\t\tif (check_exec_cmd(exec.items[i].string))\n-\t\t\texit(1);\n-\n \tif (!(options.flags & REBASE_NO_QUIET))\n \t\targv_array_push(&options.git_am_opts, \"-q\");\n \n@@ -1746,15 +1755,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n \t}\n \n-\tif (exec.nr) {\n-\t\timply_interactive(&options, \"--exec\");\n-\n-\t\tstrbuf_reset(&buf);\n-\t\tfor (i = 0; i < exec.nr; i++)\n-\t\t\tstrbuf_addf(&buf, \"exec %s\\n\", exec.items[i].string);\n-\t\toptions.cmd = xstrdup(buf.buf);\n-\t}\n-\n \tif (rebase_merges) {\n \t\tif (!*rebase_merges)\n \t\t\t; /* default mode; do nothing */\n-- \n2.24.0.windows.2\n\n"},{"id":"386626","messageId":"20191120095238.4349-1-rybak.a.v@gmail.com","threadId":"52266","inReplyTo":"20191114163549.7648-1-rybak.a.v@gmail.com","subject":"[RFC PATCH v2 0/4] rebase --edit-todo --exec","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-11-20T09:52:34Z","receivedAt":"2019-11-20T09:52:47Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Differences from v1:\n  - Added missing RFC marker.\n  - Fixed CC list.\n  - Addressed Junio's comment about masked variable in a for loop: another\n    preparatory patch is added.\n  - Cleaned up copy-pasted unused code in the new test.\n\n----\n\nOriginal cover letter:\nI've wanted this feature for a long time, and now with rebase working without\nforking rebase--interactive (thanks to Phillip Wood for working on that), I\nfinally got around to implementing it.\n\nIt still needs validation for arguments. Right now, I have two ideas:\n\n  1. iterate over original argv and make sure its just '--exec's with its\n     arguments.\n  2. check all other vars in options[].\n\nAndrei Rybak (4):\n  builtin/rebase.c: reuse loop variable\n  rebase: prepare cmd before choosing action\n  rebase: extract add_exec()\n  rebase -i: allow --edit-todo with --exec\n\n Documentation/git-rebase.txt  |  3 +-\n builtin/rebase.c              | 63 +++++++++++++++++++++--------------\n t/t3404-rebase-interactive.sh | 32 ++++++++++++++++++\n 3 files changed, 72 insertions(+), 26 deletions(-)\n\n-- \n2.24.0.windows.2\n\n"},{"id":"386627","messageId":"20191120095238.4349-4-rybak.a.v@gmail.com","threadId":"52266","inReplyTo":"20191114163549.7648-1-rybak.a.v@gmail.com","subject":"[RFC PATCH v2 3/4] rebase: extract add_exec()","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-11-20T09:52:37Z","receivedAt":"2019-11-20T09:52:50Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"In following commit addition of commands to the todo file will be\nimplemented for the --edit-todo action.  So extract a function to avoid\nduplicating the code for splitting of opts->cmd into list of commands.\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n---\n builtin/rebase.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex fa27f7b439..8a6ac7439b 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -299,6 +299,17 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)\n \t}\n }\n \n+static int add_exec(const char *cmd)\n+{\n+\tstruct string_list commands = STRING_LIST_INIT_DUP;\n+\tint ret;\n+\n+\tsplit_exec_commands(cmd, &commands);\n+\tret = add_exec_commands(&commands);\n+\tstring_list_clear(&commands, 0);\n+\treturn ret;\n+}\n+\n static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n {\n \tint ret;\n@@ -420,11 +431,7 @@ static int run_rebase_interactive(struct rebase_options *opts,\n \t\tret = rearrange_squash_in_todo_file();\n \t\tbreak;\n \tcase ACTION_ADD_EXEC: {\n-\t\tstruct string_list commands = STRING_LIST_INIT_DUP;\n-\n-\t\tsplit_exec_commands(opts->cmd, &commands);\n-\t\tret = add_exec_commands(&commands);\n-\t\tstring_list_clear(&commands, 0);\n+\t\tret = add_exec(opts->cmd);\n \t\tbreak;\n \t}\n \tdefault:\n-- \n2.24.0.windows.2\n\n"},{"id":"386628","messageId":"20191120095238.4349-5-rybak.a.v@gmail.com","threadId":"52266","inReplyTo":"20191114163549.7648-1-rybak.a.v@gmail.com","subject":"[RFC PATCH v2 4/4] rebase -i: allow --edit-todo with --exec","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-11-20T09:52:38Z","receivedAt":"2019-11-20T09:52:51Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"When using rebase, option --exec can be used, for example, to run tests\nafter every commit created by rebase.  When using interactive rebase, I\ndon't always know, if I would want to run something against each commit\nbefore I start rebasing.  Sometimes, I realize this only after doing\nsome editing in the middle of the rebase.  To do that, I have to\nmanually edit the todo file.  Additing exec command by hand or\nsemi-automatically is cumbersome and error prone.  Especially if the\nfile is big or complex, e.g. when option --rebase-merges is used.\n\nAllow using the --edit-todo action of git rebase with option --exec.\nNew test is based on test 'rebase --edit-todo can be used to modify\ntodo'.  Contents of todo file are checked using set_cat_todo_editor\nsimilarly to what test 'respects rebase.abbreviateCommands with fixup,\nsquash and exec' does.\n\nRemove unnecessary braces around call to usage_with_options, while we're\nhere.\n\nTODO: Still need better validation of options. With current\n      implementation, the following is not rejected:\n\n\tgit rebase --edit-todo -x 'git show HEAD' --autostash\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n---\n Documentation/git-rebase.txt  |  3 ++-\n builtin/rebase.c              | 16 +++++++++++++---\n t/t3404-rebase-interactive.sh | 26 ++++++++++++++++++++++++++\n 3 files changed, 41 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 639a4179d1..b5db5e80d4 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -12,7 +12,8 @@ SYNOPSIS\n \t[--onto <newbase> | --keep-base] [<upstream> [<branch>]]\n 'git rebase' [-i | --interactive] [<options>] [--exec <cmd>] [--onto <newbase>]\n \t--root [<branch>]\n-'git rebase' (--continue | --skip | --abort | --quit | --edit-todo | --show-current-patch)\n+'git rebase' (--continue | --skip | --abort | --quit |\n+\t--edit-todo [--exec<cmd>] | --show-current-patch)\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 8a6ac7439b..4fd55cfbb4 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -33,7 +33,8 @@ static char const * const builtin_rebase_usage[] = {\n \t\t\"[--onto <newbase> | --keep-base] [<upstream> [<branch>]]\"),\n \tN_(\"git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] \"\n \t\t\"--root [<branch>]\"),\n-\tN_(\"git rebase --continue | --abort | --skip | --edit-todo\"),\n+\tN_(\"git rebase --continue | --abort | --skip | \"\n+\t\t\"--edit-todo [--exec <cmd>]\"),\n \tNULL\n };\n \n@@ -409,6 +410,11 @@ static int run_rebase_interactive(struct rebase_options *opts,\n \t\tbreak;\n \t}\n \tcase ACTION_EDIT_TODO:\n+\t\tif (opts->cmd) {\n+\t\t\tret = add_exec(opts->cmd);\n+\t\t\tif (ret)\n+\t\t\t\tbreak;\n+\t\t}\n \t\tret = edit_todo_file(flags);\n \t\tbreak;\n \tcase ACTION_SHOW_CURRENT_PATCH: {\n@@ -1565,15 +1571,19 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t     builtin_rebase_options,\n \t\t\t     builtin_rebase_usage, 0);\n \n-\tif (action != ACTION_NONE && total_argc != 2) {\n+\tif (action != ACTION_NONE && action != ACTION_EDIT_TODO &&\n+\t    total_argc != 2)\n \t\tusage_with_options(builtin_rebase_usage,\n \t\t\t\t   builtin_rebase_options);\n-\t}\n \n \tif (argc > 2)\n \t\tusage_with_options(builtin_rebase_usage,\n \t\t\t\t   builtin_rebase_options);\n \n+\tif (action == ACTION_EDIT_TODO && argc > 0)\n+\t\tusage_with_options(builtin_rebase_usage,\n+\t\t\t\t   builtin_rebase_options);\n+\n \tif (options.type == REBASE_PRESERVE_MERGES)\n \t\twarning(_(\"git rebase --preserve-merges is deprecated. \"\n \t\t\t  \"Use --rebase-merges instead.\"));\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d2dfbe46b9..c3b640575d 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1082,6 +1082,32 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' '\n \ttest L = $(git cat-file commit HEAD | sed -ne \\$p)\n '\n \n+test_expect_success 'rebase --edit-todo can be used with -x' '\n+\ttest_when_finished \"reset_rebase\" &&\n+\tgit reset --hard &&\n+\tgit checkout no-conflict-branch^0 &&\n+\tcat >expected <<-EOF &&\n+\tpick $(git rev-list --abbrev-commit -1 HEAD^) L\n+\texec git show HEAD\n+\tpick $(git rev-list --abbrev-commit -1 HEAD) M\n+\texec git show HEAD\n+\tEOF\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"1 edit 2 3 4\" git rebase -i HEAD~4 &&\n+\tset_cat_todo_editor &&\n+\ttest_must_fail git rebase --edit-todo -x \"git show HEAD\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'rebase --edit-todo -x does not allow other arguments' '\n+\ttest_when_finished \"reset_rebase\" &&\n+\tgit reset --hard &&\n+\tgit checkout no-conflict-branch^0 &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"1 edit 2 3 4\" git rebase -i HEAD~4 &&\n+\ttest_must_fail git rebase --edit-todo -x \"git show HEAD\" --autostash\n+'\n+\n test_expect_success 'rebase -i produces readable reflog' '\n \tgit reset --hard &&\n \tgit branch -f branch-reflog-test H &&\n-- \n2.24.0.windows.2\n\n"},{"id":"386694","messageId":"xmqqmucq9ftj.fsf@gitster-ct.c.googlers.com","threadId":"52266","inReplyTo":"20191120095238.4349-2-rybak.a.v@gmail.com","subject":"Re: [RFC PATCH v2 1/4] builtin/rebase.c: reuse loop variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-21T01:30:16Z","receivedAt":"2019-11-21T01:30:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak <rybak.a.v@gmail.com> writes:\n\n> Variable \"int i\" is already defined at the top of the function\n> cmd_rebase, so reuse it instead of declaring other variables, which mask\n> the outer \"i\".\n\nThe log message must also mention the other reason why this\nsimplification is correct, namely, that outer \"i\" is dead at the\npoint in the code that is touched by this patch and the value is\nnever used in the later parts of the code (I just followed the\ncodepath and made sure that is the case---iow, I think this patch\nis correct).\n\nThanks.\n\n>\n> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n> ---\n>  builtin/rebase.c | 4 ----\n>  1 file changed, 4 deletions(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 4a20582e72..793cac1386 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1747,8 +1747,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  \tif (exec.nr) {\n> -\t\tint i;\n> -\n>  \t\timply_interactive(&options, \"--exec\");\n>  \n>  \t\tstrbuf_reset(&buf);\n> @@ -1769,8 +1767,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  \tif (strategy_options.nr) {\n> -\t\tint i;\n> -\n>  \t\tif (!options.strategy)\n>  \t\t\toptions.strategy = \"recursive\";\n"},{"id":"386695","messageId":"xmqqimne9efm.fsf@gitster-ct.c.googlers.com","threadId":"52266","inReplyTo":"20191120095238.4349-3-rybak.a.v@gmail.com","subject":"Re: [RFC PATCH v2 2/4] rebase: prepare cmd before choosing action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-21T02:00:13Z","receivedAt":"2019-11-21T02:00:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak <rybak.a.v@gmail.com> writes:\n\n> When git rebase is started with option --exec, its arguments are parsed\n> into string_list exec and then converted into options.cmd.\n>\n> In following commits, action --edit-todo will be taught to use arguments\n> passed with --exec option.  Prepare options.cmd before switch (action)\n> to make it available for the ACTION_EDIT_TODO branch of the switch.\n\nHmph.  With or without this change, when we hit the run_rebase label\nin this function and call into run_rebase_interactive(), opts->cmd\ndoes contain what came from the --exec option.  In that function, I\nsee ACTION_EDIT_TODO calls edit_todo_file() that edits the on-disk\nfile without paying attention to opts->cmd (the only thing in the\nfunction that pays attention to this field is ACTION_ADD_EXEC).\n\nSo I am not sure what makes this step necessary.  I guess it is not\nwrong per-se, but if the objetive of this series is to add what\ncame from the --exec option when the user interacts with the editor\nin rebase-interactive.c::edit_todo_list(), wouldn't it be sufficient\nto skip this step, pass opts to edit_todo_file() and let the helper\nuse opts->cmd while preparing the todo_list it passes to underlying\nedit_todo_list() function?\n\nI am not claiming that it would be a better way---I wouldn't be\nsurprised if it is an incorrect approach---but it is unclear why\nthis step is needed and why the tweak of the todo list must be done\nin the \"switch (action)\" we see in the post context of the first\nhunk in this patch.\n\nThanks for working on this.\n\n\n> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n> ---\n>  builtin/rebase.c | 26 +++++++++++++-------------\n>  1 file changed, 13 insertions(+), 13 deletions(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 793cac1386..fa27f7b439 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1595,6 +1595,19 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>  \t\t\ttrace2_cmd_mode(action_names[action]);\n>  \t}\n>  \n> +\tfor (i = 0; i < exec.nr; i++)\n> +\t\tif (check_exec_cmd(exec.items[i].string))\n> +\t\t\texit(1);\n> +\n> +\tif (exec.nr) {\n> +\t\timply_interactive(&options, \"--exec\");\n> +\n> +\t\tstrbuf_reset(&buf);\n> +\t\tfor (i = 0; i < exec.nr; i++)\n> +\t\t\tstrbuf_addf(&buf, \"exec %s\\n\", exec.items[i].string);\n> +\t\toptions.cmd = xstrdup(buf.buf);\n> +\t}\n> +\n>  \tswitch (action) {\n>  \tcase ACTION_CONTINUE: {\n>  \t\tstruct object_id head;\n> @@ -1731,10 +1744,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>  \t\t}\n>  \t}\n>  \n> -\tfor (i = 0; i < exec.nr; i++)\n> -\t\tif (check_exec_cmd(exec.items[i].string))\n> -\t\t\texit(1);\n> -\n>  \tif (!(options.flags & REBASE_NO_QUIET))\n>  \t\targv_array_push(&options.git_am_opts, \"-q\");\n>  \n> @@ -1746,15 +1755,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>  \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n>  \t}\n>  \n> -\tif (exec.nr) {\n> -\t\timply_interactive(&options, \"--exec\");\n> -\n> -\t\tstrbuf_reset(&buf);\n> -\t\tfor (i = 0; i < exec.nr; i++)\n> -\t\t\tstrbuf_addf(&buf, \"exec %s\\n\", exec.items[i].string);\n> -\t\toptions.cmd = xstrdup(buf.buf);\n> -\t}\n> -\n>  \tif (rebase_merges) {\n>  \t\tif (!*rebase_merges)\n>  \t\t\t; /* default mode; do nothing */\n"},{"id":"386859","messageId":"b6d8fe84-cba1-543e-b387-4842b61829f6@gmail.com","threadId":"52266","inReplyTo":"xmqqimne9efm.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v2 2/4] rebase: prepare cmd before choosing action","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-11-22T19:08:34Z","receivedAt":"2019-11-22T19:08:40Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 2019-11-21 03:00, Junio C Hamano wrote:\n> Andrei Rybak <rybak.a.v@gmail.com> writes:\n>> When git rebase is started with option --exec, its arguments are parsed\n>> into string_list exec and then converted into options.cmd.\n>>\n>> In following commits, action --edit-todo will be taught to use arguments\n>> passed with --exec option.  Prepare options.cmd before switch (action)\n>> to make it available for the ACTION_EDIT_TODO branch of the switch.\n> Hmph.  With or without this change, when we hit the run_rebase label\n> in this function and call into run_rebase_interactive(), opts->cmd\n> does contain what came from the --exec option.  In that function, I\n> see ACTION_EDIT_TODO calls edit_todo_file() that edits the on-disk\n> file without paying attention to opts->cmd (the only thing in the\n> function that pays attention to this field is ACTION_ADD_EXEC).\n>\n> So I am not sure what makes this step necessary.  I guess it is not\n> wrong per-se, but if the objetive of this series is to add what\n> came from the --exec option when the user interacts with the editor\n> in rebase-interactive.c::edit_todo_list(), wouldn't it be sufficient\n> to skip this step, pass opts to edit_todo_file() and let the helper\n> use opts->cmd while preparing the todo_list it passes to underlying\n> edit_todo_list() function?\n>\n> I am not claiming that it would be a better way---I wouldn't be\n> surprised if it is an incorrect approach---but it is unclear why\n> this step is needed and why the tweak of the todo list must be done\n> in the \"switch (action)\" we see in the post context of the first\n> hunk in this patch.\n\nI would guess that it had something to do with passing this value to the helper\nbinary rebase--interactive before libification.  I couldn't figure out this\nmechanism before commit 460bc3ce73 (\"rebase -i: run without forking\nrebase--interactive\", 2019-04-17), so that's just a guess.\n\nI will look into possible simplification of this to avoid the chain of\nconversions string_list -> char * -> string_list.\n\n> Thanks for working on this.\n\nThank you for review :-)\n"}]}