{"thread":{"id":"58255","subject":"[PATCH] rebase: introduce allow-inline-reword option","startedAt":"2022-08-02T06:39:57Z","lastAt":"2022-08-04T08:10:58Z","messageCount":7,"participants":["Théo MAILLART via GitGitGadget","Junio C Hamano","Théo Maillart","Eric Sunshine","Phillip Wood","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"460405","messageId":"pull.1228.git.git.1659422389830.gitgitgadget@gmail.com","threadId":"58255","inReplyTo":null,"subject":"[PATCH] rebase: introduce allow-inline-reword option","fromName":"Théo MAILLART via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-02T06:39:49Z","receivedAt":"2022-08-02T06:39:57Z","isPatch":true,"sender":{"key":"name:Théo MAILLART","avatar":null},"body":"From: =?UTF-8?q?Th=C3=A9o=20Maillart?= <tmaillart@gmail.com>\n\nThis new option (false by default) for interactive rebase allows users\nto modify the subject of a commit directly in the todo list, when they\nselect the \"reword\" action.\nIf the option is enabled, \"reword\" is selected and the subject has not\nchanged, then the default behaviour is used.\nIt also introduces a test for this specific option, and a related\nfunction (set_inline_reword_editor) in the lib-rebase.sh to use a\nsimpler custom fake editor to be able to modify the message part of the\nlines in a todo list (in the most simple cases).\n\nSigned-off-by: Théo Maillart <tmaillart@gmail.com>\n---\n    [RFC] rebase: reword: new feature change the subject in the todo list\n    \n    If the user only wants to modify the subject of a commit during\n    interactive rebase, now he can choose the \"reword\" command and change\n    the subject directly in the todo list. If \"reword\" is selected and the\n    subject has not changed, then we use the default behavior. Here is a\n    demo https://asciinema.org/a/T9tEmUjjl4dyDuaXalPzXNzo1\n    \n    This is probably not the correct way to implement this new feature, or\n    perhaps it is not desirable, so I would like some feedback. Thank you\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1228%2Ftmaillart%2Ftm%2Freword-inline-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1228/tmaillart/tm/reword-inline-v1\nPull-Request: https://github.com/git/git/pull/1228\n\n Documentation/git-rebase.txt  |  7 +++++\n builtin/rebase.c              | 11 ++++++++\n sequencer.c                   | 49 ++++++++++++++++++++++++++++++++---\n sequencer.h                   |  6 +++++\n t/lib-rebase.sh               | 29 +++++++++++++++++++++\n t/t3404-rebase-interactive.sh | 11 ++++++++\n 6 files changed, 109 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 080658c8710..dcfd65d0bc1 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -332,6 +332,13 @@ See also INCOMPATIBLE OPTIONS below.\n +\n See also INCOMPATIBLE OPTIONS below.\n \n+--allow-inline-reword::\n+\tWhen performing an interactive rebase, this flag allows you to change\n+\tthe subject of a specific commit directly in the todo list (only when\n+\treword command is selected) without asking you to edit the commit\n+\tmessages sequencially. If you select reword without changing the\n+\tcommit's subject then you will get the default reword behaviour.\n+\n --skip::\n \tRestart the rebasing process by skipping the current patch.\n \ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 56e4214b441..cfb86f15a69 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -103,6 +103,7 @@ struct rebase_options {\n \tint reapply_cherry_picks;\n \tint fork_point;\n \tint update_refs;\n+\tint allow_inline_reword;\n };\n \n #define REBASE_OPTIONS_INIT {\t\t\t  \t\\\n@@ -130,6 +131,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \t\treplay.allow_rerere_auto = opts->allow_rerere_autoupdate;\n \treplay.allow_empty = 1;\n \treplay.allow_empty_message = opts->allow_empty_message;\n+\treplay.allow_inline_reword = opts->allow_inline_reword;\n \treplay.drop_redundant_commits = (opts->empty == EMPTY_DROP);\n \treplay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);\n \treplay.quiet = !(opts->flags & REBASE_NO_QUIET);\n@@ -821,6 +823,11 @@ static int rebase_config(const char *var, const char *value, void *data)\n \t\treturn git_config_string(&opts->default_backend, var, value);\n \t}\n \n+\tif (!strcmp(var, \"rebase.allowinlinereword\")) {\n+\t\topts->allow_inline_reword = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(var, value, data);\n }\n \n@@ -1164,6 +1171,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"automatically re-schedule any `exec` that fails\")),\n \t\tOPT_BOOL(0, \"reapply-cherry-picks\", &options.reapply_cherry_picks,\n \t\t\t N_(\"apply all changes, even those already present upstream\")),\n+\t\tOPT_BOOL_F(0, \"allow-inline-reword\",\n+\t\t\t   &options.allow_inline_reword,\n+\t\t\t   N_(\"allow changing commit's subject directly in the todo list\"),\n+\t\t\t   PARSE_OPT_HIDDEN),\n \t\tOPT_END(),\n \t};\n \tint i;\ndiff --git a/sequencer.c b/sequencer.c\nindex 5f22b7cd377..8623296f428 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -999,6 +999,7 @@ N_(\"you have staged changes in your working tree\\n\"\n #define VERIFY_MSG  (1<<4)\n #define CREATE_ROOT_COMMIT (1<<5)\n #define VERBATIM_MSG (1<<6)\n+#define INLINE_MSG   (1<<7)\n \n static int run_command_silent_on_success(struct child_process *cmd)\n {\n@@ -1076,6 +1077,24 @@ static int run_git_commit(const char *defmsg,\n \t\tstrvec_push(&cmd.args, \"--cleanup=strip\");\n \tif ((flags & VERBATIM_MSG))\n \t\tstrvec_push(&cmd.args, \"--cleanup=verbatim\");\n+\tif ((flags & EDIT_MSG) && (flags & INLINE_MSG)) {\n+\t\tint ret;\n+\n+\t\tstrvec_push(&cmd.args, \"-F\");\n+\t\tstrvec_push(&cmd.args, \"-\");\n+\t\tcmd.in = -1;\n+\t\tret = start_command(&cmd);\n+\t\tif (ret)\n+\t\t\treturn ret;\n+\t\tif (write(cmd.in, opts->subject, opts->subject_len) !=\n+\t\t    opts->subject_len)\n+\t\t\treturn error(_(\"subject write error to child process\"));\n+\t\tif (write(cmd.in, opts->content, opts->content_len) !=\n+\t\t    opts->content_len)\n+\t\t\treturn error(_(\"content write error to child process\"));\n+\t\tclose(cmd.in);\n+\t\treturn finish_command(&cmd);\n+\t}\n \tif ((flags & EDIT_MSG))\n \t\tstrvec_push(&cmd.args, \"-e\");\n \telse if (!(flags & CLEANUP_MSG) &&\n@@ -2118,6 +2137,7 @@ static void refer_to_commit(struct replay_opts *opts,\n }\n \n static int do_pick_commit(struct repository *r,\n+\t\t\t  struct todo_list *todo_list,\n \t\t\t  struct todo_item *item,\n \t\t\t  struct replay_opts *opts,\n \t\t\t  int final_fixup, int *check_todo)\n@@ -2375,9 +2395,29 @@ static int do_pick_commit(struct repository *r,\n \t\t*check_todo = !!(flags & EDIT_MSG);\n \t\tif (!res && reword) {\n fast_forward_edit:\n+\t\t\tif (opts->allow_inline_reword && item->arg_len > 0 &&\n+\t\t\t    (strlen(msg.subject) != item->arg_len ||\n+\t\t\t     strncmp(msg.subject, todo_item_get_arg(todo_list,\n+\t\t\t\t     item),item->arg_len) != 0)) {\n+\t\t\t\tconst char *commit_buf, *subject;\n+\t\t\t\tint subject_len;\n+\t\t\t\tunsigned long commit_buf_len;\n+\n+\t\t\t\tflags |= INLINE_MSG;\n+\t\t\t\topts->subject = todo_item_get_arg(todo_list,\n+\t\t\t\t\titem);\n+\t\t\t\topts->subject_len = item->arg_len;\n+\t\t\t\tcommit_buf = get_commit_buffer(commit,\n+\t\t\t\t\t&commit_buf_len);\n+\t\t\t\tsubject_len = find_commit_subject(commit_buf,\n+\t\t\t\t\t&subject);\n+\t\t\t\topts->content = subject + subject_len;\n+\t\t\t\topts->content_len = commit_buf +\n+\t\t\t\t\tcommit_buf_len - opts->content;\n+\t\t\t}\n \t\t\tres = run_git_commit(NULL, opts, EDIT_MSG |\n \t\t\t\t\t     VERIFY_MSG | AMEND_MSG |\n-\t\t\t\t\t     (flags & ALLOW_EMPTY));\n+\t\t\t\t\t     (flags & (ALLOW_EMPTY | INLINE_MSG)));\n \t\t\t*check_todo = 1;\n \t\t}\n \t}\n@@ -4621,7 +4661,7 @@ static int pick_commits(struct repository *r,\n \t\t\t\tsetenv(GIT_REFLOG_ACTION, reflog_message(opts,\n \t\t\t\t\tcommand_to_string(item->command), NULL),\n \t\t\t\t\t1);\n-\t\t\tres = do_pick_commit(r, item, opts,\n+\t\t\tres = do_pick_commit(r, todo_list, item, opts,\n \t\t\t\t\t     is_final_fixup(todo_list),\n \t\t\t\t\t     &check_todo);\n \t\t\tif (is_rebase_i(opts))\n@@ -5101,6 +5141,7 @@ release_todo_list:\n }\n \n static int single_pick(struct repository *r,\n+\t\t       struct todo_list *todo_list,\n \t\t       struct commit *cmit,\n \t\t       struct replay_opts *opts)\n {\n@@ -5112,7 +5153,7 @@ static int single_pick(struct repository *r,\n \titem.commit = cmit;\n \n \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n-\treturn do_pick_commit(r, &item, opts, 0, &check_todo);\n+\treturn do_pick_commit(r, todo_list, &item, opts, 0, &check_todo);\n }\n \n int sequencer_pick_revisions(struct repository *r,\n@@ -5165,7 +5206,7 @@ int sequencer_pick_revisions(struct repository *r,\n \t\t\treturn error(_(\"empty commit set passed\"));\n \t\tif (get_revision(opts->revs))\n \t\t\tBUG(\"unexpected extra commit from walk\");\n-\t\treturn single_pick(r, cmit, opts);\n+\t\treturn single_pick(r, &todo_list, cmit, opts);\n \t}\n \n \t/*\ndiff --git a/sequencer.h b/sequencer.h\nindex 563fe599334..26e3e43ce99 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -42,6 +42,7 @@ struct replay_opts {\n \tint allow_rerere_auto;\n \tint allow_empty;\n \tint allow_empty_message;\n+\tint allow_inline_reword;\n \tint drop_redundant_commits;\n \tint keep_redundant_commits;\n \tint verbose;\n@@ -57,6 +58,11 @@ struct replay_opts {\n \tenum commit_msg_cleanup_mode default_msg_cleanup;\n \tint explicit_cleanup;\n \n+\tconst char *content;\n+\tint content_len;\n+\tconst char *subject;\n+\tint subject_len;\n+\n \t/* Merge strategy */\n \tchar *default_strategy;  /* from config options */\n \tchar *strategy;\ndiff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\nindex b57541356bd..8c9dacdad9a 100644\n--- a/t/lib-rebase.sh\n+++ b/t/lib-rebase.sh\n@@ -222,3 +222,32 @@ set_replace_editor () {\n \tsed -e \"s/FILENAME/$1/g\" <script | write_script fake-editor.sh &&\n \ttest_set_editor \"$(pwd)/fake-editor.sh\"\n }\n+\n+set_inline_reword_editor () {\n+\twrite_script fake-editor.sh <<-\\EOF\n+\tgrep -v '^#' < \"$1\" > \"$1\".tmp\n+\trm -f \"$1\"\n+\techo 'rebase -i script before editing:'\n+\tcat \"$1\".tmp\n+\taction=\\\\2\n+\thash=\\\\3\n+\tsubject=\\\\4\n+\tfor line in $FAKE_LINES; do\n+\t\tcase $line in\n+\t\tpick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)\n+\t\t\tsubject=\\\\4\n+\t\t\taction=\"$line\";;\n+\t\treword_*)\n+\t\t\taction=reword\n+\t\t\tsubject=$(echo \"$line\" | sed 's,reword_\\(.*\\),\\1,');;\n+\t\t*)\n+\t\t\tsed -n \"${line}s,^\\(\\([a-z][a-z]*\\)[[:space:]]*\\([a-f0-9][a-f0-9]*\\)[[:space:]]*\\(.*\\)\\),${action} ${hash} ${subject},p\" < \"$1\".tmp >> \"$1\"\n+\t\t\taction=\\\\2;;\n+\t\tesac\n+\tdone\n+\techo 'rebase -i script after editing:'\n+\tcat \"$1\"\n+\tEOF\n+\n+\ttest_set_editor \"$(pwd)/fake-editor.sh\"\n+}\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 688b01e3eb6..4c27d5f1c91 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -951,6 +951,17 @@ test_expect_success 'rebase -ix with --autosquash' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'rebase -i with --allow-inline-reword' '\n+\tgit checkout -b reword-inline primary &&\n+\t(\n+\t\tset_inline_reword_editor &&\n+\t\tFAKE_LINES=\"reword_Bchanged 1 2 reword_Dchanged 3 4\" \\\n+\t\t\tgit rebase --allow-inline-reword -i HEAD~4\n+\t) &&\n+\ttest Bchanged = $(git show -s --format=%s HEAD~3) &&\n+\ttest Dchanged = $(git show -s --format=%s HEAD~)\n+'\n+\n test_expect_success 'rebase --exec works without -i ' '\n \tgit reset --hard execute &&\n \trm -rf exec_output &&\n\nbase-commit: 350dc9f0e8974b6fcbdeb3808186c5a79c3e7386\n-- \ngitgitgadget\n"},{"id":"460434","messageId":"xmqqles6is9s.fsf@gitster.g","threadId":"58255","inReplyTo":"pull.1228.git.git.1659422389830.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase: introduce allow-inline-reword option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-02T15:23:11Z","receivedAt":"2022-08-02T15:23:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Théo MAILLART via GitGitGadget\"  <gitgitgadget@gmail.com> writes:\n\n> This new option (false by default) for interactive rebase allows users\n> to modify the subject of a commit directly in the todo list, when they\n> select the \"reword\" action.\n> If the option is enabled, \"reword\" is selected and the subject has not\n> changed, then the default behaviour is used.\n> It also introduces a test for this specific option, and a related\n> function (set_inline_reword_editor) in the lib-rebase.sh to use a\n> simpler custom fake editor to be able to modify the message part of the\n> lines in a todo list (in the most simple cases).\n>\n> Signed-off-by: Théo Maillart <tmaillart@gmail.com>\n> ---\n>     [RFC] rebase: reword: new feature change the subject in the todo list\n\nIt is not clear if you meant this as a final submission or an RFC\nbut I'll take it as an RFC for now.\n\nA handful of things come to my mind.\n\n * Would this want to be a new variant of \"reword\" that you would\n   write into the todo list file, instead of a command line option\n   that says \"every 'reword' I write in the todo list file means\n   something different now\"?\n\n * Is there a plausible UI that allows inline editing of a commit\n   log message that is more than one line long?  Should there be?\n\n * Under \"inline\" mode, when a \"reword\" is requested for a commit\n   that has more than one line of log message, what should happen?\n   Should the updated title become the ONLY content of the log of\n   the updated commit?  Should it be an error, because it is clearly\n   an information-losing operation?  Would it make sense to turn the\n   \"inline\" reword into normal reword automatically for a commit\n   with more than one line of log message?\n\n * If we choose to special case a commit with more than one line of\n   message in order to prevent the 'inline' mode from losing\n   valuable information in the original commits, what role should\n   trailer lines play when we decide if a commit has only one line\n   of message?  For example, if a lazy \"title only\" commit has no\n   body message but a sign-off and other trailers like helped-by,\n   would it make sense to keep the trailers intact and only replace\n   the title, still in inline mode?\n\nHere is an alternative design that may be conceptually cleaner.\n\n * We do not introduce a new option at all.  \"reword\" means \"open\n   the editor and you can edit the whole thing\" as always.\n\n * We introduce \"retitle\" that can be used instead of \"reword\".\n\n   The line for a commit originally shows \"pick\" followed by an\n   abbreviated commit object name followed by its title, and the\n   body of the message and the trailer is hidden.  If you change\n   \"pick\" to \"retitle\" and edit the shown title, then the original\n   log message from the commit is read as a whole, its title line is\n   replaced with what \"retitle\" line has, and the result is used as\n   the updated log message.\n\nThat way, those who write more than one line of commit log message\ncan still use the feature without having to worry about losing\ninformation when the only thing they want to fix is a typo in the\ntitle, and those who write only one line of commit log message do\nnot have to pass the new \"--inline\" option at all.  They can use\n'retitle' instead of 'reword'.\n\nHmm?\n"},{"id":"460515","messageId":"2a7040a4-20ce-495d-4182-089c6c08fbd6@gmail.com","threadId":"58255","inReplyTo":"xmqqles6is9s.fsf@gitster.g","subject":"Re: [PATCH] rebase: introduce allow-inline-reword option","fromName":"Théo Maillart","fromEmail":"tmaillart@gmail.com","sentAt":"2022-08-02T22:22:56Z","receivedAt":"2022-08-02T22:23:03Z","isPatch":true,"sender":{"key":"tmaillart@gmail.com","avatar":null},"body":"\nOn 8/2/22 17:23, Junio C Hamano wrote:\n> \"Théo MAILLART via GitGitGadget\"<gitgitgadget@gmail.com>  writes:\n>\n>> This new option (false by default) for interactive rebase allows users\n>> to modify the subject of a commit directly in the todo list, when they\n>> select the \"reword\" action.\n>> If the option is enabled, \"reword\" is selected and the subject has not\n>> changed, then the default behaviour is used.\n>> It also introduces a test for this specific option, and a related\n>> function (set_inline_reword_editor) in the lib-rebase.sh to use a\n>> simpler custom fake editor to be able to modify the message part of the\n>> lines in a todo list (in the most simple cases).\n>>\n>> Signed-off-by: Théo Maillart<tmaillart@gmail.com>\n>> ---\n>>      [RFC] rebase: reword: new feature change the subject in the todo list\n> It is not clear if you meant this as a final submission or an RFC\n> but I'll take it as an RFC for now.\n>\n> A handful of things come to my mind.\n>\n>   * Would this want to be a new variant of \"reword\" that you would\n>     write into the todo list file, instead of a command line option\n>     that says \"every 'reword' I write in the todo list file means\n>     something different now\"?\n>\n>   * Is there a plausible UI that allows inline editing of a commit\n>     log message that is more than one line long?  Should there be?\n>\n>   * Under \"inline\" mode, when a \"reword\" is requested for a commit\n>     that has more than one line of log message, what should happen?\n>     Should the updated title become the ONLY content of the log of\n>     the updated commit?  Should it be an error, because it is clearly\n>     an information-losing operation?  Would it make sense to turn the\n>     \"inline\" reword into normal reword automatically for a commit\n>     with more than one line of log message?\n>\n>   * If we choose to special case a commit with more than one line of\n>     message in order to prevent the 'inline' mode from losing\n>     valuable information in the original commits, what role should\n>     trailer lines play when we decide if a commit has only one line\n>     of message?  For example, if a lazy \"title only\" commit has no\n>     body message but a sign-off and other trailers like helped-by,\n>     would it make sense to keep the trailers intact and only replace\n>     the title, still in inline mode?\n\nI agree, taking care of more than the commit subject only does\nnot look like an easy task, and I'm probably not the right person\nto do this in a reasonable amount of time.\n\n>\n> Here is an alternative design that may be conceptually cleaner.\n>\n>   * We do not introduce a new option at all.  \"reword\" means \"open\n>     the editor and you can edit the whole thing\" as always.\n>\n>   * We introduce \"retitle\" that can be used instead of \"reword\".\n>\n>     The line for a commit originally shows \"pick\" followed by an\n>     abbreviated commit object name followed by its title, and the\n>     body of the message and the trailer is hidden.  If you change\n>     \"pick\" to \"retitle\" and edit the shown title, then the original\n>     log message from the commit is read as a whole, its title line is\n>     replaced with what \"retitle\" line has, and the result is used as\n>     the updated log message.\n>\n> That way, those who write more than one line of commit log message\n> can still use the feature without having to worry about losing\n> information when the only thing they want to fix is a typo in the\n> title, and those who write only one line of commit log message do\n> not have to pass the new \"--inline\" option at all.  They can use\n> 'retitle' instead of 'reword'.\n>\n> Hmm?\n>\nSo, if I understand your suggestion correctly, we can say that,\nmost of the logic is implemented in this patch, but I should move\nthe \"inline\" logic from the \"reword\" to a new action \"retitle\".\nIf it is ok with you, I will look into that, and get back with a new\npatch.\nThe only thing that seems unfortunate to me is that we will have\na hard time finding a meaningful short name for this new action in\nthe todo list, as \"r\" is for \"rename\".\n\n"},{"id":"460518","messageId":"xmqqy1w6e0ix.fsf@gitster.g","threadId":"58255","inReplyTo":"2a7040a4-20ce-495d-4182-089c6c08fbd6@gmail.com","subject":"Re: [PATCH] rebase: introduce allow-inline-reword option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-02T22:36:06Z","receivedAt":"2022-08-02T22:36:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Théo Maillart <tmaillart@gmail.com> writes:\n\n> So, if I understand your suggestion correctly, we can say that,\n> most of the logic is implemented in this patch, but I should move\n> the \"inline\" logic from the \"reword\" to a new action \"retitle\".\n\nYeah, I was hoping that most of the logic has already been written,\nexcept that it is likely you just ignored the original log message\nand overwrote it with the single line you got, but now you need to\nread the original and replace only the title (the \"paragraph\" before\nthe first blank line), which you may not have yet.\n"},{"id":"460519","messageId":"CAPig+cSzgaxMxfhShRHtztRH72rgA21-Vw1SrFwkiFyrP7De7Q@mail.gmail.com","threadId":"58255","inReplyTo":"xmqqy1w6e0ix.fsf@gitster.g","subject":"Re: [PATCH] rebase: introduce allow-inline-reword option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-08-02T22:59:30Z","receivedAt":"2022-08-02T22:59:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 2, 2022 at 6:43 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Théo Maillart <tmaillart@gmail.com> writes:\n> > So, if I understand your suggestion correctly, we can say that,\n> > most of the logic is implemented in this patch, but I should move\n> > the \"inline\" logic from the \"reword\" to a new action \"retitle\".\n>\n> Yeah, I was hoping that most of the logic has already been written,\n> except that it is likely you just ignored the original log message\n> and overwrote it with the single line you got, but now you need to\n> read the original and replace only the title (the \"paragraph\" before\n> the first blank line), which you may not have yet.\n\nMichael Haggerty had made a more generalized proposal[1] some time\nback which might or might not be relevant here. The notion of being\nable to edit the title of commit came up in the ensuing discussion, as\nwell.\n\n[1]: https://lore.kernel.org/git/530DA00E.4090402@alum.mit.edu/\n"},{"id":"460546","messageId":"d7de2055-e7e6-357d-60a6-1d2389fb5709@gmail.com","threadId":"58255","inReplyTo":"pull.1228.git.git.1659422389830.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase: introduce allow-inline-reword option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-03T14:16:59Z","receivedAt":"2022-08-03T14:17:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Théo\n\nOn 02/08/2022 07:39, Théo MAILLART via GitGitGadget wrote:\n> From: =?UTF-8?q?Th=C3=A9o=20Maillart?= <tmaillart@gmail.com>\n> \n> This new option (false by default) for interactive rebase allows users\n> to modify the subject of a commit directly in the todo list, when they\n> select the \"reword\" action.\n> If the option is enabled, \"reword\" is selected and the subject has not\n> changed, then the default behaviour is used.\n> It also introduces a test for this specific option, and a related\n> function (set_inline_reword_editor) in the lib-rebase.sh to use a\n> simpler custom fake editor to be able to modify the message part of the\n> lines in a todo list (in the most simple cases).\n\nI'm in two minds about editing the commit subject it the todo list. I \ncan see it would occasionally be useful to correct a typo in the commit \nsubject that you notice when editing the todo list. However I'm not sure \nhow common a case that is compared to having to edit the main body of \nthe message and I'm concerned it would encourage people towards single \nline commit message which is something we definitely want to avoid.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Théo Maillart <tmaillart@gmail.com>\n> ---\n>      [RFC] rebase: reword: new feature change the subject in the todo list\n>      \n>      If the user only wants to modify the subject of a commit during\n>      interactive rebase, now he can choose the \"reword\" command and change\n>      the subject directly in the todo list. If \"reword\" is selected and the\n>      subject has not changed, then we use the default behavior. Here is a\n>      demo https://asciinema.org/a/T9tEmUjjl4dyDuaXalPzXNzo1\n>      \n>      This is probably not the correct way to implement this new feature, or\n>      perhaps it is not desirable, so I would like some feedback. Thank you\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1228%2Ftmaillart%2Ftm%2Freword-inline-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1228/tmaillart/tm/reword-inline-v1\n> Pull-Request: https://github.com/git/git/pull/1228\n> \n>   Documentation/git-rebase.txt  |  7 +++++\n>   builtin/rebase.c              | 11 ++++++++\n>   sequencer.c                   | 49 ++++++++++++++++++++++++++++++++---\n>   sequencer.h                   |  6 +++++\n>   t/lib-rebase.sh               | 29 +++++++++++++++++++++\n>   t/t3404-rebase-interactive.sh | 11 ++++++++\n>   6 files changed, 109 insertions(+), 4 deletions(-)\n> \n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 080658c8710..dcfd65d0bc1 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -332,6 +332,13 @@ See also INCOMPATIBLE OPTIONS below.\n>   +\n>   See also INCOMPATIBLE OPTIONS below.\n>   \n> +--allow-inline-reword::\n> +\tWhen performing an interactive rebase, this flag allows you to change\n> +\tthe subject of a specific commit directly in the todo list (only when\n> +\treword command is selected) without asking you to edit the commit\n> +\tmessages sequencially. If you select reword without changing the\n> +\tcommit's subject then you will get the default reword behaviour.\n> +\n>   --skip::\n>   \tRestart the rebasing process by skipping the current patch.\n>   \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 56e4214b441..cfb86f15a69 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -103,6 +103,7 @@ struct rebase_options {\n>   \tint reapply_cherry_picks;\n>   \tint fork_point;\n>   \tint update_refs;\n> +\tint allow_inline_reword;\n>   };\n>   \n>   #define REBASE_OPTIONS_INIT {\t\t\t  \t\\\n> @@ -130,6 +131,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n>   \t\treplay.allow_rerere_auto = opts->allow_rerere_autoupdate;\n>   \treplay.allow_empty = 1;\n>   \treplay.allow_empty_message = opts->allow_empty_message;\n> +\treplay.allow_inline_reword = opts->allow_inline_reword;\n>   \treplay.drop_redundant_commits = (opts->empty == EMPTY_DROP);\n>   \treplay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);\n>   \treplay.quiet = !(opts->flags & REBASE_NO_QUIET);\n> @@ -821,6 +823,11 @@ static int rebase_config(const char *var, const char *value, void *data)\n>   \t\treturn git_config_string(&opts->default_backend, var, value);\n>   \t}\n>   \n> +\tif (!strcmp(var, \"rebase.allowinlinereword\")) {\n> +\t\topts->allow_inline_reword = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\n>   \treturn git_default_config(var, value, data);\n>   }\n>   \n> @@ -1164,6 +1171,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>   \t\t\t N_(\"automatically re-schedule any `exec` that fails\")),\n>   \t\tOPT_BOOL(0, \"reapply-cherry-picks\", &options.reapply_cherry_picks,\n>   \t\t\t N_(\"apply all changes, even those already present upstream\")),\n> +\t\tOPT_BOOL_F(0, \"allow-inline-reword\",\n> +\t\t\t   &options.allow_inline_reword,\n> +\t\t\t   N_(\"allow changing commit's subject directly in the todo list\"),\n> +\t\t\t   PARSE_OPT_HIDDEN),\n>   \t\tOPT_END(),\n>   \t};\n>   \tint i;\n> diff --git a/sequencer.c b/sequencer.c\n> index 5f22b7cd377..8623296f428 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -999,6 +999,7 @@ N_(\"you have staged changes in your working tree\\n\"\n>   #define VERIFY_MSG  (1<<4)\n>   #define CREATE_ROOT_COMMIT (1<<5)\n>   #define VERBATIM_MSG (1<<6)\n> +#define INLINE_MSG   (1<<7)\n>   \n>   static int run_command_silent_on_success(struct child_process *cmd)\n>   {\n> @@ -1076,6 +1077,24 @@ static int run_git_commit(const char *defmsg,\n>   \t\tstrvec_push(&cmd.args, \"--cleanup=strip\");\n>   \tif ((flags & VERBATIM_MSG))\n>   \t\tstrvec_push(&cmd.args, \"--cleanup=verbatim\");\n> +\tif ((flags & EDIT_MSG) && (flags & INLINE_MSG)) {\n> +\t\tint ret;\n> +\n> +\t\tstrvec_push(&cmd.args, \"-F\");\n> +\t\tstrvec_push(&cmd.args, \"-\");\n> +\t\tcmd.in = -1;\n> +\t\tret = start_command(&cmd);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t\tif (write(cmd.in, opts->subject, opts->subject_len) !=\n> +\t\t    opts->subject_len)\n> +\t\t\treturn error(_(\"subject write error to child process\"));\n> +\t\tif (write(cmd.in, opts->content, opts->content_len) !=\n> +\t\t    opts->content_len)\n> +\t\t\treturn error(_(\"content write error to child process\"));\n> +\t\tclose(cmd.in);\n> +\t\treturn finish_command(&cmd);\n> +\t}\n>   \tif ((flags & EDIT_MSG))\n>   \t\tstrvec_push(&cmd.args, \"-e\");\n>   \telse if (!(flags & CLEANUP_MSG) &&\n> @@ -2118,6 +2137,7 @@ static void refer_to_commit(struct replay_opts *opts,\n>   }\n>   \n>   static int do_pick_commit(struct repository *r,\n> +\t\t\t  struct todo_list *todo_list,\n>   \t\t\t  struct todo_item *item,\n>   \t\t\t  struct replay_opts *opts,\n>   \t\t\t  int final_fixup, int *check_todo)\n> @@ -2375,9 +2395,29 @@ static int do_pick_commit(struct repository *r,\n>   \t\t*check_todo = !!(flags & EDIT_MSG);\n>   \t\tif (!res && reword) {\n>   fast_forward_edit:\n> +\t\t\tif (opts->allow_inline_reword && item->arg_len > 0 &&\n> +\t\t\t    (strlen(msg.subject) != item->arg_len ||\n> +\t\t\t     strncmp(msg.subject, todo_item_get_arg(todo_list,\n> +\t\t\t\t     item),item->arg_len) != 0)) {\n> +\t\t\t\tconst char *commit_buf, *subject;\n> +\t\t\t\tint subject_len;\n> +\t\t\t\tunsigned long commit_buf_len;\n> +\n> +\t\t\t\tflags |= INLINE_MSG;\n> +\t\t\t\topts->subject = todo_item_get_arg(todo_list,\n> +\t\t\t\t\titem);\n> +\t\t\t\topts->subject_len = item->arg_len;\n> +\t\t\t\tcommit_buf = get_commit_buffer(commit,\n> +\t\t\t\t\t&commit_buf_len);\n> +\t\t\t\tsubject_len = find_commit_subject(commit_buf,\n> +\t\t\t\t\t&subject);\n> +\t\t\t\topts->content = subject + subject_len;\n> +\t\t\t\topts->content_len = commit_buf +\n> +\t\t\t\t\tcommit_buf_len - opts->content;\n> +\t\t\t}\n>   \t\t\tres = run_git_commit(NULL, opts, EDIT_MSG |\n>   \t\t\t\t\t     VERIFY_MSG | AMEND_MSG |\n> -\t\t\t\t\t     (flags & ALLOW_EMPTY));\n> +\t\t\t\t\t     (flags & (ALLOW_EMPTY | INLINE_MSG)));\n>   \t\t\t*check_todo = 1;\n>   \t\t}\n>   \t}\n> @@ -4621,7 +4661,7 @@ static int pick_commits(struct repository *r,\n>   \t\t\t\tsetenv(GIT_REFLOG_ACTION, reflog_message(opts,\n>   \t\t\t\t\tcommand_to_string(item->command), NULL),\n>   \t\t\t\t\t1);\n> -\t\t\tres = do_pick_commit(r, item, opts,\n> +\t\t\tres = do_pick_commit(r, todo_list, item, opts,\n>   \t\t\t\t\t     is_final_fixup(todo_list),\n>   \t\t\t\t\t     &check_todo);\n>   \t\t\tif (is_rebase_i(opts))\n> @@ -5101,6 +5141,7 @@ release_todo_list:\n>   }\n>   \n>   static int single_pick(struct repository *r,\n> +\t\t       struct todo_list *todo_list,\n>   \t\t       struct commit *cmit,\n>   \t\t       struct replay_opts *opts)\n>   {\n> @@ -5112,7 +5153,7 @@ static int single_pick(struct repository *r,\n>   \titem.commit = cmit;\n>   \n>   \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n> -\treturn do_pick_commit(r, &item, opts, 0, &check_todo);\n> +\treturn do_pick_commit(r, todo_list, &item, opts, 0, &check_todo);\n>   }\n>   \n>   int sequencer_pick_revisions(struct repository *r,\n> @@ -5165,7 +5206,7 @@ int sequencer_pick_revisions(struct repository *r,\n>   \t\t\treturn error(_(\"empty commit set passed\"));\n>   \t\tif (get_revision(opts->revs))\n>   \t\t\tBUG(\"unexpected extra commit from walk\");\n> -\t\treturn single_pick(r, cmit, opts);\n> +\t\treturn single_pick(r, &todo_list, cmit, opts);\n>   \t}\n>   \n>   \t/*\n> diff --git a/sequencer.h b/sequencer.h\n> index 563fe599334..26e3e43ce99 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -42,6 +42,7 @@ struct replay_opts {\n>   \tint allow_rerere_auto;\n>   \tint allow_empty;\n>   \tint allow_empty_message;\n> +\tint allow_inline_reword;\n>   \tint drop_redundant_commits;\n>   \tint keep_redundant_commits;\n>   \tint verbose;\n> @@ -57,6 +58,11 @@ struct replay_opts {\n>   \tenum commit_msg_cleanup_mode default_msg_cleanup;\n>   \tint explicit_cleanup;\n>   \n> +\tconst char *content;\n> +\tint content_len;\n> +\tconst char *subject;\n> +\tint subject_len;\n> +\n>   \t/* Merge strategy */\n>   \tchar *default_strategy;  /* from config options */\n>   \tchar *strategy;\n> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\n> index b57541356bd..8c9dacdad9a 100644\n> --- a/t/lib-rebase.sh\n> +++ b/t/lib-rebase.sh\n> @@ -222,3 +222,32 @@ set_replace_editor () {\n>   \tsed -e \"s/FILENAME/$1/g\" <script | write_script fake-editor.sh &&\n>   \ttest_set_editor \"$(pwd)/fake-editor.sh\"\n>   }\n> +\n> +set_inline_reword_editor () {\n> +\twrite_script fake-editor.sh <<-\\EOF\n> +\tgrep -v '^#' < \"$1\" > \"$1\".tmp\n> +\trm -f \"$1\"\n> +\techo 'rebase -i script before editing:'\n> +\tcat \"$1\".tmp\n> +\taction=\\\\2\n> +\thash=\\\\3\n> +\tsubject=\\\\4\n> +\tfor line in $FAKE_LINES; do\n> +\t\tcase $line in\n> +\t\tpick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)\n> +\t\t\tsubject=\\\\4\n> +\t\t\taction=\"$line\";;\n> +\t\treword_*)\n> +\t\t\taction=reword\n> +\t\t\tsubject=$(echo \"$line\" | sed 's,reword_\\(.*\\),\\1,');;\n> +\t\t*)\n> +\t\t\tsed -n \"${line}s,^\\(\\([a-z][a-z]*\\)[[:space:]]*\\([a-f0-9][a-f0-9]*\\)[[:space:]]*\\(.*\\)\\),${action} ${hash} ${subject},p\" < \"$1\".tmp >> \"$1\"\n> +\t\t\taction=\\\\2;;\n> +\t\tesac\n> +\tdone\n> +\techo 'rebase -i script after editing:'\n> +\tcat \"$1\"\n> +\tEOF\n> +\n> +\ttest_set_editor \"$(pwd)/fake-editor.sh\"\n> +}\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 688b01e3eb6..4c27d5f1c91 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -951,6 +951,17 @@ test_expect_success 'rebase -ix with --autosquash' '\n>   \ttest_cmp expected actual\n>   '\n>   \n> +test_expect_success 'rebase -i with --allow-inline-reword' '\n> +\tgit checkout -b reword-inline primary &&\n> +\t(\n> +\t\tset_inline_reword_editor &&\n> +\t\tFAKE_LINES=\"reword_Bchanged 1 2 reword_Dchanged 3 4\" \\\n> +\t\t\tgit rebase --allow-inline-reword -i HEAD~4\n> +\t) &&\n> +\ttest Bchanged = $(git show -s --format=%s HEAD~3) &&\n> +\ttest Dchanged = $(git show -s --format=%s HEAD~)\n> +'\n> +\n>   test_expect_success 'rebase --exec works without -i ' '\n>   \tgit reset --hard execute &&\n>   \trm -rf exec_output &&\n> \n> base-commit: 350dc9f0e8974b6fcbdeb3808186c5a79c3e7386\n"},{"id":"460608","messageId":"220804.861qtwe8dy.gmgdl@evledraar.gmail.com","threadId":"58255","inReplyTo":"pull.1228.git.git.1659422389830.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase: introduce allow-inline-reword option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-04T07:57:15Z","receivedAt":"2022-08-04T08:10:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 02 2022, Théo MAILLART via GitGitGadget wrote:\n\n> From: =?UTF-8?q?Th=C3=A9o=20Maillart?= <tmaillart@gmail.com>\n>\n> This new option (false by default) for interactive rebase allows users\n> to modify the subject of a commit directly in the todo list, when they\n> select the \"reword\" action.\n> If the option is enabled, \"reword\" is selected and the subject has not\n> changed, then the default behaviour is used.\n> It also introduces a test for this specific option, and a related\n> function (set_inline_reword_editor) in the lib-rebase.sh to use a\n> simpler custom fake editor to be able to modify the message part of the\n> lines in a todo list (in the most simple cases).\n\nI won't repeat what others have noted (e.g. reference to earlier\ndiscussions).\n\nJust an observation: When I use \"git rebase -i\" I use Emacs's\n\"git-rebase-mode.el\" to view the resulting buffer, it has useful\nshortcuts. E.g. M-p and M-n move a line up/down, \"c\" is pick\", \"r\" is\nreword etc.\n\nNow that mode can mark the whole buffer read-only, there is a way to\ninsert into it, e.g. with \"y\" to insert a <rev>, or \"x\" to insert\n\"exec\", but those bring up an interactive dialog.\n\nBut there's nothing that e.g. would warrant search/replacing the whole\nbuffer now.\n\nIt's no big deal for an Emacs mode to add it I'd think, just worth\nconsidering what we're pushing to downstream UX's.\n\n>     If the user only wants to modify the subject of a commit during\n>     interactive rebase, now he can choose the \"reword\" command and change\n>     the subject directly in the todo list. If \"reword\" is selected and the\n>     subject has not changed, then we use the default behavior. Here is a\n>     demo https://asciinema.org/a/T9tEmUjjl4dyDuaXalPzXNzo1\n>     \n>     This is probably not the correct way to implement this new feature, or\n>     perhaps it is not desirable, so I would like some feedback. Thank you\n\nWouldn't another implementation of this be to do:\n\n\texec EDITOR=\"...\" git commit --amend\n\nWhere that \"editor\" is a sed one-liner to s/<first line>/replacement/?\n\nI don't mind this feature per-se, but I will observe that since I\nstarted using git-rebase-mode.el its keymap has gotten much more\ncluttered over the years, as we've added more special modes (and it has\nmaps of its own).\n\nI don't object to it, but wonder if we couldn't add something that's\nmore generic, and wouldn't require e.g. a\n--allow-adding-a-new-paragraph-after-the-subject-that-is-already-there,\ni.e. some users will want to reword the subject, others want to add a\nsigned-off-by.\n\nFor the SOB case we already have:\n\n\tgit rebase -i -x 'git commit --no-edit --amend -s'\n\nPerhaps we could/should have instead:\n\n\tgit commit --amend-subject\n\nWhich would allow you to do:\n\n\tgit rebase -i -x 'git commit --amend-subject -m\"new subject here, does not touch anything past the first \\n\\n\"'\n\n"}]}