| Message ID | pull.1460.git.1673519809510.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase -i: allow a comment after a "break" command | expand |
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
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
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...).
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...).
"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 ...
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...).
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
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).
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 >
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
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.
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.
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^.
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.
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
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.
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
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^ &&