[PATCH] rebase -i: allow a comment after a "break" command

[PATCH] rebase -i: allow a comment after a "break" command

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When adding a "break" command to a rebase todo list it can be helpful to
add a comment as a reminder as to what the user was planning to do when
the rebase stopped. Anything following the command is interpreted as an
argument to the command and results in an error. Change this so that a
"break command may be followed by "# <comment>" in the same way as
a "merge" command. Requiring the comment to begin with "# " allows the
break command to start taking an argument in the future if that turns
out to be useful.

Reported-by: Olliver Schinagl <oliver@schinagl.nl>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
    rebase -i: allow a comment after a "break" command
    
    I'm open to suggestions for other ways to handle comments but copying
    what we do to separate merge parents from the merge commit subject
    seemed simplest.
    
    Should this print the comment when stopping for a break command?

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1460%2Fphillipwood%2Fsequencer-allow-comment-after-break-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1460/phillipwood/sequencer-allow-comment-after-break-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1460

 Documentation/git-rebase.txt |  4 +++-
 sequencer.c                  |  7 +++++--
 t/lib-rebase.sh              |  2 +-
 t/t3418-rebase-continue.sh   | 16 ++++++++++++++++
 4 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f9675bd24e6..511ace43db0 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -869,7 +869,9 @@ the files and/or the commit message, amend the commit, and continue
 rebasing.
 
 To interrupt the rebase (just like an "edit" command would do, but without
-cherry-picking any commit first), use the "break" command.
+cherry-picking any commit first), use the "break" command. A "break"
+command may be followed by a comment beginning with `#` followed by a
+space.
 
 If you just want to edit the commit message for a commit, replace the
 command "pick" with the command "reword".
diff --git a/sequencer.c b/sequencer.c
index bcb662e23be..c66f382dfbc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2509,7 +2509,9 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 	padding = strspn(bol, " \t");
 	bol += padding;
 
-	if (item->command == TODO_NOOP || item->command == TODO_BREAK) {
+	if (item->command == TODO_NOOP ||
+	    (item->command == TODO_BREAK &&
+	     (bol[0] != '#' || (bol[1] && !isspace (bol[1]))))) {
 		if (bol != eol)
 			return error(_("%s does not accept arguments: '%s'"),
 				     command_to_string(item->command), bol);
@@ -2524,7 +2526,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 			     command_to_string(item->command));
 
 	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
-	    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
+	    item->command == TODO_RESET || item->command == TODO_UPDATE_REF ||
+	    item->command == TODO_BREAK) {
 		item->commit = NULL;
 		item->arg_offset = bol - buf;
 		item->arg_len = (int)(eol - bol);
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index b57541356bd..a648013f299 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -51,7 +51,7 @@ set_fake_editor () {
 		case $line in
 		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
 			action="$line";;
-		exec_*|x_*|break|b)
+		exec_*|x_*|break_*|b_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
 		merge_*|fixup_*)
 			action=$(echo "$line" | sed 's/_/ /g');;
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 130e2f9b553..18d82869b38 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -266,6 +266,22 @@ test_expect_success 'the todo command "break" works' '
 	test_path_is_file execed
 '
 
+test_expect_success 'the todo command "break" accepts a comment' '
+	rm -f execed &&
+	test_write_lines "break # comment" "break #" "exec >execed" >expect &&
+	write_script cat-todo.sh <<-\EOS &&
+	GIT_SEQUENCE_EDITOR="grep ^\[^#\]" git rebase --edit-todo >actual
+	EOS
+	FAKE_LINES="exec_./cat-todo.sh break_#_comment b_# exec_>execed" \
+		git rebase -i HEAD &&
+	test_cmp expect actual &&
+	test_path_is_missing execed &&
+	git rebase --continue &&
+	test_path_is_missing execed &&
+	git rebase --continue &&
+	test_path_is_file execed
+'
+
 test_expect_success '--reschedule-failed-exec' '
 	test_when_finished "git rebase --abort" &&
 	test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&

base-commit: 8a4e8f6a67e7fc97048d4666eec38399b88e0e3b
-- 
gitgitgadget

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Andrei Rybak]

On 12/01/2023 11:36, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> When adding a "break" command to a rebase todo list it can be helpful to
> add a comment as a reminder as to what the user was planning to do when
> the rebase stopped. Anything following the command is interpreted as an
> argument to the command and results in an error. Change this so that a
> "break command may be followed by "# <comment>" in the same way as
> a "merge" command. Requiring the comment to begin with "# " allows the
> break command to start taking an argument in the future if that turns
> out to be useful.
> 
> Reported-by: Olliver Schinagl <oliver@schinagl.nl>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>      rebase -i: allow a comment after a "break" command
>      
>      I'm open to suggestions for other ways to handle comments but copying
>      what we do to separate merge parents from the merge commit subject
>      seemed simplest.
>      
>      Should this print the comment when stopping for a break command?

Technically, the user can look up the command via `git status`, but it
would make sense to just give the user this information directly,
similar to how exec command prints "Executing: ..." in addition to the
existing break command's message "Stopped at ...".

> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1460%2Fphillipwood%2Fsequencer-allow-comment-after-break-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1460/phillipwood/sequencer-allow-comment-after-break-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1460
> 
>   Documentation/git-rebase.txt |  4 +++-
>   sequencer.c                  |  7 +++++--
>   t/lib-rebase.sh              |  2 +-
>   t/t3418-rebase-continue.sh   | 16 ++++++++++++++++
>   4 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index f9675bd24e6..511ace43db0 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -869,7 +869,9 @@ the files and/or the commit message, amend the commit, and continue
>   rebasing.
>   
>   To interrupt the rebase (just like an "edit" command would do, but without
> -cherry-picking any commit first), use the "break" command.
> +cherry-picking any commit first), use the "break" command. A "break"
> +command may be followed by a comment beginning with `#` followed by a
> +space.

A corresponding update to append_todo_help in rebase-interactive.c
would be helpful.

>   
>   If you just want to edit the commit message for a commit, replace the
>   command "pick" with the command "reword".

[...]

> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 130e2f9b553..18d82869b38 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -266,6 +266,22 @@ test_expect_success 'the todo command "break" works' '
>   	test_path_is_file execed
>   '
>   
> +test_expect_success 'the todo command "break" accepts a comment' '
> +	rm -f execed &&
> +	test_write_lines "break # comment" "break #" "exec >execed" >expect &&
> +	write_script cat-todo.sh <<-\EOS &&
> +	GIT_SEQUENCE_EDITOR="grep ^\[^#\]" git rebase --edit-todo >actual
> +	EOS
> +	FAKE_LINES="exec_./cat-todo.sh break_#_comment b_# exec_>execed" \

It seems that helper set_cat_todo_editor could be used here, except that
tests in t3418-rebase-continue.sh use a global set_fake_editor at the
very top of the file, unlike tests in t3404-rebase-interactive.sh which
call set_fake_editor individually.  See also related commits 6a619ca03c
(t3404: remove uneeded calls to set_fake_editor, 2019-10-15) and
b2dbacbddf (t3404: set $EDITOR in subshell, 2019-10-15).

> +		git rebase -i HEAD &&
> +	test_cmp expect actual &&
> +	test_path_is_missing execed &&
> +	git rebase --continue &&
> +	test_path_is_missing execed &&
> +	git rebase --continue &&
> +	test_path_is_file execed
> +'
> +
>   test_expect_success '--reschedule-failed-exec' '
>   	test_when_finished "git rebase --abort" &&
>   	test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
> 
> base-commit: 8a4e8f6a67e7fc97048d4666eec38399b88e0e3b

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Phillip Wood]

Hi Andrei

Thanks for taking a look at this

On 12/01/2023 11:14, Andrei Rybak wrote:
> On 12/01/2023 11:36, Phillip Wood via GitGitGadget wrote:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> When adding a "break" command to a rebase todo list it can be helpful to
>> add a comment as a reminder as to what the user was planning to do when
>> the rebase stopped. Anything following the command is interpreted as an
>> argument to the command and results in an error. Change this so that a
>> "break command may be followed by "# <comment>" in the same way as
>> a "merge" command. Requiring the comment to begin with "# " allows the
>> break command to start taking an argument in the future if that turns
>> out to be useful.
>>
>> Reported-by: Olliver Schinagl <oliver@schinagl.nl>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>      rebase -i: allow a comment after a "break" command
>>      I'm open to suggestions for other ways to handle comments but 
>> copying
>>      what we do to separate merge parents from the merge commit subject
>>      seemed simplest.
>>      Should this print the comment when stopping for a break command?
> 
> Technically, the user can look up the command via `git status`, but it
> would make sense to just give the user this information directly,
> similar to how exec command prints "Executing: ..." in addition to the
> existing break command's message "Stopped at ...".

Yes I think that is probable a good idea.

>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index f9675bd24e6..511ace43db0 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -869,7 +869,9 @@ the files and/or the commit message, amend the 
>> commit, and continue
>>   rebasing.
>>   To interrupt the rebase (just like an "edit" command would do, but 
>> without
>> -cherry-picking any commit first), use the "break" command.
>> +cherry-picking any commit first), use the "break" command. A "break"
>> +command may be followed by a comment beginning with `#` followed by a
>> +space.
> 
> A corresponding update to append_todo_help in rebase-interactive.c
> would be helpful.

Thanks that's a good suggestion.

>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
>> index 130e2f9b553..18d82869b38 100755
>> --- a/t/t3418-rebase-continue.sh
>> +++ b/t/t3418-rebase-continue.sh
>> @@ -266,6 +266,22 @@ test_expect_success 'the todo command "break" 
>> works' '
>>       test_path_is_file execed
>>   '
>> +test_expect_success 'the todo command "break" accepts a comment' '
>> +    rm -f execed &&
>> +    test_write_lines "break # comment" "break #" "exec >execed" 
>> >expect &&
>> +    write_script cat-todo.sh <<-\EOS &&
>> +    GIT_SEQUENCE_EDITOR="grep ^\[^#\]" git rebase --edit-todo >actual
>> +    EOS
>> +    FAKE_LINES="exec_./cat-todo.sh break_#_comment b_# exec_>execed" \
> 
> It seems that helper set_cat_todo_editor could be used here, except that
> tests in t3418-rebase-continue.sh use a global set_fake_editor at the
> very top of the file, unlike tests in t3404-rebase-interactive.sh which
> call set_fake_editor individually.  See also related commits 6a619ca03c
> (t3404: remove uneeded calls to set_fake_editor, 2019-10-15) and
> b2dbacbddf (t3404: set $EDITOR in subshell, 2019-10-15).

I did look at using set_cat_todo_editor but it isn't that simple. It 
cannot be used with set_fake_editor so I'd need to find another way to 
supply the todo list and it always exits with a failure.

As well as checking that "break" accepts a comment this test also checks 
that the comment is still present when the user re-edits the todo list 
which is tricky to do with set_cat_todo_editor.

Best Wishes

Phillip

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Olliver Schinagl]

Hey Phillip,

Thanks for looking into this! Much appreciated!

> @@ -869,7 +869,9 @@ the files and/or the commit message, amend the 
> commit, and continue   rebasing.
>   
>   To interrupt the rebase (just like an "edit" command would do, but without
> -cherry-picking any commit first), use the "break" command. 
> +cherry-picking any commit first), use the "break" command. A "break" 
> +command may be followed by a comment beginning with `#` followed by a 
> +space.
Any technical reason for this? Traditionally, anything goes after a 
comment marker, so why be exceptional here?


Thanks,

Olliver

>   
>   If you just want to edit the commit message for a commit, replace the
>   command "pick" with the command "reword".
> diff 
> <https://lore.kernel.org/git/pull.1460.git.1673519809510.gitgitgadget@gmail.com/#iZ31sequencer.c> 
> --git a/sequencer.c b/sequencer.c index bcb662e23be..c66f382dfbc 
> 100644 --- a/sequencer.c +++ b/sequencer.c

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 12 2023, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When adding a "break" command to a rebase todo list it can be helpful to
> add a comment as a reminder as to what the user was planning to do when
> the rebase stopped. Anything following the command is interpreted as an
> argument to the command and results in an error. Change this so that a
> "break command may be followed by "# <comment>" in the same way as
> a "merge" command. Requiring the comment to begin with "# " allows the
> break command to start taking an argument in the future if that turns
> out to be useful.
>
> Reported-by: Olliver Schinagl <oliver@schinagl.nl>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>     rebase -i: allow a comment after a "break" command
>     
>     I'm open to suggestions for other ways to handle comments but copying
>     what we do to separate merge parents from the merge commit subject
>     seemed simplest.
>     
>     Should this print the comment when stopping for a break command?
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1460%2Fphillipwood%2Fsequencer-allow-comment-after-break-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1460/phillipwood/sequencer-allow-comment-after-break-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1460
>
>  Documentation/git-rebase.txt |  4 +++-
>  sequencer.c                  |  7 +++++--
>  t/lib-rebase.sh              |  2 +-
>  t/t3418-rebase-continue.sh   | 16 ++++++++++++++++
>  4 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index f9675bd24e6..511ace43db0 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -869,7 +869,9 @@ the files and/or the commit message, amend the commit, and continue
>  rebasing.
>  
>  To interrupt the rebase (just like an "edit" command would do, but without
> -cherry-picking any commit first), use the "break" command.
> +cherry-picking any commit first), use the "break" command. A "break"
> +command may be followed by a comment beginning with `#` followed by a
> +space.

You're missing a corresponding edit here to the help string in
append_todo_help(), as you note you're making "break" support what
"merge" does, and that help string documents that "merge" accepts a
comment, after this we don't do that for "break", but should one way or
the other (see below).

> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2509,7 +2509,9 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>  	padding = strspn(bol, " \t");
>  	bol += padding;
>  
> -	if (item->command == TODO_NOOP || item->command == TODO_BREAK) {
> +	if (item->command == TODO_NOOP ||
> +	    (item->command == TODO_BREAK &&
> +	     (bol[0] != '#' || (bol[1] && !isspace (bol[1]))))) {
>  		if (bol != eol)
>  			return error(_("%s does not accept arguments: '%s'"),
>  				     command_to_string(item->command), bol);
> @@ -2524,7 +2526,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>  			     command_to_string(item->command));
>  
>  	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
> -	    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
> +	    item->command == TODO_RESET || item->command == TODO_UPDATE_REF ||
> +	    item->command == TODO_BREAK) {
>  		item->commit = NULL;
>  		item->arg_offset = bol - buf;
>  		item->arg_len = (int)(eol - bol);
> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index b57541356bd..a648013f299 100644
> --- a/t/lib-rebase.sh
> +++ b/t/lib-rebase.sh
> @@ -51,7 +51,7 @@ set_fake_editor () {
>  		case $line in
>  		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
>  			action="$line";;
> -		exec_*|x_*|break|b)
> +		exec_*|x_*|break_*|b_*|break|b)
>  			echo "$line" | sed 's/_/ /g' >> "$1";;
>  		merge_*|fixup_*)
>  			action=$(echo "$line" | sed 's/_/ /g');;
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 130e2f9b553..18d82869b38 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -266,6 +266,22 @@ test_expect_success 'the todo command "break" works' '
>  	test_path_is_file execed
>  '
>  
> +test_expect_success 'the todo command "break" accepts a comment' '
> +	rm -f execed &&
> +	test_write_lines "break # comment" "break #" "exec >execed" >expect &&
> +	write_script cat-todo.sh <<-\EOS &&
> +	GIT_SEQUENCE_EDITOR="grep ^\[^#\]" git rebase --edit-todo >actual
> +	EOS
> +	FAKE_LINES="exec_./cat-todo.sh break_#_comment b_# exec_>execed" \
> +		git rebase -i HEAD &&
> +	test_cmp expect actual &&
> +	test_path_is_missing execed &&
> +	git rebase --continue &&
> +	test_path_is_missing execed &&
> +	git rebase --continue &&
> +	test_path_is_file execed
> +'
> +
>  test_expect_success '--reschedule-failed-exec' '
>  	test_when_finished "git rebase --abort" &&
>  	test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
>
> base-commit: 8a4e8f6a67e7fc97048d4666eec38399b88e0e3b

I like this direction, but I don't see why we need to continue this
special-snowflakeness of only allowing specific commands to accept these
#-comments.

Why not just have them all support it? It started with "merge", which as
4c68e7ddb59 (sequencer: introduce the `merge` command, 2018-04-25) note
can be used for:

	merge -C baaabaaa abc # Merge the branch 'abc' into master

As Olliver points out we should probably support "#" without the
following " ", which seems to be an accident of history &
over-strictness.

But in this commit you extend it to "break", but we're going out of or
way to e.g. extend this to "noop".

So I'd expect that just like the first for-loop in "parse_insn_line()"
we'd check if strchr(bol, '#') returns non-NULL, and if so set eol to
that result.

The "just like" being that we may want to explicitly forbid this or
handle it specially for some, e.g. I didn't check but do the "label" and
"reset" perhaps support arbitrary non-'\n' (probably by accident, and we
could support commments there too).

For "pick" we probably need to explicitly exclude it, although I can't
remember if we do anything useful with the part of the line after the
OID (maybe not...).

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Olliver Schinagl]

On 12-01-2023 13:25, Ævar Arnfjörð Bjarmason wrote:
> On Thu, Jan 12 2023, Phillip Wood via GitGitGadget wrote:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> When adding a "break" command to a rebase todo list it can be helpful to
>> add a comment as a reminder as to what the user was planning to do when
>> the rebase stopped. Anything following the command is interpreted as an
>> argument to the command and results in an error. Change this so that a
>> "break command may be followed by "# <comment>" in the same way as
>> a "merge" command. Requiring the comment to begin with "# " allows the
>> break command to start taking an argument in the future if that turns
>> out to be useful.
>>
>> Reported-by: Olliver Schinagl <oliver@schinagl.nl>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>      rebase -i: allow a comment after a "break" command
>>      
>>      I'm open to suggestions for other ways to handle comments but copying
>>      what we do to separate merge parents from the merge commit subject
>>      seemed simplest.
>>      
>>      Should this print the comment when stopping for a break command?
>>
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1460%2Fphillipwood%2Fsequencer-allow-comment-after-break-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1460/phillipwood/sequencer-allow-comment-after-break-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1460
>>
>>   Documentation/git-rebase.txt |  4 +++-
>>   sequencer.c                  |  7 +++++--
>>   t/lib-rebase.sh              |  2 +-
>>   t/t3418-rebase-continue.sh   | 16 ++++++++++++++++
>>   4 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index f9675bd24e6..511ace43db0 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -869,7 +869,9 @@ the files and/or the commit message, amend the commit, and continue
>>   rebasing.
>>   
>>   To interrupt the rebase (just like an "edit" command would do, but without
>> -cherry-picking any commit first), use the "break" command.
>> +cherry-picking any commit first), use the "break" command. A "break"
>> +command may be followed by a comment beginning with `#` followed by a
>> +space.
> You're missing a corresponding edit here to the help string in
> append_todo_help(), as you note you're making "break" support what
> "merge" does, and that help string documents that "merge" accepts a
> comment, after this we don't do that for "break", but should one way or
> the other (see below).
>
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -2509,7 +2509,9 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>>   	padding = strspn(bol, " \t");
>>   	bol += padding;
>>   
>> -	if (item->command == TODO_NOOP || item->command == TODO_BREAK) {
>> +	if (item->command == TODO_NOOP ||
>> +	    (item->command == TODO_BREAK &&
>> +	     (bol[0] != '#' || (bol[1] && !isspace (bol[1]))))) {
>>   		if (bol != eol)
>>   			return error(_("%s does not accept arguments: '%s'"),
>>   				     command_to_string(item->command), bol);
>> @@ -2524,7 +2526,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>>   			     command_to_string(item->command));
>>   
>>   	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
>> -	    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
>> +	    item->command == TODO_RESET || item->command == TODO_UPDATE_REF ||
>> +	    item->command == TODO_BREAK) {
>>   		item->commit = NULL;
>>   		item->arg_offset = bol - buf;
>>   		item->arg_len = (int)(eol - bol);
>> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
>> index b57541356bd..a648013f299 100644
>> --- a/t/lib-rebase.sh
>> +++ b/t/lib-rebase.sh
>> @@ -51,7 +51,7 @@ set_fake_editor () {
>>   		case $line in
>>   		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
>>   			action="$line";;
>> -		exec_*|x_*|break|b)
>> +		exec_*|x_*|break_*|b_*|break|b)
>>   			echo "$line" | sed 's/_/ /g' >> "$1";;
>>   		merge_*|fixup_*)
>>   			action=$(echo "$line" | sed 's/_/ /g');;
>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
>> index 130e2f9b553..18d82869b38 100755
>> --- a/t/t3418-rebase-continue.sh
>> +++ b/t/t3418-rebase-continue.sh
>> @@ -266,6 +266,22 @@ test_expect_success 'the todo command "break" works' '
>>   	test_path_is_file execed
>>   '
>>   
>> +test_expect_success 'the todo command "break" accepts a comment' '
>> +	rm -f execed &&
>> +	test_write_lines "break # comment" "break #" "exec >execed" >expect &&
>> +	write_script cat-todo.sh <<-\EOS &&
>> +	GIT_SEQUENCE_EDITOR="grep ^\[^#\]" git rebase --edit-todo >actual
>> +	EOS
>> +	FAKE_LINES="exec_./cat-todo.sh break_#_comment b_# exec_>execed" \
>> +		git rebase -i HEAD &&
>> +	test_cmp expect actual &&
>> +	test_path_is_missing execed &&
>> +	git rebase --continue &&
>> +	test_path_is_missing execed &&
>> +	git rebase --continue &&
>> +	test_path_is_file execed
>> +'
>> +
>>   test_expect_success '--reschedule-failed-exec' '
>>   	test_when_finished "git rebase --abort" &&
>>   	test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
>>
>> base-commit: 8a4e8f6a67e7fc97048d4666eec38399b88e0e3b
> I like this direction, but I don't see why we need to continue this
> special-snowflakeness of only allowing specific commands to accept these
> #-comments.

Oh, yes! That please :) I often want to put comments during a rebase.

Also retrieving it becomes an issue at that point though. E.g. print it 
during status/etc. E.g. if I do 'edit' on a rebase and I add a comment, 
for example:

'edit/reword <hash> [original title]#Fix typo in title/commit message'

I obviouslly also need to be reminded. e.g. what about commands that pop 
up the editor to 'amend' the commit message (as rebase --continue would 
do for an edit) also add the comment (using git commitChar) in the 
commit message. Kind of like how squash/fixup do "this is a commit based 
on two ..." with comments splattered around. (also kind of like git 
fixup -m <msg> does without a rebase?).

Feature creep, sorry though :)

>
> Why not just have them all support it? It started with "merge", which as
> 4c68e7ddb59 (sequencer: introduce the `merge` command, 2018-04-25) note
> can be used for:
>
> 	merge -C baaabaaa abc # Merge the branch 'abc' into master
>
> As Olliver points out we should probably support "#" without the
> following " ", which seems to be an accident of history &
> over-strictness.

:+1:


Olliver

>
> But in this commit you extend it to "break", but we're going out of or
> way to e.g. extend this to "noop".
>
> So I'd expect that just like the first for-loop in "parse_insn_line()"
> we'd check if strchr(bol, '#') returns non-NULL, and if so set eol to
> that result.
>
> The "just like" being that we may want to explicitly forbid this or
> handle it specially for some, e.g. I didn't check but do the "label" and
> "reset" perhaps support arbitrary non-'\n' (probably by accident, and we
> could support commments there too).
>
> For "pick" we probably need to explicitly exclude it, although I can't
> remember if we do anything useful with the part of the line after the
> OID (maybe not...).

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Phillip Wood]

On 12/01/2023 12:25, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Jan 12 2023, Phillip Wood via GitGitGadget wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index f9675bd24e6..511ace43db0 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -869,7 +869,9 @@ the files and/or the commit message, amend the commit, and continue
>>   rebasing.
>>   
>>   To interrupt the rebase (just like an "edit" command would do, but without
>> -cherry-picking any commit first), use the "break" command.
>> +cherry-picking any commit first), use the "break" command. A "break"
>> +command may be followed by a comment beginning with `#` followed by a
>> +space.
> 
> You're missing a corresponding edit here to the help string in
> append_todo_help(), as you note you're making "break" support what
> "merge" does, and that help string documents that "merge" accepts a
> comment, after this we don't do that for "break", but should one way or
> the other (see below).

Thanks, Andrei has already mentioned that, I'll update the todo help 
when I re-roll
> I like this direction, but I don't see why we need to continue this
> special-snowflakeness of only allowing specific commands to accept these
> #-comments.
> 
> Why not just have them all support it? It started with "merge", which as
> 4c68e7ddb59 (sequencer: introduce the `merge` command, 2018-04-25) note
> can be used for:
> 
> 	merge -C baaabaaa abc # Merge the branch 'abc' into master
> 
> As Olliver points out we should probably support "#" without the
> following " ", which seems to be an accident of history &
> over-strictness.

It's not an accident, labels and commit names can begin with '#' so we 
need to support

	merge #parent1 #parent2

For "break" we could just not require '#' at all as we do for "reset 
<label>" where anything following the label is ignored. That would mean 
we couldn't add an argument to break in the future though (I'm not sure 
that is really a problem in practice). If we're going to require '#' 
then we may as well following the existing rules.

> But in this commit you extend it to "break", but we're going out of or
> way to e.g. extend this to "noop".

I'm struggling to see why "noop" would need a comment - it only exists 
to avoid an empty todo list and is not meant for general use (it's not 
in the help added by append_todo_help() for this reason)

For "pick", "edit", "reword", "fixup" & "squash" we don't need a comment 
mechanism as we ignore everything after the commit name. For "reset" we 
ignore everything after the label. For "label" we could add support for 
comments but I'm not sure it is that useful and we'd need to be careful 
not to interpret a bad label name as a label + comment.

> So I'd expect that just like the first for-loop in "parse_insn_line()"
> we'd check if strchr(bol, '#') returns non-NULL, and if so set eol to
> that result.

That would break labels and commit names that contain a '#'

If we think we're never going to want "break" to take an argument then 
maybe we should just make it ignore the rest of the line like "reset 
<label>".

Best Wishes

Phillip

> The "just like" being that we may want to explicitly forbid this or
> handle it specially for some, e.g. I didn't check but do the "label" and
> "reset" perhaps support arbitrary non-'\n' (probably by accident, and we
> could support commments there too).
> 
> For "pick" we probably need to explicitly exclude it, although I can't
> remember if we do anything useful with the part of the line after the
> OID (maybe not...).

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 12 2023, Phillip Wood wrote:

> On 12/01/2023 12:25, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Jan 12 2023, Phillip Wood via GitGitGadget wrote:
>> 
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>>> index f9675bd24e6..511ace43db0 100644
>>> --- a/Documentation/git-rebase.txt
>>> +++ b/Documentation/git-rebase.txt
>>> @@ -869,7 +869,9 @@ the files and/or the commit message, amend the commit, and continue
>>>   rebasing.
>>>     To interrupt the rebase (just like an "edit" command would do,
>>> but without
>>> -cherry-picking any commit first), use the "break" command.
>>> +cherry-picking any commit first), use the "break" command. A "break"
>>> +command may be followed by a comment beginning with `#` followed by a
>>> +space.
>> You're missing a corresponding edit here to the help string in
>> append_todo_help(), as you note you're making "break" support what
>> "merge" does, and that help string documents that "merge" accepts a
>> comment, after this we don't do that for "break", but should one way or
>> the other (see below).
>
> Thanks, Andrei has already mentioned that, I'll update the todo help
> when I re-roll
>> I like this direction, but I don't see why we need to continue this
>> special-snowflakeness of only allowing specific commands to accept these
>> #-comments.
>> Why not just have them all support it? It started with "merge",
>> which as
>> 4c68e7ddb59 (sequencer: introduce the `merge` command, 2018-04-25) note
>> can be used for:
>> 	merge -C baaabaaa abc # Merge the branch 'abc' into master
>> As Olliver points out we should probably support "#" without the
>> following " ", which seems to be an accident of history &
>> over-strictness.
>
> It's not an accident, labels and commit names can begin with '#' so we
> need to support
>
> 	merge #parent1 #parent2

Ah, I would have told you that '#' was forbidden in refnames, but as
some trivial experimentation shows I was wrong about that. So yes, we
need the "# ".

> For "break" we could just not require '#' at all as we do for "reset
> <label>" where anything following the label is ignored. That would
> mean we couldn't add an argument to break in the future though (I'm
> not sure that is really a problem in practice). If we're going to
> require '#' then we may as well following the existing rules.

I'd think we'd want to parse past the "break" to find a "#", and error
out unknown stuff still, exactly to support future extensions.

I.e. we'd like to close the door on "break# foo", "break # foo" etc, but
not "break foo", unless I'm misunderstanding you here...

>> But in this commit you extend it to "break", but we're going out of or
>> way to e.g. extend this to "noop".
>
> I'm struggling to see why "noop" would need a comment - it only exists
> to avoid an empty todo list and is not meant for general use (it's not
> in the help added by append_todo_help() for this reason)

I'm struggling to see why "break" needs a comment, why not just add it
to the preceding line or something? But it seems some users like it :)

So at that point, it seems easier to both explain & implement something
that just consistently supports comment syntax, rather than overly
special-casing it.

> For "pick", "edit", "reword", "fixup" & "squash" we don't need a
> comment mechanism as we ignore everything after the commit name. For
> "reset" we ignore everything after the label. For "label" we could add
> support for comments but I'm not sure it is that useful and we'd need
> to be careful not to interpret a bad label name as a label + comment.

I think there's been a couple of request to have changing the "argument"
actually reword the $subject (I'm pretty such for "reword" that got as
far as a patch, but I may be misrecalling).

Of course that's an argument against what I was suggesting of making the
comment support a bit more general (although we could probably still
support it for "noop" or whatever).

>> So I'd expect that just like the first for-loop in "parse_insn_line()"
>> we'd check if strchr(bol, '#') returns non-NULL, and if so set eol to
>> that result.
>
> That would break labels and commit names that contain a '#'
>
> If we think we're never going to want "break" to take an argument then
> maybe we should just make it ignore the rest of the line like "reset
> <label>".

It's unfortunate that we do that, I think it's almost always better to
just error out rather that silently ignore data, except for some
explicit exceptions (such as comment syntax).

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Elijah Newren]

On Thu, Jan 12, 2023 at 9:20 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Thu, Jan 12 2023, Phillip Wood wrote:
>
> > On 12/01/2023 12:25, Ævar Arnfjörð Bjarmason wrote:
> >> On Thu, Jan 12 2023, Phillip Wood via GitGitGadget wrote:
> >>
[...]
> >> But in this commit you extend it to "break", but we're going out of or
> >> way to e.g. extend this to "noop".
> >
> > I'm struggling to see why "noop" would need a comment - it only exists
> > to avoid an empty todo list and is not meant for general use (it's not
> > in the help added by append_todo_help() for this reason)
>
> I'm struggling to see why "break" needs a comment, why not just add it
> to the preceding line or something? But it seems some users like it :)
>
> So at that point, it seems easier to both explain & implement something
> that just consistently supports comment syntax, rather than overly
> special-casing it.

Personally, I think we should allow all lines in the todo script to
have trailing comments.

> > For "pick", "edit", "reword", "fixup" & "squash" we don't need a
> > comment mechanism as we ignore everything after the commit name. For
> > "reset" we ignore everything after the label. For "label" we could add
> > support for comments but I'm not sure it is that useful and we'd need
> > to be careful not to interpret a bad label name as a label + comment.
>
> I think there's been a couple of request to have changing the "argument"
> actually reword the $subject (I'm pretty such for "reword" that got as
> far as a patch, but I may be misrecalling).

Yes, there have been, but it's a bad idea (and it's not just me saying
that; Junio has also declared it as such).  I think it's mostly based
on confusion from us having ignored non-commented stuff on a line, and
we could avoid that confusion by just commenting out the commit
subject to make it clear we will ignore it and any changes to it.

> >> So I'd expect that just like the first for-loop in "parse_insn_line()"
> >> we'd check if strchr(bol, '#') returns non-NULL, and if so set eol to
> >> that result.
> >
> > That would break labels and commit names that contain a '#'
> >
> > If we think we're never going to want "break" to take an argument then
> > maybe we should just make it ignore the rest of the line like "reset
> > <label>".
>
> It's unfortunate that we do that, I think it's almost always better to
> just error out rather that silently ignore data, except for some
> explicit exceptions (such as comment syntax).

I agree.

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Elijah Newren]

On Thu, Jan 12, 2023 at 5:28 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Thu, Jan 12 2023, Phillip Wood via GitGitGadget wrote:
>
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > When adding a "break" command to a rebase todo list it can be helpful to
> > add a comment as a reminder as to what the user was planning to do when
> > the rebase stopped. Anything following the command is interpreted as an
> > argument to the command and results in an error. Change this so that a
> > "break command may be followed by "# <comment>" in the same way as
> > a "merge" command. Requiring the comment to begin with "# " allows the
> > break command to start taking an argument in the future if that turns
> > out to be useful.
> >
> > Reported-by: Olliver Schinagl <oliver@schinagl.nl>
> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> > ---
> >     rebase -i: allow a comment after a "break" command
> >
> >     I'm open to suggestions for other ways to handle comments but copying
> >     what we do to separate merge parents from the merge commit subject
> >     seemed simplest.
> >
> >     Should this print the comment when stopping for a break command?
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1460%2Fphillipwood%2Fsequencer-allow-comment-after-break-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1460/phillipwood/sequencer-allow-comment-after-break-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1460
> >
> >  Documentation/git-rebase.txt |  4 +++-
> >  sequencer.c                  |  7 +++++--
> >  t/lib-rebase.sh              |  2 +-
> >  t/t3418-rebase-continue.sh   | 16 ++++++++++++++++
> >  4 files changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> > index f9675bd24e6..511ace43db0 100644
> > --- a/Documentation/git-rebase.txt
> > +++ b/Documentation/git-rebase.txt
> > @@ -869,7 +869,9 @@ the files and/or the commit message, amend the commit, and continue
> >  rebasing.
> >
> >  To interrupt the rebase (just like an "edit" command would do, but without
> > -cherry-picking any commit first), use the "break" command.
> > +cherry-picking any commit first), use the "break" command. A "break"
> > +command may be followed by a comment beginning with `#` followed by a
> > +space.
>
> You're missing a corresponding edit here to the help string in
> append_todo_help(), as you note you're making "break" support what
> "merge" does, and that help string documents that "merge" accepts a
> comment, after this we don't do that for "break", but should one way or
> the other (see below).
>
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2509,7 +2509,9 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
> >       padding = strspn(bol, " \t");
> >       bol += padding;
> >
> > -     if (item->command == TODO_NOOP || item->command == TODO_BREAK) {
> > +     if (item->command == TODO_NOOP ||
> > +         (item->command == TODO_BREAK &&
> > +          (bol[0] != '#' || (bol[1] && !isspace (bol[1]))))) {
> >               if (bol != eol)
> >                       return error(_("%s does not accept arguments: '%s'"),
> >                                    command_to_string(item->command), bol);
> > @@ -2524,7 +2526,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
> >                            command_to_string(item->command));
> >
> >       if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
> > -         item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
> > +         item->command == TODO_RESET || item->command == TODO_UPDATE_REF ||
> > +         item->command == TODO_BREAK) {
> >               item->commit = NULL;
> >               item->arg_offset = bol - buf;
> >               item->arg_len = (int)(eol - bol);
> > diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> > index b57541356bd..a648013f299 100644
> > --- a/t/lib-rebase.sh
> > +++ b/t/lib-rebase.sh
> > @@ -51,7 +51,7 @@ set_fake_editor () {
> >               case $line in
> >               pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
> >                       action="$line";;
> > -             exec_*|x_*|break|b)
> > +             exec_*|x_*|break_*|b_*|break|b)
> >                       echo "$line" | sed 's/_/ /g' >> "$1";;
> >               merge_*|fixup_*)
> >                       action=$(echo "$line" | sed 's/_/ /g');;
> > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> > index 130e2f9b553..18d82869b38 100755
> > --- a/t/t3418-rebase-continue.sh
> > +++ b/t/t3418-rebase-continue.sh
> > @@ -266,6 +266,22 @@ test_expect_success 'the todo command "break" works' '
> >       test_path_is_file execed
> >  '
> >
> > +test_expect_success 'the todo command "break" accepts a comment' '
> > +     rm -f execed &&
> > +     test_write_lines "break # comment" "break #" "exec >execed" >expect &&
> > +     write_script cat-todo.sh <<-\EOS &&
> > +     GIT_SEQUENCE_EDITOR="grep ^\[^#\]" git rebase --edit-todo >actual
> > +     EOS
> > +     FAKE_LINES="exec_./cat-todo.sh break_#_comment b_# exec_>execed" \
> > +             git rebase -i HEAD &&
> > +     test_cmp expect actual &&
> > +     test_path_is_missing execed &&
> > +     git rebase --continue &&
> > +     test_path_is_missing execed &&
> > +     git rebase --continue &&
> > +     test_path_is_file execed
> > +'
> > +
> >  test_expect_success '--reschedule-failed-exec' '
> >       test_when_finished "git rebase --abort" &&
> >       test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
> >
> > base-commit: 8a4e8f6a67e7fc97048d4666eec38399b88e0e3b
>
> I like this direction, but I don't see why we need to continue this
> special-snowflakeness of only allowing specific commands to accept these
> #-comments.
[...]
> For "pick" we probably need to explicitly exclude it, although I can't
> remember if we do anything useful with the part of the line after the
> OID (maybe not...).

I disagree; pick should automatically always have comments.  In
particular, everything after the OID should be preceded by a '# '.
The fact that we don't do that confuses people into thinking the
commit summary is part of the directive or that by editing the commit
summary in the todo list, that the commit message of the picked commit
will somehow be updated to reflect such edits.  I think the fact that
even you can't remember whether we do anything useful with the part of
the line after the OID, despite being one of the most prolific git
contributors of all time, reinforces the point that no one knows what
that commit summary means and we shouldn't be surprised that users get
confused by it.  We should just comment it out to make it clear that
we will always ignore those words (and any changes thereto); the
commit summary is only there as an aid to the user.

I've been meaning to send in a patch to do precisely this
(https://github.com/newren/git/commit/f1ae608477e010b96557d6fc87eed9f3f39b905e).

Of course, the same applies to edit/squash/fixup/reword, though if I
could go back in time...(warning, long tangent coming)...I would make
it so those four directives did not accept any commit ID argument.
Only "pick" and "reset" would accept a commit ID.  Instead, today's
"edit X" would be two commands ("pick X" followed by either "break" or
"edit"), "fixup X" would be "pick X" + "fixup", and "reword X" would
be "pick X" + "reword".  That'd help users understand rebase state
much better (it's so easy for users to get confused by whether they
should be using `git commit --amend` vs. `git rebase --continue` and I
think this is partially to blame, though there's other changes we
could make to help with that dichotomy as well).  The separate
directives would also make it much easier to figure out how to both
fixup and edit a single commit in the same rebase (pick the commit,
then add a fixup directive, then an edit directive).  In fact, "squash
X" could just be discarded as superfluous, since it's just "pick X" +
"fixup" + "reword" (or we could keep squash as an abbreviation for
both "fixup" + "reword").  There's other things I'd change as well,
which are tangential to this tangent, but I'm clearly veering way off
the topic of comments in the rebase todo list, so I'll stop here.

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> Of course, the same applies to edit/squash/fixup/reword, though if I
> could go back in time...(warning, long tangent coming)...I would make
> it so those four directives did not accept any commit ID argument.
> Only "pick" and "reset" would accept a commit ID.  Instead, today's
> "edit X" would be two commands ("pick X" followed by either "break" or
> "edit"), "fixup X" would be "pick X" + "fixup", and "reword X" would
> be "pick X" + "reword".  That'd help users understand rebase state
> much better (it's so easy for users to get confused by whether they
> should be using `git commit --amend` vs. `git rebase --continue` and I
> think this is partially to blame, though there's other changes we
> could make to help with that dichotomy as well).  The separate
> directives would also make it much easier to figure out how to both
> fixup and edit a single commit in the same rebase (pick the commit,
> then add a fixup directive, then an edit directive).  

Intriguing, and I feel sad that it probably is too late for all of
the above X-<.

> In fact, "squash
> X" could just be discarded as superfluous, since it's just "pick X" +
> "fixup" + "reword" (or we could keep squash as an abbreviation for
> both "fixup" + "reword").

IIUC, your "fixup" is

	git reset --soft HEAD^
	git commit --amend --no-edit

i.e. discard the log message from "fixup" and use only its tree, and
your "reword" is

	git commit --amend --edit

so "pick X" + "fixup" + "reword" would not be quite usable as a
replacement of our "squash X" (or your "pick X" + "squash"), I am
afraid.  You'd want the log message from "X" as well as "X^" to
edit the replacement of X^.

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Elijah Newren]

On Fri, Jan 13, 2023 at 12:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > Of course, the same applies to edit/squash/fixup/reword, though if I
> > could go back in time...(warning, long tangent coming)...I would make
> > it so those four directives did not accept any commit ID argument.
> > Only "pick" and "reset" would accept a commit ID.  Instead, today's
> > "edit X" would be two commands ("pick X" followed by either "break" or
> > "edit"), "fixup X" would be "pick X" + "fixup", and "reword X" would
> > be "pick X" + "reword".  That'd help users understand rebase state
> > much better (it's so easy for users to get confused by whether they
> > should be using `git commit --amend` vs. `git rebase --continue` and I
> > think this is partially to blame, though there's other changes we
> > could make to help with that dichotomy as well).  The separate
> > directives would also make it much easier to figure out how to both
> > fixup and edit a single commit in the same rebase (pick the commit,
> > then add a fixup directive, then an edit directive).
>
> Intriguing, and I feel sad that it probably is too late for all of
> the above X-<.

Yeah, I know.  One more thing for the "if we had a time machine" list...

> > In fact, "squash
> > X" could just be discarded as superfluous, since it's just "pick X" +
> > "fixup" + "reword" (or we could keep squash as an abbreviation for
> > both "fixup" + "reword").
>
> IIUC, your "fixup" is
>
>         git reset --soft HEAD^
>         git commit --amend --no-edit
>
> i.e. discard the log message from "fixup" and use only its tree, and
> your "reword" is
>
>         git commit --amend --edit
>
> so "pick X" + "fixup" + "reword" would not be quite usable as a
> replacement of our "squash X" (or your "pick X" + "squash"), I am
> afraid.  You'd want the log message from "X" as well as "X^" to
> edit the replacement of X^.

Oh, good point.  So, in that alternate world we'd still need a squash directive.

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When adding a "break" command to a rebase todo list it can be helpful to
> add a comment as a reminder as to what the user was planning to do when
> the rebase stopped. Anything following the command is interpreted as an
> argument to the command and results in an error. Change this so that a
> "break command may be followed by "# <comment>" in the same way as
> a "merge" command. Requiring the comment to begin with "# " allows the
> break command to start taking an argument in the future if that turns
> out to be useful.

Why do we special case "break" and not give the same "comment is
emitted when the control reaches the insn in the todo list" for
others like "exec" or even "pick"?

Another comment with devil's advocate hat on is if we are better off
not adding "# this comment is emitted" at all, and instead do

    pick ...
    pick ...
    exec echo this comment is emitted
    break
    pick ...

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Phillip Wood]

Hi Junio

On 12/01/2023 15:52, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> When adding a "break" command to a rebase todo list it can be helpful to
>> add a comment as a reminder as to what the user was planning to do when
>> the rebase stopped. Anything following the command is interpreted as an
>> argument to the command and results in an error. Change this so that a
>> "break command may be followed by "# <comment>" in the same way as
>> a "merge" command. Requiring the comment to begin with "# " allows the
>> break command to start taking an argument in the future if that turns
>> out to be useful.
> 
> Why do we special case "break" and not give the same "comment is
> emitted when the control reaches the insn in the todo list" for
> others like "exec" or even "pick"?

I think the break command is a bit different to the others as it stops 
the rebase and so the user may want a reminder of what they were 
planning to do when it stopped. The other commands just pick a commit or 
run a command so I'm not sure what the comment would be for.

> Another comment with devil's advocate hat on is if we are better off
> not adding "# this comment is emitted" at all, and instead do
> 
>      pick ...
>      pick ...
>      exec echo this comment is emitted
>      break
>      pick ...

That's a neat idea, maybe we should just add some documentation 
suggesting users do that?

Best Wishes

Phillip

> 

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Jeff King]

On Fri, Jan 13, 2023 at 12:52:40AM +0900, Junio C Hamano wrote:

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > When adding a "break" command to a rebase todo list it can be helpful to
> > add a comment as a reminder as to what the user was planning to do when
> > the rebase stopped. Anything following the command is interpreted as an
> > argument to the command and results in an error. Change this so that a
> > "break command may be followed by "# <comment>" in the same way as
> > a "merge" command. Requiring the comment to begin with "# " allows the
> > break command to start taking an argument in the future if that turns
> > out to be useful.
> 
> Why do we special case "break" and not give the same "comment is
> emitted when the control reaches the insn in the todo list" for
> others like "exec" or even "pick"?

I had somewhat the opposite thought. The "break" command is special in
that it is not doing anything useful except returning control to the
user. And hence producing a message is a useful add-on. So I expected
the patch to just allow:

  break this is a message the user will see

without any "#" at all.

That does close the door for further arguments, but I have trouble
imagining what they would be.

> Another comment with devil's advocate hat on is if we are better off
> not adding "# this comment is emitted" at all, and instead do
> 
>     pick ...
>     pick ...
>     exec echo this comment is emitted
>     break
>     pick ...

Yeah, I have certainly done something like that before. I think it is
just a matter of ergonomics. I'm pretty sure I have also forgotten to
put in the "break" afterwards.

Actually, before we had "break" I think I've done:

  x false this message will be visible when this fails

which is even more horrible. ;)

-Peff

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I had somewhat the opposite thought. The "break" command is special in
> that it is not doing anything useful except returning control to the
> user. And hence producing a message is a useful add-on. So I expected
> the patch to just allow:
>
>   break this is a message the user will see
>
> without any "#" at all.

Ah, I am OK with that, too.

> That does close the door for further arguments, but I have trouble
> imagining what they would be.

Making almost everything that the tool does not pay attention to
(like the patch title of "pick") into comments, floated by Elijah in
the thread, does sound another reasonable direction to go.  If we
are not doing "pick 0f2d4b69 # the title of the commit", adding the
message without "#" to "break" might be a better way to go for
consistency.

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> I had somewhat the opposite thought. The "break" command is special in
>> that it is not doing anything useful except returning control to the
>> user. And hence producing a message is a useful add-on. So I expected
>> the patch to just allow:
>>
>>   break this is a message the user will see
>>
>> without any "#" at all.
>
> Ah, I am OK with that, too.

Then how about this:

  break this is a message the user will see # and this they won't

I'm definitely with Elijah in favor of consistent # usage for comments
that go nowhere.

-- Sergey Organov

Re: [PATCH] rebase -i: allow a comment after a "break" command

[Phillip Wood]

On 13/01/2023 20:22, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> I had somewhat the opposite thought. The "break" command is special in
>> that it is not doing anything useful except returning control to the
>> user. And hence producing a message is a useful add-on. So I expected
>> the patch to just allow:
>>
>>    break this is a message the user will see
>>
>> without any "#" at all.
> 
> Ah, I am OK with that, too.
> 
>> That does close the door for further arguments, but I have trouble
>> imagining what they would be.
> 
> Making almost everything that the tool does not pay attention to
> (like the patch title of "pick") into comments, floated by Elijah in
> the thread, does sound another reasonable direction to go.  If we
> are not doing "pick 0f2d4b69 # the title of the commit", adding the
> message without "#" to "break" might be a better way to go for
> consistency.

Indeed, it seems the question is whether we want to make the changes 
Elijah suggested - if so then we should use a "#" with the "break" 
command as well but if not then it I agree it would be better not have 
have "#" for comments with "break".

My impression is that there is some support for Elijah's suggestion and 
no one has spoken up to oppose it so maybe we should go for that.

Best Wishes

Phillip