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

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