{"thread":{"id":"59079","subject":"[PATCH] rebase: cleanup \"--exec\" option handling","startedAt":"2023-01-12T17:21:57Z","lastAt":"2023-01-13T20:07:06Z","messageCount":2,"participants":["Phillip Wood via GitGitGadget","Andrei Rybak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470218","messageId":"pull.1461.git.1673542201452.gitgitgadget@gmail.com","threadId":"59079","inReplyTo":null,"subject":"[PATCH] rebase: cleanup \"--exec\" option handling","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-12T16:50:01Z","receivedAt":"2023-01-12T17:21:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen handling \"--exec\" rebase collects the commands into a struct\nstring_list, then prepends \"exec \" to each command creating a multi line\nstring and finally splits that string back into a list of commands. This\nis an artifact of the scripted rebase and the need to support \"rebase\n--preserve-merges\". Now that \"--preserve-merges\" no-longer exists we can\ncleanup the way the argument is handled. There is no need to add the\n\"exec \" prefix to the commands as that is added by todo_list_to_strbuf().\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    rebase: cleanup \"--exec\" option handling\n    \n    A small cleanup following the removal of \"--preserve-merges\"\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1461%2Fphillipwood%2Frebase-cleanup-exec-handling-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1461/phillipwood/rebase-cleanup-exec-handling-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1461\n\n builtin/rebase.c | 45 +++++++++++----------------------------------\n sequencer.c      |  4 ++--\n 2 files changed, 13 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 1481c5b6a5b..a26cc0cfdb5 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -113,7 +113,7 @@ struct rebase_options {\n \tint autostash;\n \tint committer_date_is_author_date;\n \tint ignore_date;\n-\tchar *cmd;\n+\tstruct string_list exec;\n \tint allow_empty_message;\n \tint rebase_merges, rebase_cousins;\n \tchar *strategy, *strategy_opts;\n@@ -131,6 +131,7 @@ struct rebase_options {\n \t\t.default_backend = \"merge\",\t  \t\\\n \t\t.flags = REBASE_NO_QUIET, \t\t\\\n \t\t.git_am_opts = STRVEC_INIT,\t\t\\\n+\t\t.exec = STRING_LIST_INIT_NODUP,\t\t\\\n \t\t.git_format_patch_opt = STRBUF_INIT,\t\\\n \t\t.fork_point = -1,\t\t\t\\\n \t}\n@@ -243,17 +244,6 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,\n \treturn write_basic_state(opts, head_name, onto, orig_head);\n }\n \n-static void split_exec_commands(const char *cmd, struct string_list *commands)\n-{\n-\tif (cmd && *cmd) {\n-\t\tstring_list_split(commands, cmd, '\\n', -1);\n-\n-\t\t/* rebase.c adds a new line to cmd after every command,\n-\t\t * so here the last command is always empty */\n-\t\tstring_list_remove_empty_items(commands, 0);\n-\t}\n-}\n-\n static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n {\n \tint ret;\n@@ -261,7 +251,6 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \tstruct strvec make_script_args = STRVEC_INIT;\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n \tstruct replay_opts replay = get_replay_opts(opts);\n-\tstruct string_list commands = STRING_LIST_INIT_DUP;\n \n \tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,\n \t\t\t\t&revisions, &shortrevisions))\n@@ -297,14 +286,12 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \t\t\t\t\t\t&todo_list))\n \t\t\tBUG(\"unusable todo list\");\n \n-\t\tsplit_exec_commands(opts->cmd, &commands);\n \t\tret = complete_action(the_repository, &replay, flags,\n \t\t\tshortrevisions, opts->onto_name, opts->onto,\n-\t\t\t&opts->orig_head->object.oid, &commands,\n+\t\t\t&opts->orig_head->object.oid, &opts->exec,\n \t\t\topts->autosquash, opts->update_refs, &todo_list);\n \t}\n \n-\tstring_list_clear(&commands, 0);\n \tfree(revisions);\n \tfree(shortrevisions);\n \ttodo_list_release(&todo_list);\n@@ -1032,7 +1019,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tstruct object_id branch_base;\n \tint ignore_whitespace = 0;\n \tconst char *gpg_sign = NULL;\n-\tstruct string_list exec = STRING_LIST_INIT_NODUP;\n \tconst char *rebase_merges = NULL;\n \tstruct string_list strategy_options = STRING_LIST_INIT_NODUP;\n \tstruct object_id squash_onto;\n@@ -1127,7 +1113,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"GPG-sign commits\"),\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n \t\tOPT_AUTOSTASH(&options.autostash),\n-\t\tOPT_STRING_LIST('x', \"exec\", &exec, N_(\"exec\"),\n+\t\tOPT_STRING_LIST('x', \"exec\", &options.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\tOPT_BOOL_F(0, \"allow-empty-message\",\n@@ -1250,7 +1236,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (trace2_is_enabled()) {\n \t\tif (is_merge(&options))\n \t\t\ttrace2_cmd_mode(\"interactive\");\n-\t\telse if (exec.nr)\n+\t\telse if (options.exec.nr)\n \t\t\ttrace2_cmd_mode(\"interactive-exec\");\n \t\telse\n \t\t\ttrace2_cmd_mode(action_names[options.action]);\n@@ -1378,7 +1364,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \tif ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||\n \t    (options.action != ACTION_NONE) ||\n-\t    (exec.nr > 0) ||\n+\t    (options.exec.nr > 0) ||\n \t    options.autosquash) {\n \t\tallow_preemptive_ff = 0;\n \t}\n@@ -1402,8 +1388,8 @@ 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+\tfor (i = 0; i < options.exec.nr; i++)\n+\t\tif (check_exec_cmd(options.exec.items[i].string))\n \t\t\texit(1);\n \n \tif (!(options.flags & REBASE_NO_QUIET))\n@@ -1422,17 +1408,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (gpg_sign)\n \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n \n-\tif (exec.nr) {\n-\t\tint i;\n-\n+\tif (options.exec.nr)\n \t\timply_merge(&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@@ -1543,7 +1521,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (options.empty == EMPTY_UNSPECIFIED) {\n \t\tif (options.flags & REBASE_INTERACTIVE_EXPLICIT)\n \t\t\toptions.empty = EMPTY_ASK;\n-\t\telse if (exec.nr > 0)\n+\t\telse if (options.exec.nr > 0)\n \t\t\toptions.empty = EMPTY_KEEP;\n \t\telse\n \t\t\toptions.empty = EMPTY_DROP;\n@@ -1831,11 +1809,10 @@ cleanup:\n \tfree(options.head_name);\n \tstrvec_clear(&options.git_am_opts);\n \tfree(options.gpg_sign_opt);\n-\tfree(options.cmd);\n+\tstring_list_clear(&options.exec, 0);\n \tfree(options.strategy);\n \tstrbuf_release(&options.git_format_patch_opt);\n \tfree(squash_onto_name);\n-\tstring_list_clear(&exec, 0);\n \tstring_list_clear(&strategy_options, 0);\n \treturn !!ret;\n }\ndiff --git a/sequencer.c b/sequencer.c\nindex bcb662e23be..3e4a1972897 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5745,8 +5745,8 @@ static void todo_list_add_exec_commands(struct todo_list *todo_list,\n \n \t\tbase_items[i].command = TODO_EXEC;\n \t\tbase_items[i].offset_in_buf = base_offset;\n-\t\tbase_items[i].arg_offset = base_offset + strlen(\"exec \");\n-\t\tbase_items[i].arg_len = command_len - strlen(\"exec \");\n+\t\tbase_items[i].arg_offset = base_offset;\n+\t\tbase_items[i].arg_len = command_len;\n \n \t\tbase_offset += command_len + 1;\n \t}\n\nbase-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1\n-- \ngitgitgadget\n"},{"id":"470304","messageId":"c17dfab8-5cc6-dbff-547d-08a3d7c289a7@gmail.com","threadId":"59079","inReplyTo":"pull.1461.git.1673542201452.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase: cleanup \"--exec\" option handling","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-01-13T20:06:59Z","receivedAt":"2023-01-13T20:07:06Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 12/01/2023 17:50, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> When handling \"--exec\" rebase collects the commands into a struct\n> string_list, then prepends \"exec \" to each command creating a multi line\n> string and finally splits that string back into a list of commands. This\n> is an artifact of the scripted rebase and the need to support \"rebase\n> --preserve-merges\". Now that \"--preserve-merges\" no-longer exists we can\n> cleanup the way the argument is handled. There is no need to add the\n> \"exec \" prefix to the commands as that is added by todo_list_to_strbuf().\n> \n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>      rebase: cleanup \"--exec\" option handling\n>      \n>      A small cleanup following the removal of \"--preserve-merges\"\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1461%2Fphillipwood%2Frebase-cleanup-exec-handling-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1461/phillipwood/rebase-cleanup-exec-handling-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1461\n> \n>   builtin/rebase.c | 45 +++++++++++----------------------------------\n>   sequencer.c      |  4 ++--\n>   2 files changed, 13 insertions(+), 36 deletions(-)\n> \n\nI can't really comment on code, but I appreciate any simplifications\naround `rebase --exec`, because it will probably make it easier for\nme to revive the topic about `rebase --edit-todo --exec` from the\nend of 2019:\n\n     https://lore.kernel.org/git/20191114163549.7648-1-rybak.a.v@gmail.com/T/#u\n\nIt's current state (experimented a bit a week ago) is on GitHub:\n\n     https://github.com/rybak/git/tree/edit-todo-exec\n\nThank you.\n"}]}