rebase -i: allow a comment after a "break" command

From: Phillip Wood <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
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(-)

Documentation/git-rebase.txt +0 -0

index f9675bd24e6..511ace43db0 100644

Documentation/git-rebase.txt +0 -0

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

t/t3418-rebase-continue.sh +0 -0

index 130e2f9b553..18d82869b38 100755

t/t3418-rebase-continue.sh +9 -0

+		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

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/[email protected]/#iZ31sequencer.c>
--git a/sequencer.c b/sequencer.c index bcb662e23be..c66f382dfbc
100644 --- a/sequencer.c +++ b/sequencer.c

From: Phillip Wood <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
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(-)

Documentation/git-rebase.txt +0 -0

index f9675bd24e6..511ace43db0 100644

sequencer.c +5 -2

@@ -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);

t/lib-rebase.sh +1 -1

index b57541356bd..a648013f299 100644
@@ -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');;

t/t3418-rebase-continue.sh +16 -0

index 130e2f9b553..18d82869b38 100755
@@ -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

Quoted 37 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
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(-)

Documentation/git-rebase.txt +0 -0

index f9675bd24e6..511ace43db0 100644

Documentation/git-rebase.txt the files and/or the commit message, amend the commit, and continue · line 874

Quoted 7 lines

Documentation/git-rebase.txt +3 -1

@@ -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).

Quoted 64 lines

sequencer.c +5 -2

@@ -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);

t/lib-rebase.sh +1 -1

index b57541356bd..a648013f299 100644
@@ -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');;

t/t3418-rebase-continue.sh +16 -0

index 130e2f9b553..18d82869b38 100755
@@ -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^ &&

Quoted 1 line

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...).

From: Phillip Wood <[email protected]>

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.

Quoted 8 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

Documentation/git-rebase.txt +0 -0

index f9675bd24e6..511ace43db0 100644
@@ -869,7 +869,9 @@ the files and/or the commit message, amend the commit, and continue
  rebasing.
 

Documentation/git-rebase.txt the files and/or the commit message, amend the commit, and continue · line 874

Quoted 5 lines

Documentation/git-rebase.txt +3 -1

  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).

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...).

Quoted 11 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

Quoted 8 lines

Reported-by Olliver Schinagl <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
     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 ...".

Quoted 7 lines

Documentation/git-rebase.txt +0 -0

index f9675bd24e6..511ace43db0 100644
@@ -869,7 +869,9 @@ the files and/or the commit message, amend the 

commit, and continue
  rebasing.

Documentation/git-rebase.txt the files and/or the commit message, amend the commit, and continue · line 874

Quoted 5 lines

  To interrupt the rebase (just like an "edit" command would do, but without

Documentation/git-rebase.txt +3 -1

-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.

Quoted 11 lines

t/t3418-rebase-continue.sh +0 -0

index 130e2f9b553..18d82869b38 100755
@@ -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"

Quoted 5 lines

Quoted 1 line

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).

Quoted 10 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 ...

Quoted 12 lines · Ævar Arnfjörð Bjarmason

Quoted 10 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

Documentation/git-rebase.txt +0 -0

index f9675bd24e6..511ace43db0 100644
@@ -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.

Quoted 8 lines

Documentation/git-rebase.txt the files and/or the commit message, amend the commit, and continue · line 874

Quoted 3 lines

Documentation/git-rebase.txt +3 -0

+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

Quoted 11 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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>".

Quoted 10 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 ...

Quoted 37 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
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(-)

Documentation/git-rebase.txt +0 -0

index f9675bd24e6..511ace43db0 100644

Documentation/git-rebase.txt the files and/or the commit message, amend the commit, and continue · line 874

Quoted 7 lines

Documentation/git-rebase.txt +3 -1

@@ -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).

Quoted 64 lines

sequencer.c +5 -2

@@ -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);

t/lib-rebase.sh +1 -1

index b57541356bd..a648013f299 100644
@@ -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');;

t/t3418-rebase-continue.sh +16 -0

index 130e2f9b553..18d82869b38 100755
@@ -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^ &&

Quoted 1 line

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...).

Quoted 3 lines · Phillip Wood

Quoted 2 lines · Ævar Arnfjörð Bjarmason

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

Quoted 6 lines

Quoted 2 lines

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.

Quoted 5 lines

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).

Quoted 6 lines

Quoted 3 lines

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 '#'

Quoted 3 lines

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).

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").

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.

Quoted 5 lines · Jeff King

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:

Quoted 3 lines

break this is a message the user will see

without any "#" at all.

Ah, I am OK with that, too.

Quoted 6 lines · Elijah Newren

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

Quoted 8 lines

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-<.

Quoted 4 lines

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^.

Quoted 8 lines · Jeff King

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.

Quoted 2 lines

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.