[PATCH] rebase: introduce allow-inline-reword option

[PATCH] rebase: introduce allow-inline-reword option

[Théo MAILLART via GitGitGadget]

From: =?UTF-8?q?Th=C3=A9o=20Maillart?= <tmaillart@gmail.com>

This new option (false by default) for interactive rebase allows users
to modify the subject of a commit directly in the todo list, when they
select the "reword" action.
If the option is enabled, "reword" is selected and the subject has not
changed, then the default behaviour is used.
It also introduces a test for this specific option, and a related
function (set_inline_reword_editor) in the lib-rebase.sh to use a
simpler custom fake editor to be able to modify the message part of the
lines in a todo list (in the most simple cases).

Signed-off-by: Théo Maillart <tmaillart@gmail.com>
---
    [RFC] rebase: reword: new feature change the subject in the todo list
    
    If the user only wants to modify the subject of a commit during
    interactive rebase, now he can choose the "reword" command and change
    the subject directly in the todo list. If "reword" is selected and the
    subject has not changed, then we use the default behavior. Here is a
    demo https://asciinema.org/a/T9tEmUjjl4dyDuaXalPzXNzo1
    
    This is probably not the correct way to implement this new feature, or
    perhaps it is not desirable, so I would like some feedback. Thank you

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1228%2Ftmaillart%2Ftm%2Freword-inline-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1228/tmaillart/tm/reword-inline-v1
Pull-Request: https://github.com/git/git/pull/1228

 Documentation/git-rebase.txt  |  7 +++++
 builtin/rebase.c              | 11 ++++++++
 sequencer.c                   | 49 ++++++++++++++++++++++++++++++++---
 sequencer.h                   |  6 +++++
 t/lib-rebase.sh               | 29 +++++++++++++++++++++
 t/t3404-rebase-interactive.sh | 11 ++++++++
 6 files changed, 109 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 080658c8710..dcfd65d0bc1 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -332,6 +332,13 @@ See also INCOMPATIBLE OPTIONS below.
 +
 See also INCOMPATIBLE OPTIONS below.
 
+--allow-inline-reword::
+	When performing an interactive rebase, this flag allows you to change
+	the subject of a specific commit directly in the todo list (only when
+	reword command is selected) without asking you to edit the commit
+	messages sequencially. If you select reword without changing the
+	commit's subject then you will get the default reword behaviour.
+
 --skip::
 	Restart the rebasing process by skipping the current patch.
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 56e4214b441..cfb86f15a69 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -103,6 +103,7 @@ struct rebase_options {
 	int reapply_cherry_picks;
 	int fork_point;
 	int update_refs;
+	int allow_inline_reword;
 };
 
 #define REBASE_OPTIONS_INIT {			  	\
@@ -130,6 +131,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
 	replay.allow_empty = 1;
 	replay.allow_empty_message = opts->allow_empty_message;
+	replay.allow_inline_reword = opts->allow_inline_reword;
 	replay.drop_redundant_commits = (opts->empty == EMPTY_DROP);
 	replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
 	replay.quiet = !(opts->flags & REBASE_NO_QUIET);
@@ -821,6 +823,11 @@ static int rebase_config(const char *var, const char *value, void *data)
 		return git_config_string(&opts->default_backend, var, value);
 	}
 
+	if (!strcmp(var, "rebase.allowinlinereword")) {
+		opts->allow_inline_reword = git_config_bool(var, value);
+		return 0;
+	}
+
 	return git_default_config(var, value, data);
 }
 
@@ -1164,6 +1171,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			 N_("automatically re-schedule any `exec` that fails")),
 		OPT_BOOL(0, "reapply-cherry-picks", &options.reapply_cherry_picks,
 			 N_("apply all changes, even those already present upstream")),
+		OPT_BOOL_F(0, "allow-inline-reword",
+			   &options.allow_inline_reword,
+			   N_("allow changing commit's subject directly in the todo list"),
+			   PARSE_OPT_HIDDEN),
 		OPT_END(),
 	};
 	int i;
diff --git a/sequencer.c b/sequencer.c
index 5f22b7cd377..8623296f428 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -999,6 +999,7 @@ N_("you have staged changes in your working tree\n"
 #define VERIFY_MSG  (1<<4)
 #define CREATE_ROOT_COMMIT (1<<5)
 #define VERBATIM_MSG (1<<6)
+#define INLINE_MSG   (1<<7)
 
 static int run_command_silent_on_success(struct child_process *cmd)
 {
@@ -1076,6 +1077,24 @@ static int run_git_commit(const char *defmsg,
 		strvec_push(&cmd.args, "--cleanup=strip");
 	if ((flags & VERBATIM_MSG))
 		strvec_push(&cmd.args, "--cleanup=verbatim");
+	if ((flags & EDIT_MSG) && (flags & INLINE_MSG)) {
+		int ret;
+
+		strvec_push(&cmd.args, "-F");
+		strvec_push(&cmd.args, "-");
+		cmd.in = -1;
+		ret = start_command(&cmd);
+		if (ret)
+			return ret;
+		if (write(cmd.in, opts->subject, opts->subject_len) !=
+		    opts->subject_len)
+			return error(_("subject write error to child process"));
+		if (write(cmd.in, opts->content, opts->content_len) !=
+		    opts->content_len)
+			return error(_("content write error to child process"));
+		close(cmd.in);
+		return finish_command(&cmd);
+	}
 	if ((flags & EDIT_MSG))
 		strvec_push(&cmd.args, "-e");
 	else if (!(flags & CLEANUP_MSG) &&
@@ -2118,6 +2137,7 @@ static void refer_to_commit(struct replay_opts *opts,
 }
 
 static int do_pick_commit(struct repository *r,
+			  struct todo_list *todo_list,
 			  struct todo_item *item,
 			  struct replay_opts *opts,
 			  int final_fixup, int *check_todo)
@@ -2375,9 +2395,29 @@ static int do_pick_commit(struct repository *r,
 		*check_todo = !!(flags & EDIT_MSG);
 		if (!res && reword) {
 fast_forward_edit:
+			if (opts->allow_inline_reword && item->arg_len > 0 &&
+			    (strlen(msg.subject) != item->arg_len ||
+			     strncmp(msg.subject, todo_item_get_arg(todo_list,
+				     item),item->arg_len) != 0)) {
+				const char *commit_buf, *subject;
+				int subject_len;
+				unsigned long commit_buf_len;
+
+				flags |= INLINE_MSG;
+				opts->subject = todo_item_get_arg(todo_list,
+					item);
+				opts->subject_len = item->arg_len;
+				commit_buf = get_commit_buffer(commit,
+					&commit_buf_len);
+				subject_len = find_commit_subject(commit_buf,
+					&subject);
+				opts->content = subject + subject_len;
+				opts->content_len = commit_buf +
+					commit_buf_len - opts->content;
+			}
 			res = run_git_commit(NULL, opts, EDIT_MSG |
 					     VERIFY_MSG | AMEND_MSG |
-					     (flags & ALLOW_EMPTY));
+					     (flags & (ALLOW_EMPTY | INLINE_MSG)));
 			*check_todo = 1;
 		}
 	}
@@ -4621,7 +4661,7 @@ static int pick_commits(struct repository *r,
 				setenv(GIT_REFLOG_ACTION, reflog_message(opts,
 					command_to_string(item->command), NULL),
 					1);
-			res = do_pick_commit(r, item, opts,
+			res = do_pick_commit(r, todo_list, item, opts,
 					     is_final_fixup(todo_list),
 					     &check_todo);
 			if (is_rebase_i(opts))
@@ -5101,6 +5141,7 @@ release_todo_list:
 }
 
 static int single_pick(struct repository *r,
+		       struct todo_list *todo_list,
 		       struct commit *cmit,
 		       struct replay_opts *opts)
 {
@@ -5112,7 +5153,7 @@ static int single_pick(struct repository *r,
 	item.commit = cmit;
 
 	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-	return do_pick_commit(r, &item, opts, 0, &check_todo);
+	return do_pick_commit(r, todo_list, &item, opts, 0, &check_todo);
 }
 
 int sequencer_pick_revisions(struct repository *r,
@@ -5165,7 +5206,7 @@ int sequencer_pick_revisions(struct repository *r,
 			return error(_("empty commit set passed"));
 		if (get_revision(opts->revs))
 			BUG("unexpected extra commit from walk");
-		return single_pick(r, cmit, opts);
+		return single_pick(r, &todo_list, cmit, opts);
 	}
 
 	/*
diff --git a/sequencer.h b/sequencer.h
index 563fe599334..26e3e43ce99 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -42,6 +42,7 @@ struct replay_opts {
 	int allow_rerere_auto;
 	int allow_empty;
 	int allow_empty_message;
+	int allow_inline_reword;
 	int drop_redundant_commits;
 	int keep_redundant_commits;
 	int verbose;
@@ -57,6 +58,11 @@ struct replay_opts {
 	enum commit_msg_cleanup_mode default_msg_cleanup;
 	int explicit_cleanup;
 
+	const char *content;
+	int content_len;
+	const char *subject;
+	int subject_len;
+
 	/* Merge strategy */
 	char *default_strategy;  /* from config options */
 	char *strategy;
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index b57541356bd..8c9dacdad9a 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -222,3 +222,32 @@ set_replace_editor () {
 	sed -e "s/FILENAME/$1/g" <script | write_script fake-editor.sh &&
 	test_set_editor "$(pwd)/fake-editor.sh"
 }
+
+set_inline_reword_editor () {
+	write_script fake-editor.sh <<-\EOF
+	grep -v '^#' < "$1" > "$1".tmp
+	rm -f "$1"
+	echo 'rebase -i script before editing:'
+	cat "$1".tmp
+	action=\\2
+	hash=\\3
+	subject=\\4
+	for line in $FAKE_LINES; do
+		case $line in
+		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
+			subject=\\4
+			action="$line";;
+		reword_*)
+			action=reword
+			subject=$(echo "$line" | sed 's,reword_\(.*\),\1,');;
+		*)
+			sed -n "${line}s,^\(\([a-z][a-z]*\)[[:space:]]*\([a-f0-9][a-f0-9]*\)[[:space:]]*\(.*\)\),${action} ${hash} ${subject},p" < "$1".tmp >> "$1"
+			action=\\2;;
+		esac
+	done
+	echo 'rebase -i script after editing:'
+	cat "$1"
+	EOF
+
+	test_set_editor "$(pwd)/fake-editor.sh"
+}
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 688b01e3eb6..4c27d5f1c91 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -951,6 +951,17 @@ test_expect_success 'rebase -ix with --autosquash' '
 	test_cmp expected actual
 '
 
+test_expect_success 'rebase -i with --allow-inline-reword' '
+	git checkout -b reword-inline primary &&
+	(
+		set_inline_reword_editor &&
+		FAKE_LINES="reword_Bchanged 1 2 reword_Dchanged 3 4" \
+			git rebase --allow-inline-reword -i HEAD~4
+	) &&
+	test Bchanged = $(git show -s --format=%s HEAD~3) &&
+	test Dchanged = $(git show -s --format=%s HEAD~)
+'
+
 test_expect_success 'rebase --exec works without -i ' '
 	git reset --hard execute &&
 	rm -rf exec_output &&

base-commit: 350dc9f0e8974b6fcbdeb3808186c5a79c3e7386
-- 
gitgitgadget

Re: [PATCH] rebase: introduce allow-inline-reword option

[Junio C Hamano]

"Théo MAILLART via GitGitGadget"  <gitgitgadget@gmail.com> writes:

> This new option (false by default) for interactive rebase allows users
> to modify the subject of a commit directly in the todo list, when they
> select the "reword" action.
> If the option is enabled, "reword" is selected and the subject has not
> changed, then the default behaviour is used.
> It also introduces a test for this specific option, and a related
> function (set_inline_reword_editor) in the lib-rebase.sh to use a
> simpler custom fake editor to be able to modify the message part of the
> lines in a todo list (in the most simple cases).
>
> Signed-off-by: Théo Maillart <tmaillart@gmail.com>
> ---
>     [RFC] rebase: reword: new feature change the subject in the todo list

It is not clear if you meant this as a final submission or an RFC
but I'll take it as an RFC for now.

A handful of things come to my mind.

 * Would this want to be a new variant of "reword" that you would
   write into the todo list file, instead of a command line option
   that says "every 'reword' I write in the todo list file means
   something different now"?

 * Is there a plausible UI that allows inline editing of a commit
   log message that is more than one line long?  Should there be?

 * Under "inline" mode, when a "reword" is requested for a commit
   that has more than one line of log message, what should happen?
   Should the updated title become the ONLY content of the log of
   the updated commit?  Should it be an error, because it is clearly
   an information-losing operation?  Would it make sense to turn the
   "inline" reword into normal reword automatically for a commit
   with more than one line of log message?

 * If we choose to special case a commit with more than one line of
   message in order to prevent the 'inline' mode from losing
   valuable information in the original commits, what role should
   trailer lines play when we decide if a commit has only one line
   of message?  For example, if a lazy "title only" commit has no
   body message but a sign-off and other trailers like helped-by,
   would it make sense to keep the trailers intact and only replace
   the title, still in inline mode?

Here is an alternative design that may be conceptually cleaner.

 * We do not introduce a new option at all.  "reword" means "open
   the editor and you can edit the whole thing" as always.

 * We introduce "retitle" that can be used instead of "reword".

   The line for a commit originally shows "pick" followed by an
   abbreviated commit object name followed by its title, and the
   body of the message and the trailer is hidden.  If you change
   "pick" to "retitle" and edit the shown title, then the original
   log message from the commit is read as a whole, its title line is
   replaced with what "retitle" line has, and the result is used as
   the updated log message.

That way, those who write more than one line of commit log message
can still use the feature without having to worry about losing
information when the only thing they want to fix is a typo in the
title, and those who write only one line of commit log message do
not have to pass the new "--inline" option at all.  They can use
'retitle' instead of 'reword'.

Hmm?

Re: [PATCH] rebase: introduce allow-inline-reword option

[Théo Maillart]

On 8/2/22 17:23, Junio C Hamano wrote:
> "Théo MAILLART via GitGitGadget"<gitgitgadget@gmail.com>  writes:
>
>> This new option (false by default) for interactive rebase allows users
>> to modify the subject of a commit directly in the todo list, when they
>> select the "reword" action.
>> If the option is enabled, "reword" is selected and the subject has not
>> changed, then the default behaviour is used.
>> It also introduces a test for this specific option, and a related
>> function (set_inline_reword_editor) in the lib-rebase.sh to use a
>> simpler custom fake editor to be able to modify the message part of the
>> lines in a todo list (in the most simple cases).
>>
>> Signed-off-by: Théo Maillart<tmaillart@gmail.com>
>> ---
>>      [RFC] rebase: reword: new feature change the subject in the todo list
> It is not clear if you meant this as a final submission or an RFC
> but I'll take it as an RFC for now.
>
> A handful of things come to my mind.
>
>   * Would this want to be a new variant of "reword" that you would
>     write into the todo list file, instead of a command line option
>     that says "every 'reword' I write in the todo list file means
>     something different now"?
>
>   * Is there a plausible UI that allows inline editing of a commit
>     log message that is more than one line long?  Should there be?
>
>   * Under "inline" mode, when a "reword" is requested for a commit
>     that has more than one line of log message, what should happen?
>     Should the updated title become the ONLY content of the log of
>     the updated commit?  Should it be an error, because it is clearly
>     an information-losing operation?  Would it make sense to turn the
>     "inline" reword into normal reword automatically for a commit
>     with more than one line of log message?
>
>   * If we choose to special case a commit with more than one line of
>     message in order to prevent the 'inline' mode from losing
>     valuable information in the original commits, what role should
>     trailer lines play when we decide if a commit has only one line
>     of message?  For example, if a lazy "title only" commit has no
>     body message but a sign-off and other trailers like helped-by,
>     would it make sense to keep the trailers intact and only replace
>     the title, still in inline mode?

I agree, taking care of more than the commit subject only does
not look like an easy task, and I'm probably not the right person
to do this in a reasonable amount of time.

>
> Here is an alternative design that may be conceptually cleaner.
>
>   * We do not introduce a new option at all.  "reword" means "open
>     the editor and you can edit the whole thing" as always.
>
>   * We introduce "retitle" that can be used instead of "reword".
>
>     The line for a commit originally shows "pick" followed by an
>     abbreviated commit object name followed by its title, and the
>     body of the message and the trailer is hidden.  If you change
>     "pick" to "retitle" and edit the shown title, then the original
>     log message from the commit is read as a whole, its title line is
>     replaced with what "retitle" line has, and the result is used as
>     the updated log message.
>
> That way, those who write more than one line of commit log message
> can still use the feature without having to worry about losing
> information when the only thing they want to fix is a typo in the
> title, and those who write only one line of commit log message do
> not have to pass the new "--inline" option at all.  They can use
> 'retitle' instead of 'reword'.
>
> Hmm?
>
So, if I understand your suggestion correctly, we can say that,
most of the logic is implemented in this patch, but I should move
the "inline" logic from the "reword" to a new action "retitle".
If it is ok with you, I will look into that, and get back with a new
patch.
The only thing that seems unfortunate to me is that we will have
a hard time finding a meaningful short name for this new action in
the todo list, as "r" is for "rename".

Re: [PATCH] rebase: introduce allow-inline-reword option

[Junio C Hamano]

Théo Maillart <tmaillart@gmail.com> writes:

> So, if I understand your suggestion correctly, we can say that,
> most of the logic is implemented in this patch, but I should move
> the "inline" logic from the "reword" to a new action "retitle".

Yeah, I was hoping that most of the logic has already been written,
except that it is likely you just ignored the original log message
and overwrote it with the single line you got, but now you need to
read the original and replace only the title (the "paragraph" before
the first blank line), which you may not have yet.

Re: [PATCH] rebase: introduce allow-inline-reword option

[Eric Sunshine]

On Tue, Aug 2, 2022 at 6:43 PM Junio C Hamano <gitster@pobox.com> wrote:
> Théo Maillart <tmaillart@gmail.com> writes:
> > So, if I understand your suggestion correctly, we can say that,
> > most of the logic is implemented in this patch, but I should move
> > the "inline" logic from the "reword" to a new action "retitle".
>
> Yeah, I was hoping that most of the logic has already been written,
> except that it is likely you just ignored the original log message
> and overwrote it with the single line you got, but now you need to
> read the original and replace only the title (the "paragraph" before
> the first blank line), which you may not have yet.

Michael Haggerty had made a more generalized proposal[1] some time
back which might or might not be relevant here. The notion of being
able to edit the title of commit came up in the ensuing discussion, as
well.

[1]: https://lore.kernel.org/git/530DA00E.4090402@alum.mit.edu/

Re: [PATCH] rebase: introduce allow-inline-reword option

[Phillip Wood]

Hi Théo

On 02/08/2022 07:39, Théo MAILLART via GitGitGadget wrote:
> From: =?UTF-8?q?Th=C3=A9o=20Maillart?= <tmaillart@gmail.com>
> 
> This new option (false by default) for interactive rebase allows users
> to modify the subject of a commit directly in the todo list, when they
> select the "reword" action.
> If the option is enabled, "reword" is selected and the subject has not
> changed, then the default behaviour is used.
> It also introduces a test for this specific option, and a related
> function (set_inline_reword_editor) in the lib-rebase.sh to use a
> simpler custom fake editor to be able to modify the message part of the
> lines in a todo list (in the most simple cases).

I'm in two minds about editing the commit subject it the todo list. I 
can see it would occasionally be useful to correct a typo in the commit 
subject that you notice when editing the todo list. However I'm not sure 
how common a case that is compared to having to edit the main body of 
the message and I'm concerned it would encourage people towards single 
line commit message which is something we definitely want to avoid.

Best Wishes

Phillip

> Signed-off-by: Théo Maillart <tmaillart@gmail.com>
> ---
>      [RFC] rebase: reword: new feature change the subject in the todo list
>      
>      If the user only wants to modify the subject of a commit during
>      interactive rebase, now he can choose the "reword" command and change
>      the subject directly in the todo list. If "reword" is selected and the
>      subject has not changed, then we use the default behavior. Here is a
>      demo https://asciinema.org/a/T9tEmUjjl4dyDuaXalPzXNzo1
>      
>      This is probably not the correct way to implement this new feature, or
>      perhaps it is not desirable, so I would like some feedback. Thank you
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1228%2Ftmaillart%2Ftm%2Freword-inline-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1228/tmaillart/tm/reword-inline-v1
> Pull-Request: https://github.com/git/git/pull/1228
> 
>   Documentation/git-rebase.txt  |  7 +++++
>   builtin/rebase.c              | 11 ++++++++
>   sequencer.c                   | 49 ++++++++++++++++++++++++++++++++---
>   sequencer.h                   |  6 +++++
>   t/lib-rebase.sh               | 29 +++++++++++++++++++++
>   t/t3404-rebase-interactive.sh | 11 ++++++++
>   6 files changed, 109 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 080658c8710..dcfd65d0bc1 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -332,6 +332,13 @@ See also INCOMPATIBLE OPTIONS below.
>   +
>   See also INCOMPATIBLE OPTIONS below.
>   
> +--allow-inline-reword::
> +	When performing an interactive rebase, this flag allows you to change
> +	the subject of a specific commit directly in the todo list (only when
> +	reword command is selected) without asking you to edit the commit
> +	messages sequencially. If you select reword without changing the
> +	commit's subject then you will get the default reword behaviour.
> +
>   --skip::
>   	Restart the rebasing process by skipping the current patch.
>   
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 56e4214b441..cfb86f15a69 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -103,6 +103,7 @@ struct rebase_options {
>   	int reapply_cherry_picks;
>   	int fork_point;
>   	int update_refs;
> +	int allow_inline_reword;
>   };
>   
>   #define REBASE_OPTIONS_INIT {			  	\
> @@ -130,6 +131,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>   		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
>   	replay.allow_empty = 1;
>   	replay.allow_empty_message = opts->allow_empty_message;
> +	replay.allow_inline_reword = opts->allow_inline_reword;
>   	replay.drop_redundant_commits = (opts->empty == EMPTY_DROP);
>   	replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
>   	replay.quiet = !(opts->flags & REBASE_NO_QUIET);
> @@ -821,6 +823,11 @@ static int rebase_config(const char *var, const char *value, void *data)
>   		return git_config_string(&opts->default_backend, var, value);
>   	}
>   
> +	if (!strcmp(var, "rebase.allowinlinereword")) {
> +		opts->allow_inline_reword = git_config_bool(var, value);
> +		return 0;
> +	}
> +
>   	return git_default_config(var, value, data);
>   }
>   
> @@ -1164,6 +1171,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   			 N_("automatically re-schedule any `exec` that fails")),
>   		OPT_BOOL(0, "reapply-cherry-picks", &options.reapply_cherry_picks,
>   			 N_("apply all changes, even those already present upstream")),
> +		OPT_BOOL_F(0, "allow-inline-reword",
> +			   &options.allow_inline_reword,
> +			   N_("allow changing commit's subject directly in the todo list"),
> +			   PARSE_OPT_HIDDEN),
>   		OPT_END(),
>   	};
>   	int i;
> diff --git a/sequencer.c b/sequencer.c
> index 5f22b7cd377..8623296f428 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -999,6 +999,7 @@ N_("you have staged changes in your working tree\n"
>   #define VERIFY_MSG  (1<<4)
>   #define CREATE_ROOT_COMMIT (1<<5)
>   #define VERBATIM_MSG (1<<6)
> +#define INLINE_MSG   (1<<7)
>   
>   static int run_command_silent_on_success(struct child_process *cmd)
>   {
> @@ -1076,6 +1077,24 @@ static int run_git_commit(const char *defmsg,
>   		strvec_push(&cmd.args, "--cleanup=strip");
>   	if ((flags & VERBATIM_MSG))
>   		strvec_push(&cmd.args, "--cleanup=verbatim");
> +	if ((flags & EDIT_MSG) && (flags & INLINE_MSG)) {
> +		int ret;
> +
> +		strvec_push(&cmd.args, "-F");
> +		strvec_push(&cmd.args, "-");
> +		cmd.in = -1;
> +		ret = start_command(&cmd);
> +		if (ret)
> +			return ret;
> +		if (write(cmd.in, opts->subject, opts->subject_len) !=
> +		    opts->subject_len)
> +			return error(_("subject write error to child process"));
> +		if (write(cmd.in, opts->content, opts->content_len) !=
> +		    opts->content_len)
> +			return error(_("content write error to child process"));
> +		close(cmd.in);
> +		return finish_command(&cmd);
> +	}
>   	if ((flags & EDIT_MSG))
>   		strvec_push(&cmd.args, "-e");
>   	else if (!(flags & CLEANUP_MSG) &&
> @@ -2118,6 +2137,7 @@ static void refer_to_commit(struct replay_opts *opts,
>   }
>   
>   static int do_pick_commit(struct repository *r,
> +			  struct todo_list *todo_list,
>   			  struct todo_item *item,
>   			  struct replay_opts *opts,
>   			  int final_fixup, int *check_todo)
> @@ -2375,9 +2395,29 @@ static int do_pick_commit(struct repository *r,
>   		*check_todo = !!(flags & EDIT_MSG);
>   		if (!res && reword) {
>   fast_forward_edit:
> +			if (opts->allow_inline_reword && item->arg_len > 0 &&
> +			    (strlen(msg.subject) != item->arg_len ||
> +			     strncmp(msg.subject, todo_item_get_arg(todo_list,
> +				     item),item->arg_len) != 0)) {
> +				const char *commit_buf, *subject;
> +				int subject_len;
> +				unsigned long commit_buf_len;
> +
> +				flags |= INLINE_MSG;
> +				opts->subject = todo_item_get_arg(todo_list,
> +					item);
> +				opts->subject_len = item->arg_len;
> +				commit_buf = get_commit_buffer(commit,
> +					&commit_buf_len);
> +				subject_len = find_commit_subject(commit_buf,
> +					&subject);
> +				opts->content = subject + subject_len;
> +				opts->content_len = commit_buf +
> +					commit_buf_len - opts->content;
> +			}
>   			res = run_git_commit(NULL, opts, EDIT_MSG |
>   					     VERIFY_MSG | AMEND_MSG |
> -					     (flags & ALLOW_EMPTY));
> +					     (flags & (ALLOW_EMPTY | INLINE_MSG)));
>   			*check_todo = 1;
>   		}
>   	}
> @@ -4621,7 +4661,7 @@ static int pick_commits(struct repository *r,
>   				setenv(GIT_REFLOG_ACTION, reflog_message(opts,
>   					command_to_string(item->command), NULL),
>   					1);
> -			res = do_pick_commit(r, item, opts,
> +			res = do_pick_commit(r, todo_list, item, opts,
>   					     is_final_fixup(todo_list),
>   					     &check_todo);
>   			if (is_rebase_i(opts))
> @@ -5101,6 +5141,7 @@ release_todo_list:
>   }
>   
>   static int single_pick(struct repository *r,
> +		       struct todo_list *todo_list,
>   		       struct commit *cmit,
>   		       struct replay_opts *opts)
>   {
> @@ -5112,7 +5153,7 @@ static int single_pick(struct repository *r,
>   	item.commit = cmit;
>   
>   	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> -	return do_pick_commit(r, &item, opts, 0, &check_todo);
> +	return do_pick_commit(r, todo_list, &item, opts, 0, &check_todo);
>   }
>   
>   int sequencer_pick_revisions(struct repository *r,
> @@ -5165,7 +5206,7 @@ int sequencer_pick_revisions(struct repository *r,
>   			return error(_("empty commit set passed"));
>   		if (get_revision(opts->revs))
>   			BUG("unexpected extra commit from walk");
> -		return single_pick(r, cmit, opts);
> +		return single_pick(r, &todo_list, cmit, opts);
>   	}
>   
>   	/*
> diff --git a/sequencer.h b/sequencer.h
> index 563fe599334..26e3e43ce99 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -42,6 +42,7 @@ struct replay_opts {
>   	int allow_rerere_auto;
>   	int allow_empty;
>   	int allow_empty_message;
> +	int allow_inline_reword;
>   	int drop_redundant_commits;
>   	int keep_redundant_commits;
>   	int verbose;
> @@ -57,6 +58,11 @@ struct replay_opts {
>   	enum commit_msg_cleanup_mode default_msg_cleanup;
>   	int explicit_cleanup;
>   
> +	const char *content;
> +	int content_len;
> +	const char *subject;
> +	int subject_len;
> +
>   	/* Merge strategy */
>   	char *default_strategy;  /* from config options */
>   	char *strategy;
> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index b57541356bd..8c9dacdad9a 100644
> --- a/t/lib-rebase.sh
> +++ b/t/lib-rebase.sh
> @@ -222,3 +222,32 @@ set_replace_editor () {
>   	sed -e "s/FILENAME/$1/g" <script | write_script fake-editor.sh &&
>   	test_set_editor "$(pwd)/fake-editor.sh"
>   }
> +
> +set_inline_reword_editor () {
> +	write_script fake-editor.sh <<-\EOF
> +	grep -v '^#' < "$1" > "$1".tmp
> +	rm -f "$1"
> +	echo 'rebase -i script before editing:'
> +	cat "$1".tmp
> +	action=\\2
> +	hash=\\3
> +	subject=\\4
> +	for line in $FAKE_LINES; do
> +		case $line in
> +		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
> +			subject=\\4
> +			action="$line";;
> +		reword_*)
> +			action=reword
> +			subject=$(echo "$line" | sed 's,reword_\(.*\),\1,');;
> +		*)
> +			sed -n "${line}s,^\(\([a-z][a-z]*\)[[:space:]]*\([a-f0-9][a-f0-9]*\)[[:space:]]*\(.*\)\),${action} ${hash} ${subject},p" < "$1".tmp >> "$1"
> +			action=\\2;;
> +		esac
> +	done
> +	echo 'rebase -i script after editing:'
> +	cat "$1"
> +	EOF
> +
> +	test_set_editor "$(pwd)/fake-editor.sh"
> +}
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 688b01e3eb6..4c27d5f1c91 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -951,6 +951,17 @@ test_expect_success 'rebase -ix with --autosquash' '
>   	test_cmp expected actual
>   '
>   
> +test_expect_success 'rebase -i with --allow-inline-reword' '
> +	git checkout -b reword-inline primary &&
> +	(
> +		set_inline_reword_editor &&
> +		FAKE_LINES="reword_Bchanged 1 2 reword_Dchanged 3 4" \
> +			git rebase --allow-inline-reword -i HEAD~4
> +	) &&
> +	test Bchanged = $(git show -s --format=%s HEAD~3) &&
> +	test Dchanged = $(git show -s --format=%s HEAD~)
> +'
> +
>   test_expect_success 'rebase --exec works without -i ' '
>   	git reset --hard execute &&
>   	rm -rf exec_output &&
> 
> base-commit: 350dc9f0e8974b6fcbdeb3808186c5a79c3e7386

Re: [PATCH] rebase: introduce allow-inline-reword option

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 02 2022, Théo MAILLART via GitGitGadget wrote:

> From: =?UTF-8?q?Th=C3=A9o=20Maillart?= <tmaillart@gmail.com>
>
> This new option (false by default) for interactive rebase allows users
> to modify the subject of a commit directly in the todo list, when they
> select the "reword" action.
> If the option is enabled, "reword" is selected and the subject has not
> changed, then the default behaviour is used.
> It also introduces a test for this specific option, and a related
> function (set_inline_reword_editor) in the lib-rebase.sh to use a
> simpler custom fake editor to be able to modify the message part of the
> lines in a todo list (in the most simple cases).

I won't repeat what others have noted (e.g. reference to earlier
discussions).

Just an observation: When I use "git rebase -i" I use Emacs's
"git-rebase-mode.el" to view the resulting buffer, it has useful
shortcuts. E.g. M-p and M-n move a line up/down, "c" is pick", "r" is
reword etc.

Now that mode can mark the whole buffer read-only, there is a way to
insert into it, e.g. with "y" to insert a <rev>, or "x" to insert
"exec", but those bring up an interactive dialog.

But there's nothing that e.g. would warrant search/replacing the whole
buffer now.

It's no big deal for an Emacs mode to add it I'd think, just worth
considering what we're pushing to downstream UX's.

>     If the user only wants to modify the subject of a commit during
>     interactive rebase, now he can choose the "reword" command and change
>     the subject directly in the todo list. If "reword" is selected and the
>     subject has not changed, then we use the default behavior. Here is a
>     demo https://asciinema.org/a/T9tEmUjjl4dyDuaXalPzXNzo1
>     
>     This is probably not the correct way to implement this new feature, or
>     perhaps it is not desirable, so I would like some feedback. Thank you

Wouldn't another implementation of this be to do:

	exec EDITOR="..." git commit --amend

Where that "editor" is a sed one-liner to s/<first line>/replacement/?

I don't mind this feature per-se, but I will observe that since I
started using git-rebase-mode.el its keymap has gotten much more
cluttered over the years, as we've added more special modes (and it has
maps of its own).

I don't object to it, but wonder if we couldn't add something that's
more generic, and wouldn't require e.g. a
--allow-adding-a-new-paragraph-after-the-subject-that-is-already-there,
i.e. some users will want to reword the subject, others want to add a
signed-off-by.

For the SOB case we already have:

	git rebase -i -x 'git commit --no-edit --amend -s'

Perhaps we could/should have instead:

	git commit --amend-subject

Which would allow you to do:

	git rebase -i -x 'git commit --amend-subject -m"new subject here, does not touch anything past the first \n\n"'